fix(session): reclaim orphan session codes on create#162
Conversation
WalkthroughSession-code reclaimability now covers missing or ended session data. Session creation attempts to delete existing code documents before reuse, session ending deletes the associated code, and Firestore rules authorize eligible deletions. Tests cover helper behavior and post-ending lookup results. ChangesSession code lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/firestore/firestoreAnnotations.reclaim.test.ts`:
- Around line 4-20: Expand the Vitest Firestore emulator coverage beyond
isReclaimableSessionForCode to exercise createRemoteSession and
endRemoteSession: reclaim missing and ended sessions, preserve active mappings,
delete only the ending session’s mapping, and verify persisted endedAt values
round-trip in the expected representation. Keep the tests focused on the changed
lifecycle behavior and its edge cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: adfe991c-82da-4490-93a1-75dbab5600bd
📒 Files selected for processing (2)
src/services/firestore/firestoreAnnotations.reclaim.test.tssrc/services/firestore/firestoreAnnotations.ts
| describe("isReclaimableSessionForCode", () => { | ||
| it("reclaims missing sessions", () => { | ||
| expect(isReclaimableSessionForCode(null)).toBe(true); | ||
| expect(isReclaimableSessionForCode(undefined)).toBe(true); | ||
| }); | ||
|
|
||
| it("reclaims ended sessions", () => { | ||
| expect(isReclaimableSessionForCode({ status: "ended" })).toBe(true); | ||
| expect( | ||
| isReclaimableSessionForCode({ endedAt: "2026-01-01T00:00:00.000Z" }), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("does not reclaim live active sessions", () => { | ||
| expect(isReclaimableSessionForCode({ status: "active" })).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
Cover the Firestore lifecycle, not only the predicate.
These tests do not exercise the changed createRemoteSession and endRemoteSession behavior. Add Vitest emulator coverage for reclaiming missing/ended sessions, preserving active mappings, deleting only the ending session’s mapping, and verifying the persisted endedAt representation round-trips correctly.
As per path instructions, tests should cover changed behavior and edge cases in the Vitest-focused scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/firestore/firestoreAnnotations.reclaim.test.ts` around lines 4 -
20, Expand the Vitest Firestore emulator coverage beyond
isReclaimableSessionForCode to exercise createRemoteSession and
endRemoteSession: reclaim missing and ended sessions, preserve active mappings,
delete only the ending session’s mapping, and verify persisted endedAt values
round-trip in the expected representation. Keep the tests focused on the changed
lifecycle behavior and its edge cases.
Source: Path instructions
fbed16b to
420cec4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/firestore/firestoreAnnotations.ts (1)
760-767: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake session ending and code deletion retryable as one lifecycle operation.
The session update removes
codebeforedeleteDocis confirmed. If deletion fails after the update, the operation rejects, but a retry reads a session without its code and can no longer identify the stalesessionCodes/{code}document. Use an atomic lifecycle write or retain durable cleanup state until deletion succeeds.As per path instructions, Firestore services must cover “Offline write queuing and idempotency.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/firestore/firestoreAnnotations.ts` around lines 760 - 767, Update the session-ending flow around the session update and sessionCodeDoc deletion so the session code remains durably available until stale-code cleanup succeeds, or perform both changes through an atomic Firestore lifecycle write. Ensure retries can identify and delete the same sessionCodes/{code} document after offline queuing or a partial failure, while preserving idempotent behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@firestore.rules`:
- Around line 778-787: Update the delete rule for the code mapping to reject
deletion solely by the host; allow it only when the linked session is missing or
ended, while retaining the signed-in, code-size, and sessionId validations.
Ensure the rule preserves authenticated-write checks and matches the TypeScript
session-ending lifecycle so active session mappings cannot be removed during
collision handling.
In `@src/services/firestore/firestoreAnnotations.ts`:
- Around line 276-283: Update the session-code deletion retry loop around
sessionCodeDoc and generateSessionCode so exhausting the retry limit never
leaves an unchecked candidate for the subsequent write. After the final failed
deletion, either throw immediately or verify the newly generated code before
proceeding, while preserving the existing retry behavior for attempts that
remain.
---
Outside diff comments:
In `@src/services/firestore/firestoreAnnotations.ts`:
- Around line 760-767: Update the session-ending flow around the session update
and sessionCodeDoc deletion so the session code remains durably available until
stale-code cleanup succeeds, or perform both changes through an atomic Firestore
lifecycle write. Ensure retries can identify and delete the same
sessionCodes/{code} document after offline queuing or a partial failure, while
preserving idempotent behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d28d2246-ecc5-4684-ac98-e768b94d871c
📒 Files selected for processing (4)
firestore.rulessrc/services/firestore/firestoreAnnotations.emulator.test.tssrc/services/firestore/firestoreAnnotations.reclaim.test.tssrc/services/firestore/firestoreAnnotations.ts
| // Host end path, or reclaim when the linked session is missing / ended. | ||
| allow delete: if isSignedIn() | ||
| && code.size() == 4 | ||
| && resource.data.sessionId is string | ||
| && ( | ||
| resource.data.hostUid == request.auth.uid | ||
| || !exists(/databases/$(database)/documents/sessions/$(resource.data.sessionId)) | ||
| || get(/databases/$(database)/documents/sessions/$(resource.data.sessionId)).data.status == 'ended' | ||
| || get(/databases/$(database)/documents/sessions/$(resource.data.sessionId)).data.endedAt is string | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not allow deletion of an active session’s code.
The resource.data.hostUid == request.auth.uid branch permits deleting a code while its linked session is still active. Since createRemoteSession attempts this deletion for every collision, a host can remove an active mapping, reuse the code for a new session, and leave the old session pointing at the new mapping.
Restrict deletion to missing or ended linked sessions, or make the end update and deletion an atomic, end-specific operation.
As per path instructions, Firestore rules require auth checks on all writes and parity with the TypeScript lifecycle behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@firestore.rules` around lines 778 - 787, Update the delete rule for the code
mapping to reject deletion solely by the host; allow it only when the linked
session is missing or ended, while retaining the signed-in, code-size, and
sessionId validations. Ensure the rule preserves authenticated-write checks and
matches the TypeScript session-ending lifecycle so active session mappings
cannot be removed during collision handling.
Source: Path instructions
| try { | ||
| // Rules allow delete only for host, missing session, or ended session. | ||
| await deleteDoc(sessionCodeDoc(code)); | ||
| break; | ||
| } catch { | ||
| code = generateSessionCode(); | ||
| attempts += 1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not use the unverified candidate after the retry limit.
After the eighth failed deletion, Line 281 generates a new code and the loop exits without checking whether that candidate already exists. The subsequent write can collide with an existing document and fail unexpectedly. Throw when the limit is exhausted or verify the final candidate before writing.
As per path instructions, Firestore services must cover “Offline write queuing and idempotency.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/firestore/firestoreAnnotations.ts` around lines 276 - 283,
Update the session-code deletion retry loop around sessionCodeDoc and
generateSessionCode so exhausting the retry limit never leaves an unchecked
candidate for the subsequent write. After the final failed deletion, either
throw immediately or verify the newly generated code before proceeding, while
preserving the existing retry behavior for attempts that remain.
Source: Path instructions
* fix(session): reclaim orphan session codes on create * fix(session): allow orphan sessionCodes delete in rules * test(session): expect missing code after endRemoteSession delete
1 participant
Summary
sessionCodesdocs whose session is missing or already ended (instead of only regenerating).endRemoteSessiondeletes the code document instead of markingstatus: "ended"(defense in depth; host UI prefers the endSession callable)..cursor/scripts/(not committed).Test plan
npm test -- src/services/firestore/firestoreAnnotations.reclaim.test.ts--applyoneshot (local script)Summary by CodeRabbit