Force-take-on-valid-token: resume-race during pre-detection window (mobile-dominant) #149

New issue

Closed

opened 2026-06-24 01:25:40 +02:00 by bosun · 0 comments

bosun commented

2026-06-24 01:25:40 +02:00

Owner

Background

2026-06-24 Engineer probe (bus dispatch 9d85) for #139 detection-coverage surfaced a substantive finding that reframes the "page-reload doesn't fire opponentDisconnect" observation from operator playtest:

The race isn't a detection-coverage gap — it's a valid-resume-during-pre-detection-window-wrongly-REJECTED bug.

Root cause (substrate-grounded probe at-source)

Race condition: client always resumes-on-open if a token exists (net.ts:144). Server Resume() rejects "seat is still active" when the old seat isn't yet marked disconnected (reconnect.go:205). On an INITIAL connect (page RELOAD, reconnecting=false), the client handles rejection by removeItem(LS_TOKEN) + sendJoin() (net.ts:269-270) → reloader dumped into fresh matchmaking, match seat LOST.

Mobile-specific dominance: the race is won by whichever lands first — old-socket close-detection vs. new-connect+resume.

Desktop reload: browser sends FIN promptly → server detects close (marks disconnected) before new WS finishes handshake → resume SUCCEEDS, seamless. Race is rare on desktop.
Mobile suspend/reload: iOS freezes the tab → old FIN is NEVER sent → seat stays "active" until 25s keepalive fires. Return + reload inside that window → resume arrives while seat still "active" → REJECTED → dumped to fresh matchmaking. Race is COMMON on mobile.

This matches operator's playtest finding "mobile reload" symptom + #145 (browser-restart resume UX) observation: the resume token persists (per #123 localStorage swap), but the server-side rejects-during-pre-detection-window means the resume doesn't take seat.

Critical insight — re #139 detection-coverage framing: detection isn't failing to FIRE (it does, eventually, via keepalive). And operator's "survivor doesn't see overlay on reload" is most likely the CORRECT desktop fast-recovery path (close-detected-first → brief opponentDisconnect→opponentReconnect flash → no lasting overlay needed) — NOT a detection failure. The "reliability gap" Herald flagged in c53f for #139 is actually a different substrate-axis: the gap is engine-side resume-handling, not detection-paths.

Severity: MED

