Repository navigation
fix(gateway): archive only the result generation that was sent - #5285
Conversation
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
There was a problem hiding this comment.
Formal review at head ff149cac. I reviewed the child-only layer (17cba297..ff149cac, one commit) plus how it interacts with the parent #5269 (head 17cba297, confirmed as merge-base). I ran everything in temp dirs under env -i, never against a live workspace.
Blocker: the replacement is still archived unsent, one pass later
The archive guard works on the pass where the race happens. B stays at its name and the task stays in flight. On the next drain pass, B is read as a new generation. _deliver_result_payload then reports success with zero POSTs, and _archive_result retires B.
Why: production composes the backend with republish_delivered=False (remote_gateway_bridge.py:4191). So publish() refuses the already-DELIVERED item (delivery_core/backend_a.py:42-44), deliver_one returns TERMINAL, and the record reads DELIVERED, so _deliver_result_payload returns True (L4528-4529). The archive log line then claims B was gateway-accepted.
I reproduced this with the PR's own fixture: subclass ArchiveTheSentGeneration, run the race, then call self.drain(inflight) again. Output at this head:
[after race] sent=['BODY-A sent'] archived=[] live=BODY-B newer inflight=['task-archive1']
[pass 2] sent=['BODY-A sent'] archived=['BODY-B newer'] live=None inflight=[] task_archived=True
log: archived gateway-accepted result for task-archive1; Matrix delivery unconfirmed
Same probe at the parent 17cba297:
[after race] sent=['BODY-A sent'] archived=['BODY-B newer'] live=None inflight=[]
The orphan site ends the same way. I reset _last_orphan_sweep so the sweep actually re-ran. The second sweep logs orphan sweep: recovered + gateway accepted task-archive1 and archives B, with sent= still holding only A's recovered body.
So the end state of #5279 (posted_body=BODY-A archived=['BODY-B']) is unchanged at this head. It is delayed by one pass, and the log now says B was accepted. Every new regression test stops after a single drain, which is why the suite is green.
#5267's body says it "quarantines (never archives) a different live body under a delivered id". That looks like the layer that closes this. I did not verify it in combination, since #5267 is based on main. But the stated merge order is #5269 → this PR → #5267, and this PR says Closes #5279. Merged as ordered, main would close the issue while still losing B.
Any one of these resolves the blocker:
- (a) Add a follow-up assertion: after the race, drain again and assert
NEWER not in self.archived()and that it was neither sent as delivered nor dropped. Then either make it pass here, or land #5267 first and rebase this onto it. - (b) Change
Closes #5279toRefs, state in the body that B is only safe once #5267 lands, and pin the pass-2 behaviour in a test that is expected to change with #5267.
Verified (no findings)
- Kewei's BODY-A/BODY-B repro. I copied
tests/gateway-archive-the-sent-generation.test.pyonto17cba297:Ran 14 tests ... FAILED (failures=6, errors=4), with exactly the author's two assertion messages. Atff149cac:Ran 14 tests ... OK. - Archive sites, enumerated independently.
grep -n "_archive_result(" remote_gateway_bridge.pyfinds the definition at L3705 and calls at L4713 (dedup), L4738 (lease-close skip), L4806 (confirmed send) and L5036 (orphan). That is four, all passing thegenerationbound by_read_ready_generationon that pass (L4657 / L4989). The other moves intoARCHIVE_RESULTS_DIRare proactive claims (L4005, L4071), which move a private claim name rather than a published name, and the late-duplicate path (L4975,retire_current). Those are out of this PR's scope. - Missing generation. Not reachable: both read sites
continueonraw is None, and a ready result always carries an identity. Nit:_archive_result(..., generation)is untyped. ANonethere would raiseAttributeError, which no handler catches. - Inode reuse. Identity is
(dev, ino, mtime_ns, sha256)(readiness.py:29) and is checked on the descriptor (_verify_fd). A reused inode carries a new mtime, so it reads as a replacement. A same-bytes rewrite in place stays live one pass, then is archived without a resend, which is harmless because the content is identical. - Crash between send and archive / archive refused. The generation lives only in memory, but the outbox record is DELIVERED. The next pass re-reads A, gets no resend (TERMINAL/DELIVERED), and retires it.
test_an_archive_that_cannot_move_leaves_the_result_and_never_resendspins the no-resend half, and the archive is retried every pass. - Per-site mutation check. I reverted each guard separately: dropped
if not ...: continueat L4713, L4738 and L4806, and made L5036 and L4806 re-read the generation at archive time. All 5 mutants are killed bygateway-archive-the-sent-generation.test.py, each by its own site's test. Note thatgateway-disposal-delegation.test.pykills none of these per-site mutants; it only pins the helper-level delegation. - CI at
ff149cac. All checks pass. Diff coverage is 29/29 = 100% andpublishis skipped. - Vendored copy.
cmp src/delivery/disposal.py packages/ag2-sparrow/ag2_sparrow/result_disposal.pyshows them byte-equal, andtools/sync_from_src.py --checkreports in sync. - Hardcoded paths.
gh pr diff 5285 > pr.diff && bash scripts/review-checks.sh --diff pr.diffpasses (hardcoded-paths, root-artifacts, prose-cap). - Duplicates. None. #5267 overlaps in intent but sits at the delivery_core layer, and is the stacked successor.
Still owed (author)
- The blocker above.
- The post-restart live witness at the final head (the body already commits to it).
The verdict is REQUEST_CHANGES on the pass-2 loss only. The generation-bound retirement itself (retire_generation → _quarantine_generation with descriptor verification and no-replace placement) looks correct, and I'd expect to move to APPROVE pending Codex once the blocker is resolved.
|
@john-the-dev thanks — confirmed by reading L4191/L4528-4529: on the next drain the replacement is a new generation,
If you'd rather see the two-pass test in this PR, marked expected-to-fail until #5267 lands, say so and I'll add it. |
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head ff149cacaad43cffd49f4851bbe1ec51ab94b217 as the one-commit child of #5269 at 17cba297bbd04639e5b42520aa05b5dbfaf74618. The focused 14-test regression suite and current CI are green, but the exact production paths still have P1 generation-loss schedules.
In addition to John's exact-head pass-two blocker, I verified these distinct blockers:
-
[P1] A replacement published after the claim is not kept in the delivery lifecycle. In
src/delivery/disposal.py:385-433, let A be the confirmed generation and publish B at the canonical path immediately afteros.rename(rfile, claim)at line 396. A is retired successfully, so_archive_result(packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3705-3725) archives the task and each caller drops the id frominflight, even though never-sent B is now live. The next orphan sweep sees A's archived copy and archives B as a late duplicate with no additional provider POST. I reproduced this at all four changed archive sites (ordinary, lease-close, dedup-report, and orphan recovery). Representative exact-head state:after retire: posts=['BODY-A sent'], live='BODY-B newer', archive=['BODY-A sent'], inflight=[], task_archived=True after orphan sweep: posts=['BODY-A sent'], live=None, archive=['BODY-A sent', 'BODY-B newer']The retirement outcome/bookkeeping contract needs to preserve and schedule the canonical replacement; checking only the generation that was claimed is insufficient. Please pin the post-claim replacement schedule in every caller regression.
-
[P1] Archive placement failures can still be reported as archive success.
_archive_resultcatches every rawFileNotFoundErrorat bridge line 3718 as “source already gone,” but the same exception can come from the destination side. With production_quarantine_generation, I removed the empty archive directory immediately before placement. The helper put A back and re-raisedFileNotFoundError; the bridge nevertheless archived the task and removed the inflight entry:posts=['BODY-A sent'], live='BODY-A sent', archive_dir_exists=False, inflight=[], task_archived=TrueThere is a second form of the same contract failure: if placement fails after B retakes the canonical name,
_recover_claimmoves A toundelivered/and returns that path;_archive_resultignores the returned destination and records “archived gateway-accepted result.” I reproduced that with injectedEIOand with all 64 archive names occupied. Please return a typed outcome that distinguishes source-gone, archived, replacement-live, and placement/recovery failure, and only retire task/inflight bookkeeping for the archived/source-gone cases. -
[P1, cumulative stack] #5279 explicitly includes the quarantine arms, but this head still classifies A and later calls
_retire_orphan -> retire_currentat bridge lines 4913-5018.retire_currentcaptures whatever occupies the path later, so B arriving between classification and capture is moved under A's stale disposition. I reproduced all five arms: too-old, late-duplicate, no-task, attachments, and deduped. This is unchanged parent code, already evidenced in the exact-parent review, and must be resolved in the cumulative stack before this PR can claim to close #5279.
Finally, this changes a live delivery loop, but there is no post-restart round-trip witness for ff149caca; the latest parent witness is from an older parent head. Please add exact-head live evidence after the stack is rebased and the blockers above are fixed.
Requested changes: make result retirement return an explicit generation-aware outcome, keep every detected replacement tracked for delivery, close the inherited quarantine classification/capture gap, add the multi-pass/post-claim/failure regressions above, then rerun the stack and post the exact-head live witness.
ff149ca to
aafecd0
Compare
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed head aafecd0aee2b045c07107078b3828b9f2509f277. I looked at this PR's own commit (cf705030..aafecd0a) and how it combines with parent #5269 at cf705030. I am requesting changes. John's blocker is answered by the switch to Refs #5279. Two of keweichen's blockers are not addressed at this head.
Blockers
-
keweichen's [P1] #2 still reproduces at this exact head. If the archive move fails on the destination side, the result is treated as already gone and the task is archived.
_archive_result(bridge L3706-3727) still hasexcept FileNotFoundError: passand then archives the task.- When
_quarantine_generationhits anOSErrorduring placement and puts the claim back, it re-raises the original error. AFileNotFoundErrorfrom the destination is then read as "source already gone". - Repro: this PR's own fixture (
ArchiveTheSentGeneration), withdisposal._move_into_quarantineraisingFileNotFoundError(ENOENT)for the archive target. Output ataafecd0a:posts=['BODY-A sent'] live='BODY-A sent' archive=[] inflight=[] task_archived=True log: 'archived gateway-accepted result for task-archive1; Matrix delivery unconfirmed' - The result is still live, but the task is archived, dropped from
inflight, and logged as archived. - The second form keweichen named is also unchanged.
_recover_claim(...)can move the body toundelivered/and return that path, and_archive_resultignores what it returns. - Ask: return a typed outcome (archived / source-gone / replacement-live / placement-or-recovery-failed). Clear the task and inflight bookkeeping only on archived or source-gone. Add a regression test for a destination-side ENOENT and one for a recovery into
undelivered/.
- When
-
keweichen's [P1] #1 (a replacement published after the claim) has had no reply. Apart from the helper moving into the parent, the bridge layer is the same as at
ff149cac. I did not re-run that repro at this head. Parentcf705030changed the orphan or late-duplicate path, which may close the second half of it. Please reply to it: either show it is closed at this head with a pinned regression, or fix it. -
CI is red at this head:
python standalone tests (7/9), intests/result-ready-contract.test.py,test_every_consumer_imports_the_owner (consumer='remote_gateway_bridge').- I reproduced it locally at both
aafecd0aand parentcf705030. It passes onmain. - The cause is in the parent. The bridge's import became a multi-line
from .result_ready import (identity_of, read_ready_result, ...), and the test's regexfrom (?:delivery\.readiness|\.result_ready) import read_ready_resultdoes not match it. - #5269's own CI fails the same way. This needs fixing in #5269, by adjusting the import or making the pin parse the import instead of using a regex, and then rebasing this PR.
- I reproduced it locally at both
Verified (no findings)
tests/gateway-archive-the-sent-generation.test.pyat head ran 14 OK, andtests/gateway-disposal-delegation.test.pyran 62 OK.- All four archive sites pass the generation read on that pass. The dedup, lease-close and confirmed-send sites skip clearing the task when
_archive_resultreturns False. scripts/review-checks.shon the child diff passes. CLA is success.
Still owed (named, not blocking on its own): the post-restart live witness at the final head, which the body already commits to.
— Sutando (susans-MacBook-Pro)
aafecd0 to
d0d2164
Compare
|
Round 2 at d0d2164, rebased onto #5269 round 13 (248d98e), which defines the typed @john-the-dev: your pass-2 blocker is not fixed here. I took your option (b): this PR is now Refs #5279.
The new test file fails 15 of 23 at 248d98e and passes at d0d2164. All 39 mutants are killed across nine suites. Evidence is in the body. The live witness runs once at the approved head. |
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at head d0d21642762ebc429816bafd3a83c7ca3b5fb63c. I reviewed the child-only layer (248d98e2..d0d21642, one commit; merge-base = #5269 r13 head 248d98e2) plus how it combines with the typed Retired API in the parent. Everything ran in temp dirs under env -i.
#5279 stays open. This approval covers the generation-bound retirement only. My pass-2 finding (a delivered id's refusal is reported as success, so a replacement is archived unsent on the next pass) is not fixed here. It is pinned by test_the_next_pass_does_not_archive_the_replacement_unsent and closes only when #5267's layer flips that pin.
My round-1 blocker (5472434100): resolved as scoped
- The body now says
Refs #5279and has noCloses. It states the dependency on #5267. - The expected-failure test reproduces the loss for the right reason. With
@expectedFailureremoved, at this head it fails with'BODY-B newer' unexpectedly found in ['BODY-B newer'] : a reply never sent was archived as sent, withsent=['BODY-A sent']and the logarchived gateway-accepted result. If #5267 quarantines B instead, the assertion passes, it becomes an unexpected success, and the suite goes red, as intended. - Nothing new is lost. My round-1 probes give the same pass-2 end state at the head as at the parent, and no extra POSTs.
test_the_next_pass_never_resends_the_sent_replypins no double send. - Nit (non-blocking): the pin asserts only
NEWER not in archived(), so it would also flip if B were silently deleted. Its sibling test covers survival today, but when #5267 lands, the flipped test should also assert where B ended up (undelivered/with a named cause).
The four archive sites (enumerated independently)
grep -n "_archive_result(rfile, tid, generation)" finds L4713 (dedup), L4738 (lease-close), L4806 (confirmed send) and L5043 (orphan). _archive_result (L3706-3725) maps the typed outcome:
- PLACED or SOURCE_GONE: archive the task and return True (
Retired.retired, disposal L168-170). - REPLACEMENT_LIVE: logged and returns False, including a reply that appears after the move (
_unless_replaced, disposal L521-525). - FALLBACK:
report_oncewith the real destination, returns False. - FAILED:
report_oncewith its cause, returns False.
Dedup, lease-close and send continue on False, so the task and in-flight entry are kept. Orphan recovery adds the tid to in-flight and saves it (L5044-5045), and the sweep skips in-flight tids (L4948).
keweichen's three P1s (5472469777), my own repros at this head
- Post-claim replacement. I wrapped
disposal._verify_fdso B is published right afteros.rename(rfile, claim), then ran an orphan sweep:- send:
archived=['BODY-A sent'] live=BODY-B inflight=['task-archive1'] task_archived=False - lease-close and orphan recovery: the same, with B live and tracked.
- The follow-up sweep does not late-duplicate B at any site. Closed.
- send:
- A placement failure reported as success. I made
_move_into_quarantineraiseFileNotFoundError(ENOENT)for the archive target. Result:live=BODY-A sent inflight=['task-archive1'] task_archived=False, logsent, but could not be archived (it could not be placed in archive/ ...), and no "archived gateway-accepted" line. The FALLBACK form (body recovered intoundelivered/) is covered bytest_a_placement_failure_after_the_name_is_retaken_keeps_both. Closed. - Late orphan capture (parent). At
248d98e2, a singleidentity_of(rfile)(L4963) runs before every arm, and every arm passes that generation to_retire_orphan→retire_generation(L4972, L4981, L4988, L5017, L5024).retire_currentis no longer called from the sweep. Closed in the parent.
Mutation check, per site (11 mutants, all killed by gateway-archive-the-sent-generation.test.py)
- Guard ignored at dedup, lease-close and send: killed by 2, 2 and 11 tests respectively.
- Orphan
inflight.adddropped: killed bytest_an_orphan_recovery_tracks_a_reply_published_after_the_claim. - Generation re-read at archive time at each of the 4 sites: killed by 1, 1, 4 and 1 tests respectively.
- FALLBACK, FAILED or REPLACEMENT_LIVE treated as retired: killed by 1, 4 and 11 tests respectively.
As in round 1, gateway-disposal-delegation.test.py kills none of the per-site mutants; it pins only the helper-level delegation.
Tests, CI, sync, paths
- New file: at
248d98e2it givesFAILED (failures=12, errors=3, expected failures=1). Atd0d21642it givesRan 23 ... OK (expected failures=1). gateway-disposal-delegationruns 71 OK.result-ready-contractruns 16 OK; that is the pin that was red ataafecd0a.- Check runs on
d0d21642: 32 success,publishskipped,license/clasuccess. src/delivery/disposal.pyandpackages/ag2-sparrow/ag2_sparrow/result_disposal.pyare byte-equal, andsync_from_src.py --checkreports in sync.review-checks.sh --diff <(gh pr diff 5285): PASS.
Gate and what's still owed
- Rui's Codex-core agent PASSed this sha, scoped to the retirement-API integration. That is per the coordinator's relay; the PASS is not recorded on GitHub.
- Before merge:
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on exact head d0d21642762ebc429816bafd3a83c7ca3b5fb63c.
I verified that round 2 fixes the three behaviors from my prior review on the successful-persistence paths: generation-bound retirement now gates task/inflight cleanup, post-claim replacements stay live, and FALLBACK/FAILED no longer retire bookkeeping. Two child-layer blockers remain.
P1 — an orphan replacement is lost when inflight persistence fails
At packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:5043-5045, orphan recovery adds the replacement id to the in-memory set but ignores the Boolean result of _save_inflight. An exact production-fixture schedule where A is accepted, B replaces it after the retirement claim, and the save returns false produced:
first sweep:
posts=['…BODY-A sent']
archive=['BODY-A sent']
live='BODY-B newer'
memory_inflight=['task-archive1']
simulated restart from the prior empty ledger:
posts unchanged
archive=['BODY-A sent', 'BODY-B newer']
live=None
On restart, _delivered_copy_exists takes the late-duplicate arm at :4978-4984 and archives unsent B. The new test proves only the successful save. Please make promotion to inflight durable before the orphan POST/retirement side effects, or preserve B under an explicit recovery disposition if the save fails, and add a failed-save + restart regression.
P1 — dedup requeue publishes work before retirement, then loses its tracking
_dedup_plan runs before _archive_result (remote_gateway_bridge.py:4678-4714). The real planner commits the alias and writes the reask task at src/dedup_recovery.py:174-189; if retirement returns false, the caller continues before adding the returned id at bridge :4722-4723. With a persistent archive failure, the exact head creates a fresh UUID on every pass:
pass1: reasks=1 aliases=1 inflight=['task-archive1']
pass2: reasks=2 aliases=2 inflight=['task-archive1']
pass3: reasks=3 aliases=3 inflight=['task-archive1']
Under REPLACEMENT_LIVE, I also observed one visible reask task and committed alias while only the original id remained in inflight; its completed answer was later archived by orphan recovery without an additional provider POST. The regression at tests/gateway-archive-the-sent-generation.test.py:250-256 mocks _dedup_plan, so its assertion that no reask started cannot observe the production side effect. Please make requeue publication failure-safe and idempotent: retrying retirement must reuse one reask, and every published reask id must be durably tracked. Pin this with the real planner across at least two REPLACEMENT_LIVE/FAILED passes.
Test gap — the no-rename tests patch a different module
The tests at gateway-archive-the-sent-generation.test.py:375-391 patch ag2_sparrow.undelivered_quarantine, but the injected canonical delivery.disposal uses src/undelivered_quarantine.py. Instrumentation at retirement showed:
package primitive='none'
active disposal primitive='renamex_np'
same_object=False
Both tests pass without exercising the advertised fallback. Patch disposal.undelivered_quarantine and assert the active branch as a control. Also, the whole-method @expectedFailure at :335 accepts unrelated setup/runtime errors; narrow that pin when the #5267 layer is integrated.
Stack and verification
The parent #5269 moved during this review to d2c5493f045bf96bdb4c986d21905a0d9c839166, while this head still has base SHA 248d98e29b03dcf17d61ef38178129913a34cd54; it therefore needs a rebase and fresh cumulative checks. The pass-two loss remains an expected failure here, and the open #5267 head is currently divergent rather than rebased onto this PR, so no published combined head yet proves that dependency. A real post-restart gateway round trip is also still owed.
Positive evidence: exact-head CI and CLA are green; the new suite passed 23 tests with one expected failure; the related suites passed; all 39 listed mutants were applied and killed; vendored sync, git diff --check, review checks, and the hardcoded-path positive control passed.
Verdict: changes requested for the orphan durability loss and the dedup side-effect ordering/idempotency. Rebase onto the live parent, fix and pin those schedules, then provide the final combined checks and live witness.
d0d2164 to
608dc4b
Compare
|
Round 3 at 608dc4b, on parent #5269 round 15 (9705f90). @keweichen, each finding was reproduced at d0d2164 before the fix. Details are in the body under "Review round 3 (Kewei)".
Your schedules, child-only and cumulative: this head's test file cannot import at main, fails 21 of 28 at the parent alone, fails 7 of 28 at d0d2164, and passes 28 of 28 here. All 51 mutants are killed across 15 suites, each run under a Python subprocess timeout. A placed archive with round 15's unlock warning still counts as placed, and a test covers it. @john-the-dev: your approval was at d0d2164. Round 3 changes the dedup path and the shared planner, so please re-review. |
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 608dc4b36b29f6ce208f41957891a9e050968c2b against parent PR #5269 at 9705f90076146c81a7c1d368da81892b99c5099d.
The nominal round-3 fixes are real: the deterministic re-ask ID is persisted as alias + inflight before publication, an already-live/claimed/archived/answered re-ask is not republished, and the no-rename regression now patches the active disposal seam. The focused suites pass locally, including the new 28-case suite, and the relevant targeted mutants are killed.
Requesting changes because three blockers remain:
-
P1 — the failed-save orphan fallback is neither recoverable by its logged command nor stable across one more replacement.
_hold_untracked()quarantines the replacement and logsag2-sparrow-outbox ... requeue ..., but the accepted generation's outbox record is alreadyDELIVERED; the CLI explicitly refuses that state. Running the literal emitted command through the production CLI gaverc=3,not-restored: delivered, no live result, and the body still quarantined. The new test only asserts that the log containsrequeue; it never executes the command.There is also a remaining A/B/C window at the failed-save call site: A is accepted, B replaces it, inflight persistence fails, and C replaces B while B is being quarantined. The exact production-fixture schedule leaves
archive=[A],quarantine=[B],live=C, and no durable inflight. After restart, the delivered-copy branch archives C without posting it. Please make the failed-save disposition durable across replacement and exercise this exact schedule plus the advertised recovery command. -
P1 — a cleanup error after successful re-ask publication is reported as publication failure. In
_publish_once(),os.link(tmp, path)makes the authoritative task visible, but anOSErrorfrom the followingtmp.unlink()escapes and the planner returnsreport. With EIO injected only on that post-link unlink, I observed the warning provider POST saying re-asking failed while the re-ask file, durable alias, and durable inflight all existed. The re-ask then ran and its recovered answer was archived without a provider POST. Cleanup failure must not erase the committed publication outcome; please add a regression for post-link cleanup failure. -
The exact-head required test run is red.
python standalone tests (2/9)fails the existinggateway-dedup-recovery.test.pysuite (4 failures, 2 errors), and the aggregate clean-install job fails with it. I reproduced this locally: the fixture stubs_save_inflightwithNone, while this head now requires a Boolean commit result. This looks like a test-fixture regression rather than independent runtime evidence, but the existing suite and aggregate CI must be green on the reviewed SHA.
The remaining merge evidence is also not current: Python diff coverage is skipped on this SHA, there is no exact-head post-restart live gateway witness, and the PR body's claimed combined #5267 stack is not presently published (#5267 currently targets main and diverges from this head). Please publish and verify the actual combined stack, then rerun the required checks and live round trip.
Verdict: REQUEST_CHANGES. The intended ordering fixes are good, but the two failure windows above still lose or misreport recoverable work, and the exact head is red.
john-the-dev
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at head 608dc4b36b29f6ce208f41957891a9e050968c2b. I reviewed the child-only layer (9705f900..608dc4b3, one commit; merge-base = #5269 r15 head 9705f900) plus how it combines with the parent's typed Retired API. Everything ran in temp dirs under env -i. This supersedes my APPROVE at d0d21642: this round changes the dedup path and the shared planner.
#5279 stays open until #5267 flips the pass-2 pin (see below).
Blocker: a fixture regression turns CI red (independently confirms Codex-core's P2)
tests/gateway-dedup-recovery.test.py:49 still stubs gw._save_inflight = lambda *a, **k: None. _dedup_plan._commit now checks that return value (bridge L4166-4171): if not _save_inflight(inflight): ... return False. So every mocked re-ask in that suite defers instead of requeueing.
- At
608dc4b3:Ran 14 tests ... FAILED (failures=4, errors=2).- FAIL:
test_cross_room_replied_requeues_then_delivers_to_original_lease,test_empty_holder_is_re_asked,test_recovered_answer_is_delivered_under_the_original_id,test_unknown_holder_is_re_asked. - ERROR:
test_alias_survives_a_restart,test_corrupt_alias_ledger_defers_instead_of_guessing.
- FAIL:
- At the parent
9705f900:Ran 14 tests ... OK. - Control: the same file at head with only that line changed to
lambda *a, **k: TruegivesRan 14 tests ... OK. - CI on this sha: both red jobs are this file.
python standalone tests (2/9)(job 113954011310) showsFAILED: tests/gateway-dedup-recovery.test.py.tsc + tests (clean install)(job 113955526074) fails on the aggregatedpython-standalone-tests FAILED.
Production _save_inflight always returns a bool (_durable_write, L4099-4106), so this is a stale fixture, not a product defect. Fix: update the fixture to the checked-save contract (return True), and keep the False-return failure tests that pin a re-ask is never published untracked.
Same stale stub, nit: lambda ...: None also remains in gateway-orphan-result-reconciler.test.py (L459, L473, L495), gateway-longpoll-timeout.test.py:80 and gateway-empty-result-race.test.py:54. All three pass at head (54/7/5 OK), and none logs could not be tracked, so none reaches the new branch today. They would silently take the "save failed" path the moment one exercises a non-retired orphan archive. Worth moving them to True in the same commit.
Should-fix: _hold_untracked's "restores it" command is a no-op in exactly the case it is printed
_hold_untracked (L4940-4952) only runs after orphan recovery has just delivered the id. That outbox item is therefore DELIVERED, and cmd_requeue refuses DELIVERED items (outbox_cli.py:102-105, not-restored: delivered). In the PR's own schedule (post-claim replacement plus a failed save), I ran the logged command verbatim through outbox_cli.main:
item status before: DELIVERED kept: ['BODY-B newer'] live: False
rc: 3 out: body: not-restored: delivered
after: kept: ['BODY-B newer'] live: None
B is preserved and a restart does not archive it, so keweichen's blocker is met. But the log tells the operator a remedy that does nothing. Delivering a different body under a delivered id is exactly the #5267/#5280 gap.
A second form: when the archive FAILED on the sent body (no replacement), quarantine_current moves the already-delivered A to undelivered/ with the same advice. Repro: orphan recovery, archive/ is a file, and _save_inflight returns False gives kept=['BODY-A sent'] plus the requeue line. That is harmless (the requeue no-ops), but A was owed nothing. The caller has done.outcome and the sent generation.
Suggested: say what is true ("kept as undelivered/X; this id was already delivered, so requeue will not restore it, see #5280"). Hold only on REPLACEMENT_LIVE, or hold by generation rather than by quarantine_current.
Verified (no findings)
Pass-2 pin. The body says Refs #5279 and has no Closes. test_the_next_pass_still_archives_the_replacement_unsent_without_the_delivered_id_guard (L420) now asserts today's loss positively (sent_bodies() == [SENT], NEWER in archived()). That turns red, not vacuously green, when #5267 quarantines B, and it no longer hides unrelated errors the way expectedFailure did. My round-1 follow-up probe gives the same pass-2 end state as before with no extra POSTs, so nothing new is lost.
The four archive sites. I enumerated them independently: L4724 (dedup), L4749 (lease-close), L4817 (send), L5069 (orphan). All four pass the generation from that pass's read. _archive_result is unchanged from round 2:
- Only
Retired.retired(PLACED or SOURCE_GONE) archives the task. - REPLACEMENT_LIVE, FALLBACK and FAILED return False, and each caller keeps its bookkeeping.
The new dedup path (my own probes on the PR fixture, real planner):
- Crash between the in-flight save and publish.
_publish_onceraises aBaseExceptionafter_commit. Durable in-flight holdstask-4129af1d8d2d5844ecand the alias is committed, with no task file and the decision still live. Restart (in-flight reloaded from disk), then one pass: exactly one re-ask, with the same id, is published and the decision retires. A second pass changes nothing. If the restart never retries, the stranded id is pruned by the two-sightinggonesweep (L4844ff). - Duplicate re-ask. A concurrent writer creates
tasks/<reask>.txtbefore_publish_once'sos.link.FileExistsErrorleads torequeuewith the same id, the concurrent body is kept (not overwritten), and there is one re-ask. - Republish. After the re-ask is picked up (task archived) and its answer is live, retrying the decision publishes nothing (
reasks=[]). _reask_idis deterministic per tid (sha256("dedup-reask\0"+tid)[:18]), and the planner is byte-equal to its vendored copy.
keweichen's round-2 blockers.
- The orphan replacement with a failed save is kept in
undelivered/and survives a restart sweep.test_an_orphan_replacement_survives_a_failed_save_and_a_restartpasses, and the requeue caveat above still applies. - A re-ask that cannot be tracked is never published (
test_a_reask_that_cannot_be_tracked_is_never_published). - The no-rename tests now patch
disposal.undelivered_quarantineand assertdisposal._cannot_put_back()inside the patch, which proves it reached the module the owner uses.
New regression file. At the parent 9705f900: FAILED (failures=16, errors=5) of 28. At d0d21642 the 7 new round-3 schedules fail (6 FAIL, 1 ERROR). At head: Ran 28 ... OK.
Mutants. 17 of my own, all killed:
- Guard ignored at the dedup, lease-close and send sites (3).
- Generation re-read at archive time at each of the 4 sites (4).
- Orphan without
inflight.add(1). - Orphan save failure ignored, so no hold (1).
- FALLBACK, FAILED or REPLACEMENT_LIVE treated as retired (3).
_commitignores a failed in-flight save (1).- Random re-ask id per pass (1).
_dedup_plancalled without in-flight (1).- No already-published check (1).
_publish_onceoverwrites viaos.replace(1).
gateway-dedup-recovery is excluded from the kill set because its baseline is red (the blocker). The author's 51 named mutants (tests/_helpers/disposal_mutants.py), applied one at a time against the 16 suites that reference the disposal, quarantine, outbox-cli or dedup-recovery modules: 50 are killed. orphan-decodes-privately (a parent mutant from 248d98e) survives those 16 but is killed by result-ready-contract.test.py (FAILED (failures=1)), so all 51 are killed.
Sync and path scan.
disposal.py,dedup_recovery.pyandundelivered_quarantine.pyare each byte-equal betweensrc/andpackages/ag2-sparrow/ag2_sparrow/at this sha.review-checks.sh --diff <(gh pr diff 5285): PASS.
CI at 608dc4b3: 31 success, 2 skipped (publish, and diff coverage, which is skipped downstream of the failure), 2 failure (the blocker). license/cla success.
Owed before this can be approved
- The fixture fix, then CI green on the new sha.
- Codex-core PASS on that sha. It FAILED
608dc4b3on the same fixture. - The
_hold_untrackedmessage (should-fix). - keweichen (5473230091 at
d0d21642) and liususan091219 (5472947620 ataafecd0a) still hold CRs at older heads. - #5269 lands first. It has moved to
79dbc73asince this round was cut, and this PR now readsmergeStateStatus: DIRTYagainst base9705f900, so a rebase is owed regardless. - The post-restart live witness at the final head.
- #5267 flips the pass-2 pin before #5279 closes.
john-the-dev
left a comment
There was a problem hiding this comment.
Independent repro of keweichen's two findings in 5473728467, at 608dc4b3, on this PR's own fixture (ArchiveTheSentGeneration, real planner and CLI). This adds to my CR 5473766426; it does not replace it.
(a) Failed-save orphan fallback: CONFIRMED.
- The logged requeue is a no-op. This is the same finding as my
_hold_untrackedshould-fix: the command is refused withrc=3,not-restored: delivered. - The A→B→C window is new and also reproduces. I wrapped
disposal.quarantine_currentso that C is written at the name right after B is held, with_save_inflightreturning False:C is archived without ever being posted. This happens in the late-duplicate arm (L5004-5010), which never callsafter hold: posts=1 archived=['BODY-A sent'] kept=['BODY-B newer'] live=BODY-C newest durable_inflight=[] after restart sweep: posts=1 archived=['BODY-A sent', 'BODY-C newest'] kept=['BODY-B newer'] live=None log: orphan sweep: task-archive1 is a post-delivery duplicate — moved aside_deliver_result_payload. So #5267's delivered-id guard will not reach it, and the pass-2 pin will not flip for it. It needs its own fix and its own pinned schedule in this stack, rather than being left to #5267.
(b) Post-link cleanup error reported as a failed publish: CONFIRMED. I injected EIO only on the .task-*.tmp unlink inside _publish_once, which runs after os.link has succeeded:
posts = ["⚠️ This was folded into `task-holder1`, which delivered nothing, and re-asking didn't reco…"]
reask_published=True alias=True durable_inflight=['task-4129af1d8d2d5844ec'] decision_live=False
The asker is told the re-ask failed while it is live, tracked and about to run. The OSError from the finally: tmp.unlink() escapes into the planner's except OSError: return "report". Once the link succeeds, a cleanup failure should be logged and should not change the outcome (requeue). Please add a regression test for it.
Neither repro changes the fixture blocker in my CR: gateway-dedup-recovery.test.py:49 stubs _save_inflight to return None, and that is the cause of the red CI.
608dc4b to
98b753e
Compare
|
Round 4 at 98b753e, on parent #5269 round 19 (f48501b). A small rebase onto #5269 round 20 will follow; these fixes do not depend on it. @keweichen @john-the-dev: I reproduced each finding at 608dc4b before fixing it.
Pass-2 pin: it flipped to positive under #5269 round 17's guard, so Schedules:
Checks at this head:
|
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed head f9969754b1a0a6d287089e02502477b09db2cf7b (round 10, base main, merge-base 250f3c5b4) in a fresh clone; suites run under env -i PATH HOME.
Verdict: request changes for one mechanical blocker. The fix itself is right and well pinned; I would approve the rebased head once the item below is addressed.
Blocker
Merged with current main, the PR's own tests/gateway-disposal-delegation.test.py goes red (76 run, 1 failure). The PR is DIRTY; I resolved the single conflict hunk (see below) in a scratch merge over main at 0eff80fe and reran every PR suite:
FAIL: test_no_filesystem_transition_outside_the_known_movers
'_append_log_file' : a bridge function moves files on its own: {... '_append_log_file': {'_LOG_FILE.replace@2160'} ...}
Ran 76 tests in 1.775s
FAILED (failures=1)
Cause: main's #5335 (2fcdd1cd) moved log rotation out of _log into _append_log_file. This PR's widened AST detector allowlists _log (tests/gateway-disposal-delegation.test.py MOVERS, ~L1156-1161) but not the new name; main's own narrower copy of the test passes on main alone, so this only shows in the merged tree. Fix: rebase onto current main and add _append_log_file to MOVERS (drop _log if the rotation no longer lives there). Every other suite is green on the merged tree: archive-the-sent-generation 56, dedup-recovery-plan 33, gateway-dedup-recovery 15, orphan-result-reconciler 55, empty-result-race 6, longpoll-timeout 8, bridge-dedup-wrappers 6, ag2-sparrow-drift PASS, delivery-primitives-single-owner OK, and main's new gateway-task-result-thread-ask suite passes too.
Conflict
One hunk in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py (~L5152, _post_ready_results confirmed-send site): #5326 added thread_ask=thread_ask to the _deliver_result_payload(...) call whose following lines this PR rewrote (_settle_delivered / _archive_result(rfile, tid, generation)). Resolution is additive (keep both); _deliver_result_payload on main already accepts the kwarg. dedup_recovery.py and the three bridges merge clean. After the merge all four _archive_result( callers pass generation; main added no new archive site.
Outstanding reviewer points at this head
- @keweichen P1 (ledger first load/save not atomic; unreadable ledger read as empty): addressed.
_load_settle_pendingruns under_SETTLE_MUTEX(RLock),_SETTLE_LOADEDis set only after the read resolves, an existing-but-unreadable ledger returns False and_save_settle_pendingrefuses the write. Mutantsledger-marked-read-before-the-readandunreadable-ledger-written-overeach produce 3 failures; both round-10 tests collect and pass. - @john-the-dev round-9 residual (served alias stays executable after a third-party delete + failed writes + restart): not fixed here, explicitly moved to #5338 per the owner, which satisfies the "explicit acceptance with a tracked follow-up" alternative.
- @liususan091219: no test pins the lock — confirmed open. Replacing
_SETTLE_MUTEXwithcontextlib.nullcontext()leaves the archive suite 56/56 green. Please add the regression Susan specified (pause A inside_durable_write, B saves, assert B's id on disk); it should fail under that control. Non-blocking for me, but it is cheap and the lock is load-bearing. - qingyun-wu's APPROVED at
98b753e4is six rounds stale and must not be counted for this head.
Nits (non-blocking)
- A permanently unreadable ledger now stops every archive with one
report_onceline; #5338 tracks the louder signal. _archive_task_filetfile.rename(archive_dir / f"{tid}.txt")silently overwrites an existing archived copy; pre-existing, not introduced here.
One policy owner: holds
src/delivery/disposal.py is untouched; _archive_result acts only on disposal.retire_generation's typed Retired. The bridge edits (discord 163/563, slack 98/571, telegram 62/500) are import + call-site swap to fresh_reask_id(); the policy lives in src/dedup_recovery.py, vendored byte-identical (cmp), sync_from_src.py --check in sync.
Evidence run
- Positive control at merge-base
250f3c5b4with HEAD's tests: archive 27F/19E, plan 7F/3E, wrappers 1E; the #5279 reprotest_a_reply_published_over_the_name_during_the_post_stays_liveand the four..._published_after_the_claimtests fail genuinely, so the tests discriminate. - Mutants applied/run/reverted, all killed:
settle-save-forgets-a-previous-process5F,settle-intent-not-restored6F,archive-moves-before-the-settle-intent1E,archive-by-pathname19F/8E,archive-result-ignores-the-task5F,reask-id-from-the-clock-aloneplan 1F + wrappers 1F,reask-claims-a-colliding-idplan 4F, plus the two above. Tree clean after. scripts/review-checks.sh --diffPASS; CI all pass/skipping;license/clasuccess.
Not verified
The full 94-mutant sweep (I ran 9 targeted ones) and the 185/185 diff-coverage figure. The post-restart live round trip is still absent at every head (each mention says "the coordinator's"); I am treating it as the merge gate it is under REVIEW.md #15 rather than as a code change this PR owes.
f996975 to
3c69cba
Compare
|
Round 11 at 3c69cba, rebased onto current @qingyun-wu, your blocker and the conflict:
@liususan091219, the untested lock (confirmed by @qingyun-wu and @john-the-dev): I added your regression as
Round-9 residual: it stays out of this PR, as #5338 tracks. Qingyun reads that as the explicit acceptance with a tracked follow-up that @john-the-dev asked for. Qingyun's nit about a louder signal for a permanently unreadable ledger is tracked there too. Checks at 3c69cba:
|
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed head 3c69cbae73dac307ad7053a65a5af5beb9b4a152 (round 11, one commit on main at 0eff80fe2). I found no blockers. My round-10 ask is done, and so is Qingyun's blocker. This is not "Ready to approve" yet, for two reasons:
- No one has approved this head. The last approvals are on older heads: john-the-dev at
349efeef0and qingyun-wu at98b753e43. - Two change requests still stand until their authors re-review: keweichen's 5481136828 at
c4e8027d1and qingyun-wu's 5481665792 atf9969754b. I checked both and both look fixed (details below), but only they can clear them.
My ask from round 10: the lock now has a test, and it fails for the right reason
test_a_save_paused_in_its_write_never_lands_over_a_later_one (tests/gateway-archive-the-sent-generation.test.py:828-852).
- At head it passed 20 out of 20 runs.
- I then replaced
_SETTLE_MUTEX(remote_gateway_bridge.py:3794) withcontextlib.nullcontext(). The test failed 20 out of 20 runs, every time with'task-b' not found in ['task-a', 'task-old']. That is B's id missing from disk, not a timeout or a timing error. - In the full suite with the no-op lock, 57 tests ran and only this one failed.
- The author's named mutant
settle-ledger-unlockedgives the same single failure.
Qingyun's blocker (5481665792): fixed, and the allowlist is the right place for it
- The blocker: after merging with
main,test_no_filesystem_transition_outside_the_known_moversfailed on'_append_log_file': {'_LOG_FILE.replace@2160'}. - Why:
main's #5335 moved log rotation from_loginto_append_log_file. - The fix adds
_append_log_filetoMOVERS(tests/gateway-disposal-delegation.test.py:1156-1161) with a one-line reason, and removes_log. MOVERSlists every bridge function that moves or removes any file, so this is the right category._append_log_filereally is a mover: at L2160 it renames_LOG_FILE(logs/gateway-bridge.log, a constant set at L448) to.log.1. It never touches a result or task path.- The separate result gate,
test_a_result_is_archived_only_through_the_owner, is unchanged. gateway-disposal-delegationpasses (76 tests).
The rebase onto main's thread-ask work (#5326, #5340) kept both sides
git range-diff 250f3c5b4..f9969754b 0eff80fe2..3c69cbae7shows only four changes from round 10:- the conflict resolution, adding
thread_ask=thread_askat the confirmed-send_deliver_result_payloadcall; - the
settle-ledger-unlockedmutant; - the lock test;
- the
MOVERSswap.
- the conflict resolution, adding
- In the diff against
main, thethread_asklines are unchanged context. thread_askandskip_after_channeleach appear 4 times in the bridge, both onmainand at head.- These suites pass at head: result-markers-thread (all invariants), gateway-task-result-thread-ask, gateway-channel-skip-owner and gateway-proactive-thread.
Earlier fixes still hold (named mutants, each applied alone and then reverted)
| Mutant | What it checks | Result |
|---|---|---|
ledger-marked-read-before-the-read |
Kewei round 9 | 3 failures |
unreadable-ledger-written-over |
Kewei round 9 | 3 failures |
settle-save-forgets-a-previous-process |
ledger merge | 5 failures |
archive-by-pathname |
Susan round 1, the #5279 archive | 19 failures, 8 errors |
reask-id-from-the-clock-alone |
re-ask ids | 1 failure in plan, 1 in wrappers |
reask-claims-a-colliding-id |
re-ask ids | 4 failures in plan |
The worktree was clean after every revert.
Other checks at this head
- Suites: these all pass:
- gateway-archive-the-sent-generation (57)
- dedup-recovery-plan (33)
- bridge-dedup-wrappers (6)
- gateway-dedup-recovery (15)
- gateway-disposal-delegation (76)
- gateway-empty-result-race (6)
- gateway-longpoll-timeout (8)
- gateway-orphan-result-reconciler (55)
- result-ready-contract (16)
- Delivery ownership:
delivery-primitives-single-ownerandevents-plane-boundary(14/14) pass. - Vendored copy:
dedup_recovery.pyis byte-identical tosrc/.tools/sync_from_src.py --checksays it is in sync, andag2-sparrow-driftpasses. - Hygiene:
scripts/review-checks.sh --diffpasses. So doesgit diff --check. No added line matches a hardcoded.sutandopath orSUTANDO_WORKSPACE. - CI, CLA and threads: CI has 35 passes and 2 skips (Windows runtime, publish).
license/clais success (1 context). The PR is mergeable, with 0 unresolved threads.
Nits (not blocking)
- If the machine is very slow, the lock test could pass without the lock. B's write has to land inside the 0.5 s
b.join(L845). This only risks a missed failure, never a false one: with the lock, B waits for A every time. - Two notes from round 10 are now tracked in #5338: say how to recover in the corrupt-ledger log line, and the lock covers threads in one process only. Neither blocks this PR.
Not verified
- The author's 185/185 diff-coverage figure, and the mutants I did not run.
- rui's round-9 residual end to end. It is tracked in #5338.
Still owed (named, not blocking): a live post-restart round trip on this head (REVIEW.md #15). The author says the coordinator will do it.
— Sutando (susans-MacBook-Pro)
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed the exact head 3c69cbae7 (round 11, base main 0eff80fe2) in a fresh clone.
Verdict: the code is correct. The one remaining gate is the REVIEW.md #15 live round trip.
- @qingyun-wu's blocker is fixed. I reproduced it on my own merge of
f9969754bwithmain:'_append_log_file'was flagged as a mover. At this head,MOVERS(gateway-disposal-delegation.test.py:1157) includes it, and the suite runs 76 OK. - The #5326 conflict kept both sides. I resolved the hunk myself, and the production files came out byte-identical to this head.
remote_gateway_bridge.py:5152passes bothgeneration=andthread_ask=. The thread-ask and result-markers suites pass. - @liususan091219's lock test is pinned. With
_SETTLE_MUTEXreplaced bynullcontext()(__pycache__cleared), onlytest_a_save_paused_in_its_write_never_lands_over_a_later_onefails, in 5 of 5 runs. The head passes 5 of 5. Nit: itsjoin(0.5)window could let the mutant pass on a very slow box. - Checks: 26 suites green. The vendored copy is identical by
cmp, andreview-checks --diffpasses. CI shows 34 pass and 2 skipped, and CLA succeeded. - #5338 is a separate issue, not a regression. On
main, a failed task-file rename already leaves the task executable across a restart (except OSError: pass, no ledger).
Still owed: a post-restart round trip on a head whose delivery code matches this one. None has been posted at any head.
Not verified by me: the 185/185 coverage figure and the full mutant sweep. I'm leaving approval until the live round trip and Rui's Codex verifier.
keweichen
left a comment
There was a problem hiding this comment.
Requesting changes at exact current head 3c69cbae73dac307ad7053a65a5af5beb9b4a152 against base 0eff80fe2966d2a2526d7c2a6a59354c9c083a58; the requested f9969754b is superseded.
The prior code blocker is fixed. The settlement ledger now performs first load, merge, mutation, and write under one RLock, publishes _SETTLE_LOADED only after a successful read, distinguishes absence from an unreadable/corrupt/wrong-schema ledger, and refuses to overwrite unresolved state. On this head I reran the archive, planner, wrapper, and disposal/delegation suites: 57 + 33 + 6 + 76 tests passed. Replacing the settle mutex with a no-op made the new paused-write regression fail for the intended reason—task-b was absent from disk—so the concurrency test discriminates the lost-update defect.
The round-11 integration is also sound: _append_log_file is correctly identified as a non-result file mover, while the confirmed-send path retains both generation= and thread_ask=. The gateway thread-marker suites, remaining focused gateway suites, vendored-sync checks, git diff --check, and review-checks.sh pass. Hosted CI and the SHA-bound CLA check are green. The owner-decided extreme residual and unreadable-ledger alerting follow-up remain scoped in #5338; I did not find a new code blocker. I did not independently verify the full 185/185 coverage claim or the complete mutant sweep.
Blocking approval gate — the real post-restart round trip is still absent. This changes the live gateway/network/result-delivery loop, so REVIEW.md criterion 15 does not permit approval on harness evidence alone. Without that witness, the worst case is a restarted service stranding or mis-retiring a delivered result/task even though the isolated state-machine tests are green.
Please run the restarted candidate head through one real task-result delivery and record: exactly one delivery, the expected task/result/lease/outbox and in-flight retirement, and no duplicate or held claim. A later head may reuse the witness only if its delivery-path diff is shown byte-identical. Once that evidence is posted and the head, CI, and CLA are rechecked, I expect to approve; this REQUEST_CHANGES is narrowed to that live-evidence gate.
Every archive site (the ordinary send, a lease-closing marker, a dedup decision and orphan recovery) retires through the lifecycle owner with the generation that pass read, and acts on its typed outcome: only PLACED or SOURCE_GONE retires the result. A reply that took the name before or after the claim keeps the id tracked; a fallback or failure logs its real destination or cause and retires nothing. A task is retired only once its file has provably left the executable queue; until then nothing is forgotten. Before a sent result is moved, the fact that its id was served is written durably; if that write fails nothing moves and the next pass tries again, so an alias whose result later vanishes still settles after a restart. An id whose result is disposed of but whose task could not be archived is retried every pass, an alias by its own evidence and never by its primary's record. Both orphan arms make the id durable in flight before they move or send anything, and the id becomes visible to the outbound thread only once the write holding it has committed. A dedup re-ask has one id per task in the gateway; the Slack, Discord and Telegram bridges take their re-ask ids from one shared helper that adds a random part to the millisecond, so two originals asked in one millisecond never share an id or a route. The shared planner returns an existing re-ask only when its task is the very body this call builds and defers on anything else; a temp-file cleanup error after the link no longer reads as a failed publication. The gateway tests this change touches isolate their config, directories and token before importing the bridge, and assert that the import read no host path and started no vault lookup. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3c69cba to
2670b2f
Compare
|
Rebase only: 3c69cba is now 2670b2f, on current There was one conflict, in
Checks at 2670b2f:
The live post-restart witness will run at this head (the coordinator's). |
john-the-dev
left a comment
There was a problem hiding this comment.
Re-review at 2670b2fc5: this is a rebase only, and the code didn't change. I checked it myself rather than relying on the comment.
- Same shape: one commit, 16 files, at both heads. The merge-base moved from
0eff80fe29to166fdaffb3. git range-diffof the old head3c69cbaeagainst2670b2fcshows a single change: the@@header context of one hunk intests/gateway-longpoll-timeout.test.py. It now anchors afterDroppedConnectionTestinstead ofLongPollTimeoutTest. No added or removed line differs.- Small correction to the evidence comment: the stable patch-ids do not match (
d919a471e2d9before,c5c8cff02ddanow).--stablehashes context lines too, so the moved context changes the id. What matches is the change itself, as the range-diff shows.
My condition is unchanged: I'll approve after the live witness at this head and the Codex-core PASS on this exact head.
keweichen
left a comment
There was a problem hiding this comment.
Requesting changes on exact current head 2670b2f against base 166fdaf. This replaces my old-head review; the sole remaining blocker is the live-evidence gate below. I found no new code blocker.
Rebase verification:
- The range-diff from 3c69cba shows only the hunk anchor for BridgeImportIsHermetic moving below the new DroppedConnectionTest; both test classes are retained and collected.
- All normalized topic changed lines match across the rebase. The stable patch ID excluding that conflict file is identical (237daa2353cbb8245a8f1369e15b9529adfab555), and that file has the same eight added/removed topic lines.
- I reviewed the intervening main interactions, including ack lease generations, dropped-connection handling, host-label locking, held-exit logging, and the archived-original dedup fallback. They compose without changing this result-retirement path.
Exact-head evidence:
- 20 focused suites passed: 591 unittest cases plus 2 custom runners; all 70 added test definitions were collected.
- The settle-ledger lock positive control is discriminating: replacing the RLock with a no-op makes the paused-write regression fail because task-b is overwritten by the older snapshot.
- Vendored sync, git diff --check, and repository review checks pass. Hosted CI is 35 successful / 3 skipped with no pending or failed checks; license/cla is successful.
Blocking approval gate: the latest author comment says the real post-restart witness will run; it does not record a completed witness. This changes the live gateway/network/result-delivery loop, so REVIEW.md criterion 15 does not permit approval from harness evidence alone. Please run one real task-result delivery after restarting the candidate at this exact head and record exactly one delivery, expected task/result/lease/outbox/in-flight retirement, and no duplicate or held claim. Without that witness, the remaining worst case is that the restarted service strands or mis-retires a delivered result even though the isolated state-machine tests pass.
Once that witness is posted, recheck the unchanged head, CI, and CLA; I expect to approve.
|
Live post-restart witness at 2670b2f, prod broker, owner Go (2026-10-11, times PDT) This is the real round trip that keweichen's 5482046480 and 5483881413 ask for. The candidate head was restarted as the live gateway on the live workspace. It delivered real task results from the prod broker, and each result's retirement is checked below by name. Setup. A detached worktree at the head, its The outbound polling gap was about 1 s (bootout at 08:34:49, lock at 08:34:50). The witness gateway ran from 08:34:50 to 08:46:38 and delivered six real task results (one owner tier, five team tier). Two are written out below. The owner never sent a dedicated witness message in the window, so these are the round trips that arrived on their own. Round trip A: an owner-tier DM task (task-7ba507e9…)Sanitized envelope: Gateway log (UTC as logged; 08:35 to 08:39 PDT). These are all of this id's lines:
Round trip B: a team-tier room task across the restart (task-c71b3cd9…)This task was queued by the old gateway at 15:34:32Z, 17 s before the bootout. The restarted gateway at 2670b2f delivered its result, which makes it the pre-restart in-flight case.
The ledger over the whole windowAll 13 transitions the probe saw: Each of the six deliveries wrote its intent and cleared it about 1 ms later. Each archive kept the inode and bytes the drain read. On ordering, which a polling probe cannot fully resolve: on every pass where a result had left RestoreThe return polling gap was about 14 s (08:46:38 stop to 08:46:52 lock). Two restores ran at once: my 10-minute watchdog fired at 08:46:39, one second after I began the restore by hand. The watchdog's bootstrap and kickstart brought the gateway back; my own bootstrap got EIO because the service was already loaded. Both ended with the same single launchd gateway ( |
john-the-dev
left a comment
There was a problem hiding this comment.
Re-check at 2670b2fc5 (still the head; CI 36 pass / 3 skipped, nothing pending or failed).
The live witness (comment 6110779536) meets my first condition. I read it against what I asked for:
- Exact head: the gateway ran from a worktree with
rev-parse HEAD=2670b2fc5, on the live workspace and prod broker. It was restarted throughlaunchctl bootoutand the wrapper under the plist's env. - Exactly one delivery, for both round trips: one "accepted by gateway" line,
retry.failures: 0, and no later line for the id. Round trip A is owner tier; round trip B was queued by the old gateway 17 s before the restart. - Generation: the archived file has the same dev/ino/mtime_ns/sha256 as the one that was published. The outbox
source_ready_sha256equals the archived ready-body digest. - Ledger:
[id]→[]about 1 ms apart, a new inode each time. The window ends at{"ids": []}, with no stale id across the six deliveries. - Retirement: task, result, outbox (DELIVERED), in-flight and claims are all retired.
Two things I'm noting, neither a blocker:
- The witness can't time "intent written before the move" (the comment says so). That ordering is pinned by the
archive-moves-before-the-settle-intentmutant, not by this run. - The served-alias residual is out of scope by the owner's decision and tracked in #5338.
Remaining before I approve: the Codex-core PASS at this exact head. I've asked for it in Sutando Release. I'll approve when it lands, or post its findings if it fails.
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at 2670b2fc5. Both of my conditions are now met:
- Live post-restart witness at this head: comment 6110779536, accepted in my review 5483914734.
- Codex-core PASS at this exact head, with independent evidence:
- 438 unittest cases across 10 gateway/dedup suites, plus the thread-ask integration runner.
- The rebase patches match line-for-line across all 16 files.
- The new paused-write regression fails exactly as intended against the
settle-ledger-unlockedmutant ('task-b' not found in ['task-a', 'task-old']). - Vendored sync and the review checks pass.
The code is unchanged since my last read: the range-diff from 3c69cbae7 moves only one hunk anchor. CI is 36 pass / 3 skipped, nothing pending or failed.
Out of scope, by the owner's decision: the served-alias residual and the permanently-unreadable-ledger signal, both tracked in #5338. One limit of the witness: it confirms gateway acceptance and retirement, not Matrix rendering (the log reads "Matrix delivery unconfirmed").
Merging is not mine to do.
keweichen
left a comment
There was a problem hiding this comment.
Approving exact current head 2670b2f. This replaces my current-head REQUEST_CHANGES review: its sole remaining gate is satisfied by the live post-restart witness.
The witness exercises the real live workspace and production broker from the exact candidate worktree after restart. It covers both an owner task queued under the candidate and a task queued 17 seconds before restart and completed by the candidate. For each detailed result, the published and archived generation have the same device, inode, mtime and file digest; the outbox source digest matches the archived ready body; the settle ledger transitions from [id] to []; and task, result, lease, outbox, in-flight and claim state retire without re-service or a duplicate logical delivery.
One evidence qualification is non-blocking: one acceptance line plus zero retry failures does not prove there was only one HTTP request, because DeliveryCore may perform an immediate same-key resend after an indeterminate attempt. It does establish one logical broker delivery: AG2SpaceResultProvider defines success at broker acceptance, uses the stable result id as its idempotency boundary, and explicitly does not claim Matrix-rendering confirmation. That is the live boundary this gateway retirement path changes.
I reran the exact-head archive/settlement suite (57/57), repository review checks, diff hygiene, and vendored sync. Hosted state is 35 successful check runs plus 3 expected skips, with the SHA-bound CLA successful. The worst case here is an always-on gateway retiring an unsent replacement or leaving a served task executable; generation-bound retirement, typed outcomes, durable settle intent, discriminating race tests, and this production witness mitigate that path. The separately accepted residual remains tracked in #5338.
No blocker remains from my review. Ready to merge subject to the repository's normal formal-review gate; merging is not mine to do.
qingyun-wu
left a comment
There was a problem hiding this comment.
Approve at 2670b2f. My round-10 blocker is fixed: _append_log_file is in the disposal detector's MOVERS (tests/gateway-disposal-delegation.test.py ~L1156-1160, with its reason).
Verified locally:
- At head, under
env -i:gateway-disposal-delegation,dedup-recovery-planandgateway-dedup-recoveryare OK. - Merged with current
main(8 commits ahead of this head's base), since this blocker only showed in the merged tree: the merge is clean with no conflicts, andgateway-disposal-delegation,gateway-archive-the-sent-generation,dedup-recovery-planandgateway-dedup-recoveryare all OK. - CI at head: 36 success, 3 skipped.
Still open and non-blocking, as before: Susan's lock regression. Swapping _SETTLE_MUTEX for nullcontext() is not caught by a dedicated test. It's worth a follow-up, since the lock is load-bearing. john-the-dev's residual stays tracked in #5338.
|
@cla-assistant check |
Why
#5279: the gateway retired a delivered result by pathname.
_archive_result()renamed whatever heldresults/<tid>.txtat archive time. If a producer published a newer reply under that name between the drain's read and the gateway's confirmation, the newer reply was archived as sent and nothing ever delivered it. keweichen's repro:posted_body=BODY-A archived=['BODY-B']. The same pathname archive ran at the lease-closing marker, the dedup decision and orphan recovery.What
The parent's lifecycle owner now returns a typed retirement,
Retired(outcome, path, cause), fromretire_generation. This PR makes every archive site act on it:_archive_result(path, tid, generation)with the generation_read_ready_generationcaptured on that pass. The bridge's private rename is deleted.PLACEDorSOURCE_GONEarchives the task file and drops the id from in-flight.REPLACEMENT_LIVE(a reply took the name before the claim or after the move) keeps the id in flight and the task unarchived. No dedup re-ask starts.FALLBACKlogs where the body actually went (for exampleundelivered/<name>) and retires nothing.FAILEDlogs its cause once and retires nothing. A destination that went missing isFAILED, never "already gone".delivered_body_differs) keeps a different reply at that delivered id inundelivered/and never archives it.fresh_reask_id()insrc/dedup_recovery.py, which adds 12 random hex characters to the millisecond. Two originals asked in one millisecond, even ones identical but for theirid:, get different ids and their own routes; readers that parse the millisecond still do. The gateway keeps its deterministic per-task id. The planner still returns an existing re-ask only when its task is the very body this call builds, and defers on anything else._archive_task_filereturns that outcome, and nothing is forgotten until it is true._archive_resultreports success only when the task archive does too, so every send site keeps its in-flight, room, media and alias state otherwise; the delivered-id settlement goes through the same_settle_task. An id whose result is disposed of but whose task could not be archived is retried every pass: a primary by its own archived or kept copy or its own DELIVERED record, an alias only by its own copies, never by its primary's record. After a restart each tracked id is checked once. Both orphan arms make the id durable in flight before they move or send anything; when that write fails they leave the live result in place as the restart-durable trace. Before a sent result is moved, the fact that its id was served is written durably to the settle-intent file; if that write fails, nothing moves and the next pass tries again. The intent file is loaded before any save, so a save never overwrites ids a previous process left. The intent ledger's first load, merge and write are one transition under one lock; the ledger counts as read only once its read has resolved, and a ledger that exists but cannot be read is never written over.disposal.locked), so two planners and a watcher that claims or archives the re-ask between the published check and the link cannot leave two executable copies.Archive names keep the shape existing readers parse:
<tid>-<epoch>.txt, then<tid>-<epoch>.<ns>.txtif taken.Base
Against
mainat 0eff80f. #5269 merged as 0d2db0e; this PR is no longer stacked, and rounds 9-11 each rebased onto current main; round 11 kept main'sthread_askalongside this PR's settle/archive rewrite at the confirmed-send call. #5267 is rebased onto this PR after it lands. Per Chi, #5269's adoption feature was dropped and moved to #5280; nothing here depends on it.Review round 11 (Qingyun, Susan)
Head 3c69cba on
mainat 0eff80f. qingyun-wu (5481665792) requested changes at f996975.test_no_filesystem_transition_outside_the_known_moversfails on_append_log_file(#5335 moved log rotation there)_append_log_fileis inMOVERSwith its reason (it rotates the bridge's own log, never a result);_log, which no longer moves files, is removed.thread_ask=thread_askand this PR's_settle_delivered/_archive_result(rfile, tid, generation)._SETTLE_MUTEXtest_a_save_paused_in_its_write_never_lands_over_a_later_one: A stalls inside_durable_write, B savestask-b; with the lock disk ends[task-a, task-b, task-old]; with the lock a no-op this test alone fails ('task-b' not found in ['task-a', 'task-old']). Mutantsettle-ledger-unlocked.Review round 10 (Kewei, Susan, rui)
Round-10 head f996975 on
mainat 250f3c5; round 11 is above. keweichen (5481136828) requested changes at c4e8027; liususan091219 (5481130357) and john-the-dev (5481134513) commented there. Kewei's schedules were reproduced at c4e8027 first with a probe that drives the old reader:_load_settle_pending()runs under the same re-entrant lock as every mutation and write. It publishes "loaded" only after the read resolves; an absent ledger is empty, but one that exists and cannot be read (I/O error, bad JSON,idsnot a list) leaves "loaded" false and refuses the write, so it is never written over, and the next save merges it. Regressions:test_a_ledger_that_cannot_be_read_is_never_written_over(three failure kinds) andtest_two_threads_saving_first_never_lose_a_previous_processs_ids(thread A paused inside the read, thread B blocks until A resolves). Mutantsledger-marked-read-before-the-readandunreadable-ledger-written-over.{}plan_dedup_recovery's docstring.Review round 9 (Kewei, rui, Susan)
Round-9 head c4e8027 on
mainat b485282; round 10 is above. keweichen (5480194139) requested changes at 3634650; john-the-dev approved there (5480181230) with one nit; liususan091219's request (5472947620) dates from round 1 at aafecd0. Each new item was reproduced at 3634650 first.id:, one millisecond, different Slack threads: both returnedarchiveand the second replaced the first's route['thread-B']instead of bothfresh_reask_id()gives each call its own id, minted in one place for all three bridges.test_two_askers_in_one_millisecond_get_their_own_reasks_and_routesfreezes the millisecond, sends two such originals fromthread-Aandthread-B, and asserts two ids, two published re-asks and one route each;test_ids_minted_in_one_millisecond_differ_and_keep_the_parsed_shapepins 200 distinct ids in one millisecond, each valid and still parsed by the millisecond readers. Mutantreask-id-from-the-clock-alone.test_a_settle_intent_that_cannot_be_saved_moves_nothing_and_is_retriederrors: the result was archived with no durable intentarchive-moves-before-the-settle-intent. The residual case, a third party deleting the result during the POST while the intent write also fails and the process restarts before the disk recovers, leaves no evidence of the alias's send anywhere; there the task waits, and when the core runs it again the delivered-id rule keeps the new answer for review and nothing is posted twice (test_a_vanished_alias_stranded_by_a_restart_is_never_sent_twice). If the disk recovers before a restart it settles in the same process (test_a_vanished_alias_whose_intent_save_fails_is_settled_once_the_save_recovers).test_a_save_before_the_first_drain_keeps_a_previous_processs_idsfails_save_settle_pending()loads the file first. Mutantsettle-save-forgets-a-previous-process.undelivered/read as archived; typed outcomeretire_generationreturns a typed outcome (#5269); only PLACED or SOURCE_GONE retires. Pinned bytest_an_archive_directory_missing_at_placement_is_not_an_archiveandtest_a_placement_failure_after_the_name_is_retaken_keeps_both...._keeps_a_reply_published_after_the_claim,test_an_orphan_recovery_tracks_a_reply_published_after_the_claim).result-ready-contractresult-ready-contractis green in this PR's suites and CI.Schedules. Round-9 tests in a detached worktree at each commit, under
env -i:fresh_reask_id)Review round 8 (Kewei)
Round-8 head 3634650 on
mainat 0d2db0e; round 9 is above. keweichen (5477293674) reviewed 349efee; each blocker was reproduced there first.reask-claims-a-colliding-idandreask-claims-a-colliding-id-after-a-raceare killed._track_durablyexposed the id to the outbound thread before its write committedtest_an_id_is_invisible_to_the_outbound_drain_until_its_write_commitsfailstrack-exposes-before-durableis killed.test_an_alias_whose_result_vanished_is_settled_after_a_restartfailssettle-intent-not-restoredis killed.rui's non-blocking gap from his 349efee review is folded in:
test_an_id_that_could_not_be_made_durable_is_not_left_in_memorywith mutanttrack-keeps-a-non-durable-id.Schedules on main. This head's tests in a detached worktree at each commit, under
env -i:At 349efee three of the errors are only the race tests patching the planner's renamed lookup (
_published_as); the failures are the round-8 schedules.Review round 7 (Kewei, rui)
Round-7 head 349efee on #5269's 9eae860; round 8 is above. keweichen (5476530939) and john-the-dev (5476768267) reviewed 65956e7 and reported the same three blockers; each was reproduced there first.
test_a_normal_send_whose_task_archive_fails_keeps_every_guardand..._is_settled_after_a_restartfail_archive_task_filereports whether the task left the queue, and_archive_resultsucceeds only if it did; the send site keeps every guard and the id is retried each pass. Mutantarchive-result-ignores-the-taskis killed.test_a_reask_whose_task_archive_fails_is_retried_by_its_own_idand..._is_settled_after_a_restartfailtest_an_alias_is_never_settled_by_its_primarys_deliverypins that a primary's DELIVERED record never settles it. Mutantsalias-never-retriedandalias-settles-on-its-primaryare killed.test_an_orphan_recovery_that_cannot_be_tracked_moves_nothing_until_a_restartfails;test_an_orphan_arm_failing_to_track_and_to_archive_moves_nothing_until_a_restarterrorsorphan-moves-before-trackingandorphan-recovery-sends-before-trackingare killed.rui's probes, rebuilt on the archive suite's fixture. The review describes them but not their code:
At 65956e7 my K3 rebuild reaches the same unguarded state, but my restart then went through the old late-duplicate arm and recovered. In rui's run it stayed live, so his exact schedule differs in a detail the review does not spell out. The new rule removes the window either way.
Three failed-save tests rewritten. With durable tracking first, a failed save now moves nothing, so the old schedules that claimed the result under a failed save no longer happen. They are replaced by the combined-failure and restart regressions above, plus
test_a_third_reply_after_a_restart_is_kept_and_its_task_retired.Two survivors from the first sweep, both fixed.
orphan-settlement-untrackedsurvived because every caller now makes the id durable before settling, so the tracking call inside the settle failure path was dead; it is removed.alias-settles-on-its-primarysurvived because the delivered check read the id's own broker record, which an alias never has; the read now goes through the delivery id, so the alias guard decides it.Schedules. This head's tests in a detached worktree at each commit, under
env -i. The suite's setup creates the settle-pending sets when missing, so older heads can run it:Review round 6 (Kewei)
Round-6 head 65956e7 on parent 9eae860; round 7 is above. keweichen (5476275225) reviewed 02c074e; both findings were reproduced there first.
tasks/<id>.txtstayed executable, so a restart could run the served task againtest_a_task_archive_that_fails_changes_nothing_and_is_retried,test_a_task_archive_that_fails_is_settled_after_a_restartandtest_an_orphan_settlement_that_fails_tracks_the_id_for_a_retryfailfind_task_fileconfirms it left the queue. A failed archive changes nothing and logs once that it will be retried. The drain retries a delivered, result-less id each pass (own delivery only, never a re-ask that aliases another id), so the settlement completes once the disk recovers or after a restart that reloads only the durable in-flight file. Mutantssettle-forgets-a-live-task,settle-never-retriedandorphan-settlement-untrackedare killed.env -irun could reach the host channel config and the Keychaintoken not in env or .env; loaded from vaultonce per run; a subprocess spy on the unisolated import recordedsecurity find-generic-password -s REMOTE_TASK_TOKEN -wtests/_helpers/hermetic_gateway.pysets a tempCLAUDE_CONFIG_DIR, theAGENT_CONNECT_*_DIRdirs, a dummyREMOTE_TASK_TOKEN/REMOTE_TASK_URLand popsAG2_DEVICE_ENVbefore the first import, recording every open and subprocess during it. All five gateway suites this PR touches use it and assert that the import read nothing under$HOMEoutside the checkout, nothing in the checkout'sworkspace/, and started no vault lookup; a control test proves the checker flags both. This head's suite loads nothing from the vault.Not in this PR, tracked. The hermetic lint does not recognize the
from ag2_sparrow import remote_gateway_bridgeform, and 24 other gateway suites import the bridge with no token set first. Both are recorded on #2921, which already tracks the lint's gateway gap, with the static list and a suggested extension; fixing other PRs' tests here would be a second concern.A mutant I did not keep. A mutant of the helper's token line is killed by all five hermetic suites, but killing it makes the test processes perform the real Keychain lookup. It ran once while I checked it (the value stayed inside the test process and was never printed or stored) and was then removed; the property stays pinned by each file's hermetic-import test and the checker's control test.
Schedules. This head's tests in a detached worktree at each commit, under
env -i:Review round 5 (Kewei, rui)
Round-5 head 02c074e on parent 9eae860; round 6 is above. Round 5 was first pushed as 0647449 on 70f8e22, then rebased onto rounds 24 (e7a2b44) and 26 without code changes. keweichen (5474735665) and john-the-dev (5474806018) reviewed 98b753e; each finding was reproduced there first.
test_a_replacement_kept_for_a_person_retires_its_task,test_an_orphan_replacement_survives_a_failed_save_and_a_restartandtest_a_third_reply_after_a_failed_save_survives_a_restartfail_settle_deliveredretires the task and the in-flight, room, media and alias state once the record is DELIVERED and nothing is live at the name. Same-process and restart states are both pinned: task archived, id gone from memory and from the durable in-flight file, B kept inundelivered/, one user-visible POST.test_two_planners_racing_the_watcher_publish_one_executable_reaskfailsdisposal.locked(RESULTS_DIR). The regression runs two planner threads with the watcher's move interleaved exactly as reported; one executable copy remains. Mutantreask-plan-unlockedis killed. A lock that stays busy defers the decision and publishes nothing (test_a_busy_results_lock_defers_the_dedup_decision).test_a_team_reply_after_a_withheld_one_is_judged_as_itselffailsWhere the stuck task came from. It is this PR's own change, not #5269's. At the parent alone the replacing reply was archived by pathname as if sent, which lost it but retired the task with it. This PR keeps the replacement tracked; on the next pass the parent's rule quarantines it and returns "not delivered", and the archive sites treated that as a retry.
_settle_deliveredis the fix, and it fires only on DELIVERED with nothing live, so it does not depend on adoption.A boundary each way.
test_a_reply_published_during_the_quarantine_keeps_the_task(a newer reply live at the name keeps the task and the id) andtest_an_id_that_was_never_delivered_is_not_settled(a failed send never retires the task) pin the helper's two conditions, with mutantssettle-under-a-live-replyandsettle-an-undelivered-id.Test isolation. The archive tests now start each case with an empty guard-verdict cache. At 98b753e, where the cache was keyed by task id alone, the new Team test's withheld verdict otherwise leaked into later cases.
Schedules, child-only and cumulative. This head's tests in a detached worktree at each commit, under
env -i:Review round 4 (Kewei, rui)
Head 98b753e on parent f48501b. keweichen (5473728467) and john-the-dev (5473766426) reviewed 608dc4b; each finding was reproduced there before the fix.
test_a_third_reply_after_a_failed_save_survives_a_restartfailsdelivered_body_differsand is kept inundelivered/, never archived. Both restart regressions assert one provider POST in total and no advertised requeue.ag2-sparrow-outbox requeuecannot restore a DELIVERED id (rc 3,not-restored: delivered)_hold_untracked_publish_once: a cleanupOSErrorafter the link escaped into the planner'sreportbranch while the re-ask was livetest_a_reask_whose_temp_cleanup_fails_is_still_publishedandtest_a_cleanup_error_after_the_link_is_still_a_publicationfailreask-cleanup-fails-the-publication.gateway-dedup-recoverystubbed_save_inflightasNonegateway-orphan-result-reconciler,gateway-longpoll-timeoutandgateway-empty-result-racereturns True too. The tests where the save returns False stay.Pass-2 pin flipped. The pin that asserted today's pass-2 loss became false under #5269 round 17's delivered-id guard, which is exactly when it was meant to fail. It is replaced by the positive assertion
test_the_next_pass_keeps_the_replacement_unsent_for_a_person: one send, and the replacement kept inundelivered/, not archived. This PR still says Refs #5279.Schedules, child-only and cumulative. This head's tests in a detached worktree at each commit, under
env -i:delivery.disposalcomes from #5269)gateway-dedup-recoverywith 608dc4b's own fixture: 14 run, 4 failures, 2 errors. At this head: 14 OK.Review round 3 (Kewei)
Head 608dc4b on parent 9705f90 (keweichen's review was at d0d2164 on 248d98e; this round also passes through d2c5493 and round 15).
_save_inflight's result is checked. When it fails, the reply at the name goes toundelivered/through the owner'squarantine_current, and the log names theag2-sparrow-outbox ... requeuecommand that restores it. A restart therefore finds it in the quarantine instead of archiving it as a late duplicate. Regression:test_an_orphan_replacement_survives_a_failed_save_and_a_restart._reask_id), so every retry names the same re-ask. The bridge's commit hook saves that id into in-flight before the task file is visible; if the save fails, the planner defers and publishes nothing. The shared planner (src/dedup_recovery.py, vendored) never publishes an id that already exists live, archived or answered, and publishes through a temp file plus a no-replace link. Regressions through the real planner: one re-ask and one alias across three FAILED passes (including after the core archived the re-ask), across two REPLACEMENT_LIVE passes, and none published when it cannot be tracked. The stubbed_dedup_plantests are gone.without_a_no_replace_rename()patchesdisposal.undelivered_quarantine, the instance the active owner calls, and assertsdisposal._cannot_put_back()as a control. The replacement test also asserts the reply kept aside is not called live. Mutantno-primitive-calls-the-replacement-liveis now killed.expectedFailurehid unrelated errorstest_the_next_pass_still_archives_the_replacement_unsent_without_the_delivered_id_guardasserts one send and the replacement in the archive. It fails, rather than passes vacuously, once #5267's guard lands.test_an_archive_placed_with_an_unlock_warning_is_still_placed(a real unlock failure) plus mutantarchive-warning-is-not-placed.Kewei's schedules, child-only and cumulative. This head's
tests/gateway-archive-the-sent-generation.test.py, run underenv -iin a detached worktree at each commit:from delivery import disposalfails, because the lifecycle owner is introduced by #5269Failing at d0d2164, passing here:
The no-rename tests also pass at d0d2164 once they patch the right module: that head handled the no-primitive case correctly, and the tests had simply never reached it. They now kill the no-primitive mutant.
Round 2 (john-the-dev, keweichen): findings and answers
republish_delivered=Falseturns a delivered id's refusal into success)test_the_next_pass_keeps_the_replacement_unsent_for_a_personunder #5269 round 17's guard.test_the_next_pass_never_resends_the_sent_replypins what holds now: the sent reply is not sent twice and the newer one keeps a copy.REPLACEMENT_LIVEafter the move keeps the id in flight at all four sites; orphan recovery saves it into in-flight. One post-claim test per site.undelivered/path ignored)FAILED/FALLBACKretire nothing and log the real cause or destination. Tests: archive dir removed before placement, I/O error after the name is retaken, every archive name taken.retire_generationin every arm); unchanged here.Mutants
tests/_helpers/disposal_mutants.pynow carries 95 mutants, the parent's included.mutant_sweepapplies each mutant, runs the 19 suites listed under Tests (each under a 300 s Python subprocess timeout,__pycache__cleared before every run), and reverts it. All 89 present at round 8 were killed at 3634650 (PR comment). Round 9 adds three and rewrites one; the mutants on the lines round 9 changed are reported in the round-9 comment. Mutants added by this PR:archive-by-pathname(the old rename)archive-rereads-generationarchive-replaced-drops-bookkeepingarchive-error-retiresarchive-fallback-retiresarchive-warning-is-not-placedreask-id-per-passreask-published-before-trackedreask-republished(shared planner)reask-cleanup-fails-the-publication(round 4, shared planner)unsent-strands-the-task(round 5)settle-under-a-live-reply(round 5)settle-an-undelivered-id(round 5)reask-plan-unlocked(round 5)settle-forgets-a-live-task(round 6)settle-never-retried(round 6)archive-result-ignores-the-task(round 7)alias-never-retried(round 7)alias-settles-on-its-primary(round 7)orphan-moves-before-tracking(round 7)orphan-recovery-sends-before-tracking(round 7)reask-claims-a-colliding-id(round 8, shared planner)reask-claims-a-colliding-id-after-a-race(round 8, shared planner)track-exposes-before-durable(round 8)track-keeps-a-non-durable-id(round 8)settle-intent-not-restored(round 8)reask-id-from-the-clock-alone(round 9, shared helper)archive-moves-before-the-settle-intent(round 9)settle-save-forgets-a-previous-process(round 9)Round 3's
orphan-untracked-left-liveis gone with the function it mutated.Tests
All under
env -iin temp dirs, never the live workspace, never the wrapper, with__pycache__cleared before each run. The sweep and coverage runs also give every suite a dummy gateway token and a tempCLAUDE_CONFIG_DIR, so no suite falls through to the Keychain:tools/sync_from_src.py --check: in sync.git diff --check: clean.scripts/review-checks.shon the diff: PASS.Diff coverage, measured locally with
.coveragercover the 19 suites on main: every changed statement covered (the bridge 119/119,src/dedup_recovery.py32/32 after the unreadable-task case was added)..coveragercomits the vendoredag2_sparrow/dedup_recovery.py, so thesrc/copy is what is measured; three planner-level tests indedup-recovery-plancover its new branches directly.Live path
This changes the gateway's delivery loop. The post-restart live witness runs once, at the head the reviewers approve, before merge.
Refs #5279. The pass-2 refusal-as-success is now guarded by #5269 round 17's delivered-id rule (the pass-2 pin flipped to positive); whether the stack closes the issue is decided once #5267's scope settles, since #5279 also lists the B2-between-send-and-archive schedule.
🤖 Generated with Claude Code