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:	6b4e11c
Status:	✅  Deploy successful!
Preview URL:	https://c70288f4.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]

This call marks the record adopted rather than forgetting it, so the log line names the wrong operation — a reader grepping for a lost record would be looking at the retention path.

Suggested change

	append_log(format!("[window] could not forget {workspace_id} on disk: {e}"));
	append_log(format!("[window] could not mark {workspace_id} adopted on disk: {e}"));

[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]

The spec bullet citing this test reads "Must run journal I/O and its lock waits off the main thread, including transfer/settlement/close commands and destroyed-window cleanup". The test checks five hardcoded names for a literal attribute string: it asserts nothing about the destroyed-window cleanup, and a sixth command added later that reaches record_arrival_on_disk, close_window_snapshot or forget_arrival_on_disk passes it while putting two fsyncs back on the event loop.

sidecar_commands_are_async at the bottom of this module already solves that, keyed on what a command body reaches rather than on a name list, and its comment says why: "Three clipboard commands once slipped through the async port; this scans the source so the omission can't silently recur." Widening its reaches_sidecar predicate to also match the journal helpers — plus finish_window_close, which is all close_window's body contains — would pin the rule as the spec states it, cover commands nobody has written yet, and let this test go along with the \r\n normalization 1e22d857 had to add: that scanner splits on .lines(), so CRLF never reaches its comparisons.

[dormouse-bot]

This is restore_arrival's trim closure verbatim — snapshot_without_workspace, then remove_session_from on an emptied list and write_session_to otherwise. trim can call it instead: the only difference is that trim works from the source snapshot read earlier in the function, and nothing writes to from between that read and the trim, so re-reading is equivalent and the returned Err still drives the same target rollback.

[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]

QuitMachine and CloseMachine are both pure quit-flow state machines in standalone/src-tauri/src/quit_state.rs, tested in that module's mod tests. This is the third one and the only one in lib.rs — which is why its test landed beside the arrivals-on-disk tests instead of next to a_repeat_trigger_while_voting_keeps_the_votes_already_cast. Moving it there would leave exit_after_cleanup, the part that needs an AppHandle, as the only quit-gate code in this file.

[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]

exit_after_cleanup's watchdog is what makes the spec's force rule true, and nothing reaches it — stalled_cleanup_cannot_block_an_approved_exit_forever calls force_if_waiting directly, and this scan stops at the call sites. Removing the if start_watchdog { … } block restores the unbounded wait with both tests green.

Two more assertions close it, and negating this one also pins the sense of that call site rather than just its presence. I checked all three against the current source.

Suggested change

	assert!(event.contains("exit_after_cleanup(app)"));
	assert!(event.contains("!exit_after_cleanup(app)"));
	let gate = source.split("fn exit_after_cleanup").nth(1).unwrap().split("\n}").next().unwrap();
	assert!(gate.contains("force_if_waiting") && gate.contains("QUIT_PHASE_TIMEOUT_MS"));