fix(api): idempotent offline-mutation replay + transactional account deletion

[thomasluizon]

Backend half of the #243 Phase 2b iteration 2 reliability batch. Paired with thomasluizon/orbit-ui-mobile (mobile PR linked below). Merge this PR first, then the mobile PR (deploy-API-first).

What & why (verified against the code, not the report's severities)

1. Idempotent offline-mutation replay — code-quality High

The mobile offline queue is at-least-once and sent no idempotency key, so any dropped network ACK (routine on mobile) replayed a committed mutation: logHabit (a toggle) silently un-logged a completed habit; creates duplicated. Fixed with an atomic-reserve idempotency layer:

• ProcessedRequest ledger — unique (UserId, IdempotencyKey), cascade FK to User, CreatedAtUtc retention index (+ migration).
• IdempotencyBehavior (MediatR): a request carrying an Idempotency-Key header reserves the key and caches the response in the same transaction as the mutation, so a crash cannot apply a mutation without its ledger row. A replay returns the stored response; a concurrent duplicate loses the unique race and replays the winner. Registered innermost, so ConcurrencyRetry re-runs it with a fresh reservation.
• UnitOfWork.ExecuteInTransactionAsync is now reentrant — a handler that self-transacts joins the ambient transaction instead of nesting (Npgsql forbids nested BeginTransaction).
• 30-day retention cleanup in the daily job; deleted on account reset + cascade on hard delete.

2. Transactional account deletion — code-quality High

The daily AccountDeletionService job ran 29 auto-committing ExecuteDeleteAsync calls + a separate SaveChanges with no transaction — a crash mid-sequence left orphaned user PII. Wrapped the delete+remove+save in ExecuteInTransactionAsync (mirrors ResetAccountCommand; ORBIT0002-clean, no explicit rollback). Corrected the misleading IAccountResetRepository docstring.

Dropped from the batch after verifying (no code)

• unsent-push "no retry" — over-rated: failures are logged and dead tokens pruned; best-effort reminder traffic, not launch-blocking.
• habit-advancement "no idempotency guard" — false positive: CatchUpDueDate is an absolute catch-up (DueDate < today), so a second run is a no-op; it touches no streak and writes no logs.

Config (separate from this PR)

Enabling the durable job queue (BackgroundServices__UseDurableQueue=true) is a prod env decision — all 13 recurring jobs verified idempotent, so the flip is safe.

Tests

Idempotency round-trip / replay / no-key bypass / atomic rollback / Unit-response (SQLite) · concurrent-race replay + rethrow (mocked) · ExecuteInTransactionAsync reentrancy · transactional deletion of a user + owned data + ledger · retention cleanup. Full suite green (Application 2615 · Domain 503 · Infrastructure 1496).

Refs thomasluizon/orbit-ui-mobile#243

🤖 Generated with Claude Code

[thomasluizon]

Paired client PR: thomasluizon/orbit-ui-mobile#450. Merge this (backend) first, then #450.

[claude]

test

[claude]

Code Review: PR #318 — Recommendation: NEEDS WORK

(An earlier review on this PR with body "test" was posted in error by this same automated reviewer — please disregard it; this review supersedes it.)

SUMMARY: The transactional account-deletion fix is solid (ORBIT0002-clean, cascade-safe). The idempotency-replay feature's transaction/reentrancy plumbing is also correct. However, IdempotencyBehavior was tested only against plain-string/Unit response shapes, never against the Result wrapper that virtually every real command handler returns, producing two crash-class Critical bugs on the exact LogHabitCommand flow this PR calls its motivating case, plus two High security/design gaps. Two independent skeptic subagents tried to refute the two Critical findings and could not (both CONFIRMED). Cross-model second opinion unavailable (opencode not installed in this session).

CRITICAL 1: IdempotencyBehavior crashes on every expected business-logic failure. src/Orbit.Application/Behaviors/IdempotencyBehavior.cs:43,59-60 calls JsonSerializer.Serialize(response) where TResponse is routinely Result (src/Orbit.Domain/Common/Result.cs:47-49). Result.Value throws InvalidOperationException when IsSuccess is false, with no [JsonIgnore]/converter anywhere in the repo. LogHabitCommand (src/Orbit.Application/Habits/Commands/LogHabitCommand.cs) — this PR's own flagship use case — routinely returns Result.Failure(...) for ordinary conditions (habit not found/not owned/wrong date, lines 69/72/78/117/132/168/189). Any idempotency-keyed retry hitting these paths throws while caching the response; the exception is not a DbUpdateException so the behavior's own catch clause misses it, surfacing as an unhandled 500 instead of the normal 404/400. The PR's own new tests only use IRequest/Unit shapes, never Result failure, so this was never caught. Fix: special-case Result/Result serialization (only read Value when IsSuccess) via a converter or wrapper DTO, and add a test replaying a Result.Failure through the behavior.

CRITICAL 2: Concurrent replay of the same Idempotency-Key can poison the shared transaction and crash instead of replaying the winner's response. IdempotencyBehavior.cs:39-45 adds the ProcessedRequest reservation to the shared DbContext without flushing; when the wrapped handler later calls its own SaveChangesAsync (e.g. LogHabitCommandHandler.HandleLogAsync ~line 177), EF flushes the reservation together with the handler's own entities in one transaction. LogHabitCommandHandler.IsUniqueViolation (lines 264-273) and the shared DbUniqueViolation helper both only check the Postgres SQL state (23505), not which constraint/table fired — so a unique violation on the ledger's own (UserId, IdempotencyKey) index can be misrouted into the handler's own 'already logged' recovery path (BuildAlreadyLoggedResultAsync), which then issues further queries inside a transaction Postgres has already aborted, surfacing as a raw PostgresException (25P02) that the behavior's catch clause (typed to DbUpdateException) cannot match either. This is exactly the concurrent-duplicate-retry race the feature targets. Fix: don't rely on SQL-state-only unique-violation detection when multiple uniquely-constrained inserts can share one SaveChanges call — flush the reservation in its own SaveChanges before invoking next(), or inspect the failing constraint/entry rather than only the SQL state.

HIGH 3: Idempotency ledger has no request-type discriminator (src/Orbit.Infrastructure/Persistence/IdempotencyStore.cs, unique index in the AddProcessedRequests migration is (UserId, IdempotencyKey) only). IdempotencyBehavior applies to every MediatR request as an open generic; a client that reuses the same key across two different commands gets the first command's cached response returned for the second, skipping its handler (and its PayGate/validation/side-effects) entirely. Fix: scope the unique index and lookup by (UserId, IdempotencyKey, RequestType).

HIGH 4: No opt-in allowlist for idempotency caching — a one-time secret could be persisted in the ledger for 30 days. CreateApiKeyCommand (src/Orbit.Application/ApiKeys/Commands/CreateApiKeyCommand.cs) returns the raw plaintext API key by design shown once and never otherwise persisted (the ApiKeys table stores only a BCrypt hash). Nothing prevents a client from attaching Idempotency-Key to that call; if one does, the raw key is written into ProcessedRequests.ResponseBody in cleartext for up to 30 days. Fix: make idempotency caching opt-in via an explicit marker interface on commands, not opt-out-by-default for all IRequests.

SUBAGENTS: security-reviewer FAIL (findings 3 and 4 above). contract-aligner N/A — no DTO/Controller/route change in this diff; the new Idempotency-Key header is a pure addition for the paired mobile PR (orbit-ui-mobile#243).

VALIDATION: dotnet build/test: N/A — dotnet commands required interactive approval unavailable in this review session. PR description states the full suite is green locally (Application 2615, Domain 503, Infrastructure 1496) but this could not be independently re-run here.

WHAT'S GOOD: AccountDeletionService's transactional rewrite is correct and ORBIT0002-clean, mirroring ResetAccountCommand. UnitOfWork.ExecuteInTransactionAsync's reentrancy fix (joining an ambient transaction instead of nesting) is clean and well-targeted with a compliant WHY-linked comment. ProcessedRequests cascade-delete is redundantly correct (FK cascade + explicit bulk-delete on reset + 30-day time purge). Per-user ledger scoping via the JWT claim correctly prevents cross-tenant replay. Test coverage of the transaction/rollback/race plumbing itself is good — the gap is specifically the missing Result-shaped response coverage.

RECOMMENDATION: Fix the two Critical findings before merge — both are crash-class bugs reachable through this PR's own flagship use case under ordinary conditions. Address the two High findings (ledger request-type scoping, opt-in allowlist for caching) in this PR or an immediate follow-up before mobile starts sending Idempotency-Key broadly.

[claude]

Code Review: PR #318

Recommendation: Request changes — one High-severity correctness bug in the new idempotency-replay path.

Summary

Adds an opt-in MediatR IdempotencyBehavior<TRequest,TResponse> that caches a mutation's serialized response in a new ProcessedRequests ledger (keyed by UserId + Idempotency-Key), so a retried request (lost network ACK) replays the stored response instead of re-executing the handler — plus wraps the daily account-deletion job's multi-table delete in a transaction.

The transactional-deletion half and the UnitOfWork.ExecuteInTransactionAsync reentrancy fix are solid and well tested. The idempotency behavior has one real defect: it throws on replay for the ~71 commands whose response type is the bare Result class.

Findings

High

Replaying any command whose response is the bare Result type throws NotSupportedException instead of returning the cached response.

• src/Orbit.Application/Behaviors/IdempotencyBehavior.cs:62-63 (Deserialize) calls JsonSerializer.Deserialize<TResponse>(responseBody, SerializerOptions)!.
• src/Orbit.Domain/Common/Result.cs:12 — the non-generic Result base has only a protected constructor and no [JsonConstructor]. System.Text.Json's reflection converter requires a public parameterless or single public parameterized constructor; Result has neither, so deserialization throws NotSupportedException. (Result<T> is fine — its constructor is public.)
• Verified directly: 71 commands across the codebase declare IRequest<Result> (e.g. DeleteHabitCommand, SkipHabitCommand, RestoreHabitCommand, DeleteTagCommand, UnsubscribeMarketingCommand).
• Impact: if a client attaches an Idempotency-Key to any bare-Result-returning command and the request is retried — the exact scenario this PR exists to make safe — the replay throws, surfacing as an HTTP 500 on an operation that already committed successfully the first time.
• The added tests (IdempotencyBehaviorDbTests.cs, IdempotencyBehaviorRaceTests.cs) only exercise string/Unit response types, so this gap shipped untested.
• Fix: give Result a public (or [JsonConstructor]-annotated) constructor so it round-trips through System.Text.Json, and add a replay test using a real Result/Result<T>-returning handler.

Medium

Idempotency ledger stores the full response body as plaintext with no allowlist against future secret-bearing commands.

• src/Orbit.Domain/Entities/ProcessedRequest.cs:17 (ResponseBody), src/Orbit.Infrastructure/Persistence/OrbitDbContext.cs:239-247.
• ResponseBody persists whatever the handler returns as unencrypted text for up to 30 days. No response-shape check or command allowlist stops a future command whose response carries a token/secret from being cached here if it ever opts into the Idempotency-Key header. Not exploitable today (only mobile offline-queue mutations use the header currently), but nothing enforces the boundary going forward. Consider documenting or enforcing via a marker interface.

What's good

• Reservation row and handler mutation commit in the same transaction (IdempotencyBehavior.cs:39-45) — a crash mid-request can't leave a mutation applied without its ledger row.
• UnitOfWork.ExecuteInTransactionAsync reentrancy fix (joins an ambient transaction instead of nesting) is correct, with a proper WHY-with-URL comment.
• Transactional account-deletion wrap mirrors the existing ResetAccountCommand pattern and is ORBIT0002-clean (no explicit rollback).
• Thorough test coverage otherwise: replay, no-key bypass, crash rollback, concurrent-race replay + rethrow, reentrancy, transactional deletion, 30-day retention cleanup.
• No security issues with the ledger key itself: server-derived from the JWT claim (not client input), parameterized EF query, unique-violation catch correctly scoped.

Not verifiable in this environment

• Cross-repo parity (orbit-ui-mobile#450) — sibling repo not checked out.
• dotnet build/test — covered by separate required CI checks (Build / Unit Tests / SonarCloud).

[thomasluizon]

Addressed all four review findings in fedc287. Thanks — these were real; my first cut only tested string/Unit responses and missed that Orbit commands return Result<T>.

• Critical 1 (Result serialization crash): added ResultJsonConverterFactory that reads Value only on success and rebuilds via the factory methods, so Result/Result<T> success and failure round-trip. New tests replay a Result<string> success, a Result<string> failure, and a non-generic Result.
• Critical 2 (cross-constraint unique-violation): the reservation is now flushed in its own SaveChanges before the handler runs, isolating a ledger unique-violation from the handler's own constraints. A concurrent duplicate loses the race at the reservation flush and never runs the handler (race test updated to assert handlerCalls == 0).
• High 3 (no request-type discriminator): ledger key is now (UserId, IdempotencyKey, RequestType) — column + composite unique index (migration regenerated). New test: same key + two request types both execute.
• High 4 (opt-out could cache a secret): idempotency is now opt-in via IIdempotentCommand, applied only to the non-idempotent offline commands (create habit/goal/tag, logHabit, skipHabit, updateGoalProgress). CreateApiKey and friends are unmarked, so a one-time secret can never land in the ledger. New test: an unmarked request with a key bypasses the ledger.

Full suite green locally (Application 2615, Domain 503, Infrastructure 1507).

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #318

Scope: PR #318 in thomasluizon/orbit-api — re-review of the diff since the last automated review (commit 61c4f9f → fedc287, now merged as 01c4e34)
Recommendation: APPROVE

Summary

This is a follow-up review after two prior automated passes on this PR flagged, combined, 2 Critical + 3 High findings in the new idempotency-replay feature (Result-serialization crash on replay, cross-constraint unique-violation misrouting, missing request-type discriminator, opt-out-by-default caching that could leak a one-time API-key secret, and bare non-generic Result throwing NotSupportedException on deserialize). The author's fix commit fedc287 ("make idempotency Result-safe, opt-in, and race-isolated") addresses all of them. Every changed file in the diff since the last review was independently re-read, plus an adversarial skeptic pass (a security-reviewer subagent instructed to refute each "fixed" claim); nothing survived.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

None.

Verification of the prior findings

1. Result-serialization crash (Critical) — FIXED. src/Orbit.Application/Common/ResultJsonConverterFactory.cs:50-99 adds custom JsonConverters for Result/Result that read Value only when IsSuccess and rebuild via Result.Success()/Result.Failure() factory methods, avoiding both the throwing Value getter on failure and the constructor-less non-generic Result type (this also incidentally fixes the separate "bare Result → NotSupportedException" finding from the second prior review). New tests replay a Result success, a Result failure, and a bare Result — all round-trip (IdempotencyBehaviorDbTests.cs).
2. Cross-constraint unique-violation misrouting (Critical) — FIXED. src/Orbit.Application/Behaviors/IdempotencyBehavior.cs:48-49 — the ledger reservation is now flushed via its own SaveChangesAsync immediately after Reserve(), before the wrapped handler's next() runs, isolating a ledger-key race from the handler's own constraints. IdempotencyBehaviorRaceTests.cs now asserts handlerCalls == 0 on a reservation-flush race.
3. No request-type discriminator (High) — FIXED. Ledger key is now (UserId, IdempotencyKey, RequestType), enforced by a composite unique index (OrbitDbContext.cs:243, migration 20260711012225_AddProcessedRequests). New test Handle_SameKeyDifferentRequestTypes_BothExecute confirms both execute.
4. Opt-out-by-default caching of secrets (High) — FIXED. New opt-in marker IIdempotentCommand; exactly the six intended commands implement it (CreateGoalCommand, UpdateGoalProgressCommand, CreateHabitCommand, LogHabitCommand, SkipHabitCommand, CreateTagCommand). CreateApiKeyCommand (returns a plaintext API key shown once) does not implement it — confirmed by grep and by new test Handle_UnmarkedRequest_BypassesLedgerEvenWithKey.

Regression checks (adversarial pass)

• Execution-strategy/retry interaction: UnitOfWork.ExecuteInTransactionAsync joins an already-open ambient transaction rather than nesting, so the new double-SaveChangesAsync-per-request doesn't trip Npgsql's nested-transaction restriction.
• SkipHabitCommand (implements both IIdempotentCommand and IConcurrencyRetryable): a concurrency-conflict retry re-enters IdempotencyBehavior.Handle fresh with a clean reservation, since the prior attempt's transaction (including its reservation insert) is rolled back via using-scope disposal before the retry.
• Catch-clause scope: catch (DbUpdateException) when (DbUniqueViolation.IsUniqueViolation(...)) could in principle also catch an unrelated unique-constraint violation from the handler's own SaveChangesAsync, but since FindResponseBodyAsync finds no matching ledger row in that case, it correctly falls through and rethrows rather than misreporting.
• No bare narration comments introduced (ORBIT0001 — all new comments are /// XML-doc with a WHY-linked issue reference), no TODO/FIXME/HACK, ORBIT0002-clean (no explicit rollback on the using-scoped transaction).
• An independent security-reviewer skeptic subagent re-read the same files and could not refute any "fixed" claim; found no new issue (it noted a pre-existing, out-of-scope DbUniqueViolation helper duplicated between Orbit.Application/Common and Orbit.Infrastructure/Services — not introduced by this diff, not raised as a finding).

Subagents

Agent	Verdict
security-reviewer	PASS — re-verified all prior findings fixed; no new issues in the diff since last review
contract-aligner	N/A — no DTO/Controller/route change in this diff slice

Validation

Check	Result
Build (dotnet)	N/A — skipped per instructions; CI runs Build/Unit Tests/SonarCloud separately
Tests (dotnet)	N/A — skipped per instructions; PR description states full suite green locally (Application 2615, Domain 503, Infrastructure 1507)

Deferred — N/A dimensions & files not verdicted

• Parity / i18n — frontend-only; not verifiable from orbit-api, owned by the paired orbit-ui-mobile#450 review.
• DESIGN.md / AI-slop — no UI files touched.
• FEATURES.md parity — reliability bugfix to already-reviewed behavior, not a new user-facing feature surface.
• Cross-repo contract — no DTO/Controller/route hunks in this diff slice; nothing crosses the mobile contract surface.
• All 19 files in the 61c4f9f→fedc287 diff were read and given a verdict; nothing deferred within backend scope.

What's good

• All previously-flagged Critical/High bugs are genuinely fixed, verified by reading the actual code paths rather than trusting the changelog comment.
• The fix also incidentally resolves a finding from a second, independent prior review (bare Result deserialize crash) via the same root-cause converter — a good sign this addressed the underlying issue rather than patching the reported symptom.
• New/updated tests map 1:1 to the fixed defects, including the race test now asserting zero handler invocations.
• Doc comments were updated in lockstep with the behavior change; no stale documentation.

Recommendation

Approve. All previously-blocking Critical/High findings are fixed and independently re-verified; no new issues surfaced in the diff since the last review. Confirm cross-repo parity on the paired orbit-ui-mobile#450 PR separately (per this repo's own PR description: "Merge this (backend) first, then #450").