Workspaces 7/10: journal in-flight transfers for crash recovery

[nedtwigg]

A crash during a Workspace transfer must restore it once in the target Window. begin_arrival records the Workspace in a separate sessions/arrivals.json journal; neither live snapshot is staged at transfer start. Boot recovery merges leftover records into the target and removes their source copies before reopening windows.

adopt_done marks the record settled. It remains until both windows’ saved snapshots reflect the move, covering the debounce gap after adoption. Failed recovery retains its record for retry and rolls back the target if trimming the source fails.

Validation: 83 Rust tests passed, including journal retirement after both snapshots, tear-out recovery, and retry after an unreadable source. Based on workspaces-registry.

[cloudflare-workers-and-pages]

Deploying mouseterm with    Cloudflare Pages

Latest commit:	44b66c3
Status:	✅  Deploy successful!
Preview URL:	https://4bf4f597.mouseterm.pages.dev
Branch Preview URL:	https://workspaces-durability.mouseterm.pages.dev

View logs

[dormouse-bot]

Feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

The Rust half is self-consistent and the new helpers are well covered, but staging the target's snapshot at the invoke collides with two things outside lib.rs that no test reaches:

• Tear-out regresses (inline on begin_arrival). bootFromTearOut treats "this window has a snapshot" as "this is an ordinary restore", so a torn-out window now cold-restores the staged Workspace and then throws Duplicate Workspace id when it adopts the real one. The arrival is handed back and both windows end up persisting the same id. CI won't catch it: standalone/src/workspace-move.test.ts stubs getWindowState directly, and a_staged_arrival_is_in_the_target_snapshot_until_it_is_handed_back exercises the helpers in isolation, so the two halves are never run against each other.
• The staged entry isn't durable for a transfer into a live window (inline on the spec bullet) — the target's own debounced flush overwrites it before adoption.

Two narrower ones inline: unstage_arrival_on_disk reaching remove_session_from on a live source window, and the build_window failure path leaving the source's snapshot short.

[dormouse-bot]

Feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

Moving the record into its own arrivals.json closes all four findings from the last pass, and the boot merge is idempotent the way the doc claims: replace-by-id into the target, a no-op on a source that no longer names it, and restore_arrivals ahead of restorable_labels so the tear-out target's new file is both enumerated and counted by seed_next_ws. Three things left, one of them the guarantee the PR body states.

The record is dropped ~500 ms before the target has a snapshot (inline on adopt_done). The target's write is a leading-edge debounce — scheduleWrite in lib/src/lib/window-session-aggregator.ts returns early while a timer is pending, so the write lands SESSION_SAVE_DEBOUNCE_MS after publishWorkspaceSession, not after the last store change — and adopt_done forgets the record before it. In that gap the Workspace is in no snapshot and no record, which is exactly what the body promises is covered ("a crash before the target's first flush restores it once, in the target"). A tear-out is the sharp case: the source is handling workspace-departed by invoking close_window, which deletes its snapshot and geometry outright, while the target has never written one — so a kill there loses the window and the Workspace, not just the last 500 ms of changes. await flushWindowSession() before adopt_done in adoptWorkspace, and before settle("adopt_done", id) in bootFromTearOut, closes it: the aggregator already exports it, it cancels the pending timer, and it resolves on the host write.

The title and description still describe the previous design — "move the transferring Workspace between snapshots on disk", and a body that says begin_arrival "takes the Workspace out of the source's snapshot and puts it into the target's". After 4c41d2c1 it touches neither snapshot until the boot merge. Worth recomposing before this leaves draft, since the rationale entry now exists precisely to record that the snapshot-staging design was tried and abandoned.

[dormouse-bot]

Feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

Retaining the record until both snapshots reflect the move closes the adopt_done gap from the last pass without touching the webview, and a_failed_source_write_rolls_back_the_target_and_retries pins the ordering decision I asked to be made explicit. Three things left, one of them the same hole on the other branch.

The hand-back path still loses the Workspace (inline on hand_back_arrival). The adopted path now holds the record until the target's snapshot names the Workspace; the refusal path deletes it immediately and then waits on a 500 ms debounce for the source to name it again. A crash in that window is the failure this PR exists to prevent, on the branch the PR didn't change.

The journal's I/O runs on the main thread (inline on begin_arrival). transfer_workspace, open_workspace_window, adopt_done, adopt_failed and close_window are all synchronous commands, and they now do two F_FULLFSYNCs each and wait on a lock that save_session holds across two more. The comment above load_session in the same file gives making those commands async as the reason the snapshot fsyncs are off this thread.

A settled record whose target window closes before its first flush can never retire (inline on finish_window_close) — and the next launch then reopens the window the user closed.

[dormouse-bot]

Feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

Both gaps from the last pass are closed, and the retirement predicate holds on the paths that matter: the reversed record retires only once the source's snapshot names the Workspace again, and a discarded record survives while the source is still stale, so a crash there removes the stale copy instead of resurrecting it. Two things left, both on the thread-affinity half.

Moving the whole WindowState block off the Destroyed arm breaks the ordering the arm exists to provide (inline on spawn_blocking). The arm's own comment and QuitAction::Destroy's both claim this is where label-keyed state is settled; nothing joins the task, and apply_quit_actions further down the same arm can reach app.exit(0) while it is still queued. Only hand_back_arrival does journal I/O here, so spawning just that loop would keep the rest where the comments say it is.

