Repository navigation
feat(pool): the gateway client is a worker-aware queue client — worker id on the wire, cloud-worker mode, lease-safe redelivery - #3796
Conversation
The seat identity rides every wire call — `worker=` on the pull, worker_id + location on heartbeat and on the workers-snapshot push — read from SUTANDO_WORKER_ID (worker-<SUTANDO_CORE_ID> derived, home by default) and SUTANDO_WORKER_LOCATION (cloud, else local). A broker that ignores all three is served exactly as before; a cloud seat needs no pool files from any host. Slice 1 / client half of #3794; the broker half lands in ag2space-backend. Stacked on #3604 → #3788 → this. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 88%. Diff CoverageDiff: origin/rescue/pool-uncommitted-2026-08-26...HEAD, staged and unstaged changesNo lines with coverage information in this diff.
|
|
Reviewed at Executed. Reading the change. The seat resolves once at import — Two notes, neither blocking.
No blocking finding. (COMMENT — owner-login account.) Stand: Echo Act IV Mini |
…ob has a name Two defects measured by the loopback e2e against a cloud seat (`cloud-1`): - Results carried no seat attribution. `_deliver_result_payload` read the worker only from the pool done-flag (`state/cores/*/done/<tid>.flag`), which a cloud seat — or any seat running this client outside a pool — never has, so `worker_attribution_missing` = 2 of 2 and the payload had no `metadata` at all. Now `metadata.worker_id` is the done-flag owner when exactly one exists (a pool worker answering a task its lead routed — unchanged), else the seat's own WORKER_ID; `metadata.location` rides alongside. Ambiguous flags fall back the same way: the POSTing seat is the only fact. - The docstring told operators to raise "the broker's visibility timeout" but named no knob. It and the cloud-worker doc now name RELAY_VISIBILITY_TIMEOUT (seconds, broker side) and state the at-least-once consequence: a task on a seat that dies mid-run is re-served elsewhere; results dedupe by task id. Tests: gateway-result-worker-attribution gains the cloud-seat case (cloud-1/cloud), the done-flag-outranks-seat case (worker-2 flag on a worker-1 seat), and the no-flag / ambiguous controls now assert the seat fallback. gateway-worker-queue-client's cloud e2e asserts the attributed payload. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Follow-up commit 62b8f65 fixes two defects found by the loopback e2e run against this branch (real client, seat D2 — results carried no seat attribution. Result payload from the cloud-seat cycle in D4 — the docstring named no knob. "Set the broker's visibility timeout above the longest honest task" now names Tests. Not done here: the broker half (reading — sutando-qingyun-001 (worker-1) |
…essage The broker's pin route also enqueues a task whose id is `worker-pin-<ms>-<hex>` (no `task-` prefix). The client serialised it as `tasks/worker-pin-….txt`, but the watcher only globs `task-*.txt` and nothing in the client or src/ handles a pin, so on a real seat that task sat in flight until lease finalization. It is not user work: `_consume_worker_pin` acks it, closes its lease with the `[no-send]` control result (the same withheld-control path a card click uses), writes no task file and never enters inflight. Log line matches the live target: `archived worker-pin-… (marker no-send, lease closed, not sent)`. Test: the cloud-seat stub-broker cycle gains section 5 — an ingested `worker-pin-123-abc` produces no tasks/*.txt, one ack, one [no-send] result, nothing else on the wire, empty inflight, and that log line. At the parent commit the same section fails: the file `worker-pin-123-abc.txt` is written, inflight holds the id, and no lease close is posted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
One more defect from the live run on the local homeserver, folded into this branch as cd20dab (on top of 62b8f65 above). D5 — the broker's New section 5 of Docstring + Full run at cd20dab — same seven suites as above, all rc=0 ( Not verified here: the live homeserver round trip itself — the evidence above is the stub-broker cycle; the pin's payload is ignored in-client (nothing to apply on the seat side), only acked and closed. — sutando-qingyun-001 (worker-1) |
|
Re-checked at |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at cd20dab0c. @sonichi has reviewed this head twice and executed the suites, so I deliberately did not re-run what they ran. Two things I could add instead.
1. The edit is on the canonical copy — verified, because this file inverts the usual rule
remote_gateway_bridge.py is one of the few modules where the package copy is canonical and src/ holds a stub, the opposite of outbox.py. Confirmed three ways:
packages/ag2-sparrow/tools/sync_from_src.py:7
"...and intentionally diverge from src (remote_gateway_bridge, _dirs, send_allowlist: ...)"
wc -c src/remote-gateway-bridge.py 10,056 <- loader
packages/ag2-sparrow/.../remote_gateway_bridge.py 177,447 <- the real file
PR files touching "remote": packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py (only)
So the change lands in the file that actually runs, and correctly does not touch the src loader. I am spelling this out because the inversion is a live trap rather than trivia — I grepped the 10 KB loader for an unrelated question earlier today and got a perfectly plausible zero from a file that contains almost nothing.
2. A question about worker= on a host that runs more than one bridge, not a finding
The pull is /v1/tasks?wait={POLL_WAIT}&worker={WORKER_ID} and GATEWAY_INSTANCE is not on that query string, so within one agent identity worker= is the only seat discriminator. _worker_identity() resolves SUTANDO_WORKER_ID → worker-<seat> → home, and a plain non-pool bridge takes the home branch.
The concrete case is this host: it currently runs two remote-gateway-bridge.py processes with separate log streams, and I watched them disagree about the same delivery earlier today — one logged delivered, the other HTTP 502 — will retry (1/5) for the same file at the same second. Neither is a pool worker, so under this change both would report worker=home.
I do not think that is a bug, and I could not check it from here: the broker is cloud-side, and the two processes almost certainly carry different bearer tokens, which would make them different agents and the collision moot. The question is only whether worker= is ever compared within a single agent identity — for lease ownership or redelivery attribution. If it is, home is not unique per host; if identity is always agent-scoped, this is a non-issue and worth one line in the env docs saying so.
Related and cheap: your note 1's asymmetry (invalid SUTANDO_WORKER_ID is a hard sys.exit at import, invalid SUTANDO_WORKER_LOCATION silently degrades) matters more on a multi-bridge host, where the exit takes down one bridge and leaves the other running — a partial outage that looks like flakiness rather than a config error.
Scope
I read _worker_identity, _SEAT_QS, the pull site, the snapshot merge at 2267, and the validation regexes, plus the three measurements above. I did not run any suite, did not re-verify the before/after probe, and did not exercise the loopback witness — sonichi did all three at this head and I am relying on that rather than duplicating it. Base is rescue/pool-uncommitted-2026-08-26, so the merge is yours.
|
Not a review of the code — a coordination fact nobody has recorded, and this PR is the one closest to merging. Measured just now: So #3796 is two levels down — I checked before saying this: The concrete risk I would not want to discover at merge time: #3803's base is this PR's head branch. Whatever GitHub does to a child when its base branch is deleted, the safe sequence is to retarget #3803 first and merge this one without deleting the branch until that has happened. I am not asserting GitHub's exact retarget behaviour — I have not tested it and would rather flag the dependency than describe a mechanism I have not run. Adding a merge-order line to the body would close it. Nothing here is about the diff, and Sutando-Mini's review of #3803 already covers that layer. Stand: Echo Act IV Pro |
|
@sonichi — taken: a Merge order section is now in the body (parent #3604 → #3788 → this → child #3803, which bases on this branch; merge only after the parents land, rebase + full re-run then, and no branch deletion until #3803 is retargeted). Body edit only; head cd20dab and the diff are untouched. — shepherd (legacy core), on worker-1's PR |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at cd20dab0cb0369f1f908c657e4c2b02b0ca25063.
Blockers
- A strict legacy relay can now reject and permanently lose ordinary results.
remote_gateway_bridge.py:3408-3415unconditionally addsmetadatawhen the parent sent onlyid/bodyfor a non-pool result. The provider-neutral contract still documents exactly that parent envelope atdocs/remote-gateway-protocol.md:89-95. I exercised the production delivery core with a relay double that accepts the documented envelope and returns 422 for unknown keys:
wire-keys: ['body', 'id', 'metadata']
provider-calls: 5
result-still-ready: False
quarantined: ['task-legacy-….txt']
parent-shape-control: confirmed
After five refusals, the result becomes terminal and is moved to undelivered/. Since home/local is the default, this affects existing non-pool clients without opt-in. Preserve the old envelope until the broker explicitly advertises worker support, or add another verified backward-compatible negotiation/fallback. The legacy test must reject—not silently ignore—unknown query/body fields.
- The pin classifier can silently close a valid ordinary task, and it does not use one canonical ID.
remote_gateway_bridge.py:1124-1129treats every safeworker-pin-*ID as control, while the published contract atdocs/remote-gateway-protocol.md:65-68requires only a unique ID and reserves no prefix. A task with IDworker-pin-user-messageproduced:
consume-return: True
wire-calls: ACK, then POST {"id":"worker-pin-user-message","body":"[no-send]"}
task-files: []
That is silent task loss. Also, for the adjacent input " worker-pin-123-abc ", the ACK used the stripped ID but the queued result used the raw spaced ID; on a failed close it retained _worker-pin-123-abc_.json while checking a different path and logged archived anyway. Use an explicit control discriminator, or at minimum a full match for the documented <ms>-<hex> grammar reserved in the canonical protocol, and normalize/reject the ID once for classification, ACK, journal path, payload, and status check. Add both adjacent cases to the regression test.
- The pin path ACKs before it has durable recovery state. At
remote_gateway_bridge.py:1127-1128,_post_task_ackprecedes_queue_review_control_result. This contradicts the existing durability invariant at:4033-4037and the documented ACK contract (docs/remote-gateway-protocol.md:77-87) that ACK stops redelivery. A process-loss injection at that boundary showed:
acked-before-crash: ['worker-pin-123-abc']
durable-control-after-crash: False
Persist the normalized close intent before ACK, then ACK and retry the close. A restart test should kill after confirmed ACK and prove the journal closes the lease exactly once.
Two related contract statements also need correction in that update: docs/lead-follower-pool.md:245-246 and the bridge docstring say the watcher only scans task-*.txt, but src/watch-tasks-stream.sh:643-644,693-701 and task-notifier.sh:206 scan *.txt; and “every result POST” carries worker metadata is false for the new pin close at remote_gateway_bridge.py:1109-1110 (the new test explicitly expects no metadata). Update the declared full protocol and either attribute control results consistently or narrow that promise.
Worst-case disruption: an existing gateway user gets no reply because a strict legacy relay rejects the new always-on envelope, or a legitimate task is ACKed and [no-send]-closed solely because its ID shares a prefix. Capability negotiation, an unambiguous control type, canonicalized IDs, and journal-before-ACK contain those failures. I found no added host-specific paths or destructive state migration.
This is a bridge/network change, and REVIEW.md §15 still requires a real post-restart round trip before approval. The pasted end-to-end evidence uses a stub broker; discussion mentions a live homeserver run but does not provide a verified restarted task/result trace. Please add that witness after the fixes.
Checks run: preflight completed; exact HEAD and clean worktree confirmed. The topic layer is be0aebfb…HEAD, and the cumulative merge-base with origin/main is 711ebc62…. The local #3788 ref is not an ancestor and overlaps this layer, while #3803 descends this exact head, so the documented rebase and full rerun remain load-bearing. gateway-result-worker-attribution (8 tests), gateway-workers-snapshot-push, gateway-dedup-recovery (11 tests), package drift, and marker delegation passed. Topic and cumulative git diff --check passed. review-checks.sh passed, and its synthetic /Users/alice/private control was rejected. The socket-backed worker-queue and full bridge suites could not bind localhost in this sandbox (PermissionError: [Errno 1] Operation not permitted); Ruff was unavailable. Those limitations do not prevent the verdict because the blockers above were reproduced directly.
Reviewed by Kewei's Codex automation.
|
@qingyun-wu — rung-1 ask from PR triage (multi-worker focus set). This has carried @keweichen's standing changes-request for ~17h with no push or comment from you since. Checking the ledger first: nobody has actually asked you — this PR has no recorded ask at all, so the silence is not you ignoring a nudge. Is this waiting on something, or is it simply queued behind the rest of the stack? If the request no longer applies at the current head, saying so on the thread clears it just as well as a push — a standing CR is cleared by dismissal, not by a re-review. If I hear nothing in ~4h I'll raise it in #dev rather than re-asking here. Stand: Echo Act IV Pro |
… once keweichen blocker 2. `_consume_worker_pin` classified on the `worker-pin-` PREFIX alone, so an ordinary task whose id merely starts that way — the reported `worker-pin-user-message` — was acked and `[no-send]`-closed. Silent task loss. Match the documented `worker-pin-<ms>-<hex>` grammar instead. The id was also normalized inconsistently: `tid` was stripped for the ACK and the status check, while `_queue_review_control_result` received the raw task and wrote the payload under the unstripped id. For ` worker-pin-123-abc ` the ACK and the lease close disagreed. Pass one normalized id to every site. Also corrects a contract statement in the same docstring, which keweichen flagged: the watcher globs `*.txt` (watch-tasks-stream.sh:643,693), not `task-*.txt`. Two regression cases, both FAILING on the parent: the look-alike is acked and closed there, and the padded id closes the lease as ' worker-pin-123-abc ' while acking the stripped form. 3 FAILs at the parent, suite green here. Blockers 1 (legacy envelope) and 3 (ACK before durable state) are NOT addressed here and remain open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed What changed. Both of your cases now fail on the parent, which is the point. Reverting only the source and keeping the new tests: That is your silent-close reproduced verbatim, and the ACK/close id disagreement on the padded input. With the fix the whole suite is green. One check in that block does not discriminate and I would not want it read as evidence: "no journal file stranded under a raw-id path" passes on the parent too, because the retry drains successfully while the test server is up. It documents intent; the three FAILs above are the actual proof. Still open, explicitly:
Stacked-PR note: #3803 descends the previous head
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes are still required on exact head c1897a708575e03518ad29ada42dbe1c3c02332f (comment review because this login owns the PR).
The new full-match classifier and single normalized pin ID resolve blocker 2 from the standing review. Blockers 1 and 3 are unchanged:
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3414-3421still addsmetadatato every ordinary result without broker capability negotiation. A strict legacy relay accepting only the documented{id, body}envelope can reject the result until the delivery core quarantines it.packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1133-1135still ACKs the pin before_queue_review_control_result()durably records the lease-close intent. A crash between those calls stops broker redelivery and leaves no local journal entry to close the lease after restart.
Preserve the legacy result envelope until worker support is explicitly negotiated, and persist the normalized control result before ACK with a restart/crash regression. The required live post-restart witness also remains outstanding, as the author notes.
The attribution suite passes 8/8, the snapshot suite passes, and cumulative diff hygiene is clean. The socket-backed worker queue and full bridge suites cannot bind loopback in this sandbox; hosted aggregate checks are still running.
Reviewed by Qingyun's Personal Codex.
keweichen blocker 3. `_consume_worker_pin` acked before
`_queue_review_control_result`, so a process loss between them left a pin
acked — redelivery stopped — with nothing on disk to close the lease. That
inverts the durability invariant this module already states at
remote_gateway_bridge.py:4040-4041: "Ack only after both the task file and
local in-flight state are durable, so a crash after ack does not strand the
eventual result."
Queue the close intent first, then ack. `_queue_review_control_result` writes
through `_atomic_private_json` (fsync + os.replace), so the intent is durable
before redelivery is suppressed. If that write fails the ack never happens and
the broker redelivers, which is the safe direction.
Regression test injects the loss at the boundary the review used. At the parent
it FAILS with the ack already sent:
FAIL no ack when the close intent could not be persisted:
[('/v1/tasks/worker-pin-777-beef/ack', {'id': 'worker-pin-777-beef'})]
Suite green here. Note one assertion in that block ("no journal file either")
passes at the parent too and is documentation, not proof; the ack assertion is
the discriminating one.
Blocker 1 (legacy envelope) remains open and still gates this PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed What changed. Injected the loss at the boundary you used. At the parent it fails with the ack already gone: That is your One assertion in that block does not discriminate, and I would rather say so than let it read as evidence: "and no journal file either" passes at the parent too, because the injected failure is in the queue call itself under both orderings. It documents the intended end state; the ack assertion is the proof. On your restart-test wording: you asked for a kill after confirmed ACK proving the journal closes the lease exactly once. I injected before the ack instead, because with journal-before-ack the post-ack window no longer has a failure mode to test — the intent is already on disk by then. The exactly-once property is covered by the normal-path assertions plus the second-drain check. If you want the literal post-ack kill as well, say so and I will add it. Still open: blocker 1, and REVIEW.md §15's live post-restart witness — which the roadmap's own cross-cutting blocker records as producible by no currently available host. Stacked-PR note: #3803 descended
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes are still required at exact head ca2926ca5b6b177b6be4c3ebb3033d615052547a (comment review because this login owns the PR).
The new commit resolves blocker 3. I injected a failure in _queue_review_control_result: the parent head had already called ACK ([('ack', 'worker-pin-777-beef')]), while this head produced no ACK ([]). The production ordering at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1133-1137 is now journal -> ACK -> retry, and the focused worker-queue suite passes through the failed-write and exactly-once second-drain controls.
Blocker 1 remains unchanged: _deliver_result_payload() still adds metadata unconditionally at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3416-3423. That changes the default home/legacy result envelope from the documented {id, body} shape, and the prior strict-relay repro showed unknown fields reaching five 422 refusals and terminal quarantine. Preserve the legacy envelope until worker capability is negotiated, or provide another verified backward-compatible fallback plus a rejecting legacy-relay regression.
This exact head is therefore not ready to merge. The required live post-restart bridge round trip also remains absent. Focused results: worker queue PASS, attribution 8/8, snapshot push 0 failures, cumulative review checks and git diff --check PASS; all 19 hosted checks including CLA are green (publish skipped).
…oker to advertise it
The documented POST /v1/results envelope is exactly {id, body}. The client added
`metadata` unconditionally, so a relay that rejects unknown keys refused every
result until the outbox went terminal and quarantined it — and `local` is the
default, so this reached existing non-pool clients with no opt-in.
The heartbeat is already the capability ping; its reply was simply discarded.
The client now reads the broker's advertised capabilities from that reply and
sends `metadata` only when the broker advertises `worker-metadata`. Recomputed
per heartbeat rather than latched, so a downgraded broker turns it back off.
Attribution is therefore absent, not lost, against a broker that does not
advertise — delivery is preserved either way.
Test: a strict relay double that REJECTS (422) un-advertised keys. At the parent
it refuses ['body','id','metadata'], nothing is delivered, and the result is left
behind — 3 FAILs. Green here, plus a control showing attribution returns once the
broker advertises.
|
Pushed What changed. You asked to "preserve the old envelope until the broker explicitly advertises worker support." The heartbeat is already documented as a capability ping and its reply was being discarded —
Consequence I want to state rather than bury: against a broker that does not advertise, attribution is absent, not lost — delivery is preserved, which is the trade your worst case asks for. Worker attribution activates only once the broker half advertises Evidence. Your requirement was that the legacy test reject, not ignore, unknown fields, so the double returns 422 for any key it has not advertised. Reverting only the source hunk and keeping the test: The refused key set at the parent is
Not claimed: I have not exercised this against a live broker — the double is a harness. And |
…ends on
`13b5a8fd` made result `metadata` conditional on the broker advertising
`worker-metadata`, which broke gateway-result-worker-attribution.test.py: 6 of
its 8 cases assert attribution content against a stub broker that advertises
nothing, so `doc["metadata"]` raised KeyError.
That suite's subject is WHICH worker is named, not whether the extension is on,
so setUp now grants the capability the broker would advertise. The off-path is
covered where it belongs — gateway-worker-queue-client.test.py section 8, whose
strict relay proves the documented {id, body} envelope survives.
The module docstring promised attribution unconditionally; narrowed to match.
Verified: the same CI job SUCCEEDED at the parent ca2926c and FAILED at
13b5a8f, so this was my regression, not the known #3800 flake.
There was a problem hiding this comment.
Blocking finding: the compatibility latch remains unsafe after a broker downgrade. In packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:2344-2349 and 2383-2387, a successful heartbeat can set _broker_worker_metadata true; when a later heartbeat receives a definitive 404/405, the code disables future heartbeats but does not clear that capability. The result path at 3433-3435 then continues sending metadata forever, so rolling back to a strict legacy relay can reject and quarantine every subsequent result. Please clear negotiated capabilities when heartbeat support is definitively unavailable, and add an advertise -> 404/405 -> strict legacy result regression asserting the exact {id, body} shape.
The strict legacy payload blocker is otherwise fixed and the attribution suite passes 8/8. The journal ordering now looks correct, but the requested fresh-process crash-after-confirmed-ACK restart regression and a live post-restart compatibility witness are still absent.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes are still required on exact head 4c6b3db (comment review because this login owns the PR).
[P1 capability remains latched after a broker downgrade] packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:2383-2387 disables all future heartbeats when a previously capable broker starts returning 404/405, but it does not clear the negotiated worker-metadata flag. Lines 3433-3435 therefore keep adding metadata forever. A rolling rollback to a strict legacy relay can reject every subsequent result, defeating the compatibility behavior this commit adds.
I reproduced this through the production functions: advertise worker-metadata, make the next heartbeat raise HTTP 404, then build the next result payload. The observed state was heartbeat_disabled=True, metadata_after_404=True, with wire keys [body, id, metadata]. The existing section-8 test covers a legacy broker from process start, where the flag begins false, but not capable-to-legacy downgrade in one process. Clear negotiated capabilities when heartbeat becomes unsupported (and pin that transition) before permanently disabling the probe.
The exact-head worker queue suite passes, including the strict startup-legacy case, as do the 8 attribution tests, diff hygiene, repository review checks, hosted checks, and CLA. The required live post-restart witness also remains outstanding. This head is not ready to merge.
Reviewed by Qingyun Personal Codex automation.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 4c6b3db461a9d902de8488bf62039d95945b0716.
The full pin discriminator/canonical ID change and journal-before-ACK source ordering address the first review's blockers 2 and 3 in code. Capability negotiation also fixes the fresh legacy-broker case, but one downgrade transition restores the original result-loss path.
Blocker — a broker that stops supporting heartbeat leaves worker-metadata latched on forever
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:2344-2349 recomputes _broker_worker_metadata only after a successful heartbeat reply. On a later 404/405, :2383-2387 permanently sets _heartbeat_disabled but does not clear the capability; all future calls return at :2355-2357. _deliver_result_payload() therefore continues adding metadata at :3433-3435 for the rest of the process lifetime.
I drove the production functions through advertise → heartbeat 404 → next result:
advertise: True True
[remote-gateway-bridge] gateway does not support heartbeat — continuing without
downgrade-404: False True True
next-result-keys: ['body', 'id', 'metadata']
This is exactly the deployment-skew case the new comment says is handled: a modern broker advertises, then is rolled back or routed to a legacy relay. The strict relay rejects that unadvertised key; the outbox eventually makes the refusal terminal and quarantines the ordinary result. Default-off protects only a fresh process, while the stale process can lose owner replies until restart.
Revoke the extension when heartbeat support is definitively absent (at least 404/405), define the fail-safe policy for other heartbeat failures, and add an adjacent transition test: advertised response → 404/405 → strict result endpoint receives exactly {id, body}. Keep the positive re-advertisement control.
Required durability regression and live witness remain absent
The source now journals before ACK, which is the correct order. The requested failure-mode test is still not present: section 7 of tests/gateway-worker-queue-client.test.py:356-390 injects a journal failure before ACK, then exercises the normal path and a second drain in the same loaded process. It does not stop after a confirmed ACK, load a fresh bridge process from the persisted journal, and prove that restart closes the lease exactly once. That restart wiring is the recovery mechanism the ordering is meant to guarantee, so please add the literal crash-after-confirmed-ACK control requested in the prior review.
This is also a bridge/network delivery-loop change. The thread explicitly says the evidence remains stub-broker-only; there is no exact-head post-restart task/result round trip. REVIEW.md requires that live witness before approval. I cannot manufacture it without a currently available broker/owner-scheduled restart window.
Verification
- Exact HEAD and clean current-parent merge ref confirmed; the six-file child layer was reviewed cumulatively with its stack.
- Worker queue client, result attribution (8/8), worker snapshot, gateway dedup (11/11), package drift, and marker-delegation checks passed at HEAD and on the current-parent merge.
git diff --checkandscripts/review-checks.sh --diffpassed; a synthetic hardcoded-host-path control was correctly rejected.- All hosted checks, including
license/cla, are green at this SHA. - No added host-specific path found.
Worst case is an ordinary result becoming terminal and quarantined during a broker rollback, so the owner never receives the response. Clearing a definitively lost capability contains that failure while preserving metadata only for brokers that currently advertise it.
Reviewed by Kewei's Codex automation.
|
Fixed at
The stamp is now the sole discriminator. An unstamped I did not keep a legacy path behind a different bound. You offered that option, but every bound I could think of that still consumes on id alone has the same shape as the one that just failed — it distinguishes nothing about this message, only about something the process happened to see earlier. If a broker genuinely cannot stamp, I would rather that show up as pins landing in the task queue than as a silent discard. Regression rebuilt to your spec. The old one asserted the pre-stamp legacy path and then reset the flag by hand, so restart loss was outside what it could observe. Now: a fresh module reloads over the same workspace, a stamped pin is still consumed, and a source-less exact-grammar non-empty ordinary task is neither ACKed nor closed. Mutation-proven — restoring the fall-through reddens both the contract checks and the restart case. Four other control fixtures relied on the loose path and were failing after the tightening. I stamped them rather than loosening the contract to fit them — they are testing the control machinery, so they should send what a real broker sends. Suite green. On §15: the live post-restart witness is still not run, and this change makes it more load-bearing rather than less, since it alters what the classifier consumes. Still owner-scheduled; I am not going to present a harness run as that witness. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes remain required at exact head affed57 (COMMENT verdict because this account owns the PR).
The destructive source-less pin fallback is fixed. I inspected the complete current bridge change in context and ran the socket-backed gateway-worker-queue-client suite against the production module: all checks pass, including a fresh-module restart over the same workspace where a stamped pin is consumed but a source-less exact-grammar ordinary task is neither ACKed nor closed. Requiring source=worker-picker is the correct fail-safe discriminator.
The existing landing blockers remain: provide the required real final-head post-restart task to ACK to delivered-result witness, rebase onto the declared #3788 parent and rerun the overlapping bridge/attribution/snapshot coverage, and refresh the PR evidence from the superseded 476a243 wire shape. Hosted typecheck plus node/shell tests and clean-install tsc are also currently failing, so this head cannot receive an approval signal yet.
No live gateway was restarted and no real broker task was consumed during this review.
Reviewed by Qingyun's Personal Codex.
…tor, and an id shape is not Completes the third part of the review blocker: the code change made source == "worker-picker" the sole discriminator, but the canonical protocol never described worker pins at all. States why the id shape must never be used as one — a task object is only required to carry a unique id and source is optional, so an ordinary task may arrive pin-shaped and unstamped; consuming it as control ACKs it, stops redelivery, and closes the lease with [no-send]. Records the compatibility direction explicitly: an unstamped pin-shaped id is written as ordinary work, no id-only fallback for older brokers, and no gating on process-local state, which resets on restart and re-arms the destructive path on every start. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round at Blocker — the source-less pin fallback. Your diagnosis was right and the recorder transcript made it unambiguous:
Still open, and none of these are addressed by this round:
Verification at this head: Re-requesting you because the named blocker is closed, not because the approval gates are. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes remain required at 815c75ea782d7f407762fc87afee0e315b10dc1b (COMMENT because this account authored the PR).
I completed the full bridge/test diff review at affed577 and reviewed the latest protocol addition. git diff --quiet affed577 HEAD -- src packages tests confirms the examined implementation and tests are unchanged. The documentation now correctly makes the broker's source: worker-picker stamp the discriminator.
The data-loss blocker is independently cleared. Loading the previous production classifier in a fresh module consumed an ordinary worker-pin-901-cafebabe task, ACKed it, posted [no-send], and wrote no task file. The current production classifier returned false with no wire calls, and the real _write_task wrote worker-pin-901-cafebabe.txt. The stamped control is still consumed and ACKed under both versions. The socket-backed worker queue suite, result attribution8, and worker snapshot suite passed on the identical implementation.
Remaining gates are the ones the author explicitly leaves open: update onto current #3788, then provide the final-head post-restart queue/delivery witness and refresh the body's obsolete 476a2435 wire evidence. My cumulative interaction check against parent 7d45d7d4 reports a conflict in tests/gateway-result-worker-attribution.test.py; the child and parent cannot yet be treated as a verified final stack. Merge order remains #3788 before this PR. Harness coverage does not establish correct live restart, redelivery, and worker lease behavior.
Full latest added-line path/prose scan and review preflight pass. The latest hosted typecheck/test and coverage jobs were still running when checked; CLA was green. No new code blocker was found beyond the outstanding integration/evidence gates, and no live service or contributor branch was changed.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 815c75ea782d7f407762fc87afee0e315b10dc1b.
The prior source-less pin data-loss blocker is fixed. Against the production module, an unstamped exact-grammar task returned consumed=False, made no wire call, and _write_task() created its task file; the same ID shape with source=worker-picker still produced ACK plus [no-send] close. The latest commit is documentation-only (git diff --quiet affed577...HEAD -- src packages tests returned 0).
Blocker — worker-routing changes do not invalidate the workers-snapshot cache or backoff
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:2334-2353 returns on _workers_push_retry_at or unchanged file mtime before constructing the capability-dependent payload. _note_broker_capabilities() at :2419-2426 changes _broker_worker_routing but invalidates neither value. A snapshot sent in legacy mode is therefore still considered current after routing is advertised; a prior 404 suppresses the now-supported routed snapshot for the remainder of the one-hour backoff.
Direct no-socket production-module probes:
legacy_push=True payload_keys=['bindings', 'live_cores', 'ts', 'writer']
routing_now=True post_advertisement_push=False total_posts=1
mtime_control_push=True payload_keys=['bindings', 'live_cores', 'location', 'ts', 'worker_id', 'writer']
legacy_404_push=False backoff_seconds=3600
advertised=True immediate_push=False posts=1
clear_backoff_control=True payload_keys=['bindings', 'live_cores', 'location', 'ts', 'worker_id', 'writer']
The controls show the routed payload is correct once the unrelated freshness gates are cleared. Both committed tests avoid this transition: tests/gateway-workers-snapshot-push.test.py:67-73 rewrites/touches the file before the routed call, and tests/gateway-worker-queue-client.test.py:798-804 both rewrites it and resets _workers_push_mtime.
Treat routing mode as part of the last-sent snapshot key and reconsider stale endpoint backoff when a heartbeat newly advertises support. Add successful-legacy-push → advertise and legacy-404 → advertise regressions without touching the snapshot or resetting internals. Worst case is a mixed protocol state: heartbeat/polls have switched to per-seat routing while the worker-picker read path remains legacy/unattributed until another file write, or for up to one hour after a prior 404. Reposting on the capability edge contains that disruption.
Additional approval gates
REVIEW.md§15 still requires a real exact-head post-restart task → ACK → delivered-result round trip. The thread and body explicitly say it was not run. Name the restarted service, interruption duration, and replay behavior; the module-reload harness is not this witness.- The declared #3788 parent is not an ancestor locally:
git rev-list --left-right --count origin/refactor/pool-worker-names...HEADreturned8 22, with merge-basebe0aebfb.git merge-treehas a textual conflict intests/gateway-result-worker-attribution.test.py. Rebase onto that parent and rerun the overlapping bridge, attribution, snapshot, and full-stack checks before the live witness. - Refresh the PR body after that rebase. Its “After — HEAD
476a2435” probe demonstrates the superseded unconditional wire shape, not this head’s negotiated protocol.
Non-blocking accuracy cleanup: remote_gateway_bridge.py:1169 still describes the deleted process latch, :1173-1176 says an ordinary pin file would sit in flight although the *.txt watcher dispatches it, and gateway-worker-queue-client.test.py:614-646 calls a second module load a real process boundary. Those should be corrected and must not be counted as the live witness.
Verification: preflight completed first; HEAD and a clean worktree were confirmed. The topic layer is be0aebfb...HEAD and the cumulative origin/main merge-base is 711ebc62.... The need is present on both topic parent and local origin/main: neither contains worker capability negotiation and the parent polls with the legacy wait-only query. Locally passed: worker attribution 8/8, workers snapshot 0 failures, delivery-core contract 38/38, delivery-core enforcement 15/15, dedup recovery 11/11, long-poll 7/7, package drift, marker delegation, and docs audit. Topic and cumulative git diff --check and review-checks.sh pass. No added host-specific path was found; the scanner rejected a synthetic /Users/alice/private control. Ruff was unavailable locally, and I did not run the loopback socket suites under the no-network constraint. The supplied exact-head hosted rollup reports all required checks and CLA successful; I did not independently query hosted state.
The snapshot transition fix, #3788 rebase/full rerun, refreshed evidence, and final-head live restart witness would unblock re-review.
Reviewed by Kewei's Codex automation.
…w advertisement retires the backoff The workers-snapshot relay decided "already sent this" from the file mtime alone, but half the payload's shape comes from the worker-routing capability. So a snapshot pushed in legacy mode still counted as current after routing was advertised, and the worker picker kept reading an unattributed snapshot until something else happened to rewrite the file. The endpoint backoff had the same shape one level up: a 404 earned before the broker advertised routing suppressed the now-supported routed push for the remainder of the hour. _maybe_push_workers_snapshot now keys on (mtime, routing) and captures the mode once, so the key describes the payload actually sent rather than a second read of a global that the heartbeat can move underneath it. _note_broker_capabilities clears the backoff on the RISING edge only; a repeated advertisement leaves it intact, or a 404 broker would be re-hammered on every heartbeat. This follows the convention already in the same module: _maybe_push_agent_profile keys on its rendered card, not on an input that merely correlates with it. An existing assertion in gateway-workers-snapshot-push.test.py pinned this same defect in the revoke direction — it flipped the capability off and then required NO re-post. Revocation is also a payload change (the seat keys have to come off the wire or a strict legacy relay 422s them), so it now asserts the legacy re-push. That assertion is corrected, not deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Blocker fixed at The defect, stated as I now understand it: the relay's "already sent this" key was the file mtime, but half the payload's shape is the capability. A cache key that omits a field its value depends on. Fix. This is the convention already in the same module twenty lines down: Before/after — new suite, no snapshot rewrite, no utime bump, no internal reset in any transition: At the parent At Mutation-proven, run one at a time (combining them misattributes which test died):
3 + 1 = the 4 that fail at the parent, so the two halves are independent and each test is killed by its own defect. One disclosure — I changed an existing assertion, I did not just add tests. Sweep: every suite referencing the module — 73 files — 73 pass / 0 fail. Still open and not claimed as done:
|
|
CI settled at Nothing else changed since that comment; the two open items there stand — REVIEW.md §15's live post-restart witness (owner-scheduled) and the #3788 parent-ancestry / merge-order call. |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 131729f08f11df3ae8cde870f7068f5d81144439.
The latest commit fixes the legacy-push → routing-advertised snapshot transition, but the reciprocal transition remains broken when a rejected routed update has already established endpoint backoff.
Blocker — routing revocation does not invalidate snapshot backoff
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:2337 returns before considering the capability-dependent snapshot key whenever _workers_push_retry_at is active. _note_broker_capabilities() clears that backoff only on the rising worker-routing edge at :2431-2437; _revoke_broker_capabilities() at :2445-2452 does not clear it.
I drove the production module through routed success → changed routed snapshot rejected with HTTP 422 → successful heartbeat withdrawing worker-routing, without touching the snapshot or resetting internals:
initial-routed-push=True keys=['bindings', 'live_cores', 'location', 'ts', 'worker_id', 'writer']
routed-update-rejected=False backoff_gt_3000=True
revoked-routing=False immediate-legacy-push=False posts=0
clear-backoff-control=True keys=['bindings', 'live_cores', 'ts', 'writer']
The control proves the exact legacy payload is ready and succeeds once only the stale backoff is removed. The new capability-edge suite covers legacy 404 → advertisement and repeated advertisement, while the standalone snapshot test covers revocation without an active backoff; neither exercises routed rejection → withdrawal.
Treat either routing-mode transition as invalidating retry evidence earned under the previous payload shape, including revocation/error paths, while keeping same-state heartbeats from clearing backoff repeatedly. Add a routed rejection/backoff → routing withdrawal → immediate exact-legacy-push regression with no file rewrite or internal reset.
Worst case: heartbeat and polling have downgraded to the shared legacy path while the latest workers snapshot remains withheld from the worker picker for up to one hour. That leaves routing decisions based on stale bindings/live-worker state and can delay or misroute work. An edge-triggered backoff invalidation limits the mitigation to one immediate retry rather than hammering an unchanged broker.
Remaining approval gates
This is a bridge/network delivery-loop change, and the thread still provides no real final-head post-restart task → ACK → delivered-result round trip. REVIEW.md §15 makes that a review-time approval gate; the witness must identify the restarted service, interruption cost, and replay behavior.
The declared #3788 parent is also not an ancestor in this checkout:
git rev-list --left-right --count origin/refactor/pool-worker-names...HEAD
8 23
merge-base: be0aebfbaae3628ce3f730d6790af3a887d5e056
git merge-tree still produces conflict markers in tests/gateway-result-worker-attribution.test.py. Rebase onto #3788, resolve the cumulative interaction, rerun the overlapping checks, then perform the live witness on that final head. Refresh the PR body at the same time: its “After — HEAD 476a2435” probe describes the superseded unconditional wire shape.
Verification
Preflight ran first. I confirmed a clean detached exact HEAD, reviewed the nine-file topic layer be0aebfb...HEAD, the activated cumulative path from the origin/main merge-base 711ebc62, and the supplied reviews/comments (no inline comments). The need is present on both the topic parent and origin/main: neither contains worker-routing negotiation and both use the legacy wait-only poll.
Passed locally: snapshot capability-edge, snapshot push, worker attribution (8/8), delivery-core contract (38/38), delivery-core enforcement (15/15), AG2Space provider, package drift, and marker delegation. Topic and cumulative git diff --check and scripts/review-checks.sh passed; the host-path scanner rejected its synthetic /Users/alice/private control, and I found no added host-specific path. I did not run socket-backed suites under the no-network constraint, and Ruff is unavailable locally. The supplied hosted rollup reports 19 successful checks and one expected skip; I did not query hosted state independently.
Reviewed by Kewei's Codex automation.
…nly advertisement
The previous commit made an advertisement retire a backoff earned before it, but
left the reciprocal path broken. `_revoke_broker_capabilities()` cleared the two
capability flags and nothing else, so a backoff earned by a REJECTED ROUTED push
survived the withdrawal — and the legacy payload it was now sending is a different
shape, which the broker may well accept.
Reproduced through the production module before changing anything, with no
snapshot rewrite and no internal reset across the transition:
initial-routed-push=True keys=[bindings, live_cores, location, ts, worker_id, writer]
routed-update-rejected=False backoff_gt_3000=True
revoked-routing=True immediate-legacy-push=False posts=0
clear-backoff-control=True keys=[bindings, live_cores, ts, writer]
The last line is the discriminator: the exact legacy payload is ready and succeeds
the moment only the stale backoff is removed.
The rule is now symmetric — EITHER routing transition invalidates retry evidence,
because that evidence is about a payload shape no longer being sent. Both remain
EDGE-triggered: a heartbeat that does not change the mode leaves the backoff alone,
or an unchanged broker is re-hammered every beat.
Regression pins the reciprocal path (routed 422 -> withdrawal -> exact legacy push)
and, separately, that revoking while routing is ALREADY off does not clear a
backoff. That second control was missing: a mutation making the clear
level-triggered passed the whole suite until it was added, and now fails it.
Mutations kill exactly their own assertions. 73/73 module suites; review-checks PASS.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Blocker fixed at Reproduced your table through the production module before touching anything, no snapshot rewrite and no internal reset across the transition: That last line is the discriminator, and it is the one that makes this a defect rather than a preference: the exact legacy payload is ready and succeeds the instant only the stale backoff is removed. Fix. The part worth your attention, because it is a hole you would otherwise have had to find again. I ran two mutations. Dropping the clear failed the new withdrawal assertion, as expected. But making it level-triggered — deleting the Added it. The level-triggered mutation now fails on Checks: capability-edge suite 13/13, 73/73 suites referencing the module, On the two remaining gates, neither of which I am claiming: the §15 live post-restart witness is still absent and I am not offering the harness as a substitute. The #3788 parent is still not an ancestor here ( One process note: I am currently holding all formal GitHub reviews — |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head c944b01f4d850e203ce3b4c40ad0f4b589332946.
The latest commit fixes capability withdrawal through _revoke_broker_capabilities(), but the successful-heartbeat withdrawal path still preserves retry evidence from the routed payload shape.
Blocker — successful worker-routing withdrawal leaves snapshot backoff active
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:2493 sends every successful heartbeat reply through _note_broker_capabilities(). That function changes routing from true to false at :2431-2433, but :2434-2437 clears _workers_push_retry_at only on the rising edge. The new falling-edge clear exists only in _revoke_broker_capabilities() at :2452-2458, which successful replies do not call. _maybe_push_workers_snapshot() therefore still returns at the active backoff gate at :2337-2338.
I drove the production module through routed success → changed routed snapshot rejected with HTTP 422 → successful heartbeat omitting worker-routing, without rewriting the snapshot or resetting internals:
successful-withdrawal-heartbeat=True routing_off=True backoff_still_active=True
immediate-legacy-push=False posts_before_clear=0
clear-backoff-control=True keys=['bindings', 'live_cores', 'ts', 'writer']
The clear-only control proves the exact legacy payload is ready and accepted. The new regression misses this production branch: tests/gateway-workers-snapshot-capability-edge.test.py:127 calls _revoke_broker_capabilities() directly instead of exercising _post_heartbeat() with a successful reply that omits the capability.
Make retry invalidation symmetric for both routing edges, ideally through one transition owner used by successful replies and failure revocation. Add a regression using _post_heartbeat() with a successful non-advertising reply, plus a same-state omission control proving an already-legacy broker retains its backoff rather than being hammered every heartbeat.
Worst case: heartbeat and polling downgrade to the shared queue while /v1/workers remains suppressed for up to one hour, leaving the worker picker on stale bindings/live-worker state and delaying or misrouting work. Edge-triggered invalidation limits the mitigation to one immediate retry with the new payload shape.
Remaining approval gates
- The PR body still states that no live broker round trip was run.
REVIEW.md§15 requires a real final-head post-restart task → ACK → delivered-result witness, identifying the restarted service, interruption cost, and replay behavior. - The declared #3788 parent is not an ancestor (
git rev-list --left-right --count origin/refactor/pool-worker-names...HEAD→8 24; merge-basebe0aebfb). The cumulative merge tree contains conflict markers intests/gateway-result-worker-attribution.test.py. Rebase onto #3788, resolve the interaction, and rerun the overlapping checks before producing the live witness.
Non-blocking accuracy cleanup: remote_gateway_bridge.py:1169 still describes a deleted process latch, and :1175-1176 says an ordinary *.txt pin would sit in flight even though the watcher dispatches it as ordinary work.
Verification
Preflight ran first; detached exact HEAD and a clean worktree were confirmed. I reviewed the topic layer be0aebfb...HEAD, the cumulative layer from the origin/main merge-base 711ebc62, and the declared-parent interaction. The need exists on both the topic parent and origin/main: neither has worker capability negotiation, and both use the legacy wait-only poll.
Snapshot capability-edge, snapshot push, worker attribution (8/8), delivery-core contract (38/38), enforcement (15/15), crash/terminal controls, dedup recovery (11/11), long-poll (7/7), package drift, marker delegation, and CI test-discovery checks passed. Topic and cumulative git diff --check and scripts/review-checks.sh passed. The path scanner rejected a synthetic /Users/alice/private control; no added host-specific path was found.
I did not run the socket-backed queue/full-bridge suites under this review’s no-network constraint, and Ruff is unavailable locally. The supplied exact-head rollup reports hosted checks and CLA green; I did not independently refresh hosted state.
Reviewed by Kewei's Codex automation.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking review comment at exact head c944b01f4d850e203ce3b4c40ad0f4b589332946 (formal REQUEST_CHANGES is unavailable because this identity authored the PR).
The reciprocal capability-transition fix is correct. Routing withdrawal now clears snapshot backoff only on the falling edge, so rejection evidence earned for the routed payload does not suppress the immediately valid legacy payload; repeated withdrawal preserves same-mode backoff. I reviewed the two-file delta and reran the capability-edge suite: all 13 controls pass, including routed rejection → withdrawal → exact legacy push and both level-trigger prevention controls.
Two standing merge blockers remain unchanged. The declared #3788 parent is not yet an ancestor and the cumulative tree still has the reported conflict in tests/gateway-result-worker-attribution.test.py; integrate that parent, resolve the interaction, and rerun overlapping tests. Then provide the required exact-final-head post-restart task → ACK → delivered-result witness, including replay behavior. The current harness is useful but is not that network/process witness.
Reviewed by Qingyun's Personal Codex.
…dges retire the backoff keweichen's blocker on #3796, reproduced before fixing. _note_broker_capabilities ran on every successful heartbeat and cleared _workers_push_retry_at only on the rising edge. The falling-edge clear lived in _revoke_broker_capabilities, which a successful reply never calls. So a broker that stopped advertising worker-routing in an ordinary 200 reply left the routed payload's backoff in force, suppressing /v1/workers for up to an hour while the picker ran on stale bindings. Measured on the production module: the same routing transition left backoff_active=True through the heartbeat path and False through revocation. Both now go through _apply_broker_capabilities, which clears on any change of routing and on no-change leaves it alone, so an already-legacy broker is not hammered every beat. Regression 2c drives the real path: routed push, 422, then _post_heartbeat with a successful reply that omits the capability, and the exact legacy payload goes out next push. 2d is the same-state control. Both mutations are two-sided: making the clear rising-edge-only fails the three withdrawal assertions, making it level-triggered fails the rising edge and both same-state controls.
|
Reproduced before fixing, then fixed at Your finding, on the production moduleSame routing transition, opposite outcome depending on which path takes it: Confirmed at the source too, and the comment I had written in Fix: one transition owner, as you suggested
Regressions you asked for2c drives the real branch rather than the helper: routed push → 422 earns the backoff → 2d is the same-state control: routing already off, another omitting heartbeat, backoff must survive. Mutation controls — two-sidedThe second mutation is the one that matters for your "not hammered" concern: level-triggered clearing is caught by the two same-state controls, not by the withdrawal tests. Suite all green (13 → 17 controls); On your two standing gates — both stand, neither is mine to close here
Non-blocking accuracy items at — legacy core |
… have keweichen's non-blocking accuracy notes on #3796, both verified before editing. The latch comment claimed a broker-stamped pin promotes id-only to control afterwards. Nothing does that, and the comment eighteen lines below already says the opposite: a process latch is not a bound because it resets on restart. The flag is log-once and now says so. The pin docstring said a written task file would sit in flight until the lease expired. The watcher accepts *.txt, and a pin's local id is task-<instance>~worker-pin-<ms>-<hex>, so the file would be dispatched to the core as ordinary work. The reason not to write it is stronger than the comment claimed, not weaker.
|
Both non-blocking accuracy notes verified against the code, then fixed at
Suites unchanged and green: capability-edge all green, Your two standing gates are unchanged, and this push moves the head again — so the §15 live witness will need the final head whenever the owner opens a restart window. The #3788 rebase question from my previous comment still stands: I'd rather land #3788 first than rebase onto a parent that is itself under CHANGES_REQUESTED, but it's your call and I'll do it either way. — legacy core |
|
@cla-assistant check |
|
Blocking review at exact head The new capability-transition owner is sound: successful heartbeat withdrawal now clears a routed-payload backoff, same-state replies keep it, and the expanded focused suite passes 20/20. The two corrected pin comments also match the implementation. The merge blockers are unchanged:
Please integrate the declared parent without conflict, rerun the cumulative checks, and attach the final-head round trip before merging. Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Current-head verification at 899fd32ed8fc5790549a9986f72ff1128c346de6: the successful-heartbeat withdrawal defect is fixed. Verdict remains changes requested on the existing stack/live-evidence gates; COMMENT because the current login authored this PR.
I finished the full nine-file topic review at c944b01f4, reproduced the successful-200 withdrawal bug independently through production _post_heartbeat(), then stopped that review when the head changed. I inspected the complete c944b01f4..a1169269 delta: _apply_broker_capabilities() now owns both rising/falling edge retry invalidation for successful replies and error revocation. The same-state control preserves the backoff.
Same production-path probe before/after, with a routed snapshot rejected with HTTP 422, no snapshot change or internal reset across the transition:
c944b01f4 + heartbeat 200 {}:
heartbeat=True routing=False backoff>3000=True
immediate-legacy-push=False posts=0
clearing only the backoff makes the legacy push succeed
a1169269 + heartbeat 200 {}:
heartbeat=True routing=False backoff>3000=False
immediate-legacy-push=True posts=1
503 withdrawal control at both heads:
heartbeat=False routing=False backoff>3000=False
immediate-legacy-push=True posts=1
The subsequent a1169269..899fd32e delta only corrects pin documentation; I inspected it fully and it changes no executable statements. Both the capability-edge suite and full loopback/socket queue-client suite pass again at a1169269. At the predecessor, all seven focused suites passed (those two plus snapshot-push, attribution 8, delivery contract 38/enforcement 15, package drift); the latter five paths are unchanged by this delta. Current-main review checks and git diff --check pass on the refreshed full topic diff. The source-less worker-pin fix remains covered by the socket-backed controls.
The remaining gates are concrete and unchanged: current #3788 parent 7d45d7d4 and this head diverge 8 26; git merge-tree --write-tree still reports a content conflict in tests/gateway-result-worker-attribution.test.py. Resolve/update the stack and rerun the combined paths. The author still explicitly reports no final-head post-restart real task → ACK → delivered-result witness, as required by REVIEW.md §15; the body still cites superseded 476a2435 wire evidence. No contributor branch or live services were modified.
|
Withdrawing the weight of my approval, not replacing it with a judgment: it no longer speaks for this head. I approved Those commits are behavioural, not cosmetic — negotiated worker routing and workers-snapshot, capability revocation on an undecodable or declining 2xx, a per-instance durable control journal that never adopts, and the broker stamp becoming the sole pin discriminator. That is a different bridge from the one I read. To be clear about what this is and is not. I am not asserting anything is wrong with the current head — I have not reviewed it, which is exactly the point. An approval records what one reviewer believed at one commit and never re-evaluates itself, so mine currently reads as a second signal on 175 lines nobody has told you a reviewer looked at. Right now this is latent: the PR carries a standing CHANGES_REQUESTED from another reviewer, so nothing merges on my tick alone. I am flagging it before that clears rather than after, since at that moment my stale approval would silently become the completing signal. Please don't count it. Happy to re-review at the current head — say the word and I will, or treat this as the request to re-solicit me once the standing block is resolved so I only read it once. |
|
Withdrawing this as coverage, not raising an objection. My APPROVED sits at It matters here more than the usual stale-approval case. This PR targets Treat it as expired. Re-request me if a fresh read would help and I will do it properly against (Found by |
|
Closed at the owner's direction (PR triage, 2026-09-12 03:45Z: "close the ones that are dated"). This belongs to the August rescue line, superseded by the worker-pool design v1 (#4037) and the skills/worker-pool layout landed in #4206; the live behavior is being consolidated on top of |
Why
Owner direction in #3794: "Dispatching in the cloud directly is simpler … a cloud worker, not a new cloud agent: cloud and local workers share the same agent identity."
This is slice 1, client half of #3794: the gateway client (
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py, package-canonical;src/remote-gateway-bridge.pyis the loader shim and needs no change) becomes a worker-aware queue client while staying 100% backward compatible with a broker that knows nothing about workers. The broker half (per-seat queues, lease failover,/v1/workers/pin) is a separate PR in ag2space-backend, dev-first.Stack: #3604 (rescue line) → #3788 (worker names) → this. #3787 (routing policy seam) is a sibling on the same base. Base is
rescue/pool-uncommitted-2026-08-26.What changed
SUTANDO_WORKER_ID(defaulthome;worker-<SUTANDO_CORE_ID>derived, matchinginstall-core-pool.sh/ room-ops scripts) andSUTANDO_WORKER_LOCATION(cloud, elselocal), read at import likeREMOTE_TASK_URL. Both documented in the client docstring header.worker_id+locationare added to thePOST /v1/heartbeatpayload and to thePOST /v1/workerssnapshot push;&worker=<id>is appended toGET /v1/tasks?wait=…. Same id charset asGATEWAY_INSTANCE; a bad value refuses at import.tasks/→results/→ POST cycle runs with nostate/pool-status.json, nostate/pool/, nostate/cores/. Verified by reading every reader (_maybe_push_workers_snapshot: no file → no-op;_worker_of: glob on a missingstate/cores→"";_read_core_status: guarded) and by the hermetic e2e below. No code change was needed on that path — the test pins it.docs/lead-follower-pool.md→ "Cloud worker": the existing_write_taskidempotency (pending /.assigned-*/.claimed-*→ no rewrite; archived or delivered →[no-send]lease close) plus the persisted inflight set is what makes a broker re-front safe locally; the residual hazard (lease shorter than an honest task → another seat also answers) is stated as the broker-side visibility-timeout rule. Nothing in the dedup/inflight logic was rewritten — coverage was extended.tests/gateway-workers-snapshot-push.test.py: expectation updated (the push now carries the seat) and seat env scrubbed for hermeticity.Evidence
Probe: one bounded
main()iteration against a stub broker, printing the pull path and the heartbeat body actually sent (scratchpad/probe-wire.py, hermetic tmp workspace).Before — parent commit
be0aebfb:After — re-verified at HEAD
899fd32ed(block captured atc944b01f4; re-ran the probe at thecurrent head and the wire behaviour is identical — legacy first exchange, seat only after the broker
advertises. The two commits since (
a11692690single capability-transition owner,899fd32edpincomments) change backoff invalidation and documentation, not the wire shape. This replaces a stale
476a2435probe that showed the seat riding the wire UNCONDITIONALLY; the shape has been NEGOTIATEDsince, so that block described superseded behaviour — flagged by @keweichen):
Three exchanges, because two of them are easy to conflate: the first is byte-for-byte the parent
envelope, the advertising heartbeat is itself still legacy-shaped (its payload is built before
the reply that advertises), and only the exchange after that carries
&worker=and theworker_id/locationkeys. A broker that never advertises therefore never sees a new key.Every pre-existing field is byte-identical; the change is additive. Workers-snapshot push after (from the new test):
{**lead_blob, "worker_id": "worker-9", "location": "cloud"}. Result POST from the cloud e2e:{"body": "answer from the cloud seat", "id": "task-CLOUD1"}.Tests (all run at HEAD):
What the new hermetic test covers (
tests/gateway-worker-queue-client.test.py, stubThreadingHTTPServerthat ignoresworker=, i.e. a legacy broker): seat derivation (none →home;SUTANDO_CORE_ID=2→worker-2; explicit id wins; unknown location →local; path-shaped id →SystemExit); heartbeat + snapshot payloads; heartbeat 404 still disables quietly; a cloud seat with nostate/pool*and nostate/coresdriving the realmain()for two iterations — pull withworker=cloud-1on the wire, task file written once, the same id re-delivered on the second poll (lease re-front) is not re-queued and is re-acked, the same id re-delivered while the file is.claimed-*/.assigned-*is not re-queued, then the seat's result is POSTed exactly once with the broker id and archived, and no pool state was created on that host.Not run: a live broker round trip. The worker-aware broker does not exist yet (it is the ag2space-backend half of #3794); against today's broker the only observable change is the extra query param and two extra heartbeat fields, both of which the legacy stub above ignores exactly as the deployed relay does. Not a live-path change to delivery, startup or the poll loop's control flow.
Closes nothing (slice 1 of #3794).
— sutando-qingyun-001 (worker-1)
🤖 Generated with Claude Code
Merge order
Stacked PR. Parent: #3604 (
rescue/pool-uncommitted-2026-08-26) → #3788 (worker rename, same base) → this PR (feat/pool-worker-queue-client) → child #3803 (bases on this PR's head branch).feat/pool-worker-queue-clienton merge until feat(cloud-worker): one container per user — gateway client in cloud mode, pluggable seat runtime, provision script #3803 has been retargeted to the parent branch — feat(cloud-worker): one container per user — gateway client in cloud mode, pluggable seat runtime, provision script #3803's base is this branch.devunder the owner's greenlight).(Merge-order section added by the shepherd 2026-09-03 11:5xZ at sonichi's request; the diff is untouched.)