fix: use UUIDv7 idempotency keys for retries#79
Conversation
|
@claude /simplify Please simplify this PR aggressively. Treat optional cleanup and nits as in scope. Focus on whether the UUIDv7 idempotency helper, retry-path tests, and documentation are the simplest maintainable way to close the duplicate-creation retry risk. |
|
Claude finished @justin-layerv's task in 3m 11s —— View job Simplification review
The PR closes the duplicate-creation risk correctly. Most of the new code, however, is in service of switching the UUID variant rather than fixing a retry bug — and 1. Biggest lever: do you actually need UUIDv7?The "duplicate-creation on retry" risk is closed by any stable, unique key reused across attempts — If qurl-service explicitly asked for time-ordered keys: keep it, but please call that out in the PR description (today it reads like UUIDv7 is required for retry safety, which it isn't). If they didn't: revert to 2.
|
Review — UUIDv7 idempotency keysSolid, focused change. I verified the core mechanics and they hold up. What's correct
Worth addressing1. PUT vs. the existing rationale (minor, but the comment now reads inconsistently). 2. No intra-millisecond monotonic counter (non-blocking, just noting). Nit
Nothing here is blocking. The PUT comment is the only thing I'd nudge before merge. |
0d35d2f to
be99e5b
Compare
|
Addressed the first Claude review feedback in
@claude /simplify Please re-run simplify on the updated head. Treat any remaining optional cleanup or nits as actionable. |
|
Claude finished @justin-layerv's task in 2m 34s —— View job /simplify pass on
|
be99e5b to
227f2ea
Compare
Code Review — UUIDv7 idempotency keysOverall this is a clean, well-scoped change with strong test coverage. The key is generated once before the retry loop ( Correctness ✅
Observations / minor
Test coverage ✅Good additions: sync + async network-error retry regressions (the #19 path), Nice work — no blocking concerns from me. |
|
Addressed the first
@claude /simplify Please re-run simplify on the updated head and call out any remaining nits or optional cleanup. |
|
Claude finished @justin-layerv's task in 3m 4s —— View job /simplify — round 2 on
|
227f2ea to
e510961
Compare
|
Final clarification pushed in @claude /simplify Please run a final simplify pass on |
Review —
|
|
Claude finished @justin-layerv's task in 3m 0s —— View job Final /simplify pass on
|
Review —
|
e510961 to
1bb7404
Compare
|
Addressed the final simplify hygiene notes in
@claude /simplify Please run one final simplify pass on |
|
Claude finished @justin-layerv's task in 2m 43s —— View job /simplify — final pass on
|
Code Review — PR #79: UUIDv7 idempotency keys for retriesReviewed the diff, the Correctness ✅
Security ✅Using Notes / nits (non-blocking)
Test coverage ✅Strong: parametrized UUIDv7 generation for POST/PUT/PATCH, explicit-header preservation, and both sync and async network-error retry-reuse cases. Matches the test plan in the description. Nice work — the change is tight and the reasoning in the comments (why DELETE is excluded, why POST 5xx isn't auto-retried) is genuinely helpful. |
1bb7404 to
5a12cd8
Compare
|
Addressed the latest
@claude /simplify Please run the final simplify pass on |
|
Claude finished @justin-layerv's task in 3m 48s —— View job /simplify — final pass on
|
Review —
|
5a12cd8 to
dec85a4
Compare
|
Addressed the latest automatic review nits in
@claude /simplify Please run a final simplify pass on |
|
Claude finished @justin-layerv's task in 2m 17s —— View job /simplify — final pass on
|
Review — PR #79: UUIDv7 idempotency keys for retriesNice, focused change. The UUIDv7 bit layout is correct (48-bit ms timestamp, version Minor notes (non-blocking)
Verification
LGTM once the |
|
Settling the remaining PUT question from the latest Claude review: yes, including |
Summary
Idempotency-Keyvalues for SDK-managed POST/PATCH mutation calls and future PUT endpoints, matching the qurl-service/API requirement from Send Idempotency-Key on POST/PUT/PATCH so retries are safe #47Business relevance
This reduces duplicate resource creation risk for qURL mutation calls, especially batch create flows where a mid-flight network failure can otherwise replay a write and create more items than the caller intended. The retry-safety mechanism is stable key reuse across attempts; UUIDv7 aligns the SDK-generated keys with the service-side idempotency-key format requested in #47.
Notes
This keeps the existing POST HTTP 5xx retry posture intact: automatic POST status-code retries remain limited to 429, as guarded by existing tests, because
/v1/resolvecan have server-side side effects. The bug from #19 is the network-error retry path, and this PR adds direct sync/async coverage there.Fixes #47
Addresses #19
Test plan
python3 -m ruff check .PYTHONPATH=src python3 -m mypy src/PYTHONPATH=src PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python3 -m pytest -p pytest_asyncio.plugin tests/ -v