Skip to content

fix(gateway): proactive sends go through the outbox; an uncertain send is parked, never resent - #5857

Merged
sonichi merged 14 commits into
mainfrom
fix/proactive-send-outbox-lane
Oct 25, 2026
Merged

sonichi merged 14 commits into
mainfrom
fix/proactive-send-outbox-lane

Conversation

@sonichi

@sonichi sonichi commented Oct 24, 2026 •

Copy link
Copy Markdown
Owner

PR 1 of #5853, as scoped in its latest plan correction: transport hardening only, no new notification behaviour. Nothing in this PR writes a new kind of message; it changes how results/proactive-*.txt reaches the room.

What changed, and why

The gateway's proactive drain posted each file with its own unkeyed retry loop. A timeout or an accept with no event id was re-sent up to an in-memory attempt cap, which a restart reset, and REMOTE_PROACTIVE_TRUST_OK settled a bare {"ok": true} as delivered. AG2SpaceRoomMessageProvider declares idempotent_send=False and reconcile_capable=False, so any resend after an uncertain outcome can post twice.

  • One claim owner. The proactive drain is a DeliveryCore lane (_delivery_core("proactive")) over the room-message provider, built at the one existing construction site. The outbox owns the claim, the retry and the outcome; the drain only moves the file as the outcome says. ProactiveClaimFence is not used. Item id = <file stem>~<16 hex of the body's SHA-256>. A new body under a delivered name is a new send. The room and thread are re-resolved on every pass, so they stay out of the key: a crash followed by an owner-DM change is still one item. The stored payload is what is sent, and the POST carries the item's dedupe_key. The logged outbox_cli.py requeue <item> --body-id <stem> restores the quarantined body under its file name, which maps back to the same item, and that item is re-driven once.
  • No fail-open settle, no bridge retry. PROACTIVE_TRUST_OK, _PROACTIVE_ATTEMPTS and _resolve_send_failure are removed. Definite refusals (401/403, 408, 425, 429, a refused connection, an unresolved host) retry on the outbox's schedule, counted on the item so a restart keeps the count. Another 4xx parks at once. The owner-DM room_upgraded hold is kept.
  • Uncertain stays UNKNOWN. A timeout, a 5xx or an accept with no event id parks outcome-unknown (file to results/undelivered/, operator requeue command logged), with no resend. Under a new opt-in RetryPolicy.park_interrupted_send (set only for this lane), the core marks a send before it leaves. A later claim that finds the mark and no receipt parks it UNKNOWN; one that finds the recorded event id settles it delivered.
  • Event id persisted. record_delivered gains provider_ref. record_provider_ref keeps the event id and room on the item before completion, which is what makes a crash after the receipt safe.
  • Revalidation at dispatch. DeliveryCore takes an optional revalidate hook, asked after the claim and right before the send. A message about a task (proactive-about-<task id>--<ts>.txt, typed constructor proactive_routing.about_task_filename) is withdrawn, archived with nothing posted and its item parked withdrawn:<reason>, when the task has been answered (the bridge checks), or has ended, waits on the owner, or has a pending cancel (the host's PROACTIVE_DISPATCH_GATE, Sutando: src/proactive_dispatch_gate.py). A gate that raises withdraws too.
  • Thread routing. In the task's own room, an ask already in a thread keeps its verified thread_root, and a top-level ask is rooted at its source_message_id. A malformed root posts top level, never re-rooted. The provider's room/body/key API gains an optional thread_root.

Parking an uncertain send is a single write (park_item drops the dispatch mark in the same write), and the mark is cleared only after the other transitions. A crash during settlement therefore keeps the mark, and the restart parks the item instead of resending it.

The undelivered quarantine names a proactive stem as itself, so outbox_cli.py requeue <stem> restores the body under its original name. Nothing is restored or resent until an operator runs it.

Recovery is bound to one body generation. A parked file is retired without overwriting an earlier copy, and the logged requeue also carries --body-sha256, so it restores exactly that item's body.

An unsettled dispatch (a receipt or a mark) is settled before the retry budget is checked, so a restart past the deadline still settles it DELIVERED or outcome-unknown.

A definite outcome drops the dispatch mark in the same write, and an operator requeue drops it too.

A stored destination that differs from the freshly resolved one (an owner DM that moved after a refusal) parks the item destination-changed and quarantines the file. It is never re-routed, and the logged operator release sends it to its stored room.

Only an operator release skips the destination guard. The CLI requeue records a release bound to the stored payload; an automatic room-upgrade requeue records none. Parking takes the item's lock and refuses an item that is delivered, holds a receipt or a dispatch mark, or is claimed. A delivered file that cannot be archived is renamed to a hidden .<stem>.delivered-<ts> that no bridge globs, then reaped into archive/. It is never unlinked and never put back under its live name, where Discord or Slack could post it again. Every handback to a live name is a no-replace rename, so a newer body is never clobbered. A dead drainer's claim past its reclaim window, or a torn claim past its grace period (swept with the claim route's own sweep), no longer blocks parking; the release is logged with item, pid and age. File claims are unique (.sending.<pid>.<ns>), so a claim kept after a colliding handback is never replaced by the next drain. undelivered_quarantine.restore() itself refuses an empty digest.

Recovery fails closed. A body that can't be read at park time gets no runnable command. outbox_cli requeue refuses (exit 5, before any change) an empty or malformed --body-sha256, and a missing one while several bodies are parked under the name.

Why the file's .sending.<pid> rename claim (and _recover_orphan_proactive) stay. It is the cross-bridge claim that keeps the Discord, Telegram and Slack bridges, which poll the same results/proactive-*.txt, off the file. It does not decide delivery: the outbox item's claim and dispatch mark do. A file recovered to .txt maps to the same item (stem + body hash), and that item is either terminal or settled by the mark and receipt, so it can't double-send. The crash-receipt and unknown-parks tests exercise exactly that recovery.

The withheld-review lane is unchanged: park_interrupted_send defaults off, so its crash semantics, which withheld-review-decision-final pins, stay as they are.

Where to look first

  1. delivery_core/core.py deliver_one (the guard and the hook) and backend_a.py (mark_dispatching, prior_dispatch, complete(provider_ref=)).
  2. remote_gateway_bridge.py _send_proactive, _post_proactive_message, and the proactive branch of _delivery_core.
  3. src/outbox.py (record_delivered, record_dispatching, clear_dispatching, record_provider_ref; vendored by sync_from_src.py).

Behaviour this proves

A proactive post that may have landed is never posted a second time: not on the next pass, not after a restart, not after a crash on either side of the receipt. A bare ok is never recorded as delivered. A message about a task that finished, was cancelled or is waiting on the owner between claim and send is not posted.

Feature/documentation consistency

docs/remote-gateway-protocol.md gets a new "Proactive room sends" section, and docs/catalog.json last_verified is updated. docs/src-map.md is regenerated for the new module. scripts/census-d1.py's two ag2space rows were anchored on the removed trust-ok receipt and the classify_response import. They now anchor on record_provider_ref and the room-message provider lane, with the removed names as absent anchors, and docs/census/d1-identity-census.md is regenerated.

Before/after evidence

The five required tests, run against origin/main (tests copied in) and then at this head:

origin/main                                   HEAD
gateway-proactive-unknown-parks       exit 1  exit 0
gateway-proactive-crash-receipt       exit 1  exit 0
gateway-proactive-dispatch-revalidate exit 1  exit 0
gateway-proactive-no-trust-ok         exit 1  exit 0
gateway-proactive-outbox-thread       exit 1  exit 0

Failures on origin/main, excerpted:

== unknown-parks
  FAIL a) timeout: no second POST after the restart (5)
  FAIL a) timeout: the outbox item is parked outcome-unknown ([('DELIVERED', None)])
