Repository navigation
fix(opencode): end an ACP prompt when its session goes idle - #60089
2SinceJuly wants to merge 2 commits into
Conversation
A failed durable write can drop session.execution.* during a turn, so the ACP adapter waited for an outcome that never arrived and session/prompt stayed pending. Also end the turn when session.wait reports idle, and report a session.execution.failed that arrives before the prompt is delivered instead of ignoring it forever.
There was a problem hiding this comment.
This fixes the hang from #60065, but it causes a regression in normal turns, so it needs changes before it can merge.
I ran the issue's fault-injection script against opencode acp from source on the base and on this branch. With the PR, both locked-session scenarios now resolve and the next prompt is accepted. In a healthy turn where the model hits the output limit, though, the base returns the core's output-limit error (Response still truncated after two output token limit continuations) every time (16/16). With this branch, every turn (17/17) returns stopReason: "max_tokens" with no error. All three model steps still run; only the error is lost. The reason: the session/wait long poll returns on a separate HTTP request, and it reaches the client before the terminal event on the SSE stream. The idle path then ends the turn and drops whatever hasn't arrived yet: the session.execution.failed, and possibly the last text, usage, finish reason or permission asks. That means real provider failures can now be reported to ACP clients as successful turns.
A related problem: when idle wins and no outcome was recorded, the turn ends as end_turn. In the lost-write scenario this reports a turn that failed as a success, and the issue expects an error there.
Idle should be a fallback that runs after the event stream has caught up, not a race against it. For example, after wait resolves, keep reading events until a terminal event or a short drain timeout. Only then end the turn, and with an error, since its outcome is unknown. Please also add a test where idle resolves before a session.execution.failed that is still in flight, and check that the error is reported.
Public API: unchanged. The adapter only starts calling the existing POST /api/experimental/session/:sessionID/wait endpoint.
Related: #52796
`session/wait` answers on its own request and can reach the client before the terminal event on the SSE stream, so ending the turn as soon as it reports idle dropped `session.execution.failed` along with the trailing text and usage. A healthy turn that hit the output limit was reported as `max_tokens` with no error. Idle is now a fallback: keep folding events until a terminal one arrives or the stream stays quiet for the drain timeout. When nothing ever reports how the turn ended, fail the prompt instead of completing it as a success.
|
Both points are addressed in Idle is a fallback now, not a terminator. When A lost outcome is an error, not a success. If the drain ends without a terminal event — the dropped-durable-write case — the turn records Tests in
|
There was a problem hiding this comment.
The new commit fixes the race and the lost-outcome reporting I raised earlier.
I re-ran the #60065 fault-injection script against this head:
- Mid-turn lost outcome: now fails after about 1s with
The session went idle before it reported how the turn ended, and the next prompt is accepted. - Pre-delivery failure: reports the
SQLITE_FULLerror, and the session isn't locked afterwards. - Healthy output-limit turns: 10/10 again end with the core's
output-limiterror, with all text delivered before the response. The earlier commit reportedmax_tokenshere.
bun test test/acp passes (103 tests) and tsc -b is clean.
Public API: unchanged. The adapter only calls the existing POST /api/experimental/session/:sessionID/wait.
A minor note, not blocking: the idle drain only times out after 1s of silence on the whole event subscription. If the outcome is lost while another session on the same server keeps streaming, this prompt stays open until that stream goes quiet.
|
Confirmed against |
fe790e9 to
2037806
Compare
There was a problem hiding this comment.
The new push only rewrites the commits. The code is identical to the head I last reviewed, so my earlier result still holds: both #60065 scenarios resolve and the session isn't locked afterwards, healthy turns still report their real outcome, and the ACP tests and type check pass. I have nothing new to raise.
Public API: unchanged.
Issue for this PR
Closes #60065
Type of change
What does this PR do?
When a durable write fails during an ACP turn (for example
SQLITE_FULL), thesession.execution.*outcome can be lost. The adapter only ended a prompt on that event, sosession/promptstayed pending and every later prompt was refused withSession already has an active ACP prompt.packages/cli/src/acp/turn.ts: once the prompt is delivered, the turn also waits onPOST /api/experimental/session/:id/wait. If the session goes idle before an outcome event arrives, the turn ends through the normalsettlepath, which still reports a step or execution error that was recorded.packages/cli/src/acp/translate.ts: asession.execution.failedthat arrives beforesession.inbox.deliverednow ends the turn instead of being ignored. That turn can never deliver its prompt, so there was nothing left to wait for.Normal turns are unaffected: the terminal event is published before
awaitIdleresolves, so an event still ends the turn first.Only the ACP side is in scope here. Core storage retries stay in
fix/disk-full-session-recovery/ #53704.How did you verify your code works?
bun test test/acpfrompackages/cli: 102 pass.test/acp/turn.test.tscover the lost outcome and the pre-delivery failure. Both fail without the source change (the first times out at 5s, the second never gets the error) and pass with it.bun run typecheckfrompackages/cli: no type errors.bun run checkat the repo root passes locally.Checklist
Added by the opencode bot (AI-generated).
Related: #52796