Repository navigation
fix(kanban): a sleep-drifted start-time fingerprint must not release a live worker's claim - #118353
liuhao1024 wants to merge 4 commits into
Conversation
…a live worker's claim macOS psutil create_time() readings of an unchanged process drift forward across sleep/wake; the stale-claim reaper read the drift as PID recycling and released a live worker's claim without a signal (NousResearch#118326). Claim liveness (_worker_alive) now tolerates a forward start-time drift on the same instantiation epoch; the signal path (_pid_recycled) stays exact so a recycled PID is still never signalled.
SummaryFixes #118326: on macOS, What changed
Strengths
Findings
VerdictLooks good to merge. Reviewed using Hermes-Agent |
|
Thanks for the careful review! Agreed that in the worst case |
|
Independent field confirmation of #118326 on macOS 27.0 (arm64, psutil 7.2.2, Python 3.11.16, Hermes v0.21.3 → v0.21.4), plus one gap we hit when carrying a very similar fix locally. What we observed. A Prometheus-profile kanban worker (run 56) was reclaimed as "crashed" while the process was still alive after a sleep/wake; a duplicate worker spawned beside it and both wrote to the same card. We carried a local patch on Gap worth closing in this PR. With liveness tolerant but the signal path exact, two callers still treat "strict mismatch on a live pid" as terminated, so the claim is released beside a worker that is actually alive:
Isolated tests for both (32 cases: first-probe drift vs post-SIGTERM drift, ±1 / ±200 native units, epoch change, dead pid, None/junk/legacy/UNVERIFIED) plus an SQLite probe of Regression on v0.21.4 (11 kanban test files): 191 passed / 2 skipped / 1 pre-existing failure ( |
Field report on NousResearch#118326 (PR review): the drift rescue made claim liveness tolerant, but two paths still acted on the exact fingerprint's word alone. - _terminate_reclaimed_worker's recycle branch marked a drifted-but-live worker terminated, so the reclaim released the claim beside it and the duplicate-spawn loop re-formed. It now reports survival (signal_refused) and the caller defers via the existing reclaim_deferred path. - The SIGKILL escalation in _terminate_reclaimed_worker and enforce_max_runtime trusted the tolerant _worker_alive verdict without re-checking the exact fingerprint, so a recycled near-match could take the SIGKILL. Both now refuse against _pid_recycled.
|
Thanks for the detailed field confirmation and for pinpointing both gaps — you're right on both counts, and they're now closed in d8664c5.
Operator Local regression on this branch: the fingerprint suite is 9/9 and the adjacent kanban suites pass (89 passed / 6 skipped across Since the code paths are now closed here, I kept the fix minimal rather than importing your 32-case branch — but if you spot a probe shape the two new tests ( |
|
Reproduced the direction gap on this branch (
So the "covers both cases" note above holds for forward drift only; the negative half of the observed ±1 s probe disagreement reproduces the failure without any sleep/wake. Two shapes that would cover it (for author evaluation):
The signalling path stays exact either way. |
…agreement The cross-process create_time disagreement in NousResearch#118326 is bidirectional (~100 cs either way), but the drift rescue only covered a forward delta, so a reaper reading a live worker's start time below the spawn-time record still concluded recycled and released the claim beside it. Keep the forward sleep-drift ceiling and route the negative half through the same start_time_fingerprints_match comparator the other liveness surfaces share (epoch still exact, ±2 s); the signalling path stays exact.
|
Thanks for the claim-level probe and the precise line reference — you're right that the rescue covered only the forward half, and the negative reading is exactly the release-beside-a-live-worker loop this PR exists to close. Pushed 80d1f1b with your first shape: the forward ceiling is kept for accumulated-sleep drift, and the negative half now routes through the same Regression coverage mirrors the matrix from your probe and mlahatte's local checks: a −100 cs same-epoch reading keeps the worker alive and the claim extended (this test fails on the pre-fix code, verified locally), +100 cs stays covered, a −300 cs gap beyond the comparator tolerance is still recycled, and an identical start under a different epoch remains recycled. |
|
Local verification of this PR complete (campaign-style, independent reruns on both sides; full evidence archived). Verdict from the verification side: good to merge.
One field note for the rollout: we carry a local tolerance patch (±2 s comparator only) on our box. It covers neither the 27-minute drift nor the signal-authorization case — and its stale-heartbeat path actually recorded SIGTERM+SIGKILL against a live PID in scenario B — so we'll retire it in favor of this PR once it lands, after a behavior-equivalence check. No production changes were made from our side. |
|
Thank you for the campaign-style verification — especially the base-vs-head symmetry of the three bug-level scenarios and the polarity sweep at the boundaries; that's the strongest evidence this PR has gotten. The acceptance-run parity (identical environmental failure names on both sides, +7 green targeted cases) and the no-over-correction checks (true-dead still reclaimed via the Your field note about retiring the local ±2 s patch is a useful datapoint too: scenario B's stale-heartbeat SIGTERM+SIGKILL against a live PID is exactly the release-beside-a-live-worker failure mode this PR closes, so the behavior-equivalence check should be mostly a formality once it lands. |
|
Confirming this from a Mac mini running the dispatcher: after sleep/wake, live workers were read as recycled, their claims were released, and duplicates were spawned on the same worktree. One edge worth a test: on macOS A possible fix is for |
…Darwin Review follow-up (ottotheoperator): on macOS current_instantiation_epoch() was "" (no /proc), so the 'epoch half must still match' guard in _drifted_spawn_still_matches degraded to "" == "" — after a reboot, a different process reusing the PID with a create time inside the 12 h sleep-drift ceiling was rescued as our worker and its claim extended beside it. Read the boot time via psutil.boot_time() (pinned core dependency, reads sysctl kern.boottime in-process) so a reboot invalidates the epoch on Darwin as it already does on Linux; an unreadable boot time keeps the epoch "" (check disabled, never fail-closed).
|
Thank you for the Mac mini confirmation and for the precise hole: you're right that on macOS the epoch half was Implementation notes:
Your related note on #117931 also checks out from the code: that PR's |
|
Thank you, that was quick! Once this lands we'll drop our local stopgap from #117931 on the Mac mini, since it would otherwise switch off your drift rescue. Much appreciated. |
|
Glad the shape works for you. On Sounds like the right sequencing on your side: once this lands, dropping the #117931 stopgap should be safe precisely because the drift rescue then owns the sleep/wake case end to end. Thanks again for the Mac mini confirmation and the edge report that surfaced the macOS |
|
Adding a live field report in case it helps get this reviewed. We hit this on macOS 26 (arm64), on
We traced it to the exact-equality |
|
Thanks for the live field report — and for tracing it to the exact branch. One clarification that I think squares your trace with the PR: On this PR's head that same call is tolerant before it is exact: an exact match still decides immediately, but a mismatched reading on the same boot is rescued either by the forward-drift ceiling (accumulated sleep/wake, up to 12 h) or — the half your report likely exercised — by the shared So your local fix and this PR are the same shape — same comparator for the start half, exact boot epoch — and your |
|
Field report from macOS, supporting this PR: with #117931 applied, the dispatcher still reclaimed a live worker because of start-time drift. Setup: Mac mini (Apple Silicon, macOS 26.6), Hermes What happened: the reclaim pass declared the worker dead and spawned a second worker for the same card. A diagnostic log line we added just before The epoch half is empty on macOS, and the start-time reading drifted by exactly +100 centiseconds (1.00 s). Same symptom with another model: earlier the same day, on the same machine with a different model (Bonsai‑2 27B), the dispatcher recorded three false Local stopgap until this lands: same epoch plus |
|
Thanks for the field report — the
One note on the empty epoch half: the latest commit on this branch ( Agreed on #117931 being complementary rather than overlapping: unreadable fingerprint vs. readable-but-drifted. Together they cover both halves of the macOS failure mode you hit. |
What does this PR do?
On macOS,
psutil.Process.create_time()readings of an unchanged live process drift forward across every sleep/wake by the accumulated sleep. The kanban stale-claim reaper compares that reading against the spawn-time fingerprint with exact equality, so after a wake it concludes the PID was recycled: it releases a live worker's claim without sending any signal, a duplicate worker spawns beside the original, both write the same card, and the failure breaker eventually blocks the card on pure infrastructure artifacts (#118326).This PR makes claim liveness (
_worker_alive) tolerate a forward start-time drift — same instantiation epoch and a start-time delta within_SAME_WORKER_START_DRIFT_CEILING(12 h of accumulated sleep, centiseconds) — so a drifted-but-live worker keeps its claim (extended, not reclaimed). The same rescue also unblocks the worker-side_claim_is_livefence, so a worker finishing right after a wake can close its own run again.The signalling path stays exact:
_pid_recycled(which gates every SIGTERM/SIGKILL decision) still requires an exact fingerprint match, so a genuinely recycled PID is never signalled. Follow-up to a field report on this PR: the drift rescue also had to stop the exact-vs-tolerant split from leaking into two actions —_terminate_reclaimed_worker's recycle branch no longer marks a drifted-but-live workerterminated(which released the claim beside it and re-created the duplicate-spawn loop); it reports survival (signal_refused), so the existing_worker_survived_terminationpath defers the reclaim (reclaim_deferred)._terminate_reclaimed_workerandenforce_max_runtimenow re-checks the exact_pid_recycledbefore escalating, so a tolerant-alive verdict on a recycled near-match never turns into signal permission.A claim wrongly held against a recycled near-match is at worst held until its heartbeat goes stale, at which point the existing #29747 wedged-worker backstop releases the claim — the fail-closed direction. This mirrors the reasoning #111454 applies to the cron ledger, scoped to the kanban paths that PR does not cover.
Related Issue
Fixes #118326
Type of Change
Changes Made
hermes_cli/kanban_db_dispatch.py: added_SAME_WORKER_START_DRIFT_CEILINGand_drifted_spawn_still_matches;_worker_alivenow rescues a same-epoch fingerprint whose start half drifted forward within the ceiling. No change to_pid_recycleditself.hermes_cli/kanban_db_dispatch.py(follow-up):_terminate_reclaimed_workerreports a drifted-but-live worker as surviving instead of terminated (claim defers, never released beside the live worker), and both its SIGKILL step andenforce_max_runtime's re-check the exact fingerprint before escalating — drift tolerance never becomes signal permission. Operatorreclaim_taskkeeps its human-override release; the terminal reaper retries drifted-live workers on the next tick.tests/hermes_cli/test_kanban_worker_pid_fingerprint.py: five regression tests (drifted worker keeps its claim; drifted fingerprint is never signalled nor released on the stale-heartbeat reclaim path — now assertsreclaim_deferred; a gross 13 h drift is still treated as a recycle; the post-SIGTERM recycled near-match never takes the SIGKILL on either the reclaim or the timeout path).How to Test
pytest tests/hermes_cli/test_kanban_worker_pid_fingerprint.py -q— all 9 tests pass (4 pre-existing + 5 new/updated). Observed result:9 passed.kanban_db_dispatch.pychange stashed,test_sleep_drifted_fingerprint_keeps_the_live_workerfails (the drifted worker is reclaimed); with the change applied it passes.pytest tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_terminal_worker_reaper.py tests/hermes_cli/test_kanban_reclaim_claim_lock_guard.py tests/hermes_cli/test_kanban_complete_live_claim_guard.py tests/hermes_cli/test_kanban_dispatch_lock.py tests/hermes_cli/test_kanban_dispatch_tick_hook.py tests/hermes_cli/test_kanban_dispatch_claim_allowlist.py tests/hermes_cli/test_kanban_gateway_restart_handoff.py tests/hermes_cli/test_kanban_worker_pid_fingerprint.py -q—89 passed, 6 skipped(plustest_infrastructure_spawn_refusal_never_charges_the_card, which fails identically on the unmodified baseline of this branch and is unrelated to this change).ruff check hermes_cli/kanban_db_dispatch.py tests/hermes_cli/test_kanban_worker_pid_fingerprint.py— passes.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
N/A — no skill added.
Screenshots / Logs
N/A