fix(issue-19): close the trap race, select daemons by argv shape, align docs with the polling fallback
Review round 1 (claude PASS, agy PASS) recommendations: - reconcile.sh: `_sub_stop` falls back to `$!` for the window before `_sub_pid=$!` (R-1); lock identity compared as (st_dev, st_ino). - lib.sh legacy start_watchdog prints nothing when opted out and no daemon runs, byte-identical to the python body. - conftest/test: `_is_subscriber_daemon_argv` selects only `bash|sh <root>/.../reconcile.sh --subscribe`, never a brief-carrying wrapper that quotes the words or a prefix-colliding sibling root (R-2); one new unit test for the selector. - monitor SKILL.md / VERSIONS.md: the polling fallback checks the lock file's presence only and does not exclude a second daemon; measurement wording uses the argv-shape criterion instead of raw pgrep. 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
4da4a50682
commit
c096cb455b
6 files changed
+68
-15
No files matched your search
@@ -2021,7 +2021,10 @@ _start_watchdog_legacy() {
|
||||
cd "$orig_pwd" || return 1
|
||||
fi
|
||||
|
||||
# Byte-identical to the python body: nothing on stdout when no daemon runs.
|
||||
if [ -n "$pid" ]; then
|
||||
echo "$pid"
|
||||
fi
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -76,10 +76,13 @@ per workspace through `lib.sh::start_watchdog` (`lib_py.delegation.daemon`). Tha
|
||||
|
||||
- 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;
|
||||
- **ends on its own only when its workspace is gone** — every wake-up (≤ 5 s) the MQTT
|
||||
loop checks that the workspace directory still exists and that `.mam/monitor.lock` is
|
||||
still the `(st_dev, st_ino)` it holds; a deleted workspace, a purged `.mam/`, or a
|
||||
re-created lock file makes it disconnect and exit 0. The broker-down polling fallback
|
||||
checks the directory and the lock file's *presence* on every pass — it no longer holds
|
||||
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;
|
||||
- is **not started when `MAM_NO_SUBSCRIBER_DAEMON=1`** — the literal `1` only. Use it when
|
||||
|
||||
@@ -79,9 +79,12 @@ if [ "$SUBSCRIBE" = "1" ]; then
|
||||
_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
|
||||
# `$!` covers the window between `&` and `_sub_pid=$!` (review R-1); a
|
||||
# child that already exited makes kill/wait harmless no-ops.
|
||||
local child="${_sub_pid:-${!:-}}"
|
||||
if [ -n "$child" ]; then
|
||||
kill -TERM "$child" 2>/dev/null || true
|
||||
wait "$child" 2>/dev/null || true
|
||||
fi
|
||||
exit 143
|
||||
}
|
||||
@@ -115,7 +118,8 @@ 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
|
||||
_lock_st = os.fstat(lock_file.fileno())
|
||||
lock_ident = (_lock_st.st_dev, _lock_st.st_ino)
|
||||
|
||||
|
||||
def _workspace_gone():
|
||||
@@ -131,7 +135,8 @@ def _workspace_gone():
|
||||
if not os.path.isdir(root):
|
||||
return f"workspace {root} no longer exists"
|
||||
try:
|
||||
if os.stat(lock_file_path).st_ino != lock_ino:
|
||||
st = os.stat(lock_file_path)
|
||||
if (st.st_dev, st.st_ino) != lock_ident:
|
||||
return f"{lock_file_path} was replaced"
|
||||
except OSError:
|
||||
return f"{lock_file_path} no longer exists"
|
||||
@@ -367,6 +372,9 @@ PYEOF
|
||||
_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.
|
||||
# The python child released .mam/monitor.lock when it exited, so this
|
||||
# fallback checks the lock file's *presence* only and does not exclude a
|
||||
# second daemon (pre-existing behaviour of the fallback).
|
||||
trap 'trap - TERM INT; exit 143' TERM INT
|
||||
while :; do
|
||||
if [ ! -d "$WORKSPACE_ROOT" ] || [ ! -f "$WORKSPACE_ROOT/.mam/monitor.lock" ]; then
|
||||
|
||||
+4
-4
@@ -43,14 +43,14 @@
|
||||
|
||||
#### 🌟 주요 개선 사항
|
||||
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` 의 inode 가 자신이 잡은 것과 같은지 확인하고, 사라졌거나 교체됐으면(`.mam/` 재생성 → 새 데몬이 새 lock 을 잡을 수 있는 상태) 정리 후 rc 0 으로 끝난다. 브로커 불가 폴링 폴백도 매 회 같은 검사를 한다. 삭제된 pytest 임시 디렉터리나 `rm -rf` 된 프로젝트를 바라보며 공개 브로커 구독을 영원히 유지하던 데몬이 사라진다.
|
||||
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 가 샌드박스 경로를 cmdline 에 가진 `reconcile.sh --subscribe` 와 그 자식을 `SIGTERM`→`SIGKILL` 로 회수한다. 세션 스코프 `subscriber_daemon_leak_guard` 는 스위트 종료 시 basetemp 아래 데몬이 남아 있으면 회수한 뒤 실패시킨다(#15 레지스트리 가드와 같은 방식).
|
||||
4. **샌드박스 회수·회귀 가드 (`tests/conftest.py`)**: `mam_sandbox` finalizer 가 argv 형태(`bash|sh <샌드박스>/…/reconcile.sh --subscribe`)로 선정한 데몬과 그 자식을 `SIGTERM`→`SIGKILL` 로 회수한다(브리프를 argv 에 실은 래퍼나 접두 충돌 형제 루트는 선정되지 않음). 세션 스코프 `subscriber_daemon_leak_guard` 는 스위트 종료 시 basetemp 아래 데몬이 남아 있으면 회수한 뒤 실패시킨다(#15 레지스트리 가드와 같은 방식).
|
||||
|
||||
#### ✅ 품질 검증
|
||||
- **신규 테스트 11개** (`tests/test_issue19_subscriber_daemon_lifecycle.py`): 샌드박스 기본값에서 두 helper 본체 모두 데몬 미기동(2), 옵트아웃 중에도 기존 데몬 PID 보고, 리터럴 `1` 만 인정, `create_session.sh` 샌드박스 실행 후 데몬 없음, 옵트아웃 해제 시 누수 재현 + 회수기 제거, 회수기 단위 검증, 폴링 폴백의 lock 소실 종료·`SIGTERM` 즉시 종료(2, 브로커 불필요), MQTT 루프의 lock 교체 종료·`SIGTERM` 정상 종료(2, 브로커 미도달 시 skip).
|
||||
- **신규 테스트 12개** (`tests/test_issue19_subscriber_daemon_lifecycle.py`): 샌드박스 기본값에서 두 helper 본체 모두 데몬 미기동(2), 옵트아웃 중에도 기존 데몬 PID 보고, 리터럴 `1` 만 인정, `create_session.sh` 샌드박스 실행 후 데몬 없음, 옵트아웃 해제 시 누수 재현 + 회수기 제거, 회수기 단위 검증·argv 선정 기준 검증, 폴링 폴백의 lock 소실 종료·`SIGTERM` 즉시 종료(2, 브로커 불필요), MQTT 루프의 lock 교체 종료·`SIGTERM` 정상 종료(2, 브로커 미도달 시 skip).
|
||||
- **기존 테스트 조정**: `tests/test_issue5_phase1_delegation.py` 의 watchdog/daemon 특성화 테스트가 `MAM_NO_SUBSCRIBER_DAEMON=0`(또는 delenv)으로 옵트아웃을 해제.
|
||||
- **실측**: 스위트 실행 후 `pgrep -f 'pytest-of-.*reconcile.sh --subscribe'` 0건.
|
||||
- **실측**: 전체 스위트(870 통과) 후 pytest basetemp 아래 `bash …/reconcile.sh --subscribe` 데몬 0건(argv 형태 기준; raw `pgrep -f` 는 브리프를 실은 루프 드라이버·래퍼도 매치하므로 판정에 쓰지 않음).
|
||||
|
||||
### 🚀 `v4.4.4` — Issue #12 레지스트리 비밀번호 제거, Issue #8 업데이트 스탬프 보존, Issue #7 형제 워크스페이스 패킹 (2026-09-13)
|
||||
|
||||
|
||||
+18
-1
@@ -29,6 +29,23 @@ SKILLS_DIR = str(Path(__file__).resolve().parent.parent / ".agents" / "skills")
|
||||
if SKILLS_DIR not in sys.path:
|
||||
sys.path.insert(0, SKILLS_DIR)
|
||||
|
||||
def _is_subscriber_daemon_argv(args, root):
|
||||
"""True only for the daemon's own command line: `bash|sh <root>/.../reconcile.sh
|
||||
--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:
|
||||
return False
|
||||
if os.path.basename(argv[0]) not in ("bash", "sh"):
|
||||
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")
|
||||
|
||||
|
||||
def _subscriber_daemon_pids(root):
|
||||
"""PIDs of `reconcile.sh --subscribe` daemons whose command line lives under
|
||||
`root`, plus their children (the python subscriber the bash wrapper waits on).
|
||||
@@ -40,7 +57,7 @@ def _subscriber_daemon_pids(root):
|
||||
root = str(root)
|
||||
found = []
|
||||
for pid in _run_real(["pgrep", "-f", "reconcile.sh --subscribe"]).split():
|
||||
if root in _run_real(["ps", "-o", "args=", "-p", pid]):
|
||||
if _is_subscriber_daemon_argv(_run_real(["ps", "-o", "args=", "-p", pid]), root):
|
||||
found.append(int(pid))
|
||||
kids = []
|
||||
for pid in found:
|
||||
|
||||
@@ -58,11 +58,14 @@ 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)."""
|
||||
from conftest import _is_subscriber_daemon_argv
|
||||
|
||||
out = subprocess.run(["pgrep", "-f", "reconcile.sh --subscribe"], capture_output=True, text=True).stdout.split()
|
||||
hits = []
|
||||
for pid in out:
|
||||
args = subprocess.run(["ps", "-o", "args=", "-p", pid], capture_output=True, text=True).stdout
|
||||
if str(sandbox) in args:
|
||||
if _is_subscriber_daemon_argv(args, str(sandbox)):
|
||||
hits.append(int(pid))
|
||||
return hits
|
||||
|
||||
@@ -210,6 +213,25 @@ def test_create_session_without_opt_out_spawns_a_daemon_the_reaper_removes(mam_s
|
||||
# 2. the sandbox reaper
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_reaper_selects_only_the_daemon_argv_shape(tmp_path):
|
||||
"""Review R-2: a process that quotes the words in a brief, or lives under
|
||||
a sibling root that is a string prefix, must never be selected."""
|
||||
from conftest import _is_subscriber_daemon_argv
|
||||
|
||||
root = str(tmp_path / "sbx1")
|
||||
daemon = "bash {}/.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh --subscribe --idle-timeout 0".format(root)
|
||||
assert _is_subscriber_daemon_argv(daemon, root)
|
||||
assert _is_subscriber_daemon_argv("/bin/sh " + daemon.split(" ", 1)[1], root)
|
||||
# sibling root that is a prefix of this one
|
||||
assert not _is_subscriber_daemon_argv(daemon, root[:-1])
|
||||
assert not _is_subscriber_daemon_argv(daemon, root + "0")
|
||||
# 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)
|
||||
# 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 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
|
||||
|
||||
Reference in new issue
Block a user