fix(api): stop concurrent refresh-token double-rotation + close three test gaps (#243)

[thomasluizon]

What

Closes three test-coverage gaps and fixes the real bug the third one surfaced.

Source fix — refresh-token rotation race

UserSession was missing the optimistic-concurrency token that User/Goal/Referral already carry, so two parallel RefreshSessionAsync calls with the same refresh token both rotated the row (last-write-wins) — a refresh-token reuse race where both callers minted access tokens and the losing caller's issued refresh token was silently overwritten.

• Map the Postgres xmin system-column concurrency token on UserSession (no DDL — the migration is a no-op at the SQL level, matching AddXminConcurrencyTokens).
• Translate the losing writer's DbUpdateConcurrencyException into a clean INVALID_SESSION failure in RefreshSessionAsync. RefreshSessionCommand is not IConcurrencyRetryable, so resolving it in-service (rather than letting it surface as a 409) is correct: exactly one refresh rotates, the loser is rejected, no double-rotation, no retry.

Tests (behavior + edge + failure)

• ValidationExceptionHandler — own focused test: 400 status, application/json content-type, { type, status, requestId, errors } envelope with messages grouped per property, request-id echo header, and the non-ValidationException delegation path.
• XpAwardLogBackfillHostedService — startup-delay deferral (backfill does not run inside the delay window), idempotency skip when the flag is already set, clean-run flag write + completion log, and failure-path error logging.
• AuthSessionService concurrent refresh — two refreshes racing on one token: exactly one rotates, the loser gets INVALID_SESSION, and the row is rotated once. The in-memory provider does not enforce xmin, so the conflict is injected with a save interceptor (the same shape Postgres raises on a stale token — the house pattern from ConcurrencyRetryTests).

Validation

Full api suite green locally: 4,642 passed, 0 failed (dotnet test Orbit.slnx).

Parity

Backend-internal only — no endpoint, DTO, or error-code contract change (INVALID_SESSION already exists and is already returned for other refresh failures), so no consumer-side edit is required.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #329 — fix(api): stop concurrent refresh-token double-rotation + close three test gaps

Recommendation: REQUEST CHANGES

Summary

The core fix (mapping the Postgres xmin system column onto UserSession and catching the losing racer's DbUpdateConcurrencyException as INVALID_SESSION) is architecturally sound and well-tested for the scenario it targets in isolation. However, the new catch block in AuthSessionService.RefreshSessionAsync deviates from an established, consistently-applied codebase convention — it never resets the DbContext's change tracker after catching the exception — and this leaves a poisoned tracked entity that breaks a real, reachable secondary code path (the audit-log write on POST /api/auth/operations/refresh). Verified directly against source (not just taken from subagent output): the DI scoping, the unconditional audit-write call site, and the precedent pattern all check out.

Findings

High

1. Concurrency-exception catch leaks a poisoned tracked entity into the shared per-request DbContext, breaking the next SaveChangesAsync in the same scope
src/Orbit.Infrastructure/Services/AuthSessionService.cs:70-77

try
{
    await unitOfWork.SaveChangesAsync(cancellationToken);
}
catch (DbUpdateConcurrencyException)
{
    return Result.Failure<SessionTokens>(ErrorMessages.InvalidSession);
}

When the losing racer's SaveChangesAsync throws, the UserSession entity mutated by session.Rotate(...) (line 65-68) stays tracked as EntityState.Modified with its stale xmin value — the catch returns without calling unitOfWork.DiscardChanges() or ResetTracking() (both exist on IUnitOfWork, UnitOfWork.cs:71-87).

OrbitDbContext (AddDbContext, scoped by default) and IUnitOfWork/IAgentAuditService (both explicit AddScoped, ServiceCollectionExtensions.cs:59,69, ServiceCollectionExtensions.AiServices.cs:39) resolve to the same instance within one HTTP request. AuthController.RefreshOperation (AuthController.cs:230-242) calls mediator.Send(new RefreshSessionCommand(...)) and then unconditionally, regardless of result.IsSuccess, calls RecordDirectAuthAuditAsync(...) → AgentAuditService.RecordAsync (AgentAuditService.cs:8-15), which does await dbContext.SaveChangesAsync(...) on the same context. EF Core's SaveChangesAsync re-attempts every Modified entry in the tracker, not just newly touched ones — so this second save re-attempts the stale UserSession UPDATE and throws a second, uncaught DbUpdateConcurrencyException, which ConcurrencyExceptionHandler turns into an HTTP 409. Under the exact concurrent-refresh race this PR fixes, the endpoint returns the wrong response (409 instead of the intended Unauthorized/INVALID_SESSION) and silently drops the AgentAuditLog row for that operation — an audit-trail gap on a security-relevant surface.

This is the only catch (DbUpdateConcurrencyException) site in the codebase that swallows the exception and returns without resetting tracking — ConcurrencyRetry.ExecuteAsync (src/Orbit.Application/Common/ConcurrencyRetry.cs:79-83) calls unitOfWork.DiscardChanges() before proceeding, confirming this is a deviation from house convention rather than a one-off style choice. No existing test catches it: AuthSessionServiceConcurrentRefreshTests.cs only exercises RefreshSessionAsync in isolation, and AuthControllerTests.cs mocks IAgentAuditService, so neither touches a real DbContext after the caught exception.

Fix: add unitOfWork.DiscardChanges(); (or ResetTracking()) inside the catch (DbUpdateConcurrencyException) block before returning. Add a regression test that performs a second SaveChangesAsync on the same context afterward (ideally through the real audit-write path) to close the gap.

Medium / Low

None.

What's good

• The xmin-token approach for UserSession correctly reuses the existing pattern used for User/Goal/Referral.
• Correctly not marking RefreshSessionCommand as IConcurrencyRetryable — blindly retrying would just replay the same stale token.
• The new test suites (ValidationExceptionHandlerTests, XpAwardLogBackfillHostedServiceTests, AuthSessionServiceConcurrentRefreshTests) close real gaps and correctly use the house "save interceptor" pattern to simulate xmin conflicts.
• No secrets/PII logged, no narration comments, no dead code, migration correctly mirrors precedent.

Not verifiable in this environment

• Cross-repo (orbit-ui-mobile / packages/shared) impact — N/A anyway, this PR touches no DTO or route surface.
• Build/test run — skipped per CI (Build / Unit Tests / SonarCloud already required checks on this PR).

Recommendation

Add unitOfWork.DiscardChanges() (or ResetTracking()) to the catch block in AuthSessionService.RefreshSessionAsync, plus a test proving a subsequent save on the same context succeeds after the caught conflict.

[claude]

Review Summary

Scope: PR #329 (thomasluizon/orbit-api) — focused on the diff since the last bot review
Recommendation: APPROVE

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

Summary: A prior review from this bot (submitted against commit 1a0332f) flagged one High finding: the catch (DbUpdateConcurrencyException) in AuthSessionService.RefreshSessionAsync (src/Orbit.Infrastructure/Services/AuthSessionService.cs:70-77) left a poisoned Modified UserSession entity tracked on the shared scoped DbContext, which would make the very next SaveChangesAsync in the same request (the unconditional post-refresh AgentAuditLog write in AuthController.RefreshOperation) throw a second, uncaught DbUpdateConcurrencyException — turning the intended INVALID_SESSION failure into an HTTP 409 and silently dropping the audit row.

Commit 2c80e8581e614a4cbb944f78cc8bdc91861c1811 ("fix(api): reset change tracker after refresh concurrency conflict") fixes exactly this: it adds unitOfWork.DiscardChanges(); before the return (AuthSessionService.cs:76), matching the house convention already used in ConcurrencyRetry.ExecuteAsync (src/Orbit.Application/Common/ConcurrencyRetry.cs:81). Independently verified:

• UnitOfWork.DiscardChanges() (src/Orbit.Infrastructure/Persistence/UnitOfWork.cs:71-82) correctly flips Modified/Deleted entries to Unchanged and Added to Detached, so the stale UserSession won't be re-submitted on the next SaveChanges in the same scope.
• The new regression test RefreshSessionAsync_AfterConcurrencyConflict_LeavesContextCleanForSubsequentAuditWrite (tests/Orbit.Infrastructure.Tests/Services/AuthSessionServiceConcurrentRefreshTests.cs) exercises the real vulnerable path: it asserts no Modified UserSession remains, then calls the actual AgentAuditService.RecordAsync (src/Orbit.Infrastructure/Services/AgentAuditService.cs:8-15, confirmed scoped OrbitDbContext) on the same context and asserts it does not throw — precisely closing the gap the prior review identified.
• Security review: PASS — the losing racer's stale refresh token is genuinely dead (its rotated hash is never persisted or returned), no distinguishing error signal between "raced and lost" vs. "already invalid" (both return the same generic INVALID_SESSION), no tokens/secrets logged, no [Authorize] posture change (no controller touched in this PR).
• Contract/parity check: N/A — no DTO, route, or packages/shared surface changed; confirmed by the diff (only Infrastructure persistence/services + Infrastructure tests). Cross-repo verification against orbit-ui-mobile not performed (repo not checked out in this job) — not applicable here since no contract surface changed.
• Comment policy: the migration's WHY comment (AddUserSessionXminConcurrencyToken.cs:13-14) is a URL-linked note (links to Npgsql docs), the one exception the narration-comment rule allows.
• Backward-compat guard: N/A — no field add/remove/rename anywhere in this diff.
• Migration .Designer.cs snapshot: standard auto-generated EF boilerplate, consistent with the hand-written migration.

What's good: The fix directly targets the exact reachable secondary code path the previous review traced through source (DI scoping, unconditional audit write, precedent pattern), and the new test proves it against the real AgentAuditService rather than a mock — closing the gap the earlier review noted.

Deferred (out of scope for this diff): ValidationExceptionHandlerTests.cs and XpAwardLogBackfillHostedServiceTests.cs are unchanged since the prior review, which already verdicted them clean. Build/test execution skipped — CI runs Build / Unit Tests / SonarCloud as separate required checks.

[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

[claude]

Code Review: PR #329 — fix(api): stop concurrent refresh-token double-rotation + close three test gaps (#243)

Recommendation: APPROVE

Summary

This PR maps the Postgres xmin optimistic-concurrency token onto UserSession (mirroring the existing User/Goal/Referral pattern byte-for-byte) and translates the losing writer's DbUpdateConcurrencyException in AuthSessionService.RefreshSessionAsync into a clean INVALID_SESSION failure with DiscardChanges(), closing a refresh-token double-rotation race. It also adds three unit-test suites (validation-exception envelope, XP backfill hosted-service orchestration, concurrent-refresh semantics) that reproduce the exact production sequence via save interceptors and a real AgentAuditService write on the same scoped DbContext.

All 8 changed files were read in full: 2 migration files, OrbitDbContextModelSnapshot.cs, OrbitDbContext.cs, AuthSessionService.cs, and 3 new test files. No commits since the prior bot APPROVE (2026-07-12T05:56:50Z) touch this PR's own files — the branch's newest commit only merges unrelated changes in from main (e.g., the User deactivation query filter, #324); nothing new to re-review.

Findings

Critical / High / Medium: None.

Low / Info: None posted — one theoretical observation (UnitOfWork.DiscardChanges() resets every tracked entry on the shared DbContext, not just the conflicting entity, and doesn't reload CurrentValues from the DB) was independently raised by both my review and a dedicated security-reviewer subagent. Confirmed not concretely actionable for this PR: RefreshSessionCommand doesn't implement IIdempotentCommand, so it never runs through IdempotencyBehavior (the only other code path that layers pending writes onto a shared context), and the only other tracked entity in this call path (User, loaded read-only) is never mutated. Per the rubric's signal gate, this is future-proofing on code that already works today, not a finding.

Security notes

• Token leak: on DbUpdateConcurrencyException, RefreshSessionAsync returns immediately before ever surfacing the losing writer's newRefreshToken to a caller — no plaintext token leak on the failure path, and the old (already-consumed) refresh token cannot be replayed by either party since the loser's TokenHash lookup no longer matches any row.
• Authorization: no Controllers touched by this diff; AuthController's refresh endpoint and its [AllowAnonymous]/audit-logging shape are unchanged.
• Migration safety: AddColumn<uint>("xmin", type: "xid", rowVersion: true) is a true no-op at the SQL level (Npgsql special-cases the xmin system column) — structurally identical to the two prior xmin migrations. Safe for rolling deploy; the only transitional effect is that old (pre-migration-model) instances won't apply the WHERE xmin = ... guard on their own writes until they redeploy, a self-resolving window, not a data-loss risk.

Validation

Build/Unit Tests run as separate required CI checks (Build, Unit Tests, SonarCloud) per this workflow's CI adaptation — not re-run here. PR description states 4,642/4,642 tests passing locally.

Deferred — N/A dimensions

• Dimensions 8–10, 14 (DESIGN.md/AI-slop, web↔mobile parity, i18n, FEATURES.md parity): N/A — backend-only bugfix + test coverage, no UI or user-facing feature surface.
• Dimension 11 (contract drift / backward-compat guard): N/A — no DTO, Controller route, or packages/shared type touched; not verifiable in this CI job regardless (sibling orbit-ui-mobile repo not checked out).

What's good

• Minimal-risk copy of an already-proven house pattern (byte-for-byte structural match to AddXminToReferral), not a novel mechanism.
• New tests reproduce the exact production sequence (save-interceptor-injected conflict, then a real AgentAuditService.RecordAsync write on the same DbContext) rather than mocking around it.
• No dead code, no workarounds, no scope creep.