[DRK-75] Atomic idempotency key reservation

[baoduy]

[DRK-75] Atomic idempotency key reservation

Related to DRK-75 (architecture finding IDEM-CONCURRENCY-001, severity critical).

Problem

IdempotencyEndpointFilter.InvokeAsync did a non-atomic check-then-act: it read "has this key been processed?" and, only if not, invoked the protected handler. Two concurrent requests with the same idempotency key could both observe "not processed" and both execute the side-effecting operation — the exact retry scenario idempotency exists to prevent.

Fix

• IdempotencySqlServerStore.IsKeyProcessedAsync now atomically reserves the composite key (an HTTP 102 Processing placeholder row) before returning "not processed," relying on the existing UX_CompositeKey unique index to guarantee exactly one winner under concurrency.
• IdempotencyKeyEntity.Complete(CachedResponse) added; MarkKeyAsProcessedAsync now completes the reservation row in place instead of a blind insert.
• IdempotencyDistributedCacheStore mirrors the same reservation shape (documented as narrowing, not eliminating, the race — IDistributedCache has no compare-and-set).
• New IdempotencyOptions.InFlightReservationTimeout (30s default) bounds how long a reservation is honoured, so a crashed handler doesn't permanently block retries.
• IIdempotencyKeyStore.IsKeyProcessedAsync's contract doc updated to state the new atomic check-and-reserve guarantee. No public signature changes.
• Tightened CreateItem_ConcurrentRequestsWithSameKey_OnlyOneProcessed to assert exactly 1×201/4×409 instead of tolerating more than one success.

Verification