== crash-receipt
  FAIL before-receipt: no second POST after the restart (1)
  FAIL before-receipt: the item is PARKED (DELIVERED, None)
== dispatch-revalidate
  FAIL a) result: no POST (1)
  FAIL b) cancel: no POST (1)
== no-trust-ok
  FAIL a) a bare ok is never DELIVERED (['DELIVERED'])
  FAIL b) the file is parked (archived)
== outbox-thread
  FAIL a) in-thread ask keeps its thread root (None in '!TaskRoomAbCdEf:ag2.space')
  FAIL e) the POST carries a dedupe key (None)
  FAIL b) a top-level ask is rooted at its message (None)
  • gateway-proactive-unknown-parks: covers accept then timeout then restart, and the same for no event id and for a 5xx. Each ends parked outcome-unknown with exactly one POST, and putting the file back does not send it again. The logged requeue command, run as printed, re-drives the same item exactly once. Control: a refused connection and a DNS failure are retried and delivered once.
  • gateway-proactive-crash-receipt: a child process os._exits mid-drain; a third case changes the owner DM between the crash and the restart (still one parked item, no POST); a fourth kills the settle between its writes (parked, no resend). Crashing before the receipt leaves the item parked UNKNOWN with no resend. Crashing after the receipt leaves it DELIVERED with the event id and room, with no resend. Inside the dead claim's reclaim window the file waits and is not lost.
  • gateway-proactive-dispatch-revalidate: a result, a cancel, WAITING, COMPLETED, or a gate that raises lands between the outbox claim and the send. Each produces no POST. Control: nothing lands and it posts once.
  • gateway-proactive-no-trust-ok: with REMOTE_PROACTIVE_TRUST_OK=1 set, a bare ok is never DELIVERED, is parked, and is posted once.
  • gateway-proactive-outbox-thread: an in-thread ask keeps its root, and a top-level ask is rooted at its source message. Another room gets no root, and a malformed root posts top level. The boundary is pinned for PR 2/3: with no task file, a task-last file, or the ask's thread in another room, there is no root, never a fallback to the source message id. The receipt keeps room, thread and event id.

