fix(runtime): reliably surface & correlate concurrent elicitation requests (#3584)#3587
Draft
aheritier wants to merge 7 commits into
Draft
fix(runtime): reliably surface & correlate concurrent elicitation requests (#3584)#3587aheritier wants to merge 7 commits into
aheritier wants to merge 7 commits into
Conversation
…nd non-blocking (#3584) Resolves blocking review findings 1, 2, 4, 5 and should-fix items 1, 2 from the A6 concurrency review: 1. Reliable sink no longer gated behind the blocking bridge: elicitationHandler now calls the OnElicitationRequest sink synchronously and unconditionally BEFORE the best-effort bridge send, and the bridge send itself runs on a detached goroutine bounded by ctx (elicitationBridge.send takes a context and selects on ctx.Done()), so a full/abandoned bridge channel can never delay or block sink delivery or response processing. 2. Waiter registry: (a) the registry key is now always a freshly generated internal UUID, never the MCP wire ElicitationID, so two MCP servers reusing the same wire ID can no longer evict each other's waiter; (b) resolve-vs-cancel is now decided by an atomic terminal state (elicitationWaiter.state), so a response ResumeElicitation already reported as delivered can never be silently discarded by a racing ctx.Done(). 4. Supervisor: ElicitationRequestEvent now carries the originating (sub-)session ID, and the supervisor's PendingEvents queue filters on it so a foreground stream stop/start only clears its OWN attention events, preserving a still-live detached background job's elicitation. 5. runCollecting no longer re-forwards a bridge-observed ElicitationRequestEvent to the sink (elicitationHandler is now the sole, exactly-once caller), and the App-side ElicitationID dedupe map is removed since it is no longer needed and could itself drop a legitimate delivery. Should-fix 1 (Go API compatibility): Runtime.ResumeElicitation's new elicitationID parameter is variadic, so pre-#3584 3-argument call sites keep compiling; OnElicitationRequest remains a required (no-op-able) interface method, matching the existing OnToolsChanged/OnBackgroundEvent precedent. Documented in docs/guides/go-sdk. Should-fix 2 (deterministic replay): replayPendingEvent now returns tea.Sequence instead of tea.Batch, so bubbletea replays queued attention events (e.g. two concurrent background-job elicitations) in the order they arrived instead of a Batch's concurrent, unordered execution. This is a large, cohesive commit because these fixes are structurally entangled: the elicitation request/response types, the runtime.Runtime interface, and App/supervisor/TUI wiring (plus their test doubles) all change together, and the repo must build as a whole at every commit. Refs #3584
…runtimes (#3584) Resolves blocking review finding 3: API/server-created runtimes (runtimeForSession) never registered an OnElicitationRequest sink — only pkg/app (the TUI) did — so elicitationHandler's headless fast-decline path ("no sink means no UI") fired for every background-job elicitation raised through the API, even though RemoteRuntime.OnElicitationRequest's documented contract promises a remote/SSE client CAN answer them. runtimeForSession now registers sessionElicitationSink on every runtime it builds. The sink appends the event to a lazily-created, session-scoped event log (ensureEventLog/appendSessionEvent), so: - hasElicitationSink() is true for every API/server session, opting background elicitations back into the normal wait-for-response path instead of auto-declining; - the request becomes replayable via GET /api/sessions/:id/events even for a session that was never attached via RegisterEventSource; - it stays answerable via the existing POST .../elicitation route. Refs #3584
- docs/guides/go-sdk: document that Runtime.ResumeElicitation gained a variadic elicitationID parameter and that OnElicitationRequest is a new required (no-op-able) interface method, for anyone implementing their own runtime.Runtime per the Go SDK guide (should-fix: Go API compatibility). - docs/features/api-server: note that GET /api/sessions/:id/events is now also available once a session has raised an out-of-band event (e.g. a background job's elicitation), not only for a --listen-attached run (finding 3). Refs #3584
… runtime TestRuntimeForSession_RegistersSessionScopedElicitationSink constructed the runtime but then invoked a freshly created sm.sessionElicitationSink(...) directly instead of exercising the sink actually registered on the runtime runtimeForSession returned, so deleting the real run.OnElicitationRequest(...) registration in runtimeForSession still passed. Add LocalRuntime.EmitElicitationRequestForTesting, a seam that invokes whatever sink is currently registered (exactly as elicitationHandler would, without the real MCP handshake), and rewrite the test to drive it through the concrete *runtime.LocalRuntime the session manager actually built. Verified this now fails when the registration line in runtimeForSession is removed (#3584 re-review should-fix 1).
drainInOrder recognized a sequenceMsg (tea.Sequence's ordered replay marker) purely by its []tea.Cmd shape, which tea.BatchMsg (tea.Batch's CONCURRENT, unordered marker) shares — so TestReplayPendingEvent_ReplaysConcurrentElicitationsInFIFOOrder kept passing even if replayPendingEvent's tea.Sequence were reverted to tea.Batch, silently defeating the regression test it exists for. Reject tea.BatchMsg outright and only recurse into a value whose concrete type is bubbletea's private sequenceMsg. Verified this now fails when production is temporarily reverted to tea.Batch (#3584 re-review should-fix 2).
#3584 re-review doc nits: - 'raised via POST .../elicitation' described the endpoint that RESPONDS to an elicitation as the one that raises it; reworded to 'answered via'. - The events endpoint's docs still said it only applied to a run attached via --listen; it now also applies to any API-created session once it has raised an out-of-band event (#3584 review item 3), so both the table row and the dedicated section were updated to say so. - firstElicitationID's comment pointed at a CHANGELOG.md compatibility note that doesn't exist (this repo's CHANGELOG.md is generated per release from merged PRs, not hand-edited alongside a change); it now points solely at the docs/guides/go-sdk/index.md note that does exist, with a brief explanation of why CHANGELOG.md carries none pre-release.
…elete ensureEventLog creates a bare, API-only event log (no pump goroutine) the first time a session raises an out-of-band event without ever being attached via RegisterEventSource — the common case for a background job's elicitation on an API/server-created session (#3584 review item 3). It handed that log a NO-OP cancel function. DeleteSession (and BatchDeleteSessions) call pe.cancel() unconditionally and drop the registry entry, but a client already connected to GET /api/sessions/:id/events for that session kept its reference to the underlying eventLog: with a no-op cancel it never received a terminal session_exited event and never saw its stream close, contradicting the end-of-session contract documented on Server.sessionEvents and docs/features/api-server/index.md. Fix: give the lazily-created log a real cancel that calls log.close("session ended"), matching what RegisterEventSource's pump already does on exit. Adds regression coverage (single and batch deletion) that connects a StreamEvents consumer to a lazily-created log, deletes the session, and asserts session_exited is delivered and the stream unblocks. Verified this fails against the no-op cancel and passes with the fix.
aheritier
commented
Jul 11, 2026
|
|
||
| For advanced use (custom elicitation, raw event inspection), call `chat.Runtime()` to access the underlying `runtime.Runtime` directly. | ||
|
|
||
| > [!WARNING] |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| b.ch <- ev | ||
| return nil | ||
| select { | ||
| case b.ch <- ev: |
| // means the response will reach the caller, never a discarded one. | ||
| func (r *LocalRuntime) ResumeElicitation(ctx context.Context, action tools.ElicitationAction, content map[string]any, elicitationID ...string) error { | ||
| id := firstElicitationID(elicitationID) | ||
| slog.DebugContext(ctx, "Resuming runtime with elicitation response", "agent", r.currentAgentName(), "action", action, "elicitation_id", id) |
| ok = r.elicitationWaiters.resolveSingle(result) | ||
| } | ||
| if !ok { | ||
| slog.DebugContext(ctx, "No matching elicitation request in progress", "elicitation_id", id) |
| slog.DebugContext(ctx, "No matching elicitation request in progress", "elicitation_id", id) | ||
| return errNoSuchElicitation | ||
| } | ||
| slog.DebugContext(ctx, "Elicitation response sent successfully", "action", action) |
| func (r *RemoteRuntime) ResumeElicitation(ctx context.Context, action tools.ElicitationAction, content map[string]any) error { | ||
| slog.DebugContext(ctx, "Resuming remote runtime with elicitation response", "agent", r.currentAgent, "action", action, "session_id", r.sessionID) | ||
| func (r *RemoteRuntime) ResumeElicitation(ctx context.Context, action tools.ElicitationAction, content map[string]any, elicitationID ...string) error { | ||
| id := firstElicitationID(elicitationID) |
| slog.DebugContext(ctx, "Resuming remote runtime with elicitation response", "agent", r.currentAgent, "action", action, "session_id", r.sessionID) | ||
| func (r *RemoteRuntime) ResumeElicitation(ctx context.Context, action tools.ElicitationAction, content map[string]any, elicitationID ...string) error { | ||
| id := firstElicitationID(elicitationID) | ||
| slog.DebugContext(ctx, "Resuming remote runtime with elicitation response", "agent", r.currentAgent, "action", action, "session_id", r.sessionID, "elicitation_id", id) |
aheritier
added
area/api
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3584 — elicitation (user-input) requests raised from multiple
concurrent background jobs were silently dropped: nothing was displayed to the
user and the requesting handler blocked forever.
This is finding A6 from the internal codebase audit.
Root cause
The elicitation path assumed a single, synchronous, LIFO-nested request at a
time, which broke under concurrency (e.g.
run_background_agentjobs runningRunStreamon detached concurrent goroutines):elicitationBridgewas a single global channel slot with a LIFO-nestingassumption — concurrent background jobs caused swaps/restores to interleave.
runCollectingsilently discardedElicitationRequestEvents.foreground and background elicitations could bleed into each other.
"no elicitation request in progress" branch.
PendingEventwas a single overwritableslot.
What changed
internal UUID (never the MCP wire ID, so two servers reusing an ID can't
evict each other), registered before the request event is emitted
(eliminating the TOCTOU branch).
elicitationHandlercalls theOnElicitationRequestsink synchronously and unconditionally first; the best-effort bridge send is
now ctx-bounded on a detached goroutine and can never block delivery.
runCollectingno longer re-forwards bridge events;elicitationHandleris the sole sink caller; the App-side dedupe map isremoved.
response is never discarded by a racing
ctx.Done().ElicitationRequestEventcarries the originatingsession ID; API-created runtimes register a session-scoped sink with a
replayable event log via
GET /api/sessions/:id/events(and the log closeson session delete).
PendingEventsby session ID, so a foreground stream stop no longer erases astill-live background job's elicitation.
replayPendingEventusestea.Sequenceinsteadof
tea.Batch.ResumeElicitation's newelicitationIDisvariadic so pre-Concurrent background-job elicitations are silently dropped — nothing displayed to the user #3584 callers still compile.
Testing
New/updated tests cover: duplicate wire IDs, cancel-vs-resolve race
(300-iteration
-racefuzz), wedged/abandoned bridge channel, real-handlerTOCTOU, exactly-once delivery, concurrent
RunAgentrouting, session-scopedserver sink + delete-closes-stream, foreground-stop vs detached background
prompt, and real TUI FIFO replay ordering.
task dev(lint + 19 custom cops + tests + build): greengo test -race -count=2on affected packages: greenNotes
Opened as a draft pending final maintainer review.