fix(api): retry Play RTDN concurrency conflicts and transient web-push failures

[thomasluizon]

What & why

Two disjoint resilience gaps in the notification/billing paths, hardened at source.

Task A — Play RTDN webhook (HandlePlayNotificationCommand)

Concurrent Real-Time Developer Notification deliveries that update the same user could collide on the User xmin optimistic-concurrency token and fail, because the handler had no DbUpdateConcurrencyException retry.

• The command now implements IConcurrencyRetryable, so the already-registered ConcurrencyRetryBehavior re-runs the whole (idempotent) handler on a stale-token conflict. The external Stripe write (CancelConsumedCouponAsync) only fires after a successful save, so re-execution stays side-effect-safe.
• The manual catch (DbUpdateException) no longer swallows the DbUpdateConcurrencyException subtype. Previously a concurrency conflict entered that catch, ran a wasted duplicate-message probe, and rethrew; now it excludes the concurrency subtype and handles only the genuine duplicate-messageId unique-constraint race, letting the conflict propagate to the retry behavior.

Task B — Web push delivery (PushNotificationService)

A transient blip from the push service could previously prune a perfectly healthy subscription or silently drop the notification with no retry.

• RequestPushMessageDeliveryAsync is now wrapped in a retry loop: ≤2 retries with ~300ms exponential backoff (300ms, 600ms).
• A subscription is marked stale/removed only on 410 Gone / 404 Not Found.
• Transient statuses (408, 429, 5xx) and transport-level exceptions are retried, then give up without pruning the subscription. Non-transient statuses (e.g. 400) give up immediately without a retry. User cancellation propagates unretried.

Tests

• A: marker is present; a concurrency conflict propagates without being treated as a duplicate; and — driven through the real ConcurrencyRetryBehavior — a conflict-then-success retries and grants Pro.
• B: transient (503) then success retries once and keeps the subscription; a transport exception then success retries and keeps it; 410 Gone marks stale with no retry; a non-transient 400 neither retries nor prunes.

All affected suites green locally (Orbit.Application.Tests + Orbit.Infrastructure.Tests); full solution builds with 0 errors.

No API contract, endpoint, or DTO change — purely internal hardening, so no consumer-side mirror is required.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Review Complete

Scope: PR #327 in thomasluizon/orbit-api — fix(api): retry Play RTDN concurrency conflicts and transient web-push failures (fix/play-rtdn-concurrency-and-webpush-retries → main)
Recommendation: APPROVE

This is the first review pass — no prior threads or bot reviews to reconcile against.

Severity	Count
Critical (incl. ⚠️ old-client breaks)	0
High	0
Medium	0
Low / Info	2

Findings (Low/Info only, not blocking)

• Info — PushNotificationService.SendWebPushToSubscription's catch (Exception ex) when (!ct.IsCancellationRequested) (src/Orbit.Infrastructure/Services/PushNotificationService.cs:219-228) will retry (with a small bounded delay) genuinely non-transient bugs before swallowing them with a Warning log — but this is not a new regression: the pre-PR code caught and swallowed the identical exception set unconditionally with no rethrow either; the diff just adds a bounded delay (max 2 retries, ~900ms total) in front of the same eventual outcome.
• Info — The new test Handle_ThroughRetryBehavior_ConcurrencyConflictThenSuccess_GrantsProAndRetries (tests/Orbit.Application.Tests/Commands/Subscriptions/HandlePlayNotificationCommandHandlerTests.cs:353-378) reuses the same mocked User instance across both simulated attempts, so it validates the retry wiring but not the "DB state actually rolled back" scenario ResetTracking() exists for — a reasonable Application-layer test-scope boundary, not a gap.

Analysis performed

• Traced the full idempotency chain in HandlePlayNotificationCommand.cs: message-dedup check → in-memory-only ConsumeOnNewPurchase (no external call) → SaveChangesAsync → post-save-only CancelConsumedCouponAsync (the only true external write). Confirmed this matches IConcurrencyRetryable's documented contract (external writes must resolve concurrency themselves / only fire after save) and that ConcurrencyRetryBehavior.ResetTracking() → ChangeTracker.Clear() correctly discards the failed attempt's in-memory mutations before a fresh re-fetch. If SaveChangesAsync throws DbUpdateConcurrencyException, the whole transaction rolls back — the dedup row is never committed — so the retried Handle() correctly re-runs the dedup check against still-unpersisted state and reprocesses cleanly, with no double side effects across retries.
• Confirmed the DbUpdateException catch-filter narrowing (ex is not DbUpdateConcurrencyException) is a genuine root-cause fix: previously a genuine concurrency conflict could be mis-swallowed as "duplicate message" and silently drop a legitimate entitlement update. This makes duplicate-suppression stricter, not weaker.
• Verified the web-push retry loop is bounded (MaxRetries = 2 compile-time const, no unbounded path), correctly special-cases 410/404 (prune, no retry) vs 408/429/5xx/transport errors (retry then give up without pruning) vs cancellation (propagates unretried) — matching the PR description. No attacker-controlled amplification: a broken push endpoint adds at most ~2 wasted requests + ~900ms delay per notification.
• Checked callers of IPushNotificationService.SendToUserAsync (background schedulers + command handlers) for fallout from cancellation now propagating instead of being swallowed — found ReminderSchedulerService already treats OperationCanceledException as an intentional pass-through elsewhere in the same method, so this change is consistent with existing intent, not a regression.
• Logging: unchanged severity/shape (still Debug/Warning, FCM tokens still truncated); no new sensitive fields (payload body/title, VAPID keys, auth secret) logged.
• Read both new test files in full; assertions and mock setups match the described behavior.

Subagents

