feat(sessions): make busy state server-authoritative via snapshot hydration - #904
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: 2488f762bf
ℹ️ 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".
| (session.sessionType as SessionType) ?? 'other', | ||
| session.branchId ?? undefined | ||
| ); | ||
| projectStateStore.addRunningSession(session.projectId, session.sessionId); |
There was a problem hiding this comment.
When a page reloads or the WebSocket reconnects while a PR/push session is already running, the snapshot can include sessionType: 'pr' or 'push', but this hydration path only fills the generic session registry/project spinner. BranchCardPrButton derives its button state from prStateStore/pushStateStore, so those buttons stay idle and can present Create PR/Push actions for the same branch instead of opening the active session; seed setPrCreating/setPushing for hydrated PR/push entries with a branch id.
Useful? React with 👍 / 👎.
matt2e
force-pushed
the
who-owns-busy-state
branch
from
August 5, 2026 01:35
2488f76 to
784338d
Compare
Session busy state is authoritative in SQLite, but clients could only
learn it by accumulating session-status-changed events, so a reload or
late connect lost every running/queued indicator. Add a
get_active_sessions command that projects running and queued sessions
to {sessionId, projectId, branchId, sessionType, status, isAutoReview},
deriving branch, project, and type from linked artifacts exactly as
resume_session_for_store stamps them onto "running" events; isAutoReview
mirrors SessionStatusEvent so later hydration can keep suppressing
auto-review spinners. The command is registered in both the Tauri
handler and the web dispatch() match so both transports serve it.
Purely additive (step 1 of the busy-state plan): no existing behavior
changes; frontend hydration lands separately.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…pure projections Client busy state (project spinners, session registry) was accumulated solely from session-status-changed events, so a window reload lost every running indicator and a missed terminal event left a spinner stuck forever (the registry's TTL eviction being the only, lying, escape hatch). Switch the frontend to snapshot-then-deltas (step 2 of the busy-state plan): hydrate from the get_active_sessions command landed in step 1, then apply event deltas on top. hydrateActiveSessions() runs at the three moments the plan calls out: app/page startup (from listenForSessionStatus), every WebSocket onopen in transport.ts (same spot as the PR-poll interest replay), and the page-lifecycle cache-stale resume signal. The snapshot is authoritative for WHICH sessions are active: local entries the backend no longer reports are swept (healing stuck spinners), except entries registered while the fetch was in flight, so optimistic launch-site registrations can't be raced away. Per-session metadata prefers what the client already tracks, because launch sites know things the snapshot cannot resolve: pipeline (pr/push) sessions link no artifact so their branch/project come back null, and adopted auto reviews are registered by BranchCard. Unseen snapshot entries apply the exact gating of the live running event — running, resolved project, not an auto review (using the isAutoReview field the step-1 projection carries) — so queued sessions still wait for their own running event. With the backend guaranteeing terminal events (state machine plus recover_dead_sessions), the TTL/max-size eviction in sessionRegistry.svelte.ts is deleted; the registry and projectStateStore.runningSessions are now pure projections. Unread state stays client-local. prState/pushState keep their own workflow TTLs — those are per-device UX state, not busy-state mirrors. Covered by new sessionStatusListener tests (snapshot gating, local metadata precedence, stale sweep, in-flight race guard, fetch-failure no-op, cache-stale re-hydration, delta path) and an extended transport test asserting re-hydration on socket open and reconnect. Server-side completion side-effects remain a separate step. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The busy-state note's step 3: completion outcomes for pipeline (pr/push) sessions were parsed and persisted by whichever connected frontends happened to observe the terminal event — each client fetched the transcript, extracted the PR URL or classified the push, and raced to call updateBranchPr / clearBranchPrStatus. The session runner now owns those side-effects. When it wins the atomic transition_from_running, a new session_completion module parses the PR URL (pipeline step outputs, then transcript) or classifies the push outcome, persists the result in Rust, and emits pr-created / push-completed domain events *before* the terminal session-status-changed event, so clients can reconcile from the ordered stream. The frontend completion handlers become idempotent renderers that perform no writes. Also addresses the branch-attribution gap deferred from step 1: pipeline sessions link no artifact row, so a new sessions.branch_id column (migration 0021) records the branch at launch. Snapshot projection, session resume, and the completion hook all fall back to it, and the busy-state snapshot can now resolve pipeline sessions the client has never seen. Details: - Fix infer_branch_resume_session_type needles that never matched the actual stored pipeline prompts (trailing periods), and cover force push. - Factor refresh_pr_status / clear_branch_pr_status into shared impl fns; the web dispatch arm for clear_branch_pr_status now emits pr-status-cleared like the Tauri command. - BranchCardPrButton's modal-close completion handlers keep their read-only classification as a fallback but no longer write. - Backend tests: session_completion unit tests (URL extraction, push classification, event decisions), branch_id round-trip/resolution, snapshot pipeline-session resolution; frontend tests: pr-created / push-completed rendering, terminal reconciliation, and a no-writes guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ltas The in-flight race in hydrateActiveSessions was guarded in one direction only: the sweep spared entries registered after fetchStartedAt, but the register loop had no symmetric guard. A terminal session-status-changed delta processed while the snapshot fetch was in flight would cleanupSession the entry, and the register loop would then resurrect it from the older snapshot that still reported it running — recreating exactly the stuck spinner hydration is meant to heal, until the next sparse recovery trigger (WS reconnect, cache-stale, app restart). Close it symmetrically: terminal deltas record their arrival time in a module-level map while any hydration fetch is in flight, and the register loop skips sessions whose terminal delta arrived at or after its own fetchStartedAt. The map is keyed by arrival time so overlapping hydrations each compare against their own fetch start, and is cleared once no fetch remains in flight, so a later snapshot that legitimately reports the session running again (backend-side resume) registers it normally. Covered by two new tests: the terminal-delta-during-fetch race no longer re-registers the session, and a subsequent hydration can still register it after the guard record is released. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
extract_pr_url's marker pass only trusts Assistant/ToolResult messages,
but the fallback pass scanned every role, so a PR URL pasted into a
queued user follow-up ("see https://github.com/org/repo/pull/3") could
be persisted server-side as the branch's PR number via
update_branch_pr_number. Apply the same role filter to the fallback
pass: a URL in a user message is not evidence the session created that
PR. Flagged as a carryover suggestion in the review of 8c9cf35.
Covered by a new fallback_pass_ignores_user_messages test; the doc
comment now states the role restriction applies to both passes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ed enum The two pipeline terminal arms (CompletedWithoutAi and Aborted) passed a hardcoded "completed" to run_completion_side_effects and emit_status, duplicating the status already expressed by the SessionStatus::Completed value handed to transition_from_running a few lines up. If either transition ever changed (e.g. the abort arm stops masquerading as completed), the hook's status != "completed" gate would silently diverge from the actual transition. Flagged in the review of 784338d. Bind the status enum once per arm and thread status_enum.as_str() through both the hook and the terminal event, matching how the AI session path already derives new_status from a single source. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ession Everything `run_completion_side_effects` gates on persists on the session row forever: the pipeline JSON and prompt are never cleared, and migration 0022 gave pipeline sessions their own branch_id. So resuming an already-finished pr/push session (asking a question in a completed push session, say) re-ran the hook when that follow-up turn completed, against the *old* pipeline and transcript: the stale all-succeeded push pipeline re-classified as a fresh success, wiping the branch's PR checks via clear_branch_pr_status_impl and re-flashing push-completed on every client; pr sessions re-emitted pr-created. Before the side-effects moved server-side this couldn't happen, because resumed pipeline sessions had no branch attribution and the hook bailed. Flagged in the review of 784338d. Add a one-shot marker column, `sessions.completion_effects_at` (migration 0023), checked at hook entry and written when the hook delivers an outcome. Chosen over threading a "this run owns the pipeline" flag through PipelineConfig/SessionConfig, which would kill legitimate recovery turns — a pipeline session whose AI turn errored or was cancelled, then resumed to actually finish creating the PR, must still fire pr-created on its first real completion — and because a persisted marker also protects the existing inventory of historical completed pipeline sessions, which the migration backfills. The marker means "outcome events for this session were delivered once", not "the session finished once": error/cancelled terminal states never reach evaluation (status gate) and never mark. PrUrlMissing deliberately doesn't mark either — it emits and persists nothing, so leaving it unmarked preserves the recovery turn where the next completion scans the new transcript and fires pr-created for real. Marking happens *before* emitting: dying in between merely loses one emission (recover_branch_pr, the PR refresh loop, and the clients' read-only fallback all cover that), whereas the other order would let a later resume replay the destructive push re-clear. No call-site changes — all three terminal paths go through the hook, which owns the gate. The status/marker/pipeline/kind gates plus outcome evaluation are folded into a pure `pending_completion_effect`, with transcript loading kept lazy so ordinary AI completions still don't read their messages, and `should_record_effects` isolates the PrUrlMissing exemption; both are covered by AppHandle-free unit tests. Store tests cover the default and the marker round-trip, and a migration test covers the backfill (completed pipeline sessions marked; running, errored, and non-pipeline ones left NULL). The two pre-existing repair-path migration fixtures gained the `status`/`updated_at` columns their real schemas always had, since the backfill reads them. No frontend changes: clients are already idempotent renderers, and a resumed session's completion now emits only the terminal status event. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Hydration treats the `get_active_sessions` snapshot as authoritative for which sessions are active and sweeps local registry entries the backend no longer reports, which heals stuck project spinners. But the sweep left prState/pushState alone (scoped out of step 2 as per-device UX state), so a client offline through a PR session's *entire* completion — laptop asleep, WS dropped — missed both the `pr-created` domain event and the terminal `session-status-changed` event, and its branch chip stayed in "Creating PR…" / "Pushing…" for a session that finished long ago. Only BranchCardPrButton's 5s poller healed it, and only while the card was mounted. Flagged in the review of 784338d. The sweep already knows enough to do better: the swept entry's branch is resolvable through the stores' validating lookups right up until `cleanupSession` destroys the registry metadata. Collect swept sessions a workflow store is still rendering as in-progress, then — after the snapshot window closes, so the in-flight counter and `terminalWhileFetching` guard keep covering only the fetch/apply — look up each one's persisted status (one `getSession` per genuinely stuck chip, normally zero) and reconcile. The delta-path reconcilers can't be reused as-is: they read the ordered event stream, so `handlePrCompletion` on a `completed` session would render "no PR URL was found" for a PR that actually succeeded, and that error state isn't overridden by `branch.prNumber` in the chip derivation, so the false failure would stick. Since step 3 made the backend persist the outcome at the terminal transition, a `completed` session's honest reconciliation is to drop the stale in-progress state and let persisted state render: the PR chip falls back to `branch.prNumber` (created, or idle when the session genuinely produced no URL) and the push chip returns to its git-state-derived affordance, skipping the 1.5s "done" flash for a completion that may be minutes old. `error`/`cancelled` are unambiguous and reuse the delta path's copy (`handlePrCompletion` for PR, so it stays in one place); a null or throwing lookup errors out as "Lost track of …", mirroring the card poller. Race safety comes from re-checking the tracked session id after the await: a terminal delta clears it and a relaunch replaces it, so either way the reconciliation skips. A backend-side resume between snapshot and lookup (`running`/`queued`) leaves the chip alone for its running event. Overlapping hydrations can't double-reconcile, because the first sweep removes the registry entry the second's collection step resolves the branch through. Covered by nine new tests: pr/push completion clearing, failure and cancellation rendering, both lost-track paths, the resumed-session and relaunch guards (before and during the lookup), and a non-pipeline swept session that triggers no lookup at all. Module and function docs updated — the step-2 carve-out now covers unread state only. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… throws reconcileSweptWorkflowSession treated a thrown getSession the same as a null row: both painted the sticky "Lost track of…" error chip and cleared tracking. That faithfully mirrored the card poller's catch block, but the reconciler runs at exactly the moment a transient transport hiccup is most likely — immediately after the WebSocket reconnect that triggered hydration — and unlike the poller it never retries: the sweep already removed the registry entry, so no later hydration re-collects the branch, and the error state isn't overridden by branch.prNumber in the chip derivation. One failed round-trip could permanently render a false failure for a PR that actually succeeded. Flagged in the review of 2048877. Distinguish the two outcomes: a throw is a transport failure, not evidence about the session, so log it and skip reconciliation entirely — the chip keeps its in-progress state and session tracking, which the mounted BranchCardPrButton poller heals on its next 5s tick (and the sticky chip is strictly less wrong than a false failure even while the card is unmounted). Only a lookup that succeeds and proves the row is gone (null) still errors out as "Lost track of…"; the terminal-status and resumed-session paths are unchanged. The lookup-throws test now asserts the skip (no error, no state or tracking cleared); the pr-flavored lost-track test switches to the vanished-session (null) shape, alongside the existing push variant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Clippy's `question_mark` lint rejects a standalone `if is_none() { return
None }`, so merge the pipeline check into the existing guard rather than
reaching for the cryptic `as_ref()?;` rewrite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
matt2e
force-pushed
the
who-owns-busy-state
branch
from
August 5, 2026 04:32
06049ba to
7ae3cbf
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.
Session busy state was authoritative in SQLite but reachable only by accumulating
session-status-changedevents, so a window reload lost every running indicator and a missed terminal event left a spinner stuck forever. Completion side-effects for pipeline sessions had the same ownership problem: whichever connected frontends happened to observe the terminal event each parsed the transcript and raced to persist the result.This moves both concerns to the backend and makes the client a projection of them.
Snapshot + deltas
get_active_sessionscommand projects running and queued sessions to{sessionId, projectId, branchId, sessionType, status, isAutoReview}, deriving branch/project/type from linked artifacts exactly asresume_session_for_storestamps them onto running events. Registered in both the Tauri handler and the webdispatch()match.hydrateActiveSessions()runs at app/page startup, on every WebSocketonopen, and on the page-lifecycle cache-stale resume signal. The snapshot is authoritative for which sessions are active — local entries the backend no longer reports are swept, healing stuck spinners — while per-session metadata prefers what the client already tracks, since launch sites know things the snapshot cannot resolve. Unseen snapshot entries apply the same gating as a live running event.fetchStartedAtsurvive the sweep, and terminal deltas that arrive mid-fetch block the register loop from resurrecting the session from an older snapshot.recover_dead_sessions), the TTL/max-size eviction insessionRegistry.svelte.tsis deleted.prState/pushStatekeep their own workflow TTLs — those are per-device UX state, not busy-state mirrors.Server-side completion
session_completionmodule runs when the session runner wins the atomictransition_from_running: it extracts the PR URL (pipeline step outputs, then transcript) or classifies the push outcome, persists the result, and emitspr-created/push-completedbefore the terminalsession-status-changedevent so clients reconcile from the ordered stream. Frontend completion handlers become idempotent renderers that perform no writes.sessions.branch_idcolumn (migration 0021) records the branch at launch; snapshot projection, resume, and the completion hook all fall back to it.infer_branch_resume_session_typeneedles that never matched the stored pipeline prompts (trailing periods) and adds force-push coverage; factorsrefresh_pr_status/clear_branch_pr_statusinto shared impl fns so the web arm emitspr-status-clearedlike the Tauri command.Tests
Backend:
session_completionunit tests (URL extraction, push classification, event decisions),branch_idround-trip/resolution, snapshot pipeline-session resolution. Frontend: newsessionStatusListener.test.tscovering snapshot gating, local metadata precedence, stale sweep, both in-flight race directions, fetch-failure no-op, cache-stale re-hydration,pr-created/push-completedrendering, and a no-writes guard; plus a transport test asserting re-hydration on socket open and reconnect.🤖 Generated with Claude Code