Skip to content

(restore): save the open sessions when the app closes - #479

Merged
devsuitup merged 5 commits into
devsuitup:mainfrom
abate:fix/save-state-on-exit
Oct 12, 2026
Merged

devsuitup merged 5 commits into
devsuitup:mainfrom
abate:fix/save-state-on-exit

Conversation

@abate

@abate abate commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Now: the late-save wait only

Main merged this PR's last round at f39d5ea (#555). This branch is main (421f77f, v0.0.94 released) plus:

  • A quit that joins a reload check the page has already approved waits for the page's late write. The renderer answers every unsaved-check-reason with unsaved-check-reason-done: after the write for an approved reload, at once otherwise. Main waits for it, bounded by the check timeout, and not at all for a declined or unanswered check.
  • While that wait runs, the joined check stays the one in flight, so a close, quit or install arriving meanwhile joins it instead of asking again. The wait ends at once when the renderer process is gone (render-process-gone, destroyed).
  • Changelog: a new entry under Unreleased / Fixed; the released v0.0.93 line is unchanged.

Commits, each fix after its own tests:

  • 3d9653b tests, a1ccc8e fix (late-save wait). Red: run 38145920957 (before the rebase, same tests). Green with the fix: run 38147545471.
  • 87920e2 tests (joined quit held; page gone), red run 38216641758 on exactly those two tests, all four test jobs. 9f9c57f fix, green run 38217312151 on every job.

Problem

The open working set (global.openWorkingSet, what restore reopens) is saved on a 500 ms debounce, so:

  1. the last open/close before a quit could be lost;
  2. before-quit kills every PTY while the renderer is alive; each kill arrives as process-exited → entry.closed → a scheduled persist that, if the app lingers past 500 ms (e.g. the ActivityWatch flush, up to 1.5 s), saves an empty set.

Fix

  • Renderer: once a close or quit is confirmed, the unsaved-check handler awaits flushStateForExit() (bounded to 2 s) before answering main. It cancels the debounce, writes the set at once, and blocks further working-set writes for 10 s (restored afterwards in case the exit does not happen, e.g. an installer that fails). A reload does neither; an exit during a restore writes nothing, keeping the previous run's set.
  • Main: appQuitting is set before before-quit kills the PTYs, and process-exited is no longer sent after it.

Docs: docs/session-restore.md → "Closing the app".

Tests

  • New test/exit-flush.test.js loads the shipped app.js functions: immediate write keeps other global keys; shutdown exits cannot empty the set; persistence resumes after the grace period; an exit mid-restore writes nothing.
  • test/dom-file-panel-unsaved-guard.test.js: confirmed quit flushes before answering, reload does not, cancel does not.
  • test/restore-live-elsewhere.test.js harness declares exitingApp.
  • The main-side appQuitting guard has no unit test.
  • Lint 0 errors.

🤖 Generated with Claude Code

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Review at 82cf905. Three points block; each was reproduced by the reviewer against the shipped code and I re-read the paths.

Blocking

  1. Quitting while the index is cold, or before the Restore prompt is answered, overwrites the previous saved set with [] (public/app.js flushStateForExit, persistWorkingSet({ final: true })). Nothing is open yet at that point, so the final write replaces a good set with an empty one. Fix: skip the final write when the restore has not resolved, or keep the saved entries that are not open yet. Test: quit with the saved set unrestored, then check the stored set.
  2. A session stopped on purpose during the restore comes back at the next start. If the user stops a session while the restore is still running and quits before it ends, the unchanged previous snapshot is what stays, so the stopped session is restored again.
  3. Changes made during the ten-second grace period of an aborted exit are never saved. schedulePersistWorkingSet returns early while exitingApp is set, and when exitingAppTimer expires nothing persists again. Fix: persist once when the grace period ends.

Conflicts with #441 (lazy restore)

  • The final serializer must keep the dormant entries (dormantWorkingSet), as #441's persistWorkingSet does; otherwise a quit drops them.
  • Entries the planner has not resolved yet need the same protection as in the #441 review.
  • A dormant entry dismissed during an aborted exit must not be lost. The branch also needs a rebase on current main.