• AspCore.Idempotency.MsSqlStore.Tests (real SQL Server, GitHub Actions remote-tests.yml, run 30971952365 on this branch's current tip): 9/9 passed, including the concurrency test.
• AspCore.Idempotency.Tests (non-SQL, local): 54/54 passed.
• Clean dotnet pack for DKNet.AspCore.Idempotency and DKNet.AspCore.Idempotency.MsSqlStore.
• Known gap: no measured coverage % for IdempotencySqlServerStore.cs / IdempotencyKeyEntity.cs against the squad's ≥90% target — local TestContainers.MsSql cannot start in this sandbox (arm64 Docker networking), and remote-tests.yml doesn't collect coverage by design. Functional correctness on these files is proven (9/9 including the concurrency case); the actual number should surface once this PR's build-test-coverage.yml/SonarCloud run completes.
• 8 pre-existing failures in unrelated test projects (EfCore.Events.Tests, EfCore.AuditLogs.Tests, Svc.PdfGenerators.Tests) observed on the full-solution run — not touched by this diff, not a regression here.

Scope

Only the files in DKNet.AspCore.Idempotency, DKNet.AspCore.Idempotency.MsSqlStore, and AspCore.Idempotency.MsSqlStore.Tests listed above, plus a coverlet.collector addition to AspCore.Idempotency.Tests.csproj.

[github-actions]

📊 Code Coverage Report

| Metric | Coverage |
|--------|----------|
| **Line Coverage** | 82.6% |
| **Branch Coverage** | 80.9% |
| **Method Coverage** | 84.7% |

**Lines:** 3466/undefined covered
**Branches:** 1297/undefined covered

📈 [View Full Coverage Report](https://github.com/baoduy/DKNet/actions/runs/30972571455)

[codecov]

Codecov Report

❌ Patch coverage is 96.29630% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.14%. Comparing base (a3448e6) to head (d318a58).

Files with missing lines	Patch %	Lines
...ency.MsSqlStore/Store/IdempotencySqlServerStore.cs	95.91%	0 Missing and 2 partials ⚠️
...mpotency/Store/IdempotencyDistributedCacheStore.cs	96.29%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #340      +/-   ##
==========================================
+ Coverage   79.09%   83.14%   +4.04%     
==========================================
  Files         169      169              
  Lines        4196     4247      +51     
  Branches      613      617       +4     
==========================================
+ Hits         3319     3531     +212     
+ Misses        695      530     -165     
- Partials      182      186       +4     

Flag	Coverage Δ
unittests	83.14% <96.29%> (+4.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

📊 Code Coverage Report

| Metric | Coverage |
|--------|----------|
| **Line Coverage** | 85.3% |
| **Branch Coverage** | 81.9% |
| **Method Coverage** | 87.1% |

**Lines:** 3581/undefined covered
**Branches:** 1313/undefined covered

📈 [View Full Coverage Report](https://github.com/baoduy/DKNet/actions/runs/30973782111)

[baoduy]

Automated Review Gate — PR #340 ([DRK-75] Atomic idempotency key reservation)

Score: 7.9 / 10 → REWORK (round 1)
Built right: 8/10 · Right thing: 10/10

Summary

The fix correctly closes the non-atomic check-then-act race in IdempotencyEndpointFilter by making IsKeyProcessedAsync an atomic reserve-and-check operation. The SQL Server store leverages the existing UX_CompositeKey unique index to guarantee exactly one concurrent caller per key proceeds; the distributed-cache store mirrors the reservation shape with honest documentation of its residual race window. The implementation is clean, well-scoped, and matches the spec exactly — no scope creep, no restructured filter, no public API changes.

The single issue preventing approval: test coverage. CI measures diff coverage at 53.65%, well below the squad's ≥90% threshold. The new SQLite-backed concurrency test covers the store-level race path well, but the DistributedCacheStore changes, MarkKeyAsProcessedAsync refactor, IdempotencyKeyEntity.Complete(), the InFlightReservationTimeout edge cases, and the full reservation→completion lifecycle are untested. For a critical-severity concurrency fix, that gap matters.

Score breakdown

Category	Weight	Score	Notes
Correctness & logic	25%	9	Reservation logic sound; collision handler correctly re-queries; no logic defects
Security	20%	10	No findings
Testing & coverage	20%	8	Coverage 53.65% → −2 (important), hard cap → 7.9
Maintainability & design	15%	9	Clean; fits architecture; one nit (duplicated constant) −0.5
Spec conformance	10%	10	Matches D75-1 spec exactly; all out-of-scope constraints respected
Style & conventions	5%	10	Consistent; no new warnings
AI-slop gate	5%	10	No anti-patterns

Hard caps applied: Coverage below 90% → 7.9 max. Final: 7.9.

Findings

Blocking

None.

Important

1. [important] IdempotencyDistributedCacheStore.IsKeyProcessedAsync (diff lines 447-502), IdempotencySqlServerStore.MarkKeyAsProcessedAsync (diff lines 176-189), IdempotencyKeyEntity.Complete (diff lines 208-214), IdempotencyOptions.InFlightReservationTimeout — no new tests exercise these changes. The SQLite concurrency test covers only the store-level reservation race, not the full lifecycle (reservation → completion → re-check), the distributed-cache flow, or edge cases (expired reservation, reservation completed by a competitor before the loser's re-query returns, handler crash leaving orphaned 102 placeholder). CI measures diff coverage at 53.65%.

Nit

2. [nit] ReservationStatusCode = 102 constant is duplicated in both IdempotencySqlServerStore.cs:31 and IdempotencyDistributedCacheStore.cs:38. These are in different assemblies, making a shared constant awkward, but worth noting for future consolidation.

Praise

3. [praise] The SqliteCompatibleModelCustomizer in IdempotencySqlServerStoreConcurrencyTests.cs:164-181 — cleverly strips SQL Server-specific column types while preserving UX_CompositeKey enforcement, enabling a real relational concurrency test without Docker. Precisely the right level of test infrastructure for this constraint.

4. [praise] IdempotencyEndpointFilter.InvokeAsync was left completely untouched — the fix correctly localized the atomicity guarantee into the store methods, proving the existing filter architecture was sound.

Out-of-scope (not scored, captured for follow-up tracking)

• IdempotencyPostgresStore (src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Store/IdempotencyPostgresStore.cs:68) still has the old non-atomic check-then-act pattern. Now that IIdempotencyKeyStore.IsKeyProcessedAsync's contract documents an atomic guarantee, this store is silently violating it. Tracked separately as DRK-175; not introduced by this PR.

Auto-merge preconditions

Precondition	Status
No blocking/critical findings	✅
No secrets in diff	✅
Coverage ≥ 90% on changed lines	❌ 53.65%
No protected-path files	✅
Not a draft, base is dev	✅
CI not failing	✅
No new external sources	✅

Verdict: REWORK

The implementation is sound. One fix round: bring diff coverage to ≥90% by adding tests for the uncovered paths — specifically the distributed-cache store changes, the MarkKeyAsProcessedAsync refactor, and a full lifecycle integration test (reserve → complete → re-check) using the existing SQLite fixture. A fix ticket has been dispatched to dev-backend with concrete acceptance criteria.

[baoduy]

Automated Review Gate — PR #340, round 2 (re-armed)

Score: 6.9 / 10 → REWORK (round 2 of 2)
Built right: 8/10 · Right thing: 10/10

Verdict: BLOCKING — CI failure from merge conflict with dev

build-test-coverage (10.x.x) FAILED with compile error:

IdempotencyOptions.cs(90,21): error CS0102: The type 'IdempotencyOptions' already contains a definition for 'InFlightReservationTimeout'

Root cause: another PR merged InFlightReservationTimeout into dev while this cycle was in-flight. dev now has the property at line 81 (between IdempotencyKeyPattern and JsonSerializerOptions), and the feature branch also has it at line 73 (between IdempotencyHeaderKey and IdempotencyKeyPattern). Git doesn't flag this as a textual conflict (different positions), but the merged file has two InFlightReservationTimeout properties — a semantic duplicate that fails at compile time.

The test additions from DRK-183 look sound (332 insertions, 3 new test files, zero production code touched), but their effect can't be verified until the build passes.

Score breakdown (capped by CI failure)

Category	Weight	Score	Notes
Correctness & logic	25%	8	The new test files look correct; CI failure prevents full verification
Security	20%	10	No findings
Testing & coverage	20%	—	Cannot verify — CI build failed before tests ran; no codecov result
Maintainability & design	15%	9	Test additions well-structured; merge conflict is temp infra
Spec conformance	10%	10	DRK-183 diff is test-only, matches constraint
Style & conventions	5%	10	Consistent
AI-slop gate	5%	10	No anti-patterns in test additions

Hard cap: CI failing → 6.9 max. Final: 6.9.

Findings

Blocking

1. [blocking] CI build-test-coverage failed — duplicate InFlightReservationTimeout on merge with dev. dev's IdempotencyOptions.cs:81 added the same property (via an already-merged PR) at a different position than the feature branch's IdempotencyOptions.cs:73. This is a git/semantic merge conflict — not caught textually, fails at compile.

Praise

2. [praise] IdempotencySqlServerStoreLifecycleTests.cs (203 lines) — comprehensive lifecycle coverage: reserve, complete, re-check, in-flight collision, expired reservation. Exactly the coverage gap from round 1's review.
3. [praise] IdempotencyDistributedCacheReservationTests.cs (115 lines) — exercises the distributed-cache reservation path (miss, in-flight, completed), filling the second major gap.

What went well, what's needed

The test additions from DRK-183 are well-structured and address all the coverage gaps I flagged in round 1. The fix is a one-line rebase to reconcile InFlightReservationTimeout with dev's now-canonical version (keep dev's placement + doc, remove the feature branch's duplicate). No code changes beyond that — the tests themselves don't need modification.

Auto-merge preconditions

Precondition	Status
No blocking findings	❌ CI compile failure
CI not failing	❌ build-test-coverage failed
Coverage ≥ 90%	Unknown (CI didn't reach tests)
All others	✅

Gate action

This is a git/branch-mechanics issue (rebase needed to reconcile with external dev changes), not a code defect. Routing to dev-leader per git/PR-mechanics findings contract — the implementer is forbidden from opening or editing PRs.

@dev-leader — branch needs a rebase on dev to remove the duplicate InFlightReservationTimeout (keep dev's version at line 81). Once CI passes and codecov reports diff coverage, the review re-arms for a third attempt (rework cap is 2, so this will be the final gate pass or escalate).

[github-actions]

📊 Code Coverage Report

| Metric | Coverage |
|--------|----------|
| **Line Coverage** | 86.6% |
| **Branch Coverage** | 82.4% |
| **Method Coverage** | 87.5% |

**Lines:** 3682/undefined covered
**Branches:** 1333/undefined covered

📈 [View Full Coverage Report](https://github.com/baoduy/DKNet/actions/runs/30977809406)

[baoduy]

Automated Review Gate — PR #340, final pass

Score: 9.6 / 10 → APPROVED & MERGED
Built right: 9/10 · Right thing: 10/10

PR merged into dev via gh pr merge --merge at commit 31fcfbc.

Score breakdown

Category	Weight	Score	Notes
Correctness & logic	25%	9	Atomic reservation correct; collision handler sound; no logic defects
Security	20%	10	No findings
Testing & coverage	20%	10	Coverage 96.29% (codecov/patch). Lifecycle, concurrency, distributed cache, collision edge cases, options defaults all exercised
Maintainability & design	15%	9	Clean; fits architecture; one duplicated-constant nit (−0.5)
Spec conformance	10%	10	Matches D75-1 spec exactly; all out-of-scope constraints respected
Style & conventions	5%	10	Consistent; zero new warnings
AI-slop gate	5%	10	No anti-patterns

Findings

Nit

1. [nit] ReservationStatusCode = 102 duplicated in IdempotencySqlServerStore.cs:31 and IdempotencyDistributedCacheStore.cs:38. Different assemblies, minor concern.

Praise

2. [praise] IdempotencySqlServerStoreLifecycleTests.cs — full lifecycle coverage (reserve, complete, re-check, in-flight collision, expired reservation), precisely the gaps flagged in round 1.
3. [praise] IdempotencyDistributedCacheReservationTests.cs — exercises the distributed cache reservation path with coverage of the reservation timeout edge case via InFlightReservationTimeout = 1ms.
4. [praise] IdempotencyEndpointFilter.InvokeAsync left completely untouched — atomicity correctly localized to the store methods.
5. [praise] SqliteCompatibleModelCustomizer — clever test infrastructure preserving UX_CompositeKey enforcement without Docker.

Auto-merge preconditions — all passed

Precondition	Status
No blocking/critical findings	✅
No secrets in diff	✅
Coverage 96.29% ≥ 90%	✅
No protected-path files	✅
Not a draft, base is dev	✅
CI green	✅
No new external sources	✅

Vote skipped (PR self-authored by baoduy).