Tests run

  • tests/gateway-proactive-recovery-edges.test.py (all cases fail at 9dca540): two parked generations under one name; a restart past the deadline with and without a receipt; a crash right after the final refusal followed by an operator requeue; and the owner DM moving after a 429.

  • Five new tests above, plus the rewritten tests/proactive-retry-bound.test.py: a 429 is retried on the outbox backoff and parks retry-window-exhausted, with every POST counted on the item. A timeout parks at once, and a 404 parks on the first attempt.

  • Updated to the new semantics: src/remote-gateway-bridge.test.py (proactive section: 502 and no-event-id now park instead of retrying; receipts are found by item id, with provider_ref; trust-ok section replaced; loader wiring of the dispatch gate), proactive-outbox-classification, send-failure-delegation (the sparrow binder test now pins delegation to deliver_one), gateway-disposal-delegation (_send_proactive added to the known movers), gateway-proactive-foreign-homeserver (one line: the removed attempt map), sparrowd-agent-bridges (asserts the carried env value instead of the removed module flag).

  • Ran 101 related suites locally (every test touching the bridge's proactive path, the outbox, delivery_core, the withheld lane and the hermetic-env gates): all pass. scripts/review-checks.sh --diff reports PASS, tools/test_no_drift.py reports PASS, and gen-src-map --check reports up to date.

Live path

This touches the live delivery loop. No post-restart live round trip has been run yet. It needs the owner's go to swap the gateway to this head (live-witness). Until then, the evidence is the harness above.

Overlap with open PRs

#5676 shares no file. #5828 and #5855 both edit _post_proactive's send block and _record_proactive_receipt in remote_gateway_bridge.py, and #5828 also edits tests/gateway-proactive-foreign-homeserver.test.py and tests/send-failure-delegation.test.py. This PR replaces that send block, so whichever of these lands second will need a rebase there. The other hunks were left alone. The docs/catalog.json date edit is byte-identical to #5855's.

Not in this PR (follow-ups)

Hardcoded-path check: none added (review-checks PASS). Stacked: no.

🤖 Generated with Claude Code

…d is parked, never resent

The gateway's proactive drain posted results/proactive-*.txt with its own unkeyed
retry loop: a timeout or an accept with no event id was re-sent up to the in-memory
attempt cap (reset by a restart), and REMOTE_PROACTIVE_TRUST_OK settled a bare
{"ok": true} as delivered. The room send is neither idempotent nor reconcilable, so
each resend after an uncertain outcome could post twice.

- The proactive lane is a DeliveryCore over the room-message provider, the single
  claim owner. Item = file stem + digest of room, thread and body; the POST carries
  its dedupe key. The bridge's retry loop, attempt map and trust-ok setting are gone.
- Under the new RetryPolicy.park_interrupted_send, the core marks a send before it
  leaves: a later claim that finds the mark parks it outcome-unknown, one that finds
  the recorded event id settles it delivered. Uncertain outcomes (timeout, 5xx, no
  event id) park; definite refusals retry on the outbox schedule, counted durably.
- record_delivered gains provider_ref; record_provider_ref keeps the event id (and
  room) on the item before completion.
- A revalidate hook runs at dispatch: a proactive-about-<task>--<ts>.txt message is
  withdrawn when the task has been answered, ended, waits on the owner or is being
  cancelled (bridge checks the result; the host's PROACTIVE_DISPATCH_GATE the rest).
- In the task's own room it threads like the ask: an in-thread ask keeps its verified
  thread_root, a top-level ask is rooted at its source message.
- The room provider takes an optional thread_root, and a connection that was refused
  or a host that did not resolve is a definite refusal, not an unknown outcome.

Refs #5853 (PR 1: transport hardening, no new notification behaviour).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the proactive lane

The census-d1 ag2space rows anchored on the removed trust-ok receipt and
classify_response import; they now anchor on record_provider_ref and the
room-message provider lane, with the removed names as absent anchors. The
dispatch gate takes its workspace from the canonical resolver by default.

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • packages/ag2-sparrow/ag2_sparrow/delivery_core/init.py (100%)
  • packages/ag2-sparrow/ag2_sparrow/delivery_core/backend_a.py (100%)
  • packages/ag2-sparrow/ag2_sparrow/delivery_core/contract.py (100%)
  • packages/ag2-sparrow/ag2_sparrow/delivery_core/core.py (100%)
  • packages/ag2-sparrow/ag2_sparrow/delivery_core/provider_ag2space.py (100%)
  • packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py (99.5%): Missing lines 5945
  • src/outbox.py (91.4%): Missing lines 911-912,916-918
  • src/outbox_cli.py (69.2%): Missing lines 101,103,109,112
  • src/proactive_dispatch_gate.py (100%)
  • src/proactive_routing.py (100%)
  • src/remote-gateway-bridge.py (100%)
  • src/remote-gateway-bridge.test.py (100%)
  • src/undelivered_quarantine.py (93.8%): Missing lines 230

Summary

  • Total: 410 lines
  • Missing: 11 lines
  • Coverage: 97%

packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py

Lines 5941-5949

  5941                 _EMPTY_LOGGED.add(f.name)
  5942                 _log(f"proactive {f.name} claimed empty — producer has not "
  5943                      f"flushed yet; handing back (never dead-lettered, a late "
  5944                      f"flush must still deliver)")
! 5945             _hand_back(claim, f)
  5946             continue
  5947         # Non-empty now — drop any prior empty observation so a file that merely
  5948         # flushed late can log again if it is ever re-observed empty.
  5949         _EMPTY_LOGGED.discard(f.name)

src/outbox.py

Lines 907-922

  907         rec = read_delivery_claim(Path(root), item_id)
  908         if rec is not None:
  909             if rec.state == "UNKNOWN" and not rec.malformed:
  910                 # Torn: the same grace-period sweep the claim route uses, never a younger one.
! 911                 if not _sweep_if_abandoned(_claim_path(Path(root), item_id)):
! 912                     return False
  913             elif not _record_is_reclaimable(rec, reclaim_ttl_s):
  914                 return False
  915             else:
! 916                 _release_locked(Path(root), item_id, rec.drainer_id, force=True)
! 917             if on_release is not None:
! 918                 on_release(rec)
  919         d.update(status="PARKED", reason=reason)
  920         _write_item(Path(root), item_id, d)
  921         return True

src/outbox_cli.py

Lines 97-107

   97     """
   98     sha = getattr(args, "body_sha256", None)
   99     if sha is not None and not _SHA256_RE.fullmatch(sha):
  100         # Refused before any state change: an empty or partial digest would restore any copy.
! 101         _emit({"item_id": args.item_id, "result": "refused",
  102                "error": "--body-sha256 must be a 64-character hex digest"}, args.json)
! 103         return 5
  104     if sha is None:
  105         copies = undelivered_quarantine.find_quarantined(
  106             args.results_dir or Path(args.root).parent, getattr(args, "body_id", None) or args.item_id)
  107         if len(copies) > 1:

Lines 105-116

  105         copies = undelivered_quarantine.find_quarantined(
  106             args.results_dir or Path(args.root).parent, getattr(args, "body_id", None) or args.item_id)
  107         if len(copies) > 1:
  108             # Several parked generations: which one is this item's is the operator's call, not "newest".
! 109             _emit({"item_id": args.item_id, "result": "refused",
  110                    "error": "several quarantined bodies share this name; pass --body-sha256 for one",
  111                    "candidates": {str(p): undelivered_quarantine.sha256_of(p) for p in copies}}, args.json)
! 112             return 5
  113     result, epoch_written = outbox.requeue_item_with_epoch(
  114         args.root, args.item_id,
  115         reset_attempts=args.reset_attempts,
  116         operator=args.operator or _default_operator(),

src/undelivered_quarantine.py

Lines 226-234

  226     to go is the one the user should get), and the body never loses its last
  227     name: nothing a producer can retake is unlinked.
  228     """
  229     if body_sha256 is not None and not re.fullmatch(r"[0-9a-f]{64}", body_sha256):
! 230         raise ValueError("body_sha256 must be a 64-character hex digest; an empty one would match any copy")
  231     found = find_quarantined(results_dir, task_id)
  232     if body_sha256:
  233         found = [p for p in found if sha256_of(p) == body_sha256]  # one generation, never another
  234     if not found:

sonichi and others added 9 commits October 24, 2026 15:52
…ged requeue re-drives it

Review of a2ce5c9 found two defects in the proactive lane:

- The item id hashed the room and thread, which are re-resolved each pass. A crash
  after the POST followed by an owner-DM change gave a second POST and a stale READY
  item. The id is now the file stem; the stored payload is what is sent. A send the
  gateway refused for an upgraded room never left, so its requeue replaces the stored
  payload with the room resolved now (requeue_item_with_epoch gains `payload`).
- The logged `outbox_cli.py requeue` left the item QUEUED with nothing to drive it:
  the quarantine named the stem `task-proactive-…`, so its body was never restored.
  The quarantine now names a proactive stem as itself; the restored file is claimed
  under the same id and sent once.

Regressions (both fail at a2ce5c9): gateway-proactive-unknown-parks f) runs the
logged command as printed; gateway-proactive-crash-receipt c) changes the owner DM
between the crash and the restart. gateway-proactive-lane-edges covers the new
writers and the drain's fail paths.

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sk thread boundary

