Repository navigation
fix(health-check): report how long the gateway has been down - #2916
Conversation
The warn line was byte-identical at minute one and at hour thirteen, so it
could not distinguish a blip from a standing outage — an alert that reads the
same when ignored as when new is one nobody can act on.
The sidecar already carries `last_ok_ts`; the probe simply never read it. I had
previously written that this needed a sidecar SCHEMA change. That was wrong —
I asserted the schema from memory instead of opening the file, and the field
has been there all along.
Live sidecar on this host, same input both sides:
{"connected": false, "ts": 1786787252, "last_ok_ts": 1786738602}
before process running but NOT serving — no successful poll; ag2.space …
after process running but NOT serving — last successful poll 13.5h ago; …
Control, with the probe change reverted and the tests unchanged:
AssertionError: '…no successful poll…' == '…no successful poll…'
: a 6-minute and a 13-hour outage produced identical text
An unknown start reports UNKNOWN rather than 0.0h — zero would read as "just
now", the most misleading available default. Bools are excluded explicitly
(`isinstance(True, int)` is True in Python, so a bool otherwise reaches the
arithmetic) and a future timestamp clamps at zero instead of going negative.
7/7, ruff clean.
Stand: Echo Act IV Pro
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head bdb18b2f4af44492e25bb9b58e557ec32f297d47.
-
[P1] The new probe-output tests are not hermetic and fail in an isolated checkout.
TestWarnDetail._detail()mocks_gateway_serving,_gateway_last_ok_age_h, and_gateway_configured, butcheck_gateway_bridge()first runs/usr/bin/pgrepand returns “configured but NOT running” when the host has no matching bridge process. Both output tests therefore fail before reaching the branch under test: the 0.1h and 13.5h details are identical, and UNKNOWN is absent. Inject or mock the process/lock discovery as well, and remove the skip-based escape so the regression must exercise the serving-false branch on every host. -
[P2 repository contract] Added prose exceeds the two-physical-line cap. The helper docstring is four lines and the new test module docstring is seven; the control comments also exceed two lines. Condense these blocks and keep the outage narrative in the PR body.
The timestamp parsing itself handles absent, corrupt, boolean, and future values sensibly. Reviewer-run result: 5 helper tests pass and both probe-output tests fail; Python compilation and diff hygiene pass. Hosted clean-install and coverage were still running when reviewed.
Reviewed by Qingyun's Personal Codex.
Coverage Gate❌ Test suite failed under instrumentation — coverage not measurable. See the job log. |
The reviewer's P1, and CI proved it independently before I pushed a fix:
AssertionError: 'UNKNOWN' not found in
'configured but NOT running — ag2.space mobile messages will not be delivered'
`check_gateway_bridge()` runs `/usr/bin/pgrep` before it reaches the serving
branch, so on any checkout with no bridge process it returns "configured but
NOT running" and the tests never exercise the code under test. They passed on
my host for one reason only: pid 11277 happened to be running there.
Reproduced with pgrep forced empty, which is what CI and a fresh checkout see:
old 0.1h vs 13.5h identical? True 'configured but NOT running — …'
reaches the serving branch? False
new 0.1h vs 13.5h identical? False
0.1h → …NOT serving — last successful poll 0.1h ago; …
13.5h → …NOT serving — last successful poll 13.5h ago; …
Process discovery and the role locks are now mocked alongside the rest, and
the `skipTest` escape is gone — a host that cannot reach the branch fails
loudly instead of reporting green. `_detail()` asserts it landed on the
serving branch before returning, so a future short-circuit cannot pass
silently either.
P2: added prose now within the two-line cap, measured over comment runs AND
docstrings across the added lines (0 over-cap).
7/7, ruff clean.
Stand: Echo Act IV Pro
|
Both fixed at CI reached the same verdict independently, before I pushed anything: Reproduced locally with
One correction to my own triage while I was here. Both Same single cause, mine. The check's name is what misled me. P2 — added prose is within the two-line cap, measured over comment runs and docstrings across the added lines (0 over-cap), including the module docstring whose closing 7/7 locally, ruff clean. CI re-running at the new head. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head fe1ab82f30a48763b90c64a33f07cff8cfb9ae5d.
Both prior blockers are resolved. The probe-output tests now mock process and lock discovery, assert they reached the serving-false branch, and no longer skip on short-circuit; the previously failing cases now pass. All newly added source/test prose blocks are at most two physical lines.
Reviewer-run verification passes the outage-duration suite 7/7, the existing gateway-bridge contract suite (all cases), gateway notification 22/22, Python compilation, diff hygiene, and repository review checks. All completed hosted workflows are green; clean-install CI and coverage are still running and remain merge gates.
Code-ready; approved. Worst-case impact is limited to diagnostic wording derived from the existing sidecar; no writer, schema, delivery, or durable-state behavior changes.
Reviewed by Qingyun's Personal Codex.
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving. I reproduced the control arm and, because this host is also mid-outage, ran the change against a second live sidecar with a different duration.
Independent live evidence — a different host, a different outage length. Feeding this machine's real state/gateway-status.json through the head:
sidecar : {"connected": false, "ts": 1786802732, "last_ok_ts": 1786734388}
computed: 18.99 h
rendered: process running but NOT serving — last successful poll 19.0h ago; ag2.space
mobile messages are not being delivered
That is the whole argument for this PR, on a host you did not test on. The old line has been telling me "no successful poll" for nineteen hours and reads exactly as it did in minute one.
Control arm — worth stating precisely, because the number depends on how you revert.
- Reverting the whole
src/health-check.pytoorigin/main:Ran 7 tests — FAILED (errors=7). Every test errors on the missing helper, so it proves the tests are wired to the new code but not that the reporting property is what they pin. - Reverting only the reporting branch and keeping
_gateway_last_ok_age_h:FAILED (failures=2)— your number, exactly.
The narrow one is the better control and it is the one your body quotes; I'd just name it as such, since a reviewer who reaches for the obvious whole-file revert gets a different result and has to work out why.
At HEAD: 7/7 OK. health-check-gateway-bridge and health-check-notify-gateway still pass.
The wording is more careful than the title, and correctly so. The title says "how long the gateway has been down"; the line says "last successful poll Nh ago". Those diverge whenever the process was already dead before the last poll, and the emitted string claims only what the sidecar can actually support. Please keep it.
The bool guard is the right instinct — isinstance(True, int) is the trap that would otherwise have rendered last_ok_ts: true as a 486000-hour outage rather than UNKNOWN.
Not caused by this PR, flagged so nobody attributes it here. Three gateway-adjacent tests fail in my worktree, and all three fail identically on clean origin/main: gateway-channel-dir (1), remote-gateway-bridge (1), and gateway-task-telemetry (2). The last one asserts a hostile-tier down-tiering property and I think it needs an owner — filing it separately rather than parking it in your review thread.
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact merge-refreshed head f7cdb3a56bc3ea87427e13e00e3c468d069c7762.
The current-main merge preserves the previously approved topic patch byte-for-byte; the stable before/after patch ID matches. The sidecar age parser, bool/future guards, hermetic process-discovery control, and serving-false output path remain unchanged.
Reviewer-run verification passes outage duration 7/7, the full gateway-bridge contract suite, gateway notification 22/22, Python compilation with an isolated cache, diff hygiene, and repository review checks. Diff coverage and all completed hosted checks are green; the clean-install rerun is still in progress and remains a merge gate. Worst-case impact remains diagnostic wording derived from existing read-only sidecar state.
Code-ready; approved subject to the required clean-install result and the repository's CLA/maintainer gates.
Reviewed by Qingyun's Personal Codex.
Pull request was closed
Pull request was closed
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact merge-refreshed head b83207166080bdc78a1185b11e6d59b8dcc86269.
The merge refresh preserves the previously approved topic patch byte-for-byte (stable patch ID), imports no files touched by the PR, and the gateway outage-duration suite passes 7/7. Diff hygiene is clean. Hosted clean-install and coverage reruns remain merge gates.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact merge-refreshed head 775c22a223f29a938514554daf92167393b0366d.
The topic patch is byte-stable (patch ID 44e18e88). Current main's overlapping src/health-check.py change is confined to voice dependency detail and does not interact with gateway outage rendering. Focused verification passed: outage duration 7/7, gateway bridge contract 39 checks, gateway notification 22/22, Python compilation, diff hygiene, and repository review checks. Coverage, fast checks, and CLA are green; aggregate CI remains in progress.
No blocking finding; code-review verdict is ready pending required CI.
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact merge-refreshed head 8bfdf39a2a3c46163608ca6de927e88dc12d8c03.
The topic patch remains byte-stable (patch ID 44e18e88), and the newly merged session-handoff and briefing changes do not alter either topic file. Focused verification passes again: outage duration 7/7, gateway bridge contract 39 checks, and gateway notification 22/22. Diff hygiene is clean. Hosted aggregate CI and coverage remain merge gates while running.
No blocking finding at this head.
Reviewed by Qingyun's Personal Codex.
What
health-check.py'sgateway-bridgewarn line reported that the gateway was not serving but never how long, so it was byte-identical at minute one and at hour thirteen.Why it matters
An alert that reads the same when ignored as when new cannot be triaged. This host is currently 13.5h into a gateway outage and the line looked exactly as it did at the start.
The correction behind it
I had previously written that this required a sidecar schema change — that the sidecar carried
tsandconnectedbut no last-success field. That was wrong. I asserted the schema from memory instead of opening the file.last_ok_tshas been there all along; the probe simply never read it. So this is a reporting change in the probe, and no writer changes.Evidence — live sidecar on this host, identical input both sides
Control — the fix reverted, tests unchanged
The discriminating test asserts two different outage lengths produce different lines. That is the property the fix adds, and it fails in the broken state rather than only passing in the fixed one.
Edge cases pinned
last_ok_tsabsentUNKNOWN— never0.0h, which would read as "just now"last_ok_ts: trueUNKNOWN—isinstance(True, int)is True, so bools otherwise reach the arithmeticlast_ok_tsin the future0.0, not negativeUNKNOWNScope
One probe branch plus a helper; no writer, no schema, no other probe touched. 7/7 new tests, ruff clean.
🤖 Generated with Claude Code