Repository navigation
Make session recovery deterministic across restarts - #548
Merged
Merged
Conversation
Juliusolsson05
marked this pull request as ready for review
July 16, 2026 13:43
This was referenced Jul 22, 2026
Juliusolsson05
added a commit
that referenced
this pull request
Jul 22, 2026
Two orchestrated reviewers went over the first cut. Both confirmed the core
fix and the fullscreen-misattribution call (one proved React cannot remount
the children slot across a fullscreen toggle: same element type, same
position, only the style object differs). Between them they found two real
defects and four wrong claims, all fixed here.
The cold-start path was still armed. Fix 1 disarmed the timer only for
adopted backends; a restored agent lazy-woken on mount still took the
spawned path, where attach was chained behind the wake. That is the same
self-fulfilling trap the whole PR is about: the pane is blank, so a user
facing a trust prompt cannot answer the very prompt holding readiness
false, the 30s expires, the process is killed, and Retry respawns into the
same prompt. Attach now runs before and independently of the wake, and
retries once the wake produces a backend. Attaching early is free — main
returns null without taking a reference when no entry exists.
The `attached` flag as first written leaked a reference rather than fixing
one, which a reviewer proved by executing the component: mount, attach,
unmount before the IPC resolves gave {attach:1, detach:0}, where the old
unconditional cleanup gave {1,1}. Main takes the reference when its handler
runs, which can precede unmount, and cleanup has by then already run and
cannot release it — a flag it reads later cannot help, because it never
runs again. A late attach now releases itself. Verified back to {1,1} with
a throwaway test reproducing the reviewer's exact sequence.
Both needed `attachAgentPty` to distinguish "no backend to attach to" from
"attached, buffer happens to be empty" — it returned '' for both, so a
caller could neither retry (double-increment) nor report the failure. It
now returns null for the former, which also turns the silent-blank-pane
failure mode into a toast.
Corrected claims, including two this PR introduced:
- "Nothing re-publishes to unblock the poll" is false. publishPromptGate
emits on every transition into ready and setInputReadiness dedupes only
unchanged values, so answering the prompt does unblock the wait. The
timeout was guaranteed for one reason only: the blank pane. The wrong
causal story was baked into a code comment where it actively hid the
cold-start gap above.
- The unmatched detach cannot hurt the Spotlight remount case. React runs
a deleted subtree's cleanups before the new subtree's effects, so the
outgoing leaf always detaches first. It hurts a concurrent second
consumer instead — a debug panel on the same session, or two panes
rendering it through grid-related tabs.
- The plan's original refcount story described a leak that never existed;
the late attach was already guarded. Recorded as wrong rather than
quietly dropped.
- "Parsed-feed mode: not exposed" was wrong. TileLeaf.send wakes whenever
!inputReady, which is exactly the blocked state, so pressing Send with a
permission prompt showing took the same path.
- opencode was unmentioned; it latches readiness like Codex and is
likewise unaffected.
Also documented an accepted cost that was previously silent: `lifecycle:
'live'` means a RegistryEntry exists, not that the process is healthy, and
sessions.set() runs before await session.start(). A provider that wedges
after registering is now skipped rather than killed, so it no longer gets
the failed state that surfaces Retry. Killing a busy healthy agent is far
more common and far worse, so the trade stands — but #548's self-heal no
longer covers that class and nothing replaces it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Juliusolsson05
added a commit
that referenced
this pull request
Jul 28, 2026
Two orchestrated reviewers (bloat/test-quality and correctness) found 17 findings between them. Every claim below was verified against source before acting on it. THE OWNER'S CONCERN WAS CORRECT. streamingUnwind.test.ts contained three tests of a predicate defined inside the test file — they imported nothing from production and asserted `!false && !false === true`. Worse, they papered over a gap: unwindStreamingBaseline, the actual repair, had ZERO coverage, and the renderer test this PR's own plan promised was never written. The tautologies are deleted and streamingUnwind.renderer.test.tsx now drives the real hook. CORRECTNESS DEFECTS FIXED The unwind did not cover Codex AT ALL. Codex never goes through deliverPrompt — its submit is a sequence of raw `send` calls throwing plain Errors with no write evidence, so `failed` was always null and the guard never fired. A Codex pane with a dead backend reproduced the reported bug byte for byte. Fixed by counting sends that returned successfully; a blanket unwind would have been wrong because a bracketed paste issues several writes and only a later one may throw. Every healthy pane mounted a permanent 1 Hz timer. inputReadinessChangedAt is non-null for a READY pane too, so useElapsedSeconds ticked forever while resolveReadinessText returned null and nothing was displayed — fifteen panes, fifteen idle timers, fifteen re-renders a second. The text is now resolved without a clock first, which answers "is anything shown" before any timer mounts. That is the invariant useElapsedSeconds documents and the first version violated. The elapsed number could invert. Only one of six readiness-write paths stamped the clock, so a pane ready for forty minutes that was then woken rendered "starting agent · 40m 12s" on a backend two seconds old — the number whose only job is separating normal from wedged. All paths now restamp. The transcript line counted the wrong clock entirely: it read the readiness timestamp, so a pane whose readiness settled ten minutes ago showed "loading transcript · 10m" the instant a load began. Fabricated evidence in the one place (#283) where duration IS the diagnostic. transcriptStatusChangedAt added. The gate sampler could blind the incident journal. It re-recorded every non-ready gate every 5s forever — including `occupied` (a human draft) and `blocked` (an unanswered prompt), which are the app working correctly. A parked trust dialog emitted ~17,280 events/day, and reserveJournalBytes is a PERMANENT latch: once a run hits 50 MiB every later event AND incident is dropped, crash breadcrumbs included. Now skips waiting-on-a-human states and stops after ~2 minutes of stall, when the signal has saturated. gate.eval's dedup key dropped the field that distinguishes blocked states: `blocked` carries `condition`, not `reason`, so trust-dialog → permission-prompt compared equal and was silently dropped — in the one event whose stated purpose is recording what a gate is blocked on. It also survived id reuse, letting a fresh backend inherit a dead one's `since` and skip arming the sampler. wake.result reported ok:true at the recovery boundary, but recovery succeeding is not the wake succeeding — the readiness wait can still time out, kill the backend and fail the pane. The #548/#596 path was being journaled as a success. Three WHY comments stated the opposite of what the code does: publishPromptGate already dedupes by JSON.stringify, so gate.eval records transitions and the "60 Hz firehose" it claimed to prevent was impossible. In a repo whose comment policy is load-bearing, an inverted WHY is a defect. Also: submit.begin overloaded `ok` to mean "has images" (every image submit would have bucketed as a failure); the two AgentTerminalLeaf wake tags were swapped, so the two most similar sites carried each other's meaning; spawn.end duplicated recover.spawned's duration exactly and is deleted; three data keys had no emitter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Goal
Make renderer reload adopt existing main-owned backends and make full app restart recreate each backend exactly once under the persisted local session id.
Tracks #545.
What changed
Validation
Deliberately deferred
Provider-specific proof that a cold resume accepted the historical conversation id remains the focused follow-up documented in the plan. This PR fixes backend ownership, duplicate startup, session restoration, and truthful readiness without inventing provider lineage heuristics.