test(api): cover idempotency-key claim races (Plan 006 follow-up) - #385
Conversation
Plan 006. The Idempotency-Key flow on state PUT/DELETE was read-then- mutate-then-store: two concurrent requests with the same key could both miss the initial read, both run the mutation (each appending a state_events row), and then silently lose the second idempotency record to INSERT OR IGNORE. Idempotency only protected sequential retries, not the concurrent-retry case (client timeout + retry racing the original) it exists for. Replace readIdempotency/storeIdempotency with claim-first functions: claimIdempotency atomically inserts a pending row (INSERT ... ON CONFLICT DO NOTHING) before the mutation runs, so exactly one concurrent request becomes the writer; losers replay the completed response, hit 409 IDEMPOTENCY_CONFLICT on a hash mismatch (unchanged), or 409 IDEMPOTENCY_IN_FLIGHT if the winner hasn't finished yet. completeIdempotency fills in the real response after a successful mutation; releaseIdempotencyClaim deletes the pending row if the mutation fails, so a client retry isn't stuck behind a dead claim. Route handlers wrap the mutation in try/finally so any failure path (service error or thrown exception) releases the claim. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two new integration tests to verify the atomicity and resilience of the idempotency key mechanism under concurrent requests and lease guard blocks. The review feedback correctly identifies a logical flaw in the concurrent test's status code assertion, where sorting the status array lexicographically creates dead code and can mask unexpected server errors (such as a 500 Internal Server Error). A robust assertion pattern is suggested to ensure both requests return expected statuses.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const statuses = [first.status, second.status].sort(); | ||
| // Both acceptable outcomes for the loser: it replays the winner's 200, or | ||
| // it is told the winner is still in progress (409 IDEMPOTENCY_IN_PROGRESS). | ||
| expect(statuses[0] === 200 || (statuses[0] === 409 && statuses[1] === 200)).toBe(true); |
There was a problem hiding this comment.
The current assertion has two issues:
Array.prototype.sort()sorts lexicographically by default. Even if sorted numerically, since200 < 409,200will always be at index0and409at index1. Therefore, the conditionstatuses[0] === 409 && statuses[1] === 200is logically impossible and dead code.- Because the assertion only checks if
statuses[0] === 200, if one request succeeds with200and the other fails with500(or any other status code), the sorted array will be[200, 500].statuses[0]is200, so the test will incorrectly pass, masking a potential server error.
We should use the same robust assertion pattern used elsewhere in this file (e.g., lines 426-428) to ensure both statuses are strictly within the expected set of [200, 409] and at least one is 200.
| const statuses = [first.status, second.status].sort(); | |
| // Both acceptable outcomes for the loser: it replays the winner's 200, or | |
| // it is told the winner is still in progress (409 IDEMPOTENCY_IN_PROGRESS). | |
| expect(statuses[0] === 200 || (statuses[0] === 409 && statuses[1] === 200)).toBe(true); | |
| const statuses = [first.status, second.status]; | |
| expect(statuses).toContain(200); | |
| for (const status of statuses) { | |
| expect([200, 409]).toContain(status); | |
| } |
1 participant
Summary
IDEMPOTENCY_IN_PROGRESS, shipped in fix: batch of triaged correctness bugs and small dashboard/SDK fixes #355) already covers the race this branch originally set out to fix — this branch's own source changes were superseded and dropped during rebase; only the added test cases were kept and rewritten to match main's actual implementation.Background
This branch (
claude/w4-concurrency-guards) was cut before #355 landed. Both fixed the same TOCTOU bug in idempotency-key handling independently, with different architectures. Since #355 is already merged and deployed, this PR keeps only the two new regression tests, verified against main's current implementation.Test plan
bunx tsc --noEmit -p packages/api/tsconfig.jsonbunx vitest run(29 files, 417 tests, all passing)bunx biome check packages/api/src/ packages/api/test/(no new issues; pre-existing baseline formatting errors unrelated to this change)