From a02d3b3335a42f276fde7e87fffd133f60ebff8f Mon Sep 17 00:00:00 2001 From: lila Date: Tue, 28 Apr 2026 15:44:29 +0200 Subject: [PATCH 1/6] 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. From c081e632cfcf01f69e44aef98f03a66d4210d409 Mon Sep 17 00:00:00 2001 From: lila Date: Tue, 28 Apr 2026 15:47:53 +0200 Subject: [PATCH 2/6] fix: explicit store update in evaluateAnswer, remove mutation through reference --- apps/api/src/gameSessionStore/GameSessionStore.ts | 3 ++- .../src/gameSessionStore/InMemoryGameSessionStore.ts | 7 +++++++ apps/api/src/services/gameService.ts | 10 ++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/apps/api/src/gameSessionStore/GameSessionStore.ts b/apps/api/src/gameSessionStore/GameSessionStore.ts index 75d42cb..14e27b0 100644 --- a/apps/api/src/gameSessionStore/GameSessionStore.ts +++ b/apps/api/src/gameSessionStore/GameSessionStore.ts @@ -7,5 +7,6 @@ export interface GameSessionStore { ttlMs: number, ): Promise; get(sessionId: string): Promise; + update(sessionId: string, data: GameSessionData): Promise; delete(sessionId: string): Promise; -} +} \ No newline at end of file diff --git a/apps/api/src/gameSessionStore/InMemoryGameSessionStore.ts b/apps/api/src/gameSessionStore/InMemoryGameSessionStore.ts index 8ec2265..71e4b4c 100644 --- a/apps/api/src/gameSessionStore/InMemoryGameSessionStore.ts +++ b/apps/api/src/gameSessionStore/InMemoryGameSessionStore.ts @@ -24,6 +24,13 @@ export class InMemoryGameSessionStore implements GameSessionStore { return Promise.resolve(entry.data); } + update(sessionId: string, data: GameSessionData): Promise { + const entry = this.sessions.get(sessionId); + if (!entry) return Promise.resolve(); + this.sessions.set(sessionId, { data, expiresAt: entry.expiresAt }); + return Promise.resolve(); + } + delete(sessionId: string): Promise { this.sessions.delete(sessionId); return Promise.resolve(); diff --git a/apps/api/src/services/gameService.ts b/apps/api/src/services/gameService.ts index 309fa1a..573f7a8 100644 --- a/apps/api/src/services/gameService.ts +++ b/apps/api/src/services/gameService.ts @@ -96,10 +96,16 @@ export const evaluateAnswer = async ( throw new NotFoundError(`Question not found: ${submission.questionId}`); } - session.answers.delete(submission.questionId); + const updatedAnswers = new Map(session.answers); + updatedAnswers.delete(submission.questionId); - if (session.answers.size === 0) { + if (updatedAnswers.size === 0) { await store.delete(submission.sessionId); + } else { + await store.update(submission.sessionId, { + answers: updatedAnswers, + userId: session.userId, + }); } return { From 6eaf282651532be7b96ef9b388360c59a5d9608f Mon Sep 17 00:00:00 2001 From: lila Date: Tue, 28 Apr 2026 15:51:57 +0200 Subject: [PATCH 3/6] fix: sanitise Zod validation error messages in game controller --- apps/api/src/controllers/gameController.test.ts | 11 +++++++++++ apps/api/src/controllers/gameController.ts | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/apps/api/src/controllers/gameController.test.ts b/apps/api/src/controllers/gameController.test.ts index d8115bd..d47f8b9 100644 --- a/apps/api/src/controllers/gameController.test.ts +++ b/apps/api/src/controllers/gameController.test.ts @@ -119,6 +119,17 @@ describe("POST /api/v1/game/start", () => { expect(res.status).toBe(404); expect(body.success).toBe(false); }); + + it("returns a sanitised error message when the body is invalid", async () => { + const res = await request(app) + .post("/api/v1/game/start") + .send({ ...validBody, difficulty: "impossible" }); + const body = res.body as ErrorResponse; + expect(res.status).toBe(400); + expect(body.error).toBe("Invalid game settings"); + expect(body.error).not.toContain("Invalid literal value"); + expect(body.error).not.toContain("Invalid enum value"); + }); }); describe("POST /api/v1/game/answer", () => { diff --git a/apps/api/src/controllers/gameController.ts b/apps/api/src/controllers/gameController.ts index 72a9414..2a0416e 100644 --- a/apps/api/src/controllers/gameController.ts +++ b/apps/api/src/controllers/gameController.ts @@ -14,7 +14,7 @@ export const createGameController = (store: GameSessionStore) => ({ try { const gameSettings = GameRequestSchema.safeParse(req.body); if (!gameSettings.success) { - throw new ValidationError(gameSettings.error.message); + throw new ValidationError("Invalid game settings"); } const gameQuestions = await createGameSession( gameSettings.data, @@ -35,7 +35,7 @@ export const createGameController = (store: GameSessionStore) => ({ try { const submission = AnswerSubmissionSchema.safeParse(req.body); if (!submission.success) { - throw new ValidationError(submission.error.message); + throw new ValidationError("Invalid answer submission"); } const result = await evaluateAnswer( submission.data, From 648c5d29793dd3b1527cb97d67f7473913350d81 Mon Sep 17 00:00:00 2001 From: lila Date: Tue, 28 Apr 2026 16:07:19 +0200 Subject: [PATCH 4/6] fix: improve error semantics, clarify answer key type --- .../src/controllers/gameController.test.ts | 12 +++---- apps/api/src/errors/AppError.ts | 6 ++++ .../src/gameSessionStore/GameSessionStore.ts | 7 +++-- .../InMemoryGameSessionStore.test.ts | 31 +++++++++++++------ apps/api/src/services/gameService.test.ts | 16 +++++----- apps/api/src/services/gameService.ts | 24 ++++++++------ 6 files changed, 62 insertions(+), 34 deletions(-) diff --git a/apps/api/src/controllers/gameController.test.ts b/apps/api/src/controllers/gameController.test.ts index d47f8b9..168d2d1 100644 --- a/apps/api/src/controllers/gameController.test.ts +++ b/apps/api/src/controllers/gameController.test.ts @@ -111,12 +111,12 @@ describe("POST /api/v1/game/start", () => { expect(body.success).toBe(false); }); - it("returns 404 when no terms are found for the given filters", async () => { + it("returns 422 when no terms are found for the given filters", async () => { mockGetGameTerms.mockResolvedValue([]); const res = await request(app).post("/api/v1/game/start").send(validBody); const body = res.body as ErrorResponse; - expect(res.status).toBe(404); + expect(res.status).toBe(422); expect(body.success).toBe(false); }); @@ -178,7 +178,7 @@ describe("POST /api/v1/game/answer", () => { expect(body.error).toContain("Game session not found"); }); - it("returns 404 when the question does not exist in the session", async () => { + it("returns 409 when the question does not exist in the session", async () => { const startRes = await request(app) .post("/api/v1/game/start") .send(validBody); @@ -193,11 +193,11 @@ describe("POST /api/v1/game/answer", () => { selectedOptionId: 0, }); const body = res.body as ErrorResponse; - expect(res.status).toBe(404); + expect(res.status).toBe(409); expect(body.success).toBe(false); - expect(body.error).toContain("Question not found"); + expect(body.error).toContain("Question already answered"); }); - + it("returns 400 when a field has an invalid value", async () => { const res = await request(app) .post("/api/v1/game/start") diff --git a/apps/api/src/errors/AppError.ts b/apps/api/src/errors/AppError.ts index 4677d9f..7805f3e 100644 --- a/apps/api/src/errors/AppError.ts +++ b/apps/api/src/errors/AppError.ts @@ -25,3 +25,9 @@ export class ConflictError extends AppError { super(message, 409); } } + +export class UnprocessableEntityError extends AppError { + constructor(message: string) { + super(message, 422); + } +} \ No newline at end of file diff --git a/apps/api/src/gameSessionStore/GameSessionStore.ts b/apps/api/src/gameSessionStore/GameSessionStore.ts index 14e27b0..3e6c5d2 100644 --- a/apps/api/src/gameSessionStore/GameSessionStore.ts +++ b/apps/api/src/gameSessionStore/GameSessionStore.ts @@ -1,4 +1,7 @@ -export type GameSessionData = { answers: Map; userId: string }; +export type GameSessionData = { + answers: Map; + userId: string; +}; export interface GameSessionStore { create( @@ -9,4 +12,4 @@ export interface GameSessionStore { get(sessionId: string): Promise; update(sessionId: string, data: GameSessionData): Promise; delete(sessionId: string): Promise; -} \ No newline at end of file +} diff --git a/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts b/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts index fae0365..d08be1f 100644 --- a/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts +++ b/apps/api/src/gameSessionStore/InMemoryGameSessionStore.test.ts @@ -14,14 +14,20 @@ describe("InMemoryGameSessionStore", () => { }); it("returns session data after creation", async () => { - const data = { answers: new Map([["q1", 2]]), userId: "user-1" }; + const data = { + answers: new Map([["q1", { correctOptionId: 2 }]]), + userId: "user-1", + }; await store.create("session-1", data, 60_000); const result = await store.get("session-1"); expect(result).toEqual(data); }); it("returns null after the session is deleted", async () => { - const data = { answers: new Map([["q1", 2]]), userId: "user-1" }; + const data = { + answers: new Map([["q1", { correctOptionId: 2 }]]), + userId: "user-1", + }; await store.create("session-1", data, 60_000); await store.delete("session-1"); const result = await store.get("session-1"); @@ -29,7 +35,10 @@ describe("InMemoryGameSessionStore", () => { }); it("returns null after TTL expires", async () => { - const data = { answers: new Map([["q1", 2]]), userId: "user-1" }; + const data = { + answers: new Map([["q1", { correctOptionId: 2 }]]), + userId: "user-1", + }; await store.create("session-1", data, 1); await new Promise((resolve) => setTimeout(resolve, 10)); const result = await store.get("session-1"); @@ -37,7 +46,10 @@ describe("InMemoryGameSessionStore", () => { }); it("returns session data before TTL expires", async () => { - const data = { answers: new Map([["q1", 2]]), userId: "user-1" }; + const data = { + answers: new Map([["q1", { correctOptionId: 2 }]]), + userId: "user-1", + }; await store.create("session-1", data, 60_000); const result = await store.get("session-1"); expect(result).not.toBeNull(); @@ -45,15 +57,16 @@ describe("InMemoryGameSessionStore", () => { it("update persists modified session data", async () => { const data = { - answers: new Map([ - ["q1", 2], - ["q2", 1], - ]), + answers: new Map([["q1", { correctOptionId: 2 }]]), userId: "user-1", }; + await store.create("session-1", data, 60_000); - const updated = { answers: new Map([["q2", 1]]), userId: "user-1" }; + const updated = { + answers: new Map([["q2", { correctOptionId: 1 }]]), + userId: "user-1", + }; await store.update("session-1", updated); const result = await store.get("session-1"); diff --git a/apps/api/src/services/gameService.test.ts b/apps/api/src/services/gameService.test.ts index 98cad3b..76fa3a2 100644 --- a/apps/api/src/services/gameService.test.ts +++ b/apps/api/src/services/gameService.test.ts @@ -255,7 +255,7 @@ describe("evaluateAnswer", () => { ); }); - it("throws NotFoundError for a non-existent question", async () => { + it("throws ConflictError for a non-existent question", async () => { const session = await createGameSession(validRequest, store, "user-1"); const submission: AnswerSubmission = { @@ -264,12 +264,12 @@ describe("evaluateAnswer", () => { selectedOptionId: 0, }; - await expect(evaluateAnswer(submission, store, "user-1")).rejects.toThrow( - "Question not found", - ); + await expect( + evaluateAnswer(submission, store, "user-1"), + ).rejects.toMatchObject({ statusCode: 409 }); }); - it("throws NotFoundError when the same question is submitted twice", async () => { + it("throws ConflictError when the same question is submitted twice", async () => { const session = await createGameSession(validRequest, store, "user-1"); const question = session.questions[0]!; @@ -293,7 +293,7 @@ describe("evaluateAnswer", () => { store, "user-1", ), - ).rejects.toThrow("Question not found"); + ).rejects.toMatchObject({ statusCode: 409 }); }); it("deletes the session after the last question is answered", async () => { @@ -324,11 +324,11 @@ describe("evaluateAnswer", () => { ).rejects.toThrow("Game session not found"); }); - it("throws NotFoundError when getGameTerms returns no terms", async () => { + it("throws UnprocessableEntityError when getGameTerms returns no terms", async () => { mockGetGameTerms.mockResolvedValue([]); await expect( createGameSession(validRequest, store, "user-1"), - ).rejects.toThrow("No terms found"); + ).rejects.toMatchObject({ statusCode: 422 }); }); }); diff --git a/apps/api/src/services/gameService.ts b/apps/api/src/services/gameService.ts index 573f7a8..ad34c72 100644 --- a/apps/api/src/services/gameService.ts +++ b/apps/api/src/services/gameService.ts @@ -9,7 +9,11 @@ import type { AnswerResult, } from "@lila/shared"; import type { GameSessionStore } from "../gameSessionStore/index.js"; -import { NotFoundError } from "../errors/AppError.js"; +import { + NotFoundError, + ConflictError, + UnprocessableEntityError, +} from "../errors/AppError.js"; import { shuffleArray } from "../lib/utils.js"; export const createGameSession = async ( @@ -26,10 +30,10 @@ export const createGameSession = async ( ); if (terms.length === 0) { - throw new NotFoundError("No terms found for the given filters"); + throw new UnprocessableEntityError("No terms found for the given filters"); } - const answerKey = new Map(); + const answerKey = new Map(); const questions: GameQuestion[] = await Promise.all( terms.map(async (term) => { @@ -62,7 +66,7 @@ export const createGameSession = async ( })); const questionId = randomUUID(); - answerKey.set(questionId, correctOptionId); + answerKey.set(questionId, { correctOptionId }); return { questionId, @@ -90,10 +94,12 @@ export const evaluateAnswer = async ( throw new NotFoundError(`Game session not found: ${submission.sessionId}`); } - const correctOptionId = session.answers.get(submission.questionId); + const answer = session.answers.get(submission.questionId); - if (correctOptionId === undefined) { - throw new NotFoundError(`Question not found: ${submission.questionId}`); + if (answer === undefined) { + throw new ConflictError( + `Question already answered: ${submission.questionId}`, + ); } const updatedAnswers = new Map(session.answers); @@ -110,8 +116,8 @@ export const evaluateAnswer = async ( return { questionId: submission.questionId, - isCorrect: submission.selectedOptionId === correctOptionId, - correctOptionId, + isCorrect: submission.selectedOptionId === answer.correctOptionId, + correctOptionId: answer.correctOptionId, selectedOptionId: submission.selectedOptionId, }; }; From 98c59f33c598c4aaf2d9246b684f8ad90e5a45f9 Mon Sep 17 00:00:00 2001 From: lila Date: Tue, 28 Apr 2026 16:39:36 +0200 Subject: [PATCH 5/6] formatting + adding issues --- README.md | 8 ++++---- documentation/backlog.md | 6 ++++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index e212a55..66a5195 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ Live at [lilastudy.com](https://lilastudy.com). ## Repository Structure -``` +```tree lila/ ├── apps/ │ ├── api/ — Express backend @@ -50,7 +50,7 @@ lila/ Requests flow through a strict layered architecture: -``` +```text HTTP Request → Router → Controller → Service → Model → Database ``` @@ -71,7 +71,7 @@ Vocabulary data is sourced from WordNet and the Open Multilingual Wordnet (OMW). ## API -``` +```text POST /api/v1/game/start — start a quiz session (auth required) POST /api/v1/game/answer — submit an answer (auth required) GET /api/v1/health — health check (public) @@ -90,7 +90,7 @@ Rooms are created via REST, then managed over WebSockets. Messages are typed via ## Infrastructure -``` +```tree Internet → Caddy (HTTPS) ├── lilastudy.com → web (nginx, static files) ├── api.lilastudy.com → api (Express) diff --git a/documentation/backlog.md b/documentation/backlog.md index d0cb202..3d5e5da 100644 --- a/documentation/backlog.md +++ b/documentation/backlog.md @@ -23,6 +23,12 @@ Things that are actively in progress or should be picked up immediately. Mostly Clearly planned work, not yet started. No hard ordering — sequence based on what unblocks real users first. +- **Batch distractor queries to eliminate N+1** `[debt]` + createGameSession calls getDistractors once per term in parallel — 3 queries for 3 rounds, 10 for 10. Each query does ORDER BY RANDOM() which can't use an index and gets slower as the translations table grows. Fix: add a getDistractorsForTerms(termIds[], ...) function to @lila/db that batches all distractor fetches into a single query and returns results grouped by term. The service distributes the results per question. Prerequisite: none. Blocked by: nothing, but coordinate with any ongoing @lila/db changes. + +- **Atomic session creation** `[debt]` + createGameSession reads from Postgres (getGameTerms, getDistractors) then writes to the session store (in-memory/Valkey). A crash between the two leaves the terms consumed with no session created — the user gets an error and retries, no data is corrupted, but the work is wasted. A true transaction boundary isn't achievable across two different systems (Postgres + Valkey have no shared coordinator). Options when revisiting: store sessions in Postgres instead of Valkey (full transactionality, higher latency), or accept the current behaviour and add retry logic on the client. Revisit after Valkey is in production and actual failure rates are observable. + - **Guest / try-now flow** `[feature]` Allow users to play a quiz without signing in so they can see what the app offers before creating an account. Make auth middleware optional on game routes, add a "Try without account" button on the landing/login page. From fd9667c1fd3a57dcbec57cdf8dc248d18a8c3cf9 Mon Sep 17 00:00:00 2001 From: lila Date: Tue, 28 Apr 2026 17:26:01 +0200 Subject: [PATCH 6/6] updating documentation --- documentation/backlog.md | 9 +- documentation/roasts/gameService.md | 348 ++++++++++++++++++++++++++++ 2 files changed, 351 insertions(+), 6 deletions(-) diff --git a/documentation/backlog.md b/documentation/backlog.md index 3d5e5da..6a91e2d 100644 --- a/documentation/backlog.md +++ b/documentation/backlog.md @@ -8,15 +8,9 @@ Labels: `[feature]` `[infra]` `[security]` `[ux]` `[debt]` Things that are actively in progress or should be picked up immediately. Mostly operational risk and the remaining phase 7 hardening work. -- **Google OAuth publishing** `[infra]` - Only test users can currently log in via Google. Publish the OAuth consent screen so any Google user can sign in — requires branding verification in Google Cloud Console. - - **Hetzner domain migration check** `[infra]` Verify whether the lilastudy.com domain needs to be migrated following a Hetzner DNS change. Check Hetzner dashboard for any pending migration notice. -- **Conditionally register OAuth providers** `[debt]` - Better Auth logs warnings when social providers are registered without credentials (`Social provider google is missing clientId or clientSecret`). Instead of registering all providers unconditionally, only add a provider to the config when its credentials are present in the environment. Keeps local dev clean for contributors who don't have OAuth apps set up. - --- ## next @@ -69,6 +63,9 @@ Clearly planned work, not yet started. No hard ordering — sequence based on wh - **Tighten CSP to remove unsafe-inline** `[security]` Current script-src uses 'unsafe-inline' to accommodate framework-injected inline scripts (likely TanStack Router hydration). Tightening this would require nonce-based CSP, which needs server-rendered HTML or a Caddy layer that injects per-request nonces. Not urgent — pragmatic CSP with 'unsafe-inline' is mainstream for SPAs at this scale. Revisit if the app handles more sensitive data or grows a meaningful user base +- **Publish Google OAuth consent screen** `[infra]` + App is currently in testing mode, which caps OAuth sign-ins at 100 users. Before hitting that limit, publish the consent screen in Google Cloud Console. Basic scopes (email, profile, openid) require no Google review — just fill in branding fields (app name, logo, support email, privacy policy URL) and click publish. Trigger: do this before reaching 80 users. + --- ## later diff --git a/documentation/roasts/gameService.md b/documentation/roasts/gameService.md index e69de29..73283de 100644 --- a/documentation/roasts/gameService.md +++ b/documentation/roasts/gameService.md @@ -0,0 +1,348 @@ +# 🔥 GameService Roast: `apps/api/src/services/gameService.ts` + +> *"It works on my machine" is not a scalability strategy.* + +**Project:** lila — Vocabulary Trainer +**File Roasted:** `gameService.ts` +**Date:** $(date) +**Roaster:** Qwen3.6 + +--- + +## 📋 Executive Summary + +| Metric | Score | Notes | +| ------------- | -------- | ---------------------------------------------------- | +| Code Quality | 8/10 | Clean layering, good types, consistent style | +| Correctness | 6/10 | Race condition + N+1 query are critical | +| Test Coverage | 7/10 | Good happy-path tests, missing concurrency tests | +| Scalability | 5/10 | Will choke at ~100 concurrent users without fixes | +| **Overall** | **7/10** | Solid foundation, but fix the footguns before launch | + +--- + +## 🚨 Critical Issues (Fix Before Production) + +### 1. Race Condition: Lost Update in `evaluateAnswer` + +**Location:** `gameService.ts:45-58` + `InMemoryGameSessionStore.ts:update()` + +// Current flow (VULNERABLE): +const session = await store.get(submission.sessionId); // READ +const updatedAnswers = new Map(session.answers); // MODIFY (local copy) +updatedAnswers.delete(submission.questionId); +await store.update(submission.sessionId, { answers: updatedAnswers }); // WRITE + +The Attack: + + Client submits answer A and answer B for the same question (network retry, bug, or malice) + Both requests read the same session.answers Map (question still present) + Both delete the question from their local copy + Both write back → second write overwrites first + Result: One answer is silently lost, session state desyncs + +Why Tests Missed It: Vitest runs tests synchronously. Race conditions require deliberate concurrency testing. +Fix Options: + +// Option A: Add atomic operation to store interface +interface GameSessionStore { + deleteAnswer(sessionId: string, questionId: string): Promise; +} + +// Option B: Use Valkey Lua script for atomic read-modify-write +// Option C: Optimistic locking with version numbers + +Priority: 🔴 CRITICAL — Data integrity issue +2. N+1 Query: Database Performance Bomb +Location: gameService.ts:24-26 + termModel.ts:getDistractors() + +// For each of N terms, we call getDistractors(): +const questions: GameQuestion[] = await Promise.all( + terms.map(async (term) => { + const distractorTexts = await getDistractors(term.termId, ...); // 🚩 N queries! + }) +); + +Impact Analysis: +Rounds + +DB Queries + +At 50 concurrent users +3 + +1 + 3 = 4 + +200 queries/min +10 + +1 + 10 = 11 + +550 queries/min +20 + +1 + 20 = 21 + +1,050 queries/min +Each getDistractors() runs: + +SELECT text FROM terms +JOIN translations ON ... +WHERE pos = $1 AND difficulty = $2 AND term_id != $3 AND text != $4 +ORDER BY RANDOM() LIMIT 6 + +Fix: Batch Fetch Distractors + +// Fetch all distractors in ONE query +const allDistractors = await db + .select({ termId: terms.id, text: translations.text }) + .from(terms) + .innerJoin(translations, /* ... */) + .where(and( + eq(terms.pos, pos), + eq(translations.difficulty, difficulty), + inArray(terms.id, termIds), // Batch! + )) + .limit(DISTRACTOR_FETCH_COUNT * termIds.length); + +// Group by termId in JS, then slice to 3 unique distractors per term +const distractorsByTerm = groupByTermId(allDistractors); + +Priority: 🔴 CRITICAL — Performance/scalability issue + +3. Error Handling Inconsistency +Location: gameService.ts:33-36 + +if (uniqueDistractors.length < 3) { + throw new Error(`Not enough unique distractors for term: ${term.targetText}`); // 🚩 +} + +Problem: Raw Error bypasses your errorHandler middleware: + + No HTTP status mapping (defaults to 500) + No structured logging + Inconsistent API responses + +Fix: +import { UnprocessableEntityError } from "../errors/AppError.js"; + +if (uniqueDistractors.length < 3) { + logger.warn({ termId: term.termId, uniqueCount: uniqueDistractors.length }, + "insufficient_distractors"); + throw new UnprocessableEntityError( + `Not enough unique distractors for term: ${term.targetText}` + ); +} +Priority: 🟡 HIGH — Observability & UX issue +⚠️ High-Severity Smells +4. Code Duplication: Singleplayer vs Multiplayer +Compare: gameService.ts vs multiplayerGameService.ts +// gameService.ts +const optionTexts = [term.targetText, ...uniqueDistractors.slice(0, 3)]; +const shuffledTexts = shuffleArray(optionTexts); +const correctOptionId = shuffledTexts.indexOf(term.targetText); + +// multiplayerGameService.ts (lines 35-45) +const optionTexts = [correctAnswer.targetText, ...distractorTexts]; +const shuffledTexts = shuffle(optionTexts); // Different function, same logic! +const correctOptionId = shuffledTexts.indexOf(correctAnswer.targetText); + +Risks: + + Fix shuffle bias in one place, forget the other + Add new option type (e.g., etymology hint), update one service only + Harder to test core game logic in isolation + +Fix: Extract pure function to @lila/shared or new @lila/game-logic: + +// packages/shared/src/game-logic.ts +export const buildQuestionOptions = ( + correctAnswer: string, + distractors: string[], + optionCount: number = 4 +): { options: AnswerOption[]; correctOptionId: number } => { + const uniqueDistractors = [...new Set(distractors.filter(d => d !== correctAnswer))]; + const optionTexts = [correctAnswer, ...uniqueDistractors.slice(0, optionCount - 1)]; + const shuffled = shuffleSecure(optionTexts); + const correctOptionId = shuffled.indexOf(correctAnswer); + + return { + options: shuffled.map((text, idx) => ({ optionId: idx, text })), + correctOptionId + }; +}; + +Priority: 🟡 HIGH — Maintainability issue +5. Shuffle Bias: Math.random() Trap +Location: utils.ts:shuffleArray() + multiplayerGameService.ts:shuffle() + +export const shuffleArray = (array: T[]): T[] => { + for (let i = result.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); // 🚩 Modulo bias + non-crypto RNG + // ... + } +}; + +The Math: + + Math.random() has ~53 bits of entropy (fine for vocab) + Math.floor(rand * n) has modulo bias when n isn't a power of 2 + For n=4: bias is ~0.01% (tiny, but non-zero) + +When It Matters: + + Competitive leaderboards ("option 0 is correct 26% of the time") + Achievement systems based on answer patterns + Security-sensitive features (not applicable here, but principle matters) + +Fix (if needed): +import { randomBytes } from "crypto"; + +const shuffleSecure = (array: T[]): T[] => { + const result = [...array]; + for (let i = result.length - 1; i > 0; i--) { + // Use crypto.getRandomValues for better randomness + const rand = randomBytes(4).readUInt32LE(0); + const j = rand % (i + 1); + [result[i], result[j]] = [result[j], result[i]]; + } + return result; +}; + +Priority: 🟢 LOW — Document tradeoff and move on for now + +6. Test Coverage Gaps +File: gameService.test.ts +✅ Well Tested: + + Happy path: session creation, answer evaluation + Edge cases: duplicate distractors, empty results, invalid inputs + Error propagation from DB layer + +❌ Missing Tests: + +// 1. Concurrency test (race condition) +it("rejects duplicate answers for same question under concurrent load", async () => { + const session = await createGameSession(validRequest, store, "user-1"); + const question = session.questions[0]!; + + // Submit two answers simultaneously + const [result1, result2] = await Promise.allSettled([ + evaluateAnswer({ sessionId, questionId, selectedOptionId: 0 }, store, "user-1"), + evaluateAnswer({ sessionId, questionId, selectedOptionId: 1 }, store, "user-1"), + ]); + + // Exactly one should succeed, one should throw ConflictError + expect([result1, result2].filter(r => r.status === "fulfilled")).toHaveLength(1); +}); + +// 2. TTL expiration test +it("deletes session after TTL expires", async () => { + vi.useFakeTimers(); + const session = await createGameSession(validRequest, store, "user-1"); + + vi.advanceTimersByTime(31 * 60 * 1000); // 31 minutes + + await expect(store.get(session.sessionId)).resolves.toBeNull(); +}); + +// 3. Distractor fallback strategy test +it("uses fallback when <3 unique distractors available", async () => { + mockGetDistractors.mockResolvedValue(["same", "same", "same", "same"]); + // Should either: (a) fetch from broader pool, or (b) reduce rounds gracefully +}); + +Priority: 🟡 HIGH — Prevents regression on critical fixes +🧼 Code Quality Nitpicks +7. Magic Numbers + +// gameService.ts:52 +await store.create(sessionId, {...}, 30 * 60 * 1000); // What is this? + +// termModel.ts:65 +.limit(count); // count=6, but why? + +// shared/schemas/game.ts:15 +optionId: z.number().int().min(0).max(3), // Why 4 options? + +Fix: Centralize in @lila/shared/constants.ts: + +export const GAME_SESSION_TTL_MS = 30 * 60 * 1000; +export const DISTRACTOR_FETCH_COUNT = 6; +export const GAME_OPTION_COUNT = 4; +export const MIN_UNIQUE_DISTRACTORS = 3; + +8. Mutable Reference Leakage +Location: InMemoryGameSessionStore.ts:get() + +get(sessionId: string): Promise { + return Promise.resolve(entry.data); // 🚩 Returns mutable reference to internal state +} + +Risk: Any code that does session.answers.delete(...) mutates the store's internal Map directly. +Fix: + +// Option A: Deep clone (simple, works for this data shape) +return Promise.resolve(structuredClone(entry.data)); + +// Option B: Return readonly view (TypeScript-only protection) +return Promise.resolve(entry.data as Readonly); + +// Option C: Use immutable data structures (overkill for now) + +9. Zero Observability +Problem: No logging, no metrics. You're flying blind in production. +Minimal Fix (5 minutes): + + + +// apps/api/src/lib/logger.ts +import pino from "pino"; +export const logger = pino({ + level: process.env.LOG_LEVEL || "info", + transport: process.env.NODE_ENV === "production" + ? { target: "pino-pretty" } + : undefined +}); + +// In gameService.ts: +import { logger } from "../lib/logger.js"; + +logger.info( + { userId, sourceLang, targetLang, termCount: terms.length }, + "game_session_created" +); + +logger.debug( + { sessionId, questionId, isCorrect, responseTimeMs }, + "answer_evaluated" +); + +Bonus: Export a Prometheus histogram for game_service_duration_seconds. + +10. ORDER BY RANDOM() Time Bomb +Location: termModel.ts:getGameTerms() + getDistractors() + +.orderBy(sql`RANDOM()`) // 🚩 Fine for 10k rows, slow for 1M + +The Comment Admits It: + +// TODO(post-mvp): ORDER BY RANDOM() sorts the entire filtered result set... + +Reality Check: "Post-MVP" never comes without a ticket. +Fix Options: + +-- Option A: Pre-computed random_seed column (updated nightly) +WHERE ... AND random_seed >= random() +ORDER BY random_seed +LIMIT $1 + +-- Option B: TABLESAMPLE for approximate sampling (Postgres 9.5+) +FROM terms TABLESAMPLE SYSTEM(10) +WHERE ... +LIMIT $1 + +-- Option C: Random offset (simple, but still scans) +OFFSET floor(random() * (SELECT count(*) FROM terms WHERE ...)) + +Action: Add a ticket to documentation/tickets/t00009.md now. \ No newline at end of file