feat(game): persist PvP guest identity across reconnects

[protostatis]

Summary

Persists randomly generated PvP guest identities across page reloads using opaque session tokens. Implements Phase 1 of the advisor-approved plan — identity survives page reloads and reconnects; credit balances survive within the same server process lifetime.

What changed

Server (server/index.js, server/SessionStore.js):

• New SessionStore: in-memory 256-bit bearer token store (bounded to 10K, FIFO eviction)
• identifyGuest accepts optional sessionToken to restore identity + credit balance across reconnects
• Fixed: identification no longer calls ensurePlayable (def erred to V2WagerRoom pre-match)
• Fixed: repeated IDENTIFY_GUEST on an identified socket short-circuits — prevents token-to-player corruption

Client (v2PvpTransport.js, storage.js):

• New storage.js: localStorage wrapper with field-level validation for bc2_pvp_session key
• Transport reads persisted token before IDENTIFY_GUEST handshake, saves IDENTITY_OK response

UI (V2PvpLobby.jsx, App.jsx, App.css):

• Updated disclaimer: identity persists across reloads, balance resets on restart
• New Identity button (lobby, guarded by window.confirm, disabled during active challenges)

Tests (108 total):

• 28 storage tests: round-trips, validation (rejects null/empty/non-string), corruption, localStorage-unavailable
• 12 SessionStore tests: token format (43-char base64url, 32 bytes), getOrCreate semantics, eviction
• Existing: 48 client + 60 server — all pass

Also includes prior game fixes (rematch revision tracking, mobile Web Audio API unlock).

Behavior

Scenario	Before	After
Page refresh	New identity, 100cr	Same identity, same balance
Close tab, reopen	New identity, 100cr	Same identity, same balance
Server restart	New identity, 100cr	New identity, 100cr (session map lost)

Next steps (Phase 2)

Durable credit balances across server restarts via SQLite backing + Docker persistent volume — deferred until product requirement.

Closes: Phase 1 of identity persistence

[protostatis]

Sky's Code Review

Well-structured Phase 1 implementation of PvP guest identity persistence. Uses 256-bit base64url session tokens stored client-side in localStorage and server-side in a bounded in-memory Map. Includes thorough test coverage (storage round-trips/validation/corruption, SessionStore token format/eviction/restoration). The sound.js rewrite for mobile Web Audio API unlock is a clean bonus. No security or production-breaking issues found.

Verdict: Approve

Comments

• SessionStore FIFO eviction via Map insertion order is correct and well-tested. The 10K bound prevents unbounded memory growth from stale/abandoned sessions.
• The identifyGuest guard for already-identified sockets (meta.player && meta.sessionToken) correctly prevents token-to-player corruption from repeated IDENTIFY_GUEST messages — good catch.
• The REMATCH_STARTED revision reset in both transport and controller is necessary and correctly prevents stale revision checks from rejecting the new room's first snapshot.
• The sound.js rewrite handles the mobile Safari audio unlock pattern correctly (touchend/click/keydown, silent buffer source, retry on failed resume). The armUnlockListeners guard against typeof document === 'undefined' covers SSR edge cases.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

dashboard-frontend/game/src/utils/storage.js:42 — Minor: loadSession returns the full parsed JSON object, which could include properties beyond {sessionToken, userId, name} if localStorage was externally modified. Not a security issue since only the token is trusted server-side, but adding a whitelist (return { sessionToken: parsed.sessionToken, userId: parsed.userId, name: parsed.name }) would make the return type strictly match the PvpSession typedef.

dashboard-frontend/game/server/index.js:295 — The typeof msg.sessionToken === 'string' && msg.sessionToken.length <= 128 guard is solid. Consider also rejecting tokens that don't match a base64url pattern (e.g. /^[A-Za-z0-9_-]{1,128}$/) to silently discard garbage before hitting the Map lookup — purely a defense-in-depth nit.

[protostatis]

Sky's Code Review

Well-structured Phase 1 implementation of PvP guest identity persistence using opaque session tokens backed by an in-memory store and localStorage. The design is sound — 256-bit base64url tokens, FIFO-evicted bounded store, client-side validation, and a clean 'New Identity' escape hatch. Test coverage is strong (108+ tests). Includes sensible bonus fixes for rematch revision tracking and mobile Web Audio unlock. No security issues that would block production; a few minor observations below.

Verdict: Approve

Comments

• The PR is well-organized with a clear scope (Phase 1 in-memory only) and explicit deferral notes for Phase 2 (SQLite persistence). The behavior table in the description makes the three scenarios immediately understandable.
• The sound.js rewrite from HTMLAudioElement to Web Audio API is a correct fix for iOS Safari's autoplay restrictions. The silent buffer unlock + retry-on-later-gesture pattern is the standard approach.
• The rematch revision tracking fix (resetting authoritativeRevisionRef and expectedRevision on REMATCH_STARTED) addresses a real bug where the client would reject snapshots from a new room whose revision starts below the old room's final revision. Well caught.
• Client-side storage validation in storage.js is thorough — rejects null, empty strings, non-strings, missing fields, and handles localStorage unavailability gracefully. The 28 dedicated tests give good confidence.
• No secrets, no unquoted shell variables, no Docker concerns — this is a pure application-layer change. The session token is generated server-side with crypto.randomBytes which is cryptographically appropriate.

---

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

dashboard-frontend/game/server/SessionStore.js:48 — When a session is evicted (FIFO), the corresponding DemoCreditLedger entry keyed by that userId is never cleaned up. Over time with many unique guests this is a slow memory leak in the ledger. Acceptable for Phase 1 given the 10K cap, but worth a TODO for Phase 2.

dashboard-frontend/game/server/index.js:503 — The sessionToken is already bounded to 128 chars and type-checked here, which is fine. Consider also validating it matches the base64url character set (A-Za-z0-9_-) before passing to sessions.getOrCreate(), so arbitrary strings can't be used to probe or pollute the session map. Low risk since getOrCreate handles unknown tokens by creating a new session, but a regex gate would be defense-in-depth.

dashboard-frontend/game/src/transports/v2PvpTransport.js:118 — saveSession is called on every IDENTITY_OK, which is correct. Note that if a user clicks 'New Identity' while a reconnect is in-flight, the stale reconnect's IDENTITY_OK could re-persist the old token before the new connection overwrites it. The current code handles this because handleNewIdentity clears storage before triggering a new connect, but it's worth documenting the race in a comment if it surfaces in testing.

dashboard-frontend/game/server/index.js:285 — Good fix: the short-circuit for already-identified sockets prevents the token-to-player corruption bug described in the PR. The comment clearly explains why.