[#241] Make room entry idempotent and stop polling after closure

[realproject7]

Fixes #241

Implements the reviewed #241 browser room-entry lifecycle contract (Batch 41, code batch), including the Batch-38 joining-interstitial amendment and the RE1 slow-poll serialization finding. Baseline fresh origin/main@99a0d22; head f962b3b.

EPIC Alignment

• Epic: [Audit][P1] Full-codebase quality, privacy, and release-hygiene sweep #238 — full-codebase quality/privacy/release-hygiene sweep. This ticket's role: make browser room entry + terminal polling lifecycle idempotent without altering auth or UI scope.
• Merged siblings [P1][Storage] Make durable state writes atomic and stale-lock reclamation ownership-safe #239/[P2][CLI] Handle broker, platform, and room bind failures as command errors #240 (storage, CLI listen) don't overlap; [P2][Browser] Make room entry idempotent and stop polling after closure #241 owns only src/browser/room.js. [P2][Platform] Distinguish missing remote host logs from local storage failures #242/[P2][Release] Make npm package assembly reproducible and correct stale release documentation #243 untouched (diff is room.js + one new test).
• Contract respected: exactly one event-binding set, message timer, status timer, join-form binding, claimDisplayName, and eventual entry — across both post-entry and joining-interstitial re-entry (the amendment); timers cleared on terminal closure; the final closed-history load is issued exactly once even under a slow response; normal token-fragment behavior + read-only closed-history load intact; no SSE (Follow-up: Add SSE /events active receive path for chat channels #139); no token across DOM/output/metadata/URL.

What changed (src/browser/room.js)

• Single lifecycle guard state.entryPhase (unstarted → starting → joining → entering → entered):
  • startWithToken no-ops when entryPhase !== "unstarted" — one guard covers both re-entry paths: a later fragment after entry, and a repeat fragment while the joining interstitial is shown (which previously re-bound the join form). A pre-commit failure (e.g. a bad token's /profile) resets to unstarted so a later valid token can still enter.
  • enterRoom runs its bindings + poll/status timers exactly once (guarded on entered).
  • submitProfile proceeds only from "joining" and holds "entering" for the claim+enter, so one claimDisplayName + one entry survive a double submit; a submit error reopens the interstitial for retry.
• Timer retention + terminal cleanup: the 3s message and 5s status intervals are retained as state.pollTimer / state.statusTimer; a single finalizeClosedRoom() (called where the closed room's one read-only history load completes) sets closedHistoryLoaded, clears both timers, and applies state. enterRoom skips starting timers if that first load already finalized a closed room. No timer remains active after closure.
• pollMessages single-flight serialization (RE1 finding): state.pollInFlight guards the fetch — a timer tick that fires while a poll is already awaiting its fetch skips instead of issuing a second concurrent fetch; the flag is reset in a finally on every exit path (inner-catch return, normal completion, unexpected throw). So a fetch slower than the 3s interval cannot let overlapping ticks issue several concurrent final closed-history fetches — the closed-history load is issued exactly once. A skipped tick simply polls on the next interval with the current cursor (no message loss).

Self-Verification

• Acceptance criteria 1:1 (test/browser-room-lifecycle.test.ts, Playwright over a real room server; each verified to fail on the pre-fix room.js and pass after — non-vacuous):
  • A later token-fragment transition enters once → a later token-fragment re-entry after entering…: after a second fragment, GET /brief (only issued by enterRoom) is 1 and one send posts once.
  • Joining-interstitial re-entry stays idempotent (one join-form binding / claim / entry) → a token-fragment re-entry while the joining interstitial is shown…: POST /profile (claim) is 1 and entry occurs once.
  • A closed room loads final history once and leaves no active polling timer → a closed room loads its final history once…: after closure+finalize, a further window sees zero new /messages and /status requests.
  • Exactly-once final closed-history fetch under a slow response (RE1) → overlapping poll intervals issue a single final closed-history fetch (serialized): page.route holds every GET /messages (never fulfills) so a poll's fetch never completes; the room is closed; across several 3s ticks the held count is exactly 1 (fails without the guard at 2).
  • No token in DOM/output/local metadata/URL → guards are state-only; tokenFromFragment still clears the fragment; no new token surface.
• Kill-list scan: no token exposure; no new runtime dependencies; auth/token storage (sessionStorage token, claimDisplayName) unchanged; no UI redesign; no SSE; read-only closed-history load preserved; #242/#243 files untouched.
• Evidence: pnpm typecheck ✓ · pnpm lint ✓ · pnpm kit-guard ✓ (4 panes clean) · pnpm no-stub ✓ · pnpm build ✓ · node --test dist/test/**/*.js → 443 pass / 0 fail. CI Release gates pass on f962b3b (run 29392912971).

Design Fidelity

This is a behavioral change (entry idempotency + timer/poll lifecycle) with no visual or markup change — no tokens, colors, spacing, layout, or DOM structure touched.

Surface / element	Spec	Implemented	Visual delta
Auth-error page	Unchanged	data-state="auth-error", hashchange re-entry armed — markup/style unchanged	None
Joining interstitial (#join-panel, #join-form, #display-name, #join-button)	Unchanged	Shown once via the entry guard; no markup/style change	None
Composer (#message-text, #send-button)	Unchanged	Bound once via single enterRoom; no markup/style change	None
Roster / timeline / room-status	Unchanged	Rendered by the same applyRoomState/render paths; closed state renders as before	None
Poll/status cadence	Unchanged (3s / 5s)	Same intervals, now retained + serialized + cleared on closure	None

Deviations

• The idempotency uses one entryPhase state machine rather than separate booleans — the ticket's "smallest guard that covers both paths." A guard at startWithToken covers both re-entry paths; enterRoom/submitProfile guards are the direct, minimal protection for the timers/claim.
• Serialization is a single-flight skip (pollInFlight), not a queue: for a poller, skipping a redundant concurrent fetch is correct (the in-flight poll already fetches from the current cursor); this guarantees the exactly-once final closed-history load without changing the 3s cadence.
• Tests assert closure via a freeze window (zero new polls after finalize) and a held-request count (serialization), rather than counting intervals — robust against CI timing while remaining deterministic (cleared timers never fire; a single in-flight poll is issued regardless of response speed).

[project7-interns]

Verdict: REQUEST CHANGES

Epic Alignment: FAIL

The closed-room lifecycle still permits multiple final history loads under overlapping poll callbacks, so it does not yet meet #241’s exactly-once terminal-load contract.

Checked (evidence)

• Structural gate: live REST PR body contains filled EPIC Alignment, Self-Verification, Design Fidelity, and Deviations sections.
• Re-entry: src/browser/room.js:181-237 uses a single entryPhase boundary for post-entry and joining-interstitial paths; submitProfile guards duplicate claims at :267-270.
• Timer retention: src/browser/room.js:240-258 retains and clears both interval handles.
• Riskiest part: terminal polling; it is not acceptable as written; see finding.
• Kill-list: scanned all items — hit listed below.
• CI: gh pr checks 246 → Release gates pending (live).

Findings

• [required] Overlapping polls can fetch closed-room history more than once.
  • File: src/browser/room.js:503-538
  • Why it fails: setInterval invokes pollMessages every 3 seconds (:234) without an in-flight guard. If a /messages request lasts beyond an interval, two calls can both pass :507 while closedHistoryLoaded is false, both issue the read-only history fetch at :510, and only later each calls finalizeClosedRoom. Clearing timers then cannot undo the duplicate final loads. The current freeze-window test does not delay /messages and cannot exercise this race.
  • Do instead: serialize pollMessages (for example, a state-held in-flight promise/flag released in finally) so a timer tick cannot overlap an active fetch, and add a regression with a messages response delayed past 3 seconds that asserts exactly one final-history request after closure.

Decision

REQUEST CHANGES. Entry guards and timer cleanup are sound, but the terminal-history exactly-once acceptance criterion remains unproven and can fail under normal slow-network timing.

[realproject7]

RE2 — APPROVE (code/tests) — authoritative verdict in project chat. One CI caveat.

#241's contract + the Batch-38 joining-interstitial amendment are fully met in src/browser/room.js:

• Single state.entryPhase guard at startWithToken no-ops re-entry — covers both post-entry re-entry and joining-interstitial re-entry with one guard (exactly the enter-lifecycle-entry-point fix required at contract review); bad-token /profile resets the phase so a later valid token still enters.
• enterRoom idempotent → one bindings set + one poll timer + one status timer; submitProfile gated on "joining"/"entering" → one claimDisplayName + one entry on a double submit.
• Timers retained (pollTimer/statusTimer) and cleared by finalizeClosedRoom()→stopPolling() on the closed room's single history load; enterRoom skips timers if already closed on entry — no timer active after closure.
• Read-only closed-history load, auth/token storage, UI scope, token-free boundaries preserved; no SSE/deps; only room.js touched.
• Tests (browser-room-lifecycle.test.ts, Playwright + live server): post-entry re-entry (GET /brief==1), joining-interstitial re-entry (POST /profile==1), closed-room no-further-polls — non-vacuous, all passed in CI.

CI caveat (merge gate, not the code): Release gates failed 1/442 — sole failure is device-local archive/delete for joined rooms (#210), a platform-dashboard browser test in a different module from room.js, and the same test that flaked on #244/#245. A room-entry-lifecycle change cannot affect it. Please re-trigger CI — a re-run, not a code fix.

[realproject7]

RE2 — concurring with @re1's REQUEST CHANGES; my earlier APPROVE is now qualified. (Authoritative verdict in project chat.)

@re1's slow-poll final-history race is real and I missed it: pollMessages runs on setInterval(3000) with no in-flight guard, and the top guard short-circuits only after closedHistoryLoaded is set inside finalizeClosedRoom (post-await). If a /messages fetch outlasts 3s, two polls overlap at closure — both pass the guard, both fetch the read-only history — so the final history can load more than once, violating the "loads its final read-only history once" acceptance criterion. stopPolling() clears timers but can't undo an already-in-flight duplicate fetch, and the closed-room test doesn't delay /messages so it can't catch this.

Entry-idempotency (entryPhase, both re-entry paths) and timer cleanup (no active timer after closure) remain sound. Required fix (endorsing @re1): serialize pollMessages with a state-held in-flight flag released in finally, plus a regression that delays /messages past 3s and asserts exactly one final-history request after closure. Will re-review the delta.

[project7-interns]

Verdict: APPROVE

Epic Alignment: PASS

The implementation remains within #241’s room-entry/timer lifecycle scope and closes the exactly-once terminal-history gap without touching auth, UI design, SSE, or #242/#243.

Checked (evidence)

• Re-review range: 754e7b8..f962b3b; only src/browser/room.js and test/browser-room-lifecycle.test.ts changed.
• Prior finding resolved: src/browser/room.js:508-550 uses state.pollInFlight with a finally reset, so interval ticks cannot overlap a pending messages fetch before terminal finalization.
• Regression: test/browser-room-lifecycle.test.ts holds GET /messages across multiple 3s ticks and asserts one final closed-history request.
• Riskiest part: final closed-history fetch; serialized polling now prevents duplicate in-flight reads while retained timer cleanup stops future ticks.
• Kill-list: scanned new ranges — clean.
• CI: gh pr checks 246 → Release gates pass (live, run 29392912971/job 87280060340).

Findings

• None.

Decision

APPROVE. My slow-poll exactly-once finding is resolved with deterministic coverage and green live CI.

[realproject7]

RE2 — APPROVE (re-review at f962b3b) — authoritative verdict in project chat. The slow-poll final-history race I concurred with is resolved; my qualified approval is now a full approval.

Serialization fix correct: pollMessages now takes a single-flight guard — after the closedHistoryLoaded top guard, if (state.pollInFlight) return; then set it true, with the whole body in try { … } finally { state.pollInFlight = false; }. A timer tick that fires during an in-flight fetch skips, so the closed-history fetch is issued exactly once. The finally runs on every exit path (inner-catch return / normal / throw) — no stuck-flag risk. A skipped tick just polls next interval with the current cursor — no message loss. Entry-idempotency + timer-cleanup untouched (additive delta).

Regression non-vacuous: overlapping poll intervals issue a single final closed-history fetch (serialized) holds every GET /messages (never fulfills), closes the room, waits several 3s ticks, and asserts exactly one fetch issued — fails without the guard (≥2), passes with it (1). With the existing "loads final history once + stops timers" test, exactly-once + no-active-timer is fully covered.

Release gates CI green on f962b3b (run 29392912971); delta strictly room.js + its test.