feat: store a fingerprint of the request and refuse a key reused for another - #30
Merged
Conversation
…another A key reused with a different payload replayed the first result, with no signal that anything was off. IdempotencyRecord gained fingerprint, stored beside the result and compared on a hit and on a pending reservation; a mismatch raises the new IdempotencyKeyReuseError, which carries both fingerprints and propagates out of coordinate() and the decorator. coordinate() takes idempotency_fingerprint as its one keyword; the decorator takes fingerprint_params, the names of the parameters whose bound values, defaults applied, are hashed with the new fingerprint_of(). Either side without a fingerprint means no comparison, so records already stored and callers who do not opt in keep the key-only behaviour. Closes #27
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Built on the #26 branch, and rebased onto master once #28 was squashed there; the branch is one commit on top of
0dbf68b.After a charge of 1999 was recorded under
order-42, the same key came back with amount 5 and got the 1999 charge back, with nothing to say that the request was a different one. The key is the whole identity by design, and a well-behaved client never does this; a client that derives its key from the order id and then makes a second, different request on the same order does, and the library answered it with a result for another request. Your script on master, second block:IdempotencyRecordgainedfingerprint: str | None = None, stored beside the result; a record already in Redis reads back withNone, so nothing is migrated.coordinate()takesidempotency_fingerprint: str | None = Noneas its one keyword, declared after*argsso that it stays out of the action's**kwargs— that name rather thanfingerprintbecause everything keyword-shaped incoordinate()is forwarded to the action, which makes the name one the action can never have again, andfingerprintis a plausible parameter in an auth-shaped service (a device fingerprint) whereidempotency_fingerprintmirrorsidempotency_keyand is not. The fingerprint goes onto the pending record as well as the completed one, and the comparison happens on the read, before the pending check and before the action: a record whose fingerprint differs from the caller's raisesIdempotencyKeyReuseError, carryingoperation,key,stored_fingerprintandfingerprint, out ofcoordinate()and the decorator. It is a caller error, not storage trouble, so it is the second exception those two let through, next toIdempotencyInProgressError; the HTTP layer maps it to 422 as Stripe does, and it is counted asrecord_error(operation, "key_reuse")since a client reusing keys is worth an alert. Comparing on the pending record matters: a waiter with a different payload is refused at once rather than handed someone else's result after waiting for it. Either side missing means no comparison, so a record without a fingerprint never raises and a caller without one gets the key-only behaviour — nothing changes for anyone who does not opt in. Inin_flight="run"mode the check is still on the read; a mismatch that only shows on the collision after both callers ran is logged, counted, and the caller keeps its own result, because raising after the side effect would make the caller retry a completed operation.The decorator takes
fingerprint_params: tuple[str, ...] | None = None, the names of the parameters that identify the request, in the same shape askey_paramandinfra_param. It binds the call to the signature with defaults applied, so an argument passed at its default and one left out agree; turns the named values into JSON-able Python with pydantic'sto_jsonable_python, which covers Pydantic models, dataclasses, UUIDs, datetimes and Decimals without a case per type; serialises with sorted keys and no whitespace so equal dicts built in different orders agree; and takes the SHA-256 hex, a 64-character string no payload can bloat a record with. A name the function does not have, or a signatureinspectcannot describe, raisesTypeErrorat decoration time. The hashing is public asfingerprint_of(**values), exported from the root, so a caller ofcoordinate()and the decorator agree on what "the same request" means for one operation; that is the one new name beyond the error, the field and the two arguments. The decorator passesidempotency_fingerprintonly whenfingerprint_paramsis set, so an existing assertion on how it callscoordinate()keeps holding.Rejected: hashing every argument by default, which would close the whole class of mistake unasked — a request often carries something that legitimately differs between identical retries (a timestamp, a trace id,
self), and a default that rejects honest retries as key reuse is worse than the documented status quo, so it is opt-in per operation with the parameters named. A callable form on the decorator alongside the tuple: the tuple covers the case in the issue,fingerprint_ofpluscoordinate()covers any other notion of sameness, and one shape is easier to hold than two.Docs: rule 1 on the agents page keeps the key as the identity and gains the fingerprint as the guard for when the key is wrong; rules 23 and 24 say what to fingerprint and that
idempotency_fingerprintis the coordinator's keyword; the wiring example, the API tables, the errors table and a new Common-mistakes example follow. The user guide has a "Key reuse and fingerprints" section with the decorator, thecoordinate()form, the 422 mapping and the semantics; the API reference gains the fields,fingerprint_ofand an entry forasync_idempotentit did not have; the README and quickstart mention it.CHANGELOG.mduntouched.Tests:
tests/unit/core/test_fingerprint.pyholds your scenario against a fake Redis — 1999 charged under the key withfingerprint_of(amount=1999), the key back withfingerprint_of(amount=5)raisingIdempotencyKeyReuseErrorwith both fingerprints on it and the action not run — then the same fingerprint replaying, no fingerprint replaying as before, a record stored without a fingerprint (the pre-change JSON shape, written straight into Redis) replaying for a caller with one, a caller without one replaying a record that has one, a pending record with another fingerprint refusing the waiter at once, thekey_reusecount, and bothrun-mode paths.test_entities.pydecodes the pre-change JSON shape withfingerprint is None;test_idempotent.pychecks the decorator handscoordinate()the fingerprint of the named arguments, that a defaulted argument passed and omitted agree, and the twoTypeErrors at decoration;fingerprint_ofhas its own cases for key order, differing values, the non-JSON types and the digest shape; the reuse also runs against real Redis in the integration suite. As a negative control againstorigin/masterin a worktree: the module cannot collect there (ImportError: cannot import name 'IdempotencyKeyReuseError'), and a call with only the new keyword fails asTypeError: charge_card() got an unexpected keyword argument 'idempotency_fingerprint'— on master the keyword goes to the action.The gate, before touching anything on this branch and again at the end:
(On master after #28: 151 passed, 96.07%; the warning is the pre-existing
testcontainers.redisdeprecation.)uv.lockuntouched.Your script does not pass a fingerprint, so a copy of it does,
fingerprint_of(amount=1999)on the two concurrent calls andfingerprint_of(amount=5)on the reuse, and prints the exception; against this branch in a venv withuv pip install -e ".[redis]" "testcontainers[redis]":The unmodified script against the same branch still prints
requested amount 5, got back: charge_id='ch_1' amount=1999for the second block — no fingerprint, key-only, as documented.Closes #27