fix(issue-19): match the daemon's whole script path so spaced sandbox paths are reaped

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

- conftest `_is_subscriber_daemon_argv`: `ps -o args=` joins argv unquoted,
  so the script path is taken from the shell word up to the first
  `/reconcile.sh --subscribe` instead of as one whitespace token. A sandbox
  or --basetemp path with a space no longer hides its daemon from the
  fixture reaper and the session leak guard. A candidate with whitespace
  counts only when it names an existing file, so a bash wrapper under the
  root whose later argv carries the words is still excluded.
- tests: a whitespace-path selector regression and an owned live-process
  cleanup check (both fail on c096cb4); the R-2 test also pins the bash
  wrapper exclusion.
- docs: the polling fallback's signal handling is stated precisely in
  monitor SKILL.md and VERSIONS.md (a running --once pass finishes first,
  the interval's sleep child ends on its own); create SKILL.md says an
  already-running daemon's PID is still reported under the opt-out;
  VERSIONS selector wording and test count (14).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HCGpJXhSZasJLU38ksDghe
This commit is contained in:
GodopuandClaude Opus 5 committed 2026-09-13 16:36:16 +09:00
1 parent c096cb455b
commit 8f9745b128
5 files changed
+95 -14

No files matched your search

@@ -68,7 +68,7 @@ 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.
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 unless a daemon already runs for the workspace, whose PID is still reported.
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.
@@ -84,7 +84,10 @@ per workspace through `lib.sh::start_watchdog` (`lib_py.delegation.daemon`). Tha
the lock (the python subscriber released it on exit), so while polling it does not
exclude a second daemon, as before;
- **stops promptly on `SIGTERM`/`SIGINT`** — the bash wrapper forwards the signal to the
python subscriber and exits 143. `SIGKILL` is never required;
python subscriber and exits 143. In the broker-down polling fallback the signal cuts the
interval wait short, but a reconcile pass (`--once`) already running finishes first, and
the interval's `sleep` child is left to end on its own (≤ `RECONCILE_POLL_INTERVAL`).
`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.
+3 -3
View File
@@ -44,11 +44,11 @@
#### 🌟 주요 개선 사항
1. **`MAM_NO_SUBSCRIBER_DAEMON=1` 옵트아웃 (`lib_py/delegation/daemon.py`, `lib.sh: _start_watchdog_legacy`)**: 리터럴 `1` 일 때만 데몬을 새로 띄우지 않는다. 이미 도는 데몬의 PID 는 그대로 보고하므로 `start_watchdog`/`ensure_subscriber_daemon` 의 dedup 계약은 유지되고, 미기동 시 stderr 에 한 줄 남긴다(stdout 은 비움). 테스트 샌드박스(`mam_sandbox`)가 기본으로 켜고, 데몬 자체를 검증하는 테스트만 `0`/삭제로 되돌린다.
2. **데몬 자기 종료 (`reconcile.sh --subscribe`)**: MQTT 루프는 5초 주기 wake 마다 워크스페이스 디렉터리 존재와 `.mam/monitor.lock` 의 `(st_dev, st_ino)` 가 자신이 잡은 것과 같은지 확인하고, 사라졌거나 교체됐으면(`.mam/` 재생성 → 새 데몬이 새 lock 을 잡을 수 있는 상태) 정리 후 rc 0 으로 끝난다. 브로커 불가 폴링 폴백은 매 회 디렉터리와 lock 파일의 **존재만** 확인한다(python 자식이 끝나며 lock 을 놓았으므로 폴백 중 두 번째 데몬을 배제하지는 못한다 — 종전 동작). 삭제된 pytest 임시 디렉터리나 `rm -rf` 된 프로젝트를 바라보며 공개 브로커 구독을 영원히 유지하던 데몬이 사라진다.
3. **`SIGTERM`/`SIGINT` 즉시 전달 (`reconcile.sh`)**: python 구독자를 백그라운드 자식으로 두고 bash 가 `wait` 하도록 바꿔 시그널이 미뤄지지 않는다(종전엔 포그라운드 자식이 끝날 때까지 bash 가 시그널을 보류해 `SIGKILL` 만 통했다). 트랩이 자식에 `SIGTERM` 을 전달하고 rc 143 으로 끝나며, python 쪽은 핸들러로 이벤트를 깨워 `loop_stop`/`disconnect` 를 거친다. 폴백 루프의 `sleep` 도 `& wait` 로 바꿔 즉시 중단된다.
4. **샌드박스 회수·회귀 가드 (`tests/conftest.py`)**: `mam_sandbox` finalizer 가 argv 형태(`bash|sh <샌드박스>/…/reconcile.sh --subscribe`)로 선정한 데몬과 그 자식을 `SIGTERM`→`SIGKILL` 로 회수한다(브리프를 argv 에 실은 래퍼나 접두 충돌 형제 루트는 선정되지 않음). 세션 스코프 `subscriber_daemon_leak_guard` 는 스위트 종료 시 basetemp 아래 데몬이 남아 있으면 회수한 뒤 실패시킨다(#15 레지스트리 가드와 같은 방식).
3. **`SIGTERM`/`SIGINT` 즉시 전달 (`reconcile.sh`)**: python 구독자를 백그라운드 자식으로 두고 bash 가 `wait` 하도록 바꿔 시그널이 미뤄지지 않는다(종전엔 포그라운드 자식이 끝날 때까지 bash 가 시그널을 보류해 `SIGKILL` 만 통했다). 트랩이 자식에 `SIGTERM` 을 전달하고 rc 143 으로 끝나며, python 쪽은 핸들러로 이벤트를 깨워 `loop_stop`/`disconnect` 를 거친다. 폴백 루프의 대기도 `sleep … & wait` 로 바꿔 시그널이 곧바로 처리된다(끊긴 대기의 `sleep` 자식은 남은 간격 안에 스스로 끝나고, 이미 돌고 있는 `--once` 조정 1회는 끝난 뒤 처리된다).
4. **샌드박스 회수·회귀 가드 (`tests/conftest.py`)**: `mam_sandbox` finalizer 가 argv 형태(`bash|sh <샌드박스>/…/reconcile.sh --subscribe`)로 선정한 데몬과 그 자식을 `SIGTERM`→`SIGKILL` 로 회수한다(브리프를 argv 에 실은 래퍼나 접두 충돌 형제 루트는 선정되지 않음). `ps -o args=` 는 argv 를 따옴표 없이 공백으로 이어 붙이므로 스크립트 경로는 공백 토큰이 아니라 셸 단어부터 첫 `/reconcile.sh --subscribe` 까지 통째로 잡는다 — 공백이 든 샌드박스·`--basetemp` 경로의 데몬도 선정되고, 공백이 든 후보는 실제 파일일 때만 인정해 뒤쪽 인자에 같은 낱말을 실은 bash 래퍼는 배제한다. 세션 스코프 `subscriber_daemon_leak_guard` 는 스위트 종료 시 basetemp 아래 데몬이 남아 있으면 회수한 뒤 실패시킨다(#15 레지스트리 가드와 같은 방식).
#### ✅ 품질 검증
- **신규 테스트 12개** (`tests/test_issue19_subscriber_daemon_lifecycle.py`): 샌드박스 기본값에서 두 helper 본체 모두 데몬 미기동(2), 옵트아웃 중에도 기존 데몬 PID 보고, 리터럴 `1` 만 인정, `create_session.sh` 샌드박스 실행 후 데몬 없음, 옵트아웃 해제 시 누수 재현 + 회수기 제거, 회수기 단위 검증·argv 선정 기준 검증, 폴링 폴백의 lock 소실 종료·`SIGTERM` 즉시 종료(2, 브로커 불필요), MQTT 루프의 lock 교체 종료·`SIGTERM` 정상 종료(2, 브로커 미도달 시 skip).
- **신규 테스트 14개** (`tests/test_issue19_subscriber_daemon_lifecycle.py`): 샌드박스 기본값에서 두 helper 본체 모두 데몬 미기동(2), 옵트아웃 중에도 기존 데몬 PID 보고, 리터럴 `1` 만 인정, `create_session.sh` 샌드박스 실행 후 데몬 없음, 옵트아웃 해제 시 누수 재현 + 회수기 제거, 회수기 단위 검증·argv 선정 기준 검증, 공백이 든 경로의 argv 선정·실제 데몬 회수(2), 폴링 폴백의 lock 소실 종료·`SIGTERM` 즉시 종료(2, 브로커 불필요), MQTT 루프의 lock 교체 종료·`SIGTERM` 정상 종료(2, 브로커 미도달 시 skip).
- **기존 테스트 조정**: `tests/test_issue5_phase1_delegation.py` 의 watchdog/daemon 특성화 테스트가 `MAM_NO_SUBSCRIBER_DAEMON=0`(또는 delenv)으로 옵트아웃을 해제.
- **실측**: 전체 스위트(870 통과) 후 pytest basetemp 아래 `bash …/reconcile.sh --subscribe` 데몬 0건(argv 형태 기준; raw `pgrep -f` 는 브리프를 실은 루프 드라이버·래퍼도 매치하므로 판정에 쓰지 않음).
+20 -8
View File
@@ -34,16 +34,28 @@ def _is_subscriber_daemon_argv(args, root):
--subscribe ...`. A process that merely *quotes* those words (a loop driver
or delegate wrapper carrying a brief in argv) and a sibling root that is a
string prefix of `root` (`.../pytest-9` vs `.../pytest-99`) are excluded
(review R-2) - the reaper kills what it selects."""
argv = args.split()
if len(argv) < 3:
(review R-2) - the reaper kills what it selects.
`ps -o args=` joins argv with single spaces and quotes nothing, so the
script path is taken whole - from the shell word up to the first
`/reconcile.sh --subscribe` - not as one whitespace token: a sandbox or
`--basetemp` path with a space in it must not hide its daemon from the
reaper and the leak guard (codex review P2). In that flat form a path with
whitespace is ambiguous - a bash wrapper under `root` can carry the words
later in its argv - so such a path counts only when it names an existing
file. A daemon whose workspace is already gone exits on its own (Issue #19)."""
import re as _re
shell, _, rest = args.strip().partition(" ")
if os.path.basename(shell) not in ("bash", "sh"):
return False
if os.path.basename(argv[0]) not in ("bash", "sh"):
rest = rest.lstrip()
if not rest.startswith(str(root).rstrip(os.sep) + os.sep):
return False
script = argv[1]
return (script.startswith(str(root).rstrip(os.sep) + os.sep)
and script.endswith(os.sep + "reconcile.sh")
and argv[2] == "--subscribe")
m = _re.match(r"(.*?" + _re.escape(os.sep) + r"reconcile\.sh) --subscribe(?: |$)", rest)
if not m:
return False
script = m.group(1)
return not any(c.isspace() for c in script) or os.path.isfile(script)
def _subscriber_daemon_pids(root):
@@ -58,7 +58,7 @@ def _run_helper(sandbox: Path, snippet: str, env=None) -> subprocess.CompletedPr
def _daemons_for(sandbox: Path) -> list:
"""Same argv-token selection as the conftest reaper (review R-2)."""
"""Same argv-shape selection as the conftest reaper (review R-2, P2)."""
from conftest import _is_subscriber_daemon_argv
out = subprocess.run(["pgrep", "-f", "reconcile.sh --subscribe"], capture_output=True, text=True).stdout.split()
@@ -228,10 +228,76 @@ def test_reaper_selects_only_the_daemon_argv_shape(tmp_path):
# brief-carrying wrapper that merely quotes the words and the path
quoting = "python3 -m lib_py.delegation.cli submit --prompt 'note: reconcile.sh --subscribe under {}/.agents/skills/x'".format(root)
assert not _is_subscriber_daemon_argv(quoting, root)
# ...and one run by bash from under the root, its prompt ending in the daemon's words
wrapper = "bash {0}/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job submit --prompt restart {1}".format(root, daemon)
assert not _is_subscriber_daemon_argv(wrapper, root)
# the real reconcile.sh in a different mode
assert not _is_subscriber_daemon_argv(daemon.replace("--subscribe", "--once"), root)
assert not _is_subscriber_daemon_argv("", root)
def _spaced_monitor(tmp_path: Path):
"""A monitor script under a sandbox path with a space in it, as a pytest
`--basetemp` or temp dir may have (codex review P2)."""
root = tmp_path / "pytest review" / "case"
script = root / ".agents" / "skills" / "multi-agent-mux-monitor" / "scripts" / "reconcile.sh"
script.parent.mkdir(parents=True)
script.write_text("#!/usr/bin/env bash\nwhile :; do sleep 1; done\n", encoding="utf-8")
script.chmod(0o755)
return root, script
def test_reaper_selects_daemons_under_whitespace_paths(tmp_path):
"""`ps -o args=` does not quote: the script path must be matched whole, both
at the sandbox root (fixture reaper) and at an ancestor (session guard)."""
from conftest import _is_subscriber_daemon_argv
root, script = _spaced_monitor(tmp_path)
daemon = "bash {} --subscribe --idle-timeout 0".format(script)
assert _is_subscriber_daemon_argv(daemon + "\n", str(root))
assert _is_subscriber_daemon_argv(daemon, str(tmp_path))
assert _is_subscriber_daemon_argv("/bin/sh " + daemon.split(" ", 1)[1], str(root))
# the text before the space names another root, not an ancestor
assert not _is_subscriber_daemon_argv(daemon, str(tmp_path / "pytest"))
# sibling root that this one is a string prefix of
assert not _is_subscriber_daemon_argv(daemon.replace("/case/", "/case2/"), str(root))
assert not _is_subscriber_daemon_argv(daemon.replace("--subscribe", "--once"), str(root))
# wrappers carrying the words: python, and bash from under the spaced root
quoting = "python3 -m lib_py.delegation.cli submit --prompt note: {}".format(daemon)
assert not _is_subscriber_daemon_argv(quoting, str(root))
wrapper = "bash {0}/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job submit --prompt restart {1}".format(root, daemon)
assert not _is_subscriber_daemon_argv(wrapper, str(root))
def test_reaper_removes_a_live_daemon_under_a_whitespace_path(tmp_path):
"""The owned live-process half of P2: the reaper and the leak guard find a
running `bash <spaced path>/reconcile.sh --subscribe` and take it down."""
from conftest import _reap_subscriber_daemons, _subscriber_daemon_pids
root, script = _spaced_monitor(tmp_path)
proc = subprocess.Popen(
["bash", str(script), "--subscribe", "--idle-timeout", "0"],
stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
start_new_session=True,
)
try:
deadline = time.time() + 5
while time.time() < deadline and proc.pid not in _subscriber_daemon_pids(root):
time.sleep(0.1)
assert proc.pid in _subscriber_daemon_pids(root), "fixture reaper does not see the daemon"
assert proc.pid in _subscriber_daemon_pids(tmp_path), "session leak guard does not see the daemon"
assert proc.pid in _reap_subscriber_daemons(root)
assert _wait_exit(proc, 5) is not None, "reaper left the daemon running"
assert _subscriber_daemon_pids(root) == []
finally:
if proc.poll() is None:
try:
os.killpg(proc.pid, signal.SIGKILL)
except OSError:
pass
proc.wait(timeout=5)
def test_sandbox_reaper_terminates_daemons_anchored_in_the_sandbox(mam_sandbox, monkeypatch):
from conftest import _reap_subscriber_daemons, _subscriber_daemon_pids
from lib_py.delegation import daemon