fix(api): retry Stripe-webhook concurrency conflicts + close idempotency TOCTOU (#243)

[thomasluizon]

What & why

Mirrors the Play-webhook hardening from #327 onto the Stripe webhook handler (HandleWebhookCommand), which had the same three concurrency/idempotency defects.

(a) Concurrency retry

HandleWebhookCommand now implements IConcurrencyRetryable, so a DbUpdateConcurrencyException — a stale xmin token when two webhook deliveries mutate the same User — re-runs the whole (idempotent) handler through ConcurrencyRetryBehavior (reload → re-apply → save) instead of surfacing as an unhandled 500. The handler performs only idempotent external reads (EventUtility.ConstructEvent, SubscriptionService.GetAsync), so it is safe to re-run per the IConcurrencyRetryable contract.

(b) Catch no longer swallows the concurrency conflict

The trailing generic catch previously caught DbUpdateConcurrencyException and returned Result.Failure(WebhookProcessingFailed), which would have prevented the retry behavior from ever seeing it. It now excludes DbUpdateConcurrencyException (when (ex is not OperationCanceledException and not DbUpdateConcurrencyException)) so the conflict propagates to the behavior.

(c) TOCTOU duplicate detection replaced by the unique constraint

The AnyAsync(EventId == …) check-then-AddAsync was a time-of-check-to-time-of-use race: two concurrent deliveries of the same event both pass the existence check, both process, and both insert. Removed the pre-check and now rely solely on the ProcessedStripeEvent.EventId unique index (SQLSTATE 23505), caught idempotently by the existing DbUniqueViolation handler — the losing writer's business mutation rolls back atomically with the failed reservation insert.

Tests

• Command_IsMarkedConcurrencyRetryable
• Handle_ThroughRetryBehavior_ConcurrencyConflictThenSuccess_UpgradesProAndRetries — conflict on first save, retried, second save succeeds, user upgraded, ResetTracking called once.
• Handle_SaveThrowsConcurrencyConflict_PropagatesInsteadOfSwallowingAsFailure — conflict propagates rather than being turned into a failure result.
• Handle_DuplicateEvent_UniqueViolationOnSave_IsIdempotentSuccess — duplicate → unique violation → idempotent Success.
• Handle_ReservesProcessedEventViaConstraint_WithoutCheckThenInsertPreCheck — no AnyAsync pre-check; AddAsync always attempted.

dotnet build clean; full suite green (Application 2631, Domain 504, Infrastructure 1534, Analyzers 7).

No DTO/endpoint contract change → no packages/shared / consumer update.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #333

Recommendation: APPROVE

Summary

Two files changed: src/Orbit.Application/Subscriptions/Commands/HandleWebhookCommand.cs (+4/-8) and its test file tests/Orbit.Application.Tests/Commands/Subscriptions/HandleWebhookCommandHandlerTests.cs (+82/-11). The change mirrors the already-shipped Play-webhook hardening (#327) onto the Stripe webhook handler:

• Marks the command IConcurrencyRetryable so a stale-xmin DbUpdateConcurrencyException on User reruns the whole handler via the existing ConcurrencyRetryBehavior instead of surfacing as a 500.
• Excludes DbUpdateConcurrencyException from the trailing catch-all so it actually propagates to that behavior.
• Drops a check-then-insert AnyAsync TOCTOU precheck in favor of the pre-existing DB-level unique index on ProcessedStripeEvent.EventId, deduped via the already-present DbUniqueViolation catch clause.

Verified end-to-end, not just the diff: ProcessedStripeEvent.EventId has a real unique index (OrbitDbContext.cs ConfigureProcessedStripeEventEntity, migration 20260614013038_AddProcessedStripeEvent.cs); User genuinely carries an xmin concurrency token (ConfigureConcurrencyTokens); the insert and business mutation share one SaveChangesAsync call so they commit/rollback atomically; DbUniqueViolation.IsUniqueViolation checks Postgres SQLSTATE 23505 specifically so it cannot misfire on a DbUpdateConcurrencyException; and every retried side effect in the handler (all four event-type branches) is a pure DB write plus one idempotent Stripe GetAsync read — no non-idempotent external call sits in the retried path, satisfying IConcurrencyRetryable's contract. An exhausted retry (3 attempts) propagates to ConcurrencyExceptionHandler, which returns a generic 409 — Stripe treats any non-2xx as "retry later," so this degrades gracefully into Stripe's own webhook retry.

Findings

Critical: None
High: None
Medium: None
Low / Info: None — faithful reuse of the established IConcurrencyRetryable convention (used by ~30 other commands), catch-clause reordering is correct, and the new/renamed tests directly exercise the new behavior including a full pass through the real ConcurrencyRetryBehavior.

Subagents

Agent	Verdict
security-reviewer	PASS — TOCTOU removal is safe (real unique index backs it), retry path is idempotent, no exception-detail leak, signature verification untouched
contract-aligner	N/A — no DTO, Controller route, or packages/shared file touched; PR body confirms no contract change

Validation

Build/Unit Tests/SonarCloud run as separate required CI checks for this PR (skipped here per workflow instructions).

Deferred (not applicable to this PR)

DESIGN.md/AI-slop, cross-repo parity, i18n, FEATURES.md — no apps/* UI surface or DTO/endpoint touched; this is an internal reliability fix.

What's good

• Mirrors an established, well-documented pattern (IConcurrencyRetryable) rather than inventing a bespoke retry mechanism.
• Genuinely root-causes the TOCTOU instead of adding a workaround — removes the racy precheck and trusts the DB constraint that was already there.
• Test coverage is proportionate and meaningful: exercises the actual ConcurrencyRetryBehavior, not just a marker-interface assertion.

[claude]

Code Review: PR #333

Recommendation: APPROVE

Summary

Two files changed: src/Orbit.Application/Subscriptions/Commands/HandleWebhookCommand.cs (+4/-8) and its test file tests/Orbit.Application.Tests/Commands/Subscriptions/HandleWebhookCommandHandlerTests.cs (+82/-11). The change marks HandleWebhookCommand IConcurrencyRetryable so a stale-xmin DbUpdateConcurrencyException on User reruns the whole handler through ConcurrencyRetryBehavior instead of being swallowed; excludes DbUpdateConcurrencyException from the trailing catch-all so the conflict actually propagates; and drops the check-then-insert AnyAsync TOCTOU precheck in favor of the pre-existing unique index on ProcessedStripeEvent.EventId (confirmed in OrbitDbContext.ConfigureProcessedStripeEventEntity), relying solely on the already-present DbUniqueViolation.IsUniqueViolation catch.

Traced the full path: ConcurrencyRetryBehavior is registered ahead of ValidationBehavior/IdempotencyBehavior in the MediatR pipeline, caps retries at 3, and calls unitOfWork.ResetTracking() between attempts so each retry re-reads fresh DB state; ConcurrencyExceptionHandler maps an exhausted retry to HTTP 409 (non-2xx, so Stripe's own delivery retries remain a backstop). Because the reservation insert and the business mutation share one SaveChangesAsync call, a losing duplicate delivery rolls back atomically — no partial-write window. The handler's only external calls (EventUtility.ConstructEvent, SubscriptionService.GetAsync) are idempotent reads, so re-running the whole handler on retry is safe per the IConcurrencyRetryable contract. Tests directly exercise the new paths, including an end-to-end run through the real ConcurrencyRetryBehavior asserting exactly 2 save attempts, ResetTracking called once, and the user actually upgraded on retry.

Findings

Critical

None.

High

None.

Medium

None concretely actionable.

Low / Info

[Info] The PR body says this "mirrors" #327's Play-webhook hardening, but HandlePlayNotificationCommand (unchanged by this diff) still keeps its AnyAsync precheck ahead of the unique-constraint fallback — it didn't fully adopt the "no precheck" pattern this PR introduces for Stripe. Not a defect in this diff (that file isn't touched here), just a note for a possible future follow-up to keep the two handlers consistent.
· location: src/Orbit.Application/Subscriptions/Commands/HandlePlayNotificationCommand.cs:50-55 (out of diff, reference only)

Subagents

Agent	Verdict
security-reviewer	PASS — signature verification unchanged and still runs before any mutation; no double-grant path; retry is bounded (3 attempts) and re-reads fresh state; no PII/secret leakage in logging
contract-aligner	N/A — no DTO, Controller route, or packages/shared type changed

Validation

Check	Result
Build (dotnet)	N/A — could not execute dotnet build in this session (command execution required interactive approval unavailable to this agent); covered by CI's separate Build required check
Tests (dotnet)	N/A — same reason; covered by CI's separate Unit Tests required check

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop), 9 (parity), 10 (i18n): N/A — no apps/* surface in this orbit-api-only diff.
• Dimension 11 (contract drift + backward-compat): N/A — no DTO/endpoint/shared-type touched.
• Dimension 14 (FEATURES.md parity): N/A — internal bugfix, no user-facing feature/gating/platform change.
• Both changed files were read in full and given a verdict; nothing deferred there.
• Build/Tests validation (Phase 7) deferred per the note above.

What's good

• Correctly fixes all three defects: swallowed concurrency conflicts, a genuine TOCTOU race in duplicate detection, and now treats the database as the single source of truth for uniqueness.
• The retry is provably safe given the handler's idempotent-only external calls and ResetTracking() forcing a fresh read each attempt.
• Test coverage is a highlight, especially the end-to-end run through the real ConcurrencyRetryBehavior.
• Small, focused diff with no scope creep.

Recommendation

Ready to merge as-is.

[claude]

Code Review: PR #333

Recommendation: APPROVE

Summary

This PR mirrors the Play-webhook hardening from #327 onto the Stripe webhook handler (src/Orbit.Application/Subscriptions/Commands/HandleWebhookCommand.cs), fixing three concurrency/idempotency defects:

1. Concurrency retry — HandleWebhookCommand now implements IConcurrencyRetryable, so DbUpdateConcurrencyException (stale xmin on a concurrently-mutated User) re-runs the handler via ConcurrencyRetryBehavior instead of surfacing as an unhandled 500. All Stripe calls in the handler are reads (SubscriptionService.GetAsync), so re-running is safe per the IConcurrencyRetryable contract.
2. Catch no longer swallows concurrency conflicts — the trailing generic catch now excludes DbUpdateConcurrencyException, so it propagates to the retry behavior instead of being turned into a silent Result.Failure.
3. TOCTOU idempotency fix — the AnyAsync(EventId == …) check-then-insert race is removed; duplicate detection now relies solely on the ProcessedStripeEvent.EventId unique index, caught by the existing DbUniqueViolation handler as an idempotent success.

Review process

• Own line-by-line pass against the rubric (correctness, concurrency semantics, xmin/idempotency handling, dead code, comment policy, cross-cutting hard rules): no Critical/High findings.
• Dedicated security-reviewer subagent pass over the diff and full current handler: PASS, no findings ≥ Medium. Confirmed the removed pre-check doesn't open a new race (unique index is the sole, safe enforcement point), confirmed no exception detail leaks through Result.Failure paths, confirmed the webhook route remains behind Stripe's signature verification (unchanged, out of diff scope).
• Test coverage is direct and meaningful: covers the conflict→retry→success path through the real ConcurrencyRetryBehavior (not just the handler in isolation), the propagate-don't-swallow path, the unique-violation idempotent-success path, and the removal of the AnyAsync pre-check.
• Checked for changes since my last review on this PR: the only new commit is a merge from main (e989c05) bringing in unrelated redaction/audit-log fixes — it does not touch the webhook/subscription files under review. No new diff to re-review.
• No DTO/endpoint contract change in this PR, so no orbit-ui-mobile consumer-side update is required (confirmed via PR description; not independently verifiable in this CI job since the sibling repo isn't checked out here).

No Critical, High, or actionable Medium findings. Clean, targeted fix — approving.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud