Save each booking completely or not at all - #1833
Merged
Merged
Conversation
Contributor
|
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 (2)
📝 WalkthroughWalkthroughThis PR refactors attendee creation and payment processing around stable ticket tokens, atomic batch writes, guarded payment finalization, explicit recovery decisions, and centralized completion. It removes post-create completeness checks and adds rollback, replay, concurrency, and finalize-guard coverage. ChangesAtomic attendee creation and payment-processing refactor
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant processPaymentSession
participant createAttendeeForSession
participant createBookingAtomic
participant completePaidBooking
participant recoverOrRefundUnexpectedCreate
participant Database
processPaymentSession->>createAttendeeForSession: create with stable ticket token
createAttendeeForSession->>createBookingAtomic: atomic attendee and booking write
createBookingAtomic->>Database: guarded batch and payment finalization
alt successful create
createAttendeeForSession-->>processPaymentSession: created entries
processPaymentSession->>completePaidBooking: complete booking
else unexpected create result
processPaymentSession->>recoverOrRefundUnexpectedCreate: inspect recovery facts
recoverOrRefundUnexpectedCreate->>Database: query committed attendee and session state
alt recover
recoverOrRefundUnexpectedCreate->>completePaidBooking: complete committed entries
else refund
recoverOrRefundUnexpectedCreate->>Database: store unexpected-error refund
end
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 60f18b0aeb
ℹ️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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/attendees/create.ts (1)
43-59: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winTighten
AttendeeOrderFields
kindshould reuseAttendeeKindinstead ofstring.remainingBalanceis not written bybuildAttendeeInsertand attendee rows no longer storeremaining_balance; drop it from this insert-only shape and keep it only where the returned attendee is built.🤖 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/attendees/create.ts` around lines 43 - 59, Update AttendeeOrderFields to type kind as AttendeeKind and remove remainingBalance from the insert-only shape, since buildAttendeeInsert does not write it. Preserve remainingBalance only in the separate returned-attendee construction path.
🤖 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 `@docs/importer-plan.md`:
- Around line 1263-1267: Update the importer plan’s references to the per-source
booking count and visit counts to explicitly use the resolved counter name
admin_booking_count, including the corresponding section around the later
booking-counter instructions. Ensure the steps clearly distinguish incrementing
visits from incrementing admin_booking_count.
In `@src/features/admin/attendee-form-routes.ts`:
- Around line 367-368: Update the failure handling around createAttendeeAtomic
so an encryption_error is not mapped to t("attendee_form.error_capacity").
Preserve the capacity message only for the capacity failure reason, and for
encryption_error propagate the failure or return the established generic save
error; do not suppress unexpected failures with a default.
In `@src/features/api/payment-processing/completion.ts`:
- Around line 23-33: Update the post-commit flow containing saveSessionAnswers,
logPromoCodeModifiers, and logAndNotifyRegistration to persist an idempotent
completion checkpoint or outbox before executing effects, record each effect as
completed, and resume any unfinished effects when the booking is replayed.
Ensure retries skip completed operations while reliably retrying rejected
session-answer, promo-code, or registration-notification work.
In `@src/shared/db/attendees/create.ts`:
- Around line 139-157: Update the caller around buildCapacityCheckedInsert to
pass enc.ticketTokenIndex as its attendeeIdArg parameter, then use the returned
statement.args directly without manually splicing the token index or relying on
statement.args.slice. Preserve the existing extraCondition SQL and argument
handling.
In `@test/lib/webhook-price-signature/post-commit-recovery.test.ts`:
- Around line 312-317: Make the regression test deterministically exercise the
booking-completion race by adding a barrier-controlled stub or hook at the
critical-section boundary, pausing the first request until the other request
reaches that same boundary. Update the test around webhookRequest and
redirectRequest so both requests are released together, then retain the existing
assertions for processed status and redirect response.
In `@test/shared/db/attendees/create-rollback.test.ts`:
- Around line 277-284: Strengthen the rollback test around createBookingAtomic
by fetching the contact record and asserting visits and publicBookingCount are
both 1 immediately after the atomic creation, before calling seedOrderActivity.
Keep the existing post-replay assertions and verify those counts remain
unchanged after replay, ensuring the test proves activity was written by the
original operation.
In `@test/test-utils/atomic-booking.ts`:
- Around line 5-47: Extract the shared trigger lifecycle from
withRejectedBookingWrite and withSkippedBookingWrite into one private helper
that accepts the trigger name, trigger SQL, and callback, creates the trigger,
executes the callback, and always drops the trigger in cleanup. Keep both
exported wrappers and have each delegate to this helper with its existing
trigger behavior.
---
Outside diff comments:
In `@src/shared/db/attendees/create.ts`:
- Around line 43-59: Update AttendeeOrderFields to type kind as AttendeeKind and
remove remainingBalance from the insert-only shape, since buildAttendeeInsert
does not write it. Preserve remainingBalance only in the separate
returned-attendee construction path.
🪄 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: 42cd9723-ebce-4484-9273-3cb79e5b5c39
📒 Files selected for processing (69)
TODO.mddocs/importer-plan.mdscripts/mutation/equivalent-mutants.txtsrc/docs/database.tssrc/features/admin/attendee-form-routes.tssrc/features/api/payment-processing/committed-entries.tssrc/features/api/payment-processing/completion.tssrc/features/api/payment-processing/create.tssrc/features/api/payment-processing/index.tssrc/features/api/payment-processing/recovery-decision.tssrc/features/api/payment-processing/recovery.tssrc/features/api/payment-processing/store-refund.tssrc/features/api/webhooks.tssrc/features/public/ticket-payment.tssrc/shared/db/attendee-types.tssrc/shared/db/attendees/create-batch.tssrc/shared/db/attendees/create.tssrc/shared/db/attendees/pii.tssrc/shared/db/attendees/servicing.tssrc/shared/db/client.tssrc/shared/db/contact-preferences.tssrc/shared/db/contact-tokens.tssrc/shared/db/modifier-usage.tssrc/shared/db/payment-finalize.tssrc/shared/db/processed-payments.tssrc/shared/seeds.tstest/features/admin/attendee-create-atomic.test.tstest/features/api/payment-processing/recovery-decision.test.tstest/features/public/free-reservation-atomic.test.tstest/features/public/ticket-payment.test.tstest/integration/servicing/create-atomic.test.tstest/lib/code-quality.test.tstest/lib/db/attendees/create-attendee-atomic.test.tstest/lib/db/attendees/delete-attendee.test.tstest/lib/db/settle-balance.test.tstest/lib/processed-payments/locking.test.tstest/lib/server-attendee-refresh-payment.test.tstest/lib/server-attendees/delete-incomplete.test.tstest/lib/server-bulk-email/notes-and-history.test.tstest/lib/server-privacy.test.tstest/lib/server-public/ticket-additional-coverage.test.tstest/lib/server-refunds-balance-payments.test.tstest/lib/server-reservation/helpers.tstest/lib/server-reservation/public-default-modifiers.test.tstest/lib/server-webhooks/already-processed-rollback.test.tstest/lib/server-webhooks/concurrent-processing.test.tstest/lib/server-webhooks/modifier-refunds.test.tstest/lib/test-utils/factories.test.tstest/lib/webhook-price-signature/post-commit-recovery.test.tstest/routes/unsubscribe.test.tstest/shared/db/attendees/create-rollback.test.tstest/shared/db/attendees/create.test.tstest/shared/db/contact-preferences.test.tstest/shared/db/contact-tokens.test.tstest/shared/db/listing-overview-stats.test.tstest/shared/db/listings/delete.test.tstest/shared/db/modifier-resolve.test.tstest/shared/db/modifier-usage.test.tstest/shared/db/payment-references.test.tstest/shared/db/processed-payments.test.tstest/shared/db/processed-payments/finalize-guard.test.tstest/shared/db/processed-payments/staleness.test.tstest/shared/merge/attendee-merge/repoint.test.tstest/test-utils/atomic-booking.tstest/test-utils/contact-preferences.tstest/test-utils/contact-tokens.tstest/test-utils/db-helpers/attendees.tstest/test-utils/modifiers.tstest/test-utils/processed-payments.ts
💤 Files with no reviewable changes (5)
- TODO.md
- test/shared/db/attendees/create.test.ts
- src/docs/database.ts
- test/lib/code-quality.test.ts
- src/shared/db/contact-preferences.ts
Member
Author
|
Addressed the CodeRabbit review-body comment about |
Contributor
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 `@docs/importer-plan.md`:
- Around line 1751-1755: Align the phase-5 list item beginning “Increment visits
and admin_booking_count” with its sibling items by reducing its indentation to
the surrounding list level, including its wrapped lines.
🪄 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: e4f09591-a088-420e-9b6e-b08b63443a3a
📒 Files selected for processing (9)
TODO.mddocs/importer-plan.mdsrc/features/admin/attendee-form-routes.tssrc/shared/db/attendees/capacity/checks.tssrc/shared/db/attendees/create.tstest/features/admin/attendee-create-atomic.test.tstest/lib/webhook-price-signature/post-commit-recovery.test.tstest/shared/db/attendees/create-rollback.test.tstest/test-utils/atomic-booking.ts
This was referenced Jul 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Why
A multi-ticket booking could previously fail after saving only part of its data. A successful database write could also look like a failure when its reply was lost. That could leave a partial booking or refund a payment for a ticket that exists.
This change makes the booking write all-or-nothing and makes uncertain write results safe to recover. It does not add staged checkout tables or seat holds.
Checks
deno task test:files test/lib/webhook-price-signature/post-commit-recovery.test.tsdeno task precommitmain.Required follow-up
Paid work after the database commit is not yet resumable. If saving answers, logging promo-code use, or sending registration work fails after commit, a later replay can return success without retrying that work.
A full durable outbox or completion checkpoint is required before staged payment runtime work starts. It must track each effect, prevent duplicate delivery, resume unfinished work, and block pruning while work remains. This high-priority prerequisite is recorded in
TODO.md. This PR does not add a partial retry.