From 23dd6a75e8ee2cbcf8835910df5ec0487a20b1f7 Mon Sep 17 00:00:00 2001 From: null Date: Thu, 25 Jun 2026 21:43:06 -0500 Subject: [PATCH] fix(games): atomic session start to prevent duplicate sessions on concurrent start (F-RACE-001) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simultaneous game start by both partners created two divergent active sessions (TOCTOU: a non-transactional check-then-create in GameSessionManager.startGameWithCouple). Each partner ended up in a separate session with different questions → no shared reveal. Fix: QuestionSessionRepository.startSessionAtomically runs a Firestore transaction on a per-couple pointer doc (couples/{cid}/sessions/_active). It reads the pointer (+ the pointed session) and either returns AlreadyActive (caller joins the existing session) or atomically creates the new session and re-points the lock. Concurrent starts contend on the one pointer, so the loser's transaction retries, sees the now-set pointer, and joins instead of duplicating. The pointer self-heals (checks the pointed session's status) so no clear-on-finish is needed, and it carries no status/completedAt so it's invisible to the active/history queries. GameSessionManager routes all 7 games through it. firestore.rules adds member-write for sessions/_active (deployed). Verified live on both emulators: atomic create → 1 session + pointer; sequential 2nd start → joins (1 session); literal parallel-tap race → 1 session (was 2); 0 FATAL. Co-Authored-By: Claude Opus 4.8 --- ClaudeQACoverage.md | 9 +++ ClaudeReport.md | 11 ++- .../QuestionSessionRepositoryImpl.kt | 76 +++++++++++++++++++ .../repository/QuestionSessionRepository.kt | 16 ++++ .../domain/usecase/GameSessionManager.kt | 37 +++++---- firestore.rules | 6 ++ 6 files changed, 140 insertions(+), 15 deletions(-) diff --git a/ClaudeQACoverage.md b/ClaudeQACoverage.md index d291b93e..80a3c844 100644 --- a/ClaudeQACoverage.md +++ b/ClaudeQACoverage.md @@ -105,6 +105,15 @@ conversation w/ content). **partner_started_game**: bg deliver + content-free present in code. Full 17×{fg/bg/killed} matrix not exhaustively run; routing centralized + code-verified for the rest. ## Pass F — Resilience / lifecycle / concurrency / time +**R7 DEEP DIVE:** **concurrency race — FOUND + FIXED F-RACE-001 (P1):** simultaneous game start created 2 divergent +sessions (TOCTOU in `startGameWithCouple`). **Fixed** via atomic transactional create on a per-couple `sessions/_active` +pointer (`startSessionAtomically`) + rule + deploy. **Verified live:** atomic create → 1 session + pointer; sequential +2nd start → joins (1); **parallel-tap race → 1 session (was 2)**; 0 FATAL. **Malformed/abusive deep-link intents** +(unknown type, missing extras, injection/path-traversal) → 0 crash. **Killed-state cold-start** chat deep-link → +conversation loads, 0 crash. Minor follow-up note: the race-loser sometimes lands on the Play hub rather than +WaitingForPartner→"Join the game" (no dup/crash; pre-existing routing). + + **R3:** offline (airplane mode) → Today renders from cache, no crash ✅; rotation/config-change → landscape renders, state preserved, no crash ✅; process-death/restore → ~6 cold restarts all clean to Home (auth persists) ✅; concurrency → both devices played games simultaneously, sessions synced + B-001 auto-complete on concurrent finish ✅. diff --git a/ClaudeReport.md b/ClaudeReport.md index 686e2839..67b2fed5 100644 --- a/ClaudeReport.md +++ b/ClaudeReport.md @@ -1,6 +1,6 @@ # Claude QA Report — Full-App QA (living report) -> **RUN-STATE: Round 7 (multi-angle DEEP DIVE) — 2026-06-25, client HEAD `f47c8e2`, functions deployed. Plan updated with a "Multi-angle attack mandate" + live raw-API D3.** Attacked security/data/concurrency from multiple angles (admin ground-truth read, raw Firestore REST as member+non-member, killed/cold state, malformed intents, simultaneous-start race). **Security cornerstone = FULLY CLEAN (deep):** D1 at-rest — messages/previews + all 4 game-answer collections (ToT/HowWell/DesireSync/Wheel, both users) + capsules + date-swipe actions all `enc:v1:`; couple key phrase-wrapped (argon2id), recovery phrase server-blind + `encryptedRecoveryPhrase` wiped on acceptance, plaintext `inviteCode` not exploitable (invite readable only by inviter; no code-encrypted secret persisted). D3 raw-API — **non-member denied ALL reads/writes (403)**; real premium path `users/{uid}/entitlements/premium` write **denied (403, server-only) → no self-grant**; cross-couple denied. Robustness — malformed/abusive deep-link intents (unknown type, missing extras, injection/path-traversal values) → 0 crash; killed-state cold-start chat deep-link → conversation loads. **NEW FINDING: F-RACE-001 (P1)** — simultaneous game start creates **two divergent active sessions** (TOCTOU). **SEVERITY BOARD: P1 = 1 open (F-RACE-001), P0/P2/P3 = 0 open.** Baseline restored (duplicate sessions ended, 0 active, couple intact). Two hardening notes → Future.md (App Check not enforced on Firestore; user-doc update rule allows arbitrary non-`hasPremium` fields).** +> **RUN-STATE: Round 7 (multi-angle DEEP DIVE) — 2026-06-25, client HEAD `f47c8e2`, functions deployed. Plan updated with a "Multi-angle attack mandate" + live raw-API D3.** Attacked security/data/concurrency from multiple angles (admin ground-truth read, raw Firestore REST as member+non-member, killed/cold state, malformed intents, simultaneous-start race). **Security cornerstone = FULLY CLEAN (deep):** D1 at-rest — messages/previews + all 4 game-answer collections (ToT/HowWell/DesireSync/Wheel, both users) + capsules + date-swipe actions all `enc:v1:`; couple key phrase-wrapped (argon2id), recovery phrase server-blind + `encryptedRecoveryPhrase` wiped on acceptance, plaintext `inviteCode` not exploitable (invite readable only by inviter; no code-encrypted secret persisted). D3 raw-API — **non-member denied ALL reads/writes (403)**; real premium path `users/{uid}/entitlements/premium` write **denied (403, server-only) → no self-grant**; cross-couple denied. Robustness — malformed/abusive deep-link intents (unknown type, missing extras, injection/path-traversal values) → 0 crash; killed-state cold-start chat deep-link → conversation loads. **F-RACE-001 (P1) — FIXED + VERIFIED LIVE** (atomic transactional session create). **SEVERITY BOARD: 0 open at ALL levels (P0–P3).** Baseline restored (duplicate sessions ended, 0 active, couple intact). Two hardening notes → Future.md (App Check not enforced on Firestore; user-doc update rule allows arbitrary non-`hasPremium` fields).** > > **F-RACE-001 (P1, NEW — concurrency):** When BOTH partners start the *same* game within ~the same second, the couple > ends up with **2 active sessions with different question sets** (proven live: QA "Which should end the date?" vs Sam @@ -16,6 +16,15 @@ > else create the session doc (client-generated id) + set the sentinel in the same transaction. Clear the sentinel on > finish. Needs a `firestore.rules` update (member-only sentinel write) + a rules deploy + re-verify all 7 games. (This > is an architectural change to the core game flow — flagged for a focused fix-phase implementation.) +> **FIX IMPLEMENTED + VERIFIED LIVE (2026-06-25):** `QuestionSessionRepository.startSessionAtomically` runs a Firestore +> transaction on a per-couple pointer doc `couples/{cid}/sessions/_active` — reads the pointer (+ the pointed session), +> and either returns `AlreadyActive` (caller joins) or atomically `set`s the new session + re-points the lock. Concurrent +> starts contend on the one pointer doc, so the loser's transaction retries, re-reads the now-set pointer, and joins +> instead of creating a duplicate. `GameSessionManager.startGameWithCouple` now calls it (all 7 games funnel through it); +> rule added (`sessions/_active` member-writable) + deployed. **Verified:** atomic create → 1 session + pointer written; +> sequential 2nd start → joins (1 session); **literal parallel-tap race → 1 session (was 2)**; 0 FATAL; pointer +> self-heals on completion (checks pointed session status). Files: `domain/repository/QuestionSessionRepository.kt`, +> `data/repository/QuestionSessionRepositoryImpl.kt`, `domain/usecase/GameSessionManager.kt`, `firestore.rules`. > **RUN-STATE: Round 6 (branding + Future.md regression QA) COMPLETE — 2026-06-25. Client HEAD `f47c8e2` on both emulators (build == HEAD, reinstalled).** Scope: regression-verify the new surfaces from the branding drop (`95cad84`: white-keyhole launcher/notification icons, animated app-icon-chip loader + fill, cold-launch splash, pairing hero) and the Future.md backlog clear (`f47c8e2`: inclusive gender options, turn-aware Home copy, push rate-limit budget split, results-push suppression via new ActiveGameSessionMonitor, paywall retry/offline/hide-Continue, auth privacy rotator). **0 new issues — SEVERITY BOARD STILL 0 open P0–P3.** LIVE-VERIFIED: animated loader (chip+fill, both themes), splash→handoff (white-keyhole icon, no white flash), launcher icon (round mask), This or That + How Well open with no crash (confirms #4's new VM injection is sound), paywall purchase screen shows friendly "Couldn't load plans" + Try again with Continue hidden (no dead button) + online→generic message (#5), onboarding carousel illustration. Unit tests green (NotificationRateLimiter rewritten + PartnerNotificationManagerTest repaired). No FATAL on either device all session. CODE/UNIT-VERIFIED (live deferred, low-risk over proven patterns + fragile multi-text-field/2-device paths): #1 gender step (EditProfile + onboarding sex step — same option list as the shipping Female/Male), #8 rotator on SignUp/Forgot (reuses the Login-proven BrandMessageRotator), #2 "Your turn to play." (static string in the proven GAME_WAITING path), #3 weekly-cap exemption (unit-tested; only triggers at ≥100/wk), #4 results-suppression timing (mechanism + VM wiring verified; simultaneous-finish timing is non-deterministic to drive). Baseline restored: 5554 signed out during the sign-up pass, re-signed-in QA (`Y05AKO2IlTPMa0JQW1BiNIM0uzK2`) via admin custom token; couple `Xal3Kw3gjSdn0niERYKJ` intact, Sam paired.** diff --git a/app/src/main/java/app/closer/data/repository/QuestionSessionRepositoryImpl.kt b/app/src/main/java/app/closer/data/repository/QuestionSessionRepositoryImpl.kt index 18e0a4ec..b10ff5d3 100644 --- a/app/src/main/java/app/closer/data/repository/QuestionSessionRepositoryImpl.kt +++ b/app/src/main/java/app/closer/data/repository/QuestionSessionRepositoryImpl.kt @@ -5,6 +5,7 @@ import app.closer.data.remote.FirestoreCollections import app.closer.domain.model.GameType import app.closer.domain.model.QuestionSession import app.closer.domain.repository.QuestionSessionRepository +import app.closer.domain.repository.SessionStartOutcome import com.google.firebase.firestore.FirebaseFirestore import kotlinx.coroutines.channels.awaitClose import kotlinx.coroutines.flow.Flow @@ -53,6 +54,76 @@ class QuestionSessionRepositoryImpl @Inject constructor( doc.id } + override suspend fun startSessionAtomically( + session: QuestionSession + ): Result = runCatching { + val sessionsCol = firestore.collection(FirestoreCollections.COUPLES) + .document(session.coupleId) + .collection(FirestoreCollections.Couples.SESSIONS) + val pointerRef = sessionsCol.document(ACTIVE_POINTER_ID) + val newRef = sessionsCol.document() // pre-generate id for the create branch + + firestore.runTransaction { tx -> + // ── all reads must happen before any writes in a Firestore transaction ── + val pointer = tx.get(pointerRef) + val activeId = pointer.getString("activeSessionId")?.takeIf { it.isNotBlank() } + val activeSnap = activeId?.let { tx.get(sessionsCol.document(it)) } + + if (activeSnap != null && activeSnap.exists() && + activeSnap.getString("status") == "active" + ) { + // A session is already active → converge: the caller joins it (no duplicate created). + // Concurrent starts contend on [pointerRef]; the loser's transaction retries, re-reads + // the now-set pointer, and lands here. Stale/completed pointers fall through to create. + SessionStartOutcome.AlreadyActive(snapToSession(activeSnap, session.coupleId)) + } else { + val data = mapOf( + "id" to newRef.id, + "coupleId" to session.coupleId, + "categoryId" to session.categoryId, + "questionIds" to session.questionIds, + "startedByUserId" to session.startedByUserId, + "startedAt" to session.startedAt, + "completedAt" to session.completedAt, + "partnerCompletedAt" to session.partnerCompletedAt, + "isPremium" to session.isPremium, + "status" to session.status, + "gameType" to session.gameType, + "completedByUsers" to session.completedByUsers + ) + tx.set(newRef, data) + // Re-point the per-couple active-session lock to the new session. + tx.set( + pointerRef, + mapOf( + "activeSessionId" to newRef.id, + "updatedAt" to System.currentTimeMillis() + ) + ) + SessionStartOutcome.Created(session.copy(id = newRef.id)) + } + }.await() + } + + private fun snapToSession( + doc: com.google.firebase.firestore.DocumentSnapshot, + coupleId: String + ): QuestionSession = QuestionSession( + id = doc.getString("id") ?: doc.id, + coupleId = doc.getString("coupleId") ?: coupleId, + categoryId = doc.getString("categoryId") ?: "", + questionIds = (doc.get("questionIds") as? List<*>)?.filterIsInstance() ?: emptyList(), + startedByUserId = doc.getString("startedByUserId") ?: "", + startedAt = doc.getLong("startedAt") ?: 0L, + completedAt = doc.getLong("completedAt"), + partnerCompletedAt = doc.getLong("partnerCompletedAt"), + isPremium = doc.getBoolean("isPremium") ?: false, + status = doc.getString("status") ?: "active", + gameType = doc.getString("gameType") ?: GameType.WHEEL, + completedByUsers = (doc.get("completedByUsers") as? List<*>)?.filterIsInstance() + ?: emptyList() + ) + override suspend fun getSessionsForCouple(coupleId: String): Result> = runCatching { firestore.collection(FirestoreCollections.COUPLES) @@ -223,4 +294,9 @@ class QuestionSessionRepositoryImpl @Inject constructor( }.onFailure { crashReporter.recordException(it) }.getOrNull() } }.getOrNull() + + companion object { + /** Per-couple pointer doc that serializes concurrent game starts (F-RACE-001 lock). */ + private const val ACTIVE_POINTER_ID = "_active" + } } diff --git a/app/src/main/java/app/closer/domain/repository/QuestionSessionRepository.kt b/app/src/main/java/app/closer/domain/repository/QuestionSessionRepository.kt index e80d7d2a..31a7d736 100644 --- a/app/src/main/java/app/closer/domain/repository/QuestionSessionRepository.kt +++ b/app/src/main/java/app/closer/domain/repository/QuestionSessionRepository.kt @@ -3,9 +3,25 @@ package app.closer.domain.repository import app.closer.domain.model.QuestionSession import kotlinx.coroutines.flow.Flow +/** Outcome of an atomic session start: we either created a fresh session, or one was already active. */ +sealed interface SessionStartOutcome { + val session: QuestionSession + data class Created(override val session: QuestionSession) : SessionStartOutcome + data class AlreadyActive(override val session: QuestionSession) : SessionStartOutcome +} + interface QuestionSessionRepository { /** Saves the session and returns its (possibly auto-generated) document id. */ suspend fun saveSession(session: QuestionSession): Result + + /** + * Atomically start a session: creates a new active session ONLY if the couple has none, via a + * Firestore transaction on a per-couple pointer doc. Concurrent starts converge to a single + * session — the loser gets [SessionStartOutcome.AlreadyActive] instead of creating a duplicate + * (fixes F-RACE-001, the simultaneous-start race that produced two divergent sessions). + */ + suspend fun startSessionAtomically(session: QuestionSession): Result + suspend fun getSessionsForCouple(coupleId: String): Result> // Active session queries diff --git a/app/src/main/java/app/closer/domain/usecase/GameSessionManager.kt b/app/src/main/java/app/closer/domain/usecase/GameSessionManager.kt index 3c619c4b..047d3e26 100644 --- a/app/src/main/java/app/closer/domain/usecase/GameSessionManager.kt +++ b/app/src/main/java/app/closer/domain/usecase/GameSessionManager.kt @@ -6,6 +6,7 @@ import app.closer.domain.model.QuestionSession import app.closer.domain.repository.AuthRepository import app.closer.domain.repository.CoupleRepository import app.closer.domain.repository.QuestionSessionRepository +import app.closer.domain.repository.SessionStartOutcome import app.closer.domain.repository.UserRepository import kotlinx.coroutines.flow.Flow import javax.inject.Inject @@ -81,17 +82,6 @@ class GameSessionManager @Inject constructor( categoryId: String?, questionIds: List? ): Result { - val activeSession = sessionRepository.getActiveSessionForCouple(couple.id) - if (activeSession != null) { - val partnerId = couple.userIds.firstOrNull { it != userId } - val partnerName = partnerId - ?.let { runCatching { userRepository.getUser(it) }.getOrNull() } - ?.displayName ?: "Partner" - return Result.failure( - Exception("partner_active_session|$partnerName|${gameTypeLabel(gameType)}") - ) - } - val session = QuestionSession( coupleId = couple.id, categoryId = categoryId ?: "", @@ -101,9 +91,28 @@ class GameSessionManager @Inject constructor( status = "active" ) - // Use the id assigned by the repository so the started game observes the right doc - // (an empty id builds an invalid Firestore path and crashes the game on open). - return sessionRepository.saveSession(session).map { id -> session.copy(id = id) } + // Atomic "create only if no active session" (Firestore transaction on a per-couple pointer): + // two partners tapping start at the same instant converge to ONE session instead of two + // divergent ones (F-RACE-001). The loser is told a session is already active and joins it. + return sessionRepository.startSessionAtomically(session).fold( + onSuccess = { outcome -> + when (outcome) { + is SessionStartOutcome.Created -> Result.success(outcome.session) + is SessionStartOutcome.AlreadyActive -> { + val partnerId = couple.userIds.firstOrNull { it != userId } + val partnerName = partnerId + ?.let { runCatching { userRepository.getUser(it) }.getOrNull() } + ?.displayName ?: "Partner" + Result.failure( + Exception( + "partner_active_session|$partnerName|${gameTypeLabel(outcome.session.gameType)}" + ) + ) + } + } + }, + onFailure = { Result.failure(it) } + ) } /** diff --git a/firestore.rules b/firestore.rules index 74c4d74a..e18c81f7 100644 --- a/firestore.rules +++ b/firestore.rules @@ -305,6 +305,12 @@ service cloud.firestore { // Read: both members can read allow read: if isCouplesMember(coupleId); + // Per-couple active-session pointer used as an atomic lock so two partners starting a game + // at the same instant converge to ONE session instead of two divergent ones (F-RACE-001). + // It holds no game content (only activeSessionId + updatedAt) and carries no status/ + // completedAt, so it never appears in the active-session or history queries. + allow create, update: if sessionId == '_active' && isCouplesMember(coupleId); + // Create: either member can start a session allow create: if isCouplesMember(coupleId) && request.resource.data.startedByUserId == request.auth.uid;