Attempt numbers never repeat, and an outcome is never lost - #141
Merged
Merged
Conversation
SPEC-v0.7 section 5.6. Two defects in the 0.6.1 Postgres store, each reproduced by a test that was red before the fix, and a third found while building: - The stale renewal. The renewal UPDATE matched effect_key and state only, so a renewal planned against attempt k could land after another process had renewed to k+1, run and failed, and write k+1 a second time. It is now also conditioned on the attempt it was planned from, taken from the plan, with the row count checked. SQLite carries the same clause, where it is an equivalent mutant. - The lost COMMIT. A reservation whose COMMIT was lost and re-issued returned the original plan's reservation, not the re-issue's. Both resolve functions now return the reservation on the record. - The renewal's lost-commit re-read took any RESERVED record under its action_id as its own write. It now applies the v0.6 section 4.3.3 whole-row identity check, as the insert path already did. T246 (two windows) and T246b (renewal, insert and landed variants), each through both reservation methods, in tests/test_attempt_integrity.py: separate OS processes against Postgres, driven by the test-owned proxy, which gains a hold_when mode that holds one statement on one connection. No new StateStore method. Section 12.3a records what building decided.
… review) The review's blocking finding. The renewal fix assumed the attempt number only moves by a renewal, and on Postgres it did not: _write_effect (under resolve_effect, extend_lease, hold_continuation and the kept AMBIGUOUS write) and _transition (under the four transitions) wrote back the attempt they had read under a WHERE on effect_key, action_id and state alone. A caller retrying one Action reuses its action_id, so a stale resolve decided on AMBIGUOUS at 1 could write FAILED at 1 over AMBIGUOUS at 2, and the next renewal handed out attempt 2 again. Both compare-and-sets now carry AND attempt = <the attempt read>; a moved row matches nothing and is refused. T246c holds each shape with the test proxy (resolve_effect, a non-owner's AMBIGUOUS write, mark_ambiguous); all three were red first. Also from the review: Proxy.hold_when becomes arm(), which clears holding in place, gives each arming its own release event and refuses to arm over a pending hold, with a server-free test that arms twice; the statement_of docstring says what psycopg's prepare threshold means for it; and T246b gains the insert-refuse variant, the interleaving that reaches _resolve_lost_insert's last refusal, which no test did.
SPEC-v0.7 section 5.6 states the monotonicity the renewal fix rests on and what made it true; section 8.3a gains T246c; section 10 gains two rows; section 9.6 item 8 and section 12.3a record the review, the three UPDATEs checked, the attribution residual a reused action_id still leaves on every backend, and the corrected note on the lost-insert refusal. SPEC-v0.6 sections 4.2 and 4.3.4 carry amendment pointers, and the changelog gains the stale-write fix under Fixed.
The blocking finding of the second review round. Conditioning every effect write on the attempt it read stopped the rewind, and then refused writes that carry an outcome: a commit_effect or mark_ambiguous whose record had moved wrote nothing and raised, so the effect record, which gates the next renewal, said nothing about an attempt that may have acted. The reviewer measured a renewal to attempt 3 with attempt 1's commit recorded nowhere, which for a refund that landed is a second refund. Both outcome transitions are now re-issued once against the re-read, bounded as Table A2's re-issue is, so the outcome lands where the same call a moment later would have landed it. begin_execution and fail_effect are still refused there: FAILED asserts that nothing happened, and re-issuing it over a running attempt would permit a retry beside a live dispatch. With it, four smaller findings from the same round: - the refusal on a moved record was always DuplicateEffect(in_progress), which errors.py defines as a live reservation; it now takes its type from what the re-read found and its message names the move; - Control caught two store refusals around its outcome writes, so a record a human resolved while the attempt was still running produced no receipt, no EXECUTION_AMBIGUOUS event, and a store error in place of the executor's exception. It now catches every CTRLRunError there, records the evidence whatever the store answered, and names the refusal in it. T246d covers both backends; - _expire wrote over a consumption that committed after its read; it carries the status it read, as every other approvals write does; - release() pre-released a hold that had not fired, and statement_of's docstring understated the prepare threshold, which is six executions.
SPEC-v0.7 section 5.6 says what a re-read does with a write that carries an outcome and what it does with one that claims a reservation, and separates the store race this closes from the longer window a human stands in, which needs an attempt argument on the CLI and is a follow-up. Section 8.3a restates T246c and adds T246d. Section 10 gains five rows. Section 12.3a carries the dropped-outcome finding, the truthful refusal, the evidence rule, the approvals write, the proxy holes, and a residual paragraph corrected in three places: a late fail_effect is a concurrent double execution and not misattribution, a reconcile hook reaches it with no human, and the store-side attempt memo that would close it is declined here with its reasons.
The round-2 mutation run left one row alive: removing the re-issue's bound failed no test, because a second move under the re-issue needs a rival interleaved inside it and a proxy hold fires once. v0.6's T155f is the precedent for what that costs, so the bound is driven the same way, at the store's own read: a subclass that reports the record one attempt further on every read is a rival that never stops moving, and every conditional UPDATE then misses. Bounded, the call refuses and writes nothing; unbounded, it recurses until Python stops it.
The blocking finding of the third review round. The stale re-issue's bound and the lost-commit re-issue's bound each permit one attempt, and neither passed the other's flag on, so a lost COMMIT inside a restage cleared the restage bound and a restage inside a lost-commit re-issue cleared the lost-commit bound. Driven together, every COMMIT lost and a record that keeps moving, they alternated to RecursionError at 113 deep: T155f's failure mode by another door, and outside the closed error set. Both flags now travel through both re-issues. Its test drives both halves the way each is driven alone, the real proxy at drop_before_commit = 1000 and the moving-read seam, and it uses the read pattern a search found: the obvious every-other-read pattern ends bounded even when the flags do not compose. With it, four smaller findings from the same round: - _unrecorded recorded the store's refusal and not what the executor did, so an executor that returned and one that raised NotExecuted left identical receipts. For the first the remote very likely acted. Both now reach the receipt, as the ambiguous branch already did; - the re-issue is asked for by commit_effect and mark_ambiguous rather than inferred from the state being written, so a later transition to COMMITTED or AMBIGUOUS that is a decision rather than an outcome cannot inherit it; the state set stays as an assertion; - the restage is logged before the re-issue, so one that then refuses is no longer invisible; - section 12.3a records the mark_ambiguous route to a concurrent dispatch through eager reconciliation, and that a wiring-bug refusal now surfaces in the evidence rather than at the caller.
The round-3 mutation run showed the composition test was weaker than it reads. Dropping both bound flags recurses; dropping either one alone does not recurse on any read pattern of four or fewer, it permits one re-issue more than the bound allows and then terminates, so a test asserting "no RecursionError" was green for exactly the two mutants that say each flag is load-bearing. Each bound permits one re-issue, so three is the deepest nesting any interleaving may reach, and that is what the test asserts now. The two read patterns come from a search over this proxy rather than from intuition: (0, 0, 1, 0) catches the restage dropping retrying, and (1, 0, 0, 0) catches the lost-commit re-issue dropping restaged.
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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
2 participants
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.
v0.7 item 3a (SPEC-v0.7 §4.4, §5.5, §5.6, §8.3a). Attempt numbers never repeat, and an outcome is never lost. Item 3's idempotency token and item 4's ceiling both rest on this, so it lands before either.
Every defect here is in shipped 0.6.1 code, found by reading it against the v0.7 spec and then reproduced deterministically before being fixed. Four review rounds.
What was wrong
UPDATEmatched oneffect_keyandstateonly, so a caller that readFAILEDat k, stalled while another renewed to k+1, ran and failed, then wrote k+1 a second time. Two dispatches, one number.COMMITreturned a number it did not write. Both resolve paths re-issued the reservation and then returned the original plan, so the caller held k+1 while the store held k+2.action_id, so a second process renewing under the same id left two dispatches holding one attempt. It now uses v0.6 §4.3.3's whole-row identity check._write_effectand_transitionsetattemptfrom their own earlier read while matching on(effect_key, action_id, state). A human's staleresolve_effect(failed)could overwrite attempt 2's record with attempt 1, and attempt 2 was then handed out again._checked, raiseInvalidArgumentintoControl, and produce no receipt and noEXECUTION_AMBIGUOUSevent while the record readFAILEDand a retry was permitted. An executor's timeout disappeared from the evidence._expireoverwrote a concurrent consumption, so an approval that authorised a real effect could readexpired.What it does now
Every
UPDATEon an effect record carries the attempt it read (on Postgres; SQLite's writes sit insideBEGIN IMMEDIATE). A moved row matches nothing, is re-read, and is refused with the exception the found record earns, naming the move.commit_effectandmark_ambiguousare re-issued once against the re-read so a terminal outcome is never dropped;begin_executionandfail_effectare refused there, because a staleFAILEDmust never land on a running attempt.Controlrecords the event and the receipt whatever the store answers, names the refusal in the recorded error, and re-raises the caller's own exception.The bound, proved and tested
The re-issue and the lost-commit re-issue each set a flag that is never cleared and that the other now passes through, so every recursive call strictly increases the flags set: at most three frames, and three is reachable. An earlier version let the two bounds clear each other, which recursed to
RecursionErrorat 381 deep. That is T155f's failure mode, and aRecursionErroris outside the closed error set, so no caller could classify it. The test asserts depth 3 over two interleavings found by search, because a test asserting only "this did not blow the stack" was green for each flag dropped alone (measured, not assumed).Evidence
tests/test_attempt_integrity.py.AND attempt = ?, thecarries_outcomeguard, and_resolve_lost_renewal's final raise.arm(predicate), which holds a single statement on a single connection and refuses to arm over a pending hold. No second proxy.Residual, stated rather than fixed
With a reused
action_id, a late transition can still attribute an outcome to the newer attempt, and a latefail_effectcan permit a dispatch beside a running one. Closing it needs the attempt on every transition, which the frozenStateStoresignatures do not carry; the store-side memo that would do it is written down in §12.3a and declined here.resolve_effectalso carries no attempt, so the window a human actually has, inspect then decide then resolve, is wider than the one this closes. Both are in §12.3a and the CHANGELOG.mainhas moved;git merge-treereports zero conflicts. Theapidrift is two "defined at line" numbers; item 6 regenerates.