From a02d3b3335a42f276fde7e87fffd133f60ebff8f Mon Sep 17 00:00:00 2001 From: lila Date: Tue, 28 Apr 2026 15:44:29 +0200 Subject: [PATCH] fix: deduplicate distractors against each other, guard thin distractor pool --- .../InMemoryGameSessionStore.test.ts | 17 + apps/api/src/services/gameService.test.ts | 42 +- apps/api/src/services/gameService.ts | 13 +- documentation/roasts/gameService.md | 366 ------------------ 4 files changed, 68 insertions(+), 370 deletions(-) diff --git a/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts b/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts index 918d8ed..fae0365 100644 --- a/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts +++ b/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts @@ -42,4 +42,21 @@ describe("InMemoryGameSessionStore", () => { const result = await store.get("session-1"); expect(result).not.toBeNull(); }); + + it("update persists modified session data", async () => { + const data = { + answers: new Map([ + ["q1", 2], + ["q2", 1], + ]), + userId: "user-1", + }; + await store.create("session-1", data, 60_000); + + const updated = { answers: new Map([["q2", 1]]), userId: "user-1" }; + await store.update("session-1", updated); + + const result = await store.get("session-1"); + expect(result).toEqual(updated); + }); }); diff --git a/apps/api/src/services/gameService.test.ts b/apps/api/src/services/gameService.test.ts index d5b56de..98cad3b 100644 --- a/apps/api/src/services/gameService.test.ts +++ b/apps/api/src/services/gameService.test.ts @@ -32,7 +32,14 @@ const fakeTerms = [ beforeEach(() => { vi.clearAllMocks(); mockGetGameTerms.mockResolvedValue(fakeTerms); - mockGetDistractors.mockResolvedValue(["wrong1", "wrong2", "wrong3"]); + mockGetDistractors.mockResolvedValue([ + "wrong1", + "wrong2", + "wrong3", + "wrong4", + "wrong5", + "wrong6", + ]); }); describe("createGameSession", () => { @@ -151,6 +158,39 @@ describe("createGameSession", () => { createGameSession(validRequest, store, "user-1"), ).rejects.toThrow("db timeout"); }); + + it("throws when fewer than 3 unique distractors remain after deduplication", async () => { + mockGetDistractors.mockResolvedValueOnce([ + "cane", + "cane", + "cane", + "cane", + "cane", + "cane", + ]); + + await expect( + createGameSession(validRequest, store, "user-1"), + ).rejects.toThrow("Not enough unique distractors"); + }); + + it("duplicate distractors are deduplicated against each other", async () => { + mockGetDistractors.mockResolvedValueOnce([ + "wrong1", + "wrong1", + "wrong1", + "wrong2", + "wrong3", + "wrong4", + ]); + + const session = await createGameSession(validRequest, store, "user-1"); + const question = session.questions[0]!; + const optionTexts = question.options.map((o) => o.text); + + expect(new Set(optionTexts).size).toBe(4); + expect(question.options).toHaveLength(4); + }); }); describe("evaluateAnswer", () => { diff --git a/apps/api/src/services/gameService.ts b/apps/api/src/services/gameService.ts index a64b4e7..309fa1a 100644 --- a/apps/api/src/services/gameService.ts +++ b/apps/api/src/services/gameService.ts @@ -42,9 +42,16 @@ export const createGameSession = async ( 6, ); - const uniqueDistractors = distractorTexts.filter( - (t) => t !== term.targetText, - ); + const uniqueDistractors = [ + ...new Set(distractorTexts.filter((t) => t !== term.targetText)), + ]; + + if (uniqueDistractors.length < 3) { + throw new Error( + `Not enough unique distractors for term: ${term.targetText}`, + ); + } + const optionTexts = [term.targetText, ...uniqueDistractors.slice(0, 3)]; const shuffledTexts = shuffleArray(optionTexts); const correctOptionId = shuffledTexts.indexOf(term.targetText); diff --git a/documentation/roasts/gameService.md b/documentation/roasts/gameService.md index 0db3559..e69de29 100644 --- a/documentation/roasts/gameService.md +++ b/documentation/roasts/gameService.md @@ -1,366 +0,0 @@ -# `gameService.ts` — Code Review & Fixes - ---- - -## 1. Hardcoded singleton kills the abstraction - -**Problem** - -A `GameSessionStore` interface exists, an `InMemoryGameSessionStore` implements it, and then the concrete class is immediately hardcoded as a module-level singleton. The interface is decorative — nothing can inject an alternative implementation without editing this file. - -```ts -// ❌ current — store is unreachable from outside -const gameSessionStore = new InMemoryGameSessionStore(); - -export const createGameSession = async (request: GameRequest) => { ... }; -export const evaluateAnswer = async (submission: AnswerSubmission) => { ... }; -``` - -**Fix — inject the store** - -Accept the store as a parameter (or use a factory). The simplest approach that requires no framework: - -```ts -// ✅ inject the store -export const createGameSession = async ( - request: GameRequest, - store: GameSessionStore, -): Promise => { ... }; - -export const evaluateAnswer = async ( - submission: AnswerSubmission, - store: GameSessionStore, -): Promise => { ... }; -``` - -The call site (controller) owns the store instance and passes it in. Tests can pass a fresh `InMemoryGameSessionStore` per test — no mocking required, no shared state. - -```ts -// gameController.ts -const store = new InMemoryGameSessionStore(); - -// later, swap for ValKeyGameSessionStore with one line change -``` - ---- - -## 2. Sessions are never deleted — memory leak - -**Problem** - -`GameSessionStore.delete()` is defined and implemented but never called. Every session ever created stays in the Map until the process restarts. Under real traffic this is a slow memory leak; under a spike it's a fast one. - -**Fix — delete after answer, or add a TTL** - -The simplest fix: delete the session once the last question is answered. If partial completion is needed, add a TTL on creation instead. - -```ts -// ✅ option A — delete on answer -export const evaluateAnswer = async ( - submission: AnswerSubmission, - store: GameSessionStore, -): Promise => { - const session = await store.get(submission.sessionId); - if (!session) - throw new NotFoundError(`Game session not found: ${submission.sessionId}`); - - const correctOptionId = session.answers.get(submission.questionId); - if (correctOptionId === undefined) - throw new NotFoundError(`Question not found: ${submission.questionId}`); - - // delete answered question; delete session when all questions are answered - session.answers.delete(submission.questionId); - if (session.answers.size === 0) { - await store.delete(submission.sessionId); - } - - return { - questionId: submission.questionId, - isCorrect: submission.selectedOptionId === correctOptionId, - correctOptionId, - selectedOptionId: submission.selectedOptionId, - }; -}; -``` - -```ts -// ✅ option B — TTL in InMemoryGameSessionStore -export class InMemoryGameSessionStore implements GameSessionStore { - private sessions = new Map< - string, - { data: GameSessionData; expiresAt: number } - >(); - private readonly ttlMs: number; - - constructor(ttlMs = 30 * 60 * 1000) { - // 30 minutes default - this.ttlMs = ttlMs; - } - - create(sessionId: string, data: GameSessionData): Promise { - this.sessions.set(sessionId, { data, expiresAt: Date.now() + this.ttlMs }); - return Promise.resolve(); - } - - get(sessionId: string): Promise { - const entry = this.sessions.get(sessionId); - if (!entry) return Promise.resolve(null); - if (Date.now() > entry.expiresAt) { - this.sessions.delete(sessionId); - return Promise.resolve(null); - } - return Promise.resolve(entry.data); - } - - delete(sessionId: string): Promise { - this.sessions.delete(sessionId); - return Promise.resolve(); - } -} -``` - ---- - -**Problem** - -`GameRequest.rounds` is typed as `string` in `@lila/shared`, forcing the service to cast it every time: - -```ts -// ❌ why is a round count a string? -Number(request.rounds); -``` - -**Fix — fix the schema in `@lila/shared`** - -```ts -// ✅ in packages/shared -export const GameRequestSchema = z.object({ - source_language: z.string(), - target_language: z.string(), - pos: z.string(), - difficulty: z.string(), - rounds: z.coerce.number().int().min(1).max(50), // coerce handles form inputs, validates range -}); - -export type GameRequest = z.infer; -``` - -The `z.coerce.number()` handles the case where the value arrives as a string from a query param or form — Zod does the conversion at the boundary so the rest of the system never sees a string. - ---- - -**Problem** - -The variable holds `terms` — word pairs fetched from the database. Calling them `correctAnswers` jumps ahead semantically; they only become "correct answers" once options are constructed around them. - -```ts -// ❌ these are terms, not answers yet -const correctAnswers = await getGameTerms(...); -``` - -**Fix** - -```ts -// ✅ -const terms = await getGameTerms(...); - -// and inside the map: -terms.map(async (term) => { - const distractorTexts = await getDistractors( - term.termId, - term.targetText, - ... - ); - ... - const correctOptionId = shuffledTexts.indexOf(term.targetText); - ... -}); -``` - ---- - -## 6. Tautological test: `"distractors are never the correct answer"` - -**Problem** - -The test filters the correct answer out of the options array, then asserts the remaining items are not the correct answer. It is testing that `Array.filter` works. - -```ts -// ❌ this cannot fail -it("distractors are never the correct answer", async () => { - const distractorTexts = question.options - .map((o) => o.text) - .filter((t) => t !== correctText); // removes correct answer... - - for (const text of distractorTexts) { - expect(text).not.toBe(correctText); // ...then checks they're not the correct answer - } -}); -``` - -**What to actually test** - -The real concern is that `getDistractors` doesn't return the target word. Test that the service handles it correctly if it does: - -```ts -// ✅ test that the correct answer appears exactly once even if a distractor collides -it("correct answer appears exactly once in options even if distractor matches", async () => { - // simulate getDistractors returning the correct answer as one of the distractors - mockGetDistractors.mockResolvedValueOnce(["cane", "wrong2", "wrong3"]); - - const session = await createGameSession( - validRequest, - new InMemoryGameSessionStore(), - ); - const question = session.questions[0]!; - const optionTexts = question.options.map((o) => o.text); - - // "cane" should only appear once regardless of the duplicate from getDistractors - expect(optionTexts.filter((t) => t === "cane")).toHaveLength(1); - expect(question.options).toHaveLength(4); -}); -``` - -> **Note:** the current implementation doesn't actually handle this case — a duplicate distractor would produce a 4-option list where the correct answer appears twice and one distractor slot is wasted. Worth fixing in `createGameSession` alongside the test. - ---- - -## 7. Store not reset between tests - -**Problem** - -`beforeEach` calls `vi.clearAllMocks()` which resets mock functions, but the `gameSessionStore` module-level singleton is never cleared. Ghost sessions from earlier tests persist for the entire test run. - -It doesn't bite today because each session gets a unique UUID and tests don't share IDs — but it's one non-UUID lookup away from a very confusing afternoon. - -**Fix — a consequence of fixing issue #1** - -Once the store is injected rather than module-level, each test creates its own instance: - -```ts -// ✅ no shared state, no ghost sessions -describe("evaluateAnswer", () => { - it("returns isCorrect: true for correct option", async () => { - const store = new InMemoryGameSessionStore(); - const session = await createGameSession(validRequest, store); - ... - const result = await evaluateAnswer({ ... }, store); - ... - }); -}); -``` - -No `beforeEach` cleanup needed — the store simply doesn't outlive the test that created it. - ---- - -## 8. No answer replay protection - -**Problem** - -`evaluateAnswer` can be called multiple times with the same `questionId`. The -service will evaluate it every time. In multiplayer this could be abused to -farm points or desync state. - -**Fix — delete the question from the answer key after first evaluation** - -```ts -// ✅ inside evaluateAnswer, after retrieving correctOptionId -session.answers.delete(submission.questionId); - -if (submission.selectedOptionId !== correctOptionId) { - // already removed — can't retry -} -``` - -Once the question key is deleted, a second submission hits the -`correctOptionId === undefined` branch and throws `NotFoundError`. One shot -per question. - ---- - -## 9. No ownership check in `evaluateAnswer` - -**Problem** - -The service accepts any `sessionId` without verifying it belongs to the -requesting user. If auth middleware doesn't tie sessions to users at a higher -layer, Alice can submit answers for Bob's session by guessing or intercepting -his `sessionId`. - -**Fix — store `userId` alongside the session and assert it on retrieval** - -```ts -// GameSessionStore.ts -export type GameSessionData = { answers: Map; userId: string }; - -// evaluateAnswer -const session = await store.get(submission.sessionId); - -if (!session) throw new NotFoundError(`Game session not found`); -if (session.userId !== requestingUserId) - throw new NotFoundError(`Game session not found`); -// ^^^ same error — don't confirm the session exists to the wrong user -``` - -Pass `requestingUserId` in from the controller, where it's already available -via auth middleware. - ---- - -## 10. No test for empty `getGameTerms` result - -**Problem** - -If the database returns zero terms (no words match the difficulty/language/pos -filter), `createGameSession` happily returns a session with an empty -`questions` array. The frontend receives it, tries to render question 1, and -crashes. The user sees nothing useful. - -**Fix — guard in the service and add a test** - -```ts -// ✅ inside createGameSession, after fetching terms -if (terms.length === 0) { - throw new AppError("No terms found for the given filters", 404); -} -``` - -```ts -// ✅ test -it("throws when getGameTerms returns no terms", async () => { - mockGetGameTerms.mockResolvedValue([]); - - await expect( - createGameSession(validRequest, new InMemoryGameSessionStore()), - ).rejects.toThrow("No terms found"); -}); -``` - ---- - -## 11. No test for `getDistractors` rejection - -**Problem** - -`createGameSession` uses `Promise.all` over the terms array. If -`getDistractors` rejects for any single term, the entire `Promise.all` rejects -— no session is created, no partial recovery, the user gets a 500 with -"connection refused" leaking through. - -**Fix — test the failure path and consider a fallback** - -```ts -// ✅ test -it("propagates getDistractors failure", async () => { - mockGetDistractors.mockRejectedValue(new Error("db timeout")); - - await expect( - createGameSession(validRequest, new InMemoryGameSessionStore()), - ).rejects.toThrow("db timeout"); -}); -``` - -For resilience, consider catching per-term distractor failures and falling back -to random terms from the already-fetched set rather than collapsing the whole -session.