Keep a chat's agent process between its turns - #129
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Groundwork, not yet the behaviour: the callers still make a session per turn, so nothing survives one yet. What exists now is a session that can be asked for a second turn without restarting anything. xd ran claude as "claude -p <prompt>", which answers once and exits. So everything the agent started went with it -- a background shell, a watch, a build -- and the next message met a process that had never heard of any of it. The CLI's --input-format stream-json takes turns on stdin instead and stays up between them. Verified against the real CLI before writing any of this: two turns down one pipe, one process, one session id, both answered. Three things the shape forces: Model, effort, access and working directory are argv, fixed once the process is up. can_continue refuses a turn that changed any of them, and the caller starts a session instead -- which is what it did for every turn before. The result line, not the exit, is what ends a turn now. It is noted and acted on after the line is parsed: one line carries several events, and whoever handles "finished" is entitled to drop the turn underneath the parser. Dispose kills the process even when the turn finished, because "finished" no longer means there is nothing left running. The stand-in CLIs in the remote suite now read stdin before answering, which is the contract the real one has. Cancel is still SIGINT, so stopping ends the process and the next message starts a fresh one -- exactly today's behaviour. The CLI does advertise interrupt_receipt_v1, so an in-band interrupt could keep the process across a stop later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A session belongs to the CLI it was made for, and can_continue was answering without looking at that -- so a chat switched from claude to codex could have found a healthy process that was the wrong one to say anything to, and a codex turn would have gone to claude in claude's own wire format. Only claude streams, so the switch is also what turns persistence off: codex and cerebras take their prompt in argv and end with the turn, exactly as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The daemon now holds the sessions and lends them to turns, so a turn is still the short-lived thing it was and the process answering it is not. Where the CLI can take another turn it is given one; where it cannot -- a changed model, effort, access, working directory or backend -- a new one takes its place in the pool and the old one ends. This is the half that makes the previous commit do something: until now every turn built its own session, so the streaming protocol was in place and nothing outlived a turn regardless. Sessions are owned by the table, so emptying it stops the processes. A deleted chat drops its own, which would otherwise sit waiting for a message that cannot come. The window still runs a session per turn; that caller is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The window now holds its sessions the way the daemon does: a turn borrows the chat's agent and gives it back, so what the agent started is still running when the next message arrives. A turn that the process cannot serve gets its own and replaces it. turn_free had no reason to take its listeners off the session before, because the session died with the turn. It does not any more, and leaving them on pointed a live process at a freed turn. Deleting a chat drops its agent, which would otherwise wait for a message that cannot come. The test is by launch count, not by anything the session reports: a session that quietly restarted between turns would look identical from the outside, and that is the failure worth catching. Writing it turned up the same listener problem in miniature -- one session, two turns, and the first turn's handlers still attached to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reuse test had the stub shell append to a temporary file and then read it back, which meant a Windows path being quoted into a shell script through MSYS. It did not survive the trip, and Windows was the only place that said so. build_argv is called once per spawn and never for a continued turn, so the count is already available in the test's own memory. No file, no path, no shell to quote one through. Two of my own bugs went with it: the printf format was written for the g_strdup_printf that no longer wrapped it, so the stub emitted "%s" rather than a result line; and each run's watchdog outlived the loop it guarded, firing into a Run that had already been cleared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c8b6dd8 to
22c8f9b
Compare
💡 Codex ReviewLines 486 to 488 in c8b6dd8  Restart sessions when agent secrets change
When a user adds, rotates, or removes a global agent secret between Claude turns, this condition reuses the existing process even though secrets were injected only into its launch environment. Subsequent agent commands therefore miss newly added values and retain removed or obsolete credentials indefinitely. Include a snapshot/version of the effective secrets in the launch identity, or evict pooled sessions whenever secrets are saved. Lines 467 to 470 in c8b6dd8  Restart sessions when system instructions change
When folder or project instructions are edited between Claude turns, both the local and daemon turn paths resolve the new system prompt, but this launch comparison ignores it and reuses the old process. Because Claude receives xd/src/ui/branch-build-dialog.c Lines 397 to 401 in c8b6dd8 Terminate the build process tree when stopping
When Stop is pressed during a Docker build, Lines 111 to 115 in c8b6dd8 Treat premature clean exits as failed turns
When a streaming Claude process exits with status 0 before emitting an Lines 380 to 382 in c8b6dd8 Prevent branch builds from racing normal updates
When an update is available or already installing, this independently enabled branch action can start a second installer; likewise, the normal update button remains usable while a branch build runs. Both paths eventually remove and replace the same nightly installation directory, so overlapping them can make either installer fail or leave a mixed/unknown installed result. Coordinate the two operations or disable each installation path while the other is active. xd/src/ui/branch-build-dialog.c Lines 289 to 301 in c8b6dd8 Drain build output before reporting failure
When the build exits after writing its final diagnostics, the wait callback can run before the lower-priority asynchronous stdout reader has drained those lines. This immediately renders the failure tail with incomplete output and sets Lines 308 to 312 in c8b6dd8 Avoid blocking while writing streaming prompts
When a prompt plus handover exceeds the pipe buffer, or when an otherwise live CLI stops consuming stdin, this synchronous Lines 3026 to 3032 in c8b6dd8 Do not retry continued turns as stale resumes
When a reused Claude process reports a per-turn error before producing text, ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Three from the Codex review, all the same shape: something is fixed when the process starts, and reusing the process made a change to it look like it had taken effect when it had not. Agent secrets are the worst of them, because they are environment rather than argv and no argv comparison would ever have caught them. A rotated key leaves every name identical, so the secrets are applied to an empty environment and hashed: a changed value now shows up the same way a changed name does, and the chat gets a process that has it. Instructions were being stored and then not compared. Editing a folder's instructions between turns left the running process on the old ones, silently, for as long as the chat stayed open. And a streaming process that exits cleanly in the middle of a turn was reported as a successful turn, storing whatever partial text had arrived as though it were the whole reply. It exits only after a turn, so a clean exit with one still open is the CLI going away mid-answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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: aaa632ecc3
ℹ️ 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".
| if (self->process != NULL && (self->streaming || !self->finished)) | ||
| g_subprocess_force_exit (self->process); |
There was a problem hiding this comment.
When an idle streaming session is removed because the model/settings changed, its chat was deleted, or the view was closed, this cleanup is never reached: both outstanding read_line_async calls hold a reference to self, and the persistent child keeps those reads pending indefinitely. Consequently, releasing the pool's reference cannot invoke dispose, so the old Claude process and anything it started remain alive; repeatedly replacing sessions accumulates orphaned agents. Add an explicit shutdown operation that cancels the reads and terminates the child before removing a pooled session rather than relying on final unref.
Useful? React with 👍 / 👎.
| gboolean continuing = | ||
| pooled != NULL && xd_chat_session_can_continue (pooled, backend, &spec); |
There was a problem hiding this comment.
After the first Claude response, resume_session_id remains stored, so turn->resumed is true even when this branch continues the already-running process instead of launching with --resume. If such a continued turn returns an error before emitting text or a tool, the existing stale-session recovery path clears a valid session ID and resends the prompt; the pooled process can still pass this continuation check, causing the supposed fresh retry to be written back to the same failed process. Set the resumed flag from whether a new process was actually started with the resume ID, and evict the pooled session when performing a stale-session retry.
Useful? React with 👍 / 👎.
| if (!g_output_stream_write_all (self->stdin_stream, framed, strlen (framed), | ||
| NULL, NULL, error)) | ||
| return FALSE; | ||
|
|
||
| return g_output_stream_flush (self->stdin_stream, NULL, error); |
There was a problem hiding this comment.
When an encoded prompt exceeds the child pipe's available capacity, or the CLI pauses or stops reading stdin, write_all and flush block the application's main loop. A large pasted prompt can therefore freeze the local UI and the remote daemon until the child consumes it; on initial startup, a child that fills stdout before reading stdin can deadlock because stdout reads are not queued until after this function returns. Serialize turns with asynchronous writes or otherwise make the pipe nonblocking instead of performing an unbounded synchronous write.
Useful? React with 👍 / 👎.
| qsort (applied, g_strv_length (applied), sizeof (char *), compare_strings); | ||
| for (gsize i = 0; applied[i] != NULL; i++) | ||
| g_checksum_update (sum, (const guchar *) applied[i], -1); |
There was a problem hiding this comment.
When the configured secrets change between two valid sets whose concatenated environment strings are identical, this fingerprint falsely reports that the process environment is unchanged. For example, {A: "BC=D"} hashes the same byte sequence as {A: "B", C: "D"} because both feed A=BC=D into the checksum, so the old agent is reused with stale credentials and without the newly requested variable. Hash each entry with an unambiguous length prefix or delimiter.
Useful? React with 👍 / 👎.
| const AiRunSpec *spec, | ||
| const char *secrets) | ||
| { | ||
| return g_strcmp0 (self->launched_model, spec->model) == 0 && | ||
| g_strcmp0 (self->launched_workdir, spec->workdir) == 0 && |
There was a problem hiding this comment.
When a daemon or second window answers this chat while its process is idle here, the database's backend last_seen advances but this predicate still reuses the older in-memory process because it compares only launch settings. The next handover is then empty—storage says the backend has already seen the external turn—while the reused process has not actually seen those messages, so it answers from stale conversation state and may overwrite the current session metadata. Track the transcript position represented by each pooled process and reject it when storage has advanced externally.
Useful? React with 👍 / 👎.
Re-does PR #129, whose design was right and whose code was against the C daemon that no longer exists. `claude -p <prompt>` answers once and exits, and everything it started goes with it -- a background shell, a watch, a monitor. That is why a background task started in one turn is dead by the next, and why a monitor armed to report on something never reports. `--input-format stream-json` takes turns on stdin and stays up between them. Checked against the real CLI before any of this was written: two turns down one pipe answered ONE and TWO under one session id, and a `sleep` backgrounded in the first turn answered ALIVE in the second. The turn is still short-lived; the process answering it is not. Output is lent to the turn and handed back when it ends, so the reader still owns it alone. A turn now ends at the result event rather than at end of file -- for a kept process end of file means it died. A process is reused only for a turn that wants exactly what it was started with: model, effort, access, working directory, system prompt and environment are all fixed in argv, so anything else takes a new one. Cancelling kills it, since there is no way to interrupt one turn and leave the process fit for the next, and the next message starts fresh. codex is untouched. `codex exec` reads stdin as the initial prompt only, so there is no equivalent short of its experimental app-server. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
xd ran
claude -p <prompt>, which answers once and exits. Everything the agent started went with it — a background shell, a watch, a build — and the next message met a process that had never heard of any of it.--input-format stream-jsontakes turns on stdin and stays up between them. Verified against the real CLI before any of this was written: two turns down one pipe, one process, one session id, both answered.Shape
backend.hgainsencode_turn. A backend that provides it is kept; one that does not takes its prompt in argv and ends with the turn.codex execreads stdin as the initial prompt only, so there is no equivalent short of the experimentalapp-server.What forces a new process
Model, effort, access, working directory and backend are all argv, fixed once the process is up.
can_continuerefuses those turns and the caller starts a session instead — which is what it did for every turn before, so nothing that worked stops working. Switching model mid-chat behaves exactly as it does today; it just does not get the new benefit on that turn.Three things the shape forced
finishedmay drop the turn underneath the parser.disposemust kill the process even when the turn finished, since "finished" no longer means nothing is running.Verification
22/22 suites. The new test counts launches, not anything the session reports: a session that quietly restarted between turns looks identical from the outside, and that is the failure worth catching. One launch, two turns.
The eight stand-in CLIs in the remote suite now read stdin before answering, which is the contract the real one has.
Not done
Cancel is still SIGINT, so Stop ends the process and the next message starts a fresh one — today's behaviour exactly. The CLI advertises
interrupt_receipt_v1, so an in-band interrupt could later keep the process across a stop, but that control format is not in--helpand I would not guess at it.🤖 Generated with Claude Code