From cf1b693e854aeb9bd98f2d6195e871e4ef9ce34f Mon Sep 17 00:00:00 2001 From: Godopu Date: Wed, 28 Oct 2026 02:28:55 +0900 Subject: [PATCH] fix(update): the monitor is this workspace's exact command line; create/resume take the pane workspace's admission; refusal snapshots compare the registry by rows (Issue #72, review round 3) - daemon.find_running matches ` --subscribe[ ...]` exactly instead of an unanchored, unescaped pgrep regex: another workspace's daemon (ws.a vs wsXa) or a process naming the path in an argument is no longer signalled by the update. update.sh re-checks which PIDs are still the monitor before SIGKILL. - create and resume (spawn path) take the update admission of every installed workspace at or above the pane cwd, not only the control root's (lib.sh mam_update_admit_at, lib_py.update target-workspaces --cwd); the ~/.local/bin wrapper child drops all of them. - tests: snapshot() skips the SQLite -wal/-shm and compares the DB by its rows (macOS keeps the sidecars and a read rewrites -shm); regression tests for the monitor match and for create/resume in another installed workspace (marker and lock wait); evidence line refs shifted. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/lib.sh | 33 +++++++++- .agents/skills/lib_py/delegation/daemon.py | 33 +++++++--- .agents/skills/lib_py/update.py | 22 ++++++- .../scripts/create_session.sh | 11 ++-- .../scripts/resume_session.sh | 9 +-- .../multi-agent-mux-update/scripts/update.sh | 8 +++ tests/data/skill_code_blocks.json | 20 +++---- tests/test_issue72_admission.py | 45 ++++++++++++++ tests/test_issue72_update.py | 60 ++++++++++++++++++- 9 files changed, 209 insertions(+), 32 deletions(-) diff --git a/.agents/skills/lib.sh b/.agents/skills/lib.sh index 85be750..7b862ff 100644 --- a/.agents/skills/lib.sh +++ b/.agents/skills/lib.sh @@ -3266,7 +3266,8 @@ mam_control_plane_prefix() { # # mam_update_admit [workspace [fd]]: rc 0 admitted (fd 7, or the given one, # held unless the workspace has no .mam), 1 refused (stderr). Delegate submit -# also takes the workspaces its target is in on fds 20 and up. +# also takes the workspaces its target is in on fds 20 and up, create and +# resume those their pane's cwd is in (mam_update_admit_at). mam_update_admit() { local root="${1:-$WORKSPACE_ROOT}" fd="${2:-7}" local lock="$root/.mam/update-admission.lock" marker="$root/.mam/update-in-progress" st @@ -3305,3 +3306,33 @@ mam_update_admit() { mam_update_admit_release() { eval "exec ${1:-7}>&-" } + +# mam_update_admit_at : what create and resume take for an agent they +# start in - mam_update_admit for the control root (fd 7) and for every +# other installed workspace at or above (fds 20 and up), whose update +# counts that agent (decision 1). rc 1 refused, nothing held. +# mam_update_admit_release_all drops them all; a long-lived child started +# while they are held runs it first. +MAM_UPDATE_ADMIT_FDS="" +mam_update_admit_at() { + local roots root fd=20 + roots="$(PYTHONPATH="$SKILL_DIR:${PYTHONPATH:-}" "$(_delegate_py_bin)" -m lib_py.update target-workspaces \ + --workspace "$WORKSPACE_ROOT" --cwd "$1")" \ + || { echo "ERROR: cannot resolve the installed workspaces at or above $1" >&2; return 1; } + mam_update_admit || return 1 + while IFS= read -r root; do + [ -n "$root" ] || continue + if ! mam_update_admit "$root" "$fd"; then + mam_update_admit_release_all + return 1 + fi + MAM_UPDATE_ADMIT_FDS="$MAM_UPDATE_ADMIT_FDS $fd"; fd=$((fd + 1)) + done <<< "$roots" +} + +mam_update_admit_release_all() { + local fd + mam_update_admit_release + for fd in $MAM_UPDATE_ADMIT_FDS; do mam_update_admit_release "$fd"; done + MAM_UPDATE_ADMIT_FDS="" +} diff --git a/.agents/skills/lib_py/delegation/daemon.py b/.agents/skills/lib_py/delegation/daemon.py index 630ee9d..96e51de 100644 --- a/.agents/skills/lib_py/delegation/daemon.py +++ b/.agents/skills/lib_py/delegation/daemon.py @@ -2,7 +2,8 @@ Behaviour is byte-for-byte compatible with the shell original (issue #5 phase 1, D5): same monitor path, same stderr wording and exit status when it is missing, -same `pgrep -f` pre-check pattern, same log file, same "print the PID" contract. +the same daemons found by the pre-check (matched exactly since Issue #72, see +find_running), same log file, same "print the PID" contract. Callers capture stdout with $( ), so nothing else may be printed there. """ from __future__ import annotations @@ -39,20 +40,34 @@ def monitor_script_path(workdir: str) -> str: def find_running(monitor_script: str) -> List[str]: """PIDs of subscribers already running for this monitor script. - Uses the exact `pgrep -f` pattern the shell helper used, so a daemon started - by either implementation is found by the other. Multiple matches are returned - in full: the legacy helper echoed every line and callers only ever echo it - back, so trimming here would be a silent behaviour change. + A subscriber is a process whose command line is ` + --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` + regex, because the update skill signals what this returns - a daemon of + 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 + matches are returned in full: the legacy helper echoed every line and + callers only ever echo it back, so trimming here would be a silent + behaviour change. """ try: res = subprocess.run( - ["pgrep", "-f", "bash {} --subscribe".format(monitor_script)], + ["ps", "-A", "-ww", "-o", "pid=", "-o", "args="], capture_output=True, text=True, ) - except OSError as exc: # pragma: no cover - pgrep missing - logger.warning("pgrep unavailable: %s", exc) + except OSError as exc: # pragma: no cover - ps missing + logger.warning("ps unavailable: %s", exc) return [] - return [line for line in res.stdout.split() if line.strip()] + want = "{} --subscribe".format(monitor_script) + pids = [] + for line in res.stdout.splitlines(): + parts = line.strip().split(" ", 1) + if len(parts) != 2: + continue + argv0, _, rest = parts[1].partition(" ") + if os.path.basename(argv0) == "bash" and (rest == want or rest.startswith(want + " ")): + pids.append(parts[0]) + return pids def _ignore_sighup(): # pragma: no cover - runs in the forked child diff --git a/.agents/skills/lib_py/update.py b/.agents/skills/lib_py/update.py index a88796a..daa815c 100644 --- a/.agents/skills/lib_py/update.py +++ b/.agents/skills/lib_py/update.py @@ -23,6 +23,9 @@ this module what needs parsing. Standard library only, Python 3.9. the live agent's on server V) and its row's control_plane workspace_root. Nothing for a non-herdr target. rc 1: herdr cannot answer for a rowless target. + python -m lib_py.update target-workspaces --workspace W --cwd C + The same for an agent create or resume starts in C: every installed + workspace at or above C other than W. python -m lib_py.update backup-registry --workspace W --dest D Copies the registry into D under the registry's own write lock (agent-sessions.db.lock, as atomic_yaml takes it): the DB through the @@ -296,6 +299,12 @@ def target_workspaces(workspace: str, state: dict, agent_session: str, server: s if a.get("name") in (name, sanitize_herdr_agent_name(name)): cwds += [a.get("cwd") or "", a.get("foreground_cwd") or ""] break + return _others(workspace, out + installed_above(cwds)) + + +def installed_above(cwds: Iterable[str]) -> List[str]: + """Every installed workspace at or above each cwd.""" + out = [] for cwd in cwds: d = _real(cwd) if cwd else "" while d: @@ -304,8 +313,12 @@ def target_workspaces(workspace: str, state: dict, agent_session: str, server: s if os.path.dirname(d) == d: break d = os.path.dirname(d) + return out + + +def _others(workspace: str, dirs: List[str]) -> List[str]: me = _real(workspace) - return [d for i, d in enumerate(out) if d != me and d not in out[:i]] + return [d for i, d in enumerate(dirs) if d != me and d not in dirs[:i]] # --------------------------------------------------------------------------- monitor @@ -367,7 +380,8 @@ def main(argv: Optional[List[str]] = None) -> int: s.add_argument("--workspace", required=True) s = sub.add_parser("target-workspaces") s.add_argument("--workspace", required=True) - s.add_argument("--agent-session", required=True) + s.add_argument("--agent-session", default="") + s.add_argument("--cwd", default="") s.add_argument("--server", default="default") s.add_argument("--herdr", default="") s = sub.add_parser("backup-registry") @@ -385,6 +399,10 @@ def main(argv: Optional[List[str]] = None) -> int: for line in lines: print(line) return 1 if lines else 0 + if a.cmd == "target-workspaces" and a.cwd: + for root in _others(ws, installed_above([a.cwd])): + print(root) + return 0 if a.cmd == "target-workspaces": try: state = _state() diff --git a/.agents/skills/multi-agent-mux-create/scripts/create_session.sh b/.agents/skills/multi-agent-mux-create/scripts/create_session.sh index 5899963..daac476 100755 --- a/.agents/skills/multi-agent-mux-create/scripts/create_session.sh +++ b/.agents/skills/multi-agent-mux-create/scripts/create_session.sh @@ -260,7 +260,7 @@ spawn() { SESSION_UUID="" CMD_FULL="$("$(_delegate_py_bin)" -m lib_py.agents spawn-spec "$AGENT" "$RESOLVED_BIN" "" "true" 2>/dev/null || echo "${RESOLVED_BIN} --dangerously-skip-permissions")" [ -z "$CP_PREFIX" ] || echo "WARNING: the ~/.local/bin wrapper path bypasses herdr; the control-plane binding is recorded but not forwarded" >&2 - nohup "$WRAPPER" >/dev/null 2>&1 7>&- & + ( mam_update_admit_release_all; exec nohup "$WRAPPER" >/dev/null 2>&1 ) & disown else HERDR_SESSION_NAME="$HERDR_SESSION_NAME" _herdr new-session -d -s "$SESSION_NAME" -x 140 -y 40 -c "$WORKSPACE" "$CP_PREFIX$CMD_FULL" @@ -285,9 +285,10 @@ if [ "$DRY_RUN" = "1" ]; then fi # Issue #63: a same-cwd session of this post-discovered agent records its id first (lib.sh mam_id_order_wait). mam_id_order_wait "$WORKSPACE" "$AGENT" "$SESSION_NAME" || exit 1 -# Issue #72: no new session while an update runs; held until the row is written -# (lib.sh mam_update_admit). -mam_update_admit || exit 1 +# Issue #72: no new session while an update of the control root or of an +# installed workspace the pane's cwd is in runs; held until the row is written +# (lib.sh mam_update_admit_at). +mam_update_admit_at "$WORKSPACE" || exit 1 HERDR_EPOCH=$(date +%s) NOW_ISO=$(date -u +'%Y-%m-%dT%H:%M:%SZ') spawn @@ -641,7 +642,7 @@ snap['cwd'] = os.environ['PANE_CWD'] print(f"appended: {name}", flush=True) PYEOF ROW_APPENDED=1 -mam_update_admit_release +mam_update_admit_release_all echo echo "=== created ===" diff --git a/.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh b/.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh index 0668256..df2ca1c 100755 --- a/.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh +++ b/.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh @@ -254,9 +254,10 @@ if [ "${DRY_RUN:-0}" = "1" ]; then exit 0 fi -# Issue #72: no new session while an update runs; held until the row is written -# (lib.sh mam_update_admit). -mam_update_admit || exit 1 +# Issue #72: no new session while an update of the control root or of an +# installed workspace the pane's cwd is in runs; held until the row is written +# (lib.sh mam_update_admit_at). +mam_update_admit_at "$WORKSPACE" || exit 1 # Issue #63: wait if another same-cwd session of this post-discovered agent # is still being created or has not recorded its id yet (lib.sh mam_id_order_wait). @@ -294,7 +295,7 @@ CONTROL_PLANE_JSON="$CP_JSON" bash "$(dirname "${BASH_SOURCE[0]}")/update_yaml_r --herdr-session "$HERDR_SESSION_NAME" --cmd-full "$CMD_FULL" \ ${DROP_FLAG:+$DROP_FLAG} \ ${HERDR_WORKSPACE_OPT:+--herdr-workspace "$HERDR_WORKSPACE_OPT"} -mam_update_admit_release +mam_update_admit_release_all _ensure_monitor_daemon echo "Successfully resumed $SESSION_NAME ($AGENT)" diff --git a/.agents/skills/multi-agent-mux-update/scripts/update.sh b/.agents/skills/multi-agent-mux-update/scripts/update.sh index 652f331..60da74e 100755 --- a/.agents/skills/multi-agent-mux-update/scripts/update.sh +++ b/.agents/skills/multi-agent-mux-update/scripts/update.sh @@ -185,6 +185,14 @@ if [ -n "$MON_PIDS" ]; then echo "monitor: stopping the daemon (PID $MON_PIDS)" kill -TERM "$@" 2>/dev/null || true if ! _wait_monitor_gone "$(mam_int_knob MAM_UPDATE_MONITOR_STOP_S 30 1)" "$@"; then + # SIGKILL only what is still this workspace's monitor: a PID that ended + # during the wait may already be another process's. + still="" + for p in $(_monitor_json | _json_field pids); do + case " $* " in *" $p "*) still="$still $p" ;; esac + done + # shellcheck disable=SC2086 + set -- $still for p in "$@"; do kids="$kids $(pgrep -P "$p" 2>/dev/null | tr '\n' ' ' || true)"; done echo "WARNING: the monitor did not stop within MAM_UPDATE_MONITOR_STOP_S; sending SIGKILL to $* $kids" >&2 # shellcheck disable=SC2086 diff --git a/tests/data/skill_code_blocks.json b/tests/data/skill_code_blocks.json index b18ff47..0d4a81d 100644 --- a/tests/data/skill_code_blocks.json +++ b/tests/data/skill_code_blocks.json @@ -11,7 +11,7 @@ "kind": "example", "evidence": [ "multi-agent-mux-create/scripts/create_session.sh:99", - "multi-agent-mux-create/scripts/create_session.sh:359" + "multi-agent-mux-create/scripts/create_session.sh:360" ], "mismatch": null, "note": "HERDR_SESSION_NAME environment example" @@ -67,7 +67,7 @@ "class": "c", "kind": "example", "evidence": [ - "multi-agent-mux-create/scripts/create_session.sh:517-544" + "multi-agent-mux-create/scripts/create_session.sh:518-545" ], "mismatch": null, "note": "Output format: the row create_session.sh writes" @@ -492,9 +492,9 @@ "multi-agent-mux-resume/scripts/resume_session.sh:133-144", "multi-agent-mux-resume/scripts/resume_session.sh:147-170", "multi-agent-mux-resume/scripts/resume_session.sh:188-213", - "multi-agent-mux-resume/scripts/resume_session.sh:265", - "multi-agent-mux-resume/scripts/resume_session.sh:269", - "multi-agent-mux-resume/scripts/resume_session.sh:275-278", + "multi-agent-mux-resume/scripts/resume_session.sh:266", + "multi-agent-mux-resume/scripts/resume_session.sh:270", + "multi-agent-mux-resume/scripts/resume_session.sh:276-279", "lib_py/agents/adapters/claude.py:109" ], "mismatch": [ @@ -561,8 +561,8 @@ "multi-agent-mux-create/scripts/create_session.sh:166-168", "multi-agent-mux-create/scripts/create_session.sh:207", "multi-agent-mux-create/scripts/create_session.sh:259-274", - "multi-agent-mux-create/scripts/create_session.sh:364-401", - "multi-agent-mux-create/scripts/create_session.sh:413-415" + "multi-agent-mux-create/scripts/create_session.sh:365-402", + "multi-agent-mux-create/scripts/create_session.sh:414-416" ], "mismatch": [ "C1: an existing session is exit 1 in the doc, exit 3 in the script", @@ -582,7 +582,7 @@ "base_sha": "f71be0f26fb8", "class": "a", "evidence": [ - "multi-agent-mux-create/scripts/create_session.sh:489-607" + "multi-agent-mux-create/scripts/create_session.sh:490-608" ], "mismatch": [], "replacement": "bash .agents/skills/multi-agent-mux-create/scripts/create_session.sh --workspace --agent --role ", @@ -633,14 +633,14 @@ "other" ], "passthrough": { - "4": "multi-agent-mux-create/scripts/create_session.sh:549" + "4": "multi-agent-mux-create/scripts/create_session.sh:550" } } }, "prose_mismatches": [ "C4: create named a nonexistent `agent-sessions-yaml-edit` script; removed (create_session.sh writes the row)", "C5: create's cmd_full table (claude = `claude`) disagreed with adapter spawn-spec; replaced by \"decided by spawn-spec\"", - "C6: create_session.sh header lists exit 4 \"YAML append failure\"; there is no literal `exit 4`, but the YAML write raises SystemExit(4) for a duplicate running row (create_session.sh:515) and it passes through, so 4 IS a real exit code (the plan assumed it was not)", + "C6: create_session.sh header lists exit 4 \"YAML append failure\"; there is no literal `exit 4`, but the YAML write raises SystemExit(4) for a duplicate running row (create_session.sh:516) and it passes through, so 4 IS a real exit code (the plan assumed it was not)", "R6: resume's \"The script above re-passes them\" and \"as done in Workflow above\" pointed at the removed block; reworded (same for create's Verification note)", "R7 (review 08e1f1b2): stop's table said exit 1 is always pre-stop; the registry update after the kill chain also raises SystemExit(1) (stop_session.sh:310) when the row disappeared" ], diff --git a/tests/test_issue72_admission.py b/tests/test_issue72_admission.py index 0e573c1..47d7322 100644 --- a/tests/test_issue72_admission.py +++ b/tests/test_issue72_admission.py @@ -230,6 +230,51 @@ def test_delegate_submit_waits_for_the_target_workspace_admission_lock(mam_sandb assert _no_job(mam_sandbox) +def _start_in(sandbox, entry, b, env=None): + """create or resume (spawn path) of an agent whose pane cwd is B/sub.""" + if entry == "create": + argv = ["bash", _script(sandbox, "multi-agent-mux-create/scripts/create_session.sh"), + "--workspace", str(b / "sub"), "--agent", "claude", "--role", "creator", + "--session", "gated-in-b", "--no-onboard"] + else: + _seed(sandbox, [dict(_row("gated-in-b", status="stopped", agent="agy"), + pane={"cwd": str(b / "sub"), "cmd": "agy"})]) + argv = ["bash", _script(sandbox, "multi-agent-mux-resume/scripts/resume_session.sh"), + "--workspace", str(b / "sub"), "--agent", "agy", "--session", "gated-in-b"] + return _run(sandbox, argv, env=env, timeout=300) + + +@pytest.mark.parametrize("entry", ["create", "resume"]) +def test_create_and_resume_take_the_pane_workspace_marker(mam_sandbox, mock_herdr, mock_agents, marker, + other_ws, entry): + """An agent started in an installed workspace B other than the control root + is one B's update counts, so B's marker refuses it too.""" + marker(True, other_ws) + res = _start_in(mam_sandbox, entry, other_ws) + assert res.returncode != 0, res.stdout + res.stderr + assert "an update of %s is in progress" % other_ws in res.stderr, res.stdout + res.stderr + assert json.loads(Path(mock_herdr).read_text())["agents"] == {}, "nothing was spawned" + + +@pytest.mark.parametrize("entry", ["create", "resume"]) +def test_create_and_resume_wait_for_the_pane_workspace_admission_lock(mam_sandbox, mock_herdr, mock_agents, + other_ws, entry): + holder = subprocess.Popen([sys.executable, "-c", "import fcntl, sys, time\n" + "f = open(sys.argv[1], 'a'); fcntl.flock(f, fcntl.LOCK_EX)\n" + "print('locked', flush=True); time.sleep(120)", + str(other_ws / ".mam" / "update-admission.lock")], + stdout=subprocess.PIPE, text=True, start_new_session=True) + try: + assert holder.stdout.readline().strip() == "locked" + res = _start_in(mam_sandbox, entry, other_ws, env={"MAM_UPDATE_ADMISSION_WAIT_S": "1"}) + finally: + holder.kill() + holder.wait() + assert res.returncode != 0, res.stdout + res.stderr + assert "an update of %s holds its admission lock" % other_ws in res.stderr, res.stdout + res.stderr + assert json.loads(Path(mock_herdr).read_text())["agents"] == {}, "nothing was spawned" + + @pytest.mark.parametrize("purge", [False, True]) def test_stop_refuses_and_changes_nothing(mam_sandbox, mock_herdr, marker, purge): _seed(mam_sandbox, [_row("gated-stop")]) diff --git a/tests/test_issue72_update.py b/tests/test_issue72_update.py index 552ed83..dfcd00f 100644 --- a/tests/test_issue72_update.py +++ b/tests/test_issue72_update.py @@ -199,7 +199,22 @@ def box(template, tmp_path): # --- helpers --------------------------------------------------------------------- -SKIP_SNAPSHOT = ("shim/", "update-admission.lock", "agent-sessions.db.lock", "monitor.lock", "stub-monitor.log") +SKIP_SNAPSHOT = ("shim/", "update-admission.lock", "agent-sessions.db.lock", "monitor.lock", "stub-monitor.log", + "agent-sessions.db-wal", "agent-sessions.db-shm") + + +def _db_content(p: Path) -> str: + """The registry DB by its rows: on macOS SQLite keeps -wal/-shm after close and + a read rewrites -shm, and a checkpoint moves the same rows between -wal and + the file, so neither file's bytes say whether the data changed. A file that + is not a database is compared by its bytes.""" + conn = sqlite3.connect(str(p)) + try: + return "DB:" + hashlib.sha256("\n".join(conn.iterdump()).encode()).hexdigest() + except sqlite3.DatabaseError: + return hashlib.sha256(p.read_bytes()).hexdigest() + finally: + conn.close() def snapshot(ws: Path) -> dict: @@ -211,6 +226,9 @@ def snapshot(ws: Path) -> dict: rel = str(p.relative_to(ws)) if rel.startswith(".mam/") and any(s in rel for s in SKIP_SNAPSHOT): continue + if rel == ".mam/agent-sessions.db": + out[rel] = _db_content(p) + continue out[rel] = "L:" + os.readlink(p) if p.is_symlink() else hashlib.sha256(p.read_bytes()).hexdigest() return out @@ -585,6 +603,46 @@ 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") +def test_only_this_workspaces_exact_monitor_command_line_is_its_monitor(tmp_path): + """The update signals what `lib_py.update monitor` returns: `bash --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)) + procs = [] + + def daemon_in(ws): + script = ws / MONITOR_REL + script.parent.mkdir(parents=True) + script.write_text("sleep 120 & wait $!\n") + procs.append(subprocess.Popen(["bash", str(script), "--subscribe", "--idle-timeout", "0"], + start_new_session=True)) + return str(procs[-1].pid) + + def monitor(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) + assert res.returncode == 0, res.stderr + return json.loads(res.stdout)["pids"] + + try: + a, other, spaced = root / "ws.a", root / "wsXa", root / "w s" + a.mkdir() + daemon_in(other) + procs.append(subprocess.Popen(["/bin/bash", "-c", "sleep 120 & wait $!", + "bash %s --subscribe" % (a / MONITOR_REL)], start_new_session=True)) + pid = daemon_in(spaced) + _until(lambda: monitor(spaced) == [pid]) + assert monitor(a) == [] + finally: + for p in procs: + try: + os.killpg(p.pid, signal.SIGKILL) + except OSError: + pass + p.wait() + + # --- rollback (decisions 3, 10; U4) -------------------------------------------------- FAILING_INSTALL = """