backend_a.complete cleared the dispatch mark and then parked the item in two writes.
A crash between them left a live item with no mark and no receipt, so the restart
resent a message that may have landed. park_item now drops the mark in the same write
that parks, and the mark is cleared only after the other transitions, so a crash
before either keeps it and the next claim parks rather than resends.

Regression (fails at 5748e30): gateway-proactive-crash-receipt d) kills the settle
between the writes. gateway-proactive-unknown-parks f) now also pins that nothing is
restored or resent until the operator runs the logged requeue, and that the command
restores the body under its original name. gateway-proactive-outbox-thread pins the
about-task boundary: no task file, a task-last file, or a thread in another room gives
no root, never the source message id.

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hash

Keyed on the stem alone, a new body written later under an already-delivered file name
was archived as "already delivered" and never sent. The id is now `<stem>~<sha256 of the
body, 16 hex>`; the room and thread, re-resolved on every pass, stay out of the key, so a
crash followed by an owner-DM change is still one parked item. The logged requeue passes
`--body-id <stem>`, so the restored body maps back to the same item.

Regression (fails at bf11275): gateway-proactive-unknown-parks g).

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s before the retry budget, holds on a moved DM

Four findings at 9dca540:

1. Two bodies parked under one file name: the second retire overwrote the first (same
   second-resolution name), and restore picked the newest copy. Retirement now goes
   through the quarantine's no-clobber place_as with nanosecond names, and the logged
   requeue carries --body-sha256 so restore installs exactly that item's body.
