Skip to content

fix: use UUIDv7 idempotency keys for retries#79

Merged
justin-layerv merged 1 commit into
mainfrom
justin/qurl-python-idempotency-key
Jun 12, 2026
Merged

fix: use UUIDv7 idempotency keys for retries#79
justin-layerv merged 1 commit into
mainfrom
justin/qurl-python-idempotency-key

Conversation

@justin-layerv

@justin-layerv justin-layerv commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • generate UUIDv7 Idempotency-Key values 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 #47
  • preserve caller-provided idempotency keys and reuse the generated key across internal retry attempts
  • add sync and async network-error retry regressions for POST create calls

Business 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/resolve can 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

@justin-layerv

Copy link
Copy Markdown
Collaborator Author

@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

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @justin-layerv's task in 3m 11s —— View job


Simplification review

  • Read the diff and the touched files
  • Audit the UUIDv7 helper for simplification opportunities
  • Audit the retry-path tests for redundancy
  • Audit the README change
  • Post the 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 client.py/async_client.py aren't touched at all, which is the first signal that the bug was already structurally solved (the headers dict is built once outside the retry loop in client.py:1463) and what's actually new here is (a) the UUID variant, (b) PUT joining the idempotency set, (c) regression tests for the network-error branch. The simplest version of this PR would lean into that and stop coupling the regression tests to the UUID variant.

1. Biggest lever: do you actually need UUIDv7?

The "duplicate-creation on retry" risk is closed by any stable, unique key reused across attemptsuuid4() already satisfied that and the headers dict is built outside the retry loop (src/layerv_qurl/client.py:1463, async_client.py:1442), so reuse is structurally guaranteed regardless of variant. UUIDv7 only buys you time-ordering, which is a server-side ergonomics win (index locality, time-windowed dedup, sortable observability) — not a correctness lever.

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 uuid4() and delete _uuid7() (24 lines), the README edit, the _assert_uuid7 helper, and most of the test-shape churn. That collapses the PR to ~3 substantive lines (add PUT to IDEMPOTENCY_METHODS, two regression tests).

2. _assert_uuid7 is called in 5 places; it only needs to run once

tests/test_client.py:1112,1142,1180,1276,1314 — every retry/integration test now re-checks the UUID structure. The retry tests exist to prove "key generated once, reused across attempts." That's orthogonal to which UUID variant is used.

