Skip to content

fix(workspace): save app state before shutdown#222

Open
BunsDev wants to merge 3 commits into
mainfrom
fix/crash-recovery-save-hooks
Open

fix(workspace): save app state before shutdown#222
BunsDev wants to merge 3 commits into
mainfrom
fix/crash-recovery-save-hooks

Conversation

@BunsDev

@BunsDev BunsDev commented Jul 26, 2026

Copy link
Copy Markdown
Member

Description

What: Save app state before terminating the persistence writer during app shutdown, so the restored session reflects the workspace state at quit time.

Why: on_will_terminate previously terminated the SQLite PersistenceWriter without enqueueing a final snapshot. Session persistence only happened on ambient events (window move/resize/focus/close), so any workspace change made after the last ambient save — e.g. opening a tab right before Cmd+Q — was lost on restore.

How:

  • Added workspace::run_shutdown_persistence(ctx), which enqueues a final ModelEvent::Snapshot via save_app and then calls PersistenceWriter::terminate(). terminate() joins the writer thread, guaranteeing the snapshot is flushed to SQLite before exit; it stays idempotent via thread_handle.take().
  • on_will_terminate in app/src/lib.rs now calls this single shared function.
  • The integration-test helper replays the exact same production function, so removing the shutdown save from the product path fails the test (true regression coverage — verified by disabling the save and watching the test fail).

Linked Issue

Closes #221

Testing

  • Unit test shutdown_save_sends_snapshot_before_writer_termination (app/src/workspace/global_actions.rs): asserts the snapshot event is enqueued before writer termination.
  • New integration test test_shutdown_save_persists_session_snapshot (crates/integration/src/test/session_restoration.rs): boots the app, opens a second tab, waits for the ambient save to flush, wipes the persisted snapshot, replays the shutdown persistence hooks, and asserts the snapshot is repopulated with both tabs — so the assertion can only be satisfied by the shutdown save. Registered in the manual runner and ui_tests.rs (runs in CI).
  • Regression check: with save_app disabled inside run_shutdown_persistence, the integration test fails as expected; restored, it passes.
  • Targeted gates run: cargo check -p warp-app --bin cast-codes --features gui, cargo clippy -p warp-app --features gui --all-targets --tests -- -D warnings, cargo fmt --check, ./script/check_ai_attribution, ./script/check_rebrand.
  • Not run: full ./script/presubmit (workspace-wide build too expensive locally); coverage limited to the targeted gates above.

Agent Mode

  • PR via CastCodes

Changelog

CHANGELOG-BUG-FIX: Fixed session state not being saved when quitting the app.

Copilot AI review requested due to automatic review settings July 26, 2026 10:12
@BunsDev
BunsDev marked this pull request as ready for review July 26, 2026 10:12

Copilot AI 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.

Pull request overview

This PR fixes a session-restoration edge case by ensuring the app enqueues a final persistence snapshot during shutdown before the SQLite writer thread is terminated, so the next launch restores the true “quit-time” workspace state.

Changes:

  • Added a shared shutdown persistence helper that performs a final ModelEvent::Snapshot save and then synchronously terminates the persistence writer thread.
  • Updated the production shutdown path (on_will_terminate) to use the shared helper.
  • Added integration coverage that wipes persisted state mid-test and verifies the shutdown hook repopulates the snapshot.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/integration/tests/integration/ui_tests.rs Registers the new shutdown persistence integration test in the UI test suite.
crates/integration/src/test/session_restoration.rs Adds an integration test that proves the shutdown hook repopulates persisted tabs after an explicit wipe.
crates/integration/src/bin/integration.rs Registers the new integration test in the manual runner.
app/src/workspace/mod.rs Re-exports the shutdown persistence helper for use during app shutdown.
app/src/workspace/global_actions.rs Implements run_shutdown_persistence(ctx) and adds a unit test for shutdown snapshot behavior.
app/src/persistence/testing.rs Adds integration-test helpers to count persisted tabs and clear persisted app state.
app/src/persistence/sqlite.rs Extracts app-state deletion into delete_app_state for reuse by save + testing helpers.
app/src/lib.rs Switches shutdown (on_will_terminate) to call the shared shutdown persistence helper.
app/src/integration_testing/persistence.rs Adds a test-step helper that replays the production shutdown persistence sequence mid-test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/src/workspace/global_actions.rs
Give the unit test's PersistenceWriter real WriterHandles (a dummy joinable
thread plus a clone of the same channel save_app uses, mirroring the
production wiring in sqlite::start_writer). terminate() is no longer a
no-op, so the test now verifies the shutdown ordering guarantee: the
session snapshot is enqueued before ModelEvent::Terminate.
Copilot AI review requested due to automatic review settings July 26, 2026 10:28

Copilot AI 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.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

app/src/workspace/global_actions.rs:166

  • The doc comment claims PersistenceWriter::terminate "synchronously drains the writer queue", but terminate() actually sends ModelEvent::Terminate and joins the writer thread. The guarantee you rely on is FIFO ordering + join (events enqueued before Terminate are processed before the thread exits), not necessarily draining every queued event after Terminate. Tightening the wording here will prevent future readers from assuming stronger semantics than the implementation provides.
/// Persists a final session snapshot and then shuts down the sqlite writer.
///
/// Called from the app's `on_will_terminate` callback (see `app_callbacks` in `lib.rs`).
/// The snapshot must be enqueued before `PersistenceWriter::terminate`, which synchronously
/// drains the writer queue, so the session state at quit reaches the database.
pub(crate) fn run_shutdown_persistence(ctx: &mut AppContext) {

Comment thread app/src/persistence/testing.rs
Copilot AI review requested due to automatic review settings July 26, 2026 16:12

Copilot AI 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.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comment thread app/src/lib.rs
Comment on lines 2025 to 2033
on_will_terminate: Some(Box::new(move |ctx| {
NotebookManager::handle(ctx).update(ctx, |manager, ctx| {
// Notebooks are only saved periodically, so ensure that any pending changes have
// been sent to the writer thread before terminating.
manager.close_notebooks(ctx);
});

PersistenceWriter::handle(ctx).update(ctx, |writer, _ctx| {
writer.terminate();
});
workspace::run_shutdown_persistence(ctx);

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.

Session state changed right before quit is lost on restore

2 participants