fix(issue-18): admit remote attempts atomically, end timed-out commands with their group, fence heartbeats off the terminal, renew liveness only on news

Review round 2 (planner-reviewer-codex-01, NOT PASS at d09c162):

- R1 remote.mirror_job: admission under the registry lock. The same
  dispatch_token is idempotent (a finished attempt never runs again), a
  lower attempt or the same attempt under another token is refused before
  any write, and only a strictly newer attempt replaces the record
  (register_job(supersede_older_attempt=True); events.log kept, seqs stay
  monotonic).
- R2 remote.handle_dispatch: the --exec command runs in its own session;
  at timeout_sec, or when the runner is interrupted (SIGTERM now unwinds
  it), the whole process group is terminated (TERM, then KILL) before the
  error is published.
- R3 terminal barrier: a terminal publish raises an attempt-scoped barrier
  under the lock that allocates seqs, stops the heartbeat timer, gives it
  MAM_HEARTBEAT_DRAIN_S (default 1 s) to exit and then kills it, and only
  then takes its own seq. Beats are admitted under that lock
  (next_seq(admit=...)); a beat still on the wire is cancelled with the
  timer rather than holding the terminal back; and a late non-terminal
  event never publishes a running state over a terminal record.
- R4 job_subscriber: a retained state renews the idle clock only when its
  seq is newer than everything already accepted; re-delivered or older
  snapshots still recover a terminal status.
- tests: five regressions, all failing on 77f2829; T1-14's wall clock
  15 -> 30 s against public-broker latency.
- docs: MESSAGING 2.5 (barrier, state renewal, detection vs final close),
  heartbeat docstring, loop SKILL.md (remote rows need the Python runner),
  VERSIONS v4.5.0 items 1/4/5 and the test count (25).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HCGpJXhSZasJLU38ksDghe

.agents/skills/lib_py/delegation/heartbeat.py

@@ -15,8 +15,13 @@ timer stops on its own when
   * the job record disappears, or
   * its lifetime exceeds the job's wall-clock timeout plus one period.
 
-A terminal publish also stops it directly through the pidfile
-(`<registry>/<job_id>.heartbeat.pid`) so no heartbeat can trail a `completed`.
+A terminal publish also stops it directly, so no heartbeat can trail a
+`completed` (review round 2, codex R3): it raises an attempt-scoped barrier
+under the registry lock that allocates seqs (a beat either took its seq before
+it or is refused when it asks for one), stops the timer through the pidfile
+(`<registry>/<job_id>.heartbeat.pid`), and waits for the timer to drain its
+in-flight beats - killing it after `MAM_HEARTBEAT_DRAIN_S` - before it takes
+its own seq.
 
 The timer is bound to the agent's life (review B2): it watches one process -
 `--watch-pid`, or by default the nearest ancestor of the publishing process
@@ -36,6 +41,7 @@ checks read.
 from __future__ import annotations
 
 import argparse
+import json
 import logging
 import os
 import signal
@@ -154,8 +160,31 @@ def _is_timer_process(pid: int, job_id: str) -> bool:
     return "lib_py.delegation.heartbeat" in command and job_id in command
 
 
-def stop_timer(job_id: str, registry_dir: str) -> bool:
-    """Terminate the job's timer if one runs. Returns True when one was signalled."""
+DRAIN_ENV = "MAM_HEARTBEAT_DRAIN_S"
+# Short on purpose: the barrier already keeps any beat from taking a seq after
+# the terminal, so the drain only lets an idle timer exit cleanly. A beat still
+# on the wire is cancelled with the timer (its seq is older than the terminal's)
+# rather than holding back the terminal event - which may be the orchestrator
+# closing the job - for seconds of broker round trips.
+DEFAULT_DRAIN_S = 1.0
+
+
+def drain_seconds() -> float:
+    """How long a terminal publish waits for the timer to drain (env override)."""
+    try:
+        return max(0.0, float(os.environ.get(DRAIN_ENV, DEFAULT_DRAIN_S)))
+    except ValueError:
+        return DEFAULT_DRAIN_S
+
+
+def stop_timer(job_id: str, registry_dir: str, drain: Optional[float] = None) -> bool:
+    """Terminate the job's timer if one runs. Returns True when one was signalled.
+
+    ``drain`` (seconds): also wait for the timer to exit - on TERM it stops
+    scheduling and joins its in-flight beats - and SIGKILL it if it has not
+    gone by then, so none of its beats can still publish afterwards. A terminal
+    publish uses this (review round 2, codex R3): "TERM sent" is not "timer
+    stopped"."""
     pid = running_timer(job_id, registry_dir)
     path = pidfile_path(job_id, registry_dir)
     if pid is None or not _is_timer_process(pid, job_id):
