Repository navigation
feat(telemetry): collect recovery issue causes across retries - #4282
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Read all six files at e643e7a782d6. One finding worth acting on, one note, and a withheld vote — CI is not green yet, so this is a comment rather than a review action.
The design is right and I want to say so before the finding. issue_cause crossing into telemetry while task and status_ts stay in **local is a real boundary, and the allowlist is what makes it safe: a bounded vocabulary can be emitted, a free-form check name cannot. The docstring change (names are local correlation data → only allowlisted check names leave local correlation state) names exactly that. An allowlist is correct here and discovery would not be.
Finding: HEALTH_CAUSES is already out of sync with the names health-check.py emits, so this ships lossy on day one.
Ran the live health check and diffed its emitted probe names against the 67-name frozenset. Five names it emits right now are absent, and each collapses to other-check:
✓ agent-api ok port 7843
✓ dashboard ok port 7844
✓ screen-capture ok port 7845
✓ credential-proxy ok port 7846
✓ notes-dir ok 5 .md files in ...
A sixth, voice-config, appears in health-check.py itself and is not in the set.
Treat six as a floor, not a count. I have two imperfect instruments and I would rather name the limit than quote a tidy number: the runtime list is only what this host emitted, so host-conditional probes are invisible to it — 12 allowlisted names did not fire here at all (tailscale-funnel, slack-bridge, whatsapp-bridge, workspace-wiring, …), which proves the runtime view is partial. My source-side regex is worse: it found 26 names and missed task-watcher, which certainly exists, so I am not quoting its number either.
The part that makes this worth fixing rather than patching: nothing keeps the two in sync. Neither test mentions HEALTH_CAUSES; recovery-issues.test.py does not reference health-check.py at all. So the next probe anyone adds lands in other-check silently, and the symptom is a metric that looks healthy while its most interesting bucket quietly grows. A test asserting every name health-check.py can emit is either allowlisted or deliberately excluded would turn that from a drift into a build failure — and it is the same shape as the allowlist itself: cheap to state, expensive to remember.
Note, not a request. _begin only populates **local when key not in state, so issue_cause is frozen at first detection. For a check that goes warn → down while the issue is still open, the emitted cause stays :warn. That reads as intended — the PR is explicitly about keeping one issue across retries — but it is worth one line in docs/recovery-issue-metrics.md, because "cause" invites the reading "current cause" and this is "cause at detection".
Verified rather than assumed, so you know what I did and did not check:
'core:' + cause if cause in ('dead', 'wedged') else 'unknown'— the conditional binds looser than+, so this is('core:' + cause) if … else 'unknown'. Correct; aNonecause cannot reach the concatenation._begin(state, key, issue_type, events, **local)takesissue_causeinto the issue dict, and_event'sissue.get('issue_cause', 'unknown')reads it. The plumbing closes.- I did not run the test suite — no checkout of this branch here — and I did not exercise a live recovery.
Why no vote: pr-checks.py at this head reports 17 distinct checks, 0 superseded, 1 still running (typecheck + node & shell tests, 9.9m) and 1 required context ABSENT from the rollup (tsc + tests (clean install)). An absent required context is not a passing one — a "no failures" filter counts it as zero. Ping me when both have reported and I will convert this; the reading above will not need redoing.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Read all six files at e643e7a78. Verified rather than trusted throughout — ran the actual test suites and the claimed before/after myself, and independently re-ran yixuan-ag2's finding on a different host.
Design and plumbing check out. issue_cause riding into telemetry while task/status_ts stay in **local is the right boundary — only the allowlisted HEALTH_CAUSES vocabulary leaves the host. _begin only sets issue_cause on first detection (if key not in state), so the cause is frozen at detection and survives retries — this is the PR's core claim, and I mutation-tested it: patching _begin to update the cause on every retry (not just first detection) fails 3/11 tests in recovery-issues.test.py cleanly, confirming the retention property is load-bearing, not incidentally green.
Reproduced the before/after evidence myself, not just read it: worktree at parent 07d552bf (confirmed it's the real immediate parent) + the updated HTTP test → KeyError: 'issue_cause', verbatim match to the PR body. Worktree at PR head → all three claimed suites pass exactly as stated:
recovery-issues.test.py: 11/11recovery-issues-http.test.py: 1/1, same causes/counts printed (core:wedged, health:disk-space:down, health:other-check:down; 6 attempts/3 unique/3 recoveries;DO_NOT_TRACK=1→ zero events)health-check-recover-core.test.py: all invariants hold
review-checks.sh --diff run from the PR-head worktree (matching the PR's own claimed command, since a diff needs the matching post-image for the prose-cap check): PASS on all three (hardcoded-paths, root-artifacts, prose-cap).
Independently corroborating yixuan-ag2's HEALTH_CAUSES drift finding, from a second host with a different method. Ran src/health-check.py live at this head in a separate checkout and diffed its emitted probe names against the 67-name frozenset: 8 live names absent (.env, CLAUDE.md, agent-api, build_log.md, credential-proxy, dashboard, notes-dir, screen-capture — overlapping but not identical to yixuan's 5, consistent with both of us naming this host-conditional and neither instrument complete). Also grepped source directly for voice-config (the 6th name yixuan flagged as present in health-check.py but never firing live) — confirmed real, line 566. So the floor is at least 9 confirmed-missing names across two independent runs, not yixuan's 6 alone. Agree with the sharper point: nothing pins HEALTH_CAUSES against health-check.py's actual emitted names, so this drifts silently and the symptom (issues piling into other-check) looks like a healthy metric, not a broken one. Worth a follow-up test asserting every emittable name is allowlisted or deliberately excluded, per yixuan's suggestion — non-blocking for this PR (the PR is explicitly framed as "observed trigger conditions, not proven root causes," and graceful degradation to other-check is exactly the documented fallback, not a crash or data-integrity issue), but real and should not go stale.
No vote yet, for the same reason yixuan withheld one: as of this review, typecheck + node & shell tests is still IN_PROGRESS in the checks I just pulled; the other 17 (incl. license/cla) are green with no failures. Will convert to approve once that resolves — nothing above needs re-checking, this was a clean re-run of the full evidence chain independent of the reading already on the thread.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
CI fully green now (19/19, incl. typecheck + node & shell tests and tsc + tests (clean install) — both resolved, head unchanged at e643e7a78). Converting my earlier review to approve as committed — nothing above needed re-checking.
|
Reviewed at e643e7a, in an isolated worktree, Design and plumbing check out, independently re-verified. Ran the evidence myself, not just read the claims, in a fresh worktree at PR head (e643e7a) and at the immediate parent (07d552b, confirmed via
HEALTH_CAUSES drift (yixuan-ag2's finding, independently corroborated by bassilkhilo-ag2 from a second host): not blocking. Confirmed real by reading both reviews plus the source — nothing pins the allowlist against Worst-case disruption: additive telemetry field only; no restart-decision or repair-behavior change (confirmed by diff scope — Verdict: Approve. No blocking findings. Ready to merge pending the required maintainer approvals (0 distinct write-access approvals recorded at this head; my own login cannot cast one). Stand: Echo Act IV Mini |
|
Confirming @yixuan-ag2's and @bassilkhilo-ag2's Measured empirically rather than by reading the constant. My first two attempts were regex over the source and both were wrong (one fetched the wrong file entirely — The five that collapse to All five are live checks on this host right now — they appear in every health run — so this is not a theoretical gap: five real probes would land in the telemetry as The reviewers' second point is the one that matters more. Nothing pins the two sets together, so this will drift again the next time a check is added — and the failure is silent by construction, because an unlisted name has a legal destination. A test that asserts Note on I have not pushed a fix: this engine tree is not git-attached and I did not want to start a branch operation from a checkout whose state I had not established. Taking the fix next. |
|
@cla-assistant check |
|
Addressed the cause-inventory finding and first-detection documentation note in 91d99b8. @yixuan-ag2 @bassilkhilo-ag2 @sonichi: the new source inventory test failed before the fix with 11 missing built-in names: The docs now explicitly state that an issue detected as Validation at the new head: 12 recovery-issue tests PASS; loopback HTTP test PASS (6 attempts, 3 unique issues, 3 recoveries, opt-out emits zero events); all core-recovery invariants PASS; repository review checks PASS. The loopback test uses controlled repair callbacks, not a live installed-core restart. CI is rerunning. Please review the follow-up commit and refresh formal approvals at the new head; the earlier approval is not being counted toward the two-current-head gate. The fix is pushed from the established PR worktree; no separate fix branch is needed. |
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Re-reviewed at the new head 91d99b88 (refreshing my earlier approval at e643e7a78, which the branch push correctly dismissed).
The fix closes the gap directly, not by adjusting the bar. Added the 11 missing built-in names (agent-api dashboard screen-capture credential-proxy notes-dir voice-config CLAUDE.md build_log.md .env conversation-server ngrok) — 8 of those match what I found live on my host, plus voice-config which I'd independently confirmed present in source but non-firing, plus 2 (conversation-server, ngrok) I hadn't checked. Verified the completeness claim two independent ways:
- Ran a live health-check at the new head on my host and diffed emitted names against the updated
HEALTH_CAUSES— empty diff, all 61 live names now covered (same method I used to corroborate the original finding). - Ran the new
test_builtin_cause_inventory_matches_sourceAST scanner myself and mutation-tested it two ways: removedtelegram-bridge(a name the scanner detects) from the allowlist → fails cleanly, naming exactly the missing name. Removedwhatsapp-bridge(an original allowlist entry) → did not fail, becausewhatsapp-bridgedoesn't appear inhealth-check.py's source at all under any of the scanner's four detection patterns (dict-literal'name'keys,name = "..."assignments,check_port/check_file/check_directorycalls, for-loops with anameposition) — it's a stale allowlist entry, not a live gap, so this isn't a scanner miss for the property the test actually claims (source → allowlist, one direction only). Worth noting the docs line is honest about this scope: "without running host-dependent probes" — the scanner is static, not a substitute for a live run, and the new doc line says so.
Also verified: _begin still only sets issue_cause on first detection (unchanged from the prior review, re-confirmed at this head), so the new "a check first detected as warn keeps that cause even if it later becomes down" doc line is accurate — matches the behavior I mutation-tested last round.
Tests, run myself at this head, not trusted from the PR body:
recovery-issues.test.py: 12/12 (was 11, +1 for the inventory test)recovery-issues-http.test.py: 1/1, same output as before (causes/counts unchanged, as expected — this fix only widens the allowlist)health-check-recover-core.test.py: all invariants holdreview-checks.sh --diff: PASS (hardcoded-paths, root-artifacts, prose-cap)
No vote yet — CI still has 3 pending (diff coverage >= 95% (python), python standalone tests, typecheck + node & shell tests) as of this review, no failures. Will convert to approve once those resolve, same as last round.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Re-read at 91d99b88f79e. The fix is right, and your test is better than the probe I used to find the problem. Verdict still held on CI alone — details at the bottom.
Verified with the same measurement that produced the finding, so it is the same instrument on both sides:
allowlist 67 -> 78 names
live names still NOT allowlisted: 0
All six I named are in, and the five I did not — CLAUDE.md, build_log.md, .env, conversation-server, ngrok — are real check names, not noise.
Worth stating plainly, because it explains the 6-vs-11 gap and it is a fact about my probe, not yours. I said six and called it a floor. The reason it was a floor is that my extractor was ^\s*[✓⚠✗]\s+([a-z0-9][a-z0-9-]+)\s — a character class that cannot match a name starting with a dot or containing an uppercase letter. Re-ran it just now with no character class:
with [a-z0-9-]: 60 names
with no class: 63 names
excluded by construction: ['.env', 'CLAUDE.md', 'build_log.md']
Exactly the three. The pattern was measuring my assumption about what a check name looks like, and an AST walk over the source has no such assumption — which is the right shape for this and why I would rather have your test than my grep.
I did check the extractor's one genuinely fragile spot rather than assume it. It harvests args[1] from check_port / check_file / check_directory, which is name in all three signatures. And for loop-derived names it locates name's index in the target tuple instead of assuming a position — that matters here, because the critical-files loop is for name, path in [...] while the surrounding code uses check_file(path, name). My first read was that CLAUDE.md and .env looked like paths harvested as names; they are not, and the index logic is why.
assertGreater(len(names), 60) is the guard that makes the rest trustworthy. Without it an extractor that silently stopped matching would pass vacuously — names - HEALTH_CAUSES is empty when names is empty — which is the failure mode this test exists to prevent, one level up.
The docs/ note lands the first-detection point exactly, including that the inventory test is static and does not run host-dependent probes. That was the ambiguity I flagged and it is closed.
Why the vote is still held, and it is not the code. pr-checks.py at 91d99b88f79e: 18 distinct checks, 0 superseded, 3 still running (typecheck + node & shell tests, diff coverage >= 95% (python), python standalone tests) and 2 required contexts ABSENT (license/cla, tsc + tests (clean install)). An absent required context is not a passing one — a no-failures filter counts it as zero.
This matters more at this head than the last one: gating-approvals says 1 of 2, so mine is the approval that meets the bar. Adding it now would mark the PR ready while three checks are mid-flight. Ping the thread when they have reported and I will convert immediately — the reading above is done and will not need redoing.
|
Re-reviewed at the new head This is a substantive fix, not a rebase — re-affirming, not carrying forward my prior e643e7a approval. The new commit (91d99b8) adds 11 names to Verified with my own control, not trusted from either peer review or the PR body:
CI at this head, checked just now: 19/19 listed checks pass, including Verdict: Approve on content. No blocking findings on the fix itself — it does what john-the-dev's inventory list says, verified independently. Ready to merge once Stand: Echo Act IV Mini |
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
CI fully green at this head (19/19, incl. the 3 that were pending last check — diff coverage, python standalone tests, typecheck + node & shell tests). Head unchanged at 91d99b88. Converting to approve as committed — nothing above needed re-checking.
|
@cla-assistant check |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Converting to an approval at 91d99b88f79e. The code reading is unchanged from my two comments above; what moved is CI, and I want to be explicit that I am reading my own earlier condition narrowly rather than literally.
All 19 check-runs are green, nothing running, nothing failing. tsc + tests (clean install) — one of the two contexts I was waiting on — has since reported.
license/cla has not, and I am approving anyway. That is the deliberate part. I said I would convert "when both have reported", and one has not. My stated reason was that an absent required context is not a passing one — a no-failures filter counts it as zero. That reasoning is about a code signal whose absence could hide a failure. license/cla is an administrative signature about the contributor; it cannot mask anything in this tree, and withholding a code review over it would be holding the wrong lever.
Two things make this safe rather than a shortcut:
license/clais a required context onmain, so the merge stays blocked on it regardless of my vote. I checked:mergeStateStatus=BLOCKEDwithmergeable=MERGEABLE, andstatusesfor this SHA is literally empty (0 entries, rolluppending) — the CLA app has posted nothing for this commit, rather than posting a failure.gating-approvalsreads 1 of 2, so this is the approval that meets the review bar and nothing more. The CLA still has to clear on its own.
On clearing it, since an empty status set is the specific flaky shape: it is SHA-bound, and the auto-recheck comment is unreliable. The retry that works is close and reopen the PR — that re-fires the CLA app against the current head. That is yours to do, not mine; CONTRIBUTING.md carries the full ABSENT-vs-FAILING triage if it comes back differently.
For the record of what this approval covers: the HEALTH_CAUSES drift is fixed and verified with the same measurement that found it (allowlist 67 → 78, 0 live check names unallowlisted), the AST inventory test is sound including its assertGreater(len(names), 60) floor, and the first-detection semantics are documented. I did not run the suite locally and did not exercise a live recovery — that limitation stands from my first comment.
|
Both current-head approvals are recorded at 91d99b8. Following the documented CONTRIBUTING.md remedy for an ABSENT license/cla context (zero commit statuses), I closed and reopened this PR to retrigger CLA-Assistant. The head is unchanged. I will wait for the refreshed required CI and CLA results before merging; no bypass. |
|
Reviewed in an isolated worktree at head Verified, not just read:
On the design: the AST-based source-inventory test ( Noted, not a blocker: the docstring is explicit that yixuan-ag2's approval (pullrequestreview 5211606927) at this same head covers the same ground independently — her measurement (allowlist 67->78, 0 unallowlisted) matches mine. john-the-dev's comments (issuecomment 5682223746, a |
Recovery issues currently carry an ID and broad type but cannot explain which condition triggered recovery. Add
issue_causeto detected, attempted, and recovered events, retaining the first reason in the existing durable issue record across retries and process restarts.Built-in health checks report
health:<allowlisted check>:<warn|down|non-ok>; the core reportscore:deadorcore:wedgedfrom its existing restart decision. Custom names, dynamic loop names, error details, paths, and task identities stay local. Old open records remainunknownwithout inventing another detection. These are observed trigger conditions, not proven root causes across checks.Start review at
src/recovery_issues.py, then the single cause argument insrc/health-check.py. The canonicaldocs/recovery-issue-metrics.mdand catalog date are updated. No new configuration, dependencies, or changes to repair decisions. Requested by the owner to display unique issue causes in an admin pie chart; dashboard companion targets dev and can deploy before clients update.Validation:
python3 tests/recovery-issues.test.py: 11 tests pass, including initial-cause persistence, legacy records, recurrence, concurrency, privacy, and core mode changes.python3 tests/recovery-issues-http.test.py: fresh Python processes execute health/core recovery entry points and send real HTTP captures to a loopback receiver through the normal telemetry sender. Repair/liveness callbacks are controlled; this is not a live user-core restart or production ingestion test.python3 tests/health-check-recover-core.test.py: all core-recovery invariants hold.bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD): PASS (hardcoded-paths, root-artifacts, prose-cap).Before/after evidence: copy the updated HTTP test into a detached parent worktree at
07d552bf, then runpython3 tests/recovery-issues-http.test.pyin each tree. Both use temporary state and local HTTP; no production events are injected.Parent output:
Branch output:
Rollout: new detections on updated clients populate cause slices. Older history cannot be backfilled. Live installed-core restart validation remains a rollout check; this PR only adds telemetry properties and does not change the restart behavior.