fix(app): keep sidebar running state through stale scope bootstrap snapshots - #1394
Conversation
…apshots A scope bootstrap response whose stamped seq trails the applied event watermark predates events the store already applied; reconciling it deleted live busy statuses and event-inserted sessions/cortex entries, leaving the sidebar without the running icon for the rest of the turn. Event-authoritative buckets now merge (event state wins, snapshot fills gaps) and the per-scope store registry is reactive so consumers that observed no store re-run on creation and eviction. Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🧹 Oryn Mini · ✅ Task completeCode review · Attempt 1 · Cycle 1 Waiting for your decision: Does the per-key ScopeWriteTracker rewrite on 2b607d6 satisfy the review thread's P1 (scope-wide merge preserving stale state), clearing 'changes requested'? Run details · operational receipt{
"version": 1,
"workKey": "review:3592ef4f3f4594f1c23e5931",
"sourceKey": "3592ef4f3f4594f1c23e5931",
"intent": "review",
"phase": "completed",
"attempt": 1,
"cycle": 1,
"leaseID": "3ed37c08-2833-4663-9100-6e7dc6d6a7cf",
"verdict": "actionable",
"triage": {
"category": "bug",
"priority": "P2",
"confidence": "high",
"reproduction": "source_proven",
"proof": "sufficient",
"decision": "keep_open",
"closeReason": "none",
"evidence": [
{
"path": "packages/server/src/server/server.ts",
"line": 385,
"sha": "2b607d621467dfe58f92cf50944858e24da060ed",
"detail": "Middleware captures Bus.currentSeq() before await next(), i.e. before the bootstrap handler reads snapshot fields, then sets x-synergy-seq/x-synergy-epoch — confirming the stamp is a conservative lower bound, the PR's root-cause premise."
},
{
"path": "apps/web/src/context/global-sync.tsx",
"line": 906,
"sha": "4d60cd1b31f4125f62d13dec82783a7df13d1f5f",
"detail": "Base revision applied data.sessionStatus with an unconditional reconcile(), no version gate — the mechanism that deleted a just-applied busy key when a stale snapshot landed."
},
{
"path": "apps/web/src/context/global-sync.tsx",
"line": 930,
"sha": "2b607d621467dfe58f92cf50944858e24da060ed",
"detail": "Head gates session_status, sessions, and cortex application through tracker.mergeStatus/mergeSessions/mergeCortex; unversioned or tracker-less responses keep the plain reconcile; watermark update at 962-966 never moves backward within an epoch."
},
{
"path": "apps/web/src/context/scope-snapshot-merge.ts",
"line": 62,
"sha": "2b607d621467dfe58f92cf50944858e24da060ed",
"detail": "ScopeWriteTracker overlays only keys whose last sequenced write has seq > version.seq; syncEpoch clears all tracking on epoch change so reset-resync snapshots stay authoritative."
},
{
"path": "apps/web/src/context/global-sync.tsx",
"line": 1423,
"sha": "2b607d621467dfe58f92cf50944858e24da060ed",
"detail": "session.status events record statusWrite per sessionID; session.updated records sessionWrite with archive tombstones (line 1368) and touched Cortex writes; cortex.task.created/completed/tasks.updated record at 1798/1812/1817."
},
{
"path": "apps/web/src/context/global-sync.tsx",
"line": 462,
"sha": "2b607d621467dfe58f92cf50944858e24da060ed",
"detail": "peekScopeState() reads the registry-version signal; ensureScopeState bumps on creation (508) and releaseScopeState bumps on eviction while also deleting the scope's write tracker (551-554)."
},
{
"path": "apps/web/src/components/sidebar/sidebar.tsx",
"line": 1215,
"sha": "2b607d621467dfe58f92cf50944858e24da060ed",
"detail": "SidebarSessionRow's visual memo calls peekScopeState inside createMemo, so the registry signal re-runs rows that first observed no store — the secondary fix's consumer path."
},
{
"path": "apps/web/test/context/global-sync-snapshot-event-race.dom.test.ts",
"line": 60,
"sha": "2b607d621467dfe58f92cf50944858e24da060ed",
"detail": "'Reviewer scenario' block: pre-stamp stale busy converges to the snapshot, post-stamp session insert and whole-bucket Cortex replacement survive; plus busy-survival, registry reactivity on create/evict, and fresh-scope population."
},
{
"path": "docs/decisions/implemented/bug-fix/2026-09-15-merge-stale-bootstrap-snapshots.md",
"line": 20,
"sha": "2b607d621467dfe58f92cf50944858e24da060ed",
"detail": "Decision record documents the first revision's scope-wide flaw (the review P1), the per-key decision, rejected alternatives, and bounded-memory consequences; path matches the implemented/bug-fix lifecycle scheme."
},
{
"path": "apps/web/src/context/global-sync.tsx",
"line": 783,
"sha": "2b607d621467dfe58f92cf50944858e24da060ed",
"detail": "loadSessions() applies session.list responses with plain reconcile and no freshness gate — pre-existing sibling of the fixed race, unchanged and out of this PR's scope."
}
],
"risks": [],
"decisionNote": {
"question": "Does the per-key ScopeWriteTracker rewrite on 2b607d62 satisfy the review thread's P1 (scope-wide merge preserving stale state), clearing 'changes requested'?",
"options": [
"Approve: per-key tracking plus the 'Reviewer scenario' DOM assertions directly cover the flagged reset-resync convergence case",
"Request changes: additionally constrain mergeSessions upsert-prepending to newly inserted sessions only",
"Hold until the full head-SHA CI matrix finishes before re-review"
],
"recommendation": "Option 1 — the flagged failure mode is asserted in the new test and pre-stamp keys now converge to the snapshot including deletions; treat the session-ordering nuance as an optional follow-up rather than a merge blocker."
}
},
"run": "https://github.com/SII-Holos/synergy/actions/runs/34923928636",
"summary": "Waiting for your decision: Does the per-key ScopeWriteTracker rewrite on 2b607d62 satisfy the review thread's P1 (scope-wide merge preserving stale state), clearing 'changes requested'?\nValidation has not run. No repair patch was published by this attempt."
} |
![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: 3b31ff95ed
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| behindAppliedEvents | ||
| ? mergeSessionStatusSnapshot(data.sessionStatus, store.session_status) |
There was a problem hiding this comment.
In the reset/full-resync path, behindAppliedEvents is scope-wide rather than tied to status writes. If session A remains locally busy because its idle event was missed, the bootstrap can correctly omit A from sessionStatus, but any unrelated sequenced event for session B arriving after the response stamp makes this branch overlay every local status onto that authoritative snapshot, leaving A falsely running indefinitely. The session and Cortex unions have the same issue with stale rows. Track post-stamp writes and tombstones per affected collection/key instead of giving all local entries precedence, so fail-open resync still converges to server state.
AGENTS.md reference: apps/web/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
🧹 Oryn Mini · 🧭 Your input is needed🟡 P2 · Normal · 🐛 Bug · High confidence Why this PRThe sidebar intermittently showed a running session as idle for an entire turn because a bootstrap snapshot stamped by the server before its own read landed after a busy event and reconciled the status away, and a second gap let sidebar rows miss scope stores created after their first render. This PR keeps event state only for keys written after the snapshot's stamp via a per-scope tracker and makes the store registry reactive, which also answers the review's P1 about stale state surviving resyncs. The mechanism checks out against the server's stamp-first middleware and the new tests encode both the original race and the reviewer scenario;… Full explanationThe sidebar intermittently showed a running session as idle for an entire turn because a bootstrap snapshot stamped by the server before its own read landed after a busy event and reconciled the status away, and a second gap let sidebar rows miss scope stores created after their first render. This PR keeps event state only for keys written after the snapshot's stamp via a per-scope tracker and makes the store registry reactive, which also answers the review's P1 about stale state surviving resyncs. The mechanism checks out against the server's stamp-first middleware and the new tests encode both the original race and the reviewer scenario; what remains is reviewer sign-off on that thread plus the in-flight CI. What changed
How it fits togetherHow a bootstrap response, its seq stamp, per-scope event-write tracking, and the reactive store registry combine in applyScopeBootstrapSnapshot to decide what survives a stale snapshot. flowchart TD
accTitle: Stale bootstrap snapshot merge path
accDescr: How a bootstrap response, its seq stamp, per-scope event-write tracking, and the reactive store registry combine in applyScopeBootstrapSnapshot to decide what survives a stale snapshot.
n_0["Bootstrap snapshot response"]
n_1["Seq stamp lower bound"]
n_2["Applied sequenced events"]
n_3["ScopeWriteTracker post-stamp keys"]
n_4["applyScopeBootstrapSnapshot gate"]
n_5["Scope store buckets"]
n_6["Reactive registry signal"]
n_7["Sidebar row memo"]
n_0 -->|"stamped before read"| n_1
n_2 -->|"records per-key writes"| n_3
n_1 -->|"trailing response"| n_4
n_3 -->|"overlay post-stamp keys"| n_4
n_4 -->|"reconcile or merge"| n_5
n_5 -->|"session_status read"| n_7
n_6 -->|"store appears, memo re-runs"| n_7
Review findings🔵 Low · Sibling fetch-based session/cortex writers remain ungated — apps/web/src/context/global-sync.tsx:783 loadSessions() and refreshCortex() still apply their responses with a plain reconcile and no version or tracker check; a GET in flight across an event can in principle drop an inserted session or task row the same way the bootstrap snapshot could. This is pre-existing, unchanged by this PR, and outside its stated bootstrap/resync scope, but worth a follow-up if the stuck-state symptom ever resurfaces on session-list or Cortex refresh paths. 🔵 Low · mergeSessions prepends updated sessions, diverging from event-path ordering — apps/web/src/context/scope-snapshot-merge.ts:93 The event path keeps an updated session at its index and unshifts only new ones, but mergeSessions prepends every post-stamp upsert, so a session whose update postdates the stamp jumps to the top of store.session when a stale snapshot applies. All current consumers read rows via find/filter (session-visual-state, library-panel, layout), so nothing renders that array order today — latent only. Constraining prepends to newly inserted sessions would align the two paths. Before merge
Maintainer decision: Does the per-key ScopeWriteTracker rewrite on 2b607d6 satisfy the review thread's P1 (scope-wide merge preserving stale state), clearing 'changes requested'?
Recommendation: Option 1 — the flagged failure mode is asserted in the new test and pre-stamp keys now converge to the snapshot including deletions; treat the session-ordering nuance as an optional follow-up rather than a merge blocker.
Verification🔎 Evidence supports the conclusion. Confirmed in source; not reproduced in a live run. No host validation commands were run for this report. Source evidence (10)
Discussion and CI reflect the snapshot read for this review. Current checks and approval are verified separately before merging. 🧹 Oryn Mini · Source 2b607d6 · Built with Synergy core |
oryn-mini
Bot
added
oryn:kind: 🐛 bug
Review follow-up: scope-wide stale-snapshot merging kept every local entry alive, so a status left stale by a missed idle event survived an authoritative resync indefinitely. Event writes to session status, the session list, and Cortex now record their stamp per key; snapshot application overlays only post-stamp keys and honors archive tombstones, so fail-open resyncs converge to server state while genuinely newer event writes survive. Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
oryn-mini
Bot
added
oryn:status: 🧭 needs input
1 participant
Keep sidebar running state through stale scope bootstrap snapshots
Problem
The sidebar intermittently failed to show the running icon while a session was executing, and it stayed wrong for the whole turn. Two independent gaps caused this:
x-synergy-seqonGET /scope/bootstrapbefore reading snapshot fields (a conservative lower bound). When a bootstrap or reconnect-resync response was still in flight and asession.statusbusy event arrived over the WebSocket first, the client applied the event — then the older snapshot's unconditionalreconcile(data.sessionStatus)deleted the busy key, because the server'slistStatuses()read had happened before the turn started. Session status is sticky: no further status event fires until the next transition, so the idle rendering persisted until turn end, by which point the running state was over. The same ungated reconcile could drop event-inserted entries from thesessionlist andcortexcollections. Triggers: reconnect recovery (hidden-tab ping drop, epoch reset after server restart, seq gap),scope.runtime.disposed, and scope-store LRU eviction + re-creation — hence the probabilistic symptom.peekScopeState()reads were invisible to Solid. A sidebar row whose memo first evaluated before the store existed subscribed to nothing and never re-ran when an arriving event created the store.Fix
applyScopeBootstrapSnapshotnow compares the response version against the scope's applied event watermark. When the response shares the watermark's epoch but trails its seq, the event-authoritative buckets (session_status,session,cortex) merge instead of reconciling: live event state wins, the snapshot only fills gaps (apps/web/src/context/scope-snapshot-merge.ts). Unversioned and same-or-newer responses keep the plain reconcile path, so ordinary bootstraps are unchanged.peekScopeState()observes a registry-version signal bumped on store creation and eviction, so consumers that first sawundefinedre-run when the store appears.Verification
Failing-test-first: the race is reproduced by
apps/web/test/context/global-sync-snapshot-event-race.dom.test.ts(busy event applied → stale seq-0 bootstrap response arrives → busy must survive; registry reactivity on create/evict; fresh-scope snapshot still populates).bun test test/context/global-sync-snapshot-event-race.dom.test.ts test/context/scope-snapshot-merge.test.ts— passbun test test/context/global-sync-lifecycle.dom.test.ts test/context/scope-retention.test.ts test/context/scope-recovery-retry.test.ts test/context/scope-reconnect-recovery.test.ts test/components/sidebar/— 100 passbun run --cwd apps/web typecheck— passbun run quality:quick— all 17 gates passbun run --cwd apps/web test: non-Playwright batches pass. The remaining failures are Playwright-isolated suites failing identically on the clean base of this fresh worktree (verified via stash: unbuiltpackages/plugindist, now built, plus a dev-server port conflict), so they predate this change and are environmental.Docs:
docs/architecture/frontend-data-sync.md(store-shape section + invariants) and decision recorddocs/decisions/implemented/bug-fix/2026-09-15-merge-stale-bootstrap-snapshots.md.