@@ -168,6 +197,19 @@ def stop_timer(job_id: str, registry_dir: str) -> bool:
         os.kill(pid, signal.SIGTERM)
     except OSError:
         pass
+    if drain is not None:
+        deadline = time.monotonic() + drain
+        while time.monotonic() < deadline and _pid_alive(pid):
+            time.sleep(0.1)
+        if _pid_alive(pid):
+            logger.warning("heartbeat timer %d for %s did not drain within %.0fs; killing it", pid, job_id, drain)
+            try:
+                os.kill(pid, signal.SIGKILL)
+            except OSError:
+                pass
+            deadline = time.monotonic() + 2.0
+            while time.monotonic() < deadline and _pid_alive(pid):
+                time.sleep(0.05)
     try:
         os.unlink(path)
     except OSError:
@@ -175,6 +217,40 @@ def stop_timer(job_id: str, registry_dir: str) -> bool:
     return True
 
 
+def barrier_path(job_id: str, registry_dir: str) -> str:
+    return os.path.join(registry_dir, "{}.heartbeat.barrier".format(job_id))
+
+
+def raise_barrier(job_id: str, registry_dir: str, dispatch_token=None, attempt_id=None) -> None:
+    """Mark this attempt as ending (review round 2, codex R3).
+
+    Written under the registry lock - the lock `next_seq` takes - so a beat
+    either took its seq before this point (and the terminal publisher then
+    waits for the timer to drain) or meets the barrier when it asks for a seq
+    and is refused without consuming one."""
+    with mqtt_common.registry_lock(registry_dir):
+        with open(barrier_path(job_id, registry_dir), "w") as fh:
+            json.dump({"dispatch_token": dispatch_token, "attempt_id": attempt_id}, fh)
+
+
+def barrier_blocks(job_id: str, registry_dir: str, dispatch_token=None, attempt_id=None) -> bool:
+    """True when a terminal barrier was raised for this attempt (a later
+    attempt, with its own token, is not blocked). Check it under the registry
+    lock when the answer must be atomic with a seq."""
+    try:
+        with open(barrier_path(job_id, registry_dir)) as fh:
+            barrier = json.load(fh)
+    except (OSError, ValueError):
+        return False
+    if not isinstance(barrier, dict):
+        return True
+    if dispatch_token and barrier.get("dispatch_token"):
+        return barrier["dispatch_token"] == dispatch_token
+    if attempt_id is not None and barrier.get("attempt_id") is not None:
+        return int(barrier["attempt_id"]) == int(attempt_id)
+    return True
+
+
 def start_timer(job_id: str, registry_dir: str, interval: int,
                 dispatch_token: Optional[str] = None, attempt_id: Optional[int] = None,
                 python: Optional[str] = None, watch_pid: Optional[int] = None) -> int:
