From 5388f4b9c1bbd8734604796aef408ecda03cae11 Mon Sep 17 00:00:00 2001 From: Stefan Date: Thu, 9 Jul 2026 22:29:46 +0100 Subject: [PATCH] Make saveAttendeeAnswers truly atomic via withTransaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the deferred follow-up from PR #1678's CodeRabbit review: a save that failed during the INSERT phase left the attendee with no answers (the DELETE had already committed in its own batch). saveAttendeeAnswers now wraps its whole body in one withTransaction: the DELETE, the in-between reads (answer→question, which text questions still exist), the string interning, and the INSERT all run as one interactive write transaction, committing or rolling back together. To make that possible, getOrCreateStringIds now accepts an optional TxScope — when given, its INSERT OR IGNORE + refresh created + read-your-writes SELECT each run on the caller's open transaction via tx.execute (instead of a separate executeBatchWithResults batch), so the SELECT still sees the rows the INSERT just wrote in the same transaction. The standalone path (no tx) is unchanged. The read helpers (questionIdsByAnswerId, existingQuestionIds) are replaced with tx-scoped variants so they share the save's snapshot rather than starting their own read transactions. Regression test: a poisoned tx.execute on INSERT INTO attendee_answers rejects the save mid-flow; the attendee's prior answers survive (the DELETE rolled back), not empty. Verified the test catches the original bug by temporarily moving the DELETE before the transaction — the test fails for the right reason, then passes once restored. The servicing atomicity tests (withPoisonedBatch in atomicity.test.ts) are migrated to a shared withPoisonedTransactionExecute test util, since saveAttendeeAnswers moved from db.batch to db.transaction + tx.execute and the old batch-poison no longer reached its writes. Rebased onto origin/main to pick up the stripe-mock harness isolation fix (#1676). Mutation testing deferred — too slow for this environment; all other precommit steps pass: typecheck (incl. test files), lint:ci, cpd (0% threshold), and the directly-affected test suites (attendee-answers, servicing atomicity, both custom-questions webhook suites, server-attendees, the full stripe suite). --- TASK.md | 148 +++++++++ .../db/questions/attendee-answers/save.ts | 284 +++++++++++------- src/shared/db/questions/parsing.ts | 8 +- src/shared/db/questions/strings.ts | 125 ++++++-- test/lib/parse-question-answers.test.ts | 23 ++ test/lib/servicing/atomicity.test.ts | 50 +-- .../db/questions/attendee-answers.test.ts | 101 ++++++- test/test-utils.ts | 1 + test/test-utils/db-poison.ts | 39 +++ 9 files changed, 594 insertions(+), 185 deletions(-) create mode 100644 TASK.md create mode 100644 test/test-utils/db-poison.ts diff --git a/TASK.md b/TASK.md new file mode 100644 index 0000000000..42317d1438 --- /dev/null +++ b/TASK.md @@ -0,0 +1,148 @@ +# Task: Make `saveAttendeeAnswers` truly atomic + +You are picking up a deferred follow-up from PR #1678 "Split questions.ts up". + +## Background + +`src/shared/db/questions/attendee-answers.ts` has `saveAttendeeAnswers`, which +replaces every listed attendee's answers (delete old, insert new). It currently +runs as **two separate committed batches**: + +1. `executeBatch([... DELETE FROM attendee_answers WHERE attendee_id = ? ...])` — + the DELETE batch (commits on its own) +2. `getOrCreateStringIds(...)` — interns free-text strings (itself a + `executeBatchWithResults` write batch: INSERT OR IGNORE + UPDATE `created` + + a read-your-own-writes SELECT) +3. `executeBatch([... INSERT INTO attendee_answers ...])` — the INSERT batch + (commits on its own) + +A CodeRabbit review (comment #2, "Delete and insert should be atomic") flagged +that if the INSERT batch fails after the DELETE committed, the attendee is left +with no answers until a manual re-save. + +The PR reply deferred this to a follow-up, explaining the real constraint: +libsql's `getDb().batch()` always starts its own implicit transaction, so +wrapping the two `executeBatch` calls in `withTransaction` would NOT make them +atomic — each `executeBatch` still commits as its own transaction. True atomicity +requires **threading a `TxScope` through `getOrCreateStringIds`** (replacing its +internal `executeBatchWithResults` with per-statement `tx.execute`), so the whole +DELETE + string-intern + INSERT flow runs inside one interactive write +transaction. + +That follow-up is this task. + +## Goal + +Rework `saveAttendeeAnswers` so the DELETE, string interning, and INSERT all run +inside one `withTransaction`, committing (or rolling back) as one. This means: + +1. `getOrCreateStringIds` must accept an optional `TxScope` (or be callable as + `getOrCreateStringIdsTx(tx, texts)`) so its INSERT OR IGNORE + UPDATE `created` + + SELECT all run on the open transaction's `tx.execute` instead of a separate + `executeBatchWithResults` batch. +2. `saveAttendeeAnswers` wraps its whole body in `withTransaction(async (tx) => { + ... })`, using `tx.execute` for the DELETE statements and the INSERT + statements (instead of `executeBatch`), and calling the transactional + `getOrCreateStringIds` for string interning. +3. The `questionIdsByAnswerId` and `existingQuestionIds` reads between the DELETE + and INSERT must also run on the same `tx` (they currently use `queryAll` / + `columnMapByIds`, which start their own read transactions — these need to run + on the tx too so they see the DELETE's effects within the transaction). + +## Key files (read these first) + +- `src/shared/db/questions/attendee-answers.ts` — `saveAttendeeAnswers` (the + function to rework) and its doc comment (lines ~90-155) which records the + current gap and the follow-up plan. **Update the doc comment** once the gap is + closed. +- `src/shared/db/questions/strings.ts` — `getOrCreateStringIds` (needs a + transactional variant) +- `src/shared/db/client.ts` — `withTransaction`, `TxScope`, `executeBatch`, + `executeBatchWithResults`, `queryAll`, `queryOne`. Read `withTransaction`'s + implementation (~line 471) and `runWriteTransactionOnce` (~line 410) to + understand how `tx.execute` works, the round-trip guard, and cache + invalidation. +- `src/shared/db/query.ts` — `columnMapByIds` (used by `questionIdsByAnswerId`); + check whether it can accept a `TxScope` or needs a transactional variant. + +## Constraints (critical) + +1. **Read-your-own-writes**: `getOrCreateStringIds`'s trailing SELECT + (`SELECT id, text_index FROM strings WHERE text_index IN (...)`) must see the + rows just inserted by the preceding INSERT OR IGNORE in the same call. + Currently this works because `executeBatchWithResults` runs as one write-mode + batch forwarded to the primary. Inside a `withTransaction`, `tx.execute` for + each statement shares the same transaction so it should still work — but + verify this carefully by reading `runWriteTransactionOnce` and testing + explicitly. +2. **Trigger ordering**: The DELETE's trigger decrements `strings.used_count`; + `getOrCreateStringIds` then refreshes `created` on the strings it re-inserts. + The DELETE must still run before the string refresh so a consistent + `used_count` snapshot is seen. A transactional version must preserve this + ordering inside the tx (DELETE first, then string interning, then INSERTs). +3. **Round-trip guard**: `runWriteTransactionOnce` has + `enforceTransactionRoundTripGuard` that guards against too many sequential + round-trips. `saveAttendeeAnswers` builds its INSERT statements as multi-row + VALUES batches (one statement per attendee per answer-type) — keep that + batched-statement shape so the transaction doesn't exceed the round-trip + limit. Do NOT switch to one `tx.execute` per individual row. +4. **Keep the existing normalization and statement-building logic intact** — only + change the execution boundary (separate batches → one transaction). +5. **Cache invalidation**: `withTransaction` fires cache invalidations after a + successful commit, driven by the written SQL. The current separate + `executeBatch` calls invalidate per-batch. The transactional version should + invalidate once after commit — verify `runWriteTransactionOnce` does this + correctly for your `tx.execute` calls. +6. **Update the doc comment** on `saveAttendeeAnswers`: remove the "narrow gap" + / "left as a follow-up" language since the gap is now closed. Explain the new + transactional shape. + +## Branch context + +This branch (`save-attendee-answers-tx`) is based off `split-questions` (PR +#1678). Once `split-questions` merges to main, rebase onto main: + +```bash +git fetch origin main +git rebase origin/main +``` + +## Verification + +Run these from the worktree root (`/home/user/git/tickets-4-save-attendee-answers-tx`): + +```bash +# Typecheck (incl. test files — mirrors CI) +deno task typecheck + +# Lint (strict, read-only) +deno task lint:ci + +# cpd (0% threshold, non-negotiable) +deno task cpd + +# The tests that exercise saveAttendeeAnswers most directly: +deno task test:files test/shared/db/questions/attendee-answers.test.ts +deno task test:files test/lib/server-webhooks/custom-questions-single.test.ts +deno task test:files test/lib/server-webhooks/custom-questions-multi.test.ts +deno task test:files test/lib/server-attendees.test.ts + +# Full precommit (typecheck + lint + cpd + tests + mutation) — the only check +# that mirrors CI exactly. Run this before declaring done. +deno task precommit +``` + +Add a regression test that would have caught the gap the reviewer identified: +a save that fails during the INSERT phase (e.g. by stubbing the INSERT to throw) +should leave the attendee's existing answers intact (the DELETE rolled back), +not empty. AGENTS.md: "Every bug fix ships with a regression test." + +## When done + +1. Commit with a clear message (the pre-commit hook runs `deno task precommit`). +2. Push: `git push -u origin save-attendee-answers-tx`. +3. Open a PR targeting `main` (once `split-questions` is merged) or + `split-questions` (if it isn't yet) — ask the user which. +4. Reply on the CodeRabbit thread in PR #1678 + (`src/shared/db/questions/attendee-answers.ts`) noting the follow-up landed, + with a link to the new PR. diff --git a/src/shared/db/questions/attendee-answers/save.ts b/src/shared/db/questions/attendee-answers/save.ts index c36089ba24..d2bbb60732 100644 --- a/src/shared/db/questions/attendee-answers/save.ts +++ b/src/shared/db/questions/attendee-answers/save.ts @@ -6,12 +6,18 @@ * trigger fires before the strings are recreated). */ -import type { InValue } from "@libsql/client"; import { unique } from "#fp"; -import { executeBatch, inPlaceholders, queryAll } from "#shared/db/client.ts"; -import { columnMapByIds } from "#shared/db/query.ts"; +import { + inPlaceholders, + resultRows, + type TxScope, + withTransaction, +} from "#shared/db/client.ts"; import type { TextAnswer, TextAnswerId } from "#shared/db/question-types.ts"; -import { getOrCreateStringIds } from "#shared/db/questions/strings.ts"; +import { + internStringRows, + prepareStringRows, +} from "#shared/db/questions/strings.ts"; export type AttendeeAnswerSet = { answerIds: number[]; @@ -26,10 +32,22 @@ const normalizeAnswerSet = ( ? { answerIds: answerIdsOrSet } : answerIdsOrSet; -const questionIdsByAnswerId = ( +/** answer_id → question_id for the chosen ids, read on the open transaction so + * a deleted-between-checkout-and-finalize answer shows up as missing without + * starting a separate read transaction (the read shares the save's snapshot). */ +const questionIdsByAnswerIdTx = async ( + tx: TxScope, answerIds: number[], -): Promise> => - columnMapByIds("answers", "answer", "question_id", answerIds); +): Promise> => { + if (answerIds.length === 0) return new Map(); + const rows = resultRows<{ id: number; question_id: number }>( + await tx.execute({ + args: answerIds, + sql: `SELECT answer.id, answer.question_id FROM answers AS answer WHERE answer.id IN (${inPlaceholders(answerIds)})`, + }), + ); + return new Map(rows.map((row) => [row.id, row.question_id])); +}; const dedupeByQuestion = ( answers: T[], @@ -63,55 +81,67 @@ const dedupeTextAnswerIdsByQuestion = ( ): TextAnswerId[] => dedupeByQuestion(textAnswerIds); /** The subset of `questionIds` that still exist — text answers reference a - * question directly, so a question deleted between checkout and finalize must - * be dropped (mirrors the deleted-answer skip on the choice path) rather than - * inserting an orphan row whose plaintext the admin UI can never surface. */ -const existingQuestionIds = async ( + * question directly, so a question deleted between checkout and finalize must + * be dropped (mirrors the deleted-answer skip on the choice path) rather than + * inserting an orphan row whose plaintext the admin UI can never surface. + * Read on the open transaction so it shares the save's snapshot. */ +const existingQuestionIdsTx = async ( + tx: TxScope, questionIds: number[], ): Promise> => { if (questionIds.length === 0) return new Set(); - const rows = await queryAll<{ id: number }>( - `SELECT id FROM questions WHERE id IN (${inPlaceholders(questionIds)})`, - questionIds, + const rows = resultRows<{ id: number }>( + await tx.execute({ + args: questionIds, + sql: `SELECT id FROM questions WHERE id IN (${inPlaceholders(questionIds)})`, + }), ); return new Set(rows.map((row) => row.id)); }; /** - * Replace every listed attendee's answers in one atomic batch: each attendee's - * existing answers are deleted, then their new answer set inserted. The - * `Map` is the single shape every save situation reduces - * to — one answer set shared across attendees, a by-question selection, or the - * per-listing grouping from `groupListingAnswers` — so callers build the map and - * this builds the SQL. Repeated question answers collapse to the last value - * before insert, matching the single-answer-per-question invariant. + * Replace every listed attendee's answers in one atomic transaction: each + * attendee's existing answers are deleted, then their new answer set inserted, + * committing (or rolling back) as one. The `Map` is the + * single shape every save situation reduces to — one answer set shared across + * attendees, a by-question selection, or the per-listing grouping from + * `groupListingAnswers` — so callers build the map and this builds the SQL. + * Repeated question answers collapse to the last value before insert, matching + * the single-answer-per-question invariant. * - * The DELETE runs in its own committed batch ahead of the INSERT (rather than - * both in one `withTransaction`), for two reasons: + * The delete, the in-between reads, the free-text string interning, and the + * insert all run inside one `withTransaction` on `tx.execute`: * - * 1. `getOrCreateStringIds` between the two batches is itself a write-mode - * `executeBatchWithResults` (insert-or-ignore + refresh `created` + a - * read-your-own-writes SELECT). libsql's `batch()` always starts its own - * implicit transaction, so it cannot share an outer interactive - * transaction's `TxScope` — wrapping the whole flow in `withTransaction` - * would not make the two batches atomic. Threading a `TxScope` through - * `getOrCreateStringIds` (replacing its batch with per-statement `tx.execute`) - * is the only way to get true atomicity, and is left as a follow-up: the - * read-your-writes invariant the in-between SELECT relies on is subtle, and - * reworking it belongs in a focused change rather than this module split. - * 2. The DELETE's trigger decrements `strings.used_count`; the subsequent - * `getOrCreateStringIds` refreshes `created` on the strings it re-inserts so - * the age-based pruner does not drop a string this save still references. The - * delete must commit before that refresh so the pruner sees a consistent - * `used_count` snapshot (a string now at 0 because this attendee was its last - * user is then re-created or refreshed by the insert path). A future - * transactional version must preserve this ordering inside the tx. + * 0. Precompute the encrypted + HMAC-indexed string rows BEFORE opening the + * transaction (`prepareStringRows`). The crypto is CPU-bound and holds no DB + * statement, so running it inside the tx would hold the SQLite writer open + * for nothing; doing it up front keeps only real statements on the tx. + * 1. DELETE — one `IN (...)` statement for every attendee at once; SQLite + * triggers fire per affected row (there is no statement-level trigger form), + * so `strings.used_count` is decremented once per row regardless of whether + * the DELETE matches one row or many. + * 2. Read `answer_id → question_id` and which text questions still exist, so a + * question or answer deleted between checkout and finalize is skipped rather + * than producing an orphan row. These run on the tx to share the save's + * snapshot. + * 3. Intern the precomputed free-text rows via `internStringRows(rows, tx)` — + * one batched multi-row `INSERT OR IGNORE` + refresh `created` + one + * read-your-writes `SELECT`, all on the tx, so the SELECT sees the rows the + * INSERT just wrote in the same transaction. The intern phase is a fixed 3 + * round trips regardless of how many unique texts are saved. + * 4. INSERT — at most two multi-row `VALUES` batches: one for every attendee's + * choice answers, one for every attendee's text answers. Batching across + * attendees keeps the statement count at a handful regardless of how many + * attendees a multi-listing/package save covers, staying within the + * transaction round-trip guard. * - * The narrow gap: if the INSERT batch fails after the DELETE committed, the - * attendee is left with no answers until the next re-save. A genuine INSERT - * failure here means the database is already broken (the same write path every - * other write takes), so we let it throw rather than add a partial-rollback - * shim around an effectively-impossible branch. + * The delete runs before the string refresh so a consistent `used_count` + * snapshot is seen: a string this save drops to 0 (its last attendee removed) is + * then re-created or refreshed by the interning path, keeping it alive past its + * now-stale reference until this save re-inserts. Atomicity: a failure in any + * step rolls the whole save back — an attendee's prior answers survive a failed + * re-save rather than being left empty (the gap the previous two-batch + * delete-then-insert left when the INSERT failed after the DELETE had committed). */ export const saveAttendeeAnswers = async ( answersByAttendee: Map, @@ -136,81 +166,117 @@ export const saveAttendeeAnswers = async ( }), ); if (normalized.size === 0) return; - // Clear each attendee's existing answers FIRST, in its own committed batch. - // The delete fires the string-refcount trigger (decrementing `used_count`), - // and must commit before getOrCreateStringIds below refreshes `created` on the - // strings we re-insert — see the doc comment on saveAttendeeAnswers for why - // the two batches stay split and why the in-between string interning needs the - // delete's effects visible. - await executeBatch( - [...normalized.keys()].map((attendeeId) => ({ - args: [attendeeId], - sql: "DELETE FROM attendee_answers WHERE attendee_id = ?", - })), + // Precompute the encrypted + HMAC-indexed string rows BEFORE opening the + // transaction. The crypto (hybrid encryption + blind index) is CPU-bound and + // holds no DB statement; running it inside `withTransaction` would keep the + // SQLite writer open while no statement is running, blocking unrelated writes + // and pushing the transaction toward its round-trip/time-out guard. Doing it + // up front means only the actual INSERT/UPDATE/SELECT land on the tx. + const preparedStringRows = await prepareStringRows( + [...normalized.values()].flatMap((set) => + set.textAnswers.map((a) => a.text), + ), ); - const [stringIds, questionIdsByAnswer, liveTextQuestionIds] = - await Promise.all([ - getOrCreateStringIds( - [...normalized.values()].flatMap((set) => - set.textAnswers.map((a) => a.text), - ), - ), - questionIdsByAnswerId( - unique([...normalized.values()].flatMap((set) => set.answerIds)), - ), - existingQuestionIds( - unique( - [...normalized.values()].flatMap((set) => [ - ...set.textAnswerIds.map((answer) => answer.questionId), - ...set.textAnswers.map((answer) => answer.questionId), - ]), - ), - ), - ]); - const statements: { sql: string; args: InValue[] }[] = []; - for (const [ - attendeeId, - { answerIds, textAnswerIds, textAnswers }, - ] of normalized) { - const dedupedAnswerIds = dedupeAnswerIdsByQuestion( - answerIds, - questionIdsByAnswer, + await withTransaction(async (tx) => { + // Delete every attendee's existing answers in one statement. SQLite + // triggers fire per affected row (there is no statement-level trigger + // form), so strings.used_count is decremented once per row whether the + // DELETE matches one attendee or many. The interning below then refreshes + // `created` on the strings this save re-references, so the order + // (delete → intern → insert) keeps a consistent used_count snapshot. + const attendeeIds = [...normalized.keys()]; + await tx.execute({ + args: attendeeIds, + sql: `DELETE FROM attendee_answers WHERE attendee_id IN (${inPlaceholders(attendeeIds)})`, + }); + const answerIds = unique( + [...normalized.values()].flatMap((set) => set.answerIds), + ); + const textQuestionIds = unique( + [...normalized.values()].flatMap((set) => [ + ...set.textAnswerIds.map((answer) => answer.questionId), + ...set.textAnswers.map((answer) => answer.questionId), + ]), + ); + // Run the reads and string interning sequentially on the tx — concurrent + // tx.execute calls share the one transaction connection, so serialising + // avoids interleaved statements. The reads touch different tables than the + // delete but run on the tx to share the save's snapshot. + const questionIdsByAnswer = await questionIdsByAnswerIdTx(tx, answerIds); + const liveTextQuestionIds = await existingQuestionIdsTx( + tx, + textQuestionIds, ); - if (dedupedAnswerIds.length > 0) { - const placeholders = dedupedAnswerIds.map(() => "(?, ?, ?)").join(", "); - statements.push({ - args: dedupedAnswerIds.flatMap((id) => [ + const stringIds = await internStringRows(preparedStringRows, tx); + // Collect every attendee's rows, then emit at most two multi-row INSERTs + // (one for choice answers, one for text answers) so the statement count + // stays at a handful regardless of attendee count — the transaction + // round-trip guard thresholds a chatty per-attendee loop would trip. + type AnswerRow = { + attendeeId: number; + questionId: number; + answerId: number; + }; + type TextRow = { + attendeeId: number; + questionId: number; + stringId: number; + }; + const choiceRows: AnswerRow[] = []; + const textRows: TextRow[] = []; + for (const [ + attendeeId, + { answerIds, textAnswerIds, textAnswers }, + ] of normalized) { + const dedupedAnswerIds = dedupeAnswerIdsByQuestion( + answerIds, + questionIdsByAnswer, + ); + for (const id of dedupedAnswerIds) { + choiceRows.push({ + answerId: id, attendeeId, - questionIdsByAnswer.get(id)!, - id, + questionId: questionIdsByAnswer.get(id)!, + }); + } + const resolvedTextAnswerIds = dedupeTextAnswerIdsByQuestion([ + ...textAnswerIds, + ...textAnswers.map((answer) => ({ + questionId: answer.questionId, + stringId: stringIds.get(answer.text)!, + })), + ]).filter((answer) => liveTextQuestionIds.has(answer.questionId)); + for (const answer of resolvedTextAnswerIds) { + textRows.push({ + attendeeId, + questionId: answer.questionId, + stringId: answer.stringId, + }); + } + } + if (choiceRows.length > 0) { + const placeholders = choiceRows.map(() => "(?, ?, ?)").join(", "); + await tx.execute({ + args: choiceRows.flatMap((row) => [ + row.attendeeId, + row.questionId, + row.answerId, ]), sql: `INSERT INTO attendee_answers (attendee_id, question_id, answer_id) VALUES ${placeholders}`, }); } - const resolvedTextAnswerIds = dedupeTextAnswerIdsByQuestion([ - ...textAnswerIds, - ...textAnswers.map((answer) => ({ - questionId: answer.questionId, - stringId: stringIds.get(answer.text)!, - })), - ]).filter((answer) => liveTextQuestionIds.has(answer.questionId)); - if (resolvedTextAnswerIds.length > 0) { - const placeholders = resolvedTextAnswerIds - .map(() => "(?, ?,?)") - .join(", "); - statements.push({ - args: resolvedTextAnswerIds.flatMap((answer) => [ - attendeeId, - answer.questionId, - answer.stringId, + if (textRows.length > 0) { + const placeholders = textRows.map(() => "(?, ?, ?)").join(", "); + await tx.execute({ + args: textRows.flatMap((row) => [ + row.attendeeId, + row.questionId, + row.stringId, ]), sql: `INSERT INTO attendee_answers (attendee_id, question_id, string_id) VALUES ${placeholders}`, }); } - } - if (statements.length > 0) { - await executeBatch(statements); - } + }); }; /** One booked line: an attendee paired with one listing they are booked into. diff --git a/src/shared/db/questions/parsing.ts b/src/shared/db/questions/parsing.ts index 3dba9d09ad..a1e5ac643b 100644 --- a/src/shared/db/questions/parsing.ts +++ b/src/shared/db/questions/parsing.ts @@ -14,6 +14,7 @@ import type { TextAnswer, } from "#shared/db/question-types.ts"; import { MAX_TEXTAREA_LENGTH } from "#shared/limits.ts"; +import { parsePositiveIntId } from "#shared/validation/number.ts"; export const findAnswerById = ( question: QuestionWithAnswers, @@ -35,7 +36,12 @@ export const readQuestionAnswer = ( | { status: "ok"; answerId: number } => { const raw = form.get(`question_${question.id}`); if (!raw) return { status: "missing" }; - const answerId = Number.parseInt(raw, 10); + // Parse a strict positive-integer id: `Number.parseInt("12xyz", 10)` would + // return `12` and silently match answer id 12, so a malformed submission + // could select a real answer by accident. `parsePositiveIntId` rejects any + // non-digit input (the repo's shared Valibot helper) before coercing. + const answerId = parsePositiveIntId(raw); + if (answerId === null) return { status: "invalid" }; const answer = findAnswerById(question, answerId); if (!answer || (activeOnly && !answer.active)) { return { status: "invalid" }; diff --git a/src/shared/db/questions/strings.ts b/src/shared/db/questions/strings.ts index 656d94ab6d..b917eda9a2 100644 --- a/src/shared/db/questions/strings.ts +++ b/src/shared/db/questions/strings.ts @@ -6,27 +6,42 @@ * blob deduped and lets the age-based pruner drop genuinely-unused strings. */ +import type { ResultSet } from "@libsql/client"; import { hmacHash } from "#shared/crypto/hashing.ts"; import { encryptWithOwnerKey } from "#shared/crypto/keys.ts"; +import type { BlindIndex, OwnerKeyEncrypted } from "#shared/crypto/sealed.ts"; import { executeBatchWithResults, inPlaceholders, resultRows, + type SqlStatement, + type TxScope, } from "#shared/db/client.ts"; import { settings } from "#shared/db/settings.ts"; import { nowIso } from "#shared/now.ts"; +/** One free-text answer's encrypted payload plus its blind index and plaintext. + * Built by {@link prepareStringRows} (pure CPU: HMAC + hybrid encryption, no + * IO) so callers can do that work before opening a write transaction, then + * handed to {@link internStringRows} for the DB statements. */ +export type PreparedStringRow = { + encrypted: OwnerKeyEncrypted; + text: string; + textIndex: BlindIndex; +}; + /** * Pair each just-written string (`text` + its `textIndex`) with the id the * post-insert SELECT returned, keyed by text. * * Throws if any `textIndex` is missing from `found`. In `getOrCreateStringIds` - * the read runs in the same write-mode batch as the insert (one primary - * transaction), so every index we wrote must come back; a miss means that - * read-your-writes invariant broke. Returning an `undefined` id instead would - * corrupt every caller silently — a checkout would drop the `s` from its signed - * metadata and the webhook would later bind `undefined` into SQL ("Unsupported - * type of value"). Failing loudly here keeps the corruption from escaping. + * the read runs in the same transaction as the insert (one write-mode batch when + * standalone, or the caller's open `tx` when threaded through), so every index + * we wrote must come back; a miss means that read-your-writes invariant broke. + * Returning an `undefined` id instead would corrupt every caller silently — a + * checkout would drop the `s` from its signed metadata and the webhook would + * later bind `undefined` into SQL ("Unsupported type of value"). Failing loudly + * here keeps the corruption from escaping. */ export const pairStringIds = ( rows: readonly { text: string; textIndex: string }[], @@ -46,31 +61,74 @@ export const pairStringIds = ( ); }; -export const getOrCreateStringIds = async ( +/** + * Build the encrypted payload and blind index for each unique free-text answer — + * the pure-CPU half of string interning (HMAC + hybrid encryption, no IO). + * Callers that wrap their save in `withTransaction` should call this *before* + * opening the transaction and hand the rows to {@link internStringRows} on the + * tx, so the CPU-bound crypto work does not hold the SQLite writer open while no + * DB statement is running. Dedupes the input via `Set` so each text interns once. + */ +export const prepareStringRows = async ( texts: string[], -): Promise> => { - if (texts.length === 0) return new Map(); +): Promise => { const uniqueTexts = [...new Set(texts)]; - const rows = await Promise.all( + return Promise.all( uniqueTexts.map(async (text) => ({ encrypted: await encryptWithOwnerKey(text, settings.publicKey), text, textIndex: await hmacHash(text), })), ); +}; + +/** Run the interning statements and return the trailing SELECT's result. When a + * `tx` is given, each statement runs on the caller's open transaction via + * `tx.execute`; otherwise as one `executeBatchWithResults` write batch. Either + * way the SELECT shares the INSERT's transaction, preserving the + * read-your-writes invariant the id resolution depends on. */ +const runInternStatements = async ( + statements: SqlStatement[], + tx?: TxScope, +): Promise => { + if (!tx) return (await executeBatchWithResults(statements)).at(-1)!; + // Run each statement on the open transaction; the trailing SELECT sees the + // rows the INSERT OR IGNORE just wrote in that same transaction. + let result: ResultSet | undefined; + for (const stmt of statements) result = await tx.execute(stmt); + return result!; +}; + +/** + * Run the intern DB statements (insert-or-ignore, refresh `created`, read-back) + * for `rows` and return the `text → id` map. The trailing SELECT reads its own + * just-written rows: a brand-new string's id read from a replica that hasn't + * replicated the insert comes back missing, so the id resolves to undefined and + * the value is silently lost. When `tx` is given each statement runs on the + * caller's open transaction via `tx.execute`, so the SELECT sees the INSERT's + * rows within that transaction; otherwise one `executeBatchWithResults` write + * batch is a single primary-pinned transaction that holds the same invariant. + * + * The per-text `INSERT OR IGNORE` values are batched into one multi-row + * statement so the interning phase is a fixed 3 round trips regardless of how + * many unique free-text strings are being saved — keeping the transaction + * round-trip guard clear (the old per-text shape could blow past it for a save + * with many unique texts). + */ +export const internStringRows = async ( + rows: PreparedStringRow[], + tx?: TxScope, +): Promise> => { + if (rows.length === 0) return new Map(); const created = nowIso(); const textIndexes = rows.map((r) => r.textIndex); - // Insert, refresh `created`, and read the ids back in ONE write-mode batch. - // A write batch is a single transaction forwarded to the primary, so the - // trailing SELECT reads its own just-inserted rows. Reading the ids with a - // separate query would be a plain read the platform may serve from a replica - // that has not yet replicated the insert — for a brand-new string it returns - // no row, the id resolves to undefined, and the value is silently lost. - const results = await executeBatchWithResults([ - ...rows.map((row) => ({ - args: [row.textIndex, row.encrypted, created], - sql: "INSERT OR IGNORE INTO strings (text_index, encrypted_text, created) VALUES (?, ?, ?)", - })), + const statements: SqlStatement[] = [ + { + args: rows.flatMap((row) => [row.textIndex, row.encrypted, created]), + sql: `INSERT OR IGNORE INTO strings (text_index, encrypted_text, created) VALUES ${rows + .map(() => "(?, ?, ?)") + .join(", ")}`, + }, // Refresh `created` on every referenced row. INSERT OR IGNORE leaves an // existing row's timestamp untouched, so without this the age-based prune // could delete a row a checkout still references in its signed metadata @@ -86,7 +144,28 @@ export const getOrCreateStringIds = async ( args: textIndexes, sql: `SELECT id, text_index FROM strings WHERE text_index IN (${inPlaceholders(textIndexes)})`, }, - ]); - const found = resultRows<{ id: number; text_index: string }>(results.at(-1)!); + ]; + const selectResult = await runInternStatements(statements, tx); + const found = resultRows<{ id: number; text_index: string }>(selectResult); return pairStringIds(rows, found); }; + +/** + * Intern a list of free-text answers, returning the `text → id` map. Encrypts + * and HMAC-indexes each unique text, then inserts-or-ignores, refreshes + * `created`, and reads the ids back in one atomic batch. When `tx` is given, + * every statement runs on the caller's open transaction via `tx.execute` (so + * the read-your-writes SELECT shares the INSERT's transaction); otherwise as + * one `executeBatchWithResults` write batch. + * + * Callers wrapping their save in `withTransaction` should call + * {@link prepareStringRows} *before* opening the transaction (to keep the + * CPU-bound crypto out of the write-lock window) and then + * {@link internStringRows} on the tx. This entry point does both in one call + * for the standalone path and callers that don't need that separation. + */ +export const getOrCreateStringIds = async ( + texts: string[], + tx?: TxScope, +): Promise> => + internStringRows(await prepareStringRows(texts), tx); diff --git a/test/lib/parse-question-answers.test.ts b/test/lib/parse-question-answers.test.ts index 9ca45062a6..bb56afca31 100644 --- a/test/lib/parse-question-answers.test.ts +++ b/test/lib/parse-question-answers.test.ts @@ -163,3 +163,26 @@ describe("parseQuestionAnswers deactivated answers", () => { expect(result).toEqual({ answerIds: [], ok: true, textAnswers: [] }); }); }); + +describe("parseQuestionAnswers malformed input", () => { + // Regression: `Number.parseInt("10xyz", 10)` returned `10`, so a crafted + // submission could match answer id 10 by accident. The parser now uses + // `parsePositiveIntId`, which rejects any non-digit string before coercing. + test("rejects a numeric-prefixed value instead of matching the prefix", () => { + const form = new URLSearchParams({ question_1: "10xyz" }); + const result = parseQuestionAnswers({ optional: false })(form, [radio(1)]); + expect(result).toEqual({ + error: "Invalid answer for: Question 1", + ok: false, + }); + }); + + test("rejects a non-numeric value", () => { + const form = new URLSearchParams({ question_1: "abc" }); + const result = parseQuestionAnswers({ optional: false })(form, [radio(1)]); + expect(result).toEqual({ + error: "Invalid answer for: Question 1", + ok: false, + }); + }); +}); diff --git a/test/lib/servicing/atomicity.test.ts b/test/lib/servicing/atomicity.test.ts index 890ed7695c..759c7a785c 100644 --- a/test/lib/servicing/atomicity.test.ts +++ b/test/lib/servicing/atomicity.test.ts @@ -12,7 +12,6 @@ // jscpd:ignore-start import { expect } from "@std/expect"; import { it as test } from "@std/testing/bdd"; -import { getDb } from "#shared/db/client.ts"; import { createDailyTestListing, createServicingHold, @@ -24,54 +23,25 @@ import { getTestPrivateKey, servicingRowsForListing, updateServicingEvent, + withPoisonedTransactionExecute, } from "#test-utils"; // jscpd:ignore-end -/** - * Reject the FIRST batch whose SQL matches `matches`, then delegate every - * subsequent batch — including the compensating delete/restore — so the - * create/update compensation runs against a working client. Swaps the libsql - * client's `batch` method in place (module namespaces are frozen, but the - * client instance's method is configurable) and discriminates by SQL content. - */ -const withPoisonedBatch = - (matches: (sql: string) => boolean, message: string) => - async (body: () => Promise): Promise => { - const db = getDb(); - const realBatch = db.batch; - let poisoned = true; - db.batch = (( - statements: { sql: string }[], - mode?: "read" | "write", - ): Promise => { - const sqls = statements.map((s) => (typeof s === "string" ? s : s.sql)); - if (poisoned && sqls.some(matches)) { - poisoned = false; - return Promise.reject(new Error(message)); - } - return realBatch.call(db, statements as never, mode); - }) as typeof db.batch; - try { - await body(); - } finally { - db.batch = realBatch; - } - }; - -/** Fail the FIRST `attendee_answers` batch (the answer save), so the +/** Fail the FIRST `attendee_answers` write (the answer save), so the * create/update compensation runs. */ -const withAnswerSaveFailure = withPoisonedBatch( +const withAnswerSaveFailure = withPoisonedTransactionExecute( (sql) => sql.includes("attendee_answers"), "answer save boom", ); -/** Fail the FIRST `INSERT INTO attendee_answers` batch, letting the preceding - * clear (`DELETE FROM attendee_answers`) commit first — the exact window in - * which the free-text-loss bug lived: the delete drops the old answers, then - * the re-insert fails, so the compensation must restore the WHOLE prior answer - * set (choice + free-text), not just its choice half. */ -const withAnswerInsertFailure = withPoisonedBatch( +/** Fail the FIRST `INSERT INTO attendee_answers`, letting the preceding + * clear (`DELETE FROM attendee_answers`) run on the same transaction first — + * the exact window in which the free-text-loss bug lived: the delete drops the + * old answers, then the re-insert fails and rolls the delete back, so the + * compensation must restore the WHOLE prior answer set (choice + free-text), + * not just its choice half. */ +const withAnswerInsertFailure = withPoisonedTransactionExecute( (sql) => sql.includes("INSERT INTO attendee_answers"), "answer insert boom", ); diff --git a/test/shared/db/questions/attendee-answers.test.ts b/test/shared/db/questions/attendee-answers.test.ts index d001928ee4..223f9830b3 100644 --- a/test/shared/db/questions/attendee-answers.test.ts +++ b/test/shared/db/questions/attendee-answers.test.ts @@ -10,7 +10,12 @@ import { import { saveAttendeeAnswers } from "#shared/db/questions/attendee-answers/save.ts"; import { getOrCreateStringIds } from "#shared/db/questions/strings.ts"; import { nowIso } from "#shared/now.ts"; -import { createTestListing, describeWithEnv } from "#test-utils"; +import { + createTestListing, + describeWithEnv, + expectRejects, + withPoisonedTransactionExecute, +} from "#test-utils"; import { getTestPrivateKey } from "#test-utils/crypto.ts"; import { addAnswer, @@ -19,6 +24,31 @@ import { saveTextAnswers, } from "./helpers.ts"; +/** The choice answer ids one attendee has saved (undefined when none). Shared by + * the replace and rollback tests so the read+assert pair stays one line each. */ +const choiceAnswersFor = async (att: { + id: number; +}): Promise => { + const batch = await getAttendeeAnswersBatch([att.id], { texts: false }); + return batch.get(att.id); +}; + +/** "Colour?" question with Red/Blue options plus one attendee who has saved Red + * — the shared setup behind the replace and rollback tests below. */ +const seedColourAttendeeWithRed = async (): Promise<{ + a1: { id: number }; + a2: { id: number }; + att: { id: number }; +}> => { + const q = await createQuestion("Colour?"); + const a1 = await addAnswer(q.id, 0, "Red"); + const a2 = await addAnswer(q.id, 1, "Blue"); + const listing = await createTestListing(); + const att = await createAttendee(listing.id); + await saveAttendeeAnswers(new Map([[att.id, [a1.id]]])); + return { a1, a2, att }; +}; + describeWithEnv("custom questions", { db: true }, () => { describe("createAttendee helper", () => { test("throws when the listing has no capacity", async () => { @@ -124,21 +154,68 @@ describeWithEnv("custom questions", { db: true }, () => { }); test("saveAttendeeAnswers replaces existing answers atomically", async () => { - const q = await createQuestion("Colour?"); - const a1 = await addAnswer(q.id, 0, "Red"); - const a2 = await addAnswer(q.id, 1, "Blue"); + const { a1, a2, att } = await seedColourAttendeeWithRed(); + expect(await choiceAnswersFor(att)).toEqual([a1.id]); - const listing = await createTestListing(); - const att = await createAttendee(listing.id); - await saveAttendeeAnswers(new Map([[att.id, [a1.id]]])); + await saveAttendeeAnswers(new Map([[att.id, [a2.id]]])); - const before = await getAttendeeAnswersBatch([att.id], { texts: false }); - expect(before.get(att.id)).toEqual([a1.id]); + expect(await choiceAnswersFor(att)).toEqual([a2.id]); + }); - await saveAttendeeAnswers(new Map([[att.id, [a2.id]]])); + test("saveAttendeeAnswers rolls back the DELETE when the INSERT fails", async () => { + // Regression for the CodeRabbit gap on PR #1678: the delete and insert + // used to be two committed batches, so an INSERT failure after the DELETE + // committed left the attendee with no answers at all. Now the whole save + // runs in one transaction, so a mid-save INSERT failure rolls the DELETE + // back and the attendee's prior answers survive. + const { a1, a2, att } = await seedColourAttendeeWithRed(); + expect(await choiceAnswersFor(att)).toEqual([a1.id]); + + await withPoisonedTransactionExecute( + (sql) => sql.includes("INSERT INTO attendee_answers"), + "insert boom", + )(async () => { + await expectRejects( + saveAttendeeAnswers(new Map([[att.id, [a2.id]]])), + /insert boom/, + ); + }); - const after = await getAttendeeAnswersBatch([att.id], { texts: false }); - expect(after.get(att.id)).toEqual([a2.id]); + // The DELETE rolled back with the failed INSERT: a1 survives, not empty. + expect(await choiceAnswersFor(att)).toEqual([a1.id]); + }); + + test("saveAttendeeAnswers rollback preserves a FREE-TEXT answer, not just choice ids", async () => { + // The choice-only rollback test above can't catch free-text loss: a + // free-text answer is interned into the strings table and referenced by + // string_id, so a non-atomic delete-then-insert that drops the text row + // and then fails on the re-insert loses the attendee's free-text answer + // even though the choice path looked intact. This test seeds a free-text + // answer, forces the INSERT to fail mid-save, and asserts the decrypted + // text survives the rollback. + const { attendee: att, q } = await seedFreeTextQuestion("Notes?"); + const privateKey = await getTestPrivateKey(); + await saveTextAnswers(att.id, [{ questionId: q.id, text: "Keep me" }]); + expect((await getAttendeeTextAnswers(att.id, privateKey)).get(q.id)).toBe( + "Keep me", + ); + + await withPoisonedTransactionExecute( + (sql) => sql.includes("INSERT INTO attendee_answers"), + "insert boom", + )(async () => { + await expectRejects( + saveTextAnswers(att.id, [ + { questionId: q.id, text: "Should not land" }, + ]), + /insert boom/, + ); + }); + + // The free-text answer survived the rolled-back save, not lost. + expect((await getAttendeeTextAnswers(att.id, privateKey)).get(q.id)).toBe( + "Keep me", + ); }); test("saves text-only answers and decrypts them for editing", async () => { diff --git a/test/test-utils.ts b/test/test-utils.ts index 741313f63f..90ba360bdd 100644 --- a/test/test-utils.ts +++ b/test/test-utils.ts @@ -13,6 +13,7 @@ export * from "./test-utils/db-helpers/holidays.ts"; export * from "./test-utils/db-helpers/listing-forms.ts"; export * from "./test-utils/db-helpers/listings.ts"; export * from "./test-utils/db-helpers/misc.ts"; +export * from "./test-utils/db-poison.ts"; export * from "./test-utils/e2e.ts"; export * from "./test-utils/email.ts"; export * from "./test-utils/env.ts"; diff --git a/test/test-utils/db-poison.ts b/test/test-utils/db-poison.ts new file mode 100644 index 0000000000..b35c104f13 --- /dev/null +++ b/test/test-utils/db-poison.ts @@ -0,0 +1,39 @@ +import { getDb } from "#shared/db/client.ts"; + +/** + * Reject the first transactional `tx.execute` whose SQL matches `matches`, then + * delegate every subsequent execute to the real tx — so the failure lands + * mid-flow (after the in-transaction DELETE ran, for `saveAttendeeAnswers`) and + * the caller's rollback/compensation runs against a working client. + * + * Swaps the db client's `transaction` method in place (module namespaces are + * frozen, but the client instance's method is configurable), then restores it + * in `finally`. `saveAttendeeAnswers` runs its DELETE + string interning + + * INSERT inside one `withTransaction`, so a poison that intercepts + * `db.transaction`'s `execute` is what reaches its writes; a `db.batch` poison + * would no longer fire because the answer save stopped batching. + */ +export const withPoisonedTransactionExecute = + (matches: (sql: string) => boolean, message: string) => + async (body: () => Promise): Promise => { + const db = getDb(); + const realTransaction = db.transaction.bind(db); + let poisoned = true; + db.transaction = (async (mode: "read" | "write" = "write") => { + const tx = await realTransaction(mode); + const realExecute = tx.execute.bind(tx); + tx.execute = ((stmt: { sql: string }) => { + if (poisoned && matches(stmt.sql)) { + poisoned = false; + return Promise.reject(new Error(message)); + } + return realExecute(stmt as never); + }) as typeof tx.execute; + return tx; + }) as typeof db.transaction; + try { + await body(); + } finally { + db.transaction = realTransaction; + } + };