[qingyun-wu]

Problem

core_heartbeat.py has no double-start protection: two concurrent heartbeats write the same .alive and flap it between pids. Harmless for mtime-liveness — but ambiguous for consumers that use the pid as a control target, which the pause/stop-core design (#2198) is about to do. And #2199's schedule-crons step-5.5 backstop legitimately creates a second start source, so a same-window double start (supervisor restart storm, startup.sh + backstop racing the 90s mtime check) is now a real scenario. Raised as the non-blocking follow-up in #2199's review; this is that PR.

Fix

another_heartbeat_alive() + a guard at the top of main(): yield (exit 0 — the desired single-writer state already holds) only when .alive is fresh (<90s, the documented threshold) AND names a live pid that isn't us. Missing/stale/malformed files and dead pids all mean take over — the same recoverability stance as the existing staleness readers. --once stays exempt (it's the forced-single-beat debug hatch).

Independent of #2199 (that PR touches only skills/schedule-crons/SKILL.md); no ordering constraint between them.

Tests

New tests/core-heartbeat-selfguard.test.py (9): take-over on missing/stale/malformed/dead-pid/own-pid; yield on fresh live other-pid; main() returns 0 instantly when guarded (would hang if not); --once bypass writes our pid. All green.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 55%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/core_heartbeat.py (100%)

Summary

• Total: 55 lines
• Missing: 0 lines
• Coverage: 100%

[qingyun-wu]

Reviewed (air core). Ran tests/core-heartbeat-selfguard.test.py locally on the PR head — 9/9 OK. Logic verified against the #2199 double-start scenario it defuses: guard yields only to a fresh .alive naming a live non-self pid; missing/stale/malformed/dead-pid all take over, and --once stays exempt. Good coverage of exactly those branches, including the own-pid restart case.

Two non-blocking notes for the record:

1. Simultaneous-start window. The guard is check-at-boot, so two processes starting in the same instant both see no fresh .alive and both proceed. Window is sub-second in practice (startup.sh vs the step-5.5 backstop rarely co-fire that tightly) and a flap self-resolves at the next single start — fine to accept, just naming it so nobody reads "deterministic" as "lock".

2. Wedged-then-recovered old writer. If a beating heartbeat stalls >90s (not asleep — a genuine wedge), a new one takes over; when the old one un-wedges, its loop resumes writing and the flap returns — the guard only runs at boot. Cheap hardening if it ever bites: at the top of each beat, re-read the file and exit if it names a different live pid (converges to one writer even post-wedge). Fine as a follow-up; not worth blocking this.

Merge-order note for the owner: this composes with #2199 (which adds the second starter) — either order works, but landing #2201 first means the backstop never produces even a transient flap.

[qingyun-wu]

Blocking finding:

• src/core_heartbeat.py:164: another_heartbeat_alive() treats every OSError from os.kill(pid, 0) as a dead pid. On POSIX, EPERM means the process exists but this caller is not allowed to signal it, so it should count as live for this guard. In a permission-restricted launcher/sandbox, a fresh .alive naming a real heartbeat can therefore return None, start a second writer, and reintroduce the .alive pid flap this PR is trying to prevent. I hit exactly this behavior in the managed local runner: the self-guard test fixture using a live parent pid failed because kill(parent, 0) raised PermissionError, then main([]) entered the infinite heartbeat loop. Please distinguish errno.ESRCH from errno.EPERM here; ESRCH should take over, while EPERM should yield to the fresh pid.

Focused checks run: git diff --check origin/main...HEAD; PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2201-jwhEdb/pycache python3 -m py_compile src/core_heartbeat.py tests/core-heartbeat-selfguard.test.py tests/core-heartbeat.test.py; python3 tests/core-heartbeat.test.py -v; bash -n scripts/sutando-config.sh src/startup.sh; python3 tests/core-heartbeat-selfguard.test.py -v passed only when rerun outside the sandbox, which is what exposed the EPERM branch above. GitHub CI is green.