@@ -222,6 +298,8 @@ def _should_stop(job_id: str, registry_dir: str, dispatch_token, attempt_id,
         return "job record gone"
     if rec.get("status") in TERMINAL_STATUSES:
         return "job is {}".format(rec.get("status"))
+    if barrier_blocks(job_id, registry_dir, dispatch_token, attempt_id):
+        return "the terminal barrier for this attempt is raised"
     if dispatch_token and rec.get("dispatch_token") != dispatch_token:
         return "dispatch token changed (re-dispatched)"
     if attempt_id is not None and rec.get("attempt_id", rec.get("iteration", 1)) != attempt_id:
@@ -290,12 +368,15 @@ def run(job_id: str, registry_dir: str, interval: int, dispatch_token=None,
 
         def _beat(argv=argv):
             try:
-                rc = publish_event.main(argv)
+                # _beat=True: admitted under the seq lock, never after this
+                # attempt's terminal barrier (review round 2, codex R3)
+                rc = publish_event.main(argv, _beat=True)
             except Exception as exc:  # pragma: no cover - defensive
                 logger.warning("heartbeat publish raised: %s", exc)
                 rc = 2
             if rc == 4:
-                logger.info("heartbeat publish refused (stale attempt); timer for %s will stop", job_id)
+                logger.info("heartbeat publish refused (terminal barrier, terminal record or stale attempt); "
+                            "timer for %s will stop", job_id)
                 stop["flag"] = True
 
         t = threading.Thread(target=_beat, daemon=True)

.agents/skills/lib_py/delegation/job_subscriber.py

@@ -168,6 +168,12 @@ class _Watcher:
         last = max(self.last_seq.get(jid, 0), self.state_seq.get(jid, 0))
         if jid in self.accepted_any and seq > last + 1:
             self._note_seq_gap(jid, last + 1, seq, "state", msg.topic)
+        # Review round 2 (codex R4): a state renews liveness only when it carries
+        # news - a seq beyond every event and state already accepted. The same
+        # retained snapshot re-delivered on every reconnect, or an older one
+        # replayed, still recovers a terminal status in the main loop, but it is
+        # no evidence that the agent is alive now.
+        renews = seq > last
         if seq > self.state_seq.get(jid, 0):
             self.state_seq[jid] = seq
         mqtt_common.append_event(jid, {
@@ -178,7 +184,7 @@ class _Watcher:
             "timestamp": payload.get("timestamp"),
             "detail": "status={} last_event={}".format(payload.get("status"), payload.get("last_event")),
         })
-        self.events.put((msg.topic, payload))
+        self.events.put((msg.topic, dict(payload, _renews_liveness=renews)))
 
     def on_message(self, _client, _userdata, msg) -> None:
         # --- defensive parsing -------------------------------------------
@@ -474,16 +480,18 @@ def _run_subscriber(args) -> int:
                     _check_disk_fallback(pending, args.registry_dir, terminal, dispatch_tokens, attempts)
                 continue
 
-            last_event = time.monotonic()
             retention_checked = True
-            first_event_seen = True
+            if payload.pop("_renews_liveness", True):
+                last_event = time.monotonic()
+                first_event_seen = True
 
             jid = payload["job_id"]
             event = payload.get("event")
             if event == STATE_EVENT:
-                # Issue #18: retained job state. Any state resets the idle
-                # window (the agent is alive enough to have published); a
-                # terminal status ends the wait exactly like the terminal event.
+                # Issue #18: retained job state. It renewed the idle window
+                # above only if it advanced the seq (review round 2, codex R4:
+                # a re-delivered or older snapshot is not liveness); a terminal
+                # status ends the wait exactly like the terminal event.
                 status = payload.get("status")
                 if status in TERMINAL_STATUSES and jid not in terminal:
                     synth = "completed" if status == "completed" else "error"

.agents/skills/lib_py/delegation/mqtt_common.py

@@ -515,12 +515,25 @@ def _update_job_status_legacy(job_id: str, registry_dir: str = DEFAULT_REGISTRY_
     return record
 
 
-def next_seq(job_id: str, registry_dir: str = DEFAULT_REGISTRY_DIR) -> int:
+class AdmissionRefused(Exception):
+    """A publish was refused under the registry lock before it took a seq."""
+
+
+def next_seq(job_id: str, registry_dir: str = DEFAULT_REGISTRY_DIR, admit=None) -> int:
     """Return the next monotonic sequence number for a job, persisted in the
     record's ``last_seq`` field so it stays consistent across process restarts.
-    First call returns 1."""
+    First call returns 1.
+
+    ``admit`` (optional) is called with the record under the same lock; a
+    non-empty return refuses the seq (``AdmissionRefused``) and writes nothing.
+    Heartbeat beats use it so that a terminal barrier and the seq allocation
+    are one atomic decision (review round 2, codex R3)."""
     with registry_lock(registry_dir):
         record = load_job(job_id, registry_dir)
+        if admit is not None:
+            reason = admit(record)
+            if reason:
+                raise AdmissionRefused(reason)
         seq = int(record.get("last_seq", 0)) + 1
         record["last_seq"] = seq
         record["updated_at"] = _utcnow()

.agents/skills/lib_py/delegation/publish_event.py

@@ -122,6 +122,10 @@ def build_state_payload(job: Dict[str, Any], prepared: "PreparedEvent") -> Dict[
     event so a forged state cannot close a job.
     """
     status = EVENT_TO_STATUS.get(prepared.event) or "running"
+    if prepared.event not in TERMINAL_EVENTS and job.get("status") in TERMINAL_EVENTS:
+        # A late non-terminal event must not publish a retained `running` over
+        # the terminal snapshot (review round 2, codex R3).
+        status = job["status"]
     data = {k: v for k, v in (prepared.payload.get("data") or {}).items()
             if k in ("dispatch_token", "attempt_id")}
     payload = build_payload(job_id=prepared.job_id, seq=prepared.seq, event="state",
@@ -164,18 +168,20 @@ class PreparedEvent(object):
 
 def prepare_event(job_id, event, detail="", data=None,
                   registry_dir=DEFAULT_REGISTRY_DIR, retained=False,
-                  dispatch_token=None, attempt_id=None) -> PreparedEvent:
+                  dispatch_token=None, attempt_id=None, admit=None) -> PreparedEvent:
     """Load the job, consume the next seq, and build the signed payload.
 
     Consuming the seq here (before any transport runs) is deliberate: a failed
     publish must still burn the number, so a replayed packet can never carry a
-    seq the subscriber would accept.
+    seq the subscriber would accept. ``admit`` is checked under the lock that
+    allocates the seq (``mqtt_common.next_seq``); a refusal raises
+    ``AdmissionRefused`` and consumes nothing.
     """
     job = load_job(job_id, registry_dir)
     config = broker_config_from_job(job)
     topic = job.get("topic_prefix")
     topic = f"{topic}/events" if topic else events_topic_for(job_id)
-    seq = next_seq(job_id, registry_dir)
+    seq = next_seq(job_id, registry_dir) if admit is None else next_seq(job_id, registry_dir, admit=admit)
 
     payload_data = dict(data) if data else {}
     eff_dispatch_token = dispatch_token
@@ -265,7 +271,28 @@ def _resolve_job_id(args) -> Optional[str]:
     return args.job
 
 
-def main(argv=None) -> int:
+def _beat_admission(job_id, registry_dir, dispatch_token, attempt_id):
+    """Admission for one heartbeat beat, evaluated under the lock that allocates
+    its seq (review round 2, codex R3): never after this attempt's terminal
+    barrier, on a terminal record, or after a re-dispatch."""
+    def admit(record):
+        status = record.get("status")
+        if status in TERMINAL_EVENTS:
+            return "job is {}".format(status)
+        if heartbeat_timer.barrier_blocks(job_id, registry_dir, dispatch_token, attempt_id):
+            return "the terminal barrier for this attempt is raised"
+        if dispatch_token and record.get("dispatch_token") != dispatch_token:
+            return "dispatch token changed (re-dispatched)"
+        if attempt_id is not None and record.get("attempt_id", record.get("iteration", 1)) != attempt_id:
+            return "attempt changed (re-dispatched)"
+        return None
+    return admit
+
+
+def main(argv=None, _beat: bool = False) -> int:
+    """CLI entry point. ``_beat`` is set only by the heartbeat timer
+    (``heartbeat.run``): the beat is admitted under the seq lock
+    (``_beat_admission``) and a refusal returns 4, which stops the timer."""
     parser = argparse.ArgumentParser(description="Publish a Job event to MQTT")
     target = parser.add_mutually_exclusive_group(required=True)
     target.add_argument("--job", help="job id to publish for")
@@ -389,10 +416,19 @@ def main(argv=None) -> int:
                      check_token, actual_tok)
         return 4
 
-    # Issue #18: a terminal event ends the heartbeat obligation. Stop the timer
-    # before taking this event's seq so no heartbeat can follow `completed`.
+    # Issue #18: a terminal event ends the heartbeat obligation. Review round 2
+    # (codex R3): first raise this attempt's barrier - under the lock that
+    # allocates seqs, so no beat can take a seq after it - then stop the timer
+    # and wait for it to drain its in-flight beats (killing it if it does not),
+    # and only then take this event's seq. No heartbeat can follow the terminal
+    # event, on disk or on the wire.
     if args.event in TERMINAL_EVENTS:
-        if heartbeat_timer.stop_timer(job_id, args.registry_dir):
+        if rec is not None:
+            heartbeat_timer.raise_barrier(
+                job_id, args.registry_dir,
+                dispatch_token=args.expect_dispatch_token or actual_tok,
+                attempt_id=args.attempt_id if args.attempt_id is not None else rec_attempt)
+        if heartbeat_timer.stop_timer(job_id, args.registry_dir, drain=heartbeat_timer.drain_seconds()):
             logger.info("stopped the heartbeat timer for job %s", job_id)
 
     try:
@@ -405,7 +441,11 @@ def main(argv=None) -> int:
             retained=args.retained,
             dispatch_token=args.expect_dispatch_token,
             attempt_id=args.attempt_id,
+            admit=_beat_admission(job_id, args.registry_dir, check_token, check_attempt) if _beat else None,
         )
+    except mqtt_common.AdmissionRefused as exc:
+        logger.info("heartbeat for job %s not published: %s", job_id, exc)
+        return 4
     except FileNotFoundError as exc:
         logger.error("%s", exc)
         return 1

.agents/skills/lib_py/delegation/registry.py

@@ -88,6 +88,7 @@ def register_job(
     dispatch_token: Optional[str] = None,
     attempt_id: int = 1,
     transport: str = "herdr",
+    supersede_older_attempt: bool = False,
 ) -> str:
     """Create a new ``pending`` job record and return its id.
 
@@ -147,7 +148,17 @@ def register_job(
     }
     with registry_lock(registry_dir):
         if mqtt_common._job_path(job_id, registry_dir).exists():
-            raise FileExistsError(f"job already exists: {job_id}")
+            if not supersede_older_attempt:
+                raise FileExistsError(f"job already exists: {job_id}")
+            # A remote runner adopting the orchestrator's next attempt (review
+            # round 2, codex R1). Decided under this lock, so a stale or
+            # conflicting dispatch can never roll the record back.
+            old = mqtt_common.load_job(job_id, registry_dir)
+            old_attempt = int(old.get("attempt_id", old.get("iteration", 1)) or 1)
+            if int(attempt_id) <= old_attempt:
+                raise FileExistsError(
+                    f"job {job_id} is already at attempt {old_attempt}; refusing attempt {attempt_id}")
+            record["last_seq"] = int(old.get("last_seq", 0) or 0)  # seqs stay monotonic across attempts
         _atomic_write_record(job_id, registry_dir, record)
     # Seed the persistent audit log (meta.json + status.json + a "registered"
     # event). Best-effort inside init_job_log — never blocks registration.

.agents/skills/lib_py/delegation/remote.py

@@ -33,6 +33,7 @@ import logging
 import os
 import queue
 import shlex
+import signal
 import socket
 import subprocess
 import sys
@@ -153,15 +154,39 @@ def clear_dispatch(config, topic: str) -> None:
 # remote side
 # ---------------------------------------------------------------------------
 
+def _attempt_of(record: Dict[str, Any]) -> int:
+    try:
+        return int(record.get("attempt_id", record.get("iteration", 1)) or 1)
+    except (TypeError, ValueError):
+        return 1
+
+
 def mirror_job(d: Dict[str, Any], registry_dir: str, session: str) -> Optional[str]:
-    """Create the local job record a remote agent publishes against.
+    """Create the local job record a remote agent publishes against, or refuse
+    the dispatch (None).
 
     The record carries the orchestrator's job_id, auth_token, dispatch_token
     and attempt_id, so `publish_event` on this host produces events the
-    orchestrator's subscriber accepts. A previous attempt's record (a
-    re-dispatch) is retired first: creation is the only write allowed here.
+    orchestrator's subscriber accepts. Admission (review round 2, codex R1): a
+    valid signature proves where a dispatch came from, not that it is current,
+    so
+
+      * the same dispatch_token is idempotent while its attempt is open, and is
+        never run again once that attempt reached a terminal;
+      * a lower attempt, or the same attempt under another token, is a stale or
+        conflicting copy: refused before anything is written, so the record and
+        its events.log stay as they are;
+      * only a strictly newer attempt replaces the record, and that check and
+        the write are one step under the registry lock
+        (``register_job(supersede_older_attempt=True)``). The events.log is kept:
+        the history of earlier attempts is not erased.
     """
     jid = d["job_id"]
+    try:
+        incoming = int(d.get("attempt_id") or 1)
+    except (TypeError, ValueError):
+        logger.warning("drop dispatch for %s: attempt_id %r is not a number", jid, d.get("attempt_id"))
+        return None
     path = mqtt_common._job_path(jid, registry_dir)
     if path.exists():
         old = load_job(jid, registry_dir)
@@ -169,28 +194,32 @@ def mirror_job(d: Dict[str, Any], registry_dir: str, session: str) -> Optional[s
             if old.get("status") in ("completed", "error", "cancelled"):
                 return None  # this attempt already ran to its end: a re-sent dispatch is not run again
             return jid  # same attempt, already mirrored (duplicate delivery)
-        with mqtt_common.registry_lock(registry_dir):
-            path.unlink()
-            log = mqtt_common._job_path(jid, registry_dir).with_suffix(".events.log")
-            if log.exists():
-                log.unlink()
-    registry.register_job(
-        prompt=d.get("brief", ""),
-        agent=d.get("agent") or "remote",
-        agent_session=SESSION_PREFIX + session,
-        role=d.get("role") or "Worker",
-        timeout_sec=int(d.get("timeout_sec") or 3600),
-        idle_timeout_sec=d.get("idle_timeout_sec"),
-        registry_dir=registry_dir,
-        job_id=jid,
-        auth_token=d.get("auth_token"),
-        heartbeat_sec=int(d.get("heartbeat_sec") or registry.DEFAULT_HEARTBEAT_SEC),
-        heartbeat_misses=int(d.get("heartbeat_misses") or registry.DEFAULT_HEARTBEAT_MISSES),
-        ack_timeout_sec=int(d.get("ack_timeout_sec") or registry.DEFAULT_ACK_TIMEOUT_SEC),
-        dispatch_token=d.get("dispatch_token"),
-        attempt_id=int(d.get("attempt_id") or 1),
-        transport="remote",
-    )
+        if incoming <= _attempt_of(old):
+            logger.warning("refusing dispatch for %s attempt %s: this runner already holds attempt %s "
+                           "(a stale or conflicting copy)", jid, incoming, _attempt_of(old))
+            return None
+    try:
+        registry.register_job(
+            prompt=d.get("brief", ""),
+            agent=d.get("agent") or "remote",
+            agent_session=SESSION_PREFIX + session,
+            role=d.get("role") or "Worker",
+            timeout_sec=int(d.get("timeout_sec") or 3600),
+            idle_timeout_sec=d.get("idle_timeout_sec"),
+            registry_dir=registry_dir,
+            job_id=jid,
+            auth_token=d.get("auth_token"),
+            heartbeat_sec=int(d.get("heartbeat_sec") or registry.DEFAULT_HEARTBEAT_SEC),
+            heartbeat_misses=int(d.get("heartbeat_misses") or registry.DEFAULT_HEARTBEAT_MISSES),
+            ack_timeout_sec=int(d.get("ack_timeout_sec") or registry.DEFAULT_ACK_TIMEOUT_SEC),
+            dispatch_token=d.get("dispatch_token"),
+            attempt_id=incoming,
+            transport="remote",
+            supersede_older_attempt=True,
+        )
+    except FileExistsError as exc:  # lost the race to a newer (or equal) attempt under the lock
+        logger.warning("refusing dispatch for %s: %s", jid, exc)
+        return None
     brief_dir = os.path.join(registry_dir, jid)
     os.makedirs(brief_dir, exist_ok=True)
     with open(os.path.join(brief_dir, "brief.md"), "w", encoding="utf-8") as fh:
@@ -212,13 +241,68 @@ def _publish(jid: str, registry_dir: str, event: str, detail: str, d: Dict[str,
     return publish_event.main(argv)
 
 
+def _group_alive(pgid: int) -> bool:
+    try:
+        os.killpg(pgid, 0)
+    except OSError:
+        return False
+    return True
+
+
+def _terminate_group(proc: "subprocess.Popen", grace: float = 5.0) -> None:
+    """TERM, then KILL, the agent command's own process group (it runs in its
+    own session) and wait until the group is gone. Only that owned group is
+    signalled (review round 2, codex R2)."""
+    for sig, wait in ((signal.SIGTERM, grace), (signal.SIGKILL, 2.0)):
+        try:
+            os.killpg(proc.pid, sig)
+        except OSError:
+            return  # the group is already gone
+        deadline = time.monotonic() + wait
+        while time.monotonic() < deadline:
+            proc.poll()  # reap the shell itself
+            if not _group_alive(proc.pid):
+                return
+            time.sleep(0.1)
+
+
+def _run_agent_command(exec_cmd: str, env: Dict[str, str], timeout: float):
+    """Run the agent command in its own session; returns (rc, stdout, stderr,
+    timed_out). At the deadline - or when the runner itself is interrupted -
+    the whole process group is terminated before this returns, so no
+    descendant keeps working past the job's timeout or overlaps the next
+    dispatch (review round 2, codex R2: `subprocess.run(shell=True,
+    timeout=...)` killed only the shell)."""
+    proc = subprocess.Popen(exec_cmd, shell=True, env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+                            text=True, cwd=os.getcwd(), start_new_session=True)
+    try:
+        out, err = proc.communicate(timeout=timeout)
+        return proc.returncode, out, err, False
+    except subprocess.TimeoutExpired:
+        _terminate_group(proc)
+        try:
+            out, err = proc.communicate(timeout=5)
+        except subprocess.TimeoutExpired:
+            # a descendant that left the group still holds the pipes: stop reading
+            for stream in (proc.stdout, proc.stderr):
+                try:
+                    stream.close()
+                except Exception:
+                    pass
+            out, err = "", ""
+        return 124, out, err, True
+    except BaseException:
+        _terminate_group(proc)
+        raise
+
+
 def handle_dispatch(d: Dict[str, Any], registry_dir: str, session: str,
                     exec_cmd: Optional[str], python: Optional[str] = None) -> int:
     """Run one dispatched job to its terminal event. Returns the agent rc."""
     jid = mirror_job(d, registry_dir, session)
     if jid is None:
-        logger.info("dispatch for %s attempt %s already finished here; ignoring the re-sent copy",
-                    d.get("job_id"), d.get("attempt_id"))
+        logger.info("dispatch for %s attempt %s not admitted (already finished here, or a stale or "
+                    "conflicting copy); not running it", d.get("job_id"), d.get("attempt_id"))
         return 0
     heartbeat = int(d.get("heartbeat_sec") or registry.DEFAULT_HEARTBEAT_SEC)
     rc_started = _publish(jid, registry_dir, "started", "remote runner {} claimed the job".format(session),
@@ -245,17 +329,15 @@ def handle_dispatch(d: Dict[str, Any], registry_dir: str, session: str,
                 + (["--attempt-id", str(d["attempt_id"])] if d.get("attempt_id") is not None else [])),
         })
         timeout = float(job.get("timeout_sec", 3600))
-        try:
-            res = subprocess.run(exec_cmd, shell=True, env=env, capture_output=True, text=True,
-                                 timeout=timeout, cwd=os.getcwd())
-            rc = res.returncode
-            tail = (res.stdout or "").strip().splitlines()
+        rc, out, err, timed_out = _run_agent_command(exec_cmd, env, timeout)
+        if timed_out:
+            detail = "agent command exceeded timeout_sec {}".format(int(timeout))
+        else:
+            tail = (out or "").strip().splitlines()
             detail = tail[-1][:200] if tail else "exit {}".format(rc)
             if rc != 0:
-                err = (res.stderr or "").strip().splitlines()
-                detail = "exit {}: {}".format(rc, (err[-1] if err else detail)[:200])
-        except subprocess.TimeoutExpired:
-            rc, detail = 124, "agent command exceeded timeout_sec {}".format(int(timeout))
+                errs = (err or "").strip().splitlines()
+                detail = "exit {}: {}".format(rc, (errs[-1] if errs else detail)[:200])
         cur = load_job(jid, registry_dir)
         if cur.get("status") not in ("completed", "error", "cancelled"):
             _publish(jid, registry_dir, "completed" if rc == 0 else "error", detail, d)
@@ -316,6 +398,16 @@ def run_runner(session: str, registry_dir: str, exec_cmd: Optional[str] = None,
     client.reconnect_delay_set(min_delay=1, max_delay=16)
     mqtt_common.with_retry(lambda: client.connect(cfg.host, cfg.port, cfg.keepalive),
                            attempts=3, base_delay=0.2, max_delay=1.0)()
+    # Review round 2 (codex R2): SIGTERM unwinds the runner like an interrupt,
+    # so a command still running is terminated together with its process group
+    # (_run_agent_command) instead of being left behind as an orphan.
+    def _on_term(signum, _frame):
+        raise SystemExit(128 + signum)
+
+    try:
+        previous_term = signal.signal(signal.SIGTERM, _on_term)
+    except ValueError:  # not the main thread (a caller driving the runner in-process)
+        previous_term = None
     client.loop_start()
     deadline = time.monotonic() + timeout if timeout else None
     handled = 0
@@ -339,6 +431,8 @@ def run_runner(session: str, registry_dir: str, exec_cmd: Optional[str] = None,
             if once:
                 return 0
     finally:
+        if previous_term is not None:
+            signal.signal(signal.SIGTERM, previous_term)
         client.loop_stop()
         try:
             client.disconnect()

.agents/skills/multi-agent-mux-loop/SKILL.md

@@ -55,7 +55,10 @@ A session row in `.mam/agent-sessions.yaml` with `transport: remote` is
 delegated as `remote:<name>` (the brief goes over the broker to a
 `remote.py run --session <name>` runner on another host; no herdr, pane or
 process probing happens for it). The row still needs `role` and
-`status: running` to be picked up by `--all-reviewer`.
+`status: running` to be picked up by `--all-reviewer`. Remote rows need the
+default Python loop runner: the legacy shell runner (`MAM_LOOP_PY=0`)
+addresses every session as `herdr:<name>` and does not support
+`transport: remote`.
 
 ## Roles & Responsibilities