feat: stop foreground execute processes when a user interrupts a chat - #27372
ibetitsmike wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ 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". |
1bd6649 to
a45190d
Compare
7b7cd6d to
2cc3b31
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ 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". |
a45190d to
e1c9876
Compare
2cc3b31 to
c717bbb
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ 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". |
e1c9876 to
7fc8022
Compare
c717bbb to
1191ea5
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ 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". |
1191ea5 to
1e65a02
Compare
b6a36e4 to
4a10e14
Compare
dc2b188 to
7a8997c
Compare
4a10e14 to
2e24ea9
Compare
ibetitsmike
changed the title
|
Implemented cancel-by-identity here rather than in a follow-up, since merging this as it was would ship exactly the gap you named. The stop is now armed before the start request instead of after it. When the start response arrived, it signals the process ID as before. When it did not, chatd still knows the idempotency key, and a new route resolves it: The stored reservation from #27369 does the translation, so process details stay behind the operation that owns them. Covered by tests for the lost-response path (signals by key, no process ID involved), an unidentified call with no key (signals nothing), attempt-watchdog cancellation (signals nothing), background processes spared, 404/409 treated as gone, and the cross-chat rejection on the agent side. I verified the lost-response test fails without the fallback. Explicit background work is untouched, and the stop stays best-effort with no state surviving the agent's lifetime. One thing this surfaced: with conflicts now typed to the start route, the old helper that treated any 409 as "already gone" no longer matched the signal route's 409. That is a separate helper now, so an exited process is still recognized instead of logging a spurious failure.
|
7a8997c to
7a489fb
Compare
2e24ea9 to
ead9530
Compare
|
Follow-up: a self-audit caught a real flaw in how I handled old agents here, now fixed. The stopper treated any 404 from the by-token signal as proof the process was gone. An agent without the route also answers 404, and that answer says nothing about the process, which could still be running after a user interrupt. My comment claiming it "reads as gone" was simply wrong. The route now returns a The limitation is now stated honestly in
|
7a489fb to
d3294e6
Compare
ead9530 to
c3f4d01
Compare
| // Lookup returns the value published for a key, so callers can act on | ||
| // an operation by its identity rather than by whatever handle the | ||
| // operation happened to return. A pending reservation reports false: | ||
| // it has no value yet. |
There was a problem hiding this comment.
What is the difference between a pending and non-existent reservation? I mean, under what situations could it be pending, whilst the model is trying to terminate it?
And the error should probably be different between pending/non-existent in that situation, not that I think it can happen.
There was a problem hiding this comment.
You found a real bug, and it is likelier than "not that I think it can happen". Traced end to end, the old code silently swallowed the interrupt:
Lookupreturned(zero, false)for both an absent key and a pending reservation;signalByTokenmapped both toerrProcessNotFound;- the route answered a coded 404;
- chatd's
isAlreadyGoneErrorread that code as "the process is gone" and returned without logging.
So the kill was dropped and the command kept running, which is the exact failure this PR exists to prevent. The window is not exotic either: the reservation is inserted before the spawn, and the by-key path is used precisely when the start response was lost, so a pending reservation is the expected state there, not a rare interleaving.
Fixed, and the error is now different per your second point:
Registry.Awaitwaits out a pending reservation on the caller's context, reusing thedonechannelReservealready waits on.- Published -> signal the process. Not reserved, or released while waiting ->
ErrNotReserved-> 404process_key_not_found, a genuine not-found. Still pending at the deadline ->ErrPublicationPending-> 409start_pending, which chatd does not classify as gone.
Red-green: removing the wait makes TestSignalByKeyWaitsForPendingStart and TestSignalByKeyReportsPendingAtDeadline fail; restoring it makes them pass. TestExecuteToolInterruptKill/PendingStartIsNotTreatedAsGone covers the chatd classification.
Following that thread found a second one in the same classifier, which I would not have looked for otherwise. The by-key route answered an uncoded 409 for an exited process, the SDK ran every by-key 409 through the start-conflict decoder, and an uncoded conflict is coerced to input_mismatch, so a normal exited process logged a failed kill. There is now a not_running conflict code, and isAlreadyGoneError suppresses only that one. KillByKeyTreatsExitedProcessAsGone fails if the code check is reverted to the old blanket conflict rejection.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| processWaitRetryDelay = time.Second | ||
|
|
||
| // killSignalTimeout bounds the detached best-effort kill sent | ||
| // when a user interrupt unwinds a foreground execute call. |
There was a problem hiding this comment.
Deleted. The constant name and its value say everything the comment did.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| // process only on this cause: every other cancellation (attempt | ||
| // watchdog timeout, worker shutdown, runner rebalancing) leaves the | ||
| // process running so a replay can re-attach through its idempotency | ||
| // token. |
There was a problem hiding this comment.
Cut from seven lines to three:
ErrUserInterrupt is the cancellation cause chatd uses when a user action supersedes the running turn. Foreground execute kills its process only for this cause.
The enumeration of every other cancellation source went with it; that belongs in the architecture doc, where it already is.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| } | ||
| } | ||
|
|
||
| // interruptStopper kills a foreground process when a user action |
There was a problem hiding this comment.
This naming feels bad. Turn gets interrupted so it stops the process, but this reads like it stops interrupts.
There was a problem hiding this comment.
Agreed, it read backwards. Renamed to userInterruptStopper: it stops the process, and it does so only on a user interrupt.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| logger slog.Logger | ||
| clientToken string | ||
| processID string | ||
| disarmed bool |
There was a problem hiding this comment.
Consider using a mutex, or are you certain parallel read/writes are impossible?
There was a problem hiding this comment.
I am fairly confident parallel access is impossible here, so I would rather not add a mutex that suggests otherwise. All four call sites are in executeForeground on the one goroutine running the tool call:
defer func() { stopper.stop(ctx) }()immediately after construction;stopper.disarm()on the start-conflict path;stopper.observeStart(resp.ID)after a successful start;stopper.disarm()after the wait resolves.
Context cancellation touches no stopper field: stop only reads context.Cause(ctx), and it runs in the deferred call on the same goroutine, after every other method has returned. Nothing hands the stopper to another goroutine.
That said, the long comment asserting single-goroutine access is what prompted your question, so I shortened it rather than deleting it. If you would still prefer the mutex as a guard against a future caller, say so and I will add it.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
|
|
||
| ### Interrupt kill | ||
|
|
||
| When a user action supersedes a running turn (an interrupt, an edit, or a message sent with `busy_behavior=interrupt`), the runner cancels the active task with the `chattool.ErrUserInterrupt` cause. A foreground `execute` call unwinding on that cause sends a best-effort kill signal on a detached, bounded context: the user asked for the work to stop, and the command would otherwise keep running behind a result nobody reads. A 404 or 409 answer means the process is already gone. Background processes are deliberately spared; they are addressable through their handle and the process list. |
There was a problem hiding this comment.
"keep running behind a result nobody reads".
Rewrite everything!
There was a problem hiding this comment.
Rewritten:
the command would otherwise keep running after the turn result is discarded
The surrounding section changed materially as well, since the by-key route moved to a request body and now waits out a pending reservation instead of reporting it as not-found.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| DeleteDevcontainer(ctx context.Context, devcontainerID string) error | ||
| RecreateDevcontainer(ctx context.Context, devcontainerID string) (codersdk.Response, error) | ||
| SignalProcess(ctx context.Context, id string, signal string) error | ||
| SignalProcessByToken(ctx context.Context, clientToken string, signal string) error |
There was a problem hiding this comment.
Be sure to update clientToken when/if renamed throughout the stack.
There was a problem hiding this comment.
Done in the same revision. ClientToken is IdempotencyKey everywhere in the stack, including the wire tag idempotency_key, SignalProcessByIdempotencyKey on AgentConn, and the route, which is now POST /api/v0/processes/signal-by-idempotency-key with the key in the body.
git grep -i clienttoken and git grep client_token are both empty across the stack.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
c3f4d01 to
7c5cfc1
Compare
202fa50 to
a3f2866
Compare
7c5cfc1 to
6a84991
Compare
|
Revised, and re-parented onto #27370. Re-parenting This PR no longer sits on #27371. You rejected that PR's premise twice, so it should not gate the two run-once PRs below it. The dependency turned out to be prose only: three mentions of "attempt watchdog", now "attempt timeout". #27371 is parked as a draft and its six items are answered there. Your pending-reservation question was a live bug
The window is not exotic: the reservation is inserted before the spawn, and the by-key path is used precisely when the start response was lost, so a pending reservation is the expected state there.
Pulling that thread found a second bug in the same classifier: the by-key route answered an uncoded 409 for an exited process, the SDK ran every by-key 409 through the start-conflict decoder, and an uncoded conflict is coerced to Both fixes are red-green: reverting either makes its test fail and nothing else. Route change
Other items
One pushback I did not add the mutex on the stopper. All four call sites are in
|
A user action that replaces the current turn should stop unresolved foreground work, while an internal retry or worker move should leave it running so the next attempt can recover it. The runner now cancels the active task with an explicit cause (chattool.ErrUserInterrupt) when a user interrupt, an edit, or a message sent with busy_behavior=interrupt supersedes it. A foreground execute call unwinding on that cause stops its process on a detached, bounded context. Runner shutdown and rebalancing cancel without the cause, so their processes keep running and a retried attempt re-attaches instead of running the command twice. Background processes are deliberately spared. The stop is armed before the start request rather than after it, which closes the case where the agent started the process but the response never reached chatd. There is no process ID to signal then, but the idempotency key is still known, so the agent resolves the key to the process it started through a new by-token signal route. The route is chat-scoped and answers 404 for a foreign or unknown key rather than disclosing it. The stop stays best-effort: an unreachable agent or a worker crash before unwinding leaves the process running, still visible and killable through the process list.
6a84991 to
a8c96d4
Compare
a3f2866 to
0599aa1
Compare
2 participants
Stack context
Now based on #27370, not #27371. #27371 is parked as a draft while its timeout policy is reworked; this PR never depended on it beyond three prose mentions of "attempt watchdog", which are now "attempt timeout".
Spans
agent/,codersdk/, andcoderd/x/chatd/, so the title is scopeless.What
A user action that replaces the current turn stops unresolved foreground work; an internal retry or worker move leaves it running so the next attempt can recover it.
The runner cancels the active task with an explicit cause (
chattool.ErrUserInterrupt) when a user interrupt, an edit, or a message sent withbusy_behavior=interruptsupersedes it. A foregroundexecutecall unwinding on that cause stops its process on a detached, bounded context. Runner shutdown, rebalancing, and attempt timeouts cancel without that cause, so their processes keep running and a retried attempt re-attaches through its idempotency key instead of running the command twice. Background processes are deliberately spared; they are addressable through their handle and the process list.Cancelling by identity, not by process ID
Requiring the process ID from
StartProcessleaves nothing to cancel when the agent starts the process but the response does not reach chatd.The stop is armed before the start request rather than after it. When the response did arrive, the process ID is used. When it did not, chatd still knows the idempotency key, and a chat-scoped route resolves that key to the process it started:
The key travels in the body rather than the path. It carries a provider-supplied tool call ID, and chi's path handling makes an opaque path segment fiddly: chi routes on
RawPathwhen it is set and onPathotherwise, sochi.URLParamreturns an escaped value in the first case and a decoded one in the second. A key containing a literal%lands in the second case, whereurl.PathUnescapethen fails withinvalid URL escape. Keeping the key out of the path removes that class of bug rather than handling it.TestSignalProcessByIdempotencyKey/AcceptsKeysNeedingURLEscapingcovers/,%, a space,?,#, and...The stored reservation does the translation, so process details stay behind the operation that owns them. Reservations are chat-scoped after #27369, so another chat's key is simply absent here rather than needing a separate ownership check.
SignalProcessByIdempotencyKeyonAgentConnis the client side. Agents that predate the route answer a plain 404, which the SDK reports asErrProcessKeySignalUnsupportedrather than "not found", because such an answer says nothing about the process.Fix: a pending reservation no longer swallows the interrupt
Review asked what distinguishes a pending reservation from a missing one on this path. Traced end to end, the old code lost the kill:
falsefor both an absent key and a pending one;errProcessNotFound;isAlreadyGoneErrorread that code as "the process is gone" and returned without logging.The interrupt was dropped silently and the command kept running, which is what this PR exists to prevent. The window is not exotic: the reservation is inserted before the spawn, and the by-key path is used precisely when the start response was lost, so a pending reservation is the expected state there.
The registry now exposes
Await, which waits out a pending reservation inside the caller's context and distinguishes three outcomes:process_key_not_found. A genuine not-found.start_pending. chatd logs it instead of treating the command as gone.Red-green: removing the wait makes
TestSignalByKeyWaitsForPendingStartandTestSignalByKeyReportsPendingAtDeadlinefail, and restoring it makes them pass.TestExecuteToolInterruptKill/PendingStartIsNotTreatedAsGonecovers the chatd side.Other review changes in this revision
interruptStopperis nowuserInterruptStopper: the old name read as stopping interrupts.IdempotencyKeyrename.decodeSignalRequestbecamevalidateSignal, since the two routes decode different bodies.ARCHITECTURE.mdwas rewritten.Limitations
The stop stays best-effort. An unreachable agent, or a worker crash before unwinding, leaves the process running; it remains visible and killable through the process list. No state needs to survive the workspace agent's lifetime.