fix(update): the legacy start_watchdog matches the monitor exactly too; a restart is reported only when the monitor runs (Issue #72, review round 4)
With MAM_DELEGATION_PY=0 the legacy helper still found the monitor with an unanchored pgrep -f, so a process that only named the monitor path was reported as the restarted monitor and none was started. lib.sh _monitor_pids is the shell twin of daemon.find_running; update.sh and post_update.sh check the PID start_watchdog printed with mam_monitor_started before saying "monitor: restarted". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
cf1b693e85
commit
1d63e882bc
5 files changed
+103
-12
No files matched your search
+36
-1
@@ -2235,6 +2235,41 @@ _delegate_publish_event_legacy() {
|
|||||||
"$py_bin" "$pub" --job "$job_id" --event "$event" --detail "$detail" ${extra[@]+"${extra[@]}"} || true
|
"$py_bin" "$pub" --job "$job_id" --event "$event" --detail "$detail" ${extra[@]+"${extra[@]}"} || true
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# _monitor_pids <monitor_script>
|
||||||
|
# Issue #72: PIDs whose command line is exactly `<bash> <monitor_script>
|
||||||
|
# --subscribe[ ...]` - the shell twin of lib_py.delegation.daemon.find_running,
|
||||||
|
# so both start_watchdog branches recognise the same daemons. Not a `pgrep -f`
|
||||||
|
# regex: that also matched another workspace's daemon (an unescaped `.`) and a
|
||||||
|
# process that only names the path in an argument.
|
||||||
|
_monitor_pids() {
|
||||||
|
ps -A -ww -o pid= -o args= | MAM_MONITOR_WANT="$1 --subscribe" awk '
|
||||||
|
{ line = $0; sub(/^ +/, "", line)
|
||||||
|
i = index(line, " "); if (!i) next
|
||||||
|
pid = substr(line, 1, i - 1); args = substr(line, i + 1)
|
||||||
|
j = index(args, " "); if (!j) next
|
||||||
|
n = split(substr(args, 1, j - 1), a0, "/"); rest = substr(args, j + 1)
|
||||||
|
want = ENVIRON["MAM_MONITOR_WANT"]
|
||||||
|
if (a0[n] == "bash" && (rest == want || index(rest, want " ") == 1)) print pid }'
|
||||||
|
}
|
||||||
|
|
||||||
|
# mam_monitor_started <workspace> <pid...>
|
||||||
|
# Issue #72: 0 once one of the PIDs start_watchdog printed is WORKSPACE's
|
||||||
|
# monitor (_monitor_pids), so a restart is only reported when it happened.
|
||||||
|
# Polled for up to 5 s: a daemon just started from the shell carries its
|
||||||
|
# parent's command line until it execs bash.
|
||||||
|
mam_monitor_started() {
|
||||||
|
local script="$1/.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh" n=0 p
|
||||||
|
shift
|
||||||
|
while [ "$n" -lt 50 ]; do
|
||||||
|
for p in $(_monitor_pids "$script"); do
|
||||||
|
case " $* " in *" $p "*) return 0 ;; esac
|
||||||
|
done
|
||||||
|
sleep 0.1
|
||||||
|
n=$((n + 1))
|
||||||
|
done
|
||||||
|
return 1
|
||||||
|
}
|
||||||
|
|
||||||
# start_watchdog <job_id> [workdir]
|
# start_watchdog <job_id> [workdir]
|
||||||
# Spawns a watchdog process to monitor a delegate-job JOB in the background.
|
# Spawns a watchdog process to monitor a delegate-job JOB in the background.
|
||||||
# The watchdog re-spawns the subscriber every 2 minutes (or whatever hard
|
# The watchdog re-spawns the subscriber every 2 minutes (or whatever hard
|
||||||
@@ -2253,7 +2288,7 @@ _start_watchdog_legacy() {
|
|||||||
|
|
||||||
# Check if reconcile.sh --subscribe is already running on this workspace
|
# Check if reconcile.sh --subscribe is already running on this workspace
|
||||||
local pid
|
local pid
|
||||||
pid=$(pgrep -f "bash $monitor_script --subscribe" || true)
|
pid=$(_monitor_pids "$monitor_script" || true)
|
||||||
|
|
||||||
# Issue #19: MAM_NO_SUBSCRIBER_DAEMON=1 reports a running daemon but never
|
# Issue #19: MAM_NO_SUBSCRIBER_DAEMON=1 reports a running daemon but never
|
||||||
# starts one (test sandboxes; externally supervised monitors).
|
# starts one (test sandboxes; externally supervised monitors).
|
||||||
|
|||||||
@@ -42,8 +42,8 @@ def find_running(monitor_script: str) -> List[str]:
|
|||||||
|
|
||||||
A subscriber is a process whose command line is `<bash> <monitor_script>
|
A subscriber is a process whose command line is `<bash> <monitor_script>
|
||||||
--subscribe[ ...]`, argv0 a bash, as both this module and the shell helper
|
--subscribe[ ...]`, argv0 a bash, as both this module and the shell helper
|
||||||
start it. Issue #72: matched exactly, not as the shell helper's `pgrep -f`
|
start it. Issue #72: matched exactly (lib.sh _monitor_pids is the shell
|
||||||
regex, because the update skill signals what this returns - a daemon of
|
twin the legacy helper uses), not as a `pgrep -f` regex, because the update skill signals what this returns - a daemon of
|
||||||
another workspace the unescaped path also matched, or a process that only
|
another workspace the unescaped path also matched, or a process that only
|
||||||
names the path in an argument, is not this workspace's monitor. Multiple
|
names the path in an argument, is not this workspace's monitor. Multiple
|
||||||
matches are returned in full: the legacy helper echoed every line and
|
matches are returned in full: the legacy helper echoed every line and
|
||||||
|
|||||||
@@ -37,7 +37,8 @@ fi
|
|||||||
if [ "$MON" = "1" ]; then
|
if [ "$MON" = "1" ]; then
|
||||||
if [ "${MAM_NO_SUBSCRIBER_DAEMON:-0}" = "1" ]; then
|
if [ "${MAM_NO_SUBSCRIBER_DAEMON:-0}" = "1" ]; then
|
||||||
echo "monitor: not restarted (MAM_NO_SUBSCRIBER_DAEMON=1)"
|
echo "monitor: not restarted (MAM_NO_SUBSCRIBER_DAEMON=1)"
|
||||||
elif pid="$(start_watchdog update "$WS")"; then
|
# shellcheck disable=SC2086
|
||||||
|
elif pid="$(start_watchdog update "$WS")" && mam_monitor_started "$WS" $pid; then
|
||||||
echo "monitor: restarted (PID $pid)"
|
echo "monitor: restarted (PID $pid)"
|
||||||
else
|
else
|
||||||
echo "ERROR: could not restart the monitor daemon" >&2
|
echo "ERROR: could not restart the monitor daemon" >&2
|
||||||
|
|||||||
@@ -104,6 +104,8 @@ _restart_monitor() {
|
|||||||
fi
|
fi
|
||||||
local pid
|
local pid
|
||||||
pid=$(start_watchdog update "$WS") || { echo "ERROR: could not restart the monitor daemon" >&2; return 1; }
|
pid=$(start_watchdog update "$WS") || { echo "ERROR: could not restart the monitor daemon" >&2; return 1; }
|
||||||
|
# shellcheck disable=SC2086
|
||||||
|
mam_monitor_started "$WS" $pid || { echo "ERROR: could not restart the monitor daemon (PID ${pid:-none} is not its monitor)" >&2; return 1; }
|
||||||
echo "monitor: restarted (PID $pid)"
|
echo "monitor: restarted (PID $pid)"
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -146,9 +146,12 @@ class Box:
|
|||||||
_until(lambda: _lock_held(self.mam / "monitor.lock"))
|
_until(lambda: _lock_held(self.mam / "monitor.lock"))
|
||||||
|
|
||||||
def monitor_pids(self):
|
def monitor_pids(self):
|
||||||
r = subprocess.run(["pgrep", "-f", "bash %s --subscribe" % (self.ws / MONITOR_REL)],
|
return _exact_monitor_pids(self.ws / MONITOR_REL)
|
||||||
capture_output=True, text=True)
|
|
||||||
return r.stdout.split()
|
def decoy(self):
|
||||||
|
"""A process that only names this workspace's monitor in an argument."""
|
||||||
|
return self.helper("/bin/bash", "-c", "sleep 120 & wait $!",
|
||||||
|
"bash %s --subscribe" % (self.ws / MONITOR_REL))
|
||||||
|
|
||||||
def monitor_starts(self):
|
def monitor_starts(self):
|
||||||
p = self.mam / "stub-monitor.log"
|
p = self.mam / "stub-monitor.log"
|
||||||
@@ -164,6 +167,20 @@ def _until(cond, timeout=15.0):
|
|||||||
raise AssertionError("condition not met within %ss" % timeout)
|
raise AssertionError("condition not met within %ss" % timeout)
|
||||||
|
|
||||||
|
|
||||||
|
def _exact_monitor_pids(script) -> list:
|
||||||
|
"""`bash <script> --subscribe ...` processes, judged here independently of
|
||||||
|
the code under test (a `pgrep -f` would also count a decoy)."""
|
||||||
|
out = subprocess.run(["ps", "-A", "-ww", "-o", "pid=", "-o", "args="], capture_output=True, text=True).stdout
|
||||||
|
want = "%s --subscribe" % script
|
||||||
|
pids = []
|
||||||
|
for line in out.splitlines():
|
||||||
|
pid, _, args = line.strip().partition(" ")
|
||||||
|
argv0, _, rest = args.partition(" ")
|
||||||
|
if os.path.basename(argv0) == "bash" and (rest == want or rest.startswith(want + " ")):
|
||||||
|
pids.append(pid)
|
||||||
|
return pids
|
||||||
|
|
||||||
|
|
||||||
def _lock_held(path: Path) -> bool:
|
def _lock_held(path: Path) -> bool:
|
||||||
if not path.exists():
|
if not path.exists():
|
||||||
return False
|
return False
|
||||||
@@ -603,11 +620,13 @@ def test_a_monitor_lock_held_by_an_unknown_process_refuses(box):
|
|||||||
assert_refused_unchanged(box, before, res, "monitor.lock is held by a monitor the skills did not start")
|
assert_refused_unchanged(box, before, res, "monitor.lock is held by a monitor the skills did not start")
|
||||||
|
|
||||||
|
|
||||||
def test_only_this_workspaces_exact_monitor_command_line_is_its_monitor(tmp_path):
|
@pytest.mark.parametrize("finder", ["python", "shell"])
|
||||||
"""The update signals what `lib_py.update monitor` returns: `bash <this
|
def test_only_this_workspaces_exact_monitor_command_line_is_its_monitor(tmp_path, finder):
|
||||||
workspace's reconcile.sh> --subscribe ...` only - not another workspace's
|
"""The update signals what `lib_py.update monitor` returns, and the legacy
|
||||||
daemon the path matched as a regex (ws.a, wsXa), nor a process that names
|
start_watchdog (MAM_DELEGATION_PY=0) reuses what lib.sh _monitor_pids finds:
|
||||||
the path in an argument. A path with a space is still found."""
|
`bash <this workspace's reconcile.sh> --subscribe ...` only - not another
|
||||||
|
workspace's daemon the path matched as a regex (ws.a, wsXa), nor a process
|
||||||
|
that names the path in an argument. A path with a space is still found."""
|
||||||
root = Path(os.path.realpath(tmp_path))
|
root = Path(os.path.realpath(tmp_path))
|
||||||
procs = []
|
procs = []
|
||||||
|
|
||||||
@@ -620,6 +639,12 @@ def test_only_this_workspaces_exact_monitor_command_line_is_its_monitor(tmp_path
|
|||||||
return str(procs[-1].pid)
|
return str(procs[-1].pid)
|
||||||
|
|
||||||
def monitor(ws):
|
def monitor(ws):
|
||||||
|
if finder == "shell":
|
||||||
|
res = subprocess.run(["bash", "-c", 'source "$1" && _monitor_pids "$2"', "_",
|
||||||
|
str(REPO_ROOT / ".agents" / "skills" / "lib.sh"), str(ws / MONITOR_REL)],
|
||||||
|
env=dict(_clean_env(), WORKSPACE_ROOT=str(root)), capture_output=True, text=True)
|
||||||
|
assert res.returncode == 0, res.stderr
|
||||||
|
return res.stdout.split()
|
||||||
res = subprocess.run([sys.executable, "-m", "lib_py.update", "monitor", "--workspace", str(ws)],
|
res = subprocess.run([sys.executable, "-m", "lib_py.update", "monitor", "--workspace", str(ws)],
|
||||||
cwd=str(REPO_ROOT / ".agents" / "skills"), capture_output=True, text=True)
|
cwd=str(REPO_ROOT / ".agents" / "skills"), capture_output=True, text=True)
|
||||||
assert res.returncode == 0, res.stderr
|
assert res.returncode == 0, res.stderr
|
||||||
@@ -643,6 +668,34 @@ def test_only_this_workspaces_exact_monitor_command_line_is_its_monitor(tmp_path
|
|||||||
p.wait()
|
p.wait()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("case", ["installed", "killed", "rolled-back"])
|
||||||
|
@pytest.mark.parametrize("mode", ["0", "1"])
|
||||||
|
def test_every_restart_starts_a_real_monitor_in_both_delegation_modes(box, mode, case):
|
||||||
|
"""MAM_DELEGATION_PY=0 keeps the legacy start_watchdog, the switch the
|
||||||
|
delegate skill documents. Its restart, after an install, after a stop
|
||||||
|
timeout or after a rollback, must start this workspace's monitor again and
|
||||||
|
not report a process that only names the monitor path in an argument."""
|
||||||
|
env = {"MAM_DELEGATION_PY": mode}
|
||||||
|
box.stub_monitor()
|
||||||
|
box.start_monitor(env=dict(env, STUB_MONITOR_IGNORE_TERM="1") if case == "killed" else env)
|
||||||
|
first = set(box.monitor_pids())
|
||||||
|
decoy = box.decoy()
|
||||||
|
_until(lambda: str(decoy.pid) in subprocess.run(["ps", "-A", "-ww", "-o", "pid=", "-o", "args="],
|
||||||
|
capture_output=True, text=True).stdout)
|
||||||
|
if case == "killed":
|
||||||
|
env["MAM_UPDATE_MONITOR_STOP_S"] = "1"
|
||||||
|
if case == "rolled-back":
|
||||||
|
box.installer("exit 7\n")
|
||||||
|
res = box.update(env=env)
|
||||||
|
assert res.returncode == (0 if case == "installed" else 1), res.stdout + res.stderr
|
||||||
|
assert {"installed": "monitor: restarted", "killed": "it was killed and is started again",
|
||||||
|
"rolled-back": "rolled back"}[case] in res.stdout + res.stderr
|
||||||
|
assert "PID %s)" % decoy.pid not in res.stdout, "the decoy is reported as the monitor"
|
||||||
|
_until(lambda: bool(box.monitor_pids()))
|
||||||
|
assert not (set(box.monitor_pids()) & first) and box.monitor_starts() == 2
|
||||||
|
assert decoy.poll() is None, "the decoy was not signalled"
|
||||||
|
|
||||||
|
|
||||||
# --- rollback (decisions 3, 10; U4) --------------------------------------------------
|
# --- rollback (decisions 3, 10; U4) --------------------------------------------------
|
||||||
|
|
||||||
FAILING_INSTALL = """
|
FAILING_INSTALL = """
|
||||||
|
|||||||
Reference in new issue
Block a user