fix(issue-30): review - the stall watcher's lifetime is bounded on both sides and it re-checks its dispatch before acting

Round-1 review of PR #41 (claude N1, codex R1/R2).

N1 - the wrapper stopped its stall watcher with `kill` followed by an
unbounded `wait`. A TERM that lands in the watcher's fork-exec window is
swallowed (bash defers terminating signals in the forked child until the
exec), the watcher then installs its own handler and lives until the job
closes, and the wrapper hung until someone killed the watcher by hand
(reproduced twice in test_issue14's ack test; the wrapper trace stops at
`kill` + `wait`).
R1 - a wrapper that is itself terminated left the watcher orphaned.
R2 - a `completed` or a re-dispatch that landed during a blocking herdr
wait (up to 60 s) was still followed by the idle notice and the nudge.

- multi-agent-mux-delegate-job: `stop_stall_watch` sends TERM, polls up
  to 5 s, then KILLs and reports "ignored TERM; killed"; `start_stall_watch`
  passes `--watch-pid $$`.
- lib_py/stall_watch.py: `--watch-pid` (rc 0 once that pid is gone, checked
  between herdr calls); `_dispatch_open()` re-reads the record after every
  wait and right before the nudge - terminal status, another dispatch token
  or attempt, or a missing record ends the run without a notice; a refused
  `stalled:idle` publish withholds the nudge (`_notify` returns False).
- delegate-job SKILL.md: the watcher's lifetime contract; exit-code rows 2
  and 3 name the idle clock and the wall-clock label; "before v4.7.5" ->
  v4.8.0 (M5, NB2).
- tests/test_issue30_stall_watch.py (M8): the module-wide Linux mark is
  narrowed to S6 (/proc); r4's stub stamps time.time() so its cross-process
  comparison holds on macOS too (47 passed, 1 skipped here).
- tests/test_issue30_stall_watch_stop.py (new, macOS and Linux): the stop
  routine against a TERM-ignoring stand-in returns in 4-12 s; the wrapper
  returns (rc 3) with a TERM-ignoring watcher shim; `--watch-pid` ends the
  watcher; a TERM to the real wrapper leaves no watcher; a job completed or
  re-dispatched during the wait is not nudged; a refused notice withholds
  the nudge. Every one of them hangs or fails on the round-1 code.

Not in this commit (follow-ups): M7 test hook in product code (no adapter
is unreliable, a seam is needed), M9/B3 public-broker latency (on this host
even `started` sometimes never reaches the subscriber), the leading-zero
budget parse (NB1) and M1/M2/M4/M6 go on the #17 item 4 list.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff

.agents/skills/lib_py/stall_watch.py

@@ -232,7 +232,8 @@ class StallWatch(object):
         self.once = set()
 
     def _notify(self, detail):
-        """The only publisher: a `progress` event, nothing else."""
+        """The only publisher: a `progress` event, nothing else. False when the
+        registry or the wire refused it (a refused notice withholds the nudge)."""
         cmd = [sys.executable, "-m", "lib_py.delegation.publish_event", "--registry-dir", self.args.registry_dir,
                "--job", self.args.job, "--event", "progress", "--detail", detail, "--attempts", "1"]
         if self.args.expect_dispatch_token:
@@ -245,6 +246,8 @@ class StallWatch(object):
         res = subprocess.run(cmd, capture_output=True, text=True, env=env, timeout=120)
         if res.returncode not in (0, 2):
             log.warning("stall_watch: publishing %r failed (rc %d): %s", detail, res.returncode, res.stderr.strip()[-200:])
+            return False
+        return True
 
     def _notify_once(self, key, detail, message):
         if key in self.once:
@@ -260,6 +263,28 @@ class StallWatch(object):
         except (OSError, ValueError):
             return None
 
+    def _dispatch_open(self):
+        """Whether the job is still open *and* still the dispatch we were started
+        for: a `completed` or a re-dispatch that landed during a blocking herdr
+        wait must not be nudged (review R2). A missing record counts as gone;
+        an unreadable one as a transient."""
+        try:
+            with open(os.path.join(self.args.registry_dir, self.args.job + ".json")) as f:
+                rec = json.load(f)
+        except FileNotFoundError:
+            return False
+        except (OSError, ValueError):
+            return True
+        if rec.get("status") in TERMINAL_STATUSES:
+            return False
+        tok = self.args.expect_dispatch_token
+        if tok and rec.get("dispatch_token") and rec.get("dispatch_token") != tok:
+            return False
+        att = self.args.attempt_id
+        if att is not None and rec.get("attempt_id") is not None and int(rec["attempt_id"]) != int(att):
+            return False
+        return True
+
     def run(self) -> int:
         st, sig, agent = self.st, Signals(), self.args.agent
         if not st.reliable:
