Skip to content

fix(desktop): finish regenerate archival before host shutdown - #1447

Merged
vastsa merged 4 commits into
vastsa:mainfrom
AR307:codex/pr-quit-terminal-persistence
Oct 7, 2026
Merged

vastsa merged 4 commits into
vastsa:mainfrom
AR307:codex/pr-quit-terminal-persistence

Conversation

@AR307

@AR307 AR307 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Quitting just after a regenerated answer finishes can dispose host-core while
session.saveActiveRevision is still pending. The turn has already left
activeTurns, and an empty message outbox does not prove the archive finished.
On upstream 0.17.0, a held archive RPC reproduces host disposal before archival.

Track terminal event-persistence promises. During quit, stop sidecar event
production and drain those writes, checkpoints and the outbox while host-core
is alive. The terminal-write wait uses the existing two-second quit budget;
unresponsive archival logs a warning and does not prevent exiting. The existing
single-RPC archive operation and stored transcript format remain unchanged.

Validation:

  • New quit regressions fail on unmodified upstream because host-core is already
    disposed while the archive is held. After the fix, success, storage error and
    unresponsive archive scenarios pass using real event persistence, disk outbox
    and shutdown wiring. Only Electron and the host RPC boundary are simulated.
  • Related persistence, checkpoint and regenerate tests: 30 passed on the
    standalone PR branch.
  • Desktop TypeScript check, Electron build and runtime bundle: passed on both
    the combined candidate and this standalone branch, whose sidecar uses the
    unchanged upstream runtime code.
  • node scripts/e2e-regenerate-quit.mjs: isolated Windows Electron, real
    sidecar and Rust host, controlled local provider. Regenerate through the
    context menu, quit immediately on the terminal event, restart, read both
    revisions, and verify there is no disposed-host archival error. All six
    assertions passed on this standalone branch as well as the combined candidate.

No user profile or paid model is used. macOS/Linux execution and GitHub CI have
not been run for this local draft.

Related work: #704 optimizes live branch storage, while #947 and #606 address transcript/outbox recovery. This change addresses terminal archival racing with host disposal during quit.

Validation candidate: fb48a3fb6d0a6941207fec2be09add64b3fcd397; upstream main: 72b5e826cb7a9928467091ccf745aa9b225eeb04.

AR307 and others added 2 commits October 7, 2026 14:46
A finished turn can leave activeTurns while its branch archive is still pending. Stop event production and await tracked terminal persistence with the host alive, preserving the existing bounded quit behavior when storage is unresponsive.

Cover successful, failed and stalled archives through the real outbox and quit handler, plus an isolated Electron regenerate and restart flow.
The summary is outside the quit-time persistence fix and duplicates the shared root path added by other ready maintenance PRs. Drop it so the PR stays scoped and later conflict resolution covers only the E2E plan entries.

@muzimu217 muzimu217 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified locally on head fb48a3fb6 (rebased on today's main, which now includes the merged #1435):

  • New regression suite shutdown-regenerate-persistence.test.mjs: 3/3 pass — success, storage-error and unresponsive-archive scenarios, driven through real event persistence, disk outbox and shutdown wiring.
  • Related existing suites show no regression: inflight-checkpoint 8/8, persistence-outbox 10/10, regenerate-branch-archive 4/4.

The implementation reads correct to me:

  • trackWrite keeps a Set of in-flight terminal writes and removes on both settle paths, so nothing leaks; flush() loops Promise.allSettled until the set drains.
  • The quit ordering makes that drain guaranteed to converge: sidecar event production is disposed before the drain, so no new terminal writes can arrive mid-flush. flushEventPersistence racing the existing QUIT_TURN_SETTLE_BUDGET_MS (warn-and-exit on timeout) preserves the bounded-quit contract instead of making quit hang on a stuck archive — the right trade for a shutdown path.
  • Outbox flush stays after the drain, so the archive that motivated this fix can't be lost between persistence and outbox.

One nit, non-blocking: Summary.md at the repo root is new in this PR. If it's meant as PR documentation it probably shouldn't ship in the repo — worth confirming whether upstream wants a root summary file at all.

Everything I can check locally is green; the remaining call is the maintainer's on the ADR 0060 wording.

@vastsa
vastsa merged commit 278e929 into vastsa:main Oct 7, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants