fix(heartbeat): a dead core stops reading fresh; a writer from another checkout can be stopped - #4221
fix(heartbeat): a dead core stops reading fresh; a writer from another checkout can be stopped#4221qingyun-wu wants to merge 1 commit into
Conversation
…r checkout can be stopped Closes #4213. 2026-09-12: the core's session and pane were gone for ~8 h while state/cores/<host>.alive stayed seconds old, written by a 69-day-old core_heartbeat.py (parent launchd) whose recorded socket had no server: every probe was "unobserved", the streak never advanced, and the loop kept refreshing the file. Every liveness reader trusted it and was wrong. - run_forever: an OBSERVED absence never refreshes the file (three of them still unlink it, as before). An unobserved run refreshes it for at most UNOBSERVED_BEATS_BEFORE_STALE (20 beats, ten minutes) and then lets it age past every reader's window; it is never unlinked on unobserved probes, so a live core behind a refused tmux client is still not killed by a supervisor (the incident the refused-client suite pins). - _is_writer_argv: any checkout's src/core_heartbeat.py counts, so the writer this host's records name can be stopped after a restart from a worktree. - startup.sh: stop the recorded writer before the guard; if an unrecorded one survives, say so (pid, that it does not report this core, the command to run) instead of printing "already running" and leaving the boot without a heartbeat of its own. Tests: tests/core-heartbeat.test.py TestBeatOnlyWhenObserved (5): observed absence vs unobserved refresh, the cap, the observed-death unlink, the other-checkout writer, startup's order. core-heartbeat-refused-client OK.
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at c643d3b3528b143aa62db967c186558479e7d26f (COMMENT because this account authored the PR). The stale-beat change works in the focused loop tests, but the newly automatic startup stop path is unsafe and the cross-checkout handoff remains incomplete.
Independent production stop probes used only reviewer-created temporary processes and isolated state:
observer.py python3 <other-checkout>/src/core_heartbeat.py
parent HEAD
_is_writer_argv False True
stop_other_writers 0 1
observer exit still alive SIGTERM (-15)
real <other-checkout>/src/core_heartbeat.py process, HEAD:
foreign .heartbeat.pid only recorded=[]; stopped=0; still running
same PID also in .alive recorded=[PID]; stopped=1
The first case models a recorded PID reused by an unrelated observer: the new substring search plus interpreter-suffix check treats its argument as its executed script. Startup now calls this killer automatically. The second shows that _recorded_writer_pids still rejects a pidfile whose stored script path differs from this checkout, so widening the argv matcher alone cannot hand off a writer once its .alive is missing.
The new five-test loop suite passes; with the previous production source it has two failures and one missing-constant error. The four refused-client tests pass. Full three-file diff and the surrounding stop/discovery/startup path inspected; added-path/root/prose checks pass with a rejecting forbidden-path control, and current main merge-tree is clean. I did not run the reported hanging full heartbeat suite or restart the installed service. Hosted CI was still running and the commit-status API had no license/cla context at prepost.
Worst case is terminating an unrelated process during every normal startup, or leaving the new core without a heartbeat after an incomplete handoff. Fix the two inline blockers, add production stop/discovery controls, and provide the required exact-head post-restart witness. The PR remains draft; no deployment or merge performed.
| mentions the path is not a writer. Any checkout's copy counts: the writer this | ||
| checkout's records name may have been started from another checkout (a | ||
| worktree restart), and it is still the one writer of this host's file.""" | ||
| m = _WRITER_SCRIPT_RE.search(args) |
There was a problem hiding this comment.
[P1] Prove the executed heartbeat script before signaling the recorded PID. A real reviewer-created python observer.py python3 <other-checkout>/src/core_heartbeat.py process is rejected by the parent but accepted here; production stop_other_writers sends it SIGTERM (exit -15). The suffix check only proves that the text before the match ends in python3, not that this is the executable. A stale/reused heartbeat_pid can therefore kill an unrelated process, now automatically on startup. Use authoritative argv and an anchored interpreter/script contract, with observer and genuine-writer controls.
| # A writer left by a previous core (a restart from another checkout, a core | ||
| # that died under it) would keep this boot from owning the file: stop the | ||
| # recorded one first, and name an unrecorded survivor instead of hiding it. | ||
| [ -n "$PY" ] && "$PY" "$REPO/src/core_heartbeat.py" --stop > /dev/null 2>&1 || true |
There was a problem hiding this comment.
[P2] Make recorded-writer discovery support the cross-checkout handoff too. _recorded_writer_pids still accepts a .heartbeat.pid row only when its stored script path equals this checkout’s file. With a real other-checkout writer, its valid pidfile and no .alive, production --stop finds zero PIDs and leaves it running; adding the same PID to .alive makes this head stop it. The subsequent pgrep guard consequently refuses this core’s heartbeat in the pidfile-only case. Resolve/validate that recorded writer across checkout paths, and cover missing-.alive handoff through the production discovery/stop path.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
Closes #4213. Draft, like #4217: the owner drafted that one while the #4215 live test runs, and the same hold applies here; I un-draft on her word.
The failure
2026-09-12: the core's session and tmux pane were gone for ~8 h while
state/cores/<host>.alivestayed seconds old. The writer was a 69-day-oldcore_heartbeat.py(parent launchd) whose recorded socket had no server: every probe was "unobserved", the absence streak never advanced, and the loop kept refreshing the file. Every liveness reader (health-check, the desktop, peers) trusted it and was wrong. Eight tasks sat unprocessed for up to 7.7 h.startup.shthen skipped its own heartbeat becausepgrepfound the orphan, so the next boot would have had none.What it does
run_forever: an observed absence never refreshes the file (three of them still unlink it, unchanged). An unobserved run refreshes it for at mostUNOBSERVED_BEATS_BEFORE_STALE(20 beats, ten minutes) and then lets it age past every reader's window. It is never unlinked on unobserved probes: a live core behind a refused tmux client is still not killed by a supervisor, the incidentcore-heartbeat-refused-client.test.pypins, and that suite still passes._is_writer_argv: any checkout'ssrc/core_heartbeat.pycounts as the writer, so--stopcan end the writer this host's records name after a restart from a worktree (today's orphan was started from the main checkout; the new core runs fromsutando-core-main).startup.sh: stops the recorded writer before its guard; if an unrecorded one survives, prints its pid, that it does not report this core, and the command to run, instead of✓ core heartbeat (already running).Evidence
The five: an observed absence does not refresh while an unobserved probe does; a long unobserved run stops refreshing after the cap and never unlinks; three observed absences still unlink; a writer from another checkout is a writer, a
-cprogram is not; startup stops the recorded writer before its guard and no longer prints "already running".Host note, stated rather than hidden: on this macOS host the full
tests/core-heartbeat.test.pyhangs at HEAD too (a HEAD export was still running after five minutes; it spawns a real writer inside this worker's tmux session), andcore-heartbeat-core-pid/core-heartbeat-pgrep-portabilityfail at HEAD here as well. CI (Linux) is the measurement for those; I did not measure them after.review-checksand the workspace lint clean on the diff.-- Mars-the-product-dev