Agent	Verdict
security-reviewer	PASS — independently reviewed webhook auth, concurrency/dedup interaction, retry-loop amplification, log sensitivity, and broad-catch masking across the same files; no Critical/High.
contract-aligner	N/A — no DTO, Controller route, or packages/shared type/endpoints.ts surface is touched in this diff.

Deferred

• DESIGN.md/AI-slop, Parity, i18n — N/A, frontend-only dimensions, no apps/* files in this diff.
• Contract drift + backward-compat — N/A (no added/removed/renamed field in this diff).
• FEATURES.md parity — N/A, internal reliability fix, no user-facing feature/screen/tool/gating change.
• Build/Tests — skipped per instructions; CI runs Build/Unit Tests/SonarCloud as separate required checks.

[sonarqubecloud]

Quality Gate passed

Issues
2 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

[claude]

Review Complete

Scope: PR #327 in thomasluizon/orbit-api — fix(api): retry Play RTDN concurrency conflicts and transient web-push failures (fix/play-rtdn-concurrency-and-webpush-retries → main)
Recommendation: APPROVE

Summary

Re-review triggered by the synchronize event after bc8a9567 merged main into this branch. That merge commit touches only unrelated files (AuthController.cs, GoogleAuthCommand.cs, SkipHabitCommand.cs, BulkSkipHabitsCommand.cs, ResendContactsService.cs and their tests, from PR #326) — none of the four files in this PR's actual diff. gh pr diff confirms the reviewed diff (HandlePlayNotificationCommand.cs +3/-2, PushNotificationService.cs +55/-27, plus the two test files) is byte-identical to what my prior review (submitted 2026-07-12T03:46:02Z) already examined. Per the pr-review protocol ("focus on the diff since your last review"), there is nothing new to re-review — this reconfirms the prior APPROVE rather than re-deriving it.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

• Info — PushNotificationService.SendWebPushToSubscription's catch (Exception ex) when (!ct.IsCancellationRequested) (src/Orbit.Infrastructure/Services/PushNotificationService.cs:219-228) retries (bounded, max 2 attempts, ~900ms total) genuinely non-transient bugs before swallowing them with a Warning log — not a new regression: the pre-PR code caught and swallowed the same exception set unconditionally with no rethrow either; the diff only adds a bounded delay in front of the same eventual outcome.
• Info — Handle_ThroughRetryBehavior_ConcurrencyConflictThenSuccess_GrantsProAndRetries (tests/Orbit.Application.Tests/Commands/Subscriptions/HandlePlayNotificationCommandHandlerTests.cs:353-378) reuses the same mocked User instance across both simulated attempts, so it validates the retry wiring but not "DB state actually rolled back and re-fetched" — a defensible Application-layer test-scope boundary (that scenario belongs to an Infrastructure/integration test, which this repo intentionally does not maintain), not a gap.

Independently re-traced the idempotency chain for the retry marker: HandlePlayNotificationCommand re-fetches the user fresh via userRepository.FindOneTrackedAsync on every Handle() invocation; PlayReferralCouponConsumer.ConsumeOnNewPurchase only mutates the in-memory tracked entity (no external call); the only true external write, CancelConsumedCouponAsync, fires strictly after a successful SaveChangesAsync, outside the retried scope. ConcurrencyRetryBehavior.ResetTracking() → ChangeTracker.Clear() discards a failed attempt's unpersisted mutations before the re-run, so this satisfies IConcurrencyRetryable's documented contract (idempotent, no non-idempotent external side effects inside the retry loop). The narrowed catch (DbUpdateException ex) when (ex is not DbUpdateConcurrencyException && ...) correctly stops swallowing concurrency conflicts as duplicate-message noise while still catching the genuine unique-constraint duplicate-messageId race. The web-push retry loop is compile-time bounded (MaxRetries = 2), correctly separates 410/404 (prune, no retry) from 408/429/5xx/transport errors (retry then give up without pruning) and from cancellation (propagates unretried).

Subagents

Agent	Verdict
security-reviewer	N/A this pass — reused prior pass's PASS (no Critical/High across concurrency/dedup interaction, retry-loop amplification, log sensitivity, broad-catch masking); diff unchanged since that pass.
contract-aligner	N/A — no DTO, Controller route, or packages/shared type/endpoints.ts surface touched.

Validation

Check	Result
Build (dotnet)	N/A — skipped per CI adaptation; Build/Unit Tests/SonarCloud run as separate required checks.
Tests (dotnet)	N/A — same as above.

Deferred — N/A dimensions & files not verdicted

• DESIGN.md/AI-slop, Parity, i18n — N/A, frontend-only dimensions; no apps/* files in this diff.
• Contract drift + backward-compat guard — N/A, no added/removed/renamed field in this diff (PR body confirms no API/DTO change).
• FEATURES.md parity — N/A, internal reliability hardening, no user-facing feature/screen/tool/gating change.
• Second-opinion (/second-opinion, GLM-5.2 via opencode) — UNAVAILABLE in this CI runner (no opencode); moot regardless since no Critical finding survived to need it.

What's good

Root-caused both gaps cleanly: the concurrency retry is wired through the existing IConcurrencyRetryable/ConcurrencyRetryBehavior mechanism rather than a bespoke retry loop, and the web-push retry correctly distinguishes permanent-dead (410/404), transient (408/429/5xx/transport), and non-transient (e.g. 400) outcomes instead of a blanket catch. Both new test suites assert the actual described behavior (propagation vs. duplicate-suppression; retry-then-success vs. retry-exhausted vs. no-retry-on-dead vs. no-retry-on-non-transient) rather than just happy-path coverage.

Recommendation

Merge as-is. No action required before merge; the two Info notes are non-blocking and already accounted for in the diff's own design trade-offs.