Suggestion: in the four retry tests, restore assert len(first_key) == 36 (what the test asserted before — it's still a fine sanity check). Keep the structural assertion only in the focused unit test (test_ensure_mutation_idempotency_generates_uuid7_for_supported_methods). One assertion of intent, four assertions of behavior, no coupling between them.

Fix this →

3. test_ensure_mutation_idempotency_generates_uuid7_for_supported_methods only tests PUT

tests/test_client.py:1175 — the plural name promises POST/PUT/PATCH coverage but only exercises PUT. Either:

  • rename to ..._generates_uuid7_for_put (PUT was the only newly-added method, so this is honest), or
  • pytest.mark.parametrize("method", ["POST", "PUT", "PATCH"]) and keep the broader assertion.

The first is the simpler choice given the actual test body.

4. README leaks an implementation detail into the public contract

README.md:71 — promising "UUIDv7" in user-facing docs invites callers to depend on parsing the embedded timestamp, which is exactly the kind of contract the SDK shouldn't promise (and which makes a future variant swap a breaking change). Either keep the previous wording (just "per-call Idempotency-Key") or, if you want to highlight ordering, phrase it as a property qurl-service observes rather than a variant the SDK guarantees forever.

Fix this →

5. _uuid7() has two pieces of defensive code with no payoff

src/layerv_qurl/_utils.py:261:

  • timestamp_ms & ((1 << 48) - 1) — the mask is dead until year ~10889. time.time_ns() // 1_000_000 already fits in 48 bits. Drop it.
  • getattr(uuid, "uuid7", None) hot-swap — none of the currently supported Pythons (3.10–3.13, per pyproject.toml) expose uuid.uuid7. The branch is dead on every supported runtime. Fine to keep as forward-compat for 3.14, but if you're aggressively simplifying, drop it and swap to uuid.uuid7() directly when 3.14 becomes the floor.

Either change alone is minor; together they cut the helper roughly in half.

6. Sync + async POST network-retry tests — keep both

tests/test_client.py:1115,1283 — these exercise the httpx.TransportError branch (client.py:1487, async_client.py:1466), which is a different code path from the 429 status-code branch the existing tests cover. That's the explicit gap from #19. The duplication between sync and async is unavoidable given how patch("layerv_qurl.client.time.sleep") vs patch("layerv_qurl.async_client.asyncio.sleep") differ — parametrizing across them is more friction than it saves. No change needed.


Net suggestion

If UUIDv7 is a real requirement from qurl-service: apply (2), (3), (4), and optionally (5). PR shrinks by ~25 lines without losing anything.

If UUIDv7 is not a hard requirement: revert to uuid4(), drop the helper, drop the README change, drop _assert_uuid7, keep only IDEMPOTENCY_METHODS += {"PUT"} and the two regression tests. PR shrinks to about a quarter of its current size and the bug-fix narrative becomes much clearer.
· branch justin/qurl-python-idempotency-key

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review — UUIDv7 idempotency keys

Solid, focused change. I verified the core mechanics and they hold up.

What's correct

  • Bit-packing is right. I reconstructed _uuid7() independently: 48-bit ms timestamp at bits 80–127, version nibble 0x7 at 76–79, 12-bit rand_a at 64–75, variant 0b10 at 62–63, 62-bit rand_b at 0–61 — 128 bits total, and uuid.UUID(int=value) parses to version == 7 / variant == RFC_4122. Matches RFC 9562.
  • Key is generated once, not per attempt. ensure_mutation_idempotency runs before the retry loop (client.py:1463), so the same key is reused across retries — exactly what the new sync/async network-retry tests assert. Good.
  • Forward-compatible with a future stdlib uuid.uuid7 via the getattr probe, and secrets gives crypto-strength randomness (negligible collision risk with 74 random bits/ms).
  • Test coverage is good: sync + async network-error retry reuse, PUT auto-generation, and explicit-key preservation are all covered, and the _assert_uuid7 helper validates version+variant rather than just length.

Worth addressing

1. PUT vs. the existing rationale (minor, but the comment now reads inconsistently).
The comment above IDEMPOTENCY_METHODS (_utils.py:108–110) justifies excluding DELETE because "repeated deletes are safe by HTTP semantics." PUT is also idempotent by HTTP semantics (full replace), yet this PR adds it to the set. The change itself is harmless/defensive — it protects against a service that treats PUT non-idempotently — but the stated reasoning no longer cleanly explains why PUT gets a key while DELETE doesn't. Consider a one-line note clarifying the intent (e.g. PUT gets a key to give qurl-service a stable replay signal even though HTTP treats it as idempotent).

2. No intra-millisecond monotonic counter (non-blocking, just noting).
rand_a/rand_b are fully random, so two keys minted in the same millisecond aren't ordered. This is RFC-compliant (the monotonic counter is optional) and irrelevant for idempotency-key usage, so no action needed — flagging only so the omission is a documented choice rather than an oversight.

Nit

  • A direct unit test exercising the fallback path (forcing uuid.uuid7 absent) would lock in the hand-rolled packer specifically, rather than relying on the stdlib probe happening to be None on current Pythons. The existing tests cover behavior on today's runtimes, but the fallback is the code that's actually doing the work — worth pinning.

Nothing here is blocking. The PUT comment is the only thing I'd nudge before merge.

@justin-layerv justin-layerv force-pushed the justin/qurl-python-idempotency-key branch from 0d35d2f to be99e5b Compare June 12, 2026 05:30
@justin-layerv

Copy link
Copy Markdown
Collaborator Author

Addressed the first Claude review feedback in be99e5b:

  • clarified why PUT receives an idempotency key while DELETE does not
  • added direct coverage for the hand-rolled UUIDv7 fallback path

@claude /simplify

Please re-run simplify on the updated head. Treat any remaining optional cleanup or nits as actionable.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @justin-layerv's task in 2m 34s —— View job


/simplify pass on be99e5b

The two prior nits are resolved cleanly — the IDEMPOTENCY_METHODS comment now explains why PUT carries a key while DELETE doesn't, and test_ensure_mutation_idempotency_uuid7_fallback_path pins the hand-rolled packer directly (works on both pre- and post-3.14 runtimes thanks to create=True). Bit-packing in _uuid7() still checks out: 48 ts + 4 ver + 12 rand_a + 2 var + 62 rand_b = 128 bits, no overlap.

Two small actionable simplifications remain.

1. Drop the callable() check in _uuid7src/layerv_qurl/_utils.py:261

stdlib_uuid7 = getattr(uuid, "uuid7", None)
if callable(stdlib_uuid7):
    return str(stdlib_uuid7())

uuid is the stdlib module — if uuid7 is present it's a function, and the only realistic way the attribute becomes non-callable is the test patching it to None, which is not None already rejects. The callable guard adds a layer of defense against a scenario that doesn't exist in practice. Simpler:

stdlib_uuid7 = getattr(uuid, "uuid7", None)
if stdlib_uuid7 is not None:
    return str(stdlib_uuid7())

The existing fallback test (which patches to None) keeps passing.

Fix this →

2. Drop the module-level qurl_utils alias — tests/test_client.py:19

import layerv_qurl._utils as qurl_utils
...
with patch.object(qurl_utils.uuid, "uuid7", None, create=True):

The alias is used only at line 1187, only to reach qurl_utils.uuid. But _utils.py and test_client.py both import uuid and therefore share the same module object — patch.object(uuid, "uuid7", None, create=True) (using the test's existing uuid import at tests/test_client.py:7) patches the same attribute the helper reads via getattr(uuid, ...). Drops the dedicated module alias for a single test.

Fix this →

Considered, not flagged

  • & ((1 << 48) - 1) ts mask — practically dead until ~year 10889, but encodes RFC 9562's mandated 48-bit truncation. Keep.
  • Sync/async test pairs duplicate the 201 response literal — sync/async splits force most of it; collapsing into a helper would save ~5 lines for one extra layer of indirection. Not worth it.
  • Repeated first_key = ... == calls[1].headers["idempotency-key"] idiom — same call: helpers would obscure what each test asserts. Keep inline.
  • No intra-ms monotonic counter — flagged last round, still a documented RFC-permitted choice for idempotency-key use. No action.

Neither suggestion is blocking; both are pure cleanup.


  • Pull the latest diff and confirm what changed since the first review
  • Re-evaluate the UUIDv7 helper, retry-path tests, and docs for residual simplification opportunities
  • Post the updated review
    • Branch: justin/qurl-python-idempotency-key

Comment thread tests/test_client.py Fixed
@justin-layerv justin-layerv force-pushed the justin/qurl-python-idempotency-key branch from be99e5b to 227f2ea Compare June 12, 2026 05:32
@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review — UUIDv7 idempotency keys

Overall this is a clean, well-scoped change with strong test coverage. The key is generated once before the retry loop (client.py:1463, async_client.py:1442) so reuse across retries is correct, and caller-provided keys are still preserved. A few notes below.

Correctness ✅

  • The _uuid7() bit layout is correct: 48-bit ms timestamp << 80, version nibble 0x7 << 76, 12-bit rand_a << 64, variant 0b10 << 62, and 62-bit rand_b — exactly 128 non-overlapping bits, matching RFC 9562. The tests asserting version == 7 / variant == RFC_4122 confirm this.
  • Using secrets.randbits rather than random is the right call for idempotency keys — it avoids predictable/guessable keys.
  • Graceful detection of stdlib uuid.uuid7 (Python 3.14+) via getattr(..., callable) with a fallback is a nice touch, and the fallback path is directly tested.

Observations / minor

  1. PUT is currently a no-op. Adding "PUT" to IDEMPOTENCY_METHODS is forward-looking — the SDK doesn't issue any PUT requests today (only GET/POST/PATCH/DELETE). That's harmless and defensible as future-proofing, but the README/comment phrasing reads as though it's an active path. Worth a one-line note that it's reserved for future endpoints, or dropping it until a PUT method exists. Either is fine.

  2. Fallback is not monotonic within a millisecond. The fallback doesn't carry a monotonic counter, so two keys minted in the same ms aren't strictly ordered. This is irrelevant for idempotency (uniqueness is what matters, and ~74 bits of entropy per ms makes collisions negligible) — just flagging it so no one later assumes time-sortability from the fallback.

  3. Fallback test patches the global uuid module. patch.object(qurl_utils.uuid, "uuid7", None, create=True) mutates the real stdlib uuid module attribute. It's scoped to the context manager so it's safe, but since qurl_utils.uuid is the stdlib module (not a local alias), the patch is process-global for its duration. Fine as written; noting for awareness.

Test coverage ✅

Good additions: sync + async network-error retry regressions (the #19 path), PUT key generation, the uuid7 fallback path, and explicit-key preservation. This closes the gap the PR targets well.

Nice work — no blocking concerns from me.

@justin-layerv

Copy link
Copy Markdown
Collaborator Author

Addressed the first /simplify findings in 227f2ea:

  • kept UUIDv7 because Send Idempotency-Key on POST/PUT/PATCH so retries are safe #47 explicitly requires SDK-generated UUIDv7 keys
  • updated the PR body to clarify UUIDv7 is service/API alignment, while retry safety comes from stable key reuse
  • removed UUIDv7 from public README wording
  • decoupled retry tests from UUID internals
  • parametrized the focused UUIDv7 test across POST/PUT/PATCH
  • simplified the hand-rolled UUIDv7 helper

@claude /simplify

Please re-run simplify on the updated head and call out any remaining nits or optional cleanup.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @justin-layerv's task in 3m 4s —— View job


/simplify — round 2 on 227f2ea

The aggressive cuts from round 1 all landed: helper is down to bit-packing only (no getattr probe, no fallback test, no mask), README is back to a stable contract, retry tests no longer reach into UUID internals, and the focused UUIDv7 test is parametrized across POST/PUT/PATCH. The PR is in good shape — what's left is truly nits.

Genuinely optional cleanup

1. _assert_uuid7 has one caller — inline it

tests/test_client.py:91 — after the decoupling, the helper is called exactly once (line 1183). At one call site, the helper costs more lines than it saves:

@pytest.mark.parametrize("method", ["POST", "PUT", "PATCH"])
def test_ensure_mutation_idempotency_generates_uuid7_for_supported_methods(
    method: str,
) -> None:
    headers: dict[str, str] = {}
    ensure_mutation_idempotency(method, headers)
    parsed = uuid.UUID(headers["Idempotency-Key"])
    assert parsed.version == 7
    assert parsed.variant == uuid.RFC_4122

Net: -4 lines (helper + blank), +2 lines at the call site. Defensible to keep the helper purely for the documenting name — flagging because aggressive simplification would inline it.

Fix this →

2. Define _uuid7 before ensure_mutation_idempotency

src/layerv_qurl/_utils.py:252,261ensure_mutation_idempotency references _uuid7 defined immediately after it. Works fine (Python resolves at call time), but swapping the order matches the rest of the file's convention of defining helpers before their callers and removes the forward reference. Two-line move, no behavior change.

Pre-existing nit, not in scope but worth flagging for a follow-up

3. CodeQL: mixed import layerv_qurl._utils as utils and from layerv_qurl._utils import ...

tests/test_client.py:23,2528,2552,2611,2686,3322,3336 — this warning is not introduced by this PR (git log shows the pattern dates to the initial commit, 2d8c91d). The six existing import layerv_qurl._utils as utils callsites could be folded into the top-level from layerv_qurl._utils import (...) block to silence CodeQL, but that's tangential cleanup for a separate PR — landing it here would dilute this PR's narrative.

Confirmed: don't change these

  • secrets.randbits(12) + secrets.randbits(62) could be one randbits(74) call split by mask/shift, but that obscures the RFC 9562 field layout. The current form reads like the spec — keep.
  • Sync vs. async POST network-retry tests are structurally duplicated (test_post_network_retry_reuses_auto_idempotency_key + test_async_post_network_retry...). Parametrizing across them fights the time.sleep vs. asyncio.sleep patching split — not worth it. Keep both.
  • assert len(first_key) == 36 in the four retry tests is the right shape — proves "a UUID got generated" without coupling to v7 internals.

Net suggestion

(1) and (2) together save ~6 lines and one forward reference. Both are stylistic — landing this PR as-is is fine. If you want the cleanest possible diff, inline _assert_uuid7 and swap the helper order in _utils.py. (3) is a clean follow-up PR.


  • Pull the latest diff vs origin/main and identify what changed since be99e5b
  • Re-evaluate the UUIDv7 helper, retry-path tests, and docs for residual simplification opportunities
  • Address the CodeQL note about mixed import and from import of layerv_qurl._utils
  • Post the updated review
    • Branch: justin/qurl-python-idempotency-key

@justin-layerv justin-layerv force-pushed the justin/qurl-python-idempotency-key branch from 227f2ea to e510961 Compare June 12, 2026 05:33
@justin-layerv

Copy link
Copy Markdown
Collaborator Author

Final clarification pushed in e510961: the PUT entry is explicitly marked as future-facing per #47, while current executable behavior remains POST/PATCH plus the network retry tests.

@claude /simplify

Please run a final simplify pass on e510961. I am looking for a clean result or any remaining nits/optional cleanup to address before merge.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review — fix: use UUIDv7 idempotency keys for retries

Solid, focused change. The UUIDv7 implementation is correct, the key is generated once before the retry loop so it's stably reused across attempts (verified in client.py:1463 / async_client.py:1442), and the new sync + async network-retry regressions directly cover the #19 path. Nothing blocking. A few minor notes below.

Correctness ✅

  • _uuid7() bit layout matches RFC 9562: 48-bit unix_ts_ms, 4-bit version (0x7), 12-bit rand_a, 2-bit variant (0b10), 62-bit rand_b = 128 bits. uuid.UUID(...).version == 7 and variant == RFC_4122 hold.
  • Using secrets.randbits (vs random) is a nice touch — keys are unpredictable, and 74 random bits make intra-millisecond collisions negligible.
  • Hand-rolling UUIDv7 is justified: requires-python = ">=3.10", and stdlib uuid.uuid7() only landed in 3.14.

Minor suggestions (non-blocking)

  1. PUT is added to IDEMPOTENCY_METHODS but no PUT request exists in the client (all mutations are POST/PATCH). It's harmless and forward-looking, but currently dead config. Worth noting that a PUT would fall into the wider RETRYABLE_STATUS set (429/502/503/504) via method == \"POST\" being false at client.py:1521 — intended per your comment, but entirely unexercised. Consider either adding minimal coverage or a comment flagging it as forward-looking.
  2. Network-retry tests assert len(first_key) == 36 only. Reusing the existing _assert_uuid7() helper there would also pin version/variant of the auto-generated key end-to-end, not just length. The dedicated test_ensure_mutation_idempotency_generates_uuid7_for_supported_methods covers the format, so this is optional polish.
  3. _uuid7() keys aren't monotonic within the same millisecond (no rand_a increment / counter). Fine for idempotency keys — uniqueness is all that matters here — just flagging that they shouldn't be relied on for ordering.

Style/tests

  • Reordering the if TYPE_CHECKING block below the layerv_qurl imports is consistent with import grouping. ✅
  • README left unchanged, matching the stated intent of not exposing UUID internals as a user-facing contract. ✅

Nice work on the retry-safety mechanism and the targeted regression coverage.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @justin-layerv's task in 3m 0s —— View job


Final /simplify pass on e510961

  • Pull the latest diff and read what changed since 227f2ea
  • Re-audit _uuid7(), idempotency wiring, and tests for residual cleanup
  • Post the result

The PR is in good shape. _uuid7() is now lean (no probe, no mask), the README change is reverted, the qurl_utils alias is gone, the focused test is parametrized over POST/PUT/PATCH, and the retry tests are decoupled from UUID internals (just len(first_key) == 36). Everything I'd previously flagged is addressed.

Two tiny nits remain — both optional, neither blocking.

1. _assert_uuid7 is now down to a single call site — tests/test_client.py:91, 1183

The helper exists to assert UUIDv7 structure on the generated header. After decoupling the retry tests from UUID internals, the only remaining call is in the parametrized focused test:

def _assert_uuid7(value: str) -> None:               # line 91
    parsed = uuid.UUID(value)
    assert parsed.version == 7
    assert parsed.variant == uuid.RFC_4122
...
    _assert_uuid7(headers["Idempotency-Key"])        # line 1183 — only call

A 5-line helper with one call site is more indirection than the inline three lines. Inlining keeps the assertion local to the test that owns it:

@pytest.mark.parametrize("method", ["POST", "PUT", "PATCH"])
def test_ensure_mutation_idempotency_generates_uuid7_for_supported_methods(method: str) -> None:
    headers: dict[str, str] = {}
    ensure_mutation_idempotency(method, headers)
    parsed = uuid.UUID(headers["Idempotency-Key"])
    assert parsed.version == 7
    assert parsed.variant == uuid.RFC_4122

Fix this →

2. if TYPE_CHECKING block ended up in an unusual spot — tests/test_client.py:51-52

The diff moved it from right after the third-party imports to after the from layerv_qurl.types import block:

import respx                       # third-party

from layerv_qurl import (...)      # local
from layerv_qurl._utils import (...)
from layerv_qurl.errors import (...)
from layerv_qurl.types import (..., Webhook)

if TYPE_CHECKING:                  # ← stdlib import, sitting after locals
    from collections.abc import AsyncGenerator

collections.abc is stdlib, so by convention the TYPE_CHECKING block belongs with the stdlib group (just after from unittest.mock import patch at line 10) or at the location it had before the PR (between third-party and local). Putting a stdlib import block after the local imports is the least conventional placement — likely a side-effect of the editor, not intentional.

Fix this →

Considered, not flagged

  • CodeQL "mixed import and from import" — already resolved in this commit; only import uuid remains. ✅
  • Magic constants in _uuid7() (0x7, 0b10, 12, 62, 76, 80, 64) — RFC 9562 layout. Named constants would add code, not reduce it. Keep inline.
  • time.time_ns() // 1_000_000 — idiomatic and precise. No mask needed (already removed); won't overflow 48 bits until year ~10889.
  • Sync/async POST network-retry tests duplicate the 201 response literal — sync/async splits force most of the shape; extracting a helper would be more friction than the duplication. Keep.
  • Async test inline AsyncQURLClient(...) construction — matches the existing pattern at tests/test_client.py:1008, 1230, 1252. No deviation.

If you'd rather merge as-is, neither nit changes behavior — they're cosmetic only. Otherwise both are one-shot edits.
• Branch: justin/qurl-python-idempotency-key

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review — fix: use UUIDv7 idempotency keys for retries

Solid, well-tested change. The core mechanism is correct: ensure_mutation_idempotency is called once at client.py:1463 before the retry loop, so the generated key is genuinely reused across attempts — the new sync/async network-retry tests confirm this directly. Nice to see both transports covered.

Correctness

  • _uuid7() bit layout is correct. 48-bit ms timestamp / 4-bit version (0x7) / 12-bit rand_a / 2-bit variant (0b10) / 62-bit rand_b = 128 bits, matching RFC 9562. Using secrets over random is harmless (slightly slower, but key generation isn't hot). A manual implementation is justified given requires-python = ">=3.10"uuid.uuid7() only landed in 3.14.
  • Nit: timestamp_ms isn't masked to 48 bits. It only overflows into the version nibble past year ~10889, so this is purely theoretical — flagging for completeness, not asking for a change.

IDEMPOTENCY_METHODS adding "PUT"

The new comment says DELETE is excluded because "repeated deletes are safe by HTTP semantics" — but PUT is also idempotent by HTTP semantics, so the rationale doesn't cleanly distinguish why PUT is now in and DELETE stays out. Combined with there being no PUT endpoints in the SDK today, this addition is speculative and currently untestable end-to-end (the parametrized unit test exercises the helper directly, which is good, but nothing routes a real PUT). Worth either a one-line note on why PUT specifically needs a service-side replay key, or deferring it until a PUT endpoint actually exists. Low priority — it's a single harmless set member.

Tests

Good coverage. The assert len(first_key) == 36 checks are a bit thin on their own, but _assert_uuid7 (version + RFC_4122 variant) and the parametrized POST/PUT/PATCH test cover the format properly. The TYPE_CHECKING import reorder is cosmetic and fine.

Minor / PR hygiene

The description's fourth bullet mentions README guidance, but the diff only touches _utils.py and test_client.py — no README change is present. Likely just a stale bullet; worth trimming so reviewers don't go looking for it.

Nothing blocking here from my side.

🤖 Generated with Claude Code

@justin-layerv justin-layerv force-pushed the justin/qurl-python-idempotency-key branch from e510961 to 1bb7404 Compare June 12, 2026 05:36
@justin-layerv

Copy link
Copy Markdown
Collaborator Author

Addressed the final simplify hygiene notes in 1bb7404:

  • trimmed the stale README bullet from the PR body
  • clarified that future PUT endpoints get qurl-service replay keys so retried writes map to one logical operation

@claude /simplify

Please run one final simplify pass on 1bb7404. I am looking for clean/no remaining findings, including nits and optional cleanup.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @justin-layerv's task in 2m 43s —— View job


/simplify — final pass on 1bb7404

  • Pull the latest diff and identify what changed since e510961
  • Re-examine the UUIDv7 helper, retry-path tests, and PR body for residual nits
  • Post the updated review

Verdict: clean. Nothing actionable remains. Three prior simplify rounds removed every cut I could justify — what's left I would not change.

What I looked at on this pass and explicitly kept:

  • _uuid7() (src/layerv_qurl/_utils.py:262) — body is 9 lines of bit-packing that map 1:1 to RFC 9562 field layout (48 ts / 4 ver / 12 rand_a / 2 var / 62 rand_b). The earlier defensive bits (getattr probe, callable check, 48-bit mask, fallback test) are all gone. Cannot shrink further without obscuring the spec.
  • IDEMPOTENCY_METHODS comment (_utils.py:110–113) — four lines for a one-line constant, but the PUT-vs-DELETE rationale was the thing that confused two prior reviewers. The comment is load-bearing now. Keep.
  • _assert_uuid7 (tests/test_client.py:91, one caller at 1183) — round 2 flagged inlining this and Justin chose to keep it. Inlining saves ~3 lines but loses the intent-documenting name at the call site. Taste-level either way; not worth flipping again.
  • _uuid7 defined after ensure_mutation_idempotency — round 2 suggested reordering. Not addressed, and that's fine: the same forward-reference pattern already exists in this file (build_query_params at _utils.py:218 calls _query_value defined at :223). No file-wide convention to enforce.
  • assert len(first_key) == 36 in the four retry tests (tests/test_client.py:1112,1142,1221,1244,1279,1317) — deliberately decoupled from UUIDv7 internals in round 2. Right shape: retry tests prove reuse, the focused parametrized test (:1175) proves format.
  • Sync/async POST network-retry pair (:1115 and :1286) — duplication is forced by the time.sleep vs asyncio.sleep patching split. Parametrizing fights the patch, doesn't simplify.
  • POST-create response payload literals duplicated across tests — pre-existing convention throughout test_client.py (:222, :254, :289, :1097, :1485, :1510, ...). Refactoring would be a global test-fixture pass unrelated to this PR's scope.
  • if TYPE_CHECKING block moved below the layerv_qurl imports — clean stdlib/third-party/first-party grouping. Good as-is.
  • PR body — README bullet trimmed, PUT replay-key motivation added. Reads consistently with the diff now.

No findings, no nits worth chasing. Ship it.


• Branch: justin/qurl-python-idempotency-key

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review — PR #79: UUIDv7 idempotency keys for retries

Reviewed the diff, the _uuid7 bit layout against RFC 9562, and the call site. This is a clean, well-tested change. Approving in spirit; a few notes below, none blocking.

Correctness ✅

  • UUIDv7 bit packing is correct. 48-bit unix_ts_ms (<< 80), 4-bit version 0x7 (<< 76), 12-bit rand_a (<< 64), 2-bit variant 0b10 (<< 62), 62-bit rand_b = 128 bits, matching RFC 9562. The parametrized test asserting version == 7 and variant == RFC_4122 guards this well.
  • Key reuse across retries holds. ensure_mutation_idempotency is invoked once before the for attempt in range(...) loop (client.py:1463, async_client.py:1442), so the generated key persists in request_headers across attempts. The new sync/async ConnectError-then-201 tests assert this directly — good regression coverage for the POST retries on network errors can duplicate items (batch_create especially) #19 network-error path.

Security ✅

Using secrets.randbits rather than random gives unpredictable keys. Not strictly required for idempotency semantics (uniqueness is what matters), but it's the right defensive default and costs nothing here.

Notes / nits (non-blocking)

  1. PUT is added speculatively. There are no PUT endpoints in the SDK today (grep confirms PUT only appears in this new constant/comment). The comment documents the intent, so this is low-risk, but it's behavior added for a contract that doesn't exist yet — worth confirming the qurl-service PUT endpoints are actually planned before this ships, otherwise consider deferring it to the PR that introduces them.
  2. No monotonic counter within a millisecond. RFC 9562 allows an optional per-ms counter for sortability. Omitting it is fine for idempotency keys (74 bits of randomness makes intra-ms collisions negligible), but if these keys are ever used for time-ordering server-side, that's a gap. A one-line comment noting "uniqueness only, not monotonic" would set expectations.
  3. Optional: uuid.uuid7() exists in the stdlib as of Python 3.14, but requires-python = ">=3.10", so the hand-rolled helper is justified. A brief comment to that effect would help a future reader who wonders why it isn't uuid.uuid7().
  4. The TYPE_CHECKING block reordering in test_client.py is cosmetic and unrelated to the change — harmless, just noting it.

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.

@justin-layerv justin-layerv force-pushed the justin/qurl-python-idempotency-key branch from 1bb7404 to 5a12cd8 Compare June 12, 2026 05:39
@justin-layerv

Copy link
Copy Markdown
Collaborator Author

Addressed the latest /simplify cosmetic nits in 5a12cd8:

  • inlined the single-use UUIDv7 assertion helper
  • moved the TYPE_CHECKING-only stdlib import block back with the stdlib imports

@claude /simplify

Please run the final simplify pass on 5a12cd8. I am looking for clean/no remaining findings, including nits and optional cleanup.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @justin-layerv's task in 3m 48s —— View job


/simplify — final pass on 5a12cd8

  • Pull the latest diff and confirm what changed since 1bb7404
  • Re-audit _uuid7(), idempotency wiring, and tests for residual cleanup
  • Post the result

Both round-4 cleanups landed cleanly: _assert_uuid7 is inlined into its one call site (tests/test_client.py:1175-1179), and the if TYPE_CHECKING: ... AsyncGenerator block is back next to the stdlib imports (tests/test_client.py:12-13). Re-verified the bit packing in _uuid7() independently — 48-bit unix_ts_ms + 4-bit version 0x7 + 12-bit rand_a + 2-bit variant 0b10 + 62-bit rand_b = 128 bits, no overlap, matches RFC 9562. time.time_ns() // 1_000_000 fits in 48 bits until year ~10889, so the elided mask is genuinely unnecessary. Key reuse across retries still holds: ensure_mutation_idempotency runs once before the loop at src/layerv_qurl/client.py:1463 and src/layerv_qurl/async_client.py:1442, and the two new sync/async network-error tests assert it directly against httpx.ConnectError (a TransportError subclass that exercises the right branch at client.py:1487 / async_client.py:1466).

Findings

Nothing remaining. No findings, no nits, no optional cleanup.

Considered, not flagged

  • _uuid7 defined after ensure_mutation_idempotency (_utils.py:259:262). Python resolves at call time, so the forward reference is harmless; defining the consumer immediately above its private helper is a defensible mini-pattern. Not worth a move.
  • if TYPE_CHECKING: block sits between stdlib runtime imports and third-party imports. That's the placement the round-3 pass explicitly asked for ("colocate the TYPE_CHECKING stdlib import with the other stdlib imports"). The alternative — pushing all TYPE_CHECKING blocks to the end of imports — is also conventional, but flipping back now would just undo a deliberate choice.
  • "PUT" still has no real call site. Documented as forward-facing per Send Idempotency-Key on POST/PUT/PATCH so retries are safe #47 in the IDEMPOTENCY_METHODS comment at _utils.py:110-114. The parametrized helper test (POST/PUT/PATCH) exercises the codepath even though no SDK method emits a PUT today. Stays.
  • No intra-millisecond monotonic counter. RFC 9562 makes the counter optional, and the use case is replay protection, not ordering. Documented choice across prior rounds. No action.
  • Sync/async POST network-retry tests have duplicated 201 response literals. Sync vs. async patch targets (layerv_qurl.client.time.sleep vs. layerv_qurl.async_client.asyncio.sleep) force most of the divergence; extracting a fixture would cost more lines than it saves.
  • assert len(first_key) == 36 in the four retry tests. Right shape — proves "a UUID got generated and reused" without coupling to v7 internals. Format coverage lives in the focused parametrized test.

The PR is ready to merge from a /simplify standpoint.
• Branch: justin/qurl-python-idempotency-key

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review — fix: use UUIDv7 idempotency keys for retries

Solid, focused change. The UUIDv7 generation is correct and well-tested. A few non-blocking observations below.

Strengths

  • Correct UUIDv7 bit layout. Verified the construction in _uuid7: 48-bit ms timestamp << 80, version nibble 0x7 << 76, 12-bit rand_a << 64, 2-bit variant 0b10 << 62, 62-bit rand_b — total 128 bits with version 7 and the RFC 4122 variant. The parametrized test asserting version == 7 / variant == RFC_4122 locks this in.
  • Hand-rolled impl is the right call. requires-python = ">=3.10" and uuid.uuid7() only lands in 3.14, so a manual generator is necessary rather than relying on stdlib.
  • Good regression coverage. Sync + async network-error (ConnectError) retry tests assert the auto-generated key is reused across attempts — directly covering the POST retries on network errors can duplicate items (batch_create especially) #19 path. Caller-provided-key preservation remains tested.

Observations (non-blocking)

  1. PUT is speculative config today. There are no PUT endpoints in either client, so adding "PUT" to IDEMPOTENCY_METHODS is currently inert. Worth being intentional that it also changes retry posture for any future PUT: retryable = RETRYABLE_STATUS_POST if method == "POST" else RETRYABLE_STATUS means PUT (like PATCH) would get the wider {429, 502, 503, 504} set. That's safe given the idempotency key, but the retry behavior is decided here implicitly rather than when the endpoint is actually added. Fine to keep; just flagging it.

  2. No intra-millisecond monotonicity. rand_a/rand_b are fully random each call, so two keys minted in the same millisecond aren't guaranteed to sort in generation order. The UUIDv7 spec lists monotonicity as optional, and for idempotency keys only uniqueness matters — so this is correct for the use case. Noting only in case the service ever relies on key ordering (it shouldn't need to).

  3. secrets over random is fine. Cryptographic randomness isn't strictly required for idempotency keys, but secrets is a harmless, slightly-more-defensible choice. 74 random bits per ms makes collisions negligible.

  4. Minor: the test file reorders the if TYPE_CHECKING: block above the third-party imports — purely cosmetic, assuming ruff check stays green (the PR notes it does).

Verdict

Looks good to merge. The core logic is correct and the regression tests meaningfully close the #19 gap. The only thing I'd ask the author to confirm is whether including PUT now (vs. when the endpoint exists) is a deliberate choice.

@justin-layerv justin-layerv force-pushed the justin/qurl-python-idempotency-key branch from 5a12cd8 to dec85a4 Compare June 12, 2026 05:41
@justin-layerv

Copy link
Copy Markdown
Collaborator Author

Addressed the latest automatic review nits in dec85a4:

  • documented why _uuid7() is hand-rolled for pre-stdlib-uuid7 Python versions
  • documented that the random UUIDv7 fields are for uniqueness, not monotonic ordering

@claude /simplify

Please run a final simplify pass on dec85a4. I am looking for clean/no remaining findings, including nits and optional cleanup.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @justin-layerv's task in 2m 17s —— View job


/simplify — final pass on dec85a4

  • Pull latest diff and identify what changed since 5a12cd8
  • Re-audit _uuid7(), idempotency wiring, and tests for residual cleanup
  • Post the result

Verdict: clean. No findings.

The only delta in dec85a4 is two docstring lines on _uuid7() — one documenting why it's hand-rolled (stdlib uuid.uuid7() is 3.14+, project supports 3.10+), one documenting that the random fields exist for uniqueness, not monotonic ordering. Both close the explanatory gaps the auto-review flagged and add zero behavioral surface. Nothing else moved.

Re-audit summary (everything I looked at and explicitly kept):

  • _uuid7() (src/layerv_qurl/_utils.py:262) — 9 lines of bit-packing that map 1:1 to RFC 9562 (48 ts / 4 ver / 12 rand_a / 2 var / 62 rand_b), now annotated with the two pieces of context that were missing. Cannot shrink further without losing spec correspondence.
  • IDEMPOTENCY_METHODS comment (_utils.py:110–113) — explains both why PUT carries a key (future qurl-service replay protection) and why DELETE doesn't (HTTP semantics). Load-bearing; keep.
  • Inlined UUIDv7 assertion in test_ensure_mutation_idempotency_generates_uuid7_for_supported_methods (tests/test_client.py:1169) — inlined in 5a12cd8, parametrized POST/PUT/PATCH. Right shape.
  • assert len(first_key) == 36 in the four retry-reuse tests — deliberately decoupled from UUIDv7 internals so the retry tests prove reuse and the focused test proves format. Right separation of concerns.
  • Sync/async POST network-retry pair (:1109, :1283) — the time.sleep vs asyncio.sleep patching split forces the duplication; parametrizing fights it. Keep both.
  • TYPE_CHECKING block ordering in tests/test_client.py:12-13 — colocated with the stdlib imports per 5a12cd8. Clean.
  • Random std import (random) — still used for retry jitter at _utils.py:1360 (random.random() * base * 0.5), so not dead. The new import secrets / import time / import uuid block sits beside it correctly.
  • PR body — bullets match the diff (no stale README mention), PUT framing is consistent with the inline comment.

Considered, not flagged:

  • secrets.randbits(12) + secrets.randbits(62) could be one randbits(74) split by mask — would obscure the RFC field layout. Keep as-is.
  • timestamp_ms not masked to 48 bits — only overflows past year ~10889; explicit mask would be noise.
  • POST-create response payload literals repeated across the new and existing 429/network tests — pre-existing convention throughout test_client.py; extracting a fixture would be a global cleanup unrelated to this PR.
  • PUT entry is currently inert — already documented as forward-looking in the inline comment per the PR body. No code path to exercise yet, which is fine.

Ship it.


• Branch: justin/qurl-python-idempotency-key

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review — PR #79: UUIDv7 idempotency keys for retries

Nice, focused change. The UUIDv7 bit layout is correct (48-bit ms timestamp, version 0x7, 12-bit rand_a, RFC-4122 variant 0b10, 62-bit rand_b = 128 bits total), and the new version == 7 / variant == RFC_4122 assertion guards it directly. Good call documenting that the random fields are for uniqueness, not monotonic ordering — that's the right tradeoff for an idempotency key. The sync + async network-error retry tests close the actual #19 gap, and key reuse across attempts is verified rather than assumed.

Minor notes (non-blocking)

  1. PUT is added speculatively. There are no PUT endpoints in the SDK today, and the comment says "future PUT endpoints from the API contract." Worth being explicit about the coupling: non-POST methods use the wider RETRYABLE_STATUS = {429, 502, 503, 504} set (client.py:1521, async_client.py:1500), so the moment a real PUT endpoint lands it will automatically opt into 5xx HTTP retries — not just network-error retries. That's presumably the intent (the idempotency key makes it safe), but it happens silently. Either confirm that's desired or hold the PUT entry until the endpoint actually exists.

  2. secrets.randbits vs random. Crypto-strength randomness isn't required for uniqueness here (74 random bits + ms timestamp already makes collisions negligible). secrets is fine and not a hot path — just noting it's stronger than needed if generation cost ever matters.

  3. Future cleanup. requires-python = ">=3.10"; once the floor reaches 3.14, _uuid7() can be dropped for stdlib uuid.uuid7(). A # TODO: replace with uuid.uuid7() when min Python >= 3.14 would flag it for later.

Verification

  • Bit layout checked against RFC 9562 — correct.
  • Confirmed retry-set selection treats PUT/PATCH with the wider 5xx set; POST stays 429-only as the PR claims.

LGTM once the PUT question above is settled.

@justin-layerv

Copy link
Copy Markdown
Collaborator Author

Settling the remaining PUT question from the latest Claude review: yes, including PUT is intentional. #47 explicitly defines the SDK-generated idempotency surface as POST/PUT/PATCH. There are no PUT endpoints today, so the entry is inert now, but when a PUT endpoint lands it should carry the same qurl-service replay key and safely use the non-POST retry set. The inline comment and PR body now call this out as future-facing API-contract coverage.

@justin-layerv justin-layerv self-assigned this Jun 12, 2026
@justin-layerv justin-layerv merged commit 400e197 into main Jun 12, 2026
11 checks passed
@justin-layerv justin-layerv deleted the justin/qurl-python-idempotency-key branch June 12, 2026 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Send Idempotency-Key on POST/PUT/PATCH so retries are safe

2 participants