mptyl/ThothII
1
0
Fork 0
Code Issues 25 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix: make session resume atomic across restarts

Browse Source
This commit is contained in:
User
2026-07-15 00:42:35 +02:00
parent 2b4797f133
commit 08b1f4909e
12 changed files with 1082 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.superpowers/sdd/predeploy-fix-report.md
+266 -9
View File
@@ -319,6 +319,152 @@ cd frontend && npm run build
exit 0
```
## Integrated re-review closure (2026-07-15)
This section supersedes the earlier cold same-session assertion that the replacement URL carries
`lastEventId=8`. That behavior was correct only while the backend process and its in-memory id
sequence survived. A restarted backend begins a fresh sequence, so a successful cold Resume now
explicitly discards the browser's cursor before replacing the EventSource.
All four integrated re-review findings are closed:
1. `AppShell` passes a dedicated cursor-reset epoch to `useSessionStream`. A cold same-session
Resume increments it only after `alreadyActive: false`; a high cursor such as `901` is omitted
from the replacement URL and fresh low-id events/gates are consumed. An already-active
same-session Resume still preserves its source, cursor, and store.
2. `useSessionStream` no longer mutates the cursor ref during render. Effect setup resets cursor
state on session/reset-epoch changes, callbacks are guarded by a captured active-source
identity, and cleanup clears only its own active identity. A queued event from the replaced
source cannot write the new store or poison its next reconnect URL.
3. Backend Resume is serialized per session and rechecks runtime state inside the lock. Manifest,
readiness, and reopen validation precede the transport commit. Idle/failed replacement creates
and binds the new runtime before `hub.clear`, which occurs synchronously immediately before the
first `Resuming session` publish. Reopen/create failure returns exactly
`Session could not be resumed. Check configuration and connectivity, then try again.`, keeps the
prior hub buffer/subscribers attached, and does not expose exception sentinels. Concurrent calls
perform one cold start and the waiter returns `alreadyActive: true`.
4. `SseHub.forget(id)` removes subscribers, buffered events, and the last id. Permanent session
DELETE invokes it after disk deletion; ordinary close and Resume continue to use `clear`, which
preserves the id sequence.
### Re-review files
Production:
- `backend/src/pi/pi-process-manager.ts`
- `backend/src/routes/sessions.ts`
- `backend/src/sse/sse-hub.ts`
- `frontend/src/shell/AppShell.tsx`
- `frontend/src/stream/useSessionStream.ts`
Tests/support:
- `backend/test/pi-process-manager.test.ts`
- `backend/test/routes-sessions.test.ts`
- `backend/test/sse-hub.test.ts`
- `frontend/src/shell/AppShell.session-mgmt.test.tsx`
- `frontend/src/stream/useSessionStream.test.tsx`
- `frontend/src/test/fakeEventSource.ts`
### Re-review TDD RED/GREEN evidence
Frontend RED command:
```text
cd frontend && npx vitest run src/stream/useSessionStream.test.tsx \
src/shell/AppShell.session-mgmt.test.tsx
```
RED output (exit 1):
```text
Test Files 2 failed (2)
Tests 3 failed | 21 passed (24)
reset epoch: expected the old source to close, received false
cold same-session: expected /sessions/s1/events, received ?lastEventId=901
stale source: expected an empty transcript, received "stale session one"
```
Frontend GREEN command:
```text
cd frontend && npx vitest run src/stream/useSessionStream.test.tsx \
src/shell/AppShell.session-mgmt.test.tsx
cd frontend && npx tsc -b
```
GREEN output (exit 0):
```text
Test Files 2 passed (2)
Tests 24 passed (24)
TypeScript: no output, exit 0
```
Backend RED command:
```text
cd backend && npx vitest run test/sse-hub.test.ts test/routes-sessions.test.ts
```
RED output (exit 1):
```text
Test Files 2 failed (2)
Tests 8 failed | 28 passed (36)
three Resume ordering assertions observed clear before reopen/create
reopen and create sentinels escaped as raw HTTP 500 responses
the concurrent waiter cold-started again instead of returning alreadyActive: true
SseHub.forget was absent and DELETE did not invoke permanent cleanup
```
Backend GREEN command:
```text
cd backend && npx vitest run test/sse-hub.test.ts test/routes-sessions.test.ts
cd backend && npx tsc --noEmit -p .
```
GREEN output (exit 0):
```text
Test Files 2 passed (2)
Tests 36 passed (36)
TypeScript: no output, exit 0
```
The failure tests publish a post-failure probe through the same hub and prove that a subscriber
attached before either reopen or create rejection still receives it. The concurrency test overlaps
two same-id requests behind a deferred reopen and proves one manifest/readiness/reopen/create/clear
sequence.
### Initial re-review verification (before independent-review hardening)
```text
cd backend && npx vitest run
Test Files 22 passed (22)
Tests 182 passed (182)
cd frontend && npx vitest run
Test Files 43 passed (43)
Tests 273 passed (273)
cd backend && npm run build
> tsc -p tsconfig.json
exit 0
cd frontend && npm run build
> tsc -b && vite build
✓ 4835 modules transformed.
✓ built in 8.46s
exit 0
```
`git diff --check` produced no output (exit 0). The frontend build retains its pre-existing
large-chunk warning; no new build or type errors were introduced.
Final whitespace verification:
```text
@@ -328,15 +474,15 @@ no output, exit 0
## Self-review
- Resume sequencing: backend `clear` and runtime binding precede the HTTP success; frontend state
mutation and stream generation follow it. Failure catch only emits fixed UI copy.
- Resume sequencing: reopen and runtime binding precede backend `clear` and HTTP success; frontend
state mutation and cursor-reset epoch follow it. Failure catch only emits fixed UI copy.
- Already active: same-session returns before reset/generation/manifest repaint; different session
resets the single-session store and binds the new id only after success.
- SSE exact-once: ids are transport identity, not content hashes; replay is strictly `id > cursor`;
`clear` retains the counter; pending gate matching uses only descriptor id.
- Cursor behavior: hook tracks `MessageEvent.lastEventId`, carries it only to a same-id generation,
and resets it on session-id change. Native EventSource reconnect remains supported by the route
header.
- Cursor behavior: hook tracks `MessageEvent.lastEventId`, carries it only to an ordinary same-id
generation, and resets it on session-id/cold-runtime epoch change. Native EventSource reconnect
remains supported by the route header.
- Gate defense: the Zustand set survives pending clear but resets with the session store.
- Client boundary: raw `ensure.error` is unused in public responses; generic Pi system events are
reconstructed rather than spread; frontend type mirrors the two-field event.
@@ -348,12 +494,123 @@ no output, exit 0
- The 200-event SSE ring limit remains intentional. A brand-new page can reconstruct only retained
backlog; an in-memory same-session reconnect is exact-once from its cursor.
- Per-session sequence counters remain in backend memory after `clear` by design so later cold
same-id Resume cannot reuse ids. This is one numeric map entry per session id for the process
lifetime.
- Per-session sequence counters remain in backend memory after `clear` by design so later in-process
cold same-id Resume cannot reuse ids. Permanent DELETE removes the counter via `forget`.
- Frontend tests still print pre-existing MSW unhandled-request and React ref/`act` warnings even
though all 271 tests pass. The frontend production build still reports pre-existing large chunk
though all 276 tests pass. The frontend production build still reports pre-existing large chunk
warnings. Neither warning class was introduced or expanded by this change.
- No live Pi/DWH smoke was run; this wave changes only REST/SSE/frontend lifecycle boundaries and
is covered by fake-Pi, live Fastify SSE, component, full-suite, typecheck, and production-build
gates.
## Independent-review hardening
The required independent review was run repeatedly against the uncommitted diff. Its first pass
found four Important lifecycle edges beyond the integrated findings: queued old-runtime callbacks,
post-spawn construction cleanup, concurrent frontend Resume completions, and the passive-effect
commit window. Its second pass confirmed those fixes and identified one remaining Important
retention issue in the new runtime-identity map. The final pass reported no Critical, Important, or
Minor findings and assessed the diff ready to merge.
The resulting hardening is:
- Runtime bridge callbacks are gated by the bound runtime identity. Replacement, close, and DELETE
invalidate the old identity, so queued old events cannot publish or call `failSession`. An active
runtime removed by the manager can still publish its complete public failure sequence; after the
terminal unmanaged `agent_end`, its binding is released and later events are rejected.
- `PiProcessManager` kills the spawned child and removes any registered map entry if either
spawn-boundary stderr setup or later RPC/bridge/map initialization throws.
- Resume completion compares against synchronously maintained current active-session identity.
Concurrent `alreadyActive: false` then `alreadyActive: true` results preserve the cold source,
cursor, store, and replayed gate.
- Stream source replacement uses a layout effect. A deterministic later-layout-effect test delivers
a queued old event inside the former commit-to-passive-cleanup window and proves it is ignored.
- Cursor tests cover both a restarted backend's fresh low ids and an in-process hub's preserved high
ids followed by a cursor-bearing ordinary reconnect.
### Hardening TDD RED/GREEN evidence
Backend identity/construction RED command:
```text
cd backend && npx vitest run test/pi-process-manager.test.ts test/routes-sessions.test.ts
```
```text
Test Files 2 failed (2)
Tests 3 failed | 69 passed (72)
post-spawn reader initialization did not kill the child
replaced and deleted runtime callbacks still called failSession/published
```
Additional spawn-boundary and terminal-release RED checks:
```text
cd backend && npx vitest run test/pi-process-manager.test.ts \
-t "spawn boundary initialization"
Tests 1 failed | 38 skipped (39)
cd backend && npx vitest run test/routes-sessions.test.ts -t "terminal sequence"
Tests 1 failed | 34 skipped (35)
```
Frontend concurrency/layout RED command:
```text
cd frontend && npx vitest run src/stream/useSessionStream.test.tsx \
src/shell/AppShell.session-mgmt.test.tsx
```
```text
Test Files 2 failed (2)
Tests 2 failed | 25 passed (27)
the later-layout-effect event wrote "commit-window stale text"
the false→true completion pair erased pending gate "cold-gate"
```
Final focused GREEN commands:
```text
cd backend && npx vitest run test/pi-process-manager.test.ts \
test/routes-sessions.test.ts test/sse-hub.test.ts
cd backend && npx tsc --noEmit -p .
Test Files 3 passed (3)
Tests 79 passed (79)
TypeScript: no output, exit 0
cd frontend && npx vitest run src/stream/useSessionStream.test.tsx \
src/shell/AppShell.session-mgmt.test.tsx
cd frontend && npx tsc -b
Test Files 2 passed (2)
Tests 27 passed (27)
TypeScript: no output, exit 0
```
### Final full verification after review hardening
```text
cd backend && npx vitest run
Test Files 22 passed (22)
Tests 188 passed (188)
cd frontend && npx vitest run
Test Files 43 passed (43)
Tests 276 passed (276)
cd backend && npm run build
> tsc -p tsconfig.json
exit 0
cd frontend && npm run build
> tsc -b && vite build
✓ 4835 modules transformed.
✓ built in 8.47s
exit 0
```
The final frontend run retains the repository's pre-existing MSW/ref/`act` warnings, and the build
retains the pre-existing large-chunk warning. No test, typecheck, or build failures remain.