fix(thread): a reply typed into a task that looks done, but is running, lands - #986
Conversation
…g, lands The thread reads from two feeds — the run record for what the task is, the event stream for what it said — and the record can go quietly out of date. A half-open workspace socket (TCP dead, readyState still OPEN, so no error fires) stops delivering record updates while the transcript, watchdogged since #424, keeps flowing. Nothing else asked: no polling, a five-minute staleTime, no refetch on focus — so only a page reload cleared it. A task that had been continued or auto-resumed read as done while it worked, and the composer, aimed by that record at POST /continue, got "run is still active" back: the prompt bounced into the draft, and re-sending bounced. Closed at three depths: - global-events: the same liveness watchdog the transcript stream has — silence across both data and the server's 15 s ping rebuilds the socket, and the reconnect reconciles what was missed. - run-reconcile: the healer now works in both directions. A session that has opened and not ended, under a record calling the run settled, refetches after the same grace as the existing settled-over-running case. - deliver-prompt (new): a 409 is not the end. Refetch the record authoritatively and, when the truth names the other endpoint, deliver there. A 409 the fresh record agrees with is still the server's answer, and an empty submit is never turned into an empty message. The header's actions follow the same rule: a refused action refetches the record it was drawn from.
📦 npm preview published —
|
|
🤖 |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🤖 om-auto-review-pr — code review
Verdict: ✅ Approve — no blockers, no majors. Three minors/nits below, none of which need to hold the merge.
Summary
A three-layer fix for one failure mode: a half-open workspace SSE socket stops delivering record updates while the per-run transcript keeps flowing, so the thread renders a settled record over a live session and every reply 409s back into the draft. The layers are well chosen and each is independently useful — the watchdog stops the drift happening, the healer detects it when it happens anyway, and the composer's re-route means the user's prompt survives it either way. The "what it deliberately does not do" list (no retry when the fresh record agrees, no reopen for an empty draft, no second retry, judge the continue fallback on the fresh record) is the right set of restraints, and each one is pinned by a test.
Things I specifically verified rather than took on trust:
- The watchdog cannot trip on a healthy idle stream.
pingis inEVENT_NAMES(global-events.tsx:68) and the server emits it as a named event, not an SSE comment (packages/cezar/src/server/server.ts:4864,writeSSE({event:'ping'})every 15 s), solastFrameAtis refreshed on a quiet workspace.STALE_MS = 40_000≈ 2.7 pings, andconnect()resets the clock so a fresh socket is never judged before its first frame lands. - The watchdog test is non-vacuous. Removed the
setIntervalblock and re-ranglobal-events.test.tsx: 1 failed / 49 passed, on exactly the "reopens a silently-dead socket" case. The claim in the PR body holds. - The
live → continuere-route sends nothing stale.continueWithcloses over the stalerun, but the only wire fields it derives from it arerunnerOverride(fromrun.runner, immutable for a run) andagentProfile/model(omitted unless the user touched a pill), so the re-route posts the same body a fresh record would.canContinue(continuation-provider.ts) is provider state only and does not depend onrun.status, so the guard does not misfire in the direction the stale record was wrong about. run-header.tsx's sharedonErroris safe to widen.POST /runs/:id/pinand/unreadanswer only 200/404 (server.ts:3548,3572), so the new 409 branch cannot fire for the optimisticuseMarkRunUnseen, whose own comment rules an invalidation out. Onlycontinue/cancel— the two the comment names — can reach it.- No protected surface touched. Cockpit-only (
packages/web) plusCHANGELOG.md; no route, SSE event name,RunRecordfield, or NDJSON shape changes.settledSessionSeqstays exported with its exact previous semantics.
Findings
Minor — the useCallback in useDeliverPrompt is inert, and its comment claims otherwise
packages/web/src/routes/task-thread/deliver-prompt.ts:52
mutateAsyncis referentially stable … so this callback is stable across the streaming re-renders a live thread does constantly.
sendMessageAsync is stable, but continueWith is not: useContinueAction returns a plain object literal with continueWith: (text, images) => mutation.mutateAsync(…) (follow-up-engine.tsx:169), a fresh arrow every render. With continueWith in the dep array the memo never hits, so deliverPrompt — and therefore Composer's own handleSubmit memo, which lists onSubmit (composer.tsx:346) — is rebuilt on every frame of a streaming thread.
Not a regression: the inline arrow this replaced was equally unstable, and nothing downstream is memo'd, so there is no user-visible cost today. It is worth fixing because the comment states the property as established fact and the next reader will build on it. Either wrap continueWith in a useCallback over mutateAsync in useContinueAction, or drop the stability sentence.
Minor — the healer's new direction has no guard for a truncated transcript, only a live one
packages/web/src/routes/task-thread/run-reconcile.ts:98
The module doc rests on session.ended being "the sink's one guaranteed end-of-session line … on every exit path". That holds for the sink, but the caller is the one that can vanish: sessionEnded() is invoked by the RunManager (ui-event-sink.ts:144), so a run whose cezar process was killed mid-session never got one. reconcileLoadedRun then marks exactly that record failed with error: 'interrupted — cezar process exited during the run' (store.ts:619-633).
That record is now permanently "drifted" by the new rule: settled status, liveSeq > 0, no end in the transcript. Every time such a thread is opened, the healer waits out the 2 s grace and then invalidates both runs.detail(id) and runs.list() — a refetch that can never converge, because here the record is right and the transcript is the incomplete one.
It is self-limiting rather than a loop (the effect keys on the primitive status, which comes back unchanged, so it fires once per mount) and the list refetch is cheap, so this is not merge-blocking. But interrupted runs are not rare — any Ctrl-C of the server during a run produces one — and the asymmetry is worth naming. The cheapest fix is to skip the live-direction check when the record carries the interrupted marker (run.error / finishedAt set with no session end after it); the cheapest honest alternative is one comment line saying the wasted refetch on a truncated transcript is accepted.
Nit — the re-route can replace the server's 409 wording with a generic provider message
packages/web/src/routes/task-thread/deliver-prompt.ts:74
deliver('continue') goes through useContinueAction's mutation, which short-circuits on !canContinue with new Error('Connect an agent provider to continue.'). If providers/status were still pending or errored at submit time, the user would see that instead of the server's own 409 — the one thing the fetchQuery failure path two lines up is careful to preserve ("the user still gets the server's own words about the send they made"). useProviderStatus settles at mount, so this is effectively unreachable; flagging only because the module states the opposite rule explicitly and consistency is cheap here.
Validation gate
Run on the PR head (dfe2028) in an isolated worktree, in validation.commands order:
| Command | Result |
|---|---|
npm run typecheck |
✅ pass |
npm test |
|
npm run test:unit |
✅ 36/36 |
npm run build |
✅ check:pack ok — 497 files, 85 under web/dist |
npm run test:package |
✅ 16/16 |
The 6 npm test failures are |server| suites asserting "not a git repository" (git.test.ts, git-worktree.test.ts, health-forge.test.ts, projects-api.test.ts) — they fail because the review worktree's tmp dir sits inside this repo, so getRepoInfo finds a real remote where the test expects null. The diff touches zero server files, and GitHub CI is green on this exact head, so there is no causal link. This matches the PR body's own account.
The four test files this PR touches were also run in isolation: 181/181 pass.
CI
All required checks green at review time — Unit, build, E2E, and package ✅, Publish npm snapshot ✅, license/cla ✅. No merge conflicts (MERGEABLE, head sits directly on main).
Not covered by this review
npm run test:e2e (real-Chrome smoke) was not run, and the central behaviors here — a socket that goes half-open, and a record that drifts over minutes — are the kind that only a real browser over real time can demonstrate. That is what the needs-qa label and the QA instructions comment are for.
pat-lewczuk
added
merge-queue
|
🤖
|
🧪 Manual QA instructions (
|
| Priority | Setup and action | Expected result / boundary |
|---|---|---|
| P0 | Run a task to completion so the header reads done. Open it at /p/<projectId>/tasks/<id> and block workspace/events in that tab. From a second window, click Continue on the same task so it is running again. Back in the blocked tab — still showing done — type a reply and send. |
The reply lands in the live session: it appears in the transcript and the agent acts on it. It must not bounce back into the draft with a run is still active toast. Boundary: sending it a second time must not be needed. |
| P0 | Same staged drift, but watch the header for ~5 s after the second window's Continue, without typing anything. | Within a couple of seconds of the transcript showing the session reopening, the header self-corrects from done to running and the Working… indicator appears — with no page reload and no navigation away and back. |
| P0 | The mirror: open a running task, block workspace/events, and from the second window cancel it (or let it finish). With the stale tab still in live-session mode, type a reply and send. |
The reply is delivered as a Continue — the session reopens with that prompt — rather than failing against the closed session. |
| P0 | Disconnect every agent provider (Settings → Agents → Providers), then open a done task and click Continue / send a follow-up. | The server's own refusal wording is shown once, verbatim, as a danger toast. There must be no silent second attempt on the other endpoint and no generic substitute message. |
| P0 | Stage the first drift again (stale done, run actually live), then press the composer's Continue with an empty draft. |
The run is still active toast is shown and no empty message is delivered to the live session. The header should correct itself in the same beat. |
| P1 | Stage the first drift again, then use the run header's Continue button (not the composer) on the task that is actually running. | The toast carries the server's words and the action bar redraws to the truth — the refused action stops being offered — without a reload. Repeat for Cancel on a task that has just finished. |
| P1 | Liveness watchdog: with the cockpit open and idle on the tasks list, suspend the cezar server process (kill -STOP <pid>) for ~90 s, then resume it (kill -CONT <pid>). Keep DevTools → Network open and filtered to workspace/events. |
A second workspace/events request is opened (the socket was rebuilt rather than left silently dead), and once the server is back the task list reflects anything that changed while it was suspended — again with no reload. |
| P1 | Watchdog false-positive check: leave the cockpit open and completely idle — no tasks running — for 5 minutes, with DevTools → Network filtered to workspace/events. |
Exactly one workspace/events request for the whole window. A quiet workspace must not be mistaken for a dead socket; repeated reconnects here would be a regression. |
| P1 | Background the cockpit tab (switch to another tab) for ~3 minutes, then return to it. | On return the view reconciles and is correct. No reconnect storm in the network panel from the time the tab was hidden. |
| P2 | Regression sweep, no drift staged: send a normal follow-up to a running task; click Continue on a done task; stack a message onto a queued task. | All three behave exactly as before this PR — the queued message is folded into the prompt, the running task takes the message live, the done task reopens its session. |
| P2 | Cold-load the thread of a live task on a mobile viewport and on a fresh page load. | Loading state, header, and composer render as before; the composer's mode (Send vs Continue) matches the run's real state on first paint. |
QA reviewer: move merge-queue → qa when starting. All checks pass → replace qa with merge-queue and qa-approved. A failure → replace qa with qa-failed and report the failed action, expected result, and observation.
qaGate is on in this repository, so QA approval still gates the merge — this PR stays blocked in merge-queue until qa-approved is added.
|
🤖 No blockers and no majors; three minors/nits are in the review for the author to take or leave. Validation gate run on the PR head in an isolated worktree — typecheck, autofix: skipped (not my PR — re-run with --autofix to fix it here). Not that it applies on an approve, but noting it for the record. |
|
🤖 The approving review above stands — this is not a code finding, and nothing about the diff changed. It is a base-drift note, posted because the conflict appeared during the review: The conflict is one file, and it is purely textual: That is the whole of it — Resolution: merge This is another author's PR and the run carried no The |
# Conflicts: # CHANGELOG.md # packages/web/src/routes/task-thread/task-thread.tsx
# Conflicts: # packages/web/src/routes/task-thread/task-thread.tsx
… the current-main merge The PR branch was pushed to while this one was being brought up to date: it carries a merge of main as of dac506f plus a7c3696's dispatch test fix. This branch had already merged main through 1deb166, so the resolution keeps this side's state and takes their test fix. - task-thread.tsx: both sides resolved #939's draft wrapper around deliverPrompt identically; kept the commented spelling. - CHANGELOG.md: their side predates the 0.11.0 release cut, so its "# Unreleased" section and the long-form #917 entry are already released (and condensed) on main. Kept main's released text and left this fix's entry in the post-release Fixes list.
…still reported Two defects found reviewing the session.idle turn boundary. A prompt posted while a turn is still in flight replaced that turn's resolver, so the superseded turn's `await turnEnded` never settled and its `prompt()` hung forever — reachable as soon as `ready` has resolved, which is exactly what the cockpit does when it delivers a message into a running task (#986). A new prompt now closes the turn it supersedes, emitting its turn-end the way the old per-POST `finally` did, and cancels the auto-end it may have armed. Swallowing a transport drop on a live session also swallowed a REAL one: the POST and the SSE stream fail together, and whichever the runner sees first decides. A drop is now remembered for the turn and reported when the turn ends WITHOUT a session.idle — the session never said it was finished, so the drop was never explained. A turn that does reach session.idle stays silent, which is the #897 case. Refs #897
…n under Needs you (#1005) * docs(runs): add execution plan for opencode-long-turn-needs-you * fix(opencode): take the turn end from session.idle, not a fetch that undici cut at 300s opencode holds POST /session/:id/message open for the whole agent turn. Node's global fetch is undici, whose default headersTimeout/bodyTimeout is 300_000ms, so the request died with 'TypeError: fetch failed' at exactly 5:00 — and prompt()'s finally emitted turn-end anyway, parking a run whose session was still streaming tool events. On Continue the same drop rejected bootstrap()'s un-caught first prompt, surfaced as a fatal error and got the live session SIGKILLed. Two changes, both inside the opencode runner: - opencode-http.ts: the message POST and the SSE subscription go through node:http, which applies no client-side header or body timeout. Raising undici's would mean adding an undici Agent — a new runtime dependency of the published CLI — for a request the platform can already make. The SSE stream moves too: bodyTimeout is an inactivity timer, so a quiet session would lose its event bus after the same 300s. - v1's turn-end now comes from the wire session.idle, the signal v2 has always used, with bounded ways out for a server that never sends one: a grace window re-armed by every message.* frame, an immediate end when there is no event bus at all, and settlement on teardown or a server that exited. A transport drop on a session the bus still shows alive emits neither turn-end nor the 'prompt failed' note. The #880 agent-step wall clock is untouched. Refs #897 * test(opencode): pin the over-five-minute turn, the short turn and the ways out The mock server grows two scripted turns, selected by a marker in the prompt text so the #897 shapes are reproducible without waiting five real minutes: `#drop-post` destroys the message POST's socket mid-turn and keeps streaming parts before sending session.idle, `#no-idle` never sends session.idle at all. `MOCK_NO_EVENT_BUS=1` makes GET /event 404. Six runner cases: the dropped POST on the first prompt (the Continue path that used to emit a fatal error and get the live session SIGKILLed) and on a later sendMessage (the path that used to note 'prompt failed' and park the run); the short turn, unchanged; that the prompt POST never goes through global fetch; and the two ways out of a turn waiting on a session.idle that is not coming. Five of the six are red against the pre-fix runner — the first prompt case fails with 'expected [ "opencode: fetch failed" ] to deeply equal []'. The sixth (no event bus) passes both ways on purpose: it pins the behaviour this change does NOT alter. opencode-http.test.ts covers the transport itself: a status is returned rather than thrown, a lost connection is an OpencodeTransportError, a slow two-part response still arrives, and the SSE reader resolves on headers and reassembles frames split across writes. Refs #897 * docs(opencode): the v1 turn-end now reads the same session.idle as v2 The mapper header and the wiring test still described v1's turn-end as synthesized from the prompt POST's HTTP response. Both streams take it from session.idle since #897; say so where the next reader will look. Refs #897 * fix(opencode): a superseded turn settles, and an unexplained drop is still reported Two defects found reviewing the session.idle turn boundary. A prompt posted while a turn is still in flight replaced that turn's resolver, so the superseded turn's `await turnEnded` never settled and its `prompt()` hung forever — reachable as soon as `ready` has resolved, which is exactly what the cockpit does when it delivers a message into a running task (#986). A new prompt now closes the turn it supersedes, emitting its turn-end the way the old per-POST `finally` did, and cancels the auto-end it may have armed. Swallowing a transport drop on a live session also swallowed a REAL one: the POST and the SSE stream fail together, and whichever the runner sees first decides. A drop is now remembered for the turn and reported when the turn ends WITHOUT a session.idle — the session never said it was finished, so the drop was never explained. A turn that does reach session.idle stays silent, which is the #897 case. Refs #897 * docs(runs): mark opencode-long-turn-needs-you complete
2 participants
The bug
A prompt typed into a task that reads as finished — but is actually running — never reaches the agent. The reply bounces back into the draft with a toast, re-sending bounces again, and going to Tasks and back does not help: only a full page reload fixes the view.
Why
The thread reads from two feeds: the run record for what the task is, the event stream for what it said. The record can go quietly out of date, and nothing asks again — the cockpit does not poll,
staleTimeis five minutes, andrefetchOnWindowFocusis deliberately off (query-client.ts). Navigating away and back lands on the same cached lie.The likely trigger: the workspace stream had no liveness watchdog, while the per-run transcript stream has had one since #424. A half-open socket (TCP dead,
readyStatestill OPEN, so noerrorever fires) stops delivering record updates while the transcript reopens itself and keeps flowing. A task that was continued or auto-resumed therefore keeps reading asdonewhile it works — and the composer, aimed by that record atPOST /continue, gets409 run is still activeback.The fix, at three depths
api/global-events.tsxopenhandler reconciles everything missed.task-thread/run-reconcile.tsrunningover a settled transcript; now a session that has opened and not ended, under a record calling the run settled, refetches after the same 2 s grace.task-thread/deliver-prompt.ts(new)staleTime: 0) and, when the truth names the other endpoint, deliver there instead. The prompt lands rather than bouncing.The run header's actions follow the same rule: a refused Continue/Cancel refetches the record it was drawn from, so the bar redraws to the truth instead of offering the same refusal.
What the re-route deliberately does not do
continuefallback is judged on the fresh record'slastSessionId, never on the stale one the UI was drawn from.Validation
npm run typecheck✅npm test✅ — web: 158 files / 3562 tests. The 10 failing server tests (git.test.ts,git-worktree.test.ts, health/route-parity) fail identically onmainwith these changes stashed: they assert "outside a git repository" while the test tmp dir sits inside this repo. Unrelated.npm run test:unit✅ ·npm run build✅ (check:pack ok — 497 files) ·npm run test:package✅ (16/16)New coverage: 9 tests in
deliver-prompt.test.tsx, 12 added torun-reconcile.test.ts, 4 watchdog tests inglobal-events.test.tsx, 1 inrun-header.test.tsx. The watchdog test was verified non-vacuous — it fails with the interval removed.Not run:
npm run test:e2e(real-Chrome smoke suite). Cockpit-only change, covered by the unit gate.