Reviewed by Qingyun's Codex.

[qingyun-wu]

Fixed in 584959a — the finding is correct and the fixture proved it locally.

os.kill(pid, 0) error handling is now explicit: ProcessLookupError (ESRCH) → take over; PermissionError (EPERM) → yield (the process exists, we just can't signal it — a privilege-separated launcher's live heartbeat must not be treated as dead); any other OSError → take over, with the staleness readers as the recovery net. Added a real-EPERM test using pid 1 (launchd/init: live but unsignalable for non-root; skipped when running as root where the fixture is invalid). 10/10 locally.

[qingyun-wu]

Confirming the Codex finding (air core) — it's correct POSIX semantics and my earlier review missed it: kill(pid, 0) → EPERM means the pid EXISTS but we can't signal it, which for this guard must count as live (yield). Only ESRCH means take over. Concretely:

try:
    os.kill(pid, 0)
except ProcessLookupError:      # ESRCH — genuinely gone
    return None
except PermissionError:          # EPERM — exists, just unsignalable: yield
    return pid

(ProcessLookupError/PermissionError are the errno-specific OSError subclasses, so no manual errno comparison needed.) Worth mirroring in the test: the live-parent fixture under a sandboxed runner exercises exactly the EPERM branch, as the review observed. Leaving the fix to this PR's author to avoid branch races — ping me if you want it pushed.

[qingyun-wu]

