Conversation
…wo concurrent sessions t13's token is stateless, so it can only bound how LONG an approval is good for. inside that window the same token verifies every time it's presented - for a transfer, that's the same approval moving money twice. at-most-once needs state. - consume() is one statement: UPDATE ... WHERE jti = :jti AND consumed_at IS NULL AND expires_at > :now RETURNING. that single statement IS the security decision; a SELECT followed by an UPDATE leaves exactly the window this exists to close - the concurrency test runs the race for real: two sessions, A holds the row lock uncommitted, B's identical statement is asserted to BLOCK, then A commits and B matches zero rows. verified the test has teeth by swapping consume() for read-then-write: B still blocked, then overwrote A's claim and also won - so the predicate is what does the work, not the lock - deliberately the opposite trade-off from T9's at-least-once delivery: the token is consumed BEFORE the tx is built (T17), because a transfer the human retries beats a transfer that happened twice, whereas a duplicate alert beats a missing one. same compare-and-set, opposite direction, because the cost of the duplicate is what changed - at-most-once is a property of COMMITTED work: a rolled-back consumer leaves the token spendable, or a crash between claim and broadcast would burn a human's approval with nothing to show for it. asserted - register() surfaces the one-live-token-per-proposal index as a value, not an exception, and does it inside a SAVEPOINT so a collision doesn't cost the audit row T16 is writing in the same unit of work - supersede_live() retires a live token so re-approval replaces authority instead of adding it - and retires EXPIRED-but-unconsumed rows too, recording consumed_reason='expired' rather than 'superseded'. that's the hole the t12 review found by reading the partial index: it can only test consumed_at (a partial index can't reference now()), so without this path a proposal whose token timed out unused would be permanently unapprovable - the store never commits; the caller owns the transaction boundary, because consuming a token and recording what was done with it are one unit of work - a failed CAS is classified by a follow-up read, which is safe because it's labelling an already-made refusal for the audit trail, not deciding anything gate green, 241 passed.
… integrity error, superseded != replay
the sharper of the two mediums is one i'd have defended wrongly: token.py argues
at length that attribution belongs inside the mac ("an audit trail that can be
edited by the caller is not an audit trail"), and then register() took an
unauthenticated operator_id and dropped claims.subject entirely. Consumed
.operator_id is what T17 attributes a transfer to, so a t16 mix-up between acting
and approving operator would move money against the wrong human undetectably.
- register() takes NO operator argument now: it resolves the operator by the
mac-protected subject, so there is nothing for a caller to get wrong. an
unknown subject is UNKNOWN_OPERATOR - an unattributable authorization to move
money is not an authorization
- _classify_integrity_error re-raises anything it doesn't recognise instead of
reporting DUPLICATE_JTI. jti is a fresh uuid4, so a real pk collision is
effectively impossible - meaning in production that value would almost always
be some OTHER violation relabelled (a missing operator row, a not-null, a
constraint added next year), with t16 writing the plausible lie into the
approval trail. verified by the reviewer with a real fk violation
- consume() now reads consumed_reason, so a superseded token reports SUPERSEDED
rather than ALREADY_CONSUMED: someone clicking a stale approval link is
ordinary operation, and filing it as a replay sends an operator hunting an
attack that never happened
- register() takes `now` and refuses a token that is already dead on arrival. a
backdated expires_at would otherwise park an unconsumable token in the one live
slot for its proposal, blocking every later approval until someone thought to
supersede it
- the fallback in _why_not_claimable described an unreachable case: a race lost to
a consumer that later rolled back makes the CAS SUCCEED (which the rollback test
demonstrates). the path that does reach it is a concurrent register committing
between our update and our select, and the honest label for that is
UNKNOWN_JTI - at the moment of the claim there was nothing to claim
- gate/__init__ no longer re-exports: an eager import of replay_store meant
importing the PURE token module pulled in sqlalchemy and the orm metadata,
quietly undoing the property token.py claims for itself. store/__init__ now
names replay_store as the one deliberate exception to "gate never touches
sqlalchemy directly", and why - its guarantee IS a specific statement, and
hiding that behind a repository puts the mechanism where a reader of the gate
can't see it
- the race test's blocking assertion could pass vacuously: on a loaded runner,
pool checkout plus asyncpg setup can exceed the 200ms sleep, so "b hasn't
finished" might mean "b never started". both sessions are warmed with a
round-trip first, pg_locks is queried for an actual waiter, and a failure
cancels the pending task instead of burying itself in InterfaceError noise
gate green, 247 passed.
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.
T13's token is stateless, so it can only bound how long an approval is good for. Inside that window the same token verifies every time it's presented — for a transfer, that's one approval moving money twice. "At most once" is a different property and needs state.
the primitive
One conditional UPDATE, never a read followed by a write:
That statement is the security decision; everything after it is bookkeeping. Two processes racing on the same
jticannot both find it unconsumed — the loser blocks on the row lock, re-reads after the winner commits, matches zero rows, and is told it's a replay.deliberately the opposite trade-off from T9
T9's alert delivery is at-least-once: the claim commits after a successful send, because a duplicate alert beats a missing one. Here the token is consumed before the transaction is built (T17), making it at-most-once: a transfer the human has to retry beats a transfer that happened twice. Same compare-and-set primitive, opposite direction, because the cost of the duplicate changed.
the test that carries the claim, and proof it has teeth
test_two_concurrent_consumers_and_exactly_one_winsruns the race for real against Postgres: two sessions, A consumes without committing (holding the row lock), B's identical statement is issued and asserted to be blocked, then A commits and B re-evaluates under READ COMMITTED.I verified it fails on a broken implementation by swapping
consumefor read-then-write. Result: B still blocked, then overwrote A's claim and also won — a double-spend. Worth noting because it shows where the safety actually lives: not in the locking (which happened either way) but in theconsumed_at IS NULLpredicate being part of the same statement.at-most-once is a property of committed work
test_a_rolled_back_consumer_leaves_the_token_spendable: if the transaction that consumed the token rolls back — the transfer never left the process — the token must still be spendable. Otherwise a crash between claim and broadcast burns a human's approval with nothing to show for it. This is exactly why T17 commits the claim and the recorded intent together.the rest of the surface
registerturns theuq_auth_tokens_one_live_per_proposalindex into a value (LIVE_TOKEN_EXISTS) rather than an exception, inside a SAVEPOINT — T16 registers a token in the middle of a larger unit of work, and a collision must not cost the audit row that records what happened.supersede_livemakes re-approval replace authority instead of adding it. It also retires expired-but-unconsumed rows, recordingconsumed_reason = 'expired'rather than'superseded'. That's the hole the T12 review found by reading the partial index: it can only testconsumed_at, since a partial index cannot referencenow(), so without this path a proposal whose token timed out unused would be permanently unapprovable.unknown_jti/already_consumed/expired). Read-then-write is the wrong shape for a decision, but this isn't one — the refusal already happened and nothing here can reverse it. It exists so the audit trail says "replayed" rather than "no".tests
tests/integration/test_replay_store.py— 15 cases against real Postgres (newexecute_engine/execute_sessionfixtures, kept separate from the recommend ones because the two domains own separate metadata by design). Covers consume-once-then-refuse, the recorded reason, expired, unknownjti, naivenow, the two concurrency cases, the one-live-token constraint, per-proposal scoping,jtireuse, savepoint recovery, and the four supersede paths.Gate green: ruff, mypy strict, import-linter, 241 passed.
review round (7d3eae2)
Seven findings. The review also verified the parts it couldn't settle by reading — SQL echo confirmed
consumeandsupersede_liveeach emit exactly one statement with no pre-SELECT, and a probe confirmed the SAVEPOINT really does leave a caller's pending audit row intact.Medium — attribution came from an unauthenticated argument.
token.pyputssubjectinside the MAC on the grounds that "an audit trail that can be edited by the caller is not an audit trail", and thenregistertook anoperator_idkeyword and droppedclaims.subjectentirely. SinceConsumed.operator_idis what T17 attributes a transfer to, a T16 mix-up between the acting and approving operator would move money against the wrong human, undetectably.registernow takes no operator argument at all — it resolves the operator by the MAC-protected subject, and a subject with no operator row is refused, because an unattributable authorization to move money isn't an authorization.Medium — every unrecognised
IntegrityErrorwas reported asDUPLICATE_JTI.jtiis a fresh uuid4, so a genuine primary-key collision is effectively impossible — meaning in production that value would almost always be something else swallowed: a missing operator row, a NOT NULL violation, a constraint added next year. T16 would then write a plausible "duplicate jti" into the approval trail for a referential-integrity bug on the money path. The reviewer demonstrated it with a real foreign-key violation. Only the two constraints this module can explain become values now; everything else re-raises.The five lows, all real:
SUPERSEDEDis no longer reported asALREADY_CONSUMED. An operator re-approves, then clicks the stale first link — ordinary operation, which was being filed as a replay and would send someone hunting an attack that never happened. The distinguishing column was already on the row; the query just wasn't reading it.registernow refuses a token that is already expired on arrival. A backdatedexpires_atwould otherwise park an unconsumable row in the one-live-token slot, blocking every later approval of that proposal until someone thought to supersede it.registercommitting between the UPDATE and the follow-up SELECT, and its honest label isUNKNOWN_JTI: at the moment of the claim there was nothing to claim.gate/__init__no longer re-exports. The eagerfrom .replay_store import …meant importing the pure token module pulled in SQLAlchemy and the full ORM metadata — quietly undoing the propertytoken.pyclaims for itself.store/__init__now names this module as the one deliberate exception to "gate never touches SQLAlchemy directly", and says why: its guarantee is a specific statement, and hiding that behind a repository puts the mechanism where a reader of the gate can't see it.pg_locksis queried for an actual waiter, and a failure cancels the pending task instead of burying itself inInterfaceErrornoise.Gate green: 247 passed.