Repository navigation
Conversation
Adds test/support (compiled in the test env only) with a small GenServer-based WebSocket server standing in for the Sprites exec and control endpoints, an ExUnit case template with helpers to spawn against it, and tests for the server itself. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Splits Command.build_control_args/4 into small put_control_* steps, matching the add_*_param helpers in Sprite. No behaviour change; adds end-to-end tests for direct and control-mode exec requests covering the existing options. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Exposes the exec endpoint's max_run_after_disconnect query param as a duration string (e.g. "10s"). Direct mode sends it on spawn only; control mode sends it in the op.start args under the same key. Omitted when unset, so default behaviour is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75a8fc5b11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Map.put(control_args, "stdin", if(stdin, do: "true", else: "false")) | ||
| defp put_control_max_run_after_disconnect(control_args, opts) do | ||
| case Sprite.max_run_after_disconnect(opts) do |
There was a problem hiding this comment.
When control mode is selected and a non-string value is passed, Control.checkout/1 has already marked conn_pid busy before this validation raises. The init process then exits before ControlConn.start_op/4 and never executes Control.checkin/2, so the still-live connection remains marked busy; repeated rejected spawns consume sockets and can fill the 100-slot pool, forcing later valid control commands down the direct fallback. Validate before checkout or check the connection back in on this failure path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 0fd982c: the option is now validated in Command.start/4 before the GenServer starts, so no checkout happens and callers get an ArgumentError directly; the test asserts neither transport is contacted.
| upgrade(socket, headers, state.owner) | ||
|
|
||
| for frame <- state.frames, | ||
| do: :ok = :gen_tcp.send(socket, [<<0x82, byte_size(frame)>>, frame]) |
There was a problem hiding this comment.
When a configured server-to-client frame is 126 bytes or larger, this writes its raw size into the one-byte short-length header. Values 126 and 127 are sentinels requiring subsequent extended-length fields, while larger values also misuse or overflow that header, so gun parses payload bytes as framing instead of delivering the intended output. Encode the 16-bit and 64-bit WebSocket length forms before sending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0398e8d: frames now use the 16-bit/64-bit length forms, and the server test sends 300-byte and 70,000-byte frames through gun.
| @@ -0,0 +1,78 @@ | |||
| defmodule Sprites.Test.ExecCase do | |||
There was a problem hiding this comment.
Added some tests for all options
| @@ -0,0 +1,144 @@ | |||
| defmodule Sprites.Test.FakeWsServer do | |||
There was a problem hiding this comment.
TBH this was claude coded - I did add some tests for it and as it was test code, I thought it was better to add more coverage than not
Server-to-client frames of 126+ bytes used the one-byte length field, which gun parsed as framing. Use the 16-bit and 64-bit length forms and cover both in the server's tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
In control mode the option was validated after Control.checkout/1, so a bad value crashed init with the connection still checked out. Validate in Command.start/4 instead, so callers get an ArgumentError and no connection is touched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ect (superfly#41) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ect (superfly#41) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ect (superfly#41) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
aezell
left a comment
aezell
left a comment
There was a problem hiding this comment.
@gkpacker The change looks good. There's a small nuance that might be good to make in the documentation.
The new docs say omitting max_run_after_disconnect allows commands to run indefinitely after disconnect, but the server defaults non-TTY commands to 10 seconds; only TTY commands default to unlimited runtime. Since tty: false is the SDK default, could you update both cmd/4 and spawn/4 docs to distinguish these defaults?
|
Sure, on it @aezell! |
Non-TTY commands (the SDK default) stop 10 seconds after disconnect; only TTY commands run with no time limit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@aezell done - my claude also decided to answer for me 🤣 |
Why
The exec WebSocket (
/v1/sprites/{name}/exec) accepts amax_run_after_disconnectquery param. The API docs mention it ("Commands continue running after disconnect; usemax_run_after_disconnectto control timeout"), but no SDK exposes it. Without it, a spawned command keeps running indefinitely after the client disconnects, even after a clean close. Checked against the live API on 2026-10-16:bashTTY session was still listed inGET /v1/sprites/{name}/execandpsmore than 5 minutes after the socket closed.max_run_after_disconnect=10swith an abrupt socket drop: a background loop stopped about 9s later and the session left the list.What changed
:max_run_after_disconnectexec option, a duration string such as"10s"or"5m". Omitted by default, so current behaviour (the command keeps running, no time limit) is unchanged.op.startexec args under the same key, so both transports behave the same.Sprites.cmd/4,Sprites.spawn/4, theSprites.Commandmoduledoc and the README, e.g.Sprites.spawn(sprite, "bash", [], tty: true, max_run_after_disconnect: "10s").test/support/fake_ws_server.ex(a small GenServer-based WebSocket server, with its own tests) and anExecCasetemplate, plus end-to-end tests forspawn,cmd,attach_sessionand control-mode exec covering existing options as well as the new one.mix.exsgainselixirc_pathsfor the test env.Only strings are accepted for now. Integer milliseconds can be added once the server is confirmed to accept an
mssuffix.One in roughly 50 local runs hit a pre-existing race in
Control.ensure_table_owner(concurrent first callers racing to create the shared ETS tables); it also exists onmainand is left alone here.Note
sprites-go doesn't expose this option either: neither its exec query builder (
exec.go) nor its controlop.startargs (websocket.go) setmax_run_after_disconnect.🤖 Generated with Claude Code