fix(chat): a background turn no longer looks dead when you switch tabs - #18
fix(chat): a background turn no longer looks dead when you switch tabs#18Amir-SSVLabs wants to merge 6 commits into
Conversation
vite.config.ts reads process.env.TAURI_DEV_HOST, but no Node types are installed, so \ pm run check\ reports \Cannot find name 'process'\ - one error, on main, today. The repo's own pre-commit hook runs that check, so it cannot pass without this. Pinned to 22.17.0, matching the Node the Recall sidecar ships against. CI's typecheck leg is continue-on-error, so this does not change CI colour - it unblocks the local hook and makes the leg honest enough to ratchet later.
Ask a question, switch to another chat, come back: the transcript showed the user's own message with nothing under it and no spinner, which is indistinguishable from a hung app. The turn was fine the whole time. Two invariants were broken. They are committed together because they are the same one seen from both ends: a running turn must look running, and the tab it runs in must stay attached to it. **A running turn must look running.** - showWorkspaceTabSurface set `sending = false` on every tab switch, and the authoritative restore only lands after getRun + a CLI sync + the transcript read. Both it and openSession now SEED from `workingRuns` — the set live run_state events already maintain for EVERY run, not just the open one, which is why the tab dot stayed right while the chat body looked dead — so the spinner is correct on the first frame. - openSession could only ever set `sending` true; the not-running case fell through, so an optimistic seed could stick. It is now an explicit three-way decision in a pure, tested `restoredTurnActivity`, and the paths that leave early without learning the run's real state (a failed getRun, a vendor handover) clear the seed instead of stranding it. - A run was only tracked as working once its first run_state arrived, so a user who switched away during the spawn came back to a tab that had bound its run but showed no activity. Dispatch marks it immediately, and — because a turn that never starts emits no terminal state — the failure path un-marks it, or the sidebar would spin for the session. `spawning` is the sharp edge: the event handler counts every non-terminal state as working, so a restore recognising only running/pending would call the whole cold-start window idle and tear down a live turn. TERMINAL_RUN_STATES is shared by both now, and both directions are tested. **A tab must stay bound to its run.** Evidence from a real occurrence: the answering run appeared against no tab in session-ui.json while the active tab held sessionId=null. With no binding, hydrateWorkspaceTabAfterPaint resolved the tab to no session and took a silent early return — no IPC at all, so the cached prompt rendered forever while the finished answer sat unreachable in the event log. Dispatch now records the run each tab sent, and the hydrate path repairs the binding from it rather than stopping silently. A second, weaker check in the event handler covers the case where the workspace still knows the run but the tab does not. Honest limit: which path drops the binding is still unidentified. This makes losing it recoverable and loud (both repairs log a warning naming tab and run) rather than fatal and invisible — it is not a root-cause fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oads Returning to a running turn showed a fragment of the answer starting mid-word, with document markup in it: ched tabs and came back<button onclick="probe()">Probe environment… Two causes, one visible symptom. Deltas are only rendered for the open session, so a switch-back began accumulating from whatever arrived next — hence the severed "…ched tabs". And because the opening ```fence had arrived while the user was away, the scratchpad strip could not match it, so the document's raw HTML rendered as the agent's own message. Deltas are now buffered per run and both restore paths seed from that buffer, so the answer resumes from its first word. The buffer is dropped on completion and on every path that ends a turn without one; silent turns are never buffered at all; and past the cap it keeps the TAIL behind an explicit marker rather than splicing an unmarked gap into the middle of the restored text. stripStreamingBlocks then hides any block that is open-but-unterminated. Complete blocks are stripped first, so prose following a finished block survives — tested, because cutting to end-of-string would otherwise eat the agent's closing question. This also fixes the pre-existing case where raw JSON was visible in the transcript while any scratchpad streamed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The elapsed counter restarted at zero on every tab switch: its effect stamped a start time whenever `sending` flipped true, and switching back flips it true again. A turn that had been running two minutes read as 3s - the counter was measuring how long the user had been LOOKING at the turn. Turn start is now recorded per run: at send time, so the CLI cold start the user waits through is counted rather than hidden, and on the first run_state for a turn begun in a background tab. The stamp is cleared with every other per-run trace when the turn ends, by any route - terminal state, explicit stop, failed dispatch, or session delete - so a later turn on the same run can never inherit the previous one's age.
alonmuroch
left a comment
alonmuroch
left a comment
There was a problem hiding this comment.
Good PR — real root-cause work rather than symptom-patching, and the extracted pure helpers (turn-restore.ts, stripStreamingBlocks) are properly tested. The spawning analysis is correct: commands/session.rs:647 does emit "spawning", and map_state_to_run_status folds it into RunStatus::Running, so the narrow running/pending list would indeed have called the entire cold-start window idle. The write-up of the known gap is the right way to ship an incomplete root cause.
Four things I'd like fixed before this merges. The first one reopens a narrower version of the bug the PR exists to fix.
1. The optimistic working mark isn't durable — loadSessions() wipes it mid-dispatch
src/routes/+page.svelte:8310 replaces the set wholesale:
workingRuns = new Set(
runs.filter((r) => String(r.status).toLowerCase() === "running").map((r) => r.id),
);dispatch calls loadSessions() itself at :7800, unawaited, a few lines after this PR's new markRunWorking(targetRunId, sentAt) and before sendOrResume. On the warm-session path (+ New → first message) the run is idle in the DB at that moment — warmSession leaves the process "alive and idle" (:4666) — so the filter drops it and the mark is gone until the first run_state event arrives. Switch tabs inside that window and the turn reads as dead again.
The cold-spawn path at :7901 survives only incidentally: startRun has already emitted spawning, which maps to RunStatus::Running, so the row happens to match the filter.
The comment at :8306 blesses exactly this ("it's fine if a just-spawned active run still reads idle here") — on the premise that workingRuns only drives a sidebar dot. This PR makes it the seed for tabIsWorkingOnOpen, so the premise no longer holds and the comment should change with it.
Suggest merging rather than replacing: keep any run already marked in-flight whose persisted status isn't terminal.
Related, same site: the wholesale rebuild also re-adds runs left running in the DB by a crash or kill. That was cosmetic before; now it seeds a spinner on tab switch, and silentTurnDisposition("running") returns wait forever, so the watchdog can't clear it. Pre-existing, not introduced here — but worth a look while you're in this function.
2. inspectSilentTurn bypasses forgetRunActivity
:2311-2317 still hand-deletes from workingRuns only:
sending = false;
turnStopping = false;
actorLive = disposition.actorLive;
const next = new Set(workingRuns);
next.delete(target);
workingRuns = next;This is one of the "ends a turn without a terminal event" paths the PR's own bookkeeping section is about, and it's the one that got missed. It leaves streamingByRun, turnStartedByRun and hiddenRuns populated, with three visible consequences:
- the stale partial answer re-seeds
streamingon the next visit to that tab, on top of the hydrated transcript; - the next turn on that run inherits the old start time —
markRunWorkingonly sets when absent — so the elapsed counter opens inflated; - a run whose silent turn settled this way stays in
hiddenRunspermanently, so its later visible turns are never buffered at all.
forgetRunActivity(target) here.
3. The handover guard still uses the narrow definition
:8564 is untouched:
const running = st === "running" || st === "pending";and it still gates shouldHandoverOnContinue. So opening a session during a cold spawn, with a different vendor selected, can trigger a vendor handover on a turn that is mid-flight — the same spawning disagreement the PR argues is invisible and brutal, just in the other consumer of st. Should be !isTerminalRunState(st).
4. The event-handler rebinding can't tell "unbound" from "gone"
if (activeTabId && !workspaceTabs.find((tab) => tab.id === activeTabId)?.sessionId) {A missing tab and an unbound tab both land here, and bindWorkspaceTabToSession no-ops on an unknown id — so in that case the warn, the workspaceTabs reassignment and persistWorkspaceTabs() would fire on every delta of the turn. I couldn't find a path where activeTabId outlives its tab, so this is defensive rather than a live bug, but a block whose whole purpose is to be a defect detector shouldn't be able to bury the log. Check the tab exists, and log once.
Minor
- The doc comment for
markRunWorkingis orphaned: two/** */blocks stack, and the first one (describing the mark) sits aboveturnStartedByRun. package-lock.jsoncarries an unrelated0.5.8 → 0.6.0version re-sync.@types/nodepinned at 22.17.0 and the reasoning for it: no objection.
Happy to re-review quickly once 1 and 2 are in — 3 and 4 are small enough to take or leave with a reply.
Review follow-up. Four defects, the first of which reopened a narrower version of the bug this branch exists to fix. **1. `loadSessions()` erased the mark it was supposed to preserve.** It rebuilt `workingRuns` wholesale from persisted status, and `dispatch` calls it unawaited a few lines after marking its run in-flight. On the warm path that run is still `idle` in the DB — the process is alive and waiting — so the rebuild dropped the mark until the first `run_state` arrived, and a tab switch inside that window read the turn as dead again. The poll is now a SEED, not the authority: `mergeWorkingRuns` adds runs the backend reports as running and keeps existing marks. Note that the obvious predicate — keep a mark unless persisted status is terminal — does NOT fix this, because `idle` is terminal by that definition and `idle` is exactly what the warm run reads. So marks are reaped only on a state a live process cannot sit in: completed / failed / stopped. `runStateIsSettled` names that distinction and is tested against it. Clearing marks stays with the definite signals — terminal events, an explicit stop, a failed dispatch, the watchdog. **2. The watchdog path bypassed the cleanup helper.** `inspectSilentTurn` hand-deleted from `workingRuns` only, so it left the stream buffer, the turn's start time and the silent-turn flag behind — a stale partial answer re-seeded over the hydrated transcript, the next turn's clock opening inflated, and a run permanently excluded from buffering. It now goes through `forgetRunActivity`, which is the whole point of that function existing. **3. The handover guard kept the narrow definition.** It judged mid-turn by `running`/`pending`, so opening a session during a cold spawn with another vendor selected could hand over a live turn — the same `spawning` disagreement this branch argues is invisible, in the other consumer of that state. Both now ask `runStateIsWorking`. **4. The rebind could not tell "unbound" from "gone".** A missing tab took the same branch as an unbound one, and the bind no-ops on an unknown id, so the warning and a `persistWorkspaceTabs()` would have fired on every delta. A detector must not be able to bury its own signal: it now requires the tab to exist. Also: `markRunWorking`'s doc comment had drifted above `turnStartedByRun`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks — all four were real, and #1 was the one worth catching. Fixed in 19d338a. 1.
|
| actorLive = true; | ||
| armWatchdog(); | ||
| } else { | ||
| sending = false; |
There was a problem hiding this comment.
🟡 openSession's authoritative restore clears sending when the run resolves idle but never clears the streaming it seeded from streamingByRun at line 8720, and the streaming bubble renders on streaming truthiness alone (line 12267). If the background run's message_complete/terminal run_state push was dropped — the file's own premise is that push delivery is not guaranteed, and reconcileRunEvents only covers the open run — the buffer goes stale and the finished answer renders twice forever: once in the hydrated transcript and again as a live streaming bubble. In the activity !== "working" branch, also set streaming = "" and streamingByRun.delete(id).
| // arrived, and a tab switch inside that window read the turn as dead again. | ||
| // This set is no longer just a sidebar dot: it seeds the spinner on restore | ||
| // (`tabIsWorkingOnOpen`), so an in-memory mark outranks a stale row. | ||
| workingRuns = mergeWorkingRuns(workingRuns, runs); |
There was a problem hiding this comment.
🟡 A stale workingRuns mark is now unreapable: mergeWorkingRuns deliberately keeps marks for runs whose row reports idle (and for rows missing from the list), and nothing else removes the mark when the terminal run_state push for a background run is dropped — background runs have no reconciler, and openSession's restore corrects only sending, never workingRuns. The old replace-from-DB seeding self-healed within one loadSessions; now the sidebar/tab-dot spinner persists for the rest of the session and every switch-back re-seeds sending = true via tabIsWorkingOnOpen until hydrate flips it back. Call forgetRunActivity(id) in openSession when restoredTurnActivity resolves to idle/waiting-for-user — the on-disk event log is authoritative there, so the reap is safe.
| // Dropped as soon as the message completes (it becomes a transcript row). | ||
| const streamingByRun = new Map<string, string>(); | ||
| const STREAM_BUFFER_CAP = 400_000; | ||
| const STREAM_TRUNCATED = "\n\n… (earlier output dropped from the background buffer) …\n\n"; |
There was a problem hiding this comment.
🟡 STREAM_TRUNCATED adds new user-facing UI text ("… (earlier output dropped from the background buffer) …", rendered inline in the chat transcript) without updating messages/en.json and messages/zh-CN.json. CONTRIBUTING.md ("i18n": when adding new UI text, update both locale files).
| } else if (running) { | ||
| // Authoritative turn state — also CORRECTS the optimistic seed taken when | ||
| // the tab was shown, so an over-eager spinner can never get stuck on. | ||
| const activity = restoredTurnActivity({ hasPendingAsk: !!restoredPendingAsk, runState: st }); |
There was a problem hiding this comment.
🟠 The elapsed-turn clock only survives a tab switch, not an app relaunch (or the first time this window sees an already-running background run).
turnStartedByRun is populated only two ways: markRunWorking() at dispatch time (this window sent the message), or a live run_state event once it arrives (+page.svelte:2129). openSession's authoritative restore here computes activity from st and correctly sets sending = true for a working turn, but never seeds turnStartedByRun for that run — so when the effect at +page.svelte:3269 runs (turnStartedByRun.get(runId) ?? Date.now()), it falls back to Date.now() and the counter restarts at 0. That's the exact symptom this PR sets out to fix ("the counter continue[s] instead of resetting"), just reached a different way.
Concretely: relaunch the app while a long turn is still running (or is picked up cold via maybeRestoreLastSession). catchUpFromCli a few lines above already exists specifically to handle "quit, crashed, or restarted mid-turn" for the transcript — spinner and content restore correctly for that path via tabIsWorkingOnOpen/restoredTurnActivity — but the elapsed counter doesn't get the same treatment and will read seconds-since-relaunch instead of the turn's true age.
RunMeta.started_at (backend) is set once at run creation, not per turn, so it can't be dropped in as-is for a multi-turn session — but some per-turn timestamp derived from event history would fix this. Worth a follow-up if elapsed-time accuracy matters across restarts, not blocking this PR.
4 participants
Summary
Leaving a chat while the agent is still answering, and coming back, could leave you looking
at your own message with nothing under it — no answer, no spinner, no way to tell whether
the app was working or wedged. In the worst case the answer had already arrived and was
sitting in the run's event log, permanently unreachable from the UI.
This fixes the three separate mechanisms behind that, plus the elapsed counter that reset
every time you switched tabs. All of it is reproducible on
maintoday.Background: how a turn and a tab relate
Three facts about the current design make the rest of this readable.
A conversation ("run") outlives the tab showing it. A run is a backend row plus a live
CLI process. It keeps working whether or not you are looking at it, and it is the only
place the answer exists until the UI reads it back.
Only the open session renders. The event stream carries events for every run, but the
chat view drops anything that isn't the active one:
Anything a background run says is therefore not "displayed later" — it is discarded, and
the UI has to re-read it from disk when you return.
A tab switch is two phases, and there is a gap between them.
showWorkspaceTabSurfaceswaps the visible state synchronously (instant), andhydrateWorkspaceTabAfterPaintthen does the IPC to restore the real transcript —getRun, a CLI transcript sync,getBusEvents. That second phase takes hundreds ofmilliseconds to seconds.
So three things must survive a switch: is this turn still working, what has it said so
far, and which run does this tab even own. Each of the three was broken differently.
Mechanism 1 — the spinner was cleared and restored too late
showWorkspaceTabSurfacedidsending = falseon every switch. That is correct forleaving a chat (don't inherit the previous one's spinner) but wrong for arriving at one
— and the authoritative restore only ran at the end of phase two. For the whole gap, a live
turn rendered as an idle one.
There was a second, worse version of this:
openSessioncould only ever setsendingtotrue. The "not running" case fell through and set nothing, so the restore could turn a
spinner on but never off.
Fix. Both paths now seed from
workingRuns— the set that live events already maintainfor every run, not just the open one. (That set is why the tab dot stayed correct while the
chat body looked dead; the information was already there, the chat just wasn't reading it.)
The restore is now an explicit three-way decision —
waiting-for-user/working/idle— in a pure, tested helper, so an optimistic seed always gets corrected. Paths that return
early without ever learning the run's real state (a failed
getRun, a vendor handover) nowclear the seed rather than leaving a spinner on forever.
The subtle part:
spawningThis one is worth a paragraph because getting it wrong silently reintroduces the whole bug.
The live handler decides "is this run working?" by exclusion — anything that isn't
idle/completed/failed/stoppedcounts as working. That includesspawning, thestate a run sits in while a cold CLI process boots, which is 10–20 seconds.
My first version of the restore listed the working states positively:
runningandpending. Those two rules disagree aboutspawning. The consequence: switch tabs during acold start and come back, and the "authoritative correction" would decide a live turn was
idle, tear down its spinner, and disarm its watchdog — the original bug, restored, in the
slowest window there is.
So the terminal list is now a single shared constant,
TERMINAL_RUN_STATES, used by boththe live handler and the restore, and both directions are tested. Any state either side
doesn't recognise means working, which is the safe default: a spinner that shouldn't be
there gets cleared a second later by the real state, whereas a missing one looks like a
crashed app.
Mechanism 2 — a tab could lose its link to its run, with no way back
This is the one that produced "my answer never came back".
A tab points at its conversation through
tab.sessionId. That is persisted state, and in areal occurrence on a dev machine it was gone: the run that had answered appeared against
no tab at all in
session-ui.json, and the active tab heldsessionId: null.The damage came from how that case was handled. With no session id, the hydrate step
resolved the tab to nothing and hit a silent early return — no IPC, no log, no error. So
the tab rendered its cached prompt forever, while the finished answer sat in the event log a
single
getBusEventscall away. The app log confirmed the shape exactly: aget_runfor thetab being switched to, and never one for the tab being switched back to.
Fix. Dispatch now records which run each tab sent (
lastRunByTab), deliberately inseparate storage from the binding it backs up. When hydrate finds a tab with a transcript
but no session, it repairs the binding from that record and restores properly instead of
stopping. A second, weaker check in the event handler covers the narrower case where the
workspace still knows the run but the tab has lost it.
Mechanism 3 — partial answers resumed mid-word, sometimes as raw HTML
Because background events are discarded rather than buffered, returning to a turn
mid-sentence started accumulating text from whatever chunk arrived next. Everything before
it was simply gone, which is why the answer appeared to begin in the middle of a word:
The markup in that line is the second half of the problem. Scratchpad documents travel
inside a fenced block that the UI strips before rendering. Rejoining mid-stream means the
opening fence arrived while you were away, so the strip had nothing to match on and the
document's raw HTML rendered as if the agent had written it as prose.
Fix. Deltas are now buffered per run, so both restore paths resume the answer from its
first word. The buffer is dropped when the message completes and on every path that ends a
turn without completing; silent turns are never buffered at all; and past its size cap it
keeps the tail behind a visible marker rather than splicing an unmarked hole into the middle
of the text. Separately,
stripStreamingBlockshides any block that is open-but-unterminated— which also fixes a pre-existing wart where raw JSON was briefly visible in the transcript
whenever any scratchpad streamed.
Also: the elapsed counter was measuring the wrong thing
The timer restarted at zero on every tab switch, so a turn running for two minutes could
read
3s. Its effect stamped a start time wheneversendingbecame true — and returning toa tab makes that true again — so it was measuring how long you had been looking at the
turn, not the turn's age.
The start time is now recorded per run, stamped when you press send. A cold start is
therefore counted rather than hidden, which matters because that is the slowest part of the
wait.
Bookkeeping that had to come with it
Marking runs as working introduced state that needed an owner. Every per-run trace (working
flag, stream buffer, start time, silent-turn flag) is now cleared through one function,
called not only on the terminal event but on the paths that end a turn without one: a
dispatch that fails after the run row exists, an explicit stop, a session delete. Without
that, a failed send would leave a run spinning in the sidebar for the rest of the session.
Run ids are also normalised to strings at every read and write of those collections — a
write under
123and a delete under"123"would leak the entry permanently.Known gap — please read
What clears the tab↔run binding in the first place is still unidentified. I found the
mechanism of the damage, not its origin.
This PR makes losing the binding recoverable and loud rather than fatal and invisible:
both repair paths log a warning naming the tab and the run. That warning is deliberately a
defect detector — if it shows up in normal use, something is still clearing bindings, and
the log will finally say where. It is not a root-cause fix and shouldn't be read as one.
Why the
@types/nodecommitvite.config.tsreadsprocess.env.TAURI_DEV_HOST, but no Node types were installed, sonpm run checkreports one error onmaintoday:Cannot find name 'process'. The repo'sown pre-commit hook runs that check, so it can't pass without this. Pinned to 22.17.0, the
Node version the Recall sidecar ships against.
CI's typecheck leg is
continue-on-error, so this doesn't change CI colour — it unblocksthe local hook, and the leg is now clean enough to ratchet to blocking in a follow-up.
Verification
npm run test(vitest — blocking in CI)npm run check(svelte-check)npm run lint(blocking in CI)cargo testnpm run verifydoes not pass, for a pre-existing repo-wide reason:format:checkreports ~288 unformatted files on
main. Every file this PR touches is Prettier-clean, andthat leg is
continue-on-errorin CI.Exercised by hand on Windows / Claude / signed-in: switch away mid-turn and return; let
a turn finish while away and return; watch the counter continue instead of resetting.
Not tested: macOS, Linux, or the Codex vendor path. The changes are vendor-agnostic by
construction — the diff adds no reference to
agentFor,currentProvider, or any vendorname — but I have not exercised Codex end to end.
On test coverage, honestly: the new tests cover the extracted pure helpers
(
turn-restore.ts,stripStreamingBlocks), including thespawningcase above. No test inthis repo imports a
.sveltefile, so the+page.sveltewiring that calls them is assertednowhere and was verified by hand only.
Scope
Only the general chat/tab fixes. These surfaced during unrelated feature work; that work is
deliberately not in this PR and stays on its own branch.