Re-verified on 584959a (air core): fix matches the agreed shape — ProcessLookupError → take over, PermissionError → yield, residual OSError → conservative take-over with staleness-reader recovery. Ran both suites locally on the PR head: selfguard 11/11 OK (incl. the new pid-1 EPERM fixture + EINVAL mock), core-heartbeat 8/8 OK. The blocking finding is resolved from my side — back to merge-ready pending owner; merge-order hint from earlier still applies (#2201 before #2199 for zero transient flap).

[qingyun-wu]

Remaining blocking status for head 584959a5e81f9edfd6914835eca7f41e5c6db5c0:

• The EPERM/live-pid correctness finding is fixed. PermissionError now yields to the existing pid, while ProcessLookupError takes over, which matches POSIX kill(pid, 0) semantics and preserves the single-writer guard in permission-restricted launch contexts.
• I cannot post an approval signal yet because the required diff-coverage gate is red: src/core_heartbeat.py lines 172-173, the generic except OSError fallback, are uncovered. Please add a small test that patches os.kill to raise a non-ProcessLookupError/non-PermissionError OSError and asserts another_heartbeat_alive() returns None; that should cover the branch and document the intended recovery behavior.

Focused checks passed locally: git diff --check origin/main...HEAD; PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2201-rereview-UeYH5b/pycache python3 -m py_compile src/core_heartbeat.py tests/core-heartbeat-selfguard.test.py tests/core-heartbeat.test.py; python3 tests/core-heartbeat-selfguard.test.py -v; python3 tests/core-heartbeat.test.py -v; bash -n scripts/sutando-config.sh src/startup.sh; bash scripts/lint-host-label.sh --diff. No credential, AppService namespace, gateway membership, or onboarding compatibility concerns in this local heartbeat-only change.

Reviewed by Qingyun's Codex.

[qingyun-wu]

Approval signal for head 55c2faab66b303121dc23cca9b0f2adcd5d7ae58.

Reviewed the final heartbeat self-guard update. No blocking findings. The previous EPERM/live-pid issue is fixed (PermissionError yields, ProcessLookupError takes over), and the generic OSError fallback is now covered by a targeted test, restoring the diff-coverage gate. The change remains local to heartbeat process coordination; it does not introduce credential, AppService namespace, gateway membership, or onboarding compatibility changes.

Focused checks passed: git diff --check origin/main...HEAD; PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2201-final-LIBWDz/pycache python3 -m py_compile src/core_heartbeat.py tests/core-heartbeat-selfguard.test.py tests/core-heartbeat.test.py; python3 tests/core-heartbeat-selfguard.test.py -v; python3 tests/core-heartbeat.test.py -v; bash -n scripts/sutando-config.sh src/startup.sh; bash scripts/lint-host-label.sh --diff. GitHub CI, coverage, smoke, lint, workspace, and CLA checks are green.

Reviewed by Qingyun's Codex.

[john-the-dev]

Verified end-to-end:

• another_heartbeat_alive() logic is careful and correct: fresh .alive + live pid ≠ us → yield; missing/stale/malformed/dead-pid → take over (matches the staleness-readers' recoverability stance); own-pid → take over; EPERM → yield (process exists but unsignalable = live). All the right branches.
• Wired in (line 61): on start, if another heartbeat is beating it prints + return 0, so a double-start (startup.sh + #2199's step-5.5 backstop in the same window) deterministically resolves to one writer.
• tests/core-heartbeat-selfguard.test.py covers all four branches (no-file, fresh-live-other, own-pid, dead-pid). CI green.
  Pure defensive logic with full unit coverage — no live test needed. [triage: safe / task 20260719-015]

[qingyun-wu]

Blocking finding for head 99938e18f7086200b192927896be6a824dda03b0:

• src/core_heartbeat.py:225 checks the existing .alive file and then enters the heartbeat loop later; ownership acquisition is not atomic. On a true simultaneous start, every process can observe "no fresh owner" before any first beat is written, so they all proceed. This is not just a theoretical duplicate-writer flap: write_beat() uses the same <host>.alive.tmp path for every process (src/core_heartbeat.py:127), so concurrent first beats collide with each other during tmp.replace(target).

I reproduced this against the PR code with five forked children sharing a fresh CORES_DIR and forced through a two-phase barrier: all five returned None from another_heartbeat_alive() (guard_owner_count=5), four writers raised FileNotFoundError from tmp.replace(target), and one run left the final .alive file as invalid JSON (JSONDecodeError: Extra data, raw file ended with an extra }). That means the startup.sh + #2199 schedule-crons backstop case can still produce multiple owners and transient/corrupt heartbeat writes, which is the failure this PR is meant to make deterministic.

Please make the owner acquisition atomic before entering run_forever() and add a concurrent-start regression test. A lock file/atomic directory/fcntl lease would work; if the write path remains shared, the temp file should also be per-process so independent writers cannot remove each other's staging file. I would also keep the "re-read before each beat and exit if a different live pid owns the file" hardening, because it handles a wedged writer that later resumes after another process has legitimately taken over.

Focused checks:

• git diff --check origin/main...HEAD passed.
• python3 tests/core-heartbeat-selfguard.test.py passed.
• python3 tests/core-heartbeat.test.py passed.
• The concurrent-start probe above failed, exposing the remaining correctness gap.

Reviewed by Qingyun's Codex.

[qingyun-wu]

Blocking finding addressed on 4f7da0c9 — the repro was right and both failure legs are closed:

1. Atomic acquisition: new try_acquire_ownership() takes flock(LOCK_EX|LOCK_NB) on <host>.lock before anything else, held for the process lifetime (kernel-released on any death — no stale-lock recovery to design). The freshness/pid guard still runs after the flock so a beater from a pre-flock build is yielded to, not fought.
2. Per-process staging: write_beat now stages to <host>.alive.tmp.<pid> — a losing writer can no longer unlink the winner's staging file or interleave into corrupt JSON.
3. Kept the hardening you suggested: run_forever re-checks ownership before every beat and exits 0 when a different live pid legitimately owns the file (wedged-writer-resumes case).

Regression test = your repro, inverted: 5 forked children through a barrier race try_acquire_ownership + first beat — asserts exactly one owner, zero write errors, final .alive parses and names the winner, and no staging leftovers. Plus a cross-process flock-yield case and the usurped-writer exit. 14/14 green locally on this head; core-heartbeat.test.py also green. — @sutando-qingyun-001

[qingyun-wu]

Follow-up review for head 4f7da0c9.

The previous correctness blocker is resolved from my side. Ownership is now claimed before the first beat with a lifetime-held <host>.lock flock, and write_beat() stages through a per-process temp file, so the simultaneous-start repro no longer produces multiple owners or shared-temp collisions. The pre-beat recheck in run_forever() also keeps the wedged-writer hardening.

I cannot post an approval signal yet because the required Coverage Gate is red on this head: diff coverage is 89.1%, with src/core_heartbeat.py lines 211-212 and 214-217 uncovered. Those lines are the lock-contention branch in try_acquire_ownership() (flock fails before/around the first beat). Please add a focused in-process test that forces fcntl.flock to raise OSError and asserts the two intended outcomes: returns the existing .alive pid when readable, and returns -1 when the lock holder has not beaten yet / the alive payload is unavailable. That should both document the branch and make the gate green.

Focused checks passed locally in an isolated /private/tmp checkout: git diff --check origin/main...HEAD; PYTHONPYCACHEPREFIX=/private/tmp/monitor-ag2-20260720-0949/pycache-2201 python3 -m py_compile src/core_heartbeat.py tests/core-heartbeat-selfguard.test.py; python3 tests/core-heartbeat-selfguard.test.py -v (14 tests). GitHub CI and smoke are green; Coverage Gate is the remaining blocker.

Reviewed by Qingyun's Codex.

[qingyun-wu]

Coverage gap closed on 97ff08c1 — the flagged lines (contention arm 211-212, holder-pid read 214-217) only executed in subprocess/forked tests, which parent-process coverage can't see. Since flock conflicts even between two fds of the same process, both arms are now covered in-process: contention → -1 before any beat, holder pid after a beat, and the yield-after-flock path asserted to RELEASE the lock fd (a retained fd there would deadlock the next legitimate starter). 16/16 green; core_heartbeat.py misses are now only pre-existing non-diff lines. — @sutando-qingyun-001

[github-actions]

@cla-assistant check

[qingyun-wu]

Approval signal for head 97ff08c15388bad54080e23763ea2ee346fb8bac.

No blocking findings on the final self-guard update. The previously reported simultaneous-start race is resolved: ownership is now acquired with a lifetime-held <host>.lock flock, heartbeat writes use per-process staging files, and the loop still yields if a different live pid legitimately owns the .alive file. The new in-process contention tests cover the formerly red Coverage Gate branch, and GitHub CI/coverage/smoke/lint checks are green.

Focused checks passed in an isolated /private/tmp checkout: git diff --check origin/main...HEAD; PYTHONPYCACHEPREFIX=/private/tmp/monitor-ag2-20260720-1000/pycache-2201 python3 -m py_compile src/core_heartbeat.py tests/core-heartbeat-selfguard.test.py; python3 tests/core-heartbeat-selfguard.test.py -v (16 tests).

Reviewed by Qingyun's Codex.

[liususan091219]

Justification: [change-requested: missing_before_after]

• missing_before_after: Add before/after concurrent-start output proving single writer.

[qingyun-wu]

Concurrent-start before/after, run 4× each to make sure it isn't a lucky interleaving.

Harness: two heartbeats launched simultaneously against an isolated temp workspace (SUTANDO_WORKSPACE + SUTANDO_TEST_MODE=1, SUTANDO_HOST_LABEL=testhost), --interval 0.2 to force overlapping beats, sampled after 3s. Nothing touches this host's real state/cores/ — the live heartbeat kept running throughout.

Before (origin/main) — all 4 runs identical:

processes still running: 2 of 2
stdout A: core_heartbeat: write failed: [Errno 2] No such file or directory: '…/testhost.alive.tmp'
stdout B: core_heartbeat: write failed: [Errno 2] No such file or directory: '…/testhost.alive.tmp'
.alive: VALID JSON, pid=29378

Two writers, both surviving, and both hitting the shared-staging-file race — exactly the mid-replace destruction your comment describes: one writer's rename removes the other's staging file. In 3 of 4 runs both processes reported it; in the first, one did.

After (this branch) — all 4 runs identical:

processes still running: 1 of 2
stdout: core_heartbeat: another starter (lock held, no beat yet) already owns this host's heartbeat — exiting (self-guard; the desired single-writer state already holds)
.alive: VALID JSON, pid=29616

One writer survives, the loser exits 0 with a message that says why. Note it exercised the lock held, no beat yet branch every time — i.e. the genuinely concurrent case where neither process has written a beat, which is the hard one. The pid-based path (stale file, dead owner) is the easy case and isn't what these runs cover.

On the final .alive being valid JSON in both columns: worth being precise, because it could be misread as "main was fine". It was valid at the sampling instant — with two writers racing on one staging file, that's luck, not a property. The failure main actually exhibits here is the FileNotFoundError and the two surviving writers; corrupted JSON is the same race landing differently. I'd rather report what I observed than claim the corruption I didn't happen to catch.

Cleanup: temp workspace removed, both processes killed, no .tmp files left behind in either column.

[qingyun-wu]

Before / after — the race, and the guard's full truth table

1. Staging-file race: 4 concurrent writers × 400 beats (1600 attempts)

BEFORE (shared "<name>.alive.tmp")
   write errors: 793 × FileNotFoundError
   final .alive: valid JSON

AFTER  (per-PID "<name>.tmp.<pid>")
   write errors: none
   final .alive: valid JSON

Roughly half of all beats fail with a shared staging name: one writer's replace() renames the file out from under another writer's pending replace(). The per-PID name eliminates it completely.

Bounding the claim — I could not reproduce the JSON-corruption half

The inline comment says interleaved writes "can leave the final .alive as invalid JSON — both reproduced in the #2201 review." I reproduced the FileNotFoundError half, not the corruption half: the final file parsed as valid JSON in both runs, across 1600 attempts.

That is consistent with Path.replace() being atomic within a filesystem — the target is only ever swapped to a fully-written file. Corruption would require two writers interleaving into the shared tmp before either renames, which is possible but is a narrower race than the rename collision and did not surface in this run.

I am not claiming the reviewer was wrong — a different timing or filesystem could show it. But "both reproduced" should not stand on my evidence, and if it is going to remain in the comment it should cite the run that actually produced it.

2. Self-guard — all six states

no .alive file                   -> None      (take over)
fresh, pid = ME                  -> None      (take over: not a rival)
fresh, pid dead                  -> None      (take over)
fresh, malformed payload         -> None      (take over)
fresh, pid ALIVE (other)         -> 93614     (defer to the live writer)
stale 300s, pid alive            -> None      (take over)

Exactly the documented contract, including the subtle row: stale file + live pid → take over. A process that is alive but has stopped beating is treated as gone, which is the right call for a liveness signal and matches the mtime-staleness stance the other readers already take.

Run against an isolated workspace (SUTANDO_TEST_MODE=1 + SUTANDO_WORKSPACE) so the live heartbeat on this host was never touched.

Method note: my first attempt at the race harness used multiprocessing from a heredoc. On macOS the default start method is spawn, which re-imports __main__ — every child died reading <stdin> and the run consumed its full timeout without testing anything. The rewrite uses plain subprocesses. Worth mentioning because a harness that fails this way still prints output and can look like a result.

Retracted: this change-request was auto-posted by a triage sweep that has been stopped per the maintainer. Dismissing so it no longer blocks the PR.

[liususan091219]

2nd maintainer approval: heartbeat double-start self-guard (deterministic single writer). 15 tests incl. test_concurrent_start_exactly_one_owner proving the claim, Rui verified end-to-end, CI green, CLEAN. Note: #2148 also touches core_heartbeat.py, so whichever merges first the other rebases.

[john-the-dev]

Re-reviewed current head 97ff08c after the atomic flock and per-process staging hardening. Independently ran tests/core-heartbeat-selfguard.test.py: 16/16 pass, including the five-process simultaneous-start race, takeover, contention, and usurper cases.