Repository navigation
Conversation
…nstead of poisoning its id Backend A's publish() refused any id whose record was PARKED, so a fresh result written for that task was quarantined with no POST attempt and vanished from results/ exactly as a delivery does. A PARKED record now holds only the payload that parked: publishing a different payload under the same id starts a fresh cycle (READY, attempts 0, resend_epoch + 1 so the idempotency key cannot dedupe against the parked attempt, the replaced park kept on the record as superseded_park). Re-publishing the parked payload itself stays refused, so a rescanned live file cannot turn one park into a retry per pass. Backend C already behaved this way by construction (a parked key has no READY link), so this restores parity. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at b0ff067. The code is correct for the gateway path it targets, and I found no blocker in it. Leaving a COMMENT rather than an approval for two reasons: the live round trip is still owed, and one required CI job is red.
Verified (my run + Rui's Codex agent, independently):
- New test: parent d37b794 has 2 of 4 failing; at head 4/4 pass.
- Mutants caught: reverting backend_a, dropping the epoch bump, removing the same-payload refusal, and removing superseded_park each fail the new test.
tests/delivery-core-*,tests/outbox-*, sparrow claim-protocol, health-check-outbox-parked and the quarantine/proactive-fence tests all pass. Package unittest: 22 OK.- No vendored change is needed. The policy lives in
delivery_core/backend_a.py, andtools/sync_from_src.py --checkis in sync. - Callers: Discord and Slack check PARKED before they publish, and the proactive fence always publishes
b"", so in practice only the gateway changes behavior.
Still owed before approval:
- Live post-restart round trip (REVIEW.md lesson 15). Force a park on this head, publish a different result for the same id, show the real POST plus confirmation, and show that rescans don't resend.
- CI: "shell tests (lanes)" fails on
tests/sync-workspace-guard-config.test.sh("exits 0 with a globally ignored untracked file", rc=1), which also fails the clean-install aggregator. The PR touches no shell or sync code, and the test passes locally at this head, so it looks like a flake. Please re-run.
Non-blocking findings, each reproduced with a probe:
- A/B alternation is unbounded (backend_a.py:47). Only the last parked payload is refused, so A parks, B is accepted and parks, then A is accepted again. My probe ran 6 alternations: 14 provider calls, epoch 6. Gateway quarantine normally prevents this, but not when quarantine fails or the payload metadata changes between passes. Suggestion: keep digests of all superseded parks and refuse any of them.
- Crash window (backend_a.py:59). If a PARKED record still holds a stale claim,
publish(B)returns False.deliver_onethen reports TERMINAL and the gateway quarantines B: the same silent loss this PR fixes, reached by a narrower path. outcome-unknownparks now accept a fresh cycle. The gateway'sdefer_idempotent_resendmostly avoids this, but it changes the "at most once when the outcome is ambiguous" stance. Worth saying in the docstring.- Two edge cases: a PARKED record with no
payloadfield accepts evenb""(no production caller creates one), and two different invalid-UTF-8 payloads compare equal after"replace"decoding. - Weak assertion:
tests/delivery-core-parked-fresh-publish.test.py:117checksrepublish_delivered=Falsewhile the item is READY, so it passes whatever the DELIVERED rule does.
Not verified: whether the broker accepts a second POST with a new idempotency key on a lease where the first attempt may already have landed.
|
Post-restart round trip (owner's Go given), run at both commits with the real
Parent run summary: Recipe (scratchpad, not in the repo): |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — exact head b0ff067fdfeb51460cc9fb370c139fe2bf0a1782 is not merge-ready.
[P1 blocker] The new resend epoch never reaches the shipped provider's deduplication boundary, so a replacement can be reported DELIVERED while the broker keeps the old body. Backend A increments resend_epoch in backend_a.py:46-56, but AG2SpaceResultProvider.deliver() ignores its idempotency_key argument and POSTs the envelope verbatim. The envelope still carries the same broker result ID; the provider test explicitly pins result-ID deduplication, and the protocol says gateway acceptance includes it.
I reproduced the consequence with the real Backend A, default RetrySchedule, DeliveryCore, and AG2SpaceResultProvider, against a request double implementing the pinned broker dedup rule. The broker accepted body A but every response was lost; after five attempts the local item parked. Publishing B returned True and advanced the local epoch to 1, but B's POST was deduped by the unchanged envelope ID:
after_default_window= PARKED attempts= 5 broker_body= first
publish_B= True epoch= 1
after_B= DELIVERED outcome= confirmed broker_body= first wire_last= second
The worst case is silent user-visible data loss with reassuring local evidence: B disappears as “delivered,” while only A exists remotely. Either make the successor-cycle identity part of the broker-honored dedup key, or restrict automatic fresh-cycle publication to states that prove the prior payload did not cross the boundary. Pin the chosen rule through the actual AG2 provider, not only a fake provider that observes the core-only key.
[P1 blocker] A valid crash residue still reproduces issue #3843's zero-POST quarantine for a different payload. If the process dies after complete() writes PARKED but before it releases the claim, publish() returns False merely because the stale claim exists. claim() then clears the dead claim but returns no token; the gateway reads TERMINAL and quarantines the replacement at remote_gateway_bridge.py:4460-4469. A production-function probe with a real child dying at that release boundary produced:
returned=False provider_calls=0 live_result_exists=False
quarantined=1 status=PARKED
The claim is clean on the next pass, but the replacement file is already out of the drain. Please make this transition retain/retry the replacement and add a regression through _deliver_result_payload, not only the backend happy path.
[P2 contract blocker] The newly global ClaimBackend.publish contract is false for Backend C. contract.py:178-185 now promises every backend refuses the same parked payload and gives a different payload a non-deduping cycle. Backend C's publish() does neither: my probes got same_payload_publish=True, and the parked and replacement attempts both used task-x#0 with epoch 0. The new regression instantiates A only, while the existing C requeue-key contract case is skipped. Implement and test the contract for C, or scope the documentation and parity claim to A.
The intended A-only happy path is otherwise demonstrated: the parent refuses B, this head accepts B with attempts reset and epoch 1, and the four new tests are collected and pass. I also ran the contract (43 tests, one revealing C skip), crash (3), enforcement (15), terminal (5), gateway-outage (76), operator-recovery (40), and package (22) suites; git diff --check and the repository review scanner pass, including a firing hardcoded-path control.
Two merge-readiness gates remain independently outstanding: the PR explicitly has no real post-restart round trip for this delivery-loop change, which REVIEW.md requires before approval, and exact-head CI is not yet green. Attempt 1's shell tests (lanes) genuinely ran and failed sync-workspace-guard-config.test.sh; a rerun is currently pending, and the test passes locally 5/5.
Verdict: changes requested. Re-review can proceed after the two loss paths and Backend C contract mismatch are resolved, a real post-restart parked-ID replacement is witnessed end to end, and exact-head required CI is green.
Reviewed by Kewei's Codex automation.
…on, and a dead claim on a park is retired Review round 2. A PARKED record keeps the digests of every payload it has parked on (raw bytes, bounded list), so two bodies cannot alternate through the park and two invalid-UTF-8 bodies are told apart; a park that recorded no payload refuses everything, including an empty body. A claim left on a park by a crash between the park write and the release is retired when its owner no longer runs (the cycle ended at the park, so no TTL applies); a running owner still refuses. The contract says outcome-unknown parks accept a fresh cycle and why that is safe. The DELIVERED-rule assertion is now judged in the DELIVERED state. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 2 at head c562b2b answers each non-blocking finding (details and pasted before/after in the body's "Review round 2" section):
Suites at this head: all Not verified, as you noted: whether the broker accepts a second POST with a new idempotency key on a lease whose first attempt may already have landed — that is broker-side behaviour outside this core. |
|
Round-2 head c562b2b, witness re-run (PR side; same recipe and guards as the earlier comment: real gateway from the worktree under
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Round 2 at c562b2b. Requesting changes. Most of my round-1 findings are fixed, but keweichen's P1 still reproduces at this head, and with this PR it fails silently.
Blocker: the new resend epoch never reaches the broker. provider_ag2space.py:31-42 (unchanged) ignores idempotency_key and POSTs the envelope. The broker deduplicates on the envelope's result id, which is the same broker_tid for A and B.
I reproduced this with the real Backend A, DeliveryCore (defer_idempotent_resend), and the real AG2SpaceResultProvider. The broker is a request double that dedupes by id and loses every response:
after A: PARKED max-attempts broker body: first
publish B: True epoch 1
B outcome: CONFIRMED status: DELIVERED broker body: first (wire last: second)
Before this PR, B was quarantined, where an operator could see it. Now B is archived as DELIVERED while the broker still holds A. That happens on every park where A actually landed, which includes max-attempts and exhausted retry windows after lost responses. The broker double is synthetic, but Rui's Codex agent reproduced the same thing independently.
To fix it, either carry successor identity to the broker (for example, the epoch in the result id the broker dedupes on), or only allow a fresh cycle when the park proves A never landed (a definite refusal, not an ambiguous loss). Then test it through the real provider with a deduping double.
Also open:
- The cap breaks "every payload ever parked on".
parked_digestskeeps 32 entries plus the current one. With 34 distinct bodies rotating, all 102 publishes were accepted again (102 provider calls). This is unlikely in the gateway, but the docstring and contract are now false. Either enforce the guarantee once the cap is reached, or narrow the wording. No test covers the cap; removing it survives the suite. - Backend C parity (keweichen's P2).
contract.pystates the rule for everyClaimBackend, butbackend_c.publish()is unchanged. Scope the contract to A, or implement and test C. - Torn claims still quarantine. A dead-pid claim is now retired, which is good. But an UNKNOWN/torn claim still returns
publish(B)=False, sodeliver_onereports TERMINAL and B is quarantined. - CI:
diff coverage >= 95% (python)fails at 93.9% onbackend_a.py, missing lines 46 (the legacy digest fallback) and 90 (the DELIVERED-plus-live-claim refusal). - Live witness: the comment ran d37b794 and b0ff067, not c562b2b. Its logs are paraphrased and kept locally. It doesn't show quiet rescans. Its fake broker refuses once and never dedupes, so it can't exercise this blocker. Please post raw log lines from a run on the current head.
Fixed since round 1: A/B alternation (the ping-pong now gives [True, False×5]), a park with no payload now refuses everything, raw-byte digests, the DELIVERED assertion is judged in the DELIVERED state, and a dead-pid claim is retired.
Tests: the new test file fails 4 of 9 at b0ff067 and passes 9/9 at head. Five mutants are caught. 72 suites ran and 71 passed; the one failure (gateway-proactive-foreign-homeserver) fails the same way at d37b794 on this host, so it's environmental. Package unittest is 22 OK, and the vendor sync and review-checks scans are clean.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Requesting changes on c562b2b34d520376843a4c5f0c6e75dab613d3f9 after a fresh round-two review.
The new head does fix the advertised dead-claim, payload-less, invalid-UTF-8, and two-payload regression cases: the focused test is 9/9 green at this head, and the current test copied onto b0ff067f fails in exactly those four cases. The following merge blockers remain.
[P1] The resend epoch still does not cross the AG2 Space provider/broker dedup boundary. DeliveryCore computes the fresh item#1 key, but AG2SpaceResultProvider.deliver() ignores idempotency_key and POSTs the original envelope unchanged (provider_ag2space.py:31-42); a broker duplicate is then treated as confirmed (:58-61). I reproduced this through the production Backend A/core/provider classes with a broker double that accepts A while its response is lost, then dedupes by envelope id:
after_A PARKED retry-window-exhausted attempts 5 broker_body first
publish_B True epoch 1
after_B DELIVERED outcome confirmed broker_body first wire_last second wire_id task-provider-dedup
Local state says B was delivered, while the broker still stores A. The posted live witness explicitly returns ok:false for A before accepting B, so it verifies the definite-refusal path but does not exercise the response-lost/result-id-dedup ambiguity above. The fresh-cycle identity must reach the actual broker dedup key, and this ambiguity needs a regression test plus refreshed live-path evidence.
[P1] A successful intervening delivery erases the parked-payload history. In backend_a.py:61-98, a republish from DELIVERED starts again from record = {}; unlike the PARKED branch, it carries neither parked_digests nor resend_epoch. With provider outcomes refuse(A), confirm(B), refuse(C), I get:
delivered_reset history_before_C 1 epoch_before_C 1 history_after_C None epoch_after_C None A_reaccepted True
So A parks, B is accepted and delivered, C replaces the delivered record and parks, and A is accepted again. That directly violates the ClaimBackend.publish() promise that every payload previously parked for the id remains refused.
[P1] The 32-digest cap also permits indefinite alternation. Cycling 34 distinct payloads three times produced:
cap_rotation accepted 102 of 102 provider_calls 102 stored_prior 32 epoch 101
parked[-PARKED_DIGESTS_KEPT:] (backend_a.py:79) cannot satisfy the unbounded “every payload ... stays refused” contract in contract.py:182-187. Either the contract/invariant needs a bounded design with a safe, explicit behavior after saturation, or the implementation needs durable identity history that actually enforces the stated rule.
[P2] The global publish contract remains false for Backend C. Its publish() only checks live token files (backend_c.py:197-215) and does not consult the parked namespace. The same-payload probe reports:
backend_c first True same_after_park True second_status attempted epoch 0 provider_calls 2
Please implement the contract there or scope the new behavior behind an explicit backend capability instead of asserting it for every ClaimBackend while skipping Backend C in the contract test.
Independent checks otherwise passed: delivery-core contract 43/43 (with the Backend C case skipped), crash 3/3, terminal 5/5, enforcement 15/15, gateway-outage 76/76, package unit tests 22/22, vendored sync, diff check, and hardcoded-path scan. On this exact head, diff coverage >= 95% (python) is still failing; the remaining checks and CLA are green.
…finite refusal, and never forgets a parked body The broker dedupes on the envelope id, so a fresh cycle for a parked id is safe only when the park proves the parked body never landed: a definite refusal. An ambiguous park (lost responses, exhausted retries, outcome-unknown) now refuses every payload, so the caller quarantines the new result where an operator sees it instead of archiving it as delivered while the broker keeps the old body. The parked-payload history is kept in full (no cap) and carried across a later delivery of the same id, so no parked body is ever re-admitted; a torn claim on a park refuses, like every other reclaim path. The rule is declared through BackendCapabilities.fresh_cycle_after_definite_park and the contract suite asserts it only for backends that declare it. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 3 at dad8b2d, answering both round-2 reviews point by point (details and pasted before/after in the body's "Review round 3"). Shared P1 — the resend epoch never reaches the broker. Agreed, and it cannot from this repo: the broker matches a result to its lease by the envelope P1 — a delivery in between erased the parked history. The DELIVERED republish now carries P1 — the 32-digest cap. Removed; the full history is kept per record (32-byte digests). 34 bodies rotated three times: 34 accepted, each once, 33 digests stored (was 102 of 102 accepted). Torn/UNKNOWN claims (rui). An unreadable claim names no owner, and no reclaim path in this codebase steals one; publish keeps refusing, so B is quarantined by the gateway, not archived. Pinned by P2 — Backend C. The rule is now declared: Coverage. Live witness. The earlier runs exercised only the definite-refusal path, as you noted. A re-run at this head with a fake broker that loses responses and dedupes by id (ambiguous park → quarantine, no POST for B; definite refusal → POST), with raw log lines, follows as a separate comment. Suites at this head ( |
|
Round-3 witness at head dad8b2d vs parent d37b794, two scenarios each, real gateway from a worktree under
Raw gateway lines naming the id (line numbers as logged; retry-cadence lines omitted): PR / ambiguous: PR / definite: parent / ambiguous: parent / definite: Broker |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — exact head dad8b2dd382ac4573d47903aba1090493092685b is not merge-ready.
Round three does fix the two pure cases it tests: an all-ambiguous cycle refuses B, while a first-attempt permanent refusal admits B. I ran the 18-test regression successfully at this head; the same test file fails in exactly six claimed cases at parent c562b2b34. Required CI, 100% diff coverage, CLA, vendored sync, diff check, and the review scanner are green. The newly posted live witness also demonstrates both pure cases. The following blockers remain.
[P1] A mixed ambiguous → permanent cycle still recreates the silent-loss path. DeliveryCore converts an OUTCOME_UNKNOWN into retryable NOT_DELIVERED, but records only the final attempt as permanent-refusal; Backend A then treats that final reason as proof that the broker holds nothing. A production-class probe through the real Backend A, core, and AG2SpaceResultProvider scripted the first A POST as accepted with its response lost, the retry as ok:false, and the B POST through the broker result-id dedup boundary:
attempt_1 not_delivered READY attempts 1 broker_body A
attempt_2 not_delivered PARKED permanent-refusal attempts 2 broker_body A
publish_B True epoch 1
attempt_B confirmed DELIVERED provider_posts 3 wire_last_body B broker_body A
The local record archives B as delivered while the broker still contains A. A later refusal cannot retroactively prove that an earlier ambiguous attempt did not cross the boundary. The regression covers ambiguous-only and permanent-first cycles (lines 342-373); the live witness covers those same two pure arms, not this mixed sequence. Please retain a durable cycle-level ambiguity taint and refuse a successor whenever any attempt in the parked cycle may have landed.
[P2] The new history is unbounded protocol metadata. _parked_digests appends every digest forever (backend_a.py:40-51,84-86), while cleanup() remains a no-op that says Backend A is bounded by lock striping. This conflicts with the mandatory bounded-metadata invariant in contract.py:165-176. Repeating the production publish/park writers for one id produced:
cycles 2000 history 1999 bytes 136299
cleanup_pruned 0 cleanup_detail A: bounded by lock striping
This makes both record size and rewrite work grow without limit. A bounded saturation state that fails closed after the retained history fills would preserve safety without eviction/readmission.
[P2] Backend C still violates the capability contract, and the new contract test never parks it. fresh_cycle_after_definite_park=False explicitly means a park is final for every payload (contract.py:118-126), but C has an undelivered parked namespace and publish() ignores it. The real backend reports:
capability False
publish A True; park A; terminal True
publish B after park True; claim B True; terminal still True
The false-capability arm at delivery-core-contract.test.py:125-133 checks only that a second publish loses while A is still live; it never creates a park. Please enforce the declared final-park rule in C or define and test a capability that accurately describes its post-park behavior.
[P2] Operator requeue drops the current parked digest from the lasting history. The operator may explicitly retry A, but after that retry is delivered, the normal delivered-republish path automatically accepts A again because only parked_digests is checked and the current payload_digest was never folded into it:
requeue requeued history None
after_requeue_delivery confirmed DELIVERED history None own_digest True
same_parked_body_reaccepted True
That contradicts the new publish() contract that every payload ever parked remains refused after a later delivery (contract.py:181-195). Preserve the parked digest through the authorized requeue/delivery transition, while still allowing that one explicit operator retry, or narrow the contract and justify the resulting duplicate behavior.
Verdict: changes requested. The exact-head gates are green and the pure-arm evidence is now present, but the mixed-cycle data-loss path plus the three contract/state gaps above still block merge.
Reviewed by Kewei's Codex automation.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Round 3 at dad8b2d. Much better. The original provider-identity case is closed, coverage is green, and the cap is gone. One narrower form of the same silent loss remains, so I'm keeping changes requested. This combines my run and Rui's Codex agent's; we reached it independently.
Fixed:
- The original repro is closed. p3 uses the real provider with a broker that dedupes by id and loses every response. Now
publish(B)=False, B stays PARKED and gets quarantined, and it is no longer DELIVERED while the broker holds A. - Error classification is right. A pure 503, timeout, connection reset or RemoteDisconnected each parks as
max-attempts/retry-window-exhaustedand refuses B. A definite 400, 409 orok:falseadmits B correctly. A malformed 200 or IncompleteRead raises; that's pre-existing, and it isn't misread as definite. - History. It is uncapped and survives a DELIVERED republish, and both mutants are caught. 1,000 parks give a ~68 KB record, about 69 B per park, so it's linear and acceptable.
- Torn claims. They still refuse, but that's now deliberate and tested, and B stays visible. Accepted.
- CI. Diff coverage is green: 33 pass, 0 fail.
Remaining blocker: definiteness is judged only by the cycle's last attempt.
A later refusal doesn't prove an earlier attempt never landed. The scenario: A is stored remotely but its response is lost (timeout, reset or 503), and the next attempt returns {ok:false} or a 409. A parks as permanent-refusal, publish(B)=True, the broker dedupes B on the unchanged id, and B ends up DELIVERED locally with A remote.
Both of us reproduced this through the real AG2SpaceResultProvider, the default RetrySchedule and DeliveryCore, against a deduping request double. It needs a broker that refuses A's retry but dedupe-accepts B, so it's less likely than round 2's case, but it's the same class of loss. The docstring's claim that "an ambiguous park refuses every payload" doesn't hold for mixed histories.
To fix it, persist "this cycle saw an ambiguous outcome" on the record, across retries and restarts. Core can set it before defer_idempotent_resend folds the unknown into not-delivered. Then refuse a fresh cycle if that flag is set. Add the three mixed variants as regressions: lost then 409, lost then ok:false, and 503 then refusal.
Also open: Backend C. fresh_cycle_after_definite_park=False is declarative only, since no code reads it. And C still returns True for publish(A) → park(permanent-refusal) → publish(A). That contradicts both the flag's comment ("final for every payload") and the global contract text. The contract test's False branch never parks, and a mutant setting A's flag to False survives the suite.
- Either scope the promises to A and describe C accurately, or implement C's behaviour and test it.
- Nit: the round-3 comment says the digests are "32-byte", but they're stored as 64-character hex.
Live witness: none yet at this head (REVIEW.md lesson 15). When you post it, include raw gateway log lines with the running sha, the forced-park path, and quiet rescans over a stated duration.
Tests: at c562b2b the new parked tests fail 6 of 18 and the contract tests error on 2 of 45. At head they pass 18 and 45. Mutants splitting definite from ambiguous are caught. 72 suites ran with 71 passing; the one failure is environmental and fails the same at d37b794. Package unittest is 22 OK and sync is clean. Combined with #5269 at 1d6d3e3, 73 suites ran with 72 passing.
…definite, and the parked history is bounded A refusal on the last attempt proves nothing about an earlier attempt whose response was lost: the core now marks the cycle ambiguous on the record before it folds an indeterminate outcome into a retryable attempt, and Backend A refuses a successor for any tainted park even when its final reason is a permanent refusal. The taint lives on the item record, so it survives retries, restarts and an operator requeue of the same cycle, and a legitimately fresh cycle starts without it. The parked-body history is capped at PARKED_HISTORY_LIMIT; past it the id saturates and refuses every new payload rather than forgetting one, and cleanup() says so. An operator requeue folds the parked body into the history, so the one explicit retry goes out but the delivered-republish path never readmits that body. Backend C now enforces the rule its capability declares: a park is final for every payload. The capability flag is read by publish() in both backends, so flipping it changes behaviour. Both contract arms create a real park. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 4 at head 573ea50 — point by point (details and the before/after table are in the body's "Review round 4"): Mixed cycle (both P1). The last attempt no longer decides. Bounded history (Kewei P2). Backend C (both P2). C's Requeue (Kewei P2). Nit (rui). Digests are 64-char hex sha256; the body said "32-byte". Fixed. Coverage. Changed statements under the gate's config: backend_a 16/16, backend_c 5/5, core 5/5, src/outbox 8/8. Not done, stated. Carrying the epoch to the broker's dedup key — the broker matches by envelope The live witness is being re-run at this head with the mixed scenarios (lost-then-refusal) and raw log lines, and will be posted as a comment. |
|
Round-4 witness at head 573ea50 (four variants) plus the round-3 head dad8b2d as the "before" (two mixed variants). Same recipe and guards as the earlier comments (real gateway from a worktree under
Record at the PR head after lost-then-okfalse: Raw gateway lines naming the id (retry-cadence lines omitted): PR / lost-then-okfalse: PR / lost-then-409: PR / 503-then-refusal: PR / definite: r3 (dad8b2d) / lost-then-okfalse: r3 (dad8b2d) / lost-then-409: Recipe: |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Round 4 at 573ea50. Good progress. Every round-3 case I raised is closed, and so is Backend C. The live witness at this head shows the three mixed variants quarantining B, with quiet rescans. One gap remains in the taint and silently loses B, so I'm keeping changes requested. Rui's Codex agent and I found it independently.
Fixed (my probes and Codex's):
- Mixed outcomes: lost→409, lost→
ok:false, 503→409, timeout→409 and reset→409 all now refuse B. B stays PARKED and the broker holds A. Pure-ambiguity controls refuse B too, and a clean 409 still admits B correctly. - The taint persists: it survives a reconstructed backend, a requeue and a torn claim. All 27 fresh-publish regressions pass, and delivery-core is 95 collected, 94 passed.
- Backend C now refuses the parked body with the capability flag False. The contract suite exercises it, and the A-flag-False mutant dies (12 failures).
- Saturation at 128 refuses rather than evicts on the normal park path (200 distinct bodies give 128 accepted, then refused).
Blocker: the taint is written too late, in complete(), after the provider call.
Any attempt that lands A but never reaches complete() leaves no taint:
- a process crash or kill during the POST;
- an exception the provider doesn't classify, such as
http.client.IncompleteReadorjson.JSONDecodeErroron a garbled 200.
After the dead owner's claim is reclaimed, a later 409 or ok:false parks as untainted permanent-refusal, publish(B)=True, the broker dedupes B, and local is DELIVERED while the broker holds A. Reproduced:
- by me, with IncompleteRead and with a garbled 200;
- by Codex, with a real isolated child process that exits right after the double stores A (
exit=17 … publish(B)=True; DELIVERED; remote=A).
Fix: persist "an attempt may have dispatched" before the side-effect boundary, for example a started-attempt counter written ahead of the provider call. Then admit a fresh cycle only when every started attempt has a definite classification. Also, or alternatively, map any post-send exception to ProviderIndeterminate. A restart test that begins after complete() has written the flag can't cover this window.
Should fix: requeue bypasses the bound. The saturation check runs only in publish's PARKED branch. requeue_item appends history without checking it, and the DELIVERED transition only reads the existing flag. Codex looped 140 times through publish, park, requeue and confirmed delivery: history length is 140, it never saturates, and the next different publish returns True. Put the history and saturation writer behind one contract covering publish, requeue and the delivered transition. Separately, saturation is visible only in the record (no log or health-check line), it has no recovery short of a hand edit, and the gateway's quarantine line says "permanent-refusal after N attempt(s)" for taint and saturation refusals alike, which misleads operators.
Should fix before merging alongside #5269 (fa63a46): gateway-terminal-result-moved-once::test_a_newer_reply_superseded_during_the_put_back_is_kept fails 3/3 when the two are combined (['NEWEST BODY'] != ['NEWER BODY']), and passes on #5269 alone. The test parks with a definite 400 and assumes a later different body is quarantined; under this PR that body gets a fresh cycle. Whichever lands second must update it, so please agree a merge order.
Clarify: a DELIVERED republish drops cycle_ambiguous from the new record. That's reasonable for a new cycle; please say so explicitly. Also, a complete() with a stale token returns before writing the taint. And one mutant survives, "reconcile always taints", because no test covers a reconcile receipt that clears the taint.
Tests:
- parked-fresh-publish: 8 failed + 2 errors at dad8b2d → 27 OK at head.
- contract: 1 failed → 45 OK.
- operator-recovery: 1 failed → 41 OK.
- 10 mutants killed. Suites: 72 files, 71 passing; the one failure is the known environmental one. Sparrow is 22 OK and sync is clean.
- CI: 35 pass, coverage green.
Witness gap: none of the witness runs crash the gateway between the ambiguous attempt and the refusal, which is exactly the blocker's path.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Changes requested on exact head 573ea50d173c908d76de2ccd9093dced976f91b5. The normal in-process mixed-outcome cases are fixed, but this head is not merge-ready because three production paths still violate the same safety contract.
-
P1 — a rolling upgrade treats legacy ambiguity as proven-clean and can silently lose the successor body. The PARKED admission check reads a missing
cycle_ambiguousas false. I created the mixed cycle with the actual merge-based37b79474: an indeterminate call stored A, then a permanent refusal parked it. The on-disk result was:base_record {"cycle_ambiguous": null, "payload": "A", "payload_digest": null, "reason": "permanent-refusal", "status": "PARKED"}Opening that unchanged outbox with this head produced:
head_result {"published_B": true, "outcome": "confirmed", "local_status": "DELIVERED", "wire_body": "B", "external_body": "A"}The deduping provider retained A while local state reported B delivered. Missing certainty must fail closed, or legacy state needs a migration/versioned explicit-clean marker.
-
P1 — process death after the provider side effect but before
complete()loses the cycle taint. The provider call happens atcore.py:109, while ambiguity is not persisted untilcore.py:132-137reachesbackend_a.py:210-215. In a forked production-class repro, the broker stored A and the child exited before returning:child_exit 17 broker_body A after_crash_status READY cycle_ambiguous None claim_owner_state DEAD after_refusal PARKED permanent-refusal cycle_ambiguous None publish_B True after_B_local DELIVERED outcome confirmed wire_body B broker_body APersist attempt-start evidence before crossing the side-effect boundary, or conservatively taint a reclaimed interrupted attempt.
-
Blocker — operator requeue bypasses the advertised 128-entry history bound.
requeue_item()appends the parked digest without applying the cap or settingsaturated; after that retry confirms, the DELIVERED publish arm also omits a length check. Repeating the real publish → permanent park → production requeue → confirm lifecycle yielded:cycles 140 history_len 140 saturated None next_fresh_publish True limit 128This needs one shared writer contract for append/deduplicate/cap/saturation across publish and requeue, plus a full-lifecycle regression.
Verified positives on this SHA: the focused suites passed (27/27 parked-fresh, 45 contract tests with one intentional skip, and 41/41 operator-recovery), the vendored copy is in sync, review checks pass, exact-head CI and CLA are green, and the current-head live post-restart witness covers the intended mixed cases. Those checks do not exercise the upgrade, crash-before-completion, or repeated-requeue windows above. Worst-case disruption is silent result substitution after an upgrade or worker death, so these paths must fail closed before merge.
|
Process note (owner's call): from here the live post-restart witness runs ONCE, at the head you approve, before the merge — not on every intermediate head — and the merge waits on that posted evidence. Please review the code and tests first; when the head is approved I run the witness at that exact SHA (with the kill-mid-attempt / recovery variants already prepared) and post the raw log lines before anything merges. The earlier witness comments stand as evidence for the mechanisms they exercised, not as merge evidence for the final head. |
… history through one writer, and name each refusal A cycle was judged by what complete() recorded, which runs after the provider call: an attempt that landed and then died or raised mid-send left no mark, so a later definite refusal parked the cycle as untainted and a successor body went out under an id the broker still held. begin_attempt() now writes a started-attempt mark under the item lock before the provider is called; only a classified completion clears it, and a successor attempt folds a stale mark into the cycle taint. The AG2 Space provider maps any unclassified post-send exception to ProviderIndeterminate. Parked history goes through one writer in the outbox (publish, requeue and every transition), so a requeue loop saturates at the same bound; saturation logs once and the health check counts saturated and ambiguous parks. publish() records why it refused a body and the gateway quarantine line names that cause. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ecord must prove every started attempt was classified A parked record from before attempts were tracked carries no taint and no attempt counts, and both absences read as "clean": after an upgrade in place, a mixed cycle the old code had parked as permanent-refusal admitted a successor body the broker deduped against the one it still held. A record cannot vouch for what it never recorded. complete() now counts the attempts it classifies; publish() admits a fresh cycle only when the record itself shows attempts_started == attempts_classified >= 1, so a legacy or torn record refuses — visibly, named attempt-evidence-missing — exactly like an ambiguous one. The contract says so, and the round-5 tests for the pre-send mark and the one-writer history land with it. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…hout a default The protocol method's body is the one statement of the contract change the suite never executed; a backend that forgets begin_attempt must fail at the call, not inherit silence. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 5+6 at head f0eb9c9 (three commits on top of 573ea50). Principle: fail closed — a fresh cycle is admitted only when the record itself proves the parked cycle was definite; nothing is inferred from silence. rui (5465870034)
Kewei (5465933673)
Pasted before/after, suites (152/153 and 44/45 green; the two reds are host-environmental and fail identically at 573ea50), coverage of every changed line, and the merge-order check are in the body's "Review round 5+6" section. The post-restart witness runs once at the approved head before merge (owner's call); earlier witness comments stand as mechanism evidence. |
Superseded by APPROVE at 3fe292e (pullrequestreview-5471468464); the findings in this review are verified fixed at that head.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — exact head 3fe292e84cd42467422bd9fca88cff59608a331a is not merge-ready.
Round 8 does close all three findings from my b57f02682 review: the production RetrySchedule path admits B after a definite park, both real merge-base shapes remain sticky-uncertain, boolean counters are rejected in the already-parked case, and the outbox-confirmed payload is now carried back in DrainResult. The new tests discriminate against the prior head and pass here. Two adjacent boundaries remain open.
Blockers
-
P1 — the payload proof is not bound to the result-file generation that gets archived or quarantined.
The bridge reads the live file at
remote_gateway_bridge.py:4611, compares the in-memory payload withDrainResult.dispatched_digestat 4510–4521, then archives whatever now occupies the pathname at 4753–4755 through a generation-blind rename/unlink at 3703–3708.I atomically replaced result A with result B inside the provider request, after A had been read but before confirmation returned. The exact-head production path produced:
posted_body= BODY-A archived= ['BODY-B'] quarantined= [] live_exists= False inflight= []Thus A is confirmed while the newer, unsent B is silently removed and recorded as delivered. The refusal path has the same boundary: replacing A just before
_quarantine_undelivered()quarantines B under A's refusal and removes it from the live drain. The new 5-test gateway suite stays green because it never publishes a replacement between read and disposal. Please bind retirement to the exact generation read—claim-by-rename or a compare-and-dispose identity/body guard—and test replacement at the real archive and quarantine boundaries.The planned #5269-first rebase does not by itself close the confirmed path: its current head still calls the same generation-blind
_archive_result()after success, and the two PRs conflict in this bridge. The combined resolution needs an explicit generation-aware archive test. -
P2 — a missing or boolean
attempts_classifiedvalue can be normalized into valid evidence.mark_untracked_attempts()validates onlyattempts_started. A later attempt then coerces the other counter throughint(... or 0) + 1inbegin_attempt()/complete(). Starting from READY A, one permanent refusal yielded:classified missing: parked counters=(1, 1), missing_mark=None, B_admitted=True classified None: parked counters=(1, 1), missing_mark=None, B_admitted=True classified false: parked counters=(1, 1), missing_mark=None, B_admitted=True started=1/true: parked counters=(2, 2), missing_mark=None, B_admitted=TrueThese are malformed/torn-state inputs rather than shapes the current atomic writer emits, so this is a contract/fail-closed blocker rather than the ordinary production path above. But it contradicts the PR's explicit guarantee that absent/torn/boolean evidence remains uncertain. The added boolean test checks an already-PARKED record and never exercises this laundering transition. Require both counters to be present, non-boolean, nonnegative integers before treating the tracking marker as valid; otherwise set the sticky missing-evidence mark.
Verification and remaining gates
- Exact-head focused suites pass: parked/fresh-publish 51/51, gateway confirmed-body 5/5, delivery-core contract 49 with 1 expected skip, gateway outage recovery 78/78, operator recovery 45/45, parked health 18/18.
- With the HEAD tests on
b57f02682, the two new suites fail 6 cases and 3 cases + 1 missing-helper error, respectively; the intended fixes therefore discriminate. - Vendored sync,
git diff --check, and the repository review scanner pass. Exact-head CI has 34 successful checks + 2 intentional skips,license/clais green, and diff coverage reports 192/192 changed Python lines. - Approval also remains blocked by the declared #5269-first rebase/conflict resolution and by the missing real post-restart witness on the final combined head. The latest live witness is still at
573ea50d1, not this head; REVIEW.md criterion 15 does not permit approval before that evidence.
Verdict: changes requested. Close the generation race and counter laundering, land/rebase the declared stack with combined tests, then provide the exact-final-head post-restart round trip.
Reviewed by Kewei's Codex automation.
A READY record with one counter present, a boolean, classified ahead of started, or attempts noted beyond the attempts started here was normalised into a fully classified pair by one definite attempt and then admitted a fresh body. Both counters are now validated together before anything touches them; any invalid shape sets the sticky missing-evidence mark, and an attempts count ahead of attempts_started (an older engine draining the same outbox) marks it too. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 9 is at 97f284a (one commit on top of 3fe292e; nothing amended). Point by point on the round-8 review: Blocker 2 (counter laundering) — fixed. Blocker 1 (archive/quarantine bound to the generation read) — deferred to the rebase onto #5269, stated in the body. #5269 already carries the generation identity from Suites: 67 files green under |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — exact head 97f284ab12b69f97592d6e6443ec001e4d7fc6f2 is not merge-ready.
Round 9 does fix the counter laundering reported on 3fe292e84: both counters are now validated together, including missing/None/boolean/negative/classified-ahead shapes, and attempts > attempts_started is rejected. I ran the focused file at this head (59/59 passed); running this head's test file against the parent produced the expected five failures. Two P1 boundaries remain.
Blockers
-
P1 — producer-stamped
0/0is still accepted as proof after an older consumer made an uncertain attempt.Current
publish()stampsattempts_started = attempts_classified = 0on a new record (backend_a.py:179-187). A merge-base worker can then consume that shared record withoutbegin_attempt(): its non-idempotentOUTCOME_UNKNOWNpath parks without touching these new fields, and a process dying inside the provider likewise leaves them at0/0. At this head,attempt_tracking_is_valid()accepts0/0whenattemptsis absent, sorequeue_item()does not set the sticky uncertainty mark. One later current-head refusal advances the counters to1/1;publish()then treats that as complete evidence and admits B although A may have landed.I reproduced both histories with the exact-head publisher, the merge-base delivery core, the same outbox, and a provider-side file proving A was stored:
old UNKNOWN: rc=0; after_old=PARKED/outcome-unknown attempts=None counters=0/0 after_head=PARKED/permanent-refusal attempts=1 counters=1/1 missing_mark=None cycle_ambiguous=None publish_B=True old mid-send death: rc=17; after_old=READY attempts=None counters=0/0 after_head=PARKED/permanent-refusal attempts=1 counters=1/1 missing_mark=None cycle_ambiguous=None publish_B=TrueThe new
attempts > startedtest catches only an older worker that reachednote_attempt; it cannot catchOUTCOME_UNKNOWNor death before completion. This ordering is not covered by the new tests, which either have the merge base create the counterless record or explicitly treat0/0as valid. The code comment itself claims to detect an old engine draining the same outbox, so this is within the contract Round 9 introduces. Please bind tracking evidence to the consumer/claim generation and preserve uncertainty through requeue; producer-stamped counters alone cannot prove which engine dispatched. -
P1 — result disposal is still generation-blind, so confirmation of A can silently archive an unsent replacement B.
The bridge reads A from the live pathname, compares the in-memory A to the outbox-confirmed digest, then calls
_archive_result()on the pathname (read atremote_gateway_bridge.py:4611, archive at4753-4755)._archive_result()renames or unlinks whatever now occupies that path. The quarantine arms are pathname-only too. Round 9 does not touch this file.Replacing A atomically with B after A was read but before its provider response returned still gives:
ARCHIVE_RACE: posted_body=BODY-A archived=['BODY-B'] quarantined=[] live_exists=False QUARANTINE_RACE: posted_calls=0 archived=[] quarantined=['BODY-B'] live_exists=FalseCurrent #5269 adds generation-aware quarantine machinery, but its confirmed-success archive remains generation-blind, including normal, skip/dedup, and orphan-recovery archive sites. A rebase alone therefore does not close this path. The combined change needs generation-bound archive/quarantine retirement in the shared result-lifecycle owner, plus tests that replace the file between read and each disposal boundary.
Independent merge gates
- Exact-head CI is red: 32 success, 2 failure, 2 skipped.
shell tests (lanes)genuinely failedtests/tmux-send-line.test.shcase R9; the aggregate job failed from that lane. The file is outside this diff, so PR causation is unproven, but the required gate is not green. CLA passes and diff coverage is 99%. - The declared #5269-first sequence has not happened: #5269 remains open/blocked with red CI, and this head is not rebased onto it.
- No exact-final-head post-restart round trip exists. The latest witness is still at
573ea50d1; REVIEW.md criterion 15 requires the live delivery-loop proof at the final combined head.
Verdict: changes requested. Close both loss paths, land/rebase the declared stack with combined boundary tests, restore required CI, and provide the final-head live witness before requesting another review.
Reviewed by Kewei's Codex automation.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-review at exact head 97f284ab12b69f97592d6e6443ec001e4d7fc6f2 (one commit on 3fe292e8: outbox.attempt_tracking_is_valid, plus its use in mark_untracked_attempts and _every_started_attempt_classified). No blocker in this PR from me. I'm posting a COMMENT, not APPROVE: Rui's Codex-core verification of this sha hasn't come back yet, and this head's CI is red (an unrelated flake, below). APPROVE follows once both are green. My approval at 3fe292e8 is dismissed as stale.
Counter validation: verified
-
The mixed-version residual from my last review is closed. A merge-base worker attempts a record the head already stamped and loses the response (
attempts 2 > started 1). It now getsattempt_evidence_missing: True, andpublish_B False. The other mixed variant (the base worker dies mid-send and leaves no field behind) still admits B. As I said last round, it can't be seen on the record, and the gateway singleton is the guard. -
@keweichen's new P1 #1 (at this head): reproduced, and I do not block on it.
- His variant: a merge-base worker with a non-idempotent provider takes a head-stamped
0/0record, stores A and parks itoutcome-unknown. Afterrequeue_itemthe record has no sticky mark, and one head refusal gives1/1andpublish_B True, with the broker holding A. - It is the same class as the mid-send death variant above. Both need two engine versions draining one outbox at once, and the gateway singleton exists to prevent that. My bar for that precondition hasn't changed since round 8, so this is not a blocker from me.
- But the round-9 docstring says it detects "an old engine draining the same outbox", which overclaims. Please either narrow that sentence, or add the cheap fail-closed rule. Head code never parks a record without starting an attempt (
retry_ready_lockednever parks), sorequeue_item/publishcan treat a PARKED record withattempts_started == 0as unproven. That closes his UNKNOWN variant. The mid-send death variant can only be closed by stamping the claim record with the engine that took it.
- His variant: a merge-base worker with a non-idempotent provider takes a head-stamped
-
Torn and half-present shapes all refuse. I injected 18 shapes into both a PARKED record and a READY mid-cycle record, then drove the READY one to a definite park. All 36 cases refuse with
attempt-evidence-missing, and every READY case sets the sticky mark. The controls (valid1/1,attempts 1) admit B. The shapes:attemptsset withstartedmissing;classifiedmissing,None,FalseorTrue;classified 2 > started 1;attempts 3 > started 2;startedasTrue,'1'or1.0;attemptsasTrueor'1';- negative counters;
- a fresh READY record (
started 0) withclassifiedmissing,NoneorFalse.
-
@keweichen's P2 is fixed. The last three shapes are his round-8 laundering case. At
3fe292e8they gavepublish_B True; here they refuse. -
No regressions on records this code writes:
retry_sched.py(gateway composition) admits B;- the park → park → deliver → park → deliver chain admits every new body;
- the merge-base shapes all refuse, both requeue modes included (outcome-unknown, permanent park, worker died mid-send, READY with a lost response);
- the
record_deliveredcrash, crash before/after the send, the unclassified raise and the 128-entry bound all still hold.
-
New tests: the 5 new
TornCountersStayUncertaintests fail at3fe292e8. All the changed suites pass at head (parked-fresh-publish59/59), and the vendoredoutbox.pymatches. -
Mutants of the new validation:
- drop
attempts <= started: 1 failure, and mixed-indeterminate admits B; <=→<: 15+1;- drop
classified > started: 1; - drop
is_count(classified): 1F+2E; - absent
attempts→ invalid: 14, and the gateway composition refuses B; mark_untracked_attemptsback to started-only: 5;publish()stops calling the validator: 1F+4E.
Two survive, both nits:
- dropping the
started < 0 or classified < 0clause: a negative pair still can't reachstarted >= 1 == classified; - accepting a boolean
attempts: no test pins it, though my probe shows head refuses it.
- drop
@keweichen's other P1 (the generation race at archive): pre-existing on main
I reproduced it with the gateway harness at both origin/main and this head. The live task-race1.txt is atomically replaced (A → B) inside the provider request, after A was read and before the confirmation returns. The output is identical on both trees:
origin/main: posted ['BODY-A'] archived ['BODY-B'] quarantined [] live None
97f284ab: posted ['BODY-A'] archived ['BODY-B'] quarantined [] live None
_archive_result (rename/unlink by pathname) is byte-identical on main, on #5269 and here. So this PR doesn't introduce the race or widen it on the confirmed path. Its new quarantine sites are generation-blind in the same way as main's existing terminal path.
- It is a real data-loss race, and it should be fixed. I don't hold this PR on it.
- It belongs in the generation-bound disposal work: the #5269-first rebase, where the two new quarantine sites take
generation=and_archive_resultgets the same identity binding, with a replacement-at-archive test on the combined head. Or it gets its own issue. - @keweichen, if you see a way this diff makes it reachable where
maindoesn't, please point me at it and I'll re-run.
CI at this head: red, not from this diff
32 succeeded, 2 skipped, 2 failed: shell tests (lanes) and its aggregator tsc + tests (clean install), run 37946145439, attempt 1, headSha 97f284ab.
- The gating failure is
tests/tmux-send-line.test.sh, case R9 "real stale-gate":staged=0 rc=5 … pane state changed below the prompt … (now 'Login successful. Press Enter to continue…'). That is a real-tmux timing case, and the round-9 diff touches onlyoutbox.py,backend_a.pyand one test file. - The same suite passed in CI at
3fe292e8, and it passes 2/2 locally at this head. - The other red line in that job,
sutando-migrate.test.sh, is the known non-gating failure (#5087). - No open issue tracks the tmux-send-line flake. The job needs a rerun before merge.
Still owed before merge
- the Codex-core pass at this sha;
- a green CI rerun;
- the #5269-first rebase, carrying the two items from my round-8 review: the
test_a_dead_owners_unverified_claim_goes_back_livecall count → 2, andgeneration=at the two new quarantine sites. #5269 has since moved to1c6bb492; - the live post-restart witness at the final head.
Stale: head moved to 97f284a. Re-reviewed there (pullrequestreview-5471656976); APPROVE follows once Codex-core passes this sha and CI is green.
|
Scope note on the two P1s from #5267 (review), so the changes-request has an explicit answer rather than an implied one:
Both items stay open on this PR until then. |
…ibly; defer the fresh cycle Owner's decision: ship this fail-closed and defer admitting a later, different result for a parked id until attempt evidence survives version skew (an older engine consuming a record leaves no trace a later refusal can be checked against). Kept: DrainResult.dispatched_digest and the gateway rule that a result is archived only when the payload the outbox sent is that file's payload; a later, different body for a parked or delivered id is quarantined with a named cause and the operator's requeue command, never sent and never archived; the parked body itself stays refused without a resend. Removed (restored to main): the fresh-cycle admission and the machinery that existed only to justify it -- begin_attempt / dispatch_pending, attempts_started / attempts_classified, cycle_ambiguous, attempt_evidence_missing, parked_digests history and saturation, the schema marker, the fresh_cycle_after_definite_park capability, the provider catch-all, and the health-check counts of those states. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
sonichi
changed the title
|
@john-the-dev @keweichen — the owner has decided to defer the fresh-cycle feature and ship this PR fail-closed. New head 8cf269c (two commits on 97f284a: 2be06a8 narrows the contract, 8cf269c restores a How each open P1 is answered by the narrowed contract:
Kept from round 8: Checks at 8cf269c: 66 suite files green, package unittest OK, vendored sync in sync, review-checks PASS, changed-statement coverage 25/25. Merge order: #5269 → the #5279 fix → this PR rebased; the live witness runs once at the approved head. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-review at exact head 8cf269c4e33eb66f6098b60215d406de5dd5db35. The net diff against the merge base d37b79474 is 5 files, +285/−3: contract.py, core.py, remote_gateway_bridge.py and two test files. No blocker in this diff. I'm posting a COMMENT, not APPROVE, for two reasons. First, I haven't seen a Codex-core pass for this exact sha. Second, the point-by-point says the head to approve is the rebase onto a main that already contains #5269 and the #5279 fix. APPROVE follows at that head once Codex-core passes it.
Fail-closed for every parked-id path: verified at the gateway, not just the backend
The backend is now byte-identical to the merge base, so publish() on a PARKED id refuses every payload. I drove _post_ready_results through each outbox state with B in the live file. Each row is a fresh outbox under the production composition (retry_schedule=RetrySchedule(), republish_delivered=False):
| state | head | merge base |
|---|---|---|
| PARKED (permanent) → B | 0 POSTs, B quarantined, "a later, different result for a parked outbox id is refused (the parked body: …)" | 0 POSTs, B quarantined, park reason only |
| PARKED (permanent) → the same A again | 0 POSTs, A quarantined (no resend) | same |
| PARKED (outcome-unknown) → B | 0 POSTs, B quarantined with the cause named | quarantined, park reason only |
| DELIVERED A → B | 0 POSTs, B quarantined | B archived as delivered (silent drop) |
| DELIVERED A, crash before archive, rescan A | 0 POSTs, A archived | same |
| READY A stored but unsent, live file B | 1 POST (A), B quarantined | 1 POST (A), B archived (silent drop) |
- No double send in any row. Every refused body goes to
undelivered/with a named cause and the requeue command; none is dropped or archived unsent. - The two silent drops on the merge base (DELIVERED-different and the stored-A/live-B confirm) are the real fixes here.
- Every
_archive_result(rfile, …)site is gated on_deliver_result_payload(...)returning True: the dedup report, the skip/lease-close path, the normal drain, and orphan recovery._deliver_result_payloadreturns True only on DELIVERED-with-matching-payload or CONFIRMED-with-matching-digest. So "only the sent payload is archived" holds, apart from the pathname race below.
Removing the counter path closes @keweichen's P1 #1
- Every earlier repro now refuses B with no POST:
- the mixed-version variants (base worker indeterminate; base worker dies mid-send; base non-idempotent
outcome-unknownpark, then requeue); - the merge-base shapes (outcome-unknown and permanent parks through requeue, with and without
--reset-attempts; worker died mid-send; READY with a lost response); - the
record_deliveredcrash, crash before/after the send, the unclassified raise.
- the mixed-version variants (base worker indeterminate; base worker dies mid-send; base non-idempotent
retry_sched.pyandchain.pynow refuse every body after the first park, as the narrowed contract intends (deferred to #5280).- No dangling references:
git grepforbegin_attempt|attempt_tracking|mark_untracked|stamp_attempt|fold_parked_digest|parked_digests|last_refusal|dispatch_pending|attempt_evidence_missing|cycle_ambiguous|fresh_cycle_after_definite_park|PARKED_HISTORY_LIMITreturns nothing.delivery-core-parked-fresh-publish.test.pyis gone, along with every added contract and operator-recovery test that exercised the removed path.src/outbox.pyand its vendored copy match..coveragercis unchanged against the base.
Tests and mutants
- At head:
gateway-archive-only-the-confirmed-body8/8,gateway-outage-recovery78/78, and also contract, operator-recovery, health-check-parked, enforcement, crash-between-terminal-and-release, terminal-not-contended, ag2space-provider, orphan-reconciler and owner-mention, all green. Againstorigin/main(7495effb), the two changed suites fail 4F+1E and 2F respectively. src/remote-gateway-bridge.test.pyshows its usual 5 host-environment failures here; that count is unchanged by every mutant except one.- Mutants, all killed:
- the DELIVERED compare always matches: 2
- the CONFIRMED digest compare is removed: 2
- the parked-cause wording branch is removed: 1 + 2 in outage-recovery
_record_holds_payloadalways returns False: 2- the core hashes the wrong bytes: 1 + 12, plus 6 more in remote-gateway-bridge
- the core omits
dispatched_digest: 2 - the CONFIRMED-other arm returns True: 2
Non-blocking
- Missing digest skips the guard.
if sent is not None and sent != …means aDrainResultwithoutdispatched_digestskips the guard and archives.DeliveryCoreis the only producer and always sets it, and dropping it is a killed mutant, so this is a nit: failing closed (or comparing the stored record) when the digest is absent would remove the escape. - A legitimate rescan can be quarantined. The DELIVERED-different arm keys on exact payload bytes. If the rebuilt envelope differs between passes for the same file, a delivered result is quarantined (visibly, not lost), for example when attachment outcomes change or
metadata.worker_idchanges after a crash between confirm and archive. Worth a line in #5279's work.
Combination with #5269 at b1f5be1c
- The merge still conflicts in
remote_gateway_bridge.py. I resolved it like this: keep_dispose_terminal, addpayload=so it names the "later, different result" cause, and passgeneration=at the two new quarantine sites. - All ten suites pass on the combined tree, including
gateway-disposal-delegation(61) andgateway-terminal-result-moved-once(51). test_a_dead_owners_unverified_claim_goes_back_liveis now moot and passes 3/3 as written. With the fresh cycle gone, NEWER is refused without a POST, so the original "never re-POSTed" assertion holds and no change is needed.- The #5279 race still reproduces on the combined tree, as expected until that fix lands:
posted ['BODY-A'] archived ['BODY-B'].
CI at this head
All 34 check runs are complete: 32 succeeded, 2 skipped (PowerShell, publish). license/cla passes. review-checks.sh on gh pr diff 5267: PASS. GitHub reports MERGEABLE/BLOCKED (the review gates).
Owed before merge
- #5269 lands.
- The #5279 fix lands.
- This PR is rebased onto that
main, with the replacement-during-POST regression the point-by-point promises. On the rebase, carry the parked-cause wording into_dispose_terminaland passgeneration=at the two new quarantine sites. - Codex-core passes that exact head.
- The live post-restart witness runs at that head.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
COMMENT — exact head 8cf269c4e33eb66f6098b60215d406de5dd5db35. Under the owner’s narrowed fail-closed scope, I found no new in-scope code blocker. I am not approving or clearing my existing change request yet: this head still intentionally depends on the #5269 → #5279 fix → rebase #5267 sequence, and the exact-final-head live post-restart witness is still owed.
1. Old-engine 0/0 → 1/1 laundering: resolved by removing the feature
This is no longer another counter heuristic. DesignAClaimBackend.publish() is back to the fail-closed rule: every existing PARKED record returns False, regardless of reason, payload, or counters. The contract now states that a park is final and defers fresh-cycle admission to the version-skew-safe follow-up, #5280.
The current archive/body regression is 8/8 green at this head. Copied onto 97f284ab1, it fails two cases; critically, the valid-looking attempts_started=1 / attempts_classified=1 park POSTs B there, while this head makes zero POSTs, leaves the item PARKED, and quarantines B. A mixed-version production-class probe shows the same distinction: the prior recovery engine admits B after the old engine turns 0/0 into 1/1; this head still returns publish(B)=False.
2. Only the payload actually sent is treated as delivered: verified
DeliveryCore.deliver_one() hashes the backend’s stored payload—the bytes actually handed to the provider—and returns that as dispatched_digest. The bridge checks both terminal-record payload equality and the active attempt’s dispatched digest before allowing its caller to archive (terminal path, confirmed path).
Verified through the real Backend A/core/provider/bridge composition:
- old DELIVERED A + live B: zero POSTs; B is quarantined, never archived;
- queued A + restored live B: A is POSTed and recorded DELIVERED; B is quarantined, never archived;
- matching confirmed body: archived as before;
- PARKED A + later B: zero POSTs; B is quarantined with the named park reason.
The focused suites pass: archive/body 8/8, gateway outage/recovery 78/78, delivery-core contract 43/43 with one expected skip, enforcement 15/15, terminal 5/5, crash-boundary 3/3, and operator recovery 40/40. The head archive tests copied to the merge base fail 4 cases plus one missing-helper error, including both silent-archive schedules, so the regression discriminates.
3. Generation-blind pathname retirement: still a required external gate
The pre-existing replacement race remains exactly as declared in #5279: the bridge reads A from the live pathname and, after A is confirmed, still archives whatever occupies that pathname at the later _archive_result() call. That is not introduced or widened by this cumulative diff, but this exact head is intentionally not merge-ready while it remains. The declared mitigation is the right one: land #5269’s lifecycle owner, land the #5279 generation-bound archive fix, rebase this PR, and rerun the combined archive/quarantine boundary tests.
Verification and verdict
- Exact-head CI: 33 passed, 2 intentional skips; CLA and diff coverage pass.
- Package unit tests: 22/22; vendored sync is clean.
git diff --checkandreview-checks.shpass; the hardcoded-path scanner’s positive control fails as expected.- No exact-final-head post-restart delivery-loop witness exists yet.
Worst-case disruption under the narrowed design is deliberate fail-closed withholding: a legitimate later answer for a PARKED id remains visible in results/undelivered/ and needs operator recovery/re-asking until #5280 supplies a safe fresh cycle. That is preferable to silently claiming the wrong body was delivered. The more severe replacement-loss risk is #5279 and remains a merge prerequisite.
Verdict: COMMENT, not approval. No further change is requested to this pre-rebase head for the retired fresh-cycle feature. Re-review after #5269 and the #5279 fix land, this PR is rebased with combined generation tests, and the real post-restart witness is posted at that exact final SHA.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at 8cf269c.
This converts my round-10 review (5472174482, no blocker) now that the Codex-core compatibility check PASSED this exact sha. It was posted in the Sutando Release room and covers both current-code and legacy-code crash repros: A stays PARKED, B is not accepted and nothing is sent.
My verified findings stand as written in 5472174482:
- Fail-closed: every parked-id path is fail-closed, and two silent-archive cases on the merge base now quarantine B.
- Kewei's P1 #1: closed by removing the counter path.
- CI: green.
This approval is for this sha only. The agreed merge order is #5269, then the #5279 fix, then a rebase of this PR with:
- the "file replaced during send" regression test;
payloadthreaded into_dispose_terminal(for the_record_holds_payloadcheck);generation=at both new quarantine sites.
The rebased head needs its own Codex check and review. keweichen's CR at 97f284a and the live witness remain owed.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed head 8cf269c4. The diff is correct, and I found no blocker in it. I am not marking it ready to approve, because the reviewers and the author all agree that this head is not the one to merge. The agreed order is #5269, then the fix for #5279 (the race where a result file is replaced during the send), then a rebase of this PR. The rebased head needs its own review. keweichen's CHANGES_REQUESTED (filed on 97f284ab) also still stands. His COMMENT at this head explicitly does not clear it, and a COMMENT never clears a CR.
What I ran (fresh worktree, env -i):
gateway-archive-only-the-confirmed-body: 8/8 OK.gateway-outage-recovery: 78/78 OK.- Mutation: I made the bridge ignore
dispatched_digest(remote_gateway_bridge.py:4504,sent = None). The archive suite then fails 2 tests. With the file restored, it passes. scripts/review-checks.sh --diff: PASS. CI is green (33 pass, 2 skipped),license/clais success, and the PR is mergeable. john-the-dev's APPROVE at this head is substantive and verified.
Owed before merge (all already named by the reviewers):
- #5269 lands. It currently has CHANGES_REQUESTED from both keweichen and john-the-dev at its head.
- The #5279 fix lands.
- This PR is rebased on top of both, with three additions:
- the regression test for a file replaced during the send;
payload=passed into_dispose_terminal;generation=at the two new quarantine sites.
- A Codex-core pass and fresh reviews at the rebased head.
- A live round trip at the final head, after a restart (named, not blocking on its own).
Nit (john-the-dev's): when dispatched_digest is absent, the guard is skipped. Today the only producer always sets it, but failing closed when it is missing would remove that gap.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Code-owner approval at 8cf269c (Qingyun's decision in the [PA] pending-actions database). The merge stays with Chi.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Commenting at 8cf269c4e. This is not a change request. It updates my review of 2026-10-09, because the order it named has moved on.
Where the agreed order stands
- #5269 merged on 2026-10-10.
- #5285 ("archive only the result generation that was sent", the fix for the race in #5279) merged on 2026-10-12. Issue #5279 itself is still open.
- So the next step is the rebase of this PR, and it is now required: the PR conflicts with main (209 commits behind). A dry merge conflicts in
packages/ag2-sparrow/ag2_sparrow/delivery_core/core.pyandpackages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py.
Why I'm not approving
- keweichen's CHANGES_REQUESTED (filed at
97f284ab1) still stands. A COMMENT does not clear it. - john-the-dev's and qingyun-wu's approvals are at
8cf269c4e. A rebase that resolves conflicts in the delivery core needs fresh review, so they will not cover the head that merges.
Owed at the rebased head (from the reviewers, unchanged)
- The regression test for a file replaced during the send, now against #5285's code.
payload=passed into_dispose_terminal, andgeneration=at the two new quarantine sites.- A Codex-core pass and fresh reviews, including keweichen's.
- A live round trip after a restart (named, not blocking on its own).
CI at 8cf269c4e is green and license/cla is success (1 context). No unresolved threads.
— Sutando (susans-MacBook-Pro)
|
Closing on the owner's decision (2026-10-23). Main already quarantines a different body under a delivered or confirmed outbox id (delivered_body_differs + _quarantine_unsent in remote_gateway_bridge.py, generation-bound, via #5269), which also covers the generation-binding P1 raised here. What this PR still added (the park-reason wording, DrainResult.dispatched_digest, the publish() contract docs) can ride on #5280, the follow-up for admitting a later body. 209 behind main with conflicts; branch left in place. |
Refs #3843 (thread 2)
Why
Thread 2 of #3843: once an outbox id was parked, a later result for the same task was discarded with no POST — the gateway reported the parked body's reason, and in the delivered-id case it archived a body the broker never saw as if it had gone out.
What (fail-closed; owner's decision)
ClaimBackend.publish()contract; Backend A's behaviour on main already refused, now stated and enforced end to end).undelivered/with one log line that names the cause —a later, different result for a parked outbox id is refused (the parked body: outbox item is terminal: <reason> after N attempt(s))— and the operator'srequeue … --results-dir … --body-id …command. The parked body itself is still refused without a resend.DrainResult.dispatched_digestcarries the sha256 of the bytes handed to the provider; the gateway quarantines (never archives) a different live body under a delivered id, or a restored body while the outbox sends its stored earlier one.Net diff vs main: 5 files, +285/−3 (round 9 was 15 files, +2093/−24).
Deferred
Admitting a later result after a definite refusal (the "fresh cycle") is deferred to #5280: ten review rounds showed it needs attempt evidence that survives version skew (an older engine consuming a record leaves no trace), which record state alone cannot provide. All of that machinery (dispatch mark, attempt counters, cycle taint, evidence marker, parked-body history and saturation, the backend capability) is removed from this PR; the appendix keeps the round history.
Tests
tests/gateway-archive-only-the-confirmed-body.test.py(8): a later different body for a parked id is quarantined with the named cause, zero POSTs, id stays PARKED; the parked body itself is refused without a resend; no record shape an older engine could leave (clean 0/0 or 1/1 counters,outcome-unknown,retry-window-exhausted) reopens the id — B is refused and never sent; old-DELIVERED replay and QUEUED-A/restored-B are quarantined, not archived; the confirmed body is archived as before; the no-file path logs and does not retry.tests/gateway-outage-recovery.test.py: after an ambiguous park (retry-window-exhausted) a later body is quarantined naming both the refusal and the parked reason, zero POSTs.delivery-core-*,outbox-*,sparrow-outbox-*,gateway-*(incl..sh),health-check-*outbox*,bridge-marker-no-leak— 66 files green underenv -i; packageunittest discoverOK;tools/sync_from_src.py --checkin sync;git diff --checkclean;scripts/review-checks.shPASS. Changed-statement coverage 25/25.Not closed here: keweichen's producer-replacement race (a file replaced during the POST is archived by pathname) reproduces on main and at this head; it is #5279, fixed on top of #5269 lifecycle owner.
Merge order and witness
#5269 → the #5279 fix → this PR rebased (the rebased head carries the generation-bound archive and its replacement regression). The post-restart live witness runs once at the approved head before merge.
Appendix: review rounds 1-9 (history, superseded by the fail-closed scope above)
Refs #3843 — this PR is thread (2) of that issue (a parked id discarding later results). Thread (1), the retry cadence, was already fixed by #5191; thread (3), a terminal item's live result file rescanned and logged on every pass, is a separate follow-up PR, so the issue stays open until that lands. — defect (2) only; the retry-cadence/backoff half (defect 1) is a separate change and will come as its own PR naming this one as parent.
Why. From the issue: "A terminal outbox item poisons the task id permanently. Writing a fresh result file for the same task is quarantined with NO new POST attempt … The new result file disappears from
results/exactly as a successful delivery does, so the documented recovery (write a result file) is a silent no-op indistinguishable from success." The cause is in the delivery core's backend A:publish()refused any id whose record wasPARKED("the operator holds it"), so the gateway's_deliver_result_payloadsawTERMINALfor the new body and quarantined it. Backend C already accepted a fresh publish for a parked key by construction (a parked key has no READY link), so A and C disagreed.What.
packages/ag2-sparrow/ag2_sparrow/delivery_core/backend_a.pypublish(): aPARKEDrecord now holds only the payload that parked. Publishing a different payload under the same id is a later reply and starts a fresh cycle —READY,attempts0,resend_epoch + 1(so the idempotency key cannot dedupe against the parked attempt, same mechanismrequeue_itemuses), and the replaced park kept on the record assuperseded_park(attempts, reason, published_at). Re-publishing the same payload stays refused, so a live file rescanned every pass cannot turn one park into a retry per pass.DELIVEREDrepublish is unchanged. The contract docstring incontract.pysays so.src/outbox.pyand the vendored copies are untouched (tools/sync_from_src.py --check: in sync); the delivery core is package-canonical.Regression test
tests/delivery-core-parked-fresh-publish.test.py(backend A, provider refuses to the cap): the parked payload re-published over five passes makes zero provider calls and staysPARKED; a different payload publishes, is attempted once with a key not among the parked attempt's keys, and confirms; a fresh result that fails again parks again and then holds; aDELIVEREDid behaves as before.Before (parent
d37b79474):After (HEAD):
Also run at HEAD (all green,
env -u SUTANDO_WORKSPACE_DIR -u SUTANDO_TASKS_DIR -u SUTANDO_RESULTS_DIR -u SUTANDO_INSTANCE_ID -u REMOTE_TASK_* -u AGENT_CONNECT_*):tests/delivery-core-*.py(contract incl. "a parked item is not re-drained", crash-between-terminal-and-release, enforcement incl. "a PARKED item must not be claimed automatically" and "an operator requeue must present a NEW key", terminal-not-contended),tests/sparrow-outbox-claim-protocol.test.py, alltests/outbox-*.py,tests/bridge-marker-no-leak.test.py,tests/health-check-outbox-parked.test.py,tests/proactive-undeliverable-claim-released.test.py,tests/list-undelivered-replies.test.py;packages/ag2-sparrow:python3 -m unittest discover -s tests(22 OK) andtools/sync_from_src.py --check(in sync).git diff --checkclean;scripts/review-checks.shPASS.Live path, stated plainly. This is the gateway's outbound delivery loop. No post-restart round trip was run for this PR. What it takes: a gateway started from this worktree under
env -ion a scratch workspace, one result forced to park (a relay refusal), then a second, different result written for the same task id, and the gateway log showing a POST for it (not a quarantine) — that witness needs the owner's go on this host and is arranged separately.Review round 2 (head c562b2b)
Answers to the non-blocking findings in the round-1 review, each pinned by a test in
tests/delivery-core-parked-fresh-publish.test.pythat fails at b0ff067 and passes at this head (same file, 9 tests; the four new behaviour tests fail at the parent, the strengthened DELIVERED assertion passes at both because the rule was already right and only the assertion was weak):delivery_core/backend_a.pyPARKEDkeepsparked_digests(sha256 of the raw bytes, last 32) andpublish()refuses any payload whose digest the id ever parked on; a never-parked body still gets a cycletest_two_payloads_cannot_alternate_through_the_park(the 6-alternation probe: 0 provider calls after both parks, epoch stays 1)publish(B)False → quarantine_record_is_reclaimable(rec, 0.0): the cycle ended at the park, so no TTL applies); a running owner still refusestest_a_dead_claim_left_on_a_park_does_not_refuse_the_fresh_result,test_a_live_claim_on_a_park_still_refusespublish()'s docstring and theClaimBackend.publishcontract: the ambiguous body itself is never resent by this path (its digest is parked);defer_idempotent_resendgoverns that body alonepayloadacceptedb""_parked_digests()returns None when the park recorded no payload, andpublish()then refuses everything (nothing can be proven different)test_a_park_that_recorded_no_payload_refuses_even_an_empty_one"replace"decode; legacy records withoutpayload_digestderive theirs from the stored texttest_invalid_utf8_payloads_are_told_apart_by_their_bytesrepublish_delivered=Falserefuses THIRD and the status stays DELIVERED; the default accepts THIRD)test_a_delivered_id_is_unchanged_by_this_ruleBefore (b0ff067, same test file):
After (c562b2b):
Also green at this head (env -u SUTANDO_WORKSPACE_DIR/TASKS_DIR/RESULTS_DIR/INSTANCE_ID/REMOTE_TASK_/AGENT_CONNECT_, temp dirs only): every
tests/delivery-core-*,tests/outbox-*,tests/sparrow-outbox-*,tests/bridge-marker-no-leak.test.pyandtests/gateway-*suite (61 files) excepttests/gateway-owner-presence-peer-gate.test.pyandtests/gateway-proactive-foreign-homeserver.test.py, which fail identically at b0ff067 on this host ("predicate: no identity -> no homeserver") and touch nothing this PR changes; packageunittest discover -s tests: 22 OK;tools/sync_from_src.py --check: in sync;git diff --checkclean;scripts/review-checks.shPASS.Not changed: the legacy case of a record written before
payload_digestexisted whose parked body was invalid UTF-8 — its derived digest comes from the stored text, so that one body could be accepted once more; records written at this head carry the raw digest.Review round 3 (head dad8b2d)
Both round-2 reviews (keweichen, john-the-dev) share one P1: the fresh cycle's resend epoch never reaches the broker, which dedupes on the envelope result id, so after an ambiguous park (lost responses, exhausted retries, outcome-unknown) a different result B was archived DELIVERED while the broker still held A — a silent loss where the pre-PR behaviour quarantined B visibly. This round narrows the rule instead of trying to carry the epoch across the wire.
Rule now (contract wording in
contract.pyClaimBackend.publishandbackend_a.py): a parked id admits a never-parked payload only when the park is a definite refusal (permanent-refusal: the broker declined the body, or nothing was sent, so it holds nothing for the id); every other park refuses every payload, and the gateway quarantines the new result where an operator sees it. Every payload the id ever parked on stays refused — no cap, and the history survives a later delivery of the same id. A torn claim on a park refuses (unknown is never stolen, like every reclaim path). The rule is declared throughBackendCapabilities.fresh_cycle_after_definite_park(A: True; C: False) and the contract suite asserts it only where declared.AG2SpaceResultProviderwith a deduping broker that loses responses (ThroughTheRealProvider)publish(B)True; B reported DELIVERED, broker body still Apublish(B)False,deliver_oneTERMINAL, record stays PARKED, broker body A, no POST for B; after a definite refusal B is delivered and the broker holds BTrue is not false)test_a_torn_claim_on_a_park_refusesfresh_cycle_after_definite_park=Falseon C;ContractCase_Casserts a live id refuses every other payload;ContractCase_Aasserts ambiguous-park refusal and definite-refusal admissiontest_a_legacy_park_without_a_digest_is_judged_by_its_stored_text,test_a_delivered_id_with_a_live_claim_refuses; the regression file alone covers every line ofpublish()Carrying the epoch to the broker (reviewers' alternative) was not done: the broker matches a result to its task lease by the envelope
id, and this repo cannot change what the broker dedupes on; the narrowing above is the safe rule from this side. The resend epoch still changes the local idempotency key for a definite-refusal successor, as before.tests/delivery-core-parked-fresh-publish.test.pyat c562b2b (the new file copied over):At dad8b2d:
Ran 18 tests … OK; contract suite 45 OK (1 pre-existing skip).Suites at this head (
env -i, temp dirs): 65 of 66 files intests/delivery-core-*,outbox-*,sparrow-outbox-*,gateway-*(incl. the two.sh),bridge-marker-no-leakgreen;gateway-voice-room-membershipis intermittent on this host (1 red in 3 runs at this head, 2 of 2 green at the parent; it asserts a cached gateway-read count and touches nothing in this diff). Packageunittest discover -s tests22 OK;tools/sync_from_src.py --checkin sync;git diff --checkclean;review-checks.shPASS.Live witness: the earlier runs (d37b794, b0ff067, c562b2b) exercised the definite-refusal path only. A re-run at this head with a deduping fake broker that also loses responses — showing the ambiguous park → quarantine (no POST for B) and the definite refusal → POST, with raw log lines — follows as a PR comment.
Review round 4 (head 573ea50)
Answers Kewei's review 5465597114 and rui's 5465618053 at dad8b2d. Before = the new tests copied onto dad8b2d; after = 573ea50. Run:
python3 tests/delivery-core-parked-fresh-publish.test.py(27 tests),tests/delivery-core-contract.test.py(45),tests/outbox-operator-recovery.test.py(41).ok:false/ 503-then-refusal parked aspermanent-refusaland admitted B while the broker held ADeliveryCorepassesambiguous=Truetocomplete()whenever an attempt in the call was indeterminate (set before the idempotent re-send folds it to NOT_DELIVERED; a reconcile receipt clears it); Backend A persistscycle_ambiguouson the item record under the lock, so it survives retries, restarts and an operator requeue;publish()refuses a successor for a tainted park whatever its final reason; a legitimately fresh cycle starts untaintedTheWholeCycleMustBeDefinitethrough the realAG2SpaceResultProvider+ a scripted deduping broker (lose/http:503thenhttp:409/ok:false): before 5 FAIL (None is not true : the taint is written before the fold), after OK — B stays PARKED/quarantined, 0 POSTs for B, broker holds A; restart and requeue variants includedparked_digests;cleanup()claim falsePARKED_HISTORY_LIMIT = 128per id; a park past it setssaturatedand the id refuses every new payload (still PARKED, visible);cleanup()states the cap; contract invariant 4 says soHistoryIsBounded: 2000 distinct bodies → exactly 128 cycles, recordsaturated,_parked_digests128, record < 12 KB, further publish False,deliver_oneTERMINAL; before ERROR (no constant) / FAILDesignCClaimBackend.publish()refuses any id with anundelivered/<key>~*entry whenfresh_cycle_after_definite_parkis False; Backend A'spublish()reads its flag too (a park refuses everything when False)FAIL ContractCase_C … a park is final for the parked body, after OK; mutant flipping A's flag to False: 12 of 27 failoutbox.requeue_item()(src-canonical, vendored copy synced) foldspayload_digest— or the sha256 of a legacy record's text — intoparked_digests; the explicit retry goes out, the delivered-republish path refuses that body afterwardsRequeueKeepsTheParkedBody+outbox-operator-recovery::test_requeue_keeps_the_parked_body_in_the_ids_history: before ERROR /['d-z'] != ['d-z', 'd-a'], after OK.coveragerc(concurrency = multiprocessing): backend_a 16/16, backend_c 5/5, core 5/5, src/outbox 8/8Not done, stated: carrying the resend epoch to the broker's dedup key (round 3) — the broker matches a result to its lease by envelope
id; this repo cannot change what it dedupes on, so the fix stays on the admission side. Torn/UNKNOWN claims keep refusing (round 3) and are pinned.Suites at 573ea50 under
env -i: alldelivery-core-*,outbox-*,sparrow-outbox-*,gateway-*(incl. the two.sh),bridge-marker-no-leak— 65 python files + 2 shell, all rc 0; packageunittest discover -s tests22 OK;tools/sync_from_src.py --checkin sync;git diff --checkclean;review-checks.shPASS.The live witness is re-run at this head with the mixed scenarios (lost-then-refusal) by the driver, not by this round.
Review round 5+6 (head f0eb9c9)
Rule: fail closed on every uncertainty. A parked id admits a fresh cycle only when its record proves the cycle was definite; nothing is inferred from an absent field, a last reason, or a count the code never wrote. Three commits on top of 573ea50 (
8aae2c6d4,0f2369fe6,f0eb9c9f3); gateway bridge delta unchanged apart from the four-line quarantine wording from round 5.complete(), after the provider call: an owner killed mid-POST, anIncompleteRead, a garbled 200 left no mark; a later definite refusal admitted Bbegin_attempt()persistsdispatch_pending+attempts_startedunder the item lock BEFORE the provider is called (core.deliver_one); onlycomplete()clears the mark and countsattempts_classified; a successor'sbegin_attemptfolds a stale mark intocycle_ambiguous;AG2SpaceResultProvidermaps every unclassified post-send exception toProviderIndeterminateexit 17),IncompleteRead, garbled 200,KeyErrorafter send → at 573ea50 B admitted / at headpublish(B)=False,last_refusal=parked-cycle-ambiguous, broker holds Acycle_ambiguous, no counts — both absences admitted Bpublish()requiresattempts_started == attempts_classified >= 1on the record itself; otherwise refusesattempt-evidence-missinggit archive d37b79474→ baseDeliveryCoredrives the mixed cycle → head opens the unchanged outbox), a torn count: at 573ea50publish(B)=True/ at head refused, PARKED stays visible,deliver_one(B)→ TERMINAL (quarantine)outbox.fold_parked_digest(src-canonical, vendored) used bypublish,requeue_itemand every transition; saturation setssaturated, logs oncelast_refusalrecorded per cause; gateway line readsfresh cycle refused: <cause> (outbox item is terminal: …); health-checkoutbox-parkedcountsN saturatedandN ambiguous-cycle; contract documents the operator recovery (requeue of the parked body under a new resend epoch; restore of quarantined bodies)test_each_refusal_names_its_cause,health-check-outbox-parked::test_saturated_and_ambiguous_parks_are_counted_distinctlycomplete()with a stale token skips the taint writecomplete()writes nothing, which is safe only becausebegin_attemptalready left the started mark the next incarnation foldstest_a_classified_attempt_leaves_no_pending_mark, contracttest_begin_attempt_writes_nothing_for_a_token_that_lost_its_claimtest_a_reconcile_receipt_clears_the_ambiguity: a receipt resolves the cycle, nocycle_ambiguouson the recordambiguous=Trueafter reconcile) fails itbegin_attemptvalidates the token only; it admits no fresh cycle after a park, so an unclassified attempt cannot be mistaken for a definite one (comment in code)Pasted: the final regression files run at 573ea50 vs f0eb9c9
Legacy-record evidence (
test_a_record_the_merge_base_wrote_refuses_a_fresh_cycle): the merge base's package is extracted withgit archive d37b79474, its ownDeliveryCore+ Backend A drivepublish(A)→ indeterminate call → permanent refusal (the shape in Kewei's probe:status PARKED, reason permanent-refusal, cycle_ambiguous absent, no attempt counts); the head then opens that outbox unchanged:publish(B)=False,last_refusal=attempt-evidence-missing, item stays PARKED,deliver_one(B)→ TERMINAL. Skips only when the base commit is unreachable from the checkout; the on-disk fixture test runs everywhere.Suites at head (
env -uworker-routing vars, temp dirs): 153 files acrossdelivery-core-*,outbox-*,sparrow-outbox-*,health-check-*,bridge-marker-no-leak→ 152 green;health-check-fix-down-bridgesfails identically at 573ea50 on this host (a live slack-bridge is running here, so its "configured but not running" case cannot hold — environmental). 45gateway-*files → 44 green;gateway-owner-presence-peer-gatefails under theenv -ulist at 573ea50 too and passes underenv -i(worker env leak, environmental). Packageunittest discover -s tests: 22 OK.tools/sync_from_src.py --check: in sync (src/outbox.py,src/health-check.pyare the canonical edits).git diff --checkclean;review-checks.shPASS.Coverage of lines changed vs
origin/main(coverage run --rcfile=.coveragercover the delivery/outbox/health-check suites, intersected withgit diff -U0 origin/main):backend_a.py165 added lines, 0 uncovered;backend_c.py20/0;core.py12/0;provider_ag2space.py5/0;src/outbox.py48/0;src/health-check.py10/0;contract.py64 added, 0 uncovered after the protocol-body test.Merge order with #5269: a scratch merge of #5269's head
c2ba5fedbonto this head CONFLICTS inremote_gateway_bridge.py— #5269 moved the terminalwhyconstruction into_dispose_terminal, and this PR's round-5 wording (fresh cycle refused: <last_refusal> (…)) sits on the same lines. Proposed order: #5269 first; this PR then rebases and carries thelast_refusalwording into_dispose_terminal(four lines).delivery-core-parked-fresh-publishis unaffected by the merge (39/39 on the merged tree).Witness: the post-restart witness runs once at the approved head before merge (owner's call); the earlier witness comments stand as mechanism evidence. The kill-mid-attempt and garbled-200 variants (SIGKILL the gateway while the broker holds A's response, restart, definite refusal, then B) are prepared for that run.
Review round 7 (rui's round-6 review + the second P1 from Rui's Codex-core verification)
The sticky-taint rule (commit 4662be8). Certainty is never inferred from silence, and an attempt made before attempts were tracked can never be laundered:
begin_attemptmarks a record that carriesattempts > 0(or a retry record) withoutattempts_startedasattempt_evidence_missingBEFORE it starts counting;requeue_itemsets the same mark BEFORE--reset-attemptszeroes the only trace;publish()refuses on the mark (attempt-evidence-missing); only a new cycle rebuilds the record without it.requeue_itemclearslast_refusaland the DELIVERED arm records its own cause, so a quarantine line never names a stale one; the health check counts parks without attempt evidence; the provider catch-all is pinned by a strict test (deleting it fails).The second P1 (commit b57f026): the mark outlives the outcome write.
complete()cleareddispatch_pendingand counted the attempt as classified in a write that preceded the outcome's own write (record_delivered()/park_item()/ retry note), so an owner that died enteringrecord_delivered()after the broker confirmed A leftREADYwithstarted == classifiedand no taint; a later refusal parked the cycle as untainted and admitted B. Now the outcome transition is written first and the mark is cleared in a write after it: a crash before the outcome is durable leaves the attempt unclassified, and the nextbegin_attemptfolds the stale mark intocycle_ambiguous. A crash between the two writes leaves the mark on a durable outcome; a PARKED record then refuses (attempt-unclassified) — visibly, the operator's requeue recovers it — and a DELIVERED record's republish is already a new cycle.git archive d37b79474, one lost-response attempt) opened by the head and driven to a definite parkpublish(B)True → B DELIVERED while the broker holds Aattempt-evidence-missing, B quarantinedrequeue_item --reset-attempts, then definitely refusedpublish(B)Truebegin_attempton a record withattempts > 0and no countersnext_attempt_at, no counters)record_delivered()after the broker confirmed A (real child,os._exit(17)patched in), dead claim reclaimed, later refusalREADY, started 1 == classified 1, no taint →publish(B)TrueREADY, dispatch_pending True, started 1 ≠ classified 0→ cycle tainted →publish(B)False,last_refusal parked-cycle-ambiguous, broker holds A, no POST for Blast_refusalafter requeue + confirmed deliveryattempt-evidence-missingattempt-evidence-missingparks counted as plain PARKEDPasted (HEAD test files copied onto f0eb9c9,
env -uworker vars, temp dirs):Changed-statement coverage under
.coveragerc(the four regression files alone):backend_a.py87/87,src/outbox.py40/40,src/health-check.py8/8,contract.py3/3 — 100%.Merge order. #5269 lands first. This PR then rebases, carries its four
last_refusallines into_dispose_terminal, and re-decidestest_a_dead_owners_unverified_claim_goes_back_live(under this PR a definitely parked, untainted id may accept a later different body; that test's "never re-POSTed" assertion must be scoped to tainted/ambiguous parks). The post-restart witness runs once at the approved head before merge (the kill-mid-attempt and garbled-200 variants are prepared); earlier witness comments stand as mechanism evidence.Review round 8 (rui's and Kewei's round-7 reviews; head 3fe292e)
The schema marker (commits 8a12b5a, 3fd5f49, 3fe292e;
src/outbox.py, vendored byte-equal). A record now proves its own attempt tracking:publish()stampsattempts_started: 0, attempts_classified: 0on every fresh record (stamp_attempt_tracking), andbegin_attempt/requeue_itemmarkattempt_evidence_missingsticky whenever theattempts_startedcounter is absent (mark_untracked_attempts). Nothing is inferred fromattemptsor aretrydict any more. So the production composition (RetrySchedule()writesretrybeforebegin_attempt) is no longer falsely marked and a definite park admits B; and the two merge-base shapes that carried neither field (anoutcome-unknownpark then requeue, with or without--reset-attempts; a bare READY left by a worker killed mid-send) carry no counter either and refuse. Rolling upgrade: every pre-marker READY/QUEUED/PARKED record is uncertain by construction; the "untried legacy record" test now asserts that ambiguity instead of admitting B.Typed admission (
contract.py,core.py,remote_gateway_bridge.py).DrainResultcarriesdispatched_digest— the sha256 of the bytes the outbox actually handed the provider._deliver_result_payloadarchives a result file only when the digest the core confirmed equals the digest of that file's payload; aTERMINAL+DELIVEREDrecord is accepted only if the record's stored payload (payload_digest, or the text of a pre-digest record) is the file's payload. Otherwise the file is quarantined with the named cause (fresh cycle refused: <last_refusal or id-already-live> (a different body was delivered under this id | the outbox sent its stored body, not this one)), never archived. The same-payload retry/recovery path is untouched because the proof is the payload, notpublish()'s boolean.P2.
_every_started_attempt_classifiedandmark_untracked_attemptsaccept onlyintthat is notbool(outbox.is_count); persistedtrue/truecounters are not evidence.DesignAClaimBackend(root, retry_schedule=RetrySchedule())), one definite refusal, then Battempt_evidence_missing: True→publish(B)Falsestarted 1 == classified 1→publish(B)True, B confirmedoutcome-unknownpark (git archive d37b79474),requeue_itemwith and without--reset-attempts, then one definite refusalpublish(B)True, local DELIVERED B, broker holds Apublish(B)Falseattempt-evidence-missingattempts, noretry), opened by the head, one definite refusalpublish(B)Truebegin_attempt→ refusedattemptsabsentattempts_started: true, attempts_classified: trueLIVE-REFUSEDunder the same id,_post_ready_results()fresh cycle refused: id-already-live (a different body was delivered under this id), archive empty_post_ready_results()(the outbox sent its stored body, not this one)Pasted (HEAD test files copied onto
git archive b57f02682,env -uworker vars, temp dirs):Also at HEAD:
delivery-core-contract49 OK (1 skip),delivery-core-enforcement15,outbox-operator-recovery45,health-check-outbox-parked18,gateway-outage-recovery78,gateway-runtime-identity,gateway-assignment-attribution40 — all OK;packages/ag2-sparrowunittest discover -s testsOK;sync_from_src.py --checkin sync;git diff --checkclean;scripts/review-checks.shPASS. Changed-statement coverage under.coveragerc(diff vs main, the regression files alone):backend_a.py88/88,src/outbox.py43/43,remote_gateway_bridge.py28/28,core.py9/9,contract.py4/4,src/health-check.py8/8 — 100%.Merge order, unchanged. #5269 lands first; this PR then rebases and re-decides
gateway-terminal-result-moved-once(rui: at b57f026 it passed only because NEWER was falsely marked; after the rebase a definitely parked, untainted id admits NEWER by design, and that test's assertion is scoped to tainted/ambiguous parks). The post-restart witness runs once at the approved head before merge.Review round 9 (Kewei's round-8 review; head 97f284a)
Blocker 2 — torn or outrun counters (commit 97f284a;
src/outbox.py, vendored byte-equal;delivery_core/backend_a.py).outbox.attempt_tracking_is_valid()is now the one judgement of the counters, applied bymark_untracked_attempts()beforebegin_attempt()/requeue_item()touch anything and by_every_started_attempt_classified()at admission: bothattempts_startedandattempts_classifiedmust be present as real non-boolean integers,0 <= classified <= started, andattempts(when present) must be a real integer no greater thanattempts_started— an older engine draining the same outbox notesattemptswithout starting one here, so a count ahead ofattempts_startedis treated as an attempt that may have landed A. Any other shape sets the stickyattempt_evidence_missingmark; the laterint(... or 0) + 1normalisation can no longer launder it because the mark is written first and never removed until a new cycle rebuilds the record.publish(B)attempts_started: 1, noattempts_classified(1, 1), no mark, B admittedattempt-evidence-missingattempts_started: 1,attempts_classified: Noneattempts_started: 1,attempts_classified: falseattempts_started: 1,attempts_classified: 2attempts_started: 1,attempts_classified: 1,attempts: 2attempts_started: true,attempts_classified: 1attempts_classified: 1, noattempts_startedattempts_started: 0,attempts_classified: 0,attempts: 0(control)tests/delivery-core-parked-fresh-publish.test.py::TornCountersStayUncertain(8 tests): at 3fe292eFAILED (failures=5)— the five rows marked "B admitted"; at 97f284aRan 8 tests … OK. Each branch ofattempt_tracking_is_valid()is driven by a named test; the diff-coverage gate on the Linux runner is the measurement of record.Blocker 1 — archive/quarantine must target the exact generation read — is deferred to the rebase onto #5269, by design, not skipped. #5269 owns the result-file lifecycle:
read_ready_result_with_identityreturns the generation's(dev, ino, mtime_ns, sha256)and_dispose_terminal/_quarantine_undelivered(..., generation=)dispose only that generation through the locked claim. Fixing the archive path here would duplicate that machinery in the same bridge lines the two PRs already conflict on. Rebase plan, in order: #5269 lands first; this PR rebases;_deliver_result_payloadpassesgeneration=at the archive site and at both quarantine sites (so a replacement published during the POST is neither archived as sent nor quarantined under A's cause — it stays live and is drained on its own); thelast_refusallines move into_dispose_terminal;test_a_dead_owners_unverified_claim_goes_back_liveis re-decided to two calls (the restored different body gets its fresh cycle); a combined generation-aware archive test is added at the real archive and quarantine boundaries; then the rebased head gets its own re-check, Codex pass and the post-restart witness before merge.Suites at 97f284a (
env -i, temp dirs): 67 files acrossdelivery-core-*,outbox-*,sparrow-outbox-*,gateway-*(incl. the two.sh),health-check-*outbox*,bridge-marker-no-leakall rc 0; packageunittest discoverOK;sync_from_src.py --checkin sync;git diff --checkclean;review-checks.shPASS. The live post-restart witness runs once at the approved (rebased) head before merge; earlier witness comments stand as mechanism evidence.🤖 Generated with Claude Code