fix(issue-19): bound the subscriber daemon to its workspace, forward SIGTERM, keep sandboxes from leaking it
create_session.sh detaches `reconcile.sh --subscribe --idle-timeout 0` per workspace; test sandboxes never reaped it and the daemon neither noticed its workspace vanish nor honoured SIGTERM (bash deferred the signal behind the foreground python child). 202 orphans were found holding public-broker wildcard subscriptions for deleted pytest tmpdirs. - daemon.py / lib.sh: MAM_NO_SUBSCRIBER_DAEMON=1 (literal) skips the spawn but still reports a running daemon; cli prints a stderr note, stdout stays empty. - reconcile.sh: python subscriber runs as a waited background child with a TERM/INT trap (exit 143); the MQTT loop exits when the workspace dir is gone or .mam/monitor.lock is no longer the held inode; SIGTERM/SIGINT handled via the stop event so loop_stop/disconnect run; the polling fallback checks the same and sleeps in a waited child. - conftest: sandbox default MAM_NO_SUBSCRIBER_DAEMON=1, per-sandbox reaper finalizer, session-scope leak guard over the pytest basetemp. - tests: 11 lifecycle tests (tests/test_issue19_subscriber_daemon_lifecycle.py); phase-1 watchdog/daemon characterization tests opt back in. - docs: monitor SKILL.md lifecycle contract + pitfalls, create SKILL.md note. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
This commit is contained in:
1 parent
8e7282c948
commit
ce315d8c5e
9 files changed
+536
-12
No files matched your search
@@ -2007,7 +2007,11 @@ _start_watchdog_legacy() {
|
||||
local pid
|
||||
pid=$(pgrep -f "bash $monitor_script --subscribe" || true)
|
||||
|
||||
if [ -z "$pid" ]; then
|
||||
# Issue #19: MAM_NO_SUBSCRIBER_DAEMON=1 reports a running daemon but never
|
||||
# starts one (test sandboxes; externally supervised monitors).
|
||||
if [ -z "$pid" ] && [ "${MAM_NO_SUBSCRIBER_DAEMON:-0}" = "1" ]; then
|
||||
echo "subscriber daemon not started (MAM_NO_SUBSCRIBER_DAEMON=1)" >&2
|
||||
elif [ -z "$pid" ]; then
|
||||
# Start the wildcard monitor subscriber daemon with --idle-timeout 0 (never idle out)
|
||||
# and ensure it runs with $workdir as cwd to anchor relative log paths.
|
||||
local orig_pwd="$PWD"
|
||||
|
||||
@@ -85,6 +85,9 @@ def main(argv: Optional[List[str]] = None) -> int:
|
||||
# Byte-identical to the legacy shell helper's failure path.
|
||||
print("ERROR: monitor script not found: {}".format(exc.args[0]), file=sys.stderr)
|
||||
return 1
|
||||
if not pids and daemon.daemon_disabled():
|
||||
print("subscriber daemon not started ({}=1)".format(daemon.NO_DAEMON_ENV),
|
||||
file=sys.stderr)
|
||||
for pid in pids:
|
||||
print(pid)
|
||||
return 0
|
||||
|
||||
@@ -20,6 +20,17 @@ MONITOR_RELPATH = os.path.join(
|
||||
)
|
||||
LOG_RELPATH = os.path.join(".mam", "multi-agent-mux-monitor.log")
|
||||
|
||||
# Issue #19: opt-out switch for the per-workspace subscriber daemon. Test
|
||||
# sandboxes set it so that `create_session.sh` under test never leaves a
|
||||
# detached `reconcile.sh --subscribe --idle-timeout 0` behind; an operator can
|
||||
# set it too when the monitor is supervised elsewhere. Only the literal "1"
|
||||
# disables the spawn - any other value keeps the normal behaviour.
|
||||
NO_DAEMON_ENV = "MAM_NO_SUBSCRIBER_DAEMON"
|
||||
|
||||
|
||||
def daemon_disabled() -> bool:
|
||||
return os.environ.get(NO_DAEMON_ENV, "") == "1"
|
||||
|
||||
|
||||
def monitor_script_path(workdir: str) -> str:
|
||||
return os.path.join(workdir, MONITOR_RELPATH)
|
||||
@@ -80,12 +91,19 @@ def ensure_subscriber_daemon(job_id: str, workdir: Optional[str] = None) -> List
|
||||
`job_id` is accepted for signature compatibility and is deliberately unused:
|
||||
the daemon is per-workspace (log paths anchor on `workdir`), exactly as in
|
||||
the shell original. Raises FileNotFoundError when the monitor is absent so
|
||||
the CLI can reproduce the legacy stderr line and exit 1.
|
||||
the CLI can reproduce the legacy stderr line and exit 1. With
|
||||
MAM_NO_SUBSCRIBER_DAEMON=1 (issue #19) nothing is spawned and only the PIDs
|
||||
of a daemon that already runs are returned.
|
||||
"""
|
||||
workdir = workdir or os.getcwd()
|
||||
monitor_script = monitor_script_path(workdir)
|
||||
if not os.path.isfile(monitor_script):
|
||||
raise FileNotFoundError(monitor_script)
|
||||
if daemon_disabled():
|
||||
# Issue #19: report an already-running daemon if there is one, but
|
||||
# never start a new one. Callers echo the (possibly empty) PID list.
|
||||
logger.info("%s=1: not starting the subscriber daemon", NO_DAEMON_ENV)
|
||||
return find_running(monitor_script)
|
||||
running = find_running(monitor_script)
|
||||
if running:
|
||||
return running
|
||||
|
||||
@@ -68,6 +68,8 @@ If any check fails → abort with a non-zero exit and report the reason (automat
|
||||
When running multiple agent sessions alongside other workflows (e.g., cmux, background workers, manual herdr sessions), sharing the default herdr server can lead to session name conflicts, monitoring clutter, and accidental destruction of user sessions via global commands.
|
||||
|
||||
To prevent this, you can run this skill inside an **isolated herdr session** using the `HERDR_SESSION_NAME` environment variable or the `--herdr-session <name>` flag (opt-in; alias: `--herdr-server`; legacy env alias: `HERDR_SERVER_NAME`).
|
||||
On success the script also starts the per-workspace monitor daemon through `start_watchdog` (`reconcile.sh --subscribe --idle-timeout 0`, one per workspace, see the lifecycle contract in [multi-agent-mux-monitor/SKILL.md](../multi-agent-mux-monitor/SKILL.md)). Set `MAM_NO_SUBSCRIBER_DAEMON=1` to skip that spawn when the monitor is supervised elsewhere or the workspace is ephemeral (Issue #19); the `watchdog PID:` line is then empty.
|
||||
|
||||
Additionally, you can specify `--herdr-workspace <name>` (default: workspace slug without `mam-` prefix) to name and group the agent panes inside a dedicated Herdr workspace tab in the Herdr runtime (`herdr workspace create --label` / `rename`) as well as recording it in `.mam/agent-sessions.yaml`. Note that `--herdr-workspace` configures the workspace tab label within the session, whereas `--herdr-session` selects the daemon socket itself.
|
||||
|
||||
Under the hood this maps to a real, separate herdr **session** (`herdr --session <name>` — its own socket, its own `agent list`/`workspace list`, completely invisible to the default session and vice versa), not just a workspace label inside the same server. `lib.sh`'s shim bootstraps the named session's server headlessly (`herdr --session <name> server`, backgrounded) the first time it's needed, and scopes every subsequent herdr call to it automatically — this headless bootstrap is what lets it work even when the skill itself is running from inside another herdr-managed pane (a plain interactive `herdr --session <name>` launch is blocked there by herdr's "nested herdr is disabled" guard; headless `server` mode isn't).
|
||||
|
||||
@@ -69,6 +69,23 @@ Run it under whatever supervisor you already use (a dedicated herdr pane, `nohup
|
||||
or a background job). Nothing else needs to be running for the monitor to work —
|
||||
it reconciles YAML ↔ herdr ↔ disk on its own.
|
||||
|
||||
### Daemon lifecycle contract (Issue #19)
|
||||
|
||||
`multi-agent-mux-create` starts one detached `reconcile.sh --subscribe --idle-timeout 0`
|
||||
per workspace through `lib.sh::start_watchdog` (`lib_py.delegation.daemon`). That daemon:
|
||||
|
||||
- is **one per workspace** — `.mam/monitor.lock` (flock) rejects a second instance, and
|
||||
`start_watchdog` returns the running PID instead of spawning again;
|
||||
- **ends on its own only when its workspace is gone** — every wake-up (≤ 5 s on the MQTT
|
||||
loop, every poll on the broker-down fallback) it checks that the workspace directory
|
||||
still exists and that `.mam/monitor.lock` is still the inode it holds. A deleted
|
||||
workspace, a purged `.mam/`, or a re-created lock file makes it disconnect and exit 0;
|
||||
- **stops promptly on `SIGTERM`/`SIGINT`** — the bash wrapper forwards the signal to the
|
||||
python subscriber and exits 143. `SIGKILL` is never required;
|
||||
- is **not started when `MAM_NO_SUBSCRIBER_DAEMON=1`** — the literal `1` only. Use it when
|
||||
the monitor is supervised elsewhere; a daemon that already runs is still reported. Test
|
||||
sandboxes set it by default and additionally reap any daemon anchored in the sandbox.
|
||||
|
||||
The herdr commands the script issues (`herdr agent list`, `herdr agent get <session>`)
|
||||
are real native herdr commands — do not substitute tmux-era names like `herdr ls` /
|
||||
`herdr list-panes` outside a shell that has sourced `.agents/skills/lib.sh`.
|
||||
@@ -141,6 +158,8 @@ YAML: no such session
|
||||
|
||||
- **Don't expect `--once` to stay alive** — it does a single pass and exits. Use `--subscribe` for continuous monitoring.
|
||||
- **`--idle-timeout` defaults to 3600s** — a monitor meant to run indefinitely needs `--idle-timeout 0` explicitly, or it will quietly exit after an hour of broker silence.
|
||||
- **`--idle-timeout 0` is bounded by the workspace, not by time** — the daemon exits when its workspace directory or `.mam/monitor.lock` disappears (Issue #19). Do not delete `.mam/monitor.lock` under a running monitor unless you mean to stop it.
|
||||
- **Spawning from tests or ephemeral checkouts** — set `MAM_NO_SUBSCRIBER_DAEMON=1` so `create_session.sh` does not detach a daemon that outlives the checkout; the pytest sandbox does this for you.
|
||||
- **The poll interval is a default** — `RECONCILE_POLL_INTERVAL` (15s) is what the broker-down fallback uses. A workspace with 5+ agent sessions can bump it to reduce noise.
|
||||
- **Coalesce repeated drifts** — the same drift re-appears on every pass until it is resolved. A caller that acts on `drifts[]` should compare against the previous pass and act only on *new* entries; the script does not deduplicate for you.
|
||||
- **Don't fight the user's explicit action** — if `multi-agent-mux-stop` is mid-flight and the monitor sees the same session in two states within 5s, prefer the user's most recent action. The monitor should not auto-revert a fresh `terminated` to `running` because of a stale `herdr has-session` check.
|
||||
|
||||
@@ -72,12 +72,27 @@ if [ "$SUBSCRIBE" = "1" ]; then
|
||||
# no single interpreter needs both paho and PyYAML (review items 4/5/6).
|
||||
PYBIN="$(_delegate_py_bin)"
|
||||
|
||||
# Issue #19: the subscriber runs as a background child and this shell waits
|
||||
# on it, so SIGTERM/SIGINT reach the daemon promptly. With the child in the
|
||||
# foreground bash deferred the signal until the child exited on its own,
|
||||
# which for --idle-timeout 0 was never - only SIGKILL worked.
|
||||
_sub_pid=""
|
||||
_sub_stop() {
|
||||
trap - TERM INT
|
||||
if [ -n "$_sub_pid" ]; then
|
||||
kill -TERM "$_sub_pid" 2>/dev/null || true
|
||||
wait "$_sub_pid" 2>/dev/null || true
|
||||
fi
|
||||
exit 143
|
||||
}
|
||||
trap _sub_stop TERM INT
|
||||
|
||||
# The MQTT subscribe loop exits 3 to signal "broker unavailable → poll instead".
|
||||
set +e
|
||||
YAML_PATH="$AGENT_SESSIONS_YAML" HOME_DIR="$HOME_DIR" CLAUDE_PROJECT_DIR="$CLAUDE_PROJECT_DIR" LOCAL_BIN="$LOCAL_BIN" \
|
||||
WORKSPACE_ROOT="$WORKSPACE_ROOT" SUB_TIMEOUT="$SUB_TIMEOUT" SUB_IDLE_TIMEOUT="$SUB_IDLE_TIMEOUT" \
|
||||
SKILLS_DIR="$SKILLS_DIR" LIB_SH="$LIB_SH" \
|
||||
"$PYBIN" - <<'PYEOF'
|
||||
"$PYBIN" - <<'PYEOF' &
|
||||
import os, sys, json, time, subprocess
|
||||
|
||||
lib_sh = os.environ.get('LIB_SH', '')
|
||||
@@ -100,6 +115,27 @@ except BlockingIOError:
|
||||
except Exception as e:
|
||||
print(f"MQTT Monitor: failed to acquire monitor lock ({e}). Exiting.", flush=True)
|
||||
sys.exit(1)
|
||||
lock_ino = os.fstat(lock_file.fileno()).st_ino
|
||||
|
||||
|
||||
def _workspace_gone():
|
||||
"""Issue #19: the daemon serves exactly one workspace. Once that workspace
|
||||
is deleted (a purged sandbox, an rm -rf'd project) or its `.mam/monitor.lock`
|
||||
is no longer the inode we hold (a re-created `.mam/`, where a fresh daemon
|
||||
could now take a new lock), there is nothing left to reconcile - exit
|
||||
instead of holding a broker subscription forever."""
|
||||
try:
|
||||
root = os.path.abspath(workspace_root) if workspace_root else os.getcwd()
|
||||
except OSError:
|
||||
return "cwd no longer exists"
|
||||
if not os.path.isdir(root):
|
||||
return f"workspace {root} no longer exists"
|
||||
try:
|
||||
if os.stat(lock_file_path).st_ino != lock_ino:
|
||||
return f"{lock_file_path} was replaced"
|
||||
except OSError:
|
||||
return f"{lock_file_path} no longer exists"
|
||||
return None
|
||||
|
||||
# Locate skills/multi-agent-mux-delegate-job/scripts to import mqtt_common — relative first, then
|
||||
# an upward walk from cwd. No hardcoded absolute path (review item 6).
|
||||
@@ -271,11 +307,28 @@ if not state['connected']:
|
||||
client.loop_stop()
|
||||
sys.exit(3)
|
||||
|
||||
import threading
|
||||
import threading, signal
|
||||
stop_event = threading.Event()
|
||||
state['stop_signal'] = None
|
||||
|
||||
|
||||
def _on_stop_signal(signum, _frame):
|
||||
state['stop_signal'] = signum
|
||||
stop_event.set()
|
||||
|
||||
|
||||
signal.signal(signal.SIGTERM, _on_stop_signal)
|
||||
signal.signal(signal.SIGINT, _on_stop_signal)
|
||||
start = time.time()
|
||||
try:
|
||||
while True:
|
||||
if state['stop_signal'] is not None:
|
||||
print(f"MQTT Monitor: signal {state['stop_signal']} received, exiting", flush=True)
|
||||
break
|
||||
gone = _workspace_gone()
|
||||
if gone:
|
||||
print(f"MQTT Monitor: {gone}; exiting", flush=True)
|
||||
break
|
||||
now = time.time()
|
||||
wall_left = (timeout - (now - start)) if timeout else None
|
||||
idle_left = (idle_timeout - (now - state['last_msg'])) if idle_timeout else None
|
||||
@@ -302,19 +355,30 @@ finally:
|
||||
pass
|
||||
sys.exit(0)
|
||||
PYEOF
|
||||
_sub_pid=$!
|
||||
wait "$_sub_pid"
|
||||
sub_rc=$?
|
||||
_sub_pid=""
|
||||
set -e
|
||||
|
||||
if [ "$sub_rc" = "3" ]; then
|
||||
echo "MQTT Monitor: broker unavailable — falling back to polling (interval ${POLL_INTERVAL}s)" >&2
|
||||
_self="$SKILLS_DIR/multi-agent-mux-monitor/scripts/reconcile.sh"
|
||||
_start=$(date +%s)
|
||||
# Issue #19: same lifecycle as the MQTT loop - stop when the workspace is
|
||||
# gone, and sleep in a waited child so SIGTERM/SIGINT are not deferred.
|
||||
trap 'trap - TERM INT; exit 143' TERM INT
|
||||
while :; do
|
||||
if [ ! -d "$WORKSPACE_ROOT" ] || [ ! -f "$WORKSPACE_ROOT/.mam/monitor.lock" ]; then
|
||||
echo "MQTT Monitor: workspace $WORKSPACE_ROOT or its monitor.lock is gone; exiting" >&2
|
||||
break
|
||||
fi
|
||||
bash "$_self" --once --emit-diff >/dev/null 2>&1 || true
|
||||
if [ "$SUB_TIMEOUT" != "0" ] && [ "$(( $(date +%s) - _start ))" -ge "$SUB_TIMEOUT" ]; then
|
||||
break
|
||||
fi
|
||||
sleep "$POLL_INTERVAL"
|
||||
sleep "$POLL_INTERVAL" &
|
||||
wait $! || true
|
||||
done
|
||||
fi
|
||||
exit 0
|
||||
|
||||
Reference in new issue
Block a user