fix(issue-18): let an exiting heartbeat timer skip the busy lifecycle lock; kill a timer whose pid cannot be recorded
Review round 5 (planner-reviewer-codex-01, NOT PASS at2b1d590): - F1 bounded lock cycle: the stopper holds the per-job lifecycle lock while it waits for the timer to exit, and the timer's exit cleanup waited for that same lock, so every stop ran out the drain and ended in SIGKILL with a false "did not drain" warning. The exiting timer now tries the lock without blocking (lifecycle_lock(blocking=False)); when it is busy the ownership operation holding it removes the pidfile, and when it is free the own-pid comparison is kept. - F2 untracked owner: when writing the new timer's pid fails after the spawn, _spawn_timer_locked kills and reaps that exact process while the lock is still held and drops only a partial pidfile of that attempt, then re-raises. - tests: an idle timer stops well before a 10 s drain without SIGKILL; a pidfile failure after spawn leaves no survivor, no pidfile and a free lock (both fail on2b1d590); a spawn failure leaves nothing behind and the next start works. - docs: MESSAGING 2.5 and VERSIONS item 1 describe the lifecycle lock; VERSIONS test count 31. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HCGpJXhSZasJLU38ksDghe
This commit is contained in:
1 parent
2b1d590010
commit
263d6bf30d
4 files changed
+158
-15
No files matched your search
@@ -826,6 +826,119 @@ def test_replacing_the_timer_drains_the_old_one_before_the_terminal(tmp_path, mo
|
||||
os.kill(new_pid, signal.SIGKILL)
|
||||
|
||||
|
||||
def test_an_idle_timer_stops_gracefully_well_before_the_drain_deadline(tmp_path, monkeypatch, caplog):
|
||||
"""Review round 5 (codex F1): the stopper holds the lifecycle lock while it
|
||||
waits for the timer to exit, so the exiting timer must not wait for that
|
||||
lock. An idle timer (nothing in flight) ends on TERM at once - well before
|
||||
a generous drain deadline and without the SIGKILL fallback."""
|
||||
import logging
|
||||
from lib_py.delegation import heartbeat, registry
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
monkeypatch.setenv("MAM_ENV_FILE", str(tmp_path / ".mam.env.absent"))
|
||||
monkeypatch.setenv("DELEGATE_JOB_LOGS_DIR", str(tmp_path / "logs"))
|
||||
reg = tmp_path / "jobs"
|
||||
jid = registry.register_job("idle-stop", registry_dir=str(reg))
|
||||
tok = _job(reg, jid)["dispatch_token"]
|
||||
pid = heartbeat.start_timer(jid, str(reg), 60, dispatch_token=tok, python=sys.executable, watch_pid=0)
|
||||
try:
|
||||
assert pid and _alive(pid)
|
||||
time.sleep(2.0) # past interpreter start-up: TERM handler installed, nothing in flight
|
||||
caplog.set_level(logging.WARNING, logger="delegate_job.heartbeat")
|
||||
t0 = time.monotonic()
|
||||
assert heartbeat.stop_timer(jid, str(reg), drain=10) is True
|
||||
elapsed = time.monotonic() - t0
|
||||
assert elapsed < 3.0, f"an idle timer took {elapsed:.1f}s to stop (drain deadline 10 s)"
|
||||
assert "did not drain" not in caplog.text, caplog.text
|
||||
assert not _alive(pid)
|
||||
assert not (reg / f"{jid}.heartbeat.pid").exists()
|
||||
finally:
|
||||
if _alive(pid):
|
||||
os.kill(pid, signal.SIGKILL)
|
||||
|
||||
|
||||
def test_a_timer_whose_pid_cannot_be_recorded_is_killed_at_once(tmp_path, monkeypatch):
|
||||
"""Review round 5 (codex F2): if publishing the new pid fails after the
|
||||
timer was spawned, that exact process is killed and reaped before the
|
||||
lifecycle lock is released - no untracked owner survives, no partial
|
||||
pidfile is left, and the lock is free for the next owner."""
|
||||
import builtins
|
||||
import errno
|
||||
import fcntl
|
||||
from lib_py.delegation import heartbeat, registry
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
monkeypatch.setenv("MAM_ENV_FILE", str(tmp_path / ".mam.env.absent"))
|
||||
monkeypatch.setenv("DELEGATE_JOB_LOGS_DIR", str(tmp_path / "logs"))
|
||||
reg = tmp_path / "jobs"
|
||||
jid = registry.register_job("pidfile-fault", registry_dir=str(reg))
|
||||
pidfile = str(reg / f"{jid}.heartbeat.pid")
|
||||
spawned = []
|
||||
real_popen, real_open = heartbeat.subprocess.Popen, builtins.open
|
||||
|
||||
def recording_popen(*a, **k):
|
||||
proc = real_popen(*a, **k)
|
||||
args = a[0] if a else k.get("args", [])
|
||||
if any("lib_py.delegation.heartbeat" in str(x) for x in args):
|
||||
spawned.append(proc)
|
||||
return proc
|
||||
|
||||
def failing_open(file, mode="r", *a, **k):
|
||||
if str(file) == pidfile and "w" in mode:
|
||||
raise OSError(errno.ENOSPC, "No space left on device", pidfile)
|
||||
return real_open(file, mode, *a, **k)
|
||||
|
||||
monkeypatch.setattr(heartbeat.subprocess, "Popen", recording_popen)
|
||||
monkeypatch.setattr(builtins, "open", failing_open)
|
||||
try:
|
||||
with pytest.raises(OSError) as exc:
|
||||
heartbeat.start_timer(jid, str(reg), 60, python=sys.executable, watch_pid=0)
|
||||
finally:
|
||||
monkeypatch.setattr(builtins, "open", real_open)
|
||||
try:
|
||||
assert exc.value.errno == errno.ENOSPC
|
||||
assert len(spawned) == 1, spawned
|
||||
proc = spawned[0]
|
||||
assert proc.poll() is not None, "the spawned timer must not survive untracked"
|
||||
assert proc.returncode == -signal.SIGKILL
|
||||
assert not os.path.exists(pidfile)
|
||||
with open(reg / f"{jid}.heartbeat.lock", "a+") as fh: # the lifecycle lock is free again
|
||||
fcntl.flock(fh.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
|
||||
fcntl.flock(fh.fileno(), fcntl.LOCK_UN)
|
||||
finally:
|
||||
for proc in spawned:
|
||||
if proc.poll() is None:
|
||||
proc.kill()
|
||||
proc.wait(timeout=5)
|
||||
|
||||
|
||||
def test_a_timer_that_cannot_be_spawned_leaves_nothing_behind(tmp_path, monkeypatch):
|
||||
"""Review round 5 (codex F2), the other half: when the spawn itself fails
|
||||
the error propagates, no pidfile is written, and the lifecycle lock is
|
||||
released and serves the next owner at once."""
|
||||
import fcntl
|
||||
from lib_py.delegation import heartbeat, registry
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
monkeypatch.setenv("MAM_ENV_FILE", str(tmp_path / ".mam.env.absent"))
|
||||
monkeypatch.setenv("DELEGATE_JOB_LOGS_DIR", str(tmp_path / "logs"))
|
||||
reg = tmp_path / "jobs"
|
||||
jid = registry.register_job("spawn-fault", registry_dir=str(reg))
|
||||
with pytest.raises(OSError):
|
||||
heartbeat.start_timer(jid, str(reg), 60, python=str(tmp_path / "no-such-python"), watch_pid=0)
|
||||
assert not (reg / f"{jid}.heartbeat.pid").exists()
|
||||
with open(reg / f"{jid}.heartbeat.lock", "a+") as fh:
|
||||
fcntl.flock(fh.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
|
||||
fcntl.flock(fh.fileno(), fcntl.LOCK_UN)
|
||||
pid = heartbeat.start_timer(jid, str(reg), 60, python=sys.executable, watch_pid=0)
|
||||
try:
|
||||
assert pid and _timer_pid(reg, jid) == pid and _alive(pid)
|
||||
finally:
|
||||
heartbeat.stop_timer(jid, str(reg), drain=5)
|
||||
if _alive(pid):
|
||||
os.kill(pid, signal.SIGKILL)
|
||||
|
||||
|
||||
def _signed_state(job_id, seq, token, dispatch_token, status="running", attempt_id=1):
|
||||
import hashlib, hmac
|
||||
from lib_py.delegation import publish_event as pe
|
||||
|
||||
Reference in new issue
Block a user