Substantive substrate-gap. Rare on desktop (race window is microseconds). Common on mobile (race window is up to 25s = full keepalive interval). The browser-restart resume UX (#145) and #139's overall reliability story both depend on this fix.

Fix-direction: force-take-on-valid-token

Make Resume() with a valid token DISPLACE a still-"active" seat instead of rejecting. The unguessable 128-bit token proves ownership; a resume inherently means "replacing the old conn" (last-resume-wins semantics).

Cross-path care (the load-bearing subtlety): the displaced old conn's readPump later fires Leave → handleMatchDisconnect(m, oldP), which would wrongly mark the now-active seat disconnected (uses oldP.idx unconditionally). Fix must neutralize that:

On force-take: set oldP.match=nil so the old Leave handler no-ops (it gates on p.match!=nil)
Tear down the old socket explicitly

Without (1)+(2), the displaced conn's later Leave would cause server to think the now-active player is disconnected, triggering grace-pause + breaking the seat-take semantics.

Acceptance criteria

Resume() with valid token displaces still-"active" seat instead of rejecting
Old seat's readPump Leave is neutralized via oldP.match=nil + socket teardown
Race test added: simulate fast page-reload-during-pre-detection-window; assert force-take semantics (new seat active, old conn cleanly torn down, no spurious opponentDisconnect on now-active player)
Mobile suspend/reload scenario: page-reload within 25s keepalive window successfully takes seat (vs current reject + fresh matchmaking)
Existing happy-path/grace/unknown-token tests still green (reconnect_test.go)

Why a dedicated tracker (not folded into #139)

Substrate-axis separation: this is server reliability fix (Engineer lane); #139 is client-overlay UX rendering (Shipwright lane)
Independence: server fix lands independently of overlay work; the overlay still serves its purpose (rendering existing grace-pause + reconnect states)
Substrate-of-record clarity: the fix-mechanism is substantively distinct from "show reconnecting overlay"

Cross-refs

cellblock#139 (reconnecting-overlay UX; Herald spec c53f flagged detection-reliability prerequisite — this tracker addresses the actual mechanism)
cellblock#145 (browser-restart resume UX; this fix is the engine-prerequisite for auto-resume-on-app-load)
cellblock#115 (reconnect-via-resume substrate; same resume-handshake path)
cellblock#122 + #126 (keepalive detection bounds; pre-detection-window duration is 25s read-deadline)
cellblock#123 (localStorage swap; resume token persists but reject-during-pre-detection-window means resume doesn't take effect)
cellblock PR #148 (parallel Engineer work; probe surfaced this finding)

Anchor

2026-06-24 Engineer page-reload-race source-probe per Bosun routing 9d85 → reframed #139 detection-coverage finding as resume-handling-race. Substrate-grounded probe distinguishes "detection fails to fire" from "valid resume incorrectly rejected during pre-detection window" — different substrate-axes, different fix-direction, different lane.

## Background 2026-06-24 Engineer probe (bus dispatch 9d85) for #139 detection-coverage surfaced a substantive finding that reframes the "page-reload doesn't fire opponentDisconnect" observation from operator playtest: **The race isn't a detection-coverage gap — it's a valid-resume-during-pre-detection-window-wrongly-REJECTED bug.** ## Root cause (substrate-grounded probe at-source) **Race condition**: client always resumes-on-open if a token exists (net.ts:144). Server `Resume()` rejects "seat is still active" when the old seat isn't yet marked disconnected (reconnect.go:205). On an INITIAL connect (page RELOAD, `reconnecting=false`), the client handles rejection by `removeItem(LS_TOKEN) + sendJoin()` (net.ts:269-270) → **reloader dumped into fresh matchmaking, match seat LOST.** **Mobile-specific dominance**: the race is won by whichever lands first — old-socket close-detection vs. new-connect+resume. - **Desktop reload**: browser sends FIN promptly → server detects close (marks disconnected) before new WS finishes handshake → resume SUCCEEDS, seamless. Race is rare on desktop. - **Mobile suspend/reload**: iOS freezes the tab → old FIN is NEVER sent → seat stays "active" until 25s keepalive fires. Return + reload inside that window → resume arrives while seat still "active" → REJECTED → dumped to fresh matchmaking. Race is COMMON on mobile. This matches operator's playtest finding "mobile reload" symptom + #145 (browser-restart resume UX) observation: the resume token persists (per #123 localStorage swap), but the server-side rejects-during-pre-detection-window means the resume doesn't take seat. **Critical insight — re #139 detection-coverage framing**: detection isn't failing to FIRE (it does, eventually, via keepalive). And operator's "survivor doesn't see overlay on reload" is most likely the CORRECT desktop fast-recovery path (close-detected-first → brief opponentDisconnect→opponentReconnect flash → no lasting overlay needed) — NOT a detection failure. The "reliability gap" Herald flagged in c53f for #139 is actually a different substrate-axis: the gap is engine-side resume-handling, not detection-paths. ## Severity: MED Substantive substrate-gap. Rare on desktop (race window is microseconds). Common on mobile (race window is up to 25s = full keepalive interval). The browser-restart resume UX (#145) and #139's overall reliability story both depend on this fix. ## Fix-direction: force-take-on-valid-token Make `Resume()` with a valid token DISPLACE a still-"active" seat instead of rejecting. The unguessable 128-bit token proves ownership; a resume inherently means "replacing the old conn" (last-resume-wins semantics). **Cross-path care (the load-bearing subtlety)**: the displaced old conn's readPump later fires `Leave → handleMatchDisconnect(m, oldP)`, which would wrongly mark the now-active seat disconnected (uses `oldP.idx` unconditionally). Fix must neutralize that: 1. On force-take: set `oldP.match=nil` so the old Leave handler no-ops (it gates on `p.match!=nil`) 2. Tear down the old socket explicitly Without (1)+(2), the displaced conn's later Leave would cause server to think the now-active player is disconnected, triggering grace-pause + breaking the seat-take semantics. ## Acceptance criteria 1. `Resume()` with valid token displaces still-"active" seat instead of rejecting 2. Old seat's readPump Leave is neutralized via `oldP.match=nil` + socket teardown 3. Race test added: simulate fast page-reload-during-pre-detection-window; assert force-take semantics (new seat active, old conn cleanly torn down, no spurious opponentDisconnect on now-active player) 4. Mobile suspend/reload scenario: page-reload within 25s keepalive window successfully takes seat (vs current reject + fresh matchmaking) 5. Existing happy-path/grace/unknown-token tests still green (reconnect_test.go) ## Why a dedicated tracker (not folded into #139) - **Substrate-axis separation**: this is server reliability fix (Engineer lane); #139 is client-overlay UX rendering (Shipwright lane) - **Independence**: server fix lands independently of overlay work; the overlay still serves its purpose (rendering existing grace-pause + reconnect states) - **Substrate-of-record clarity**: the fix-mechanism is substantively distinct from "show reconnecting overlay" ## Cross-refs - cellblock#139 (reconnecting-overlay UX; Herald spec c53f flagged detection-reliability prerequisite — this tracker addresses the actual mechanism) - cellblock#145 (browser-restart resume UX; this fix is the engine-prerequisite for auto-resume-on-app-load) - cellblock#115 (reconnect-via-resume substrate; same resume-handshake path) - cellblock#122 + #126 (keepalive detection bounds; pre-detection-window duration is 25s read-deadline) - cellblock#123 (localStorage swap; resume token persists but reject-during-pre-detection-window means resume doesn't take effect) - cellblock PR #148 (parallel Engineer work; probe surfaced this finding) ## Anchor 2026-06-24 Engineer page-reload-race source-probe per Bosun routing 9d85 → reframed #139 detection-coverage finding as resume-handling-race. Substrate-grounded probe distinguishes "detection fails to fire" from "valid resume incorrectly rejected during pre-detection window" — different substrate-axes, different fix-direction, different lane.

bosun referenced this issue from a commit

2026-06-24 02:15:56 +02:00

feat(client): self RECONNECTING overlay during grace window (#139 PART 1)

bosun referenced this issue from a commit

2026-06-24 02:38:37 +02:00

feat(client): survivor OPPONENT DISCONNECTED overlay (#139 PART 2)