@@ -268,7 +293,13 @@ class StallWatch(object):
         fail_since, idle_since = None, None
         while True:
             sig.job_status = self._job_status()
-            if sig.job_status in TERMINAL_STATUSES:
+            if sig.job_status in TERMINAL_STATUSES or not self._dispatch_open():
+                return RC_TERMINAL
+            if self.args.watch_pid and not _alive(self.args.watch_pid):
+                # The wrapper is gone (killed, crashed): nobody will stop us, so
+                # stop ourselves. Checked between herdr calls, so at most one
+                # wait (<= 60 s) late.
+                log.info("stall_watch: wrapper pid %d is gone; stall watch stops", self.args.watch_pid)
                 return RC_TERMINAL
             if sig.agent_status is None:
                 until, tmo = watcher.VALID_STATES, 1000  # learn the current state
@@ -298,6 +329,9 @@ class StallWatch(object):
                 continue
             backoff.record_success()
             fail_since = None
+            if not self._dispatch_open():
+                log.info("stall_watch: job %s closed or re-dispatched during the wait; stall watch stops", self.args.job)
+                return RC_TERMINAL
             if out.kind == watcher.MATCHED:
                 new = out.agent.get("agent_status")
                 sig.pane_id = out.agent.get("pane_id") or sig.pane_id
@@ -347,14 +381,29 @@ class StallWatch(object):
                 self._notify_once("never_empty", "stall_watch:input_never_empty:%s" % agent,
                                   "stall_watch: input box of %s never looked empty in %ds" % (agent, int(sig.idle_for)))
         if judge(sig, st) == NOTIFY_IDLE_AND_NUDGE:
+            if not self._dispatch_open():  # collect() took time; the job may have closed meanwhile
+                log.info("stall_watch: job %s closed or re-dispatched before the nudge; nothing sent", self.args.job)
+                return idle_since
             log.warning("stall_watch: agent %s idle for %ds with no background work; nudging once",
                         self.args.target, int(sig.idle_for))
-            self._notify("stalled:idle")
             st.nudged = True
+            if not self._notify("stalled:idle"):
+                log.warning("stall_watch: the stalled:idle notice was refused; nudge withheld")
+                return idle_since
             self.src.prompt(NUDGE.format(job=self.args.job))
         return idle_since
 
 
+def _alive(pid: int) -> bool:
+    try:
+        os.kill(pid, 0)
+    except ProcessLookupError:
+        return False
+    except PermissionError:
+        return True
+    return True
+
+
 def main(argv=None) -> int:
     p = argparse.ArgumentParser(description="notify-only stall watcher for one local herdr job (#30)")
     p.add_argument("--job", required=True)
@@ -364,6 +413,7 @@ def main(argv=None) -> int:
     p.add_argument("--herdr-session", default="")
     p.add_argument("--expect-dispatch-token", default=None)
     p.add_argument("--attempt-id", type=int, default=None)
+    p.add_argument("--watch-pid", type=int, default=None, help="exit once this process (the wrapper) is gone")
     args = p.parse_args(argv)
     logging.basicConfig(level=logging.INFO, format="%(asctime)s stall_watch %(levelname)s %(message)s")
     # The wrapper stops us with SIGTERM once its subscriber ends: leave quietly.

.agents/skills/multi-agent-mux-delegate-job/SKILL.md

@@ -48,6 +48,8 @@ multi-agent-mux-delegate-job submit \
   #   argv string is capped at 131,072 bytes on Linux. The loop always uses it.
   #   MAM_JOB_TIMEOUT_S: the budget when --timeout is not given (default 3600).
   #   MAM_STALL_IDLE_S (600) / MAM_STALL_HERDR_FAIL_S (120) / MAM_STALL_WATCH=0: the stall watcher (Issue #30).
+  #   The watcher lives exactly as long as the job's wrapper: the wrapper stops it (TERM, 5 s, then KILL)
+  #   when its subscriber ends, and the watcher exits on its own once the wrapper pid is gone.
   #   --parent-job <id> (Issue #34): this job is delegated from job <id>, which
   #   must exist in the registry (else exit 1). A loop waiting on <id> waits for
   #   it too and shows it to reviewers. Default: $MAM_JOB_ID (the remote runner
