Repository navigation
feat(health): /health names the instance and sessions; the bridge pushes health and names its host stably [HealthStatus] - #4944
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.pyLines 2624-2632 2624 try:
2625 src = _monorepo_src("health_snapshot.py")
2626 if src:
2627 if src not in sys.path:
! 2628 sys.path.insert(0, src)
2629 import health_snapshot
2630 value = health_snapshot.snapshot(_STATE.parent)
2631 except Exception as e: # noqa: BLE001 — health is optional; it never breaks a push
2632 error = f"{type(e).__name__}: {e}"Lines 2674-2686 2674 """The host's stable label: gethostname() drifts with the network (a DHCP lease renames
2675 it). The monorepo src/ is put on the path first, so the label never depends on call order."""
2676 src = _monorepo_src("util_paths.py")
2677 if src and src not in sys.path:
! 2678 sys.path.insert(0, src)
2679 try:
2680 return _stable_host_label()
! 2681 except OSError:
! 2682 return "unknown-host"
2683
2684
2685 def _maybe_push_workers_snapshot(record) -> bool:
2686 """Push-on-change relay of the pool's workers snapshot (the worker
|
host_id came from socket.gethostname(), which on macOS follows the network (Marks-MacBook-Pro became Marks-MBP.localdomain on a DHCP lease), so the same Mac could appear under two names. It now uses workspace_lock._host_label(): Sutando's stable per-host label, falling back to the short hostname when the package runs standalone.
…hes health to the broker /health gains a top-level instance (the agent id the serving gateway lane signed in as, written to its gateway-status sidecar while connected), suspended (the pool-suspended marker), and a per-agent session read from the beat or supervisor file, never guessed. The gateway bridge computes the snapshot in-process, so a Sutando without agent-api still reports: the core's row rides the heartbeat with the worker_health.v1 capability, each worker's row and the suspension ride the workers report, and a health change sends either without waiting.
The broker marks worker rows stale 120 s after the last report it received and asks for one at least every 60 s. The push runs between polls (up to POLL_WAIT + 10 s apart), so a 20 s refresh keeps the gap under 60 s.
8023d1e to
d7777aa
Compare
marklysze
changed the title
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Reviewed at d7777aa. The code looks correct and CI is green. I'm leaving this as a comment and not approving yet, for one reason: Part 1 changes a live path (the heartbeat and the /v1/workers push), and the PR body marks its post-restart round trip as "Pending". REVIEW.md lesson 15 says a live-path change isn't approved on harness proof alone. I'll approve once the round trip is posted.
Blocker
- Live round trip for Part 1. Restart the bridge, show the broker receiving
healthandworker_health.v1, then read the rows back from the owner-only health call. Part 2 (host_id) does show a prod-lane restart, but its value was never read back from the broker.
Verified
- Open and MERGEABLE. Required checks green, CLA passes, diff coverage 98% (the only uncovered line is the
sys.path.insertat bridge :2628). - The new tests really run in CI (lesson 18): the job logs show
gateway-health-push,health-snapshotandtest_agent_profile_host_idexecuting. - On the PR head, all of these pass locally: gateway-health-push (14), health-snapshot (52), test_pool_advertisement_push (including the 20 s cadence), test_agent_profile_host_id, gateway-workers-report-body, agent-health-report, gateway-serving-shared-owner, test_no_drift, gateway-auto-reenroll (31).
src/remote-gateway-bridge.test.pypasses in a clean env. The 5 failures I saw with my own shell env also fail at the merge-base, so they come from my environment, not this PR.scripts/review-checks.shon the diff is clean (hardcoded paths, root artifacts, prose cap).- Ran the snapshot read-only against a live workspace. Core
sessionresolves to the core's tmux session.instancecomes back null, which is expected there: that bridge predates the PR, so it doesn't writeagent_id. - The heartbeat
capabilitieslist is built fresh on each call, so nothing duplicates.
Nits (none block merge)
docs/remote-gateway-protocol.md:188-190still says the profile is pushed "on the same change signal and cadence as the workers snapshot". That's no longer true: workers now re-send every 20 s, and the profile stays at 600 s.remote_gateway_bridge.py:2660sendssuspendedas-is. Whenstate/pool-suspendedisn't JSON,health_snapshot.py:322uses the raw text asreason, with no slug or length clamp on it. The real writer does write JSON, so this is low risk, but it slightly contradicts "only summary fields leave the Mac".health_snapshot.py:310-323duplicates the marker parsing inpool_remedy.py:55-69. That's probably forced by the rule that core can't name the skill, but it can drift.remote_gateway_bridge.py:2724: theexcept OSErroris dead code, since_host_labelalready catches everything./v1/workersPOSTs go from about one per 600 s to about three per minute for every Sutando with a pool, older brokers included. The body names this as a backend requirement, and a failing snapshot fails open.
Not verified: any broker exchange, the desktop and webclient consumers of instance and session, and the case of several serving lanes on one Mac (where instance is deliberately null).
…ence doc, no dead fallback The suspension's reason leaves as a slug and its time only as a number, like every other health field; the profile doc no longer claims the workers' cadence; _host_label cannot raise, so the unknown-host fallback was unreachable.
|
@bassilkhilo-ag2, thanks for the review. Here's the live round trip for Part 1, plus the readback for Part 2, then the nits. Live round trip on a local AG2 Space stackSetup
1. Bridge restart, then the pushes (the bridge's own log on that lane): The workers report goes out about every 25 s, which is the 20 s refresh bounded by the task poll. The profile goes out every 600 s. 2. Owner-only readback from the broker.
3. Fresh rows while the app was running. The desktop's test build showed them live:
Nits, fixed in
|
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Approving at 9e1fd18.
The blocker is cleared. The live round trip (comment 5922000939) covers what I asked for:
- On a local AG2 Space stack, a bridge restart was followed by
workers-snapshotandagent-profilepushes. The workers report went out about every 25 s (the 20 s refresh bounded by the task poll) and the profile every 600 s. - The owner-only
GET /relay/v1/agents/healthread the stored rows back withdetail: "full", the agent id, and Part 2's stablehost_label.
That run used a test merge of an earlier head. The one commit since then (9e1fd189) only touches the suspension row, the protocol doc and a dead fallback, not the push loop, so the round trip still applies.
The nit fixes check out (d7777aa..9e1fd18):
suspendednow goes out as summary fields only._suspended_rowslugsreasonthrough_health_row, falls back to"suspended", and keepsatonly when it's a number. The new test pins a free-text reason with a path in it down to a slug.- The protocol doc now gives the profile's 600 s cadence separately from the workers' 20 s refresh.
- The dead
except OSErrorfallback and its test are gone._stable_host_labelalready catches everything.
CI: 28 checks pass, 2 skipped.
The duplicated marker parsing (health_snapshot vs pool_remedy) is still there. It's fine as a known tradeoff of the core/skill boundary.
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Reviewed at 9e1fd189 against merge-base 0c03031b (10 files, +488/-24, matches the body). Not repeating @bassilkhilo-ag2's or the in-room findings; one blocker, which is this repo's architecture rule rather than a behaviour bug, plus nits.
Blocker
This PR adds a second core-side parser of state/pool-suspended. src/health_snapshot.py _suspended() (new, diff L462-475) and src/health-check.py check_pool_suspended() (main, L8330-8346) run the same policy: read_text().strip() → json.loads → isinstance(rec, dict) → reason/at, with the same "a non-record marker still suspends" fallback. The thread already accepted that core may not import skills/worker-pool/scripts/pool_remedy.py:_marker (the third copy); that boundary argument does not apply here, because both of these live in src/. CLAUDE.md "Fix a bug where the policy lives": no shared owner exists → create one; do not add a copy; that is the fix, not a follow-up.
Ask: a dependency-light src/ reader (for example pool_suspension.read(workspace) -> {reason, at} | None) that health_snapshot._suspended and health-check.check_pool_suspended both call, each keeping its own rendering, with a contract test on the reader and a delegation test on the two callers. Whether pool_remedy._marker then delegates to it too is the worker-pool skill's call (the skill may import src/; core may not import the skill), but the two core copies cannot both ship.
Nits (non-blocking)
remote_gateway_bridge.py:2667_suspended_row:isinstance(True, (int, float))is true in Python, so{"at": true}reaches the wire in a timestamp slot. Verified:_suspended_row({"reason": True, "at": True})→{'reason': 'true', 'at': True}. Low risk, the real writer writes a number.remote_gateway_bridge.py:2643_health_row:.strip("-")[:40]truncates after stripping, so a long reason can end in-.[:40].strip("-")fixes it.- Body says the new suite is 14 tests; it is 15 (
9e1fd189addedtest_a_suspension_leaves_as_a_slug_and_a_numeric_time_only). - The live round trip ran at
8023d1e35;d7777aa4..9e1fd189touches only_suspended_row, the deadexcept OSError, docs and tests, so it still carries, but thesuspendedslug is the one wire field pinned by unit test only.
Verified (worktree at 9e1fd189, all exit 0)
python3 tests/gateway-health-push.test.py Ran 15 tests ... OK
python3 tests/health-snapshot.test.py Ran 52 tests ... OK
python3 packages/ag2-sparrow/tests/test_agent_profile_host_id.py ALL PASS
python3 packages/ag2-sparrow/tests/test_pool_advertisement_push.py ALL PASS
python3 packages/ag2-sparrow/tools/test_no_drift.py PASS
python3 tests/gateway-serving-shared-owner.test.py OK (4 readers delegating)
python3 tests/gateway-workers-report-body.test.py Ran 3 OK
python3 tests/agent-health-report.test.py Ran 12 OK
bash scripts/review-checks.sh --diff <merge-base diff> PASS
ruff check (4 changed .py) All checks passed!
Discrimination: with src/health_snapshot.py and remote_gateway_bridge.py reverted to 0c03031b and the tests kept, gateway-health-push fails (errors=15), health-snapshot fails (errors=7), and the two sparrow suites raise AttributeError. _now = time.monotonic; capabilities is a fresh literal per beat; .alive carries session (core_heartbeat.py:487). The one /Users/mark in added lines is a redaction fixture in the test file; review-checks passes.
Not verified
Any exchange with a real broker (I read the author's round-trip comment, did not reproduce it); the desktop/webclient consumers of instance/session; the backend's [a-z0-9-]{1,40} validator; the multi-lane case where instance is null.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at 9e1fd18. CI and CLA green; hardcoded-path scan clean. On the wire, only alive/motion/condition/reason-slug/since plus a suspended slug and timestamp are added; session, instance and paths stay in the local /health. Tests call production functions. Two small changes requested (both repo review-rule categories), the rest non-blocking:
Requested
- Body claim not backed (false_claim). The body says "an unreadable host still builds a profile (
unknown-host)", buttest_agent_profile_host_id.pyhas two tests and neither covers it, and theexcept OSError → "unknown-host"was removed from_build_agent_profile(remote_gateway_bridge.py~2729). Repro: withutil_pathsunavailable andsocket.gethostnameraising,workspace_lock._host_label()letsOSErrorescape and the profile push fails ("pool advertisement push failed"). Unlikely on macOS, but either restore the fallback with a test, or correct the body. - Duplicated policy that has already drifted (fragmentation).
src/health_snapshot.py:310-325_suspendedre-implementsskills/worker-pool/scripts/pool_remedy.py:56-69_marker: a record with noreasonreadsnullhere but"suspended"in the pool (the bridge's_suspended_rowmasks it on the wire, not in/health). Core can't import the skill, so a small dependency-lightsrc/reader both call would be the shared owner per CLAUDE.md "Shared adapter policy".
Non-blocking
3. The stable label depends on src/ already being on sys.path (a side effect of _health_snapshot/HITL). With gethostname patched to Drift-MBP.localdomain I get Drift-MBP without it and the stable label with it. Normal loop ordering covers it; a disabled heartbeat plus a backed-off workers push does not.
4. WORKERS_REFRESH_S also re-sends the legacy and standalone bodies every 20 s (was ~600 s). Worth stating in the body, or limiting the refresh to bodies that carry health.
Not verified: broker-side handling of worker_health.v1 / its owner-only check (took the cited lines on trust); whether /health binds loopback-only; I did not run the head suites locally.
— Sutando (susans-MacBook-Pro)
health_snapshot, health-check and the pool's remedy each parsed state/pool-suspended, and the copies had drifted (a record without a reason read null in /health, suspended in the pool). src/pool_suspension.read() owns the parse and normalisation; each caller keeps its own rendering, pinned by a contract test and a delegation test per caller.
…health-only refresh - An unreadable host builds a profile with host_id unknown-host again. - The stable label puts the monorepo src/ on the path itself, so call order can't change it. - Only a workers body carrying health refreshes every 20 s; others keep the 600 s re-send. - A reason is truncated before its dashes are stripped; suspended.at excludes bool.
|
Thanks @sonichi and @liususan091219. Both reviews are addressed at
@bassilkhilo-ag2: your approval was at Checks: all the touched suites pass, plus |
|
Read at exact head Both standing blockers are addressed at this head.
One new nit (verified).
One question I could not answer (worst-case disruption). The new Run at this head: health-snapshot 52/52, gateway-health-push 15/15, pool-suspension-shared-owner 8/8, health-check-pool-suspended 6/6, the four worker-pool remedy/suspend suites OK, core-never-names-the-worker-pool-skill OK, gateway-serving-shared-owner OK, gateway-workers-report-body OK, agent-health-report OK, src-map 27/27; sparrow tests run as scripts (pytest not installed here): |
… sanctioned helper state-paths-adoption requires any src/ module that names a runtime-state path to resolve the workspace through workspace_default; read() and path() now accept none.
|
One more commit, |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Re-reviewed at 8a8f5b65; this replaces my CHANGES_REQUESTED from 9e1fd189 (review 5373601397). The duplicated state/pool-suspended parser is gone: src/pool_suspension.py is the one reader and health_snapshot, health-check and pool_remedy delegate to it (verified by reading all three at this head, and by the mutation runs below).
Status of the earlier blocker: FIXED
src/pool_suspension.py is the only definition of the marker path and its parse. grep -rn 'pool-suspended' src/ skills/worker-pool/scripts/ packages/ returns, outside tests, only that file plus two non-parsers: recovery_issues.py:23 (a probe-name list) and health-check.py:8334,8338 (the probe's name). health_snapshot._suspended, health-check.check_pool_suspended and pool_remedy._marker all delegate to pool_suspension.read. The skill imports core; core imports no skill (core-never-names-the-worker-pool-skill ok).
Discrimination, three runs, one caller mutated back to an inline parse each, tree restored to an empty git diff:
FAIL test_health_snapshot_renders_the_readers_record(None != {'reason': 'from-the-reader', 'at': 9})FAIL test_health_check_renders_the_readers_recordFAIL test_the_pools_remedy_reads_through_it
liususan's item: deleting except OSError from _profile_host_id makes test_agent_profile_host_id raise OSError: no host. Reverting fresh = body is base or ... fails test_without_health_the_workers_body_keeps_the_slow_cadence: AssertionError: no health: no 20 s refresh.
Blockers
None.
Nits
- Non-UTF-8 marker, confirmed here.
read()usesread_text(encoding="utf-8")and its docstring promisesOSError, but a bad byte raisesUnicodeDecodeError(isinstance OSError = False), so every caller'sexcept OSErrormisses it./healthhas no guard (agent-api.py:1005, indo_GETwith no enclosingtry), so the endpoint fails whole.health-checkandpool_remedyhave the same gap on main, so only that blast radius is new, and the real writer only writes JSON._read_jsonhere already catches(OSError, ValueError); matching it makes the docstring true. pool_suspension.py:16insertssrc/intosys.pathon every import. Cosmetic.
What I verified
Isolated worktree at 8a8f5b65, every command exit 0:
pool-suspension-shared-owner 9/9; health-snapshot 52/52; gateway-health-push 15/15;
health-check-pool-suspended 6/6; agent-health-report 12/12; workers-report-body 3/3;
gateway-serving-shared-owner ok; core-never-names-...-skill ok; src-map up to date
worker-pool: suspend-resume 13, remedy 27, remedy-timer 25, ensures-supervisor 5
sparrow: agent_profile_host_id, pool_advertisement_push, test_no_drift -> ALL PASS
scripts/review-checks.sh --diff <0c03031b...HEAD> -> PASS (hardcoded-paths + root-artifacts + prose-cap clean). ruff check on the 10 changed .py files -> All checks passed!. No hardcoded host paths added. CI: 25 SUCCESS, 2 SKIPPED, 2 running, 0 failures, license/cla: SUCCESS.
Nit repro: after printf '\xff\xfe\x00bad' > $W/state/pool-suspended, pool_suspension.read, health_snapshot.snapshot and pool_remedy.suspension each raise UnicodeDecodeError.
Live push path. The delta since 9e1fd189 does touch it: _health_row, _suspended_row, the new _profile_host_id, and fresh = in _maybe_push_workers_snapshot, so "the round trip's push loop is unchanged" is imprecise. The changed branches sit off the path the 2026-09-30 round trip witnessed: with health present, that run's configuration, fresh evaluates as before and _profile_host_id returns _stable_host_label() unchanged. The new branches, health-absent cadence and host-label failure, are pinned by the mutations above, so the existing witness carries and I am not asking for a fresh one.
What I did not verify
Any exchange with a real broker, including how one without backend #1582 treats the new fields. No round trip of my own, no desktop/webclient consumer of instance/session, no several-serving-lanes case, and the two in-flight CI jobs. Whether bassilkhilo-ag2's approval at 9e1fd189a carries to the three later commits.
… its files say A quit kills the private tmux server outright, so no seat records its own end and the inbox beat stays fresh: after a relaunch the stopped workers read alive and healthy for up to 90 s with no session at all. The suspension already lists them in stopped.
|
One more commit, Found: right after an app quit and relaunch,
Fix: a worker listed in
Tests:
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at 8e477bb71. I reviewed 8a8f5b65b in full (merge-base 0c03031b), then the one commit since, 8e477bb7, as its own delta.
The new commit, 8e477bb7
A worker named in the suspension's stopped list now reads alive: false, condition: unknown, reason: suspended, whatever its seat files say. This does not turn a failure green:
unknownkeepsoverallatunknown, neverok.- The marker never expires, so a pool that was never resumed is still flagged: by
suspendedin the snapshot and by health-check'spool-suspendedwarn. - Only workers the suspension itself listed are affected.
Revert-control for the delta: with src/health_snapshot.py restored to 8a8f5b65b, test_a_worker_the_suspension_took_down_is_not_alive_whatever_its_files_say fails (53 tests, failures=1). At 8e477bb7 every suite listed below still passes, and review-checks on the new gh pr diff passes.
What I checked against the three asks
- What
/healthexposes. The new local fields areinstance(the serving lane's agent mxid, fromgateway-status*.json), per-agentsession(tmux session names) andsuspended{reason, at}./healthis served byagent-api.py, whose bind defaults to127.0.0.1. The bridge sends onlyalive/motion/condition/reason/sinceper agent (HEALTH_FIELDS), withreasonslugged to[a-z0-9-]{1,40}, plussuspendedas a slug and a numericat.instance,sessionand paths never go on the wire._reenroll_identity()(now written asagent_idin the status file) returns only the mxid, never the token. I found no secret, token or host path leaving the Mac. - Can "pushes health" spam? No; the rate is bounded. I ran the real
_post_heartbeatagainst a stubbedhealth_snapshotwhose coremotionflips on every call, with a simulated clock, for 600 loop iterations at 1 s each. Result: 40 heartbeats in 600 s, minimum gap 15.0 s (main sends 10 atHEARTBEAT_INTERVAL = 60). The bound is theHEALTH_TTL_S = 15cache, because health can only change when the cache refreshes. The workers POST has the same bound: change-gated through the same cache, plus the 20 s refresh, and only for a body that carries health. - Overlap with #4945. Both touch
src/health_snapshot.py. At8e477bb7, a test merge of #4944 + #4945 conflicts textually in two hunks ofsnapshot()and indocs/health-snapshot.md: this PR addssession, and #4945 adds the second argument to_alive. I resolved it by keeping both. Then I merged #4952 on top. On the combined tree these all pass: health-snapshot 59, gateway-health-push 15, pool-suspension-shared-owner 9, core-input-watch 123, health-check-pool-suspended 6, agent-health-report 12, pool-restart 16, pool-remedy 27, pool-suspend-resume 13, test_pool_advertisement_push, and core-never-names-the-worker-pool-skill. The body's merge order (#4944 → #4945 → #4952) works; whichever PR lands second needs that two-line rebase.
Ran at head (clean env, all rc=0)
gateway-health-push 15/15; health-snapshot 53/53 (52 at `8a8f5b65b`); pool-suspension-shared-owner 9/9
health-check-pool-suspended 6/6; agent-health-report 12/12; gateway-workers-report-body 3/3
gateway-serving-shared-owner ok; core-never-names-the-worker-pool-skill ok
sparrow: test_agent_profile_host_id ALL PASS, test_pool_advertisement_push ALL PASS, test_no_drift PASS
worker-pool: pool-suspend-resume 13, pool-remedy 27
gh pr diff 4944 | scripts/review-checks.sh --diff at head: PASS (hardcoded paths, root artifacts and prose cap all clean). gh pr checks: 29 pass, 2 skipping.
Standing reviews
@liususan091219's CHANGES_REQUESTED (filed at 9e1fd189) has not been re-filed at this head, but I checked both of its items here:
unknown-host._profile_host_id()catchesOSErroragain, andtest_agent_profile_host_id(which covers it) passes. Reverting the bridge makes that test fail.- Duplicated
pool-suspendedpolicy.src/pool_suspension.pyis now the one reader.health_snapshot,health-checkandpool_remedydelegate to it, andpool-suspension-shared-owner9/9 pins that.
On that basis I read both items as addressed. @bassilkhilo-ag2's approval at 9e1fd189 is stale by 3 commits.
Revert-control
I reverted only the non-test source (remote_gateway_bridge.py, health_snapshot.py, health-check.py, pool_remedy.py, plus pool_suspension.py removed) to 0c03031b and kept the tests:
- gateway-health-push: errors=15/15
- health-snapshot: errors=7
- pool-suspension-shared-owner: ModuleNotFoundError
- test_agent_profile_host_id and test_pool_advertisement_push: AttributeError
The new tests discriminate. Tree restored to clean.
Findings (none blocking)
src/pool_suspension.py:48. @sonichi's non-UTF-8 nit is still open at this head. I re-ran it: with\xff\xfe\x00badas the marker,pool_suspension.read()andhealth_snapshot.snapshot()both raiseUnicodeDecodeError, so local/healthfails as a whole. The bridge is safe here because_health_snapshotcatchesException. The real writer writes JSON, so this is low risk. A(OSError, ValueError)catch inread()would make its docstring true.remote_gateway_bridge.py:2880and the next few lines._last_core_healthis updated before the POST. If that POST fails, the change is not re-sent until the 60 s interval or the next change. This is benign, but the broker can briefly hold the older verdict.
Live path (lesson 15)
The author's round trip on the local AG2 Space stack (comment of 2026-10-01T00:10Z) ran at 8023d1e35, not this head. I agree with @sonichi's reading of the later delta (_health_row, _suspended_row, _profile_host_id, the fresh = gate). The configuration that run witnessed (health present) takes the same branch, and the new branches are pinned by the mutation-discriminating tests above. So I am accepting that witness, not asking for a new one.
Not verified
- Any broker exchange of my own, or a broker without backend #1582.
- The desktop and webclient consumers of
instanceandsession. - The multi-lane
instance: nullcase, beyond reading_instance.
|
@liususan091219: your review at
Since then, CI is green at |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed at 8e477bb71 (compare from 9e1fd189, 4 commits ahead). This replaces my CHANGES_REQUESTED. Each item was run against the head code and the 9e1fd189 code:
unknown-host: withutil_pathsunavailable andgethostname()raising,9e1fd189raisesOSError; head builds{"host_id": "unknown-host"}.test_an_unreadable_host_still_builds_a_profilefails on the old bridge and passes at head.- Shared reader: a no-
reasonrecord ({"at":5,"stopped":[]}) now readssuspendedin/health,pool_remedy._markerandcheck_pool_suspended. At9e1fd189it wasnullin/healthandNone since 5in health-check. Delegation tests call each caller and assert it went throughpool_suspension.read. - With
src/offsys.pathand a drifting hostname:Drift-MBPbefore,Stable-Labelnow. The new test fails on the old code. - A body without health keeps the 600 s re-send.
test_without_health_the_workers_body_keeps_the_slow_cadencefails on the old code.
Also checked: 8e477bb7 (a stopped worker shows as not alive) has a test covering both cases; pool_remedy's src/ path insertion causes no name collisions; no hardcoded host paths. All changed test files pass locally, CI is green, CLA passes. Not verified: a live broker round trip, and the full PR outside the 4-commit delta.
— Sutando (susans-MacBook-Pro)
|
@qingyun-wu: could you review the What changes under
The live round trip on a local AG2 Space stack is in #4944 (comment). |
# Conflicts: # docs/health-snapshot.md # src/health_snapshot.py
|
@cla-assistant check |
The health feature's Sutando side, consolidated so it can be reviewed once. It covers:
/healthnames theinstanceand each agent'ssession;/healthand the host the broker shows are the same.It absorbs #4891. Merge order: this PR → the verdicts PR → #4952. The desktop then repins its engine to the main that contains all three.
Part 1: instance, session and the bridge health push (was #4944)
What changed, and why
This is the Sutando part of Agent Health in AG2 Space (the HealthStatus work). AG2 Space needs to show an owner every one of their agents and whether each needs them. It also needs to open the right terminal and not list the same Mac twice.
GET /health(src/health_snapshot.py) gains three fields. All of them come from files that already exist:instance: the agent id the serving gateway lane signed in as. The bridge now writesagent_idinto its ownstate/gateway-status[.<lane>].jsonwhile connected. That id is re-read on every write, so a lane switch or re-login shows up at the next poll.nullwhen no lane is serving within 180 s, or when two serving lanes name different ids. A wrong id would show this Mac twice.gateway_serving.verdict_from_record, the shared owner.session(per agent): the tmux session to open..alivebeat, thencore-supervisor.json.core-supervisor.<session>.json.nullwhen no file names one. It is never guessed. Core names no skill.suspended:{reason, at}whilestate/pool-suspendedexists (the pool paused on app quit, feat(worker-pool): the owning host can suspend and resume the pool's remedy #4879). Otherwisenull.Gateway bridge (
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py):_monorepo_src, the same optional-src pattern the HITL projector uses. So a Sutando without agent-api still reports; cloud Sutandos run no agent-api. A standalone sparrow reports no health.health: {alive, motion, condition, reason, since}and theworker_health.v1capability. A change in that row sends the heartbeat at once.health, and the body gains a top-levelsuspended. Health joins the push-on-change signal, so a change re-pushes an unchanged advertisement.POLL_WAIT + 10= 35 s apart, so the gap stays under 60 s.test_an_unchanged_pool_is_resent_on_the_cadence_so_a_restarted_broker_heals.sessionorinstance.reasonis sent as a slug of[a-z0-9-]{1,40}, as agreed with the backend.Review first
src/health_snapshot.py:_instance,_suspended,_session.remote_gateway_bridge.py:_health_snapshot,_with_health, the_maybe_push_workers_snapshotidentity, and_post_heartbeat.docs/health-snapshot.mdanddocs/remote-gateway-protocol.md.Docs
docs/health-snapshot.md: the new fields, plus a "Reaching AG2 Space" section.docs/remote-gateway-protocol.md: the heartbeathealthfield and capability, the workers reporthealth/suspended, and the ≤60 s re-send against the broker's 120 s staleness window.docs/catalog.json:last_verifiedbumped for both.Before / after
The same throwaway workspace for both runs:
@mark-desktop.agent:ag2.spaceagent-apiran on port 7899 from this worktree, withsrc/health_snapshot.pyatorigin/mainand then at HEAD.Before (
origin/main),curl -s http://127.0.0.1:7899/health:{ "checked_at": 1790726855.8, "overall": "attention", "agents": [ { "id": "core", "role": "core", "label": null, "alive": true, "motion": "idle", "condition": "healthy", "reason": null, "since": null }, { "id": "40659240fd884f63bcd19fa684b451f1", "role": "worker", "label": null, "alive": true, "motion": "idle", "condition": "abnormal", "reason": "needs-login", "since": 1790726847.0308847 } ] }After (HEAD):
{ "checked_at": 1790726872.1, "instance": "@mark-desktop.agent:ag2.space", "overall": "attention", "suspended": { "reason": "app-quit", "at": 1790726847 }, "agents": [ { "id": "core", "role": "core", "label": null, "session": "sutando-core", "alive": true, "motion": "idle", "condition": "healthy", "reason": null, "since": null }, { "id": "40659240fd884f63bcd19fa684b451f1", "role": "worker", "label": null, "session": "sutando-worker-40659240fd884f63bcd19fa684b451f1", "alive": true, "motion": "idle", "condition": "abnormal", "reason": "needs-login", "since": 1790726847.0308847 } ] }What the bridge would POST, captured with
_reqstubbed and nothing sent. Same workspace, bridge atorigin/mainand then at HEAD:Before:
After:
Tests
Live round trip
Done. Bridge restarted on a local AG2 Space stack (broker = ag2space-backend #1582); the broker received
healthandworker_health.v1(detail: "full") and the owner-onlyGET /relay/v1/agents/healthread the rows back, including Part 2'shost_label. Evidence: #4944 (comment)Related
/healthreader, Open terminal viasessioninstance/health) and feat(worker-pool): the owning host can suspend and resume the pool's remedy #4879 (pool suspend/resume). fix(gateway): the agent profile names its host by the stable label [HealthStatus] #4891 (stablehost_id) is independent.Part 2: the stable host id in the agent profile (was #4891)
What changed, and why
The bridge's agent profile (
PUT /v1/agents/<mxid>/profile) names the Mac it runs on inhost.host_id. That came fromsocket.gethostname().split(".")[0], which on macOS follows the network: on the owner's Mac todaygethostname()isMarks-MBP.localdomain(a DHCP lease), while the stable name isMarks-MacBook-Pro. So the same Mac could be published under two names as it moves between networks.host_idnow comes fromworkspace_lock._host_label(), the helper the package already uses for its lock records:util_paths._host_label(), the stable per-host label everystate/cores/<host>.aliveandhosts/<host>/path is keyed on ($SUTANDO_HOST_LABEL, thenscutil --get LocalHostName, then the short hostname);The planned owner-facing agent-health panel labels each Sutando with this
host_id, so a label that changes with the network would show one Mac as two.Where to look
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py_build_agent_profile()(one call changed, one import);packages/ag2-sparrow/tests/test_agent_profile_host_id.py; the new test wired into.github/workflows/ci.yml's out-of-tree list.Privacy
Unchanged in scope:
hostis already sent today. Per the broker (origin/devapi/v1.py:get_agent_profile:2189 →_owner_of_caller:1822, owner ceilinghost:"owner":2023), only bearers of agents with the same owner can read it; room members and other users cannot, and there is no room-state copy.Docs
N/A: no documented behaviour changes; the field's meaning (which Mac) is the same, only its stability improves.
Before / after (on the owner's Mac, real hostname)
Live path: post-restart round trip (desktop app, prod lane)
The desktop app's engine checkout was switched to a branch containing this commit, and only the gateway bridge was restarted (the app's keepalive respawned it):
Not done: reading the profile back from the prod broker. The bridge's bearer isn't in its process environment, and pulling it from the keychain for a readback wasn't worth it for this change. The pushed payload is the one
_build_agent_profile()builds, shown above.Unrelated, pre-existing: the log also shows
hitl projection error (isolated): HTTP Error 502on the prod lane, 464 times since 2026-09-28T20:19Z, before and after the restart. It is not caused by this change and is being raised separately.Tests
Cases: the stable label wins over a drifting
gethostname(); the stable label does not depend onsrc/already being onsys.path; an unreadable host (noutil_pathsandgethostname()raising) still builds a profile (unknown-host); standalone (noutil_paths) falls back to the short hostname.Risks / rollback
On hosts where the stable label differs from the short hostname, the broker sees a new
host_idonce, on the next profile push. Nothing keys onhost_idtoday (it is a display field). Rollback = revert.Review follow-ups (round 2)
26883d0b1: one reader forstate/pool-suspended(sonichi blocker, liususan #2)src/pool_suspension.py.read(workspace)returns{reason, at, stopped}normalised, orNonewhen the pool isn't suspended:reasondefaults tosuspended;atis a number only, never a bool;stoppedkeeps only strings;readraisesOSError, and each caller decides what that means.health_snapshot._suspended,health-check.check_pool_suspendedandpool_remedy._markerall delegate to it, and each keeps its own rendering. The pool is a skill, so it may importsrc/; core still names no skill.tests/pool-suspension-shared-owner.test.py(8 tests): a contract test of the reader, plus a delegation test per caller.reasonnow readssuspendedeverywhere, notnullin/health.693c811cd: bridgehost_id: "unknown-host"again. New test:util_pathsunavailable plusgethostname()raising._profile_host_id()puts the monoreposrc/onsys.pathitself before reading the stable label. New test: withsrc/removed fromsys.pathandgethostname()drifting, the profile still gets the stable label. The test fails without the fix.test_without_health_the_workers_body_keeps_the_slow_cadence, anddocs/remote-gateway-protocol.mdis updated._suspended_rowdrops a boolat._health_rowtruncates before stripping dashes, so a reason never ends in-.gateway-health-push: 15).8a8f5b65b:pool_suspension.path()andread()takeworkspace=Noneand resolve it throughworkspace_default.resolve_workspace. CI'sstate-paths-adoptiongate requires this of anysrc/module that names a runtime-state path.Checks:
state-paths-adoption,pool-suspension-shared-owner,health-check-pool-suspended,pool-suspend-resume,health-snapshot(52),gateway-health-push(15),test_agent_profile_host_id,test_pool_advertisement_push,test_no_drift,gateway-workers-report-body,agent-health-report,gateway-serving-shared-owner,core-never-names-the-worker-pool-skill,src-map.review-checks.shanddocs_auditpass, andruffis clean.