2. A saved receipt or an interrupted send was checked only after the retry-schedule gate,
   so a restart past the deadline parked it exhausted. backend_a.claim skips that gate
   for an unsettled dispatch; the core then settles it DELIVERED or outcome-unknown.
3. retry_failed_locked parked and the mark was cleared in a second write. Every definite
   outcome now drops the mark in the same write, and an operator requeue drops it too.
4. After a refusal the owner DM moved; the retry sent the stored room while logging the
   new one. A stored destination that differs from the resolved one now holds the file,
   never re-routed; an unsettled dispatch still settles as before.

Regressions: gateway-proactive-recovery-edges (each case fails at 9dca540).

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d destination holds

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… hold/park fail paths

A flat `import outbox` resolves to the package's vendored copy, so the edge test
loads src/outbox.py by file path to measure the canonical writers.

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…a moved destination is held for the operator

- A body that cannot be read at park time no longer prints a runnable requeue with an
  empty --body-sha256 (which restored the newest copy, another generation's body); the
  log says no command is available and why.
- outbox_cli requeue refuses, before any state change (exit 5), an empty or malformed
  --body-sha256, and a missing one while several bodies are parked under the name; the
  refusal lists the candidates' digests.
- A file whose stored destination no longer matches the resolved one is parked
  destination-changed and quarantined, with a logged release that sends it to its stored
  room, instead of being handed back and re-claimed every pass.
- docs: why the file's rename claim stays beside the outbox claim, and why it cannot
  double-send.

Regression: gateway-proactive-recovery-edges 5 (two generations, A unreadable at park)
fails at 56f3603; 4 now pins the parked hold and its release.

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…arking is claim-aware

- A requeue from the CLI records an operator release bound to the stored payload; an
  automatic requeue (room-upgrade resend) records none. The destination guard now applies
  to every dispatch that is not released, so an upgrade-requeued item cannot be sent to a
  stale room after the authority moves on.
- park_unclaimed parks under the item's lock and refuses a delivered, receipted, marked or
  claimed item, so a stale park cannot erase a concurrent delivery and its receipt.
- undelivered_quarantine.restore() itself refuses an empty or malformed digest.
- A park whose file could not be moved logs the release on the next pass, and a delivered
  file that cannot be archived is handed back (archived later), never unlinked.
- Tests assert exit 5 exactly, and the candidate digests in the refusal.

Regressions in gateway-proactive-recovery-edges 6-10 fail at b878f14.

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…es retire hidden; handbacks never clobber

- park_unclaimed releases a claim a reclaim may take (a dead owner past the reclaim
  window) before parking; a live or unknown owner is still refused, and that handback is
  now logged.
- A delivered file that cannot be archived is renamed to a hidden .<stem>.delivered-<ts>
  no bridge globs (a live name let Discord or Slack post it again), reaped into archive/
  each pass; never unlinked, never put back live.
- Every handback in the proactive drain goes through the no-replace rename, so a newer
  canonical body a producer wrote meanwhile is never overwritten.
- A refused room-upgrade park is handed back and not treated as parked.
- Tests: a malformed ('abc') digest for the CLI and restore(); the refusal's candidate
  digests compared exactly (--json).

Regressions in gateway-proactive-recovery-edges 9, 11, 12 and 13 fail at 838e867.

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment review of head 272b0d256d1df03f3719408071f9a8bba9813e81.

Verdict: no change-request condition fires; not approving yet because the approve bar is not met. There is no approval from anyone at this head, and this PR changes the live delivery loop, so it needs at least one expert approval (gateway / outbox area) before an approval from this account.

Missing pieces

  1. An expert approval at this head.
  2. The post-restart live round trip, which the PR body says has not been run (live-witness). Under this account's rule that is named, not blocking, once (1) exists.

What I verified at this head (scratch worktree, isolated HOME, no real workspace or credentials)

  • Suites, all exit 0: gateway-proactive-unknown-parks, -crash-receipt, -dispatch-revalidate, -no-trust-ok, -outbox-thread, -recovery-edges, -lane-edges, proactive-retry-bound, proactive-outbox-classification, send-failure-delegation, gateway-disposal-delegation, src/remote-gateway-bridge.test.py.
  • Mutation controls in delivery_core/core.py deliver_one, to check that the tests can tell the guards are there:
    • disabling the interrupted → OUTCOME_UNKNOWN branch: gateway-proactive-crash-receipt fails (11 FAIL lines);
    • ignoring the revalidate veto: gateway-proactive-dispatch-revalidate fails (16);
    • skipping mark_dispatching: gateway-proactive-crash-receipt fails (8).
  • _send_proactive clears the per-thread _PROACTIVE_SEND.seen before each send, so an auth refusal from an earlier file cannot be re-raised on a later one.
  • _proactive_dispatch_veto wraps the host gate: a gate that raises withdraws the message and does not post it, which matches the PR body.
  • Hardcoded-path scan over added lines: none. CI: every reported check passes. license/cla: success (1 context).
  • Limit of these checks: they exercise the harness, not a real gateway. They cannot show how the hosted gateway treats dedupe_key or a thread root.

No unresolved review threads.

— Sutando (susans-MacBook-Pro)

…wo missing log lines

- A file claim is named .sending.<pid>.<ns>, so a claim retained after a colliding
  handback is never replaced by the next drain's claim of the newer body; orphan
  recovery reads the pid from either shape.
- park_unclaimed sweeps a torn (empty or truncated) claim past its grace period with the
  claim route's own sweep; a fresh one is still refused.
- The release of a dead drainer's expired claim (or a swept torn one) is logged with the
  item id, pid and age; a delivered file that can be neither archived nor hidden is
  logged with its path and left claimed, never put back live.

Regressions in gateway-proactive-recovery-edges 9b, 12, 14 and 15 fail at 272b0d2.

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi added a commit that referenced this pull request Oct 25, 2026
…rt` line naming record_delivered

The literal anchor rotted whenever the import gained a name, as it does on the
#5857 rebase. The pattern now tolerates other imported names and still rejects
a line without `record_delivered` or with a longer identifier.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…kept delivered claim

- A malformed claim's release is logged with its file's age (it has no trustworthy
  claimed_at), not "1792893933s old".
- A delivered file that can be neither archived nor hidden is said to stay claimed; the
  line no longer claims it is never put back live, since a restart's orphan recovery can
  return it, where its DELIVERED item archives it without a resend.

Tests: gateway-proactive-recovery-edges 16 (fails at 7424c87) pins the malformed line
and that a malformed claim naming a live owner is refused; 17 proves a delivered file
orphan recovery returns to its live name is archived, not resent.

Refs #5853.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@sonichi

sonichi commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Live post-restart gateway witness at 254a098, on the owner's go, after Dot and Mini cleared this head (window 02:54:09Z to 02:55:01Z). Room and event ids masked.

  1. Swap: launchd gateway stopped; the PR worktree's wrapper started with the plist env only. singleton: acquired workspace poller lock at 02:54:14Z; the lock pid equals the worktree's gateway pid.
  2. Delivery through the outbox: one proactive file to the owner's DM. delivered proactive proactive-<ts>.to-ag2space.txt to <room> (<event>) at 02:54:28Z; outbox item DELIVERED with the provider ref set; one archived copy, none left under the live name.
  3. Restart: the PR gateway was stopped and started again with a new pid; lock re-acquired at 02:54:32Z.
  4. Replay after restart: the archived bytes were put back under the same live name, as orphan recovery would. proactive proactive-<ts>.to-ag2space.txt was already delivered to <room> (<same event>) — archived, not sent again at 02:54:41Z; item unchanged (DELIVERED, same event id), no second delivered line.
  5. Room: exactly one copy of the test message in the DM (the only other match is the owner's heads-up that quoted it).
  6. Restore: launchd gateway bootstrapped from the main checkout; state=running, new pid holds the poller lock (02:54:53Z); no worktree gateway left; it has since delivered task results normally.

Before/after snapshots of tasks/, results/ and state/ differ only by heartbeat and lock noise.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at 254a098 (agent-assisted review by Mars, Qingyun's Sutando seat). This is my own read of the code, not a relay of the other approvals.

  • The double-send guard holds. Under park_interrupted_send (this lane only, and only for a provider that is neither reconcile-capable nor idempotent), deliver_one asks prior_dispatch first. A recorded event id settles the item CONFIRMED with no send. A dispatch mark left by an earlier incarnation with no receipt parks it OUTCOME_UNKNOWN. Only then is the send marked (mark_dispatching, under the item lock, refused if the token no longer owns the claim) and made.
  • The receipt is persisted before completion: _record_proactive_receipt → record_provider_ref, raising if it can't be kept. record_delivered drops the mark in the same write, and a non-delivered complete clears it only after the transition, so a crash in between parks rather than resends.
  • claim() settles an unsettled dispatch before the retry budget, so a restart past the deadline still resolves it. PROACTIVE_TRUST_OK and the bridge's own retry loop are gone: uncertain outcomes stay UNKNOWN.
  • revalidate runs after the claim and right before the send. A task already answered (result file or delivered copy), or vetoed by the host gate, is withdrawn, and a gate that raises withdraws too (_proactive_dispatch_veto).
  • Ran locally at this head, all passing: gateway-proactive-crash-receipt, -dispatch-revalidate, -unknown-parks, -no-trust-ok, -recovery-edges, -lane-edges, -outbox-thread, proactive-retry-bound, proactive-outbox-classification, gateway-disposal-delegation, send-failure-delegation. CI 36 green.
  • The §15 witness at this head (comment 6392690507) shows the claim that matters live: one delivery, a restart, the archived bytes replayed under the live name, settled from the stored event id with no second send, and one copy in the room.

Minor, non-blocking: _proactive_dispatch_veto builds RESULTS_DIR / f"{tid}.txt" from the payload's task_id without checking its shape. It's only an exists() probe on a file this gateway wrote, but rejecting a task_id that isn't task-<hex> would keep a ../ from ever reaching the path.

@sonichi
sonichi merged commit 9953657 into main Oct 25, 2026
39 checks passed
@sonichi
sonichi deleted the fix/proactive-send-outbox-lane branch October 25, 2026 03:06
sonichi added a commit that referenced this pull request Oct 25, 2026
…ile path, receipts keyed by the lane's item id

Conflicts resolved:
- remote_gateway_bridge.py: main's lane is kept whole. #4399's rename-based
  _retire_delivered_proactive / _reap_delivered_proactive and its bare-stem
  receipt helper are dropped for #5857's inline delivered path (archive, else a
  hidden `.<stem>.delivered-<ns>`, never unlinked) and its no-clobber
  _reap_hidden_delivered. On top of main, _recover_orphan_proactive reads a dead
  owner's claim's item by the lane's id (`<stem>~<body digest>`) through
  outbox.receipt_state (fail-closed; read_item reads malformed JSON as QUEUED):
  DELIVERED is hidden for the reaper and never re-queued, an unclean record keeps
  the claim for an operator, only a missing record goes back live.
- scripts/census-d1.py: main's anchor (`record_provider_ref`) taken; --verify
  reports 0 rotted anchors.
- tests/gateway-disposal-delegation.test.py: main's mover list taken; the
  dropped helpers are no longer movers.
- tests/gateway-proactive-recovery-edges.test.py case 17 now pins that a
  delivered claim found by orphan recovery is hidden, never re-queued, then
  archived with one POST.
- tests/gateway-delivered-retire-keeps-witness.test.py rewritten on main's
  Lane helper and real outbox records, with Mini's probe: a delivered item is
  not read as absent and re-queued.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi added a commit that referenced this pull request Oct 25, 2026
… the proactive outbox

main's #5857 moved proactive sends onto an outbox DeliveryCore. The drain
conflicts are resolved by taking main's drain whole and dropping this
branch's own send machinery that it supersedes: the per-file send and
settlement (_send_claimed), the durable attempt count and its pre-send
gate, the withdrawal sentinel, and the unknown-outcome handling. Kept on
top of main's drain: the lane record with its send_lock protocol, now 2,
and the send lock held across the outbox's dispatch-time recheck and POST.

On the Hatch side the hold line is queued as a message about its task
(proactive-about-<task>--0.txt), so main's dispatch recheck withdraws it,
unsent, once the task is answered. reply's commit (claim, answer) runs under
the send lock, releases its claim if the answer cannot be written, and
reports the line's state from the outbox. Its held-line outcome, once-only
check and health row read the outbox's items; an unreadable item fails
closed. About-task names also take a named instance's task id (`~`).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi added a commit that referenced this pull request Oct 25, 2026
…nt conflicts with #5857's crash settlement (not pushed)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants