feat(lifecycle): keep Staged running when its last window closes - #936
Merged
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 941614d388
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// signalled first and waited on against this one deadline, because the | ||
| /// `RunEvent::Exit` path runs inside `applicationWillTerminate:`, where the OS | ||
| /// gives us limited time before killing the process outright. | ||
| const SHUTDOWN_BUDGET: Duration = Duration::from_secs(2); |
There was a problem hiding this comment.
When an active ACP agent does not acknowledge session/cancel within two seconds, this deadline expires and spawn_quit calls app.exit(0) while the agent child is still alive. The cancellation path in crates/acp-client/src/driver.rs can wait five seconds for the prompt response and, for remote agents, another five seconds in graceful_stop; because process exit does not run the child's kill_on_drop, slow local agents or remote proxy process groups can be orphaned and continue consuming resources or modifying the workspace after Staged quits. The shutdown path must either wait through the driver's teardown bounds or explicitly force-kill remaining session processes before exiting.
Useful? React with 👍 / 👎.
matt2e
force-pushed
the
closing-window-should-not-quit
branch
from
September 23, 2026 03:34
941614d to
2fa4447
Compare
Closing the window (red button / Cmd+W) terminated the app, taking every running agent session with it — and Cmd+Q was worse: `PredefinedMenuItem::quit` maps to `NSApp terminate:`, which reaches no Tauri hook, so it skipped the action-shutdown handler entirely and left agent CLIs (spawned with their own process group and only `kill_on_drop`) orphaned. New `app_lifecycle` module owns both halves of the fix, adapted to the multi-window model (#928) this lands on top of: - Closing a window with peers still live (visible or hidden) is just a close: sessions belong to the process, so the window is destroyed normally and the existing `Destroyed` hook does the per-window cleanup. Closing the *last* window is where the interception bites: on macOS `CloseRequested` is prevented and the window hidden, so sessions keep streaming; the Dock icon (`RunEvent::Reopen`), `Window ▸ Staged`, or a quit arriving while hidden brings it back — `show_a_window` prefers `main` for its restored geometry but recovers any surviving `win-N` peer. Hiding also drops that window's `tauri-{label}` PR-poll client to its unfocused tier, which a hidden window's missing webview blur would not. Other platforms have no Dock or tray to recover a hidden window, so closing the last window still quits there — now through the confirmation gate. - A custom Quit menu item makes Cmd+Q routable, so `request_quit` can gate it on active sessions and raise a confirmation dialog; confirming cancels each session with `CompletionReason::AppQuit` (the cancel is what runs the ACP child's graceful stop), waits for sessions and actions inside one shared 2s budget, sweeps any still-active rows to cancelled/app_quit, then exits. Queued sessions count as active; running actions are reported but don't gate. Quit and `Window ▸ Staged` route through the shared `dispatch_menu_event` router as focus-independent backend actions — every window being hidden is exactly when they matter. The dialog is addressed to exactly one window (`emit_to` plus a window-scoped frontend listener, the same pattern as menu routing): where the user is, or a window revealed for the purpose. A broadcast would raise one dialog per window, each unaware of the others' answers. The pending-prompt flag remembers its host window and is cleared when that window is destroyed, so the force-on-second- request escape hatch can't fire with no dialog on screen. `RunEvent::Exit` now runs the same idempotent cleanup, which is the only hook on the terminate: path — Dock ▸ Quit and logout stop sessions and actions instead of orphaning them. Ownership is checked against `owner_pid`, so a quit never prompts about or cancels another Staged instance's work. The quit commands are refused in the web-mode dispatch table: a browser client must not be able to terminate the desktop host. The store-incompatibility screens' Close buttons now quit rather than closing a window that would only hide. Phase 4 of the plan (routing Dock ▸ Quit through the prompt via a runtime `applicationShouldTerminate:`) is deliberately left out: the shared cleanup already prevents the process and data damage there, only the prompt is missing. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
The quit confirmation lived in the webview: `request_quit` emitted `app:quit-requested` to one chosen window and a Svelte `AlertDialog` rendered it. That forced a window into the quit path, and the case it hurt is the central scenario of this branch — the whole point of closing-window-is-not- quitting is that sessions keep streaming with everything hidden, so "all windows hidden, sessions running, user hits Cmd+Q" isn't a corner to tolerate, it's the primary path the confirmation exists to serve. Reaching it materialised a full application window (restored geometry, hydrating project tree) to host a two-button question, and `cancel_quit` did nothing but clear a flag — so Cmd+Q then Cancel left the user with a visible window they had to close a second time, having asked for neither. Parenting a native alert would not have fixed that: `.parent()` is exactly what makes `tauri-plugin-dialog` render an `NSAlert` as a window-modal *sheet*, so the reveal would have stayed. Unparented is a different widget, not a different modality of the same one — rfd 0.16 routes a parentless dialog to `CFUserNotificationDisplayAlert`, displayed by the system rather than by AppKit. That's what buys window-independence, so the reveal drops out of the quit path entirely: quitting from a hidden state stays hidden, cancelling returns the app to exactly the state the user left it in, and the branch where no window could be revealed and the app quit *without asking* disappears rather than being preserved. Structurally this resolves the review finding about a pending prompt outliving its host window by removing the concept of a host window. `QuitState.prompt_host` collapses to `prompt_pending: AtomicBool`; `clear_prompt_if_host`, the `Destroyed` arm of `on_window_event`, and `reveal_a_window` (now inlined into its one caller, `show_a_window`) all go away. The frontend half goes with them: `QuitConfirmDialog`, the `quitPrompt` store, `quitListener`, `quitPromptCopy`, the `QuitRequestedPayload` type, and the `confirm_quit` / `cancel_quit` commands. `quit_app` stays — the store-incompatibility screens still need it — and stays refused in the web-mode dispatch table. The prompt copy ports to Rust, where `get_branch` / `get_project` resolve the names the Svelte dialog used to read from its stores; the review's suggestion to fold the session list into the preceding sentence with a colon is taken while the wording moves. `OkCancelCustom`'s ok slot is the default (Return) button, so it holds "Keep Running" and the *cancel* slot holds "Quit & Stop Sessions" — a stray Return must not be what kills running agents. Accepted costs, all inherent to the widget: the alert carries generic system chrome rather than Staged's icon; the "Stopping sessions…" progress state is gone, since a native alert dismisses on click while `shutdown_cleanup` runs out its 2s budget; and it is not modal to the app, so work can start behind it. The last resolves correctly — `shutdown_cleanup` re-queries active sessions instead of trusting the prompt's snapshot — and it keeps the force-quit escape hatch dispatchable, which a second Cmd+Q needs. Because that dismissal leaves nothing on screen, `request_quit` now returns early when a shutdown is already under way instead of raising a second alert about sessions the first one is stopping. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
… close The force path out of an unanswered quit prompt keyed off nothing but "a quit request arrived while `prompt_pending` is set", which off macOS put it on the last window's close button — the only interactive quit trigger there is without an app menu. Two quick clicks on X therefore killed running agents with the question never answered, and X is exactly the control people click again when a window doesn't shut: reflex aimed at the window, not an answer to a system alert they may not have noticed (unparented, so it carries no app chrome tying it to the click). `request_quit` now takes a `QuitTrigger` instead of the vestigial `force: bool` that no caller ever set, and only `QuitTrigger::Explicit` — the app menu's Quit item / `Cmd+Q`, and the store-incompatibility screens' Close button — may take the force path. A `QuitTrigger::WindowClose` request falls through to the normal gate, so a repeat close re-raises the alert (a second copy of it if the first is still up, which is a cheap thing to dismiss next to a forced quit) built from a fresh blocker snapshot — and if the sessions have finished in the meantime it just quits, with nothing left to warn about. That keeps the escape hatch where it was justified: a deliberate second `Cmd+Q` still gets out of a prompt that never appeared or never came back, which it must, since the alert is not app-modal. Off macOS the close button loses the hatch and needs no replacement — asking again is itself the way out of an invisible prompt, because clicking X puts an answerable question back on screen. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
…in flight `shutdown_cleanup` guarded itself with `quit_in_progress.swap(true)`: the first caller did the work and every later one returned immediately. That's the wrong contract for one of its three callers. Confirming "Quit & Stop Sessions" runs the cleanup on a background thread — up to 2s of bounded waits with *nothing on screen*, since the native alert dismisses on click and the webview's "Stopping sessions…" progress state went with it. An impatient Dock ▸ Quit in that window sends `terminate:`; Tauri fires `RunEvent::Exit` on the main thread inside `applicationWillTerminate:`; the guard saw the flag set and returned instantly; `applicationWillTerminate:` returned and the OS killed the process mid-cancel. The agent CLIs were orphaned — own process groups, and neither an OS kill nor `process::exit` runs the `kill_on_drop` destructors — and the DB sweep was skipped, so the next launch found active rows owned by a dead pid and reported them as errored sessions. Exactly the two failure modes this branch exists to prevent, reachable by a second quit gesture the missing feedback invites. The swap-guard becomes a completion flag behind a mutex, extracted as `QuitState::run_cleanup_once` — holding the lock for the duration of the work is the mechanism, so a late caller parks on `lock()`, then sees `done == true` and returns having waited. On the `Exit` path that holds `applicationWillTerminate:` open until sessions are actually stopped and swept, bounded by the same `SHUTDOWN_BUDGET` that was sized for that context in the first place. `quit_in_progress` stays, unchanged in meaning, because its two readers (`request_quit`'s already-shutting-down early return, `on_close_requested`'s stay-out-of-the-way check) run on the main thread while a cleanup may be in flight and need a non-blocking load. It just stops being the once-guard: the mutex claims, the atomic publishes. Blocking the main thread here is safe. It happens only on the `terminate:`/`ExitRequested` paths, where the app is exiting and repainting no longer matters; the cleanup's waits are satisfied by session threads on the tokio runtime and action process-group signals, none of which need the event loop to turn — established behavior, since a Dock ▸ Quit with no confirmed quit in flight already runs the whole cleanup synchronously inside `applicationWillTerminate:`. `spawn_quit`'s reason for existing (keep the event loop turning during a user-initiated quit) is untouched. Poisoning is taken over with `PoisonError::into_inner` deliberately: if the first caller panicked mid-cleanup, `done` is still `false` and the late caller re-runs the work — the right recovery, since every step is idempotent (cancelling a cancelled session is a no-op, the sweep is a guarded CAS per row). That's why `std::sync::Once` is the wrong tool despite matching blocking semantics: a panicked `call_once` poisons the `Once` and makes every later caller panic, and a panic inside `applicationWillTerminate:` aborts with no cleanup at all. Extracting the latch also gives it test coverage the old guard couldn't have, since `shutdown_cleanup` needs an `AppHandle`: a late caller returning only after the running cleanup finished and without re-running it, a sequential second call being a no-op, `quit_in_progress` reading true from inside the work, and a panicked cleanup being retried. This makes the impatient Dock ▸ Quit safe, not visible — there is still no feedback for up to 2s after confirming a quit, which is what invites the second gesture. Restoring a progress indication without a webview (Dock badge, `NSApp` activity) is a separate decision, deliberately not bundled here. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
`shutdown_cleanup` ends by sweeping whatever is still active in the DB to
cancelled/`app_quit`, so the next launch doesn't find rows owned by a dead
process and report them as errored sessions. The snapshot it works from,
`owned_active_sessions`, filters on ownership — running rows stamped with our
`owner_pid`, plus all queued rows, which carry no owner and count as ours by
convention. The per-row CAS behind it did not: `transition_from_active` guarded
only on `status IN ('queued', 'running')`, and that ownership filter doesn't
survive the gap between the two.
A review of `941614d3` flagged the interleaving. Another instance pointed at the
same data dir drains its queue mid-sweep and claims a row via
`transition_queued_to_running` — one statement that sets `status='running'` and
stamps *its* pid. The row is now that instance's live session, but its status
still matches, so our CAS succeeds and cancels another process's in-flight work.
The review suggested splitting by snapshotted status — a `queued`-only CAS for
rows snapshotted as queued. That makes the *other instance's* claim mutually
exclusive with the sweep, but it isn't the only claimant inside the window: the
session completion handler drains the branch queue on any terminal transition,
including the cancels `shutdown_cleanup` just issued, so this process's own
drain can claim a queued row too. Under the status split that claim also fails
the narrow CAS, leaving the row `running` under our pid as we exit — precisely
the artifact the sweep exists to prevent. The distinction that matters isn't
"queued or running at snapshot time", it's "ours or not ours at write time", so
the ownership check moves into the statement:
WHERE id = ?5 AND (status = 'queued' OR (status = 'running' AND owner_pid = ?6))
`transition_from_active` becomes `transition_from_owned_active` — the sweep is
its only caller, so it could change shape freely — and both interleavings now
resolve in one predicate. Another instance's claim stamps a different pid and
fails it; our own drain's claim stamps ours and still matches, which is right,
because this process is exiting and taking that session with it.
That argument surfaces the same-process sibling, fixed here too:
`drain_queued_sessions_for_branch` now returns early once a shutdown is claimed
(`QuitState::is_quitting`, extracted from the `quit_in_progress` load
`on_close_requested` already did). Without it, shutdown feeds itself — its
cancels trigger drains that claim queued rows and spawn fresh agent children
which `app.exit(0)` then orphans, since they run in their own process groups and
an exit runs no `kill_on_drop` destructors. The sweep would put those DB rows
right; nothing would put the processes right.
Unchanged: queued rows still count as every instance's own, so a quit still
prompts about and cancels unclaimed queued work. This narrows exactly one thing
— a row that stops being queued because someone *else* claimed it mid-sweep.
The sweep's snapshot-and-CAS body is extracted as `sweep_sessions(&Store)`, so
the tests that used to inline a copy of the loop exercise the real one, and a
new case asserts a running row owned by another pid survives it untouched. At
the store level, where the CAS fires exactly as it does inside the race window:
that row returns `false` and keeps its status, alongside positives for a queued
row and for a running row carrying our pid.
Verified with `just check-all`.
Signed-off-by: Matt Toohey <contact@matttoohey.com>
… points The shutdown-feeds-itself gate on `drain_queued_sessions_for_branch` was one `is_quitting` load at the top of the function, and a review of `abc299a9` flagged what that leaves open: the drain awaits between claims, so a drain already past the gate when a shutdown is claimed — one triggered by a session finishing naturally just as the user confirms the quit — can still claim a queued row and spawn a fresh agent child after `cancel_owned_sessions` has snapshotted the registry. The ownership-aware sweep CAS puts the DB row right (our pid matches), but `app.exit(0)` orphans the process: agent CLIs run in their own process groups, and an exit runs no `kill_on_drop` destructors. The gate is re-checked at the two points that matter, through a shared `app_lifecycle::is_quitting(app)` extracted from the entry check's inline try_state dance: - In the drain loop, immediately before each start — the review's suggestion. For the commit- and git-pipeline paths this is as tight as the check can get: their bodies run synchronously from dispatch to `start_pipeline_session` registering the session's cancellation token, so nothing can interleave past it. - In `start_queued_session_for_branch`, after the claim and immediately before `start_session` — because for agent sessions the loop check is not the last chance. Real awaits sit between the claim and the spawn (`review_tip_sha`, `commit_pre_head_sha`, the remote workdir resolve, and context building before the claim), room for a whole shutdown to start. Past this check the path is synchronous and `start_session` registers the cancellation token before any child spawns. Bailing after the claim is deliberate, and is what `abc299a9` bought: a row left `running` under our pid is exactly the shape the sweep's ownership-aware CAS moves to cancelled/app_quit, and the claim succeeding proves the sweep hasn't processed that row yet — the CAS and the claim guard on the same `status = 'queued'`. The bail also lands before the `session-status-changed: running` emit, so no client hears about a session that will never run. This shrinks the window, not closes it: `run_cleanup_once` publishes `quit_in_progress` before the shutdown snapshots the registry, so the losing interleaving narrows to a shutdown doing both inside the few synchronous statements between the final load and the token registration. Closing it outright would mean shutdown-side changes (a cancel that re-snapshots, or deferring cancels by DB snapshot via `cancel_or_defer`) — a bigger contract change than this finding calls for. No new test: the gates read managed app state, and the drain chain takes the concrete Wry `AppHandle` (through `start_session`'s `SessionConfig`), so a mock-runtime harness can't reach them without genericizing the whole spawn path — the same reason the entry gate landed with store-level tests only. Verified with `just check-all`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…struction The post-claim quit gate in `start_queued_session_for_branch` claimed the path from its final `is_quitting` load to the registration of the session's cancellation token was synchronous and a few statements wide. A review of `5d6d7441` showed how far that undersold the agent-session case: `start_session` constructed its `AcpDriver` before calling `registry.register`, and construction resolves the agent binary through `doctor::resolve`'s login-shell probes — a 10s timeout per shell, and with no provider pinned, `discover_providers()` probes every known agent. Seconds of wall clock sat inside the claimed "few statements": room for a shutdown on another thread to publish `quit_in_progress`, snapshot a registry that doesn't contain the session, and reach `app.exit(0)` — after which the child spawns unregistered and nothing stops it. The pipeline paths never had the problem: `start_pipeline_session` registers at entry, before any slow work. Of the two shapes the review offered — register at entry, or move the last-look `is_quitting` load next to the registration — registering at entry is the one that covers the slow phase rather than stepping over it. A last look after construction would leave the constructing session invisible to the shutdown snapshot for those seconds, and its bail would sit inside a function whose direct callers write an `Err` up as an *errored* session — the next-launch artifact this branch exists to prevent — where the quit sweep would have written cancelled/app_quit. Registered at entry, the slow phase is visible: shutdown's cancel fires the session's token, the connect path checks it before protocol setup (`run_acp_session` selects on the token ahead of `initialize`), and the child it spawned is torn down by the connection task's `graceful_stop` — a stop `wait_for_sessions` holds the exit open for, since the session now deregisters through its normal terminal path with the recorded AppQuit reason. The mechanism is `SessionRegistry::register_for_startup`: register, run the fallible startup (driver construction and the user-message persist), and deregister on the way out of a failure — the session thread that normally deregisters never spawns on that path, and a stale entry would hold `wait_for_sessions` open for its full budget and misreport `is_running`. This closes the same gap for ordinary cancels, not just quits: a user cancel landing during construction used to take `cancel_session_impl`'s `!was_running` branch — a DB write the thread about to spawn would then contradict by running the agent anyway. Now it reaches the token. The pipeline handoff's reliance on `register()` replacing the pipeline's token without a tokenless gap (`PipelineOutcome::HandedOffToAi`) is preserved; the replacement just happens before the slow work instead of after it. The oversold comment at the drain's post-claim gate is rewritten to match: the losing interleaving is now genuinely a shutdown publishing and snapshotting inside the few statements between that load and the registration — the status emit and the call itself — a window this change shrinks, not closes, and the gate's bail-strands-the-claim reasoning stands unchanged. New tests cover the latch at the registry level, where no AppHandle is needed: a cancel arriving mid-startup fires the token and records its reason while the session stays registered, and a failed startup deregisters so a shutdown doesn't wait on a session that can never stop. Verified with `just check-all`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
A review of `a0c52320` found two places where a Stop the registry had already answered `true` to was then thrown away. They are the same mechanism. When `registry.cancel(id)` finds an entry, `cancel_session_impl` takes the fast path and writes no status, because that entry's observer — the session thread — is expected to record the terminal state itself. The `true` is a commitment, and two paths drop the entry without an observer. `register_for_startup`'s failure arm is the first, and new in `a0c52320`. A Stop landing while the driver resolves fires the token, records its reason and returns `true`; the resolve then fails (pinned provider binary gone, `discover_providers()` empty), the entry is deregistered, and the session thread that would have observed the token never spawns. On the pipeline-handoff path `finish_failed_pipeline_handoff_start` then wins `transition_from_running` and the row lands on `error`/`Crashed` — where before the token was registered at all, `cancel_session_impl`'s `!was_running` fallback wrote `Cancelled` at cancel time and made that transition lose. On the queued-branch path the `Err` reaches nothing but a log line in `drain_queued_sessions_for_branch`'s callers, so the row stays `running` under our pid and the next launch reports it as an errored session — the artifact this branch exists to prevent, produced here by a Stop. `register` replacing a live entry is the second, and pre-existing. `PipelineOutcome::HandedOffToAi` deliberately skips `deregister` so the insert swaps the token with no gap, but the swap carried nothing across: a cancel landing after `run_pipeline` returns and before `start_session` reaches `register` — a window that holds `git_identity_env_from_global_config()`, which shells out to `git config` — fires a token the pipeline thread is already past observing, answers `true`, and is then erased by the replacement. For an app quit that is this branch's core failure mode intact: the id is in `cancel_owned_sessions`' snapshot, so `wait_for_sessions` blocks on a session that will never honour the cancel, times out at `SHUTDOWN_BUDGET`, and `app.exit(0)` orphans the agent child. One rule covers both, named as `RunningSession::accepted_cancellation`: an entry leaving the registry without an observer hands its cancellation to whoever takes over that job. - `register` carries it onto the replacement, alongside the `pending_cancellations` intent it already applied — the same treatment, because both are cancellations recorded against a session id whose token has since been replaced. Only one can be set at a time (`cancel_or_defer` parks an intent only when nothing is registered), so the pending one is simply preferred. Scoping the carry to a *cancelled* predecessor is what keeps an ordinary handoff from starting its AI turn pre-cancelled. - `register_for_startup` reports it rather than dropping it, through a `StartupFailure` pairing the startup's error with it, and `start_session` records `Cancelled` before returning that error. This restores the ordering the pre-registration code had by accident — the cancelled write lands first, the failure's own `error` transition loses — and gets the queued-branch row off `running`. The error is still returned and still emitted by the caller; only the persisted status differs. Finding a live entry to replace means the handoff, not a session winding down: an ordinary session's thread deregisters before its terminal DB write, so while its entry is present the row is still `running` and `transition_to_running` refuses to start another turn on it. Carrying from a live predecessor therefore can't cancel a turn the user just asked for. Not closed: `cancel_with_completion_reason` clones the entry out from under the registry lock and applies the cancellation after releasing it, so a cancel already past that clone when the startup-failure removal runs lands on an `Arc` no longer in the map and goes unreported. That is the same window `start_session`'s post-`deregister` token re-read already documents, and closing it means applying cancellations under the registry lock — a bigger contract change than these findings call for. Tested at the registry level, where no `AppHandle` is needed: a cancel landing mid-startup coming back on the failure, a startup nobody cancelled reporting none, a cancelled predecessor's token and reason arriving on the replacement, and an uncancelled predecessor leaving the replacement clean. Verified with `just check-all`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…can't see `a0c52320` registered a session's token before its slow driver construction and described what that buys: a cancel fires the token, `run_acp_session` selects on it ahead of `initialize`, the child is torn down by the connection task's `graceful_stop`, and `wait_for_sessions` holds the exit open for it. A review of that commit showed the chain only completes inside `SHUTDOWN_BUDGET` if the startup it now covers also fits there — and routinely it won't: `doctor` gives each login-shell probe a 10s timeout, and the unpinned-provider branch pays that per provider through `discover_providers()`. That work is blocking and not token-aware, so the cancel can't shorten it. Shutdown therefore waits on a session it previously couldn't see, times out, warns, sweeps and exits — and if the resolve finishes just inside the deadline, `connect` spawns the child moments before `app.exit(0)`: the orphan case again. The weak link is that `connect` spawns unconditionally. Its check is a `select!` *after* the spawn, so the design's answer to "cancelled before we connected" is to start the agent and immediately kill it — correct only while the kill outruns the exit. The session loop now takes a last look at the token before it connects and returns `Ok(AgentRunOutcome::Cancelled)` when it has already fired, so nothing is started that has to be raced back down. The `generate_pikchr` worker has always gated its `driver.run` this way; the main session loop was the path missing the check. Terminal handling is untouched, because that outcome is exactly what the spawn-then-teardown path produced: the row still lands `cancelled` with the registry's recorded reason (`app_quit` for a quit), and shutdown's wait now ends on the session's own deregister instead of on the budget. An ordinary Stop during startup gets the same benefit — the agent CLI never launches at all rather than launching to be killed. The doc comment on `register_for_startup` now states the guarantee it can actually keep. Registration buys visibility, not a bounded stop: a cancel is *observed* only once the blocking startup ends, routinely past the 2s budget, so shutdown still times out on such a session, warns, sweeps its row to cancelled/`app_quit` and exits, killing the still-probing thread with the process. That is a clean end rather than a leak precisely because of the gate — the thread has spawned no agent child, and now never will. Considered and rejected: having shutdown skip the wait for sessions that haven't connected, on the theory that exiting sooner kills the resolving thread before it can spawn anything. It doesn't dominate. The resolve runs as blocking `Command` probes on the thread that called `start_session` (a tokio worker for every production caller), so while it runs it neither observes the token nor spawns anything agent-shaped — doctor's probes are short-lived login shells in their own process groups, which `run_command_with_timeout` reaps. So nothing is leaked by waiting, and nothing is saved by not waiting: the orphan window in both shapes is "the resolve finishes within the teardown-sized slice just before the process dies", which skipping the wait moves earlier rather than closes. The gate closes it in both. What skipping would add is a `connected` bit on `RunningSession`, a shutdown-side filter, and the loss of the session's own terminal write in exchange for a sweep row saying the same thing — latency bought with contract. No new test: the gate sits between a constructed `AcpDriver` and a spawned agent child inside `start_session`'s session thread, which takes a concrete `AppHandle`, so reaching it needs the whole spawn path — the same limit `5d6d7441`'s quit gates landed under. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
`is_quitting` gated the queue drain and nothing else. A review of `89c821c3` named what that leaves open: `start_session` and `start_pipeline_session` had no gate of their own, so every other way into a session — the `start_session` and `resume_session` commands, `start_branch_session`, `start_project_session` (all four also dispatchable in web mode), and the four `start_pipeline_session` sites in `prs` — could still enter inside `SHUTDOWN_BUDGET`, after `cancel_owned_sessions` had taken its registry snapshot. Such a session does register, so `sweep_active_sessions` puts its DB row right, but it is neither cancelled nor waited on, and `app.exit(0)` orphans its agent child — own process group, and an exit runs no `kill_on_drop` destructors. That is the one outcome the sweep cannot fix. The review classed it as a new decision rather than a defect, and it is the last coverage gap in the invariant this branch exists for, so it is closed the way the review framed it: one gate in each of the two funnels rather than a gate per caller. Every agent start now reaches `register_for_startup` by way of `start_session`, and every pipeline reaches `start_pipeline_session`; the project-MCP `start_repo_session` only ever enqueues a row and drains, so it inherits the drain's gate. A refused start must not be an `Err`. An error from here becomes an errored session one way or another — the pipeline-handoff path writes `error`/`Crashed` outright via `finish_failed_pipeline_handoff_start`, and everywhere else the row is simply left `running` under our pid for the next launch to report as one, which is the artifact this branch exists to prevent. So the refusal does what the sweep would have done and answers with a non-error outcome: - The row is written `cancelled`/`app_quit` by `finish_cancelled_before_run` (`finish_cancelled_startup` generalized to scalars, since `PipelineConfig` needs it too). Leaving that to the sweep would be right only until the sweep runs: it is `shutdown_cleanup`'s last step, so a refusal in the window between it and `app.exit(0)` would strand a `running` row. - The registry entry is dropped, and a cancellation it had already accepted is preferred over `AppQuit`. That matters on one path: the pipeline handoff skips `deregister` so `start_session`'s `register` can swap the token with no gap. A refusal never reaches that `register`, so without this the pipeline's entry would outlive the thread that owned it — `is_running` true forever, and `wait_for_sessions` burning the whole budget on a session nothing can stop. Taking the entry out means taking over its job of recording the terminal state, which is the rule `15735fb1` named. - Both functions return `SessionStartOutcome` instead of `()`. Most callers keep `?;` and are right to — the row and the status event are already correct, so returning their session id says what actually exists. The callers that care are the queued-branch drain and the two queued pipeline starts in `prs`, which now report `Ok(false)` (a refusal leaves the branch as idle as a claim that lost its race), and the pipeline handoff, which resolves its own artifact the way its `Cancelled` arm does. Of the drain's three gates, two stay and one goes. The entry gate and the per-iteration gate bail *before* `transition_queued_to_running`, and an unclaimed row is a different DB state, not just an earlier one: still `queued`, carrying no owner, and so still available to another instance pointed at the same data dir right up until our sweep reaches it — the case `abc299a9` made the sweep's CAS ownership-aware for. Claiming and then refusing would stamp our pid on work we are about to throw away. The third gate, added by `5d6d7441` after the claim, has no such argument: it fires at the same point as the runner's gate and deliberately stranded the claim for the sweep, which the runner's gate has no need to do. It is removed rather than kept as a strictly worse duplicate. Also from that review: the pre-connect gate's inventory of what precedes it named only driver construction and the env snapshot capture. Between those and the check, the async block also starts the project MCP server, starts the pikchr MCP server, and reads plus base64-encodes the attached images. They are awaits rather than blocking work and none of them spawns an agent child, so the gate's reasoning is unaffected — but a session cancelled as early as its registration does stand two localhost HTTP servers up before bailing (tasks on the session thread's runtime, so they go down with it). The comment now says what is there. Deliberately not covered: `generate_pikchr`'s diagram sub-session reaches its agent through `register_external` and `driver.run`, not `start_session`, so a tool call landing after `cancel_owned_sessions`' snapshot registers a token shutdown never fires. It is narrower than anything closed here — the parent session has to be mid-turn and already being cancelled — and it is a different cancellation chain, so it belongs to its own change rather than folded into this one. Tested at the level the gate allows. The registry half needs no app at all: which reason a refusal persists, and that it frees the handoff predecessor's entry so `wait_for_sessions` stops waiting on it. The whole refusal is then driven against a real store with a mock app, which making `emit_status` runtime-generic is what enables. The gate itself is still out of reach — `start_session` takes the concrete `AppHandle` the spawn path needs. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
A review of `89c821c3` left three findings on the startup path, all about a cancellation surviving the trip from the registry to the row. **The carry-forward race.** `register` reads `previous.accepted_cancellation()` under the registry lock, but `cancel_with_completion_reason` cloned the entry's `Arc` under the lock and applied the cancellation after releasing it. A cancel caught between those two points reads as `None`: the replacement starts clean, the cancel then lands on an `Arc` the map no longer holds, and `cancel` still answered `true` — so `cancel_session_impl` wrote no status. On the handoff path that is exactly what `15735fb1` set out to close: the id is in shutdown's cancel snapshot, the AI session runs uncancelled, `wait_for_sessions` burns the whole `SHUTDOWN_BUDGET`, and `app.exit(0)` orphans the agent child. The review offered liveness-reporting — have `apply_cancellation` say whether it landed on an entry still in the map, so `cancel` can answer `false` and let `cancel_session_impl` take its store-write fallback. Applying under the lock is the better of the two, and the earlier session was wrong to have called it a bigger contract change: `register` has always applied a carried or pending cancellation with the guard held, and the pre-`86e4efbe` registry cancelled straight out of the map the same way. Nothing in `apply_cancellation` reaches back into the registry — the reason mutex is only ever taken *under* the registry lock, never the reverse, and `CancellationToken::cancel` notifies its waiters, which schedules the waiting tasks rather than running them inline — so the lock scope costs nothing. Liveness-reporting, by contrast, answers `false` to cancels that *were* honoured (the entry's successor or its remover took them over), buying a duplicate store write and a reason downgraded to `Interrupted` for a quit's `AppQuit`. The mechanism is `RegistryInner::cancel_registered`, which takes `&RegistryInner` rather than `&SessionRegistry` so the lock guard is the only way to reach it; `cancel_with_completion_reason` and `cancel_or_defer` both route through it. That closes both sites at once — the `register` carry, and the `deregister_reporting_cancellation` removal that `register_for_startup`'s doc had been documenting as open. The window was narrower than either the review or that doc said, which is worth recording rather than losing. `apply_cancellation` holds the reason mutex across `token.cancel()`, and both takeover readers go through `accepted_cancellation`, which wants that same mutex — so a cancel already *inside* `apply_cancellation` was serialized against them anyway, and what was exposed is the instant between the map lookup and that mutex. That is an accident of two unrelated lock scopes, invisible at both sites and undone by any tidying that releases the reason guard before firing the token. The two new tests pin the invariant to the registry lock rather than to the accident: they park a cancel inside `apply_cancellation` — with a waker that blocks, since `cancel` notifies its waiters synchronously, the only interposition point this race has — and require the swap and the removal to wait for it. Against the old split they pass, for the incidental reason; against the old split with the reason guard released early they both fail; under the registry lock they hold either way. **The failed handoff talking over a Stop.** When the handoff's `start_session` failed with a carried cancellation, `finish_failed_pipeline_handoff_start` correctly lost `transition_from_running` — `finish_cancelled_before_run` had already written `cancelled` and emitted it — but the `emit_status` behind it fired regardless. The last event a client saw was `error`/`Crashed` on a row that says cancelled, and the AiHandoff step was stamped "Failed to start AI session: …" for what was the user's Stop. Both now hang off `transitioned`, the way the other three `PipelineOutcome` arms gate their side effects. The emit keeps one escape the review didn't call for: a row that is *gone* rather than moved on, when the user deletes the pending commit mid-pipeline. That is the case the unconditional emit existed for — no other writer emitted anything, and the event is all that clears the client's `running` state — so gating on `transitioned` alone would have quietly dropped it. All of it moves into `finish_failed_pipeline_handoff_start`, now generic over the runtime and taking the `PipelineConfig`, which is what puts it in reach of a test: a mock app and a real store drive both the win (row `error`/`Crashed`, step `Failed`) and the loss (row still cancelled/`Interrupted`, step still pending). **The startup failure not draining its branch.** The session thread's terminal path drains the branch queue for any `branch_id.is_some()` once its transition wins, cancelled sessions included. The startup-failure path didn't, so a Stop landing during a startup that then fails cleared the row and left the branch's remaining queued sessions parked until some unrelated terminal transition happened to trigger a drain. `finish_cancelled_before_run` now reports whether it won the transition, and `start_session` kicks the drain on that answer, through the pipeline arms' own helper — renamed `drain_queued_after_pipeline_terminal` to `drain_queued_after_terminal_state`, since pipelines are no longer its only callers. Safe on the quit path because the drain is `is_quitting`-gated. The refusal deliberately kicks none: it only ever runs while that gate is closed, and a queued row left unclaimed is what a quit wants to leave behind. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
…t the quit `1775b4e8` gated every session start on the shutdown, one gate in each of the two funnels, and named what it deliberately left out: `generate_pikchr`'s diagram sub-session reaches its agent through `register_external` and `driver.run` rather than `start_session`, so a tool call landing after `cancel_owned_sessions` has snapshotted the registry registers a token nothing will ever fire, and spawns an agent child `app.exit(0)` then orphans — own process group, and an exit runs no `kill_on_drop` destructors. Narrower than anything closed there (the parent session has to be mid-turn and already being cancelled), but the same invariant, and the last way into an agent process that the shutdown could not see. `register_external` is not the chokepoint, despite being the shape the gap is described by. Its only production caller is that worker — the other two are tests, which have no business consulting a quit flag — the registry has no route to the Tauri-managed `QuitState`, and, decisively, the correct order is claim *then* ask, which a gate inside a function whose whole job is to hand out a token cannot express. So the gate goes in the worker, as `pikchr_mcp::reserve_child_session`, and the obligations it discharges are written on `register_external` where the next external registrant will meet them. **Claiming first is what closes the window rather than narrowing it.** The funnel gates in `start_session` / `start_pipeline_session` read `quit_in_progress` with nothing yet registered for a snapshot to find, so they shrink their race and say so. Here both sides are ordered: `shutdown_cleanup` publishes the flag and *then* snapshots the registry; the worker registers and *then* reads the flag. - A claim landing before the snapshot is in it. Shutdown fires the token, `forward_user_cancel` hands it to the worker, the `is_cancelled` check `generate_pikchr_source` takes before each `driver.run` refuses to start an agent at all, and `wait_for_sessions` holds the exit open until the worker deregisters. If the worker's own startup outruns `SHUTDOWN_BUDGET` — the preview server plus `AcpDriver::new`'s login-shell probes — shutdown times out, warns, sweeps the row and exits, killing the thread with the process: the clean end `89c821c3` described for `register_for_startup`, and for the same reason, that the gate means no agent child was ever started. - A claim landing after the snapshot reads a flag already published, and refuses here. Both fail together only if the claim landed after the snapshot *and* the read landed before the publish — impossible given the two program orders (publish → snapshot, register → read) and the total order of the registry mutex. **A refusal leaves nothing behind**, which is what lets it be a plain early return instead of `1775b4e8`'s `finish_cancelled_before_run`. The claim now happens before anything is persisted, so `create_pikchr_child_session` splits into `new_pikchr_child_session` (build the row) and `persist_pikchr_child_session` (write it), and a refused call writes no session row for the sweep to chase and announces no diagram session into the parent's transcript that never drew anything. Holding a registry entry for an id whose row doesn't exist yet is safe by construction: the row and the announcement are what publish the id, both come after the check, and the only thing that can reach an unpublished id is the shutdown walking every registered one, which fires the token and waits for the entry to go. **What the refusal returns** is an error, like every other failure in this tool. There is no MCP way to say "stop your turn", so a clear terminal refusal and a retryable failure are the same wire shape; what makes the retry harmless is that the refusal is free — no row, no registry entry, and above all no agent child — so a loop spins on an atomic load while the parent session, cancelled by the same shutdown, is torn down underneath it. The message says not to bother. **The wait was also measuring the wrong thing.** Shutdown does already cover `register_external` entries — `cancel_owned_sessions` and `wait_for_sessions` walk the whole registry, not just what the runner started — but the guard was held by the parent MCP request future, which only *awaits* the worker and is dropped as soon as the parent session's runtime goes down. On the quit path the parent is cancelled in the same loop as the child, so the two teardowns race, and a parent finishing first retired the child's entry while the specialist's agent CLI was still being stopped: `wait_for_sessions` returns, the sweep runs, and the exit proceeds over the child it was supposed to be waiting for. The guard moves into the worker thread, declared first so it drops last — after that thread's runtime and every task on it, which is where the agent child actually lives. The same move fixes the normal path's inconsistency in the safe direction: the entry now retires a hair after the tool result rather than a hair before the worker is done. Tested where the ordering is reachable without an `AppHandle`, which is why `reserve_child_session` takes the quit check as a parameter: a reservation is visible to a registry snapshot taken at the last instant the gate could still say "carry on", and a refusal releases the slot it claimed so a shutdown that did catch it stops waiting. The gate's own call site stays out of reach — `is_quitting` wants the concrete `AppHandle`, and driving `generate_pikchr` needs an MCP `RequestContext`. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
…awns `a7f6de3e` gated the diagram sub-session on the shutdown and described the rest of the chain: shutdown fires the reserved token, `forward_user_cancel` hands it to the worker, and the `is_cancelled` check `generate_pikchr_source` takes before each `driver.run` is what keeps an agent from starting. A review of that commit showed the handoff cannot happen in time for the first attempt, which is the only one a shutdown races. `LocalSet` polls the main future first and ticks spawned tasks only once it returns Pending. Between the `spawn_local` and that check nothing yields; the check reads the worker's token rather than the registered one; and the next yield is inside `AcpDriver::run` → `connect`, which has no await and no token check before `cmd.spawn()`. So the forwarder is first polled with the specialist's agent child already running — the spawn-then-teardown shape `89c821c3` deliberately removed from the main session loop, whose gate reads its registered token directly and so never had the problem. Usually that resolves: the forwarder fires at connect's first await, `run_acp_session`'s select aborts, `graceful_stop` kills the child, and `wait_for_sessions` is still holding the exit open. It bites in exactly the case this gate exists for — that teardown outrunning `SHUTDOWN_BUDGET`, after which `app.exit(0)` orphans the child: own process group, and an exit runs no `kill_on_drop` destructors. The worker now takes a synchronous look at the registered token immediately after spawning the forwarder, through `arm_worker_if_user_cancelled` — the record-then-cancel body lifted out of `forward_user_cancel`, so the pre-check and the forwarder can't tell different stories about the same cancellation. A quit and a Stop are indistinguishable here (`cancel_owned_sessions` fires the registered token exactly as `cancel_session` does), so the reason recorded is the one the forwarder would have recorded; both steps are idempotent — first reason wins, a second `cancel` is a no-op — so which of them arrives first doesn't matter. That covers every cancellation up to that point, including the seconds `AcpDriver::new` spends probing login shells, which is the bulk of the startup a quit can land in: the worker reaches the pre-`driver.run` check with its own token already armed and bails having started nothing. What remains is a cancel arriving in the few statements between the look and the spawn, which is the forwarder-and-teardown race again — now a slice of the startup rather than all of it. The comment at the site says that, instead of claiming the coverage the review disproved. Tested where the ordering is reachable without an `AppHandle`: a token fired before the worker body, then the forwarder spawned and the look taken in the production order, against a driver whose `run` records that it was reached. Without the look that driver runs — the assertion fails, since `LocalSet` polls the main future first — and with it the row still lands cancelled/`Interrupted` carrying the Stop message, which is what the spawn-then-teardown path produced. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
matt2e
force-pushed
the
closing-window-should-not-quit
branch
from
September 23, 2026 05:49
2fa4447 to
a7ac34f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closing the window (red button / Cmd+W) terminated Staged, taking every running agent session with it. Cmd+Q was worse:
PredefinedMenuItem::quitmaps toNSApp terminate:, which reaches no Tauri hook, so it skipped the action-shutdown handler entirely and left agent CLIs orphaned.A new
app_lifecyclemodule owns both halves, adapted to the multi-window model from #928.Close-to-hide
Destroyedhook does the per-window cleanup.CloseRequestedand hides the window instead, so sessions keep streaming. The Dock icon (RunEvent::Reopen), the newWindow ▸ Stageditem, or a quit brings it back;show_a_windowprefersmainfor its restored geometry but recovers any survivingwin-Npeer.tauri-{label}PR-poll client to its unfocused tier, which a hidden window's missing webview blur would not.Gated quit
request_quitcan gate it on active sessions. The confirmation is an unparented native alert —rfdroutes a parentless dialog toCFUserNotificationDisplayAlert, which the system displays rather than AppKit, so quitting from a fully hidden state never materialises a window and cancelling returns the app to exactly the state the user left it in.OkCancelCustom's default (Return) button holds "Keep Running"; a stray Return must not kill running agents.CompletionReason::AppQuit, waits for sessions and actions inside one shared 2s budget, sweeps any still-active rows to cancelled/app_quit, then exits. Queued sessions count as active; running actions are reported but don't gate. Ownership is checked againstowner_pid, so a quit never touches another Staged instance's work.Window ▸ Stagedroute through the shareddispatch_menu_eventrouter as focus-independent backend actions — every window being hidden is exactly when they matter.RunEvent::Exitruns the same idempotent cleanup, the only hook on theterminate:path, so Dock ▸ Quit and logout stop sessions and actions instead of orphaning them.quit_appis refused in the web-mode dispatch table: a browser client must not be able to terminate the desktop host. The store-incompatibility screens' Close buttons now quit rather than closing a window that would only hide.Known gaps
applicationShouldTerminate:) is deliberately left out — the shared cleanup already prevents the process and data damage there, only the prompt is missing.shutdown_cleanupre-queries active sessions instead of trusting the prompt's snapshot, and it keeps the second-Cmd+Q force-quit escape hatch dispatchable.Verified with
just check-all.