Repository navigation
fix(native-chat): resume structured chats cleanly after restart - #20509
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change removes generic provider-exit status rows from Codex settlement and filters retired rows during history rendering. It adds dead-generation settlement for unfinished work and interrupted turns. The adapter router now preserves stop proofs and coordinates global shutdown. Host teardown retries owed child wind-down, bounds eviction, validates lease fences, and acknowledges released sessions. Runtime and lifecycle tests cover these paths. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The restart regression tests can pass when history loading resets, leaving the intended restored-history behavior incompletely protected. This is a bounded test-coverage risk rather than a demonstrated runtime failure. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the problem, implementation, testing, scope, and known limitations. However, it does not provide the required linked issue, does not include both required before and after visual attachments, and omits the AI Disclosure section.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one design question and minor test-coverage notes.
Reviewed changes
- Retire owned provider children on expected teardown — new
evictOwnedStructuredAgentSessionsstops every host-owned child, settles it via a newsettle-dead-generationeviction step, keeps failed sessions inretainSessionIdsfor a retry, and drops the adapter's stop proof viaacknowledgeSessionRelease.tearDownRuntimenow flushes the host (and evicts) before theadapter.closeAll()backstop. - Router route/stop-proof state machine —
structured-agent-session-adapter-router.tstracks per-sessionlive/stoppedroutes, gatesacquirewhileclosing, drains in-flight acquisitions beforecloseAll, and publishes an O(1)allAdaptersClosedglobal proof. - Relocate the unexpected-exit row — removed the unconditional
Provider exited: <reason>status from the codex translator; the new dead-generation settlement writes the actionableUNEXPECTED_PROVIDER_EXIT_OUTCOMEonly when an in-flight response was actually interrupted, and settles expected closes silently. - Epoch-scoped tombstone migration (schema v3) — new
journal_epoch_migrationstable andJournalEpochMigrationRunneratomically tombstone historical syntheticrestart-eviction:<session>:<fence>rows once per epoch, without advancing activity or cursors; read-only and future schemas are untouched. - Tests — 18 files covering teardown ordering/retry, handoff acknowledgement, restart restore, unexpected exits, migration atomicity/idempotence, and router shutdown races.
I traced the teardown, migration, and unexpected-exit paths and could not construct a leak, a data-integrity defect, or a benign-restart row. The one thing worth a human decision is below.
ℹ️ Probe-proven restart settles a running turn with no user-facing explanation
When a host restarts and the owner probe proves the child gone (pid-absent / identity-mismatch) while the journal still holds a running turn, the retry path forces showUnexpectedExitOutcome: false and turnVerdictFromDeathEvidence yields unverifiable, so the turn is revised but no outcome row is written. This is deliberate and pinned by structured-agent-session-dead-generation-settlement.test.ts:79 ("without a technical chat row or fake end time"), and it is consistent with not fabricating an end time — but it means a crashed response produces a turn that silently changes state, while an observed provider death produces actionable copy. Flagging only so the product intent is explicit.
Technical details
# Silent settlement of probe-proven restart deaths
## Affected sites
- `structured-agent-session-settlement-retry.ts:77-79` — `showUnexpectedExitOutcome` is derived from the `provider-exit:` id prefix, so a `restart-eviction:` latch always passes `false`.
- `structured-agent-session-dead-generation-settlement.ts:129-137` — an `unverifiable` verdict has no status body, only a turn revision.
- `structured-agent-session-dead-generation-settlement.test.ts:79-113` — asserts no `status` item and no end time.
## Open question for the human
- Is a probe-proven crash of an in-flight response supposed to remain completely silent, or should it get a variant of `UNEXPECTED_PROVIDER_EXIT_OUTCOME` (without a fabricated `completedAt`)? If silent is intended, no change is needed.ℹ️ Nitpicks
cloneTombstoneMigrationState(journal-epoch-migration-runner.ts:88-97) shallow-copies the container maps, so clonedsubmissions/itemsshare their value objects. It is safe today because the runner only ever buildstombstonemutations (which touch only the cloneditems/tombstones), but a future item-mutation input would letacceptSubmissionFromProviderItemmutate live state before aROLLBACK.structured-agent-session-settlement-retry.ts:77-79reconstructs the show-outcome decision from the settlement id prefix instead of persisting whatunfinishedStructuredAgentSessionWorkWasInterruptedoriginally decided. The invariant that makes it safe holds today; worth a comment if a new latch id is ever added.JournalEpochMigrationRunner.runtakes aBEGIN IMMEDIATEwrite lock on every attach even when the marker already exists (read-only early-return aside) — a small cost on the read path.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Restarting Orca turned a resumable structured chat into a user-visible `Provider exited: recorded pid absent on host`. Quit never released the durable lease, so restart probed the recorded pid, adjudicated the session evicted, and wrote a synthetic status row against a chat that was perfectly resumable. The fix is the missing teardown phase plus the missing fence check: quit now evicts every provider child this host owns — stopping it, settling its journal and handing the lease back — and the release compare-and-swaps on the fence it expected. Restart then finds a released lease and reopens the chat silently. What the user sees is decided by the typed death evidence rather than the shape of a settlement id: only an `exit-observed` death writes copy, and that copy now carries its cause so an auth failure and an OOM kill do not read alike. The reassuring wording stays. Historical synthetic rows are filtered out of the render projection, which needs no schema change and leaves every real provider-exit row alone. Also: - Bound the new eviction phase well below the quit deadline; a quit that dies mid-eviction leaves the lease unreleased, which is the original bug. - Scope the interruption verdict to work that was mid-response. A provider that died while waiting on an approval interrupted nothing. - Keep host bookkeeping in step with the adapter: the provider-child flag clears when the child is proven stopped, not seven steps later. - Drop the router's duplicate shutdown gate and acquisition drain — both adapters already own theirs — and latch the router closed so a late acquire cannot fan a session back out to closed adapters. - Attach the real cause to the settlement failure a quit reports, and remove a recovery-ticket field that was hardcoded at its only construction site.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: cf96a8b6-c927-4bc7-9b03-ba21585bed50
📒 Files selected for processing (34)
src/main/codex/codex-structured-journal-settlement.tssrc/main/codex/codex-structured-journal-translation-settlement.test.tssrc/main/codex/codex-structured-journal-translation.test.tssrc/main/codex/codex-structured-session-background-tasks.test.tssrc/main/native-chat/agent-session-wire/agent-session-history-page.tssrc/main/native-chat/agent-session-wire/agent-session-restart-eviction-status-filter.test.tssrc/main/native-chat/agent-session-wire/agent-session-restart-eviction-status-filter.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter.tssrc/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.tssrc/main/native-chat/agent-session-wire/structured-agent-session-eviction.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-eviction.tssrc/main/native-chat/agent-session-wire/structured-agent-session-handoff-forward.tssrc/main/native-chat/agent-session-wire/structured-agent-session-handoff-types.tssrc/main/native-chat/agent-session-wire/structured-agent-session-handoff.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-handoff.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-journal-handles.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-lease-release.tssrc/main/native-chat/agent-session-wire/structured-agent-session-provider-restore.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-recovery-exits.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-refusal-retry.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-settlement-retry.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-settlement-retry.tssrc/main/native-chat/agent-session-wire/structured-agent-session-surface-lifetime.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-unexpected-exit.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-unexpected-exit.tssrc/main/runtime/agent-session-surface-release-transition.tssrc/main/runtime/structured-agent-session-runtime-exit.test.tssrc/main/runtime/structured-agent-session-runtime.ts
💤 Files with no reviewable changes (1)
- src/main/codex/codex-structured-session-background-tasks.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| { | ||
| name: 'evict-owned-sessions', | ||
| run: () => withPhaseTimeout(collaborators.evictOwnedSessions, CHILD_EVICTION_TIMEOUT_MS) | ||
| }, | ||
| { name: 'flush-event-sinks', run: () => collaborators.runtimeState.flushAllEventSinks() } | ||
| ] | ||
| } | ||
|
|
||
| export async function tearDownStructuredAgentSessionHost(input: { | ||
| phases: readonly StructuredAgentSessionTeardownPhase[] | ||
| sessions: Map<string, StructuredAgentSessionHostSession> | ||
| retainSessionIds?: ReadonlySet<string> | ||
| acknowledgeSessionRelease?: (sessionId: string) => void |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Fence timed-out session eviction before adapter shutdown.
If withPhaseTimeout times out, withTimeout does not cancel or join evictOwnedStructuredAgentSessions. Teardown then retains the session journal but continues to installed.adapter.closeAll(). StructuredAgentSessionAdapterRouter.closeAll() delegates to provider closeAll(), which enumerates the same provider session while disposeSession() may still be closing it. This can issue concurrent provider shutdown and settlement operations for one session.
Add a per-session fence that closeAll() honors, or join pending evictions before adapter shutdown.
| }) | ||
| await restarted.restoreReadableSessions() | ||
| const history = restarted.history({ sessionId: SESSION, direction: 'tail' }) | ||
| expect(history.ok && history.page.items.some((item) => item.body.kind === 'status')).toBe(false) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that each history request succeeds.
Both expressions evaluate to false when history.ok is false. The tests then pass without checking the history items.
Assert history.ok first. Inspect history.page.items only after that assertion succeeds.
Also applies to: 395-396
The read-time filter hid every status row carrying a `restart-eviction:` identity. That identity is still minted, so a genuine provider death settled under it would have been dropped from every rendered page. Match the retired `Provider exited` copy as well, so only the legacy rows are hidden. Three smaller corrections alongside it: - The settlement retry path now applies the same unfinished-work check the live exit path uses, so a provider that died waiting on an approval no longer gets told a response was in progress. - Bound the exit reason before composing the outcome copy, so a stderr dump in the reason cannot push the "you can continue" sentence past the row's byte cap. - Correct the teardown comment: tail rows are protected by eviction's own per-session ordering, and `closeAll` is a backstop for children eviction never took, including one whose eviction was refused.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 0df85175-8d5f-4a9d-af98-cf9d1ece6892
📒 Files selected for processing (8)
src/main/native-chat/agent-session-wire/agent-session-restart-eviction-status-filter.test.tssrc/main/native-chat/agent-session-wire/agent-session-restart-eviction-status-filter.tssrc/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.tssrc/main/native-chat/agent-session-wire/structured-agent-session-settlement-retry.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-settlement-retry.tssrc/main/native-chat/agent-session-wire/structured-agent-session-unexpected-exit.tssrc/main/runtime/structured-agent-session-runtime.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/native-chat/agent-session-wire/structured-agent-session-unexpected-exit.ts
- src/main/runtime/structured-agent-session-runtime.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
| * into the copy rather than left in the durable record nothing renders. */ | ||
| export function unexpectedProviderExitOutcome(reason?: string): string { | ||
| const detail = reason | ||
| ?.slice(0, MAX_UNEXPECTED_EXIT_REASON_CHARS) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim the exit reason before applying the bound.
Line 30 slices the raw reason before trim(). If a provider emits 512 leading spaces followed by OOM, detail becomes empty and the status loses the exit cause. Trim first, then apply the bound.
Proposed fix
const detail = reason
- ?.slice(0, MAX_UNEXPECTED_EXIT_REASON_CHARS)
- .trim()
+ ?.trim()
+ .slice(0, MAX_UNEXPECTED_EXIT_REASON_CHARS)
.replace(/[.\s]+$/, '')
There was a problem hiding this comment.
Important
One regression to fix before merging: clearing session.hasProviderChild at the proven stop (rather than after releaseLease) removes a partially-evicted session from the quit-path retry selection, so a mid-eviction failure can leave the durable lease naming a dead child — the exact condition this PR exists to clear.
Reviewed changes
- Replaced the epoch tombstone migration with a read-time
restart-eviction:status filter applied inbuildPage; deleted the migration runner/marker/repair modules, thejournal_epoch_migrationstable, and revertedJOURNAL_DB_SCHEMA_VERSIONto 2. - Made the user-facing outcome evidence-driven:
unexpectedProviderExitOutcome(reason)carries the cause, the retry derivesshowUnexpectedExitOutcomefrom the death-evidence verdict instead of the settlement-id prefix, andisInProgressItemno longer treats a pending approval/question as an interrupted response. - Cleared host provider-child bookkeeping at the proven stop step and made a quit's settlement failure carry its real cause.
- Bounded the
evict-owned-sessionsquit phase withwithPhaseTimeoutand prepopulated the retain set so a timed-out eviction does not have its journal closed underneath it. - Simplified the adapter router (removed the
closing/in-flight drain machinery, madeallAdaptersClosedfinal) and extended the recovery ticket with the released fence, dead acquisition generation, and settlement id. - Tests: reworked router shutdown tests, added idle-approval and evidence-discriminator settlement tests, added restart-eviction filter coverage, and added a runtime backstop-exit test.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the prior review's teardown-retry finding is still open and should land before merge.
Reviewed changes
- Confined the legacy
restart-eviction:read filter to rows whose copy starts withProvider exited(RETIRED_RESTART_EVICTION_TEXT_PREFIX), so a genuine provider death settled under that same identity with the current outcome copy still renders; a test now pins both sides. - Bounded the exit cause with
MAX_UNEXPECTED_EXIT_REASON_CHARS = 512before composingunexpectedProviderExitOutcome, so a stderr dump cannot push the actionable "You can continue…" tail past the row's byte cap; a 4k-frame-reason test asserts the tail survives. - Gated the pending settlement retry's
showUnexpectedExitOutcomeon bothverdict.state === 'interrupted'andunfinishedStructuredAgentSessionWorkWasInterrupted, matching the live exit path, so a witnessed exit that interrupted only a waiting prompt writes no copy; a test covers it. - Clarified the runtime teardown comment (no behavior change).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Electron QA on
After restart (same session, first turn only):
Second turn after restart:
|
The read-time filter that hides the retired `Provider exited …` rows ran on the way OUT of the page builder, after the paging math had already measured the unfiltered timeline. A backward window landing entirely on those rows returned an empty page that still reported `hasOlder: true` with a null `window.oldest`, so the renderer's backfill loop re-asked from the same anchor forever. Its only no-progress guard compares `window.oldest?.sequence` to the anchor, and `undefined === n` never breaks. The live subscription opens behind that loop, so the transcript never finished loading either. Filter where items ENTER the page pipeline instead: the reduced snapshot gets one renderable timeline, the forward path gets one renderable batch, and the window bound, effective limit, `hasOlder`, `window.oldest` and `nextCursor` are all computed over that single array. A window with nothing left behind it now reports end-of-history. Also restore the eviction retry contract. Clearing `hasProviderChild` as soon as the adapter proves the child gone is honest, but it is a different fact from the wind-down this host still owes. A retry after a step aborted between the two was reading "no child here" and skipping both the dead-generation settlement and the lease release the aborted attempt had promised to repeat. The obligation is now tracked separately and cleared only by a release that actually landed. And rename the filter to the copy it retires: it drops only rows carrying the retired `Provider exited` text, not restart-eviction status rows in general.
|
Electron QA at
After restart (same session):
Second turn after restart:
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Restored the teardown retry contract by tracking the owed provider-child wind-down separately from
hasProviderChild(owesProviderChildWindDown), so a step that aborts after the adapter proves the child gone no longer strands the durable lease. Selection includes owed sessions, the settlement step guards on the obligation, and the flag clears only once a release lands; a new test drives adrain-publishedfailure and then a successful retry. - Moved the retired-
Provider exitedread filter to where items enter the page pipeline (renderableTimelinefor tail/before,projectRenderableBatchforafter), so the window bound,hasOlder,window.oldestandnextCursorare computed over the same filtered array. A backward window landing entirely on retired rows now reports end-of-history instead of returning an empty page that still claims older history — the state that wedged the renderer's backfill loop. - Renamed the filter to
agent-session-retired-provider-exit-status-filterto reflect that it retires one copy, not therestart-eviction:identity.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
An eviction recorded "nothing owed" whenever it ran over a session with no provider child of its own, and the retry then read that record in preference to the child in front of it. A session suspended to an agent terminal is exactly that shape, and the trip back to native re-acquires into the SAME session object rather than replacing it, so the next close skipped both the dead-generation settlement and the lease release — leaving the record claiming a live owner this host had just stopped, and a pending send unsettled. The obligation is now derived the way the quit sweep already derived it, from one shared predicate: a live child always owes a wind-down, and a remembered `false` only carries the obligation forward, never cancels it. Also drops a memoization in the history page that could never hit. Its key was the snapshot's items array, which the reducer rebuilds on every `snapshot()` call, so each backward page allocated a fresh key; the one reader that does share a snapshot across pages reads forward and never calls it. The comment claimed a multi-page read filtered once, which was not true of either path. Tests: the handoff round trip that strands the lease, and the quit sweep picking up an eviction whose close retry never came.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run covered the delta from the prior review at 0301103 to 41798d5.
- Derived the owed wind-down from the live child —
evictHeldStructuredAgentSessionnow computes its obligation through a sharedowesProviderChildWindDown(session)predicate (hasProviderChild || owesProviderChildWindDown === true) instead of?? hasProviderChild. A childless eviction (e.g. over a session a TUI owns) could record a rememberedfalsethat then outranked a child re-acquired into the same session object on a handoff back to native, skipping both the dead-generation settlement and the lease release. The quit sweep now shares that one predicate, so the two call sites cannot drift. - Removed a history-page memoization that could never hit —
renderableTimelineno longer caches onsnapshot.items, which the reducer rebuilds on everysnapshot(); the old key was fresh per backward page, and the one reader that shares a snapshot across pages reads forward and never calls it. Behavior is unchanged and the misleading "filters once" comment is gone. - Tests — added a handoff round-trip test (to-TUI → restart with an unreachable terminal → failed close → to-native handoff → close) that leaves the lease stranded without the predicate fix, plus a quit-sweep test for an eviction whose close retry never came.
I ran structured-agent-session-surface-lifetime.test.ts (21 tests) and agent-session-retired-provider-exit-status-filter.test.ts (5 tests) at 41798d5; all green. Prior Pullfrog threads are resolved, and the fix closes the last open finding from earlier in this PR.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…rdown order retryUnexpectedExitSettlement, hasUnfinishedStructuredAgentSessionWork and isRetiredProviderExitStatusItem each have no consumer outside the file that defines them, so they no longer advertise an external contract. The quit-path phase list documents its order as load-bearing, but nothing asserted it. Pin the phase names so evict-owned-sessions cannot drift out of its slot between drain-attaches and flush-event-sinks.
Review statusReviewed across six rounds (adversarial review + readiness checklist at both ends + Electron QA). Every round but the last found a real defect in the round before it, so the diff below is materially different from what was first submitted. Current head: What changed during reviewThe journal migration and the It is replaced by a read-time filter. Two defects found and fixed in the replacement:
Also: the string-sniffing Evidence
Known limits
Follow-ups (not blocking, deliberately out of scope here)
|
`closeAll` cleared the route table and set one boolean, after which that boolean was the only surviving evidence about any session. Two call sites then spent it: `releaseAcquisition` and the stop path each turned a route-lookup MISS into reported success. Eviction reads a `true` from the stop path as proof the provider child is gone and releases the durable lease on it, so a session the router never routed could have its lease handed back on the strength of "I have no record, but everything is closed." Loss of contact is not evidence of process death. The fix keeps the evidence instead of the inference: adapter shutdown only resolves once every child is proven stopped, so `closeAll` now marks each routed session `stopped` rather than forgetting it. A routed session still answers `true` from its own retained proof; a session with no route answers `false`, which leaves it indexed for a real retry. `releaseAcquisition` drops its short-circuit and asks the adapters, which answer from their own session maps. The acquire-side latch is unchanged: once closed, the router stays closed and refuses new work. Behaviour that changed: a post-`closeAll` stop for a session the router never routed, or one the host already acknowledged as released, now reports unproven instead of proven. That matches what the same call already answered before `closeAll`, and no real flow reaches it — quit evicts every owned session before `closeAll` runs, and eviction only asks the adapter for sessions whose provider child this host acquired through the router.
…uction The retirement filter hides a status row on two facts: a restart-eviction item id and copy that opens with the retired prefix. The identity half is still minted today, so the filter cannot tell a new producer's row from the legacy row it exists to hide — any future writer of that copy would be dropped from every transcript with no trace. Until now that safety property lived only in a doc comment. Scan the shipped tree for a string literal that OPENS with the retired prefix, which is exactly what the filter's `startsWith` reads. Comments are stripped first, so prose about the retirement is not a producer, and the filter's own constant is exempt. Tests are excluded: writing the copy is how the filter is exercised.
Addendum — two follow-ups since the status comment aboveHead is now 1. The adapter router no longer reports a stop it cannot observe.
It mattered concretely:
This also removes an inconsistency that predates the PR: a post- 2. The retired-copy filter's safety property is now falsifiable. The filter hides a row only when it has both a Also corrected: the Review section above previously claimed a successful global close "uses an O(1) proof rather than retaining historical session IDs." Change 1 reverses that, and the body has been updated. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run covered the delta from the prior review at 41798d5 to fd9eeb96 (commits 3bb2ac71 and fd9eeb96).
- Stopped the router issuing a stop receipt it never observed —
stopSessionnow returnsfalsefor a session with no route instead of answering from theallAdaptersClosedflag,releaseAcquisitionfans out to the adapters again rather than short-circuiting on that flag, andcloseAllkeeps each routed session'sstoppedproof in the route map instead of clearing it. A retried teardown still getstruefor a session the router actually stopped, while an address it never saw gets loss of contact, not proof of a stop. - Added a retired-copy ratchet —
agent-session-retired-provider-exit-copy-ratchet.test.tsscans every non-test source undersrcand fails if any production literal opens withProvider exited, since such a row would be silently hidden byagent-session-retired-provider-exit-status-filter.tswhenever it also carries arestart-eviction:identity.
The router change restores the base semantics for an unrouted session (the intermediate commit had briefly returned the global close flag as a receipt). I traced the quit/retry ordering and every production writer of hasProviderChild/routes: evict-owned-sessions runs before closeAll on the first pass, a retried teardown hits the retained stopped route, and an abort-after-proven-stop retry skips the stop step via hasProviderChild === false — so no host-owned child loses its stop receipt or strands its lease. All prior review threads are resolved.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Electron QA at Seeded via
Assertion 3: Scrolled to oldest:
Genuine death (retired run between
Second live turn after restart:
|
Fix forward instead. The lifecycle change in this branch stops any new `Provider exited: <reason>` row from being written; rows a previous build already persisted stay in those transcripts and age out with them. A permanent read-time filter for a cosmetic, shrinking set was not worth its maintenance cost, and its paging seam was the only place a backward window could land entirely on hidden rows. Removes the filter module and its test, restores agent-session-history-page.ts to its pre-branch form, and drops the tests that only existed to prove the filter did not over-match or wedge the backfill loop. The copy ratchet stays and now carries the whole guarantee: with no filter in front of it, any production writer that resurrects the retired prefix reaches the user's transcript directly.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run covered the delta from the prior pullfrog review at fd9eeb96 to 6cc4b0f (the single commit 6cc4b0f).
- Reverted the read-time retired-copy filter — deleted
agent-session-retired-provider-exit-status-filter.tsand its test, dropped therenderableTimeline/projectRenderableBatchpaging seam, and restoredagent-session-history-page.tsto its pre-branch form (verified byte-identical to the merge base). Legacyrestart-eviction:rows carrying the bareProvider exited: <reason>copy render again by design. - Moved the whole guarantee onto the copy ratchet —
agent-session-retired-provider-exit-copy-ratchet.test.tsno longer exempts the deleted filter file; it fails on any production literal opening withProvider exited, and a repo-wide grep confirms no production writer remains.
The src/main/native-chat suite is green (144 files, 1156 tests), and both prior pullfrog threads are resolved.
ℹ️ The PR description still describes the removed read-time filter
The body's "What Changed", "Backward compatibility", and "Testing" prose still claim the legacy rows "are hidden at read time, in the render projection every history page and live batch funnels through", call the filter "permanent", and say the ratchet guards against rows "silently hidden by that filter". 6cc4b0f removed all of that, so the description now contradicts the code on exactly the point a reviewer is asked to trust. Worth updating before merge so the design decision stays legible.
Technical details
# Stale PR description after the read-time-filter revert
## Affected sites
- PR body, "What Changed" — the read-time-hiding paragraphs and the "read-only and permanent" sentence describe `agent-session-retired-provider-exit-status-filter.ts`, deleted in `6cc4b0f`.
- PR body, "Backward compatibility" — "only the render projection drops them" no longer holds.
- PR body, "Testing" — the ablation "removing the read-time filter fails both new projection tests (2 of 2)" is moot; those tests are deleted.
## Required outcome
- The description should state that already-written legacy rows stay visible, and that the copy ratchet is now the sole writing-side guard.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Electron QA at Fresh Codex structured chat (
Visual: the first turn took ~4m and Codex painted a After first restart (unfiltered):
Second turn after first restart:
After second restart (still unfiltered, both turns present):
|
ELI5
Restarting Orca no longer turns a resumable structured conversation into a scary provider/PID error. An idle conversation reopens quietly and stays sendable. If a provider really stops during an active response, the conversation still says so — and now says why.
Why
Quit never released the durable lease. The lease kept naming a provider child that the quit had already killed, so the next launch probed that recorded pid, found it absent, adjudicated the session evicted, and wrote a synthetic status row into the transcript of a chat that was perfectly resumable.
That is a lifecycle-ownership bug, not a presentation bug. The host owned the child; nothing handed the lease back when it stopped one.
What Changed
Two things were genuinely missing. The rest of the retirement sequence — gate acquisitions, drain in-flight work, stop the child, settle the journal, release, close — already existed on
mainas the per-session eviction step list; it simply was never run on the quit path, and its release had no fence check.evict-owned-sessions, which puts every provider child this host owns through that existing eviction sequence. Because that eviction is what stops the children, it runs before the adapter backstop closes rather than after, so each stop is a real receipt instead of a vacuous one from an already-closed router. The phase is bounded well below the global quit deadline; a quit that dies mid-eviction would leave the lease unreleased, which is the original bug.With those in place, restart finds a released lease and reopens silently.
What the user sees is now decided by the typed death evidence rather than by the shape of a settlement id:
exit-observeddeath writes user-facing copy. Apid-absentoridentity-mismatchadjudication — every unclean restart — writes none.The provider stopped while this response was in progress: <cause>. You can continue in this conversation.This is a fix forward. Once the lease is released correctly, no new row of this kind is written. Rows an earlier build already persisted are left exactly as they are: they stay in those transcripts and age out with the conversations that hold them. Nothing hides them at read time, nothing rewrites them, and there is no data migration or journal schema change — bumping the schema would latch older builds read-only on any journal a newer build had opened, which renders the whole transcript empty and unwritable.
A ratchet test scans the shipped tree so no future producer can resurrect the retired copy. With nothing filtering that string on the way out, such a row would land in the user's transcript directly, which is the reported symptom.
Smaller items in the same area: host bookkeeping clears its provider-child flag when the child is proven stopped rather than seven steps later; the settlement failure a quit reports carries its real cause; a recovery-ticket field hardcoded at its only construction site is gone; and the adapter router drops a duplicate shutdown gate and acquisition drain (both adapters already own theirs, and the retry-until-drained close loop already covers the second) while keeping its close memoization, its per-session stop receipt, and a new closed latch so a late acquire cannot fan a session back out to closed adapters.
Linked Issue
N/A — user-reported regression; no issue was supplied.
Visual Proof
Before: reopening a structured native chat after restart could add
Provider exited: recorded PID absent on hostto the transcript.After: the same structured conversation reopens without the internal diagnostic and accepts another turn.
Testing
pnpm test src/main/native-chat src/main/runtime— 919 files passed, 3 skipped; 9014 tests passed, 31 skippedpnpm tcpnpm run check:code-quality:changed— zero findings across all changed-code checks, re-run after commitpnpm formatmain, so the settlement-id string sniff decides the copy again, fails the idle-restart guard (3 of 21 in the surface-lifetime suite, includingsettles an idle absent owner without chat pollution) and the evidence-discriminator tests (5 of 13, includingreads the evidence, not the settlement id, when deciding to speak)RESTORE_READYProvider exitedorrecorded PID absent on hostmessageSTILL_SENDABLENot yet exercised: packaged
quitAndInstall, Claude structured chat, SSH, Windows, or Linux. The Electron run covered local macOS Codex and normal restart teardown/reopen.Review
Agent skill upstream boundary
Notes
Scoped to native structured-chat provider ownership and journal settlement. No new UI, styling, or wire opcode.
Checklist