@@ -96,9 +98,9 @@ Under fail-closed operation (`MAM_DELEGATE_FAIL_CLOSED=1`, default), the CLI ret
 |:---:|:---:|:---:|---|
 | `0` | Success | `completed` | Job succeeded and completed event received or verified on disk with approval (`PASS`/`AGREE`). |
 | `1` | Failure | `error` | Execution failure / delivery failure: no `started` within `--ack-timeout` T s after the injection (`agent_blocked`; the prompt is never re-injected), or the input never reached the pane. |
-| `2` | Timeout | `error` (`deadline_timeout`) | Subscriber timeout (total budget or idle timeout reached without terminal event). **R1 liveness grace (Issue #18)**: after the subscriber idles out the wrapper keeps re-observing the disk while the job's events.log shows an event at most K x N (`--idle-timeout`) old - the automatic heartbeat counts - up to the wall-clock `--timeout`. herdr status and the pane are logged as a diagnostic only. |
-| `2` | Wall clock | `error` (`wall_clock_timeout`) | The budget (`--timeout`) ran out, events or not (Issue #33; was `deadline_timeout` before v4.7.5). |
-| `3` | Broker Error | `error` (`deadline_timeout`) | Broker unavailable (`MQTT_BROKER_UNAVAILABLE`) and terminal status not found on disk during deadline. |
+| `2` | Timeout | `error` (`deadline_timeout`) | Subscriber idle timeout (K x N without an event; the total budget is the wall-clock row below). **R1 liveness grace (Issue #18)**: after the subscriber idles out the wrapper keeps re-observing the disk while the job's events.log shows an event at most K x N (`--idle-timeout`) old - the automatic heartbeat counts - up to the wall-clock `--timeout`. herdr status and the pane are logged as a diagnostic only. |
+| `2` | Wall clock | `error` (`wall_clock_timeout`) | The budget (`--timeout`) ran out, events or not (Issue #33; was `deadline_timeout` before v4.8.0). |
+| `3` | Broker Error | `error` (`wall_clock_timeout` once the budget is spent, else `deadline_timeout`) | Broker unavailable (`MQTT_BROKER_UNAVAILABLE`) and terminal status not found on disk during deadline. |
 | `5` | Session Busy | `cancelled` (`session_busy`) | Precondition failed / `session_busy`: Preflight check (`preflight_agent`) or TUI idle handshake wait failed (prompt never delivered, retryable). |
 
 In all execution paths, the final line of standard output prints the persistent audit log directory path (`<logs_root>/<job_id>`), fulfilling the parsing contract for orchestrator runners.

.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job

@@ -692,7 +692,7 @@ start_stall_watch() {
     echo "stall watch: human job, no local pane - stall detection off" >&2; return 0
   fi
   local args=(--job "$JOB_ID" --registry-dir "$REGISTRY_DIR" --target "${session#herdr:}"
-              --agent "$agent" --herdr-session "${HERDR_SESSION_NAME:-}")
+              --agent "$agent" --herdr-session "${HERDR_SESSION_NAME:-}" --watch-pid "$$")
   [ -n "$tok" ] && args+=(--expect-dispatch-token "$tok")
   [ -n "$att" ] && args+=(--attempt-id "$att")
   PYTHONPATH="$SCRIPT_DIR/..${PYTHONPATH:+:$PYTHONPATH}" "$PY" -m lib_py.stall_watch "${args[@]}" \
@@ -703,11 +703,23 @@ start_stall_watch() {
 
 # A watcher that already ended on its own with an error (herdr unavailable)
 # is not left silent: its last lines go to this wrapper's stderr.
+# The stop is bounded: a TERM that lands in the watcher's fork-exec window is
+# swallowed (bash defers terminating signals in the forked child until the
+# exec), and an unbounded `wait` then hung this wrapper until someone killed
+# the watcher by hand (PR #41 review N1). TERM, up to 5 s, then KILL.
 stop_stall_watch() {
   [ -n "${STALL_PID:-}" ] || return 0
-  local rc=0
+  local rc=0 i
   if kill -0 "$STALL_PID" 2>/dev/null; then
     kill "$STALL_PID" 2>/dev/null || true
+    for ((i = 0; i < 25; i++)); do
+      kill -0 "$STALL_PID" 2>/dev/null || break
+      sleep 0.2
+    done
+    if kill -0 "$STALL_PID" 2>/dev/null; then
+      echo "stall watch for $JOB_ID ignored TERM; killed (pid $STALL_PID)" >&2
+      kill -9 "$STALL_PID" 2>/dev/null || true
+    fi
     wait "$STALL_PID" 2>/dev/null || true
   else
     wait "$STALL_PID" 2>/dev/null || rc=$?