Non-blocking

  • Windows logoff and shutdown bypass the new flush (unsaved-guard.js), so a pending debounce can still be lost.
  • The CHANGELOG entry must end with (#479).

79 tests in the seven related files pass locally on Node 24. Not verified: Node 20/22 with c8.

@jbr-sekoia

Copy link
Copy Markdown
Collaborator

Tested on Linux (Ubuntu, GNOME on Wayland), PR head 82cf905.

Setup. An isolated instance, with a throwaway HOME and its own data directory. The fixture is one git project with synthetic transcripts. A stub claude on PATH logs its argv and stays alive. The default mode is "Ask on startup".

Working

  • Quit inside the 500 ms debounce. Sessions 1–3 were open, session 1 was reselected, then the app quit at once. The stored set went from [1,2,3*] to [1*,2,3]. The relaunch asked "Restore 3 sessions from last time?", and Restore reopened all three.
  • Window closed inside the debounce. Session 3 was stopped, then the window closed 200 ms later. The stored set was [1*,2], and the process exited.
  • Tests. test/exit-flush.test.js and test/dom-file-panel-unsaved-guard.test.js: 14/14.

Blocking: "quitting before the Restore prompt is answered overwrites the saved set with []." Reproduces.

  • Before the prompt is answered. With a stored set of [1*,2], quitting while the prompt is showing leaves []. The next launch shows no prompt.
  • Cold-index variant. With mode auto and the stored set [1*,2], the session cache was wiped and the scan held cold by a FIFO standing in for a transcript. Quitting during the cold index wrote [] before the scan resumed. No session was resumed.

Not tried. Blocking points 2 (a session stopped during the restore comes back) and 3 (no save after the 10 s grace period).

@abate
abate force-pushed the fix/save-state-on-exit branch from 82cf905 to 00783c2 Compare October 8, 2026 09:33
@abate

abate commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks, and thanks @jbr-sekoia for the reproduction. Rebased on current main (eaf59ff); the fixes are in 00783c2, on top of the rebased original commit.

Blocking

  1. Quitting during a cold index or before the prompt is answered writes [].
    • Main now has pendingRestoreEntries(), which keeps three kinds of entry: those the planner has not resolved, those restoreAwaitingConsent offers, and those restoreInFlight is still starting.
    • The final write goes through persistWorkingSet, so it keeps all three at their saved place.
    • A quit before restoreWorkingSet has even read the saved set writes nothing (savedWorkingSetRead).
    • Tests: a cold index ([a, b] kept), the prompt unanswered ([a, b] kept), and before the read (no write).
  2. A session stopped during the restore comes back.
    • flushStateForExit no longer returns early while restoringWorkingSet is set. The final write records the open sessions, without the closed ones, plus what the restore still holds.
    • Test: a restored, b restored then stopped, and c still in flight. The stored set is [a, c].
  3. Changes during the grace period are never saved.
    • A persist or a scheduled persist that is skipped while exitingApp is set now records that it was skipped. When the 10 s grace period ends, the set is written once, and only if something asked for a save.
    • Tests: a change during the grace period is written when it ends, and with no change nothing is written again.

Conflicts with #441. I checked them in a throwaway merge of both branches, where both test suites pass (64):

  • The final write uses the same body as persistWorkingSet, so it keeps the dormant entries, and the unresolved planner entries via pendingRestoreEntries.
  • dismissDormantSession during an aborted exit calls persistWorkingSet. That call is recorded as skipped and written when the grace period ends. A test of this in the merged branch passes.
  • Whichever PR merges second has to add the other's variables to the declarations lists of test/restore-lazy.test.js, test/exit-flush.test.js and test/restore-pending-persist.test.js. No code change is needed.

Non-blocking

  • Windows logoff and shutdown: query-session-end and session-end still approve the quit without a dialog. Main now also sends exit-flush, and the renderer writes the set without waiting for an answer. This is best effort, since Windows may end the app before the write lands; docs/session-restore.md says so. The unsaved-guard test checks that the message is sent and no dialog is shown.
  • CHANGELOG: the entry now ends with (#479).

docs/session-restore.md ("Closing the app") describes the kept entries, the read guard, the save after the grace period and the logoff path.

Each fix was removed in turn, and each removal fails a test. The related suites pass (60). On the full suite, the only failures are the CLI-state canary, test-pr and sandbox-wrapper tests, which fail the same way on main in my sandboxed environment (the sandbox ones are fixed in #483).

@jbr-sekoia jbr-sekoia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review at 00783c2 (rebased on eaf59ff). The three blocking points and the Linux reproduction are fixed in the code. Two of the new guards can be deleted with the suite still green, and one of them is the line that makes the final write happen at all.

Previous points

  1. Quitting during a cold index or before the Restore prompt is answered: resolved. The final write goes through persistWorkingSet, which keeps restorePlanner.pending() and restoreAwaitingConsent. A quit before restoreWorkingSet has read the saved set writes nothing (savedWorkingSetRead). These are the two cases from the Linux comment. I traced them in code and they are covered by test/exit-flush.test.js, but I did not repeat the live run.
  2. A session stopped during the restore comes back: resolved. flushStateForExit no longer returns early while restoringWorkingSet is set. The stopped entry is closed, and it has already left restoreInFlight, so it is left out. Adding the old early return back fails "an exit during a restore keeps the sessions not started yet and leaves out one stopped".
  3. No save after the grace period of an aborted exit: resolved for a scheduled save. Removing the persist when the grace period ends fails a test, and so does removing the flag in schedulePersistWorkingSet. See Blocking 2 for a direct persistWorkingSet() call.
  • #441: a throwaway merge of 00783c2 and f4bd11b conflicts only in test/restore-pending-persist.test.js (the declarations list). After dormantWorkingSet is added to test/exit-flush.test.js and exitingApp/persistSkippedWhileExiting to test/restore-lazy.test.js, 73 tests pass across the restore, exit-flush and unsaved-guard files. The final write includes dormantWorkingSet.
  • CHANGELOG: ends with (#479).

Blocking

  1. Deleting savedWorkingSetRead = true from restoreWorkingSet (public/app.js) leaves every related test green. The setup in test/exit-flush.test.js sets the flag itself. With that line gone, flushStateForExit never writes on any quit, so the last change before a quit is lost again, and nothing fails. Fix: one test that calls the shipped restoreWorkingSet (mode off is enough), then flushStateForExit, and checks that the set was written.
  2. The quit answer is not shown to wait for the write (public/file-panel.js, unsaved-check handler). If await Promise.race([window.flushStateForExit(), bound]) is replaced by a call that is not awaited, test/dom-file-panel-unsaved-guard.test.js still passes. The stub records answers.length when it is called, not when it resolves. With that mutation, main gets unsaved-check-result while the write is still in flight and starts killing the PTYs and closing the DB. That is the race this PR exists to remove. Fix: make the stub return a promise the test resolves by hand, and assert that no answer is sent before it resolves.

Non-blocking

  • The skip flag in persistWorkingSet is never tested (public/app.js). Removing persistSkippedWhileExiting = true from its early return leaves the suite green. Two callers depend on it: the persist after runRestore (auto mode and the Restore button), and dismissDormantSession once #441 is merged. Without the flag, a restore that finishes during an aborted exit is not saved until the next change.
  • appQuitting in main.js has no test. Removing the guard, or the assignment, leaves the suite green. The PR body says so.
  • Removing onExitFlush registration in public/app.js leaves the suite green. The main side of the Windows logoff path is tested and the renderer side is not. That path is best effort, as the docs say.
  • A cancelled Windows logoff turns off the flush for later quits (unsaved-guard.js). endSession calls approveQuit(), and quitApproved stays true. If the logoff is cancelled, every later close or quit skips unsaved-check, so flushStateForExit never runs and a pending debounce can be lost. This existed before the PR, but the new flush depends on that check.

Tests: 89 pass in the 8 restore and unsaved-guard files at 00783c2 (Node, SWITCHBOARD_TEST_CONCURRENCY=1). Mutations killed: the read guard, the pending entries in the held list, the early return during a restore, the persist at the end of the grace period, the scheduled-save flag, the final bypass, the reload and cancel conditions, the exit-flush send. Not verified: a live run at the new head, Windows logoff, Node 20/22 with c8.

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Review at 00783c2, rebased on current main. Thanks: the three earlier blockers are fixed (quitting while the index is cold or before the Restore prompt is answered no longer writes [], a session stopped on purpose no longer comes back, and changes suppressed during the grace period of an aborted exit are saved), Windows shutdown now dispatches a best-effort flush, and the changelog ends with (#479). 89 tests in eight related files pass locally on Node 24. One new point blocks; the reviewer executed it against the shipped handler and guard.

Blocking

  1. A Quit or Close that joins an unanswered Reload check skips the flush (public/file-panel.js ~184, unsaved-guard.js ~27 and ~63). The guard shares one in-flight request. If a Reload check is waiting for the user and a Quit or Close arrives, it joins that request, which still says reload; the renderer therefore does not run the exit flush, while main proceeds with the exit. In the executed probe: zero writes, the saved set stayed [a] while the open sessions were [a, b]. Fix: when a later request with a different reason joins an in-flight check, carry the stronger reason (exit) to the renderer, or run the flush on the main-side decision to exit and not on the renderer's reading of the reason. Test: start a reload check, send a quit before it is answered, answer, and expect exactly one flush with the final set.

Integration with the other open PRs (to settle before merge)

  • #441: the final serializer has to keep the dormant, the unresolved planner and the in-flight restore entries, and must not revive a session stopped on purpose.
  • #478: the flush has to run after its final close confirmation, and the shared Reload/Exit intent has to resolve the same way in both.

Non-blocking

  • Quitting before the restore ends preserves the pending sessions but clears their active marker, so the next restore selects the last session (public/app.js ~205).
  • The immediate flush stub in test/dom-file-panel-unsaved-guard.test.js (~243) does not notice removing the await; the main-side notification suppression and the shutdown subscriptions are untested.
  • Remove the new inline rationale comment at unsaved-guard.js ~85.

Not run: Electron, a real shutdown, Node 20/22 with c8, lint, the full suite, fresh CI. CI on this head is not approved yet.

@abate
abate force-pushed the fix/save-state-on-exit branch from 00783c2 to 71ea998 Compare October 9, 2026 14:00
@abate

abate commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks to both of you. Rebased on main at 805d064, the last commit before e329a33: my token cannot push a branch that contains the workflow change of #498, so the branch is three commits behind main (e329a33, #507, v0.0.90), none of which touches these files. The fixes are in 71ea998, on top of the two rebased commits.

Blocking

  1. A Quit or Close joining an unanswered Reload check (devsuitup).
    • The guard records the reason of the check in flight. When a quit or close joins a reload check, main sends unsaved-check-reason (id, 'quit') once, and the renderer acts on the reason it holds when the user answers. So the answer waits for the final write, as for a plain quit.
    • Tests: in test/unsaved-guard.test.js, a reload check, then a quit and a close: one question, one upgrade, one quit; a reload joining a quit check leaves it a quit. In test/dom-file-panel-unsaved-guard.test.js, a reload check with a dirty editor, then the upgrade, then Discard: exactly one flush, and a later reload does not flush.
  2. savedWorkingSetRead = true untested (jbr-sekoia). A new test runs the shipped restoreWorkingSet with the mode off, then flushStateForExit, and expects the set written. Deleting the line fails it.
  3. The immediate flush stub (jbr-sekoia). The stub now returns a promise the test resolves by hand; the test asserts that no answer is sent before it resolves, and one after. Dropping the await fails it.

Non-blocking

  • The skip flag on a direct save. Tested: a direct persistWorkingSet() during the grace period writes nothing, and the set is written when the grace period ends.
  • onExitFlush registration. The test runs the registration line from the shipped app.js against a stub API and checks that the event writes the set.
  • A cancelled Windows logoff. query-session-end now approves quits for 60 s only (LOGOFF_CANCEL_MS). session-end keeps the approval. Electron has no event for a cancelled logoff, so the approval simply runs out: later quits ask about unsaved edits again and write the set. Tested both ways.
  • Active marker. While the restore has not opened the saved active session, the write keeps the marker on it and clears it on the open sessions. Once it is open, the live choice counts as before. Tested both ways.
  • appQuitting is still untested: it sits inside the before-quit handler and the PTY exit closure in main.js, which the tests do not load. I did not extract it for this.
  • Comment at unsaved-guard.js ~85: removed.
  • Docs: docs/session-restore.md ("Closing the app") and .ai/contexts/viewer-panel.md describe the reason upgrade, the active marker and the logoff limit.
  • test/schedule-live-resume.test.js, new on main, loads persistWorkingSet; it now declares exitingApp and persistSkippedWhileExiting.

Integration with #441 and #478. The dev branch #480 combines the three. There, the unsaved-check handler asks the close question of #478 first and runs the flush only after it is confirmed, and the final write holds dormantWorkingSet next to the skipped and pending entries, with stopped (closed) sessions left out. I will rebuild #480 on these heads.

Mutations, one at a time; each turned at least one test red: deleting savedWorkingSetRead = true, the skip flag, the onExitFlush call, the await on the flush, reading the upgraded reason, recording it in the guard, both halves of the active-marker change, the logoff revert and keeping the approval on session-end. Two survive, both in the renderer's reason map: dropping the has(id) check and the delete. Each only lets the map keep an entry for a check that has already ended, which no later check reads, since ids are not reused.

Lint: 0 errors. On the full suite (Node 22), the only failures are the CLI-state canary, test-pr and sandbox-wrapper tests, which fail the same way on main in my sandboxed environment. Not run: Electron, a real Windows logoff, Node 20 with c8.

@abate
abate requested a review from devsuitup October 9, 2026 14:00

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Review at 00783c2, rebased on current main. Thanks: the three earlier blockers are fixed (quitting while the index is cold or before the Restore prompt is answered no longer writes [], a session stopped on purpose no longer comes back, and changes suppressed during the grace period of an aborted exit are saved), Windows shutdown now dispatches a best-effort flush, and the changelog ends with (#479). 89 tests in eight related files pass locally on Node 24. One new point blocks; the reviewer executed it against the shipped handler and guard.

Blocking

  1. A Quit or Close that joins an unanswered Reload check skips the flush (public/file-panel.js ~184, unsaved-guard.js ~27 and ~63). The guard shares one in-flight request. If a Reload check is waiting for the user and a Quit or Close arrives, it joins that request, which still says reload; the renderer therefore does not run the exit flush, while main proceeds with the exit. In the executed probe: zero writes, the saved set stayed [a] while the open sessions were [a, b]. Fix: when a later request with a different reason joins an in-flight check, carry the stronger reason (exit) to the renderer, or run the flush on the main-side decision to exit and not on the renderer's reading of the reason. Test: start a reload check, send a quit before it is answered, answer, and expect exactly one flush with the final set.

Integration with the other open PRs (to settle before merge)

  • #441: the final serializer has to keep the dormant, the unresolved planner and the in-flight restore entries, and must not revive a session stopped on purpose.
  • #478: the flush has to run after its final close confirmation, and the shared Reload/Exit intent has to resolve the same way in both.

Non-blocking

  • Quitting before the restore ends preserves the pending sessions but clears their active marker, so the next restore selects the last session (public/app.js ~205).
  • The immediate flush stub in test/dom-file-panel-unsaved-guard.test.js (~243) does not notice removing the await; the main-side notification suppression and the shutdown subscriptions are untested.
  • Remove the new inline rationale comment at unsaved-guard.js ~85.

Not run: Electron, a real shutdown, Node 20/22 with c8, lint, the full suite, fresh CI. CI on this head is not approved yet.

@abate

abate commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on current main (75ab0ad), now that my token can push a branch that contains the workflow change of #499. New head 8bf1c90; the patch is the same as 71ea998, with no conflicts. Full suite and lint at the new head: only the failures this sandboxed environment has on main.

@abate
abate force-pushed the fix/save-state-on-exit branch from 71ea998 to 8bf1c90 Compare October 9, 2026 14:36
@abate
abate requested a review from devsuitup October 9, 2026 14:36
@devsuitup

Copy link
Copy Markdown
Owner

Thanks for the rebase. git range-diff of 71ea998 against 8bf1c90 shows the same patch (only context lines move), so the findings of the review at 71ea998 stand.

One thing the rebase did not fix: the (#479) changelog line is still under ## v0.0.90 — 2026-10-09, a released section, in CHANGELOG.md at 8bf1c90. Now that you can push on top of current main, please move it under ## Unreleased (same subsection).

@abate
abate force-pushed the fix/save-state-on-exit branch from 8bf1c90 to 4a84554 Compare October 10, 2026 09:04
@abate

abate commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on current main (a02f96c, v0.0.92); new head 4a84554. The changelog line is now under ## Unreleased → ### Fixed.

The rebase met #520 in persistWorkingSet: the held list keeps #520's attached continuation entries (keepAttached) next to this PR's not-started restore entries. When a not-started entry holds the saved active marker, it keeps it. Otherwise an attached entry is active when it is the shown session, as on main. test/continuations-harness.js declares this PR's two flags. CI on 4a84554: Test (Node 20/22 on Linux and Windows, e2e, lint, changelog) is green.

A note on the review at 71ea998: its text is the review of 00783c2 ("Review at 00783c2…"), and its points were answered in the previous reply. At this head they are covered by:

  • A quit or close joining an unanswered reload check: test/unsaved-guard.test.js, "a quit that joins an unanswered reload check tells the renderer it is now an exit, once", and test/dom-file-panel-unsaved-guard.test.js, "a reload check that a quit joins before it is answered flushes once".
  • The flush stub that resolved at once: "a confirmed close or quit answers only once the app state is written; a reload does not flush" resolves the write by hand and checks that no answer is sent before.
  • The active marker when quitting before the restore ends: test/exit-flush.test.js, "an exit before the restore ends keeps the saved active session marked".
  • The comment in unsaved-guard.js: removed.
  • Integration with (restore): add Restore on click startup mode #441 and (window): ask before a window close ends the app #478: resolved in the combined branch [DO NOT MERGE] dev: main + #441 for local testing #480 (dormant and not-started entries held, the close question before the flush, one shared reason).

Full suite locally: only the failures this sandboxed environment has on main. Not run: Electron, a real Windows logoff.

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Re-review at 4a84554 — approve, with non-blocking points. No defect found in the code that blocks; nothing merged is reverted (#518/#520 runRestore untouched, keepAttached kept; notStarted drops none of its entries). Base a02f96c, mergeable; CI green on the exact head (runs 38040124304 / 38040124329); changelog under Unreleased / Fixed.

Previous findings — all closed except one:

Finding Status
Quit during a cold index / before the prompt writes [] closed (exit-flush tests 4–6, fail on main)
Session stopped during restore comes back closed (test 7)
Changes during the grace period never saved closed (tests 8, 10)
Windows logoff bypasses the flush closed, best effort (unsaved-guard.js:90-102, guard test 14, exit-flush 12)
Cancelled logoff turns the flush off closed (LOGOFF_CANCEL_MS, guard test 18)
Quit joining a reload check skips the flush closed (guard 16, dom 10), residual race below
Active marker cleared on an early quit closed (exit-flush 11)
savedWorkingSetRead, await on the flush, skip flag, onExitFlush untested closed — mutations deleting each now fail a test
appQuitting untested still open (N1)
#441 / #478 integration not applicable until one of them lands; the second to land handles it

Red-first: every commit on the branch ships tests with its fix, so CI never shows the new tests red. Replaying the PR's test files on top of main (Node 22) does fail them — exit-flush 12/12, dom-file-panel-unsaved-guard 9 and 10, unsaved-guard 14, 16, 18 — so I am not holding the PR on it; please keep the tests-first commit for new fixes, as #538–#540 do.

Non-blocking

  1. main.js:2442, :3538: appQuitting has no test (0 hits in test/ and e2e/); the renderer half (exitingApp) is covered, so a regression on the main side would only be caught by the 10 s grace.
  2. public/app.js:209: the #520 rule (a not-started entry holding the saved active marker keeps it over a shown keepAttached entry) is unpinned — marking both active leaves 137/137 related tests green.
  3. public/file-panel.js:164-165 with unsaved-guard.js:32-37 (plausible): if the renderer has answered the reload check but main has not processed it yet, the id is already deleted and the unsaved-check-reason upgrade is dropped, so the close/quit skips the flush. IPC latency only; reachable from a programmatic quit (updater install).
  4. unsaved-guard.js:35, :99: the two rationale comments on empty catches go against the comment rule (main.js uses bare catch {}).
  5. Pre-existing, plausible: a quit joining a reload check still reloads before quitting, and the reloaded page starts with exitingApp=false.

Merge order with #540: they conflict (unsaved-guard.js ask/inflight and exports, tests, CHANGELOG, viewer-panel.md), and #540's fixed 3 s bound can destroy a busy page before this flush runs; whichever lands second needs a fresh look.

Not run here: a live Electron quit, a real Windows logoff, a macOS dock close.

@abate

abate commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

Non-blocking points addressed at b41237a (tests first: 4e2eefd, run 38068143104 red on all four test jobs on exactly the two new tests; b41237a run 38068446906 green on every job).

  1. appQuitting: test/exit-flush.test.js "main stops reporting session exits once the quit starts killing them" — source-text, the house pattern for main.js: process-exited is sent only under !appQuitting, and before-quit raises the flag before the first killPty. Dropping the guard, or raising the flag after the loop, fails it.
  2. (restore): follow a session's continuation instead of resuming the pre-fork transcript #520 rule: "a not-started entry holding the saved active marker keeps it over a reattached session shown meanwhile" (exit-flush.test.js). Marking both active fails it.
  3. Upgrade after the answer: the renderer remembers a reload check it answered yes (answeredReloads, public/file-panel.js); an unsaved-check-reason that reaches it after the answer writes the app state then (best effort, like the logoff path), once, and not for a reload answered no. Test in dom-file-panel-unsaved-guard.test.js; each part of the condition fails it when mutated.
  4. The two rationale comments are gone (bare catch {}).
  5. A quit joining a reload check no longer reloads first: will-prevent-unload keeps the check it joined and skips the reload when its reason became an exit (unsaved-guard.js, test "a reload check that a quit joined quits without reloading first"). This is also what lets point 3's flush run in the page that answered.

Docs: docs/session-restore.md and .ai/contexts/viewer-panel.md say both.

Merge order: this PR before #540/#478; they get rebased on it, keeping this flush after confirmWindowClose.

@abate
abate requested a review from devsuitup October 10, 2026 16:48

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Re-review at b41237a — approve (2 minor, 1 nit, non-blocking).

Red-first: 4e2eefd (tests only) fails all four test jobs (run 38068143104) on exactly the two new behaviour tests, both on assertions; b41237a green on every check (runs 38068446906 / 38068446891). Locally (Node 22) unsaved-guard, dom-file-panel-unsaved-guard and exit-flush: 45/45.

Previous point Status
appQuitting untested closed, weakly — test/exit-flush.test.js:209 matches main.js text (nit below)
#520 active-marker rule unpinned closed — exit-flush.test.js:198 asserts [a,false],[b,false],[c,true]
Reason upgrade lost after the renderer answered closed, best effort — file-panel.js:153-160 answeredReloads + fire-and-forget flush; covered, including no flush for a reload answered no
Rationale comments on empty catches closed — bare catch {}
A quit joining a reload check reloads first closed — unsaved-guard.js:125-129 checks check.reason !== 'reload', with a test

Minor

  1. unsaved-guard.js:126-129 (plausible): a reload request arriving while a quit/close question is open is now dropped after a yes (it went ahead at 4a84554). Right for a quit or close; but if the yes comes from confirmQuit (updater install, main.js:3297) and quitAndInstall then does not quit, the reload is lost and the page stays in exit mode for 10 s with saving stopped. "a reload joining a quit check leaves it a quit" only checks the channel; asserting wc.reloads === 0 after a yes would pin the behaviour.
  2. file-panel.js:158 (plausible): when the page had already answered the reload, the flush is fire-and-forget while main quits at once, so the save races window destruction — documented as best effort. A narrower pre-existing window remains (main already processed the reload answer, the quit's question reaches a reloading page, 2.5 s timeout, quit without saving).

Nit: test/exit-flush.test.js:209-218 matches the text of main.js; a behaviour-preserving refactor breaks it and a text-preserving regression passes.

Conflicts: with #540 now only test/unsaved-guard.test.js (both append tests); with #478 still heavy (unsaved-guard.js 6 hunks, public/file-panel.js, CHANGELOG, viewer-panel.md, both guard test files).

Mergeable on a02f96c, changelog under Unreleased / Fixed, only one-line pointers. Not run: a live Electron quit/reload, a real Windows logoff, an updater install.

@devsuitup
devsuitup dismissed jbr-sekoia’s stale review October 11, 2026 13:59

Superseded: each item of this review was verified closed at f39d5ea, with a test that fails when its line is removed (B1 app.js:354, B2 file-panel.js:173, NB1-NB4). See the review comment on this PR.

abate added a commit to abate/switchboard that referenced this pull request Oct 11, 2026
…d active marker

Tests for the last review round of devsuitup#479, which devsuitup#548 did not merge: a quit
that joins a reload check the page already answered still writes the app
state, and a reload check a quit joined quits without reloading first.
Two more pin existing behaviour: main stops reporting session exits once
the quit starts killing them, and a not-started entry holding the saved
active marker keeps it over a reattached session shown meanwhile.
@abate
abate force-pushed the fix/save-state-on-exit branch 3 times, most recently from f54f3fa to 91c13ef Compare October 11, 2026 14:07
devsuitup added a commit that referenced this pull request Oct 11, 2026
…sions when the app closes, last review round) (#555)
@devsuitup

Copy link
Copy Markdown
Owner

@abate f39d5ea is on main: #555 merged it with a merge commit at 14:13Z. The only conflict was the CHANGELOG with #477, and both lines are kept under Unreleased.

Your new head 91c13ef sits on 2bae3ce. Against main, it now carries only the tests for the late save after an answered reload, for the minor point in my review. I'll review that round as its own delta once the fix is pushed. It is not needed for v0.0.93, so it will go into the next release.

Showing the test red locally is enough now. No red CI run is needed, so you can push the tests and the fix together.

abate pushed a commit to abate/switchboard that referenced this pull request Oct 11, 2026
… open sessions when the app closes, last review round)

Merged onto main after devsuitup#477: the only conflict was the CHANGELOG, where both PRs moved their stale v0.0.90 entry to Unreleased; both lines are kept under Unreleased and neither under v0.0.90.
@abate
abate force-pushed the fix/save-state-on-exit branch from 5920379 to 1dfff09 Compare October 11, 2026 14:24
@devsuitup

Copy link
Copy Markdown
Owner

Review of 5920379 (the late-save round): changes requested. One rebase, plus one minor point.

  • Rebase. bd66b9f and ce3be2d are exactly what landed through (restore): save the open sessions when the app closes, last review round (#479) #555 (git diff origin/main ce3be2d is empty). Kept next to that merge, they give 7 conflicts, so CI never ran on this head. Rebasing onto origin/main without them is clean (merge-tree from ce3be2d: 0 conflicts).
  • Red then green, verified locally. At 91c13ef 2 tests fail on real assertions (unsaved-guard.test.js:309, dom-file-panel-unsaved-guard.test.js:330), the same two as CI run 38145920957. At 5920379 all 58 pass.
  • Minor, a regression from this round, reproduced.
    • Where: unsaved-guard.js:61-66 with :85. entry.finish clears inflight when the answer arrives, but quitApproved is now set only once the page reports its save done, up to 2.5 s later.
    • What happens: a window close in that gap is not joined and starts a second check. A scratch replay shows unsaved-check 1 reload → unsaved-check-reason 1 quit → unsaved-check 2 quit.
    • Effect: a second dialog can show, and two flushStateForExit run at the same time.
    • Fix: keep the joined check visible to joiners until wait.done, or have the close handler return while the quit is asking, and test it.
  • Nit. When the renderer is gone (render-process-gone or destroyed), main still waits the full 2.5 s for a done message that cannot come. It is bounded.
  • Holds. The wait cannot hold a quit for ever, the PR adds no comments, and lint has 0 errors.

@abate
abate force-pushed the fix/save-state-on-exit branch from 1dfff09 to 7b3fdbb Compare October 11, 2026 14:31
@abate

abate commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up to the 13:48 minor (a quit joining a reload the page already approved). Rebased on main 24ac041 (#555 merged f39d5ea); this PR now carries only these two commits.

Was there a gap? Yes. Main processes the renderer's answer to the reload, resolves the joined quit, and calls quit() at once. The renderer's late write (answeredReloads) starts only when the upgrade reaches it, and it needs two IPC round trips (getSetting, then setSetting). The window closes in between, because will-prevent-unload is approved and nothing waits. #540's destroy path does not change this; the window is closed normally before the write lands.

Fix:

  • The renderer answers every unsaved-check-reason with unsaved-check-reason-done (id).
    • For a reload it has already approved, it sends that after the write ends, on success or failure.
    • Otherwise it sends it at once, since a check still being asked writes before it answers.
  • Main holds a joined quit until that report arrives.
    • The wait is bounded by the check timeout (2.5 s).
    • There is no wait when the check is declined or ended unanswered, so a hung page is not waited for twice.

Tests first:

  • 91c13ef, run 38145920957, red on all four test jobs on exactly the two new tests:
    • a quit that joins a reload check waits for the page to finish the save that upgrade asks for, but not for ever (bounded wait, declined, hung);
    • the page tells main when the save a late upgrade asked for is done, and at once when it has none to do.
    • The rebased 8e4cc64 has the same tree as 91c13ef.
  • 7b3fdbb, run 38147545471, green on every job.
  • Each of the six single mutations of the new lines fails a test.

The changelog line is extended, since the check requires an entry. docs/session-restore.md and viewer-panel.md describe the report.

@devsuitup

Copy link
Copy Markdown
Owner

Review of 7b3fdbb: changes requested.

  • The new tests fail first, then pass (verified locally). At 8e4cc64, 2 of 72 tests fail on assertions: dom-file-panel-unsaved-guard.test.js:317 and unsaved-guard.test.js:301. At 7b3fdbb, 72 of 72 pass, and CI is green.
  • Major: CHANGELOG lands in a released section. The base is 24ac041; v0.0.93 (79e2589) has since been released. A clean merge puts the edit of the (restore): save the open sessions when the app closes #479 line inside the released ## v0.0.93 section, and ## Unreleased stays empty. Please rebase onto 79e2589 and move the entry under Unreleased.
  • Minor, still open: the race from my previous comment (reproduced). 7b3fdbb's code is identical to 5920379's. Replaying the race still gives unsaved-check 1 reload → unsaved-check-reason 1 quit → unsaved-check 2 quit.
    • Cause: unsaved-guard.js:85 clears inflight when the answer arrives, while :61-66 approves the quit only after wait.done.
    • Fix: keep the joined check visible to joiners until wait.done, or have a close return while a quit is held, and pin it with a test.
  • Nit: renderer gone. waitReasonDone (:40-52) does not release on render-process-gone or destroyed, so it still waits the full timeout.

@abate
abate force-pushed the fix/save-state-on-exit branch from 7b3fdbb to 87920e2 Compare October 12, 2026 07:14
@abate

abate commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed at 9f9c57f, rebased onto main 421f77f.

Major (changelog in a released section). The previous edit of the released #479 line under v0.0.93 is reverted; the follow-up has its own entry under Unreleased / Fixed.

Minor (race: answer clears inflight before the quit is approved). A check joined by a stronger reason is marked held: its finish no longer clears inflight, and the joined chain clears it once the late-save wait ends (or at once for a declined or unanswered check). A close, quit or install arriving in between joins that chain. Test a close or quit arriving while a joined quit waits for the late save joins it instead of asking again: reload, quit joins, answer, then a window close and an install before the report — one unsaved-check, the close and the install both go through after the report. Plus a joined quit the page declined leaves nothing for a later close to join (pins the clearing).

Nit (renderer gone). waitReasonDone releases on render-process-gone and destroyed, removing its listeners on release. Test a page that dies while a joined quit waits for its late save releases the quit at once, both events, no bounded timer left.

Red first: 87920e2 (tests only), run 38216641758 fails exactly the two new tests on all four test jobs. Fix 9f9c57f: run 38217312151 green on every job. Mutations: dropping the held flag, clearing inflight in finish regardless, dropping either gone listener, or never clearing inflight in the chain each fail a test. Locally the full suite fails only the 13 sandbox-baseline tests; lint 0 errors.

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Re-review at 9f9c57f. The code is approved; the branch needs a rebase before it can merge.

Merge blocker: CHANGELOG conflict with current main
CHANGELOG.md conflicts with main (now abc9a54). Two changes on main touch the same spot. #580 added its line under ## Unreleased / ### Fixed. v0.0.95 (#581) then renamed that section to ## v0.0.95 — 2026-10-12 and opened a new, empty ## Unreleased. Please rebase onto main and put this PR's line under the new ## Unreleased / ### Fixed, not inside the v0.0.95 section. All other files merge cleanly.

Previous points (review of 7b3fdbb)

Point Status Evidence
Major: CHANGELOG edit landed in the released v0.0.93 section Closed The diff adds one line under ## Unreleased / ### Fixed. The released #479 line is unchanged. The changelog check passes.
Minor: the answer clears inflight before a joined quit is approved, so a later close or quit asks again Closed unsaved-guard.js:64 sets held. :95 no longer clears inflight for a held check. :74-77 clears it once the wait ends. Covered by "a close or quit arriving while a joined quit waits for the late save joins it instead of asking again" and "a joined quit the page declined leaves nothing for a later close to join".
Nit: waitReasonDone did not release when the renderer died Closed unsaved-guard.js:44-54 listens to render-process-gone and destroyed, and removes both listeners on release. The test covers both events and checks that no bounded timer is left.

Tests fail first, then pass (local, Node 22, unsaved-guard.test.js + dom-file-panel-unsaved-guard.test.js)

Commit Content Result
3d9653b tests only 2 of 42 fail, on assertions
a1ccc8e fix 42 of 42 pass
87920e2 tests only 2 of 44 fail, on assertions
9f9c57f fix 45 of 45 pass
  • CI matches: the runs on 87920e2 and 91c13ef fail, and run 38217312151 on 9f9c57f is fully green.
  • The related test files at the head give 325 passing, 0 failing and 2 skipped.

Minor (non-blocking)

  1. test/unsaved-guard.test.js, the new tests, against unsaved-guard.js:35-38. Every main-side test sends unsaved-check-reason-done after the answer. When the question is still open, the renderer sends it before the answer (file-panel.js:163), and that is the common case.
    • Mutation run: adding if (pending.has(id)) return; to the unsaved-check-reason-done handler leaves 32 of 32 tests passing.
    • Impact: that regression would hold every joined quit for up to 2.5 s after a quick answer.
    • Suggested test: send reasonDone(id) before answer(id, true), then expect the quit on the next tick with no bounded timer still running.

Checked, nothing found

  • The wait timer starts at the join, not at the answer. This is harmless in both cases:
    • when the answer already left, the result arrives right after;
    • when the question is still open, the renderer reports done at once.
  • A reload during the held wait joins the chain. The original will-prevent-unload handler still skips the reload.
  • No new inline comments, and the docs (viewer-panel.md, session-restore.md) match the behaviour.

Not run: Electron, a real quit or reload, the full suite.

abate added 5 commits October 12, 2026 15:34
… late save

When the renderer answered the reload before the upgrade reached it, its
save runs after main already went on to quit, and the window can close
before the write lands. Main should wait for the page to report the save
done, bounded, and the page should report it.
…save is done

The renderer answers every reason upgrade with unsaved-check-reason-done,
after the save when it is writing for an already approved reload, at once
otherwise; main waits for it, bounded by the check timeout.
…when the page dies

A close or a quit that arrives after the page answered a reload that a
quit joined, but before the page reported its late save, must join that
quit instead of asking again. A page that crashes or is destroyed while
the quit waits must release it at once rather than after the bound.
The answer used to clear the check in flight while the joined quit still
waited for the page's late save, so a close or quit in between started a
second question. The joined check now stays in flight until that wait
ends, and the wait ends at once when the renderer process is gone.
…s the joined quit on that answer

The renderer reports the upgrade done before it answers when the question is still open; the joined quit must then go on the answer, with no bounded wait left running.
@abate
abate force-pushed the fix/save-state-on-exit branch from 9f9c57f to 80c87e3 Compare October 12, 2026 13:36
@abate

abate commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed review of 9f9c57f. Backup of the old head: abate:backup-479-9f9c57f.

Point Change
Merge blocker: CHANGELOG conflict Rebased onto main 61bee93. The #479 line is under the new ## Unreleased / ### Fixed (introduced in 551f721). ## v0.0.95 and older sections are byte-identical to main; git diff 61bee93 80c87e3 -- CHANGELOG.md is that one added line.
Minor: no test sends unsaved-check-reason-done before the answer 80c87e3 (tests only): "a page that reports the upgrade done before it answers lets the joined quit go on the answer". Sends the report, checks no quit while the question is open, answers yes, expects the quit on the next tick and no bounded timer left. Passes at 80c87e3 (33/33). With the mutation if (pending.has(id)) return; in the reason-done handler it fails (32/33, this test).

Commits rebased: 26d53d8, 551f721, a1c1d94, 767c556 (former 3d9653b, a1ccc8e, 87920e2, 9f9c57f), plus 80c87e3.

CI at 80c87e3: Test run 38256934189 and Build & Release run 38256934158, both green (incl. e2e).

@abate
abate requested a review from devsuitup October 12, 2026 13:51

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Re-review at 80c87e3. Approve. The rebase is clean, the CHANGELOG line sits in the right section, and the earlier minor point is closed.

What changed since 9f9c57f (git range-diff)

  • Commits 1, 3 and 4 are unchanged.
  • Commit 2 changes only the CHANGELOG hunk, which is now under the new ## Unreleased.
  • Commit 5 (80c87e3) is test-only: +15 lines in test/unsaved-guard.test.js.
  • unsaved-guard.js, public/file-panel.js and preload.js are byte-identical to the approved 9f9c57f.

Previous points

Point Status Evidence
Merge blocker: CHANGELOG conflict with main Closed The #479 line sits under ## Unreleased / ### Fixed, above ## v0.0.95. The v0.0.95 section is unchanged, and the changelog check passes.
Minor: no main-side test sent unsaved-check-reason-done before the answer Closed New test "a page that reports the upgrade done before it answers lets the joined quit go on the answer" (test/unsaved-guard.test.js:345). Mutation check: adding if (pending.has(id)) return; to the unsaved-check-reason-done handler makes this test fail, while the 9f9c57f test file still passes 32/32 against the same mutant.

Checks

  • Base and merge: the branch is based on current main (61bee93), the merge has no conflicts, and GitHub reports MERGEABLE.
  • Local tests (Node 22): unsaved-guard + dom-file-panel-unsaved-guard pass 46/46. The related set (file panel, restore, New Session dialog, keyboard focus, choice dialog) passes 510/510.
  • CI on 80c87e3 is green: lint, changelog, e2e, tests on Node 20/22 × ubuntu/windows, and the 4 builds.
  • Pending #589 (modal stack, async discard check): a trial 3-way merge has no textual conflicts, and the related tests pass on the merged tree (351/351). Whichever of the two lands second should re-run dom-file-panel-unsaved-guard.test.js.

Not run: Electron, a real quit or reload, the full suite.

@devsuitup
devsuitup merged commit 45d8037 into devsuitup:main Oct 12, 2026
12 checks passed
@abate
abate deleted the fix/save-state-on-exit branch October 15, 2026 21:57
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