feat: add a run-once reservation mechanism to the workspace agent process API - #27369
ibetitsmike wants to merge 1 commit into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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". |
d3be23b to
c6f4fb5
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, 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". |
c6f4fb5 to
d9b87ba
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
I think the atomic reservation in this PR is the right core idea, but it is being added at the wrong level.
The reusable rule is not specific to process starts:
For selected mutating tool calls, the workspace agent should perform one operation for a given ID and return the same work or result when that ID is repeated.
This stack does not need to convert every tool. It should add that shared mechanism and use execute as its first and only operation.
For example, the agent could keep an in-memory record containing:
tool-call ID
operation name
accepted input
running or completed state
result
A new ID reserves the record before doing any work. Repeating the same ID and input returns the existing state or result. Reusing the ID with different input fails.
execute would use this mechanism to start one process and retain its process ID. The existing process manager would continue to own process output, exit state, and signals.
This split keeps the useful work in this PR, especially reservation before cmd.Start and concurrent callers waiting for the owner. It also avoids embedding the shared replay rule inside process management, where another mutating tool could not reuse it later.
The scope can remain small: build the shared mechanism, connect execute, and leave every other tool unchanged.
🤖 This review was automatically generated with Coder Agents.
f3c8095 to
3dfed86
Compare
ibetitsmike
changed the title
|
Agreed, and rebuilt this PR around it. The reservation rule now lives in its own mechanism, I did diverge from the record shape you sketched in one way, and want to flag it rather than bury it. The record carries operation name, key, accepted input fingerprint, pending/published/completed state, and a result slot, but the registry is generic over that result and never interprets it. Forcing a common result type is where the abstraction would have broken:
Two things worth a look:
Conflicts are machine-readable now, per your #27370 comment: One correction to the wording here: this comment says "tool-call ID", and a bare provider tool-call ID is not stable across regenerations. Your #27370 comment gets it right, and the composed key is what landed.
|
3dfed86 to
6c58dd6
Compare
|
Follow-up: a self-audit of this branch found a duplicate-execution race in the reservation recovery path, now fixed. A reservation can legitimately outlive the process it names for a moment:
Reservations now carry a generation, Also in this branch: dropped
|
6c58dd6 to
7bedd8a
Compare
| requireConflictCode(t, w2, workspacesdk.ProcessConflictInputMismatch) | ||
| }) | ||
|
|
||
| t.Run("SameTokenDifferentChatsConflicts", func(t *testing.T) { |
There was a problem hiding this comment.
Why should this conflict? Don't see a reason for two different chats to operate in the same "space", which would inherently allow this case.
There was a problem hiding this comment.
Agreed, and this test now asserts the opposite. Your composite-key suggestion below made the conflict unnecessary: reservations are keyed by {chatID, idempotencyKey}, so two chats using the same key value each get their own reservation.
SameKeyDifferentChatsStartIndependently now starts under chat A and chat B with the same key and asserts two distinct process IDs. TestSameKeyInDifferentChatsStartsIndependently covers the same thing at the manager level while chat A's start is still pending, which was the case that used to fail fast with a mismatch.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| // started under an idempotency token. It bounds how long the | ||
| // token keeps deduplicating starts (and serving the exited | ||
| // result) after the process exits. | ||
| tokenedProcessReapAge = 60 * time.Minute |
There was a problem hiding this comment.
Just checking, this does not prevent collection process results after, say, 2 hours? I could very well see a situation where a process is started and then go on to wait 2 hours for a sub-agent to produce results, only to then want to know exit status and output.
Just thinking out loud, but rather than storing N entries for 60 minutes, how about just storing the last N tokens/results for one specific chat, irrespective of time?
Time based constructs are always tricky and can result in unexpected bugs or issues.
There was a problem hiding this comment.
Collection after 2 hours already works, and I should have said so in the comment rather than leaving you to check. Retention starts when the process exits, not when it starts (m.runOnce.Complete is called from the cmd.Wait goroutine). A process still running after two hours is never reaped, so the sub-agent case you describe returns the real exit status and output.
The window bounds how long an exited result stays retrievable. For reference, the pre-existing untokened reap age is 5 minutes, so this stack already extends result retention 12x for keyed processes.
On last-N-per-chat instead of time: I do not think it is smaller. A per-chat count is unbounded across unbounded chats, so it needs a global bound too, and then an eviction order across chats. That turns into a process-history retention policy rather than a swap of one rule for another. I would rather keep the simple rule here and do the count-based cache as its own change if you want it. Happy to file it.
I did tighten the comment to state the contract instead of implying a general cap.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| // The workdir is taken as requested rather than resolved, because the | ||
| // default directory can resolve differently across retries (before | ||
| // and after the agent manifest loads), and an identical retry must | ||
| // attach instead of conflicting. |
There was a problem hiding this comment.
I think the logic of using request workdir is sound, but the comment is a bit out there. Technically it shouldn't be possible to reach here before manifest has been loaded.
There was a problem hiding this comment.
Dropped the manifest claim. You are right that it should not be reachable, and I could not construct a case where it is, so asserting it was speculation.
The comment now says only what is verifiable: the workdir is taken as requested rather than resolved, so an identical retry attaches even when the default directory resolves differently.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| // agentrunonce.ErrInputMismatch. | ||
| // | ||
| // ctx bounds only the wait for a concurrent start that owns the | ||
| // same token; the spawned process itself is never tied to it. |
There was a problem hiding this comment.
I'd prefer to see this re-written in English. This way of writing is very LLM, very metaphorical.
There was a problem hiding this comment.
Rewritten:
The context only limits how long a duplicate start waits for the concurrent start holding the same key. It does not cancel the spawned process.
I also did a pass over every comment this stack adds, not just the ones you flagged. Added comments in the four core files went from 44-53% comment:code down to roughly 30-40%, against a repo norm near 20% in those same files. Details in the summary comment.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
|
|
||
| // The registry is keyed by the bare client token; chat identity | ||
| // is part of the fingerprint, so cross-chat reuse of a token | ||
| // conflicts instead of attaching. |
There was a problem hiding this comment.
This comment is misplaced. Don't comment about fingerprint behavior on top of token, leads to misunderstandings.
There was a problem hiding this comment.
Deleted rather than moved. The composite key removed the behavior it described: chat is no longer part of the fingerprint, so there is nothing about cross-chat fingerprint matching to explain here.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| done chan struct{} | ||
| // generation distinguishes successive reservations of one key, so | ||
| // a caller acting on a stale observation cannot disturb the | ||
| // reservation that replaced it. |
There was a problem hiding this comment.
Metaphorical, not comprehensible
There was a problem hiding this comment.
Rewritten:
Forgetuses it to avoid deleting a replacement reservation.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| r.mu.Lock() | ||
| existing, ok := r.entries[key] | ||
| if !ok { | ||
| if r.closed { | ||
| r.mu.Unlock() | ||
| return zero, ErrClosed | ||
| } | ||
| // Insert the reservation before returning so a | ||
| // concurrent caller waits for this owner rather than | ||
| // performing the operation a second time. | ||
| r.generation++ | ||
| reserved := &entry[V]{ | ||
| fingerprint: fingerprint, | ||
| done: make(chan struct{}), | ||
| generation: r.generation, | ||
| } | ||
| r.entries[key] = reserved | ||
| r.mu.Unlock() | ||
| return Outcome[V]{ | ||
| Owner: &Ticket[V]{registry: r, key: key, entry: reserved}, | ||
| Generation: reserved.generation, | ||
| }, nil | ||
| } | ||
| // The fingerprint is recorded when the key is reserved, so a | ||
| // mismatched input is rejected without first waiting out an | ||
| // owner it could never attach to. | ||
| if existing.fingerprint != fingerprint { | ||
| r.mu.Unlock() | ||
| return zero, ErrInputMismatch | ||
| } | ||
| if existing.published { | ||
| outcome := Outcome[V]{Value: existing.value, Generation: existing.generation} | ||
| r.mu.Unlock() | ||
| return outcome, nil | ||
| } | ||
| r.mu.Unlock() |
There was a problem hiding this comment.
This belongs in a method to avoid the risky mutex handling.
There was a problem hiding this comment.
Done. Reserve is now a loop over two methods that each own their locking:
tryReservetakes the lock withdefer Unlockand returns either the resolved outcome or the pending entry to wait on;waitForPublicationdoes the blocking select and is documented as requiring the lock not be held.
No method unlocks a mutex it did not take.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| // request attaches to the process the first request spawned | ||
| // instead of spawning a duplicate. It holds process IDs; the | ||
| // manager remains the owner of process state itself. | ||
| runOnce *agentrunonce.Registry[string] |
There was a problem hiding this comment.
Suggestion:
type runonceKey struct { chatID uuid.UUID; token string; }
This would avoid risking cross-chat pollution and does not conflict with same-token different-chat.
There was a problem hiding this comment.
Taking this. It is better than both my fail-closed conflict and the earlier "just return the existing process" suggestion, and it cascaded further than I expected:
- The registry is now generic over the key type, and
agentprockeys it withrunOnceKey{chatID, key}. chatIDis out ofstartFingerprint: chat is the namespace now, so digesting it too was redundant.chatIDis out of chatd's key derivation entirely (feat(coderd/x/chatd): identify tool calls in the dispatcher and dedupe execute replays #27370), which is also what you asked for there.- The
proc.chatID != chatIDcheck in the by-key signal path (feat: stop foreground execute processes when a user interrupts a chat #27372) is gone. The key owns chat isolation; a second check invites the question of why it is there. - The cross-chat conflict test became a cross-chat independence test.
This reverses my position from the last round, where I argued chat belonged in the fingerprint. You found a third option that keeps the isolation without the false conflict.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| // ClientToken makes the start idempotent: a repeated token | ||
| // attaches to the process it already started. Reuse with | ||
| // different parameters is refused. | ||
| ClientToken string `json:"client_token,omitempty"` |
There was a problem hiding this comment.
This token is all about idempotency, but is then named something generic like "client" that does not IMO convey what it is or what it does.
I'd like to see a better name for this. It doesn't necessarily have to be anything about idempotency, but something better than ClientToken that suggest you can use the same value multiple times.
There was a problem hiding this comment.
Renamed to IdempotencyKey, including the JSON tag (client_token -> idempotency_key) and the derived names throughout the stack (tokenKey -> runOnceKey, tokenedProcessReapAge -> keyedProcessReapAge, SignalProcessByToken -> SignalProcessByIdempotencyKey).
No compatibility cost: git grep client_token at the merge-base returns nothing, so nothing outside this stack ever saw the old name.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| // ProcessConflictCode identifies why a start request lost to an | ||
| // existing client-token reservation. It travels in the response body | ||
| // so callers can branch on the kind of conflict without matching | ||
| // error text. |
There was a problem hiding this comment.
"travels in the response body" 😔. Honestly starting to space on these comments now so won't review them further down but I have no doubts about them being poor 😂.
There was a problem hiding this comment.
Dropped that phrase; the doc comment now states only the branching purpose.
I took the wider point too, since you stopped flagging partway through and never got to #27370 or #27372. I measured the whole stack rather than fixing the eleven you named: added comments were 44-53% of added code in the four core files, against roughly 20% in the same files at base. I went through every added comment against a keep-only-if-it-explains-a-non-obvious-invariant rule. The four core files are now roughly 30-40%, and the same pass covered #27370 and #27372. Summary comment has the per-file numbers.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
121599a to
3f1ac82
Compare
|
Revised. Your composite-key suggestion turned out to be the largest change here, and it cascaded into #27370 and #27372 as well. Design Registry keys are now Naming and structure
Comments You stopped flagging partway through PR1 and never reached #27370 or #27372, so I measured the whole stack instead of fixing only the eleven you named. Added comments were 44-53% of added code in the four core files, against roughly 20% in the same files at base:
One pushback I kept the 60-minute retention rather than switching to last-N-per-chat, reasoning in the thread. Short version: retention starts at process exit, so your 2-hour case already works, and a per-chat count needs a global bound plus a cross-chat eviction order to be safe. Happy to do that as its own change if you still want it.
|
…cess API Adds agent/agentrunonce, a keyed reservation registry that makes a repeated operation run once: the first caller to reserve a key performs the work, later callers presenting the same input attach to the value it published. The registry never interprets that value, so operations whose results have nothing in common can share the rule. Process starts are its first adapter; the process manager keeps owning output, exit state, signals, and git-watcher registration. Process starts pass a client token as the key and a digest of the fields that decide what gets spawned (command, requested workdir, env, background, chat) as the input fingerprint. Recording the fingerprint when the key is reserved lets a mismatched retry fail immediately rather than first waiting out a start it could never attach to. Start conflicts now carry a machine-readable code so callers do not have to match error text: input_mismatch is permanent, start_pending means the owning start had not published a process yet and a retry may still attach.
3f1ac82 to
22dc279
Compare
|
@mafredri The stale bot closed all four PRs in this stack while they sat idle; they are reopened, and the whole stack is now rebased onto current main (about 520 commits of drift). Rebase notes:
When you have time, another pass over the stack would be appreciated. All 31 of your earlier threads have replies, including the two where I pushed back with evidence and left the final call to you.
|
2 participants
Stack context
Part 1 of the CODAGT-757 stack. Replaces the earlier "idempotent start tokens" version of this PR after review: the reservation rule now lives in its own mechanism rather than inside process management.
Why
A chat that resumes after a crash replays tool calls history still shows as unresolved. For
executethat means running the shell command a second time. Preventing it needs a rule the agent can apply to a repeated request: perform the operation once, and answer a repeat with what the first attempt produced.That rule is not specific to starting processes.
edit_filesis the clearest second candidate: replaying it is actively harmful, because the text it searched for has already been replaced, so the retry either fails or matches a changed file differently.What
agent/agentrunonceholds the rule and nothing else:The registry is generic over both the key and the published value, and never interprets either. That is what lets operations with unlike results share the rule: this PR's adapter publishes a handle to a live process that the process manager keeps managing, while a future
edit_filesadapter would publish a plain response value. Both fake adapters are covered in the registry's own tests.Process starts are the only adapter here. The process manager still owns output buffers, exit state, signals, and git-watcher registration.
Reservations are scoped to the chat
Following review, the registry key is a composite of the chat and the idempotency key, so two chats that send the same key value get independent reservations instead of colliding. Chat is therefore no longer part of the fingerprint, and cross-chat reuse no longer produces a false conflict.
The fingerprint covers only what decides what gets spawned: command, requested workdir, env, and the background flag. Display intent and how long the caller waits are excluded, since neither changes the process.
Two behavioral notes:
Machine-readable conflicts
Start conflicts carry a code, so callers do not classify them by matching error text:
input_mismatchstart_pendingThe code rides a route-specific
ProcessConflictErrorrather than a new field on the globalcodersdk.Response, following the existingWatchErrorprecedent. Agents that predate this never answered 409 here, so an uncoded conflict is reported as permanent: treating an unresolved start as permanent costs an error result, while the reverse risks running the command twice.Review changes in this revision
{chatID, idempotencyKey}composites; the cross-chat conflict test became a cross-chat independence test.ClientTokenis nowIdempotencyKeyon the wire (idempotency_key) and in Go. Noclient_tokenreference existed anywhere at merge-base, so there is no compatibility cost.ErrOwnerPendingis nowErrPublicationPending, andOutcome.Owneris nowOutcome.Ticket, so the API no longer uses "owner" for a concept the reviewer found unclear.Reservemoved intotryReservepluswaitForPublication, so no method unlocks a mutex it did not take.process_internal_test.gowas rewritten around shared helpers (newTestManager,startGate,startAsync,awaitStart), and the two close-before-spawn tests collapsed into one table.