Make saveAttendeeAnswers truly atomic (transactional delete + intern + insert) - #1686
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAttendee answer persistence now performs deletion, reads, string interning, and replacement inserts in one transaction. Transactional rollback tests cover choice and free-text answers. Submitted answer IDs now require strict positive-integer parsing. ChangesAtomic attendee answer persistence
Strict answer parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant saveAttendeeAnswers
participant TxScope
participant StringInterning
participant attendee_answers
Caller->>saveAttendeeAnswers: submit normalized answers
saveAttendeeAnswers->>TxScope: start write transaction
TxScope->>attendee_answers: delete existing answers
TxScope->>attendee_answers: read answer and question mappings
TxScope->>StringInterning: intern prepared free-text rows
StringInterning->>TxScope: execute INSERT, UPDATE, and SELECT
TxScope->>attendee_answers: insert replacement answers
TxScope-->>saveAttendeeAnswers: commit or roll back
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e924c9779
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/db/questions/attendee-answers.ts`:
- Around line 168-178: The attendee answer cleanup in attendee-answers.ts is
doing one DELETE per attendee inside withTransaction, which adds unnecessary
round trips. Update the delete phase to use a single batched DELETE with an IN
clause (or equivalent one-statement multi-id delete) while keeping the existing
delete → intern → insert order in attendee-answers save flow. Please also verify
the strings.used_count trigger is defined FOR EACH ROW so the consolidated
DELETE still fires per affected row and preserves counting semantics.
In `@src/shared/db/questions/parsing.ts`:
- Around line 36-40: The question answer parsing in parsing.ts is too permissive
because parseInt accepts numeric prefixes like "12xyz" and can match a real
answer by accident. Update the logic around the raw form value, answerId
parsing, and findAnswerById to validate the entire submitted string before
converting it to a number, ideally using the repository’s Valibot helpers, and
reject any malformed input as invalid rather than continuing to lookup an
answer.
In `@test/shared/db/questions/attendee-answers.test.ts`:
- Around line 27-50: The rollback regression currently only seeds a choice
answer in seedColourAttendeeWithRed, so it cannot catch free-text loss. Update
the shared test setup to also create a free-text question and saved answer
alongside the existing Colour?/Red path, then extend the replace/rollback
assertions to verify both the choice answer and the free-text answer still exist
after the forced insert failure using choiceAnswersFor and the shared seed
helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ccd6b40d-bd63-4e88-92af-170b1d898f47
📒 Files selected for processing (92)
TASK.mdbiome.jsonsrc/features/admin/attendee-form-routes.tssrc/features/admin/attendee-page-data.tssrc/features/admin/attendees-csv.tssrc/features/admin/attendees-merge.tssrc/features/admin/calendar.tssrc/features/admin/group-page-data.tssrc/features/admin/listing-page-data.tssrc/features/admin/listings-view.tssrc/features/admin/modifiers.tssrc/features/admin/questions.tssrc/features/api/payment-processing.tssrc/features/public/ticket-form.tssrc/features/public/ticket-payment.tssrc/features/public/ticket-submit/parse.tssrc/features/public/ticket-submit/paths.tssrc/features/public/ticket-submit/prepare.tssrc/features/public/types.tssrc/shared/db/attendees/servicing.tssrc/shared/db/question-types.tssrc/shared/db/questions.tssrc/shared/db/questions/aggregates.tssrc/shared/db/questions/attendee-answers.tssrc/shared/db/questions/delete.tssrc/shared/db/questions/parsing.tssrc/shared/db/questions/queries.tssrc/shared/db/questions/sort-order.tssrc/shared/db/questions/strings.tssrc/shared/db/questions/tables.tssrc/shared/merge/attendee-merge.tssrc/shared/qr.tssrc/ui/templates/admin/attendee-detail.tsxsrc/ui/templates/admin/attendee-form.tsxsrc/ui/templates/admin/attendees.tsxsrc/ui/templates/admin/questions.tsxsrc/ui/templates/attendee-table.tsxsrc/ui/templates/components/question-text.tsxsrc/ui/templates/public/reservations.tsxtest/e2e/n-plus-one-guard.test.tstest/lib/checkout-pricing-consistency.test.tstest/lib/column-order/attendee-answers.test.tstest/lib/csv-questions.test.tstest/lib/db/free-text-migration.test.tstest/lib/db/legacy-migration.test.tstest/lib/db/with-transaction.test.tstest/lib/parse-question-answers.test.tstest/lib/render-questions.test.tstest/lib/server-attendee-form/questions.test.tstest/lib/server-attendees.test.tstest/lib/server-booking-preserve.test.tstest/lib/server-calculate.test.tstest/lib/server-groups/attendees.test.tstest/lib/server-listing-export-checkin.test.tstest/lib/server-listings/export.test.tstest/lib/server-listings/show-groups-and-answers.test.tstest/lib/server-misc-admin-handlers.test.tstest/lib/server-modifiers/scope-links.test.tstest/lib/server-parents-gate.test.tstest/lib/server-public/custom-questions-multi.test.tstest/lib/server-public/custom-questions-single.test.tstest/lib/server-questions/answer-edit.test.tstest/lib/server-questions/answers.test.tstest/lib/server-questions/helpers.tstest/lib/server-questions/listing-questions.test.tstest/lib/server-questions/question-delete.test.tstest/lib/server-questions/questions.test.tstest/lib/server-webhooks/custom-questions-multi.test.tstest/lib/server-webhooks/custom-questions-single.test.tstest/lib/servicing/atomicity.test.tstest/lib/servicing/custom-questions.test.tstest/lib/test-utils/stubs-and-mocks.test.tstest/lib/test-utils/temp-db-files.test.tstest/lib/ticket-form.test.tstest/shared/db/listings/delete.test.tstest/shared/db/modifier-resolve.test.tstest/shared/db/questions/attendee-answers.test.tstest/shared/db/questions/crud.test.tstest/shared/db/questions/helpers.tstest/shared/db/questions/listing-mapping.test.tstest/shared/db/questions/with-listings.test.tstest/shared/merge/attendee-merge/apply.test.tstest/shared/merge/attendee-merge/diff.test.tstest/shared/merge/attendee-merge/helpers.tstest/test-utils.tstest/test-utils/db-poison.tstest/test-utils/db.tstest/test-utils/factories.tstest/test-utils/temp-db-files.tstest/ui/templates/admin/attendee-detail.test.tstest/ui/templates/admin/attendees.test.tsxtest/ui/templates/components/question-text.test.tsx
💤 Files with no reviewable changes (2)
- biome.json
- src/shared/db/questions.ts
6e924c9 to
8cbf9f0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cbf9f00c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/db/questions/strings.ts`:
- Around line 93-113: The `strings.ts` interning path is still issuing one
`INSERT OR IGNORE` per text inside `runInternStatements`, which causes extra
transactional round trips. Update the statement construction in this section so
the `rows.map(...)` insertions are combined into a single multi-row `INSERT OR
IGNORE` statement, matching the batched shape used by the non-transactional path
and the pattern already applied in `attendee-answers.ts`. Keep the `UPDATE
strings` and `SELECT id, text_index` statements as-is, but ensure the insert
batch still feeds `runInternStatements` correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e4a4ab09-8153-4d44-99ee-636f702e63d1
📒 Files selected for processing (8)
TASK.mdsrc/features/admin/attendees-merge.tssrc/shared/db/questions/attendee-answers.tssrc/shared/db/questions/strings.tstest/lib/servicing/atomicity.test.tstest/shared/db/questions/attendee-answers.test.tstest/test-utils.tstest/test-utils/db-poison.ts
f2a8c16 to
03ec748
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/lib/parse-question-answers.test.ts`:
- Around line 171-173: Update the numeric-prefix regression test for
parseQuestionAnswers to include both radio(1) and radio(10) in the available
answers, ensuring the prior parseInt("10xyz", 10) behavior would incorrectly
match answer 10 and the test meaningfully detects the regression.
In `@test/test-utils/db-poison.ts`:
- Around line 16-39: Update withPoisonedTransactionExecute to avoid mutating the
shared getDb() singleton during parallel tests: use an isolated database
instance for the transaction/execute hooks, or mark all tests using this helper
as serial. Ensure cleanup restores any modified state and prevents concurrent
tests from observing the poison behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cbb1de05-76f8-4d94-accd-83b589343355
📒 Files selected for processing (10)
TASK.mdsrc/features/admin/attendees-merge.tssrc/shared/db/questions/attendee-answers.tssrc/shared/db/questions/parsing.tssrc/shared/db/questions/strings.tstest/lib/parse-question-answers.test.tstest/lib/servicing/atomicity.test.tstest/shared/db/questions/attendee-answers.test.tstest/test-utils.tstest/test-utils/db-poison.ts
|
Replying to each open thread — all addressed in the latest commit ( Codex P1 (batch attendee-answer writes inside the transaction) — Fixed. The per-attendee DELETE loop is now one Codex P2 (precompute free-text encryption before opening the transaction) — Fixed. CodeRabbit (batch the per-attendee DELETE) — Fixed as above; confirmed the CodeRabbit (reject malformed answer IDs in CodeRabbit (include a free-text answer in the rollback regression) — Fixed. Added a dedicated test that seeds a free-text answer, poisons the INSERT, and asserts the decrypted text survives the rollback via All non-mutation precommit steps pass: typecheck (incl. test files), lint:ci, cpd (0%), and the directly-affected suites (attendee-answers, servicing atomicity, both webhook suites, server-attendees, parse-question-answers, full stripe suite — 250 tests). |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/db/questions/strings.ts (1)
118-151: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider collapsing the intern sequence into one
INSERT ... ON CONFLICT(text_index) DO UPDATE SET created = excluded.created RETURNING id, text_index. This keeps the timestamp refresh and id lookup in a single round trip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/db/questions/strings.ts` around lines 118 - 151, Refactor internStringRows to replace the separate INSERT, timestamp UPDATE, and SELECT statements with one INSERT ... ON CONFLICT(text_index) DO UPDATE SET created = excluded.created RETURNING id, text_index statement. Build values for all rows, execute it through runInternStatements, extract the returned rows, and continue passing them to pairStringIds.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/shared/db/questions/strings.ts`:
- Around line 118-151: Refactor internStringRows to replace the separate INSERT,
timestamp UPDATE, and SELECT statements with one INSERT ... ON
CONFLICT(text_index) DO UPDATE SET created = excluded.created RETURNING id,
text_index statement. Build values for all rows, execute it through
runInternStatements, extract the returned rows, and continue passing them to
pairStringIds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 788012ac-7a9c-4805-a6be-de71ae777cd3
📒 Files selected for processing (10)
TASK.mdsrc/features/admin/attendees-merge.tssrc/shared/db/questions/attendee-answers.tssrc/shared/db/questions/parsing.tssrc/shared/db/questions/strings.tstest/lib/parse-question-answers.test.tstest/lib/servicing/atomicity.test.tstest/shared/db/questions/attendee-answers.test.tstest/test-utils.tstest/test-utils/db-poison.ts
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).
03ec748 to
5388f4b
Compare
1 participant
Picks up the deferred follow-up from PR #1678's CodeRabbit review (comment #2: "Delete and insert should be atomic").
What this changes
saveAttendeeAnswersreplaces every listed attendee's answers — delete the old ones, then insert the new ones. It used to run as two separate committed batches (a DELETE batch, then an INSERT batch), with string interning in between. If the INSERT batch failed after the DELETE had already committed, the attendee was left with no answers until a manual re-save.Now the whole flow runs inside one interactive write transaction (
withTransaction), so it commits or rolls back as a single unit. An INSERT failure rolls the DELETE back; the attendee's prior answers survive.How
The save runs in five phases, all but the first inside one
withTransaction:prepareStringRows) — pure CPU: HMAC + hybrid encryption, no IO. Done before opening the transaction so the crypto work doesn't hold the SQLite writer open with no statement running.IN (...)statement for every attendee at once. SQLite triggers are implicitlyFOR EACH ROW(there is no statement-level trigger form), sostrings.used_countis decremented once per row regardless of whether the DELETE matches one row or many.answer_id → question_idand which text questions still exist, on the tx so they share the save's snapshot. A question or answer deleted between checkout and finalize is skipped rather than producing an orphan row.internStringRows(rows, tx)— one batched multi-rowINSERT OR IGNORE+ refreshcreated+ one read-your-writesSELECT, all on the tx. Fixed 3 round trips regardless of how many unique texts are saved.VALUESbatches: one for every attendee's choice answers, one for every text answer. Batching across attendees keeps the statement count at a handful regardless of attendee count.Total round-trips for a save is now fixed at ~8 (1 DELETE + 2 reads + 3 intern + ≤2 INSERT), well under the 30-statement transaction round-trip guard.
The delete → intern → insert ordering is preserved inside the transaction: the DELETE's trigger decrements
strings.used_countfirst, then the interning refreshescreatedon the strings this save re-references, so the pruner sees a consistent snapshot.strings.tsis split intoprepareStringRows(pure CPU) andinternStringRows(the DB statements), withgetOrCreateStringIdskept as a convenience entry point that does both for the standalone path (e.g.ticket-submit/parse.ts) where the crypto ordering doesn't matter.Also fixed (review follow-ups)
parsing.ts: replacedNumber.parseInt(raw, 10)with the repo's sharedparsePositiveIntId(Valibotv.digits()-based), which rejects any non-digit string before coercing.Number.parseInt("12xyz", 10)returned12, so a malformed submission could select a real answer by accident. Added two regression tests.withPoisonedTransactionExecutetotest/test-utils/db-poison.ts(poisoningdb.transaction'sexecute), sincesaveAttendeeAnswersmoved fromdb.batchtodb.transaction+tx.executeand the oldwithPoisonedBatchno longer reached its writes. Migrated the servicing atomicity tests to it.Regression tests
tx.executeto reject the firstINSERT INTO attendee_answersmid-save. The attendee's prior choice answer survives — the DELETE rolled back with the failed INSERT.getAttendeeTextAnswers. The choice-only test couldn't catch free-text loss because free-text is interned into the strings table and referenced bystring_id."12xyz"and"abc"are both rejected asinvalid(previously"12xyz"matched answer 12 viaparseInt's prefix parsing).Verified the rollback tests catch the real bug by temporarily moving the DELETE back to its own committed batch before the transaction (the old non-atomic shape): the tests failed for the right reason, then passed again once the DELETE moved back inside the transaction.
Verification
Branch context
Based on
split-questions(PR #1678). Once that merges to main, this branch can rebase onto main.Summary by CodeRabbit
Bug Fixes
Tests