journal_commands_run_off_the_main_thread pins less than the spec bullet that cites it (inline on the assert). It checks five hardcoded names for a literal attribute string, and says nothing about the "destroyed-window cleanup" half of the rule. sidecar_commands_are_async in the same module already enforces this shape keyed on what a command body reaches, which is what catches the next one.

One reuse note inline on remove_workspace_from_disk.

[dormouse-bot]

Feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

Narrowing the worker to the hand-back loop puts ownership, the reap and the registry back on the main thread where QuitAction::Destroy's comment and the arm's own say they are settled, and the gate does hold the walk's Exit until the journal write lands. Two things left, plus a spec line.

The gate is unbounded, and the quit watchdog is downstream of it (inline on the ExitRequested arm). §Quit flow's watchdog table ends phases 1 and 3 in "log and app.exit(0)", and §Trigger interception says the approved gate alone is what lets that exit through. There are two gates on that arm now, and give_up has already returned by the time the second one refuses it — so the only thing that can still exit is CleanupGate::finish, which has no deadline and one discarded Result between it and never running. A hand-back that never reports back leaves an app that cannot be quit at all, which is what the watchdog exists to prevent. Whichever way you resolve it, those two spec rows are now wrong as written.

quit_waits_for_every_destroyed_window_handback pins the arithmetic, not the wiring (inline on the test) — and the wiring is the "including exit requested through the quit walk or Tauri" half of the bullet that cites it.

One suggestion on the spec's description of what completes on the main thread, and a note on where CleanupGate lives.

[dormouse-bot]

Feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

Verification of 8d09f10 on the break I flagged: the deferral does hold the walk's Exit and the watchdog's app.exit(0) until return_arrival_on_disk lands, and moving ownership, the reap and the registry broadcast back to the main thread leaves hand_back_arrival reading none of the state that now runs ahead of it. The review posted alongside this one has the rest; one thing it and the gate both miss, inline — on macOS an approved quit can still end the process without reaching ExitRequested at all.

[dormouse-bot]

Feedback on work in progress, not a merge verdict — mark it ready when you want the full review.

The gate now bounds correctly: exit_after_cleanup arms exactly one timer per false → true transition of exit_requested, and the sticky forced flag lets the forced app.exit(0) back through all three gates, should_terminate included. One thing left.

Nothing pins the bound itself (inline on the scanner). stalled_cleanup_cannot_block_an_approved_exit_forever drives CleanupGate's methods by hand, and every_approved_exit_path_checks_cleanup only asks whether three call sites name exit_after_cleanup. Delete the whole if start_watchdog { … } block and both stay green — so Must force an approved exit after QUIT_PHASE_TIMEOUT_MS waiting for cleanup is pinned only in the half that cannot fire on its own, which is the half the commit adds. The same substring match leaves the sense of the ExitRequested call site unpinned: dropping the ! from } else if !exit_after_cleanup(app) { inverts the gate without failing anything.

[dormouse-bot]

Out of draft, so this is the full pass. One new finding, inline on the boot-recovery call: a journal record retained past boot is invisible to the Workspace id counter, so an id that lives only in arrivals.json can be minted a second time during that run and end up naming two different Workspaces at the next launch.

The five threads still open from the draft rounds are each already fixed downstream in #630 (workspaces-transfer-fixes) — the adopt_done log line, the every_approved_exit_path_checks_cleanup assertions, remove_workspace_from_disk reused for restore_arrival's trim, CleanupGate moved into quit_state.rs, and the journal-command scan widened into blocking_commands_run_off_the_main_thread. I'm not re-raising them here; resolve them if carrying the fixes downstream is the intent.

[dormouse-bot]

Checked the six findings deferred to #630 against its head 8708519b (which contains this PR's head 44b66c3a), and all of them landed:

Finding	At 8708519b
adopt_done log named the wrong operation	could not mark {workspace_id} adopted on disk
Hardcoded five-name command check	blocking_commands_run_off_the_main_thread scans every #[tauri::command] body for the journal helpers, close_window_snapshot/finish_window_close, begin_arrival/hand_back_arrival and the sidecar family, and pins the Destroyed worker split
restore_arrival's duplicated trim	Both sites call remove_workspace_from_disk
Third state machine living in lib.rs	CleanupGate and its tests are in quit_state.rs beside QuitMachine/CloseMachine
Exit-path scan missed the watchdog	every_approved_exit_path_checks_cleanup now asserts !exit_after_cleanup(app) at both call sites and if start_watchdog / force_if_waiting / QUIT_PHASE_TIMEOUT_MS inside the gate
Retained record invisible to the id counter	saved_workspace_ids extends with record_workspace_id over the journal, and saved_window_labels reserves each record's from/to, both read before restore_windows

The label reservation is the part I had not raised: restorable_labels drops arrivals (it keeps only main and ws-<n>), so a failed recovery's endpoints were reopenable as fresh tear-out labels until saved_window_labels started seeding from the journal.

Nothing from this PR's threads is left open on my side. I have not re-reviewed the rest of 8708519b here — that belongs on #630.