Skip to content

fix(session): reclaim orphan session codes on create #162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gelbh merged 3 commits into from
Jul 24, 2026
+55 −8
Merged

fix(session): reclaim orphan session codes on create #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efb3166
fix(session): reclaim orphan session codes on create
gelbh Jul 24, 2026
c3f773d
fix(session): allow orphan sessionCodes delete in rules
gelbh Jul 24, 2026
420cec4
test(session): expect missing code after endRemoteSession delete
gelbh Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion firestore.rules
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,16 @@ service cloud.firestore {
&& request.resource.data.diff(resource.data).affectedKeys().hasOnly(['status'])
&& request.resource.data.status == 'ended';

allow delete: if false;
// 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
);
Comment on lines +778 to +787

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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

}

match /_rateLimits/{docId} {
Expand Down
4 changes: 2 additions & 2 deletions src/services/firestore/firestoreAnnotations.emulator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,14 @@ describe("firestoreAnnotations emulator", () => {
unsubscribe();
});

it("marks ended sessions as ended on lookup", async () => {
it("deletes join code when ending a session", async () => {
const { uid } = await connectEmulatorsForTests();
const session = await createRemoteSession(DUBLIN_CITY_GAME_AREA, uid);
const sessionCode = session.code;
await endRemoteSession(session.id);

const lookup = await lookupRemoteSessionByCode(sessionCode);
expect(lookup.status).toBe("ended");
expect(lookup.status).toBe("missing");

const fetched = await getRemoteSessionById(session.id);
expect(fetched?.endedAt).toBeTruthy();
Expand Down
20 changes: 20 additions & 0 deletions src/services/firestore/firestoreAnnotations.reclaim.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { describe, expect, it } from "vitest";
import { isReclaimableSessionForCode } from "./firestoreAnnotations";

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);
});
});
Comment on lines +4 to +20

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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

28 changes: 23 additions & 5 deletions src/services/firestore/firestoreAnnotations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
arrayRemove,
arrayUnion,
collection,
deleteDoc,
deleteField,
doc,
getDoc,
Expand Down Expand Up @@ -66,6 +67,19 @@ function sessionsCollection() {
function sessionCodeDoc(code: string) {
return doc(getFirestoreDb(), "sessionCodes", code);
}

/** True when a sessionCodes doc may be deleted and the code reused. */
export function isReclaimableSessionForCode(
sessionData: Record<string, unknown> | null | undefined,
): boolean {
if (sessionData == null) {
return true;
}

return (
sessionData.status === "ended" || typeof sessionData.endedAt === "string"
);
}
function annotationsCollection(sessionId: string) {
return collection(getFirestoreDb(), "sessions", sessionId, "annotations");
}
Expand Down Expand Up @@ -259,8 +273,14 @@ export async function createRemoteSession(
break;
}

code = generateSessionCode();
attempts += 1;
try {
// Rules allow delete only for host, missing session, or ended session.
await deleteDoc(sessionCodeDoc(code));
break;
} catch {
code = generateSessionCode();
attempts += 1;
}
Comment on lines +276 to +283

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

}

const sessionRef = doc(sessionsCollection());
Expand Down Expand Up @@ -744,9 +764,7 @@ export async function endRemoteSession(sessionId: string): Promise<void> {
});

if (session?.code) {
await updateDoc(sessionCodeDoc(session.code), {
status: "ended",
});
await deleteDoc(sessionCodeDoc(session.code));
}
}

Expand Down
Loading