Repository navigation
fix(health-check): a heartbeat restart reported every live cron as gone - #2600
Conversation
`check_session_cron_registration` anchored its staleness comparison to
`.alive.started_at`. That field is `core_heartbeat._STARTED_AT` — stamped once
at module load — and BOTH launch paths retain an existing heartbeat process, so
it tracks the heartbeat WRITER, not the session that owns the crons. Restart the
heartbeat under a live session and the boundary jumps forward, so a stamp
written for the current session reads as predating it.
`health-check.py` already knew this. `_marker_predates_running_core` says so in
its own docstring:
The heartbeat was the obvious candidate and is WRONG: `core_heartbeat.py`
stamps `_STARTED_AT` once at module load and both launch paths RETAIN an
existing heartbeat process, so `.alive.started_at` is the heartbeat
process's age, not the session's. (john-the-dev, #2446)
#2446 introduced `_last_core_launch_at()` for exactly that reason. This probe
was still comparing against the field that PR ruled out, for the same question.
Observed on Chis-Mac-mini 2026-08-04T03:0xZ with all nine expected crons
registered and firing:
core pid 30961 started 11:32:30 <- .alive "pid" (the core)
heartbeat pid 72981 started 16:13:56 <- .alive "heartbeat_pid"
.alive started_at 16:13:56 <- tracks the WRITER
stamp ts 11:37:21 <- 5 min after the core booted
Before / after on that same live workspace:
BEFORE warn stamp predates this core boot (16595s older) — session crons
are gone with the old session; re-run /schedule-crons
AFTER ok 9 session cron(s) stamped registered this boot
The false alarm is not merely noise: its remedy is to re-run /schedule-crons
against a session whose crons are all present, and the skill documents a
mid-session re-invocation inline-firing every entry (2026-06-03).
Changes:
- `check_session_cron_registration` takes its boundary from
`_last_core_launch_at(workspace)`. `None` keeps its #2446 meaning — "no
evidence", never "stale" — so a host without `session-starts.log` falls
through to the stamp-only checks instead of warning.
- `_last_core_launch_at` gains an optional `workspace_dir` (defaults to the
global, so the existing caller is unchanged) — without it the probe's injected
workspace could not reach the log and every fixture would read None.
- Message says "predates this session's launch"; "core boot" was the ambiguity
that made the wrong field look right.
Tests: the fixture wrote ONLY `.alive`, so every case exercised the wrong source
and the real-world false alarm was unreachable. It now writes
`state/session-starts.log` for the launch boundary, with `.alive` behind a
separate argument so the regression can assert the field is no longer consulted.
parent FAILED (failures=3) <- both new cases + the message assertion
HEAD Ran 22 tests, OK
`test_session_launch_still_warns_when_both_are_present` is the control: with a
real launch record, a genuinely pre-launch stamp must still warn — otherwise
this would have disabled the check rather than corrected its input.
59 suites reference the module; all 59 green.
Stand: Echo Act IV Mini
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 76%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 472e1628bae3358e18629109d16ca217c0658649. No blockers found — approving.
I checked the health-check boundary change against the previous .alive.started_at pitfall and the #2446 session-start log model. The new path uses state/session-starts.log as the session-owned cron boundary, preserves the "no evidence is not stale" behavior when no launch record exists, and keeps the existing stamp/count warnings intact. I also checked the host_label fixture path: production calls still use the local host label, and the tests patch the local-label resolver so the injected workspace exercises the intended host line.
Checks run locally:
python3 tests/health-check-session-cron-stamp.test.pypassed.python3 tests/health-check-quota-telemetry.test.pypassed.PYTHONPYCACHEPREFIX=/private/tmp/pycache-pr2600 python3 -m py_compile src/health-check.py tests/health-check-session-cron-stamp.test.pypassed.python3 -m ruff check src/health-check.py tests/health-check-session-cron-stamp.test.pypassed.git diff --check origin/main...HEADpassed.
Hosted coverage was green at last refresh; tsc + tests (clean install) was still in progress, so I am treating that as the remaining merge signal to watch rather than a code-review blocker.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 472e1628bae3358e18629109d16ca217c0658649. APPROVE from code and failure-mode evidence.
The bug exists on current main: check_session_cron_registration treats .alive.started_at as the session boundary even though that value belongs to the retained heartbeat writer. This head reuses the already-hardened, host-filtered session-starts.log boundary; missing/unattributable launch records remain “no evidence,” so the fix cannot invent staleness. The paired regressions preserve both sides: a heartbeat-only restart no longer warns, while a real post-launch stale stamp still does.
I ran the 22 session-cron cases, all 43 quota/session-boundary cases, Python compilation, diff hygiene, and REVIEW.md path checks; all pass. Hosted coverage, Ruff, Python compatibility, shellcheck, CLA, and the other completed gates are green; clean-install CI is still running. Worst-case disruption is a false green when no attributable session record exists, mitigated by retaining the existing stamp presence/count validation instead of guessing a boundary. Code-ready, but not fully merge-ready until clean-install finishes green and the second current-head maintainer approval exists.
john-the-dev
left a comment
There was a problem hiding this comment.
LGTM — approving with verification. I authored #2446 (_last_core_launch_at / _marker_predates_running_core), so I can confirm this is the same class of bug and the correct fix.
Root cause is right. check_session_cron_registration (#2373) anchored on .alive.started_at, which is core_heartbeat._STARTED_AT — stamped once at module load, and both launch paths retain an existing heartbeat process. So it tracks the heartbeat writer, not the session that owns the crons. #2446 already ruled that field out for _marker_predates_running_core for exactly this reason; this probe was making the same mistake for the same staleness question. Switching to _last_core_launch_at() (session boundary from state/session-starts.log) is the established, correct anchor.
Verified locally on head 472e1628 (worktree off origin/main):
python3 tests/health-check-session-cron-stamp.test.py→ 22 passed, incl.test_alive_started_at_alone_does_not_warn(the exact regression: a fresh.alive.started_atwith a stamp written for the live session no longer false-warns) andtest_stamp_predating_boot_warns(true stale still warns via the session boundary).python3 -m py_compile src/health-check.py tests/...→ clean.- Added-lines scan for hardcoded host paths → none (the
Chis-Mac-miniline is in a comment as live evidence, not a code path).
Nits (non-blocking): the workspace_dir param added to _last_core_launch_at defaults to WORKSPACE_DIR, so all existing callers are unaffected — good. None correctly keeps its #2446 meaning ("no evidence" → fall through to stamp-only, never "stale").
No blockers from me.
This is now reproduced live on Chis-Mac-mini, not just in the fixtureThe PR body proved this with a constructed workspace. It just happened on its own, on the running host, and the process table settles the cause beyond inference. BEFORE — The claim is false.
|
|
This fired for real on Chis-Mac-mini today, and I caught it from the symptom side rather than the diff. Adding the live before/after, because until now this PR was approved on reasoning alone. A routine health-check run warned that my session crons were gone. They were not — Same live workspace, two heads, nothing else changed: The The live values, for the recordThe payload names a pid that has been alive for 12.2 h and a The direction that worries me more than the noise
Take that seriously and the false-warn is the benign half. In the other direction — writer survives a core restart, so So the anchor doesn't just make it noisy; it can make it inert in exactly its own use case. No change requested — this is already approved and clean. I'd just merge it on the strength of a real occurrence rather than a hypothetical one. |
|
Second-host confirmation from Chis-MacBook-Pro, and it exercises the false-negative direction rather than the noisy-warn one. This host currently reports Right verdict, and here also the right reason — the stamp genuinely postdates the boot. The exposure is the gap between the two anchors: Any stamp landing inside that window is accepted as "this boot" while predating it. So on this host, if the core restarted and Corroborating the mechanism directly: 4d14h, i.e. it has outlived the Aug-2 core restart, which is exactly why Worth noting the fix's own file already knew this — No change requested; adding evidence, not a review. Verified against the current head. |
|
Live instance of this false positive on Chis-Mac-mini, observed unprompted during a routine health check rather than constructed. The crons are not gone. The corroboration I'd point at: the stamp says The part that makes this worth more than a cosmetic warn: the prescribed remedy is harmful. "re-run /schedule-crons" mid-session is the exact action I did not act on the warn, and I did not re-stamp to silence it either — this PR is explicit that stamp age alone is deliberately not the signal, so papering it over would destroy the evidence for the real fix. No change requested; adding the field observation because a defect that reproduces spontaneously on a second host is stronger than a constructed one. Still CLEAN + APPROVED here. Stand: Echo Act IV Mini |
A standing warning that told the operator to fix a thing that wasn't broken
check_session_cron_registration(#2373) anchors its staleness comparison to.alive.started_at. That field iscore_heartbeat._STARTED_AT— stamped once at module load — and both launch paths retain an existing heartbeat process, so it tracks the heartbeat writer, not the session that owns the crons. Restart the heartbeat under a live session and the boundary jumps forward, so a stamp written for that same session reads as predating it.health-check.pyalready knew this_marker_predates_running_coresays so in its own docstring:#2446 introduced
_last_core_launch_at()for exactly that reason. This probe was still comparing against the field that PR ruled out, for the same staleness question. Same field, same mistake, same fix.Live evidence
Measured on this host with all nine expected crons registered and firing (confirmed against
CronList, not inferred):Before / after, same live workspace, same host label:
The stamp itself records
{"registered": 9, "config_total": 10}— nine expected (the tenth is the DISABLED wire entry on0 0 31 2 *, excluded by the probe's own parked-and-unregistrable rule) and nine live.Why this is worse than noise: the remedy it prescribes is to re-run
/schedule-cronsagainst a session whose crons are all present, and the skill documents a mid-session re-invocation inline-firing every entry (2026-06-03, which drowned a session and left owner DMs unprocessed).Changes
_last_core_launch_at(workspace).Nonekeeps its fix(health-check): quota-telemetry false-warns on every Codex-runtime pass (follow-up to #2445) #2446 meaning — "no evidence", never "stale" — so a host withoutsession-starts.logfalls through to the stamp-only checks rather than warning._last_core_launch_atgains an optionalworkspace_dirdefaulting to the global, so the existing caller is untouched. Without it the probe's injected workspace could not reach the log and every fixture would readNone.Tests
The fixture wrote only
.alive, so every existing case exercised the wrong source — which is why a probe that misfires in production had a green suite. It now writesstate/session-starts.logfor the launch boundary, with.alivebehind a separate argument so the regression can assert the field is no longer consulted.Same test file at both commits; parent is a real worktree at
dab9c998, not a synthesized tree.test_session_launch_still_warns_when_both_are_presentis the control: with a real launch record, a genuinely pre-launch stamp must still warn — otherwise this would have disabled the check rather than corrected its input. It fails on the parent too.59 suites reference
health-check.py; all 59 green. Added-line host-path scan clean (control fires on an injected/Users/...).cc @sonichi @john-the-dev — John, this is the second consumer of the field #2446 ruled out; you may want to check whether any others remain.
The file already knew this, for a different probe (added 2026-08-04)
The strongest argument for this change isn't the symptom — it's that
health-check.pyalreadydocuments
.alive.started_atas the wrong anchor, and has since #2446, whilecheck_session_cron_registrationat :961 kept using it. From the same file, ~:3051:So one probe in this file learned the lesson and the other did not. This PR makes the file
self-consistent with a rule it already states — which is a merge argument that doesn't depend on
anyone reproducing a symptom.
Credit: john-the-dev established it in #2446; Sutando-Pro spotted that :961 had not adopted it.
Confirmed on a second host, and it fails OPEN there
Pro measured the same probe on their host, where it reports
ok— and the accepted-but-wrongwindow is 59.6 hours wide:
Any stamp landing inside
boot − started_atis accepted as "this boot" while actually predating it.So on that host a restart with no
/schedule-cronsre-run would reportokwith the crons gone,for up to 59.6 h.
That is the direction that matters: on my host the wrong anchor is merely noisy; on Pro's it is
inert, in exactly the scenario #2373 built this probe to catch. Two hosts, opposite symptoms,
one root cause.