From 134fe9704c63bd9b1f46b64b01868f904f5a33cc Mon Sep 17 00:00:00 2001
From: Godopu <godopu16@gmail.com>
Date: Sun, 13 Sep 2026 17:22:39 +0900
Subject: [PATCH] fix(issue-19): arm the subscriber's stop handlers before it
 connects; read $! with nounset off

Review round 3 (reviewer-grok-01, NOT PASS N1; non-blocking B1):

- reconcile.sh (N1): the python subscriber installs its SIGTERM/SIGINT
  handlers before connect/loop_start. A signal during the broker handshake,
  the CONNACK wait, or right after the paho thread logs "subscribed to" now
  takes the clean path (the signal line, loop_stop/disconnect, rc 0) instead
  of Python's default action; the CONNACK wait ends on it, and a signalled
  subscriber never falls back to polling (rc 3).
- reconcile.sh (B1): `_sub_stop` reads `$!` with nounset off. On bash 3.2
  `${!:-}` and `${!+x}` are both unbound before the first `&`, so the trap
  died with rc 1 instead of 143.
- tests: SIGTERM during a CONNACK wait against a silent local listener (no
  broker needed), and the verbatim `_sub_stop` run without and with a
  child; both fail on 8f9745b.
- docs: monitor SKILL.md and VERSIONS.md state the handler order; test
  count 16.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HCGpJXhSZasJLU38ksDghe
---
 .../skills/multi-agent-mux-monitor/SKILL.md   |  4 +-
 .../scripts/reconcile.sh                      | 64 ++++++++++++-------
 VERSIONS.md                                   |  4 +-
 ...est_issue19_subscriber_daemon_lifecycle.py | 52 +++++++++++++++
 4 files changed, 97 insertions(+), 27 deletions(-)

diff --git a/.agents/skills/multi-agent-mux-monitor/SKILL.md b/.agents/skills/multi-agent-mux-monitor/SKILL.md
index 45159e3..6a5e70a 100644
--- a/.agents/skills/multi-agent-mux-monitor/SKILL.md
+++ b/.agents/skills/multi-agent-mux-monitor/SKILL.md
@@ -84,7 +84,9 @@ 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. In the broker-down polling fallback the signal cuts the
+  python subscriber and exits 143; the subscriber's handlers are in place before it
+  connects, so a signal during the broker handshake or right after it subscribes takes
+  the same clean path. 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;
diff --git a/.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh b/.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh
index 1d0dffa..fbf25fc 100755
--- a/.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh
+++ b/.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh
@@ -80,8 +80,15 @@ if [ "$SUBSCRIBE" = "1" ]; then
   _sub_stop() {
     trap - TERM INT
     # `$!` 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:-${!:-}}"
+    # child that already exited makes kill/wait harmless no-ops. Before the
+    # first `&` bash 3.2 treats `$!` as unbound under `set -u` - `${!:-}` and
+    # `${!+x}` included - so it is read with nounset off (review B1).
+    local child="${_sub_pid:-}"
+    if [ -z "$child" ]; then
+      set +u
+      child="$!"
+      set -u
+    fi
     if [ -n "$child" ]; then
       kill -TERM "$child" 2>/dev/null || true
       wait "$child" 2>/dev/null || true
@@ -290,28 +297,6 @@ def on_connect(_c, _u, _flags, reason_code, _props):
         print(f"MQTT Monitor connection failed: rc={rc}", flush=True)
 
 
-cfg = mqtt_common.broker_config_from_env()
-client = mqtt_common.make_client("monitor_sub", cfg)
-client.on_message = on_message
-client.on_connect = on_connect
-print(f"MQTT Monitor: connecting to {cfg.host}:{cfg.port} (TLS={cfg.tls})...", flush=True)
-
-# Connection failure → fall back to polling (review item 4).
-try:
-    client.connect(cfg.host, cfg.port, cfg.keepalive)
-except Exception as e:
-    print(f"MQTT Monitor: connect failed ({e}); falling back to polling", flush=True)
-    sys.exit(3)
-
-client.loop_start()
-_wait = time.time()
-while time.time() - _wait < 5 and not state['connected'] and not state['failed']:
-    time.sleep(0.1)
-if not state['connected']:
-    print("MQTT Monitor: broker did not accept connection; falling back to polling", flush=True)
-    client.loop_stop()
-    sys.exit(3)
-
 import threading, signal
 stop_event = threading.Event()
 state['stop_signal'] = None
@@ -322,8 +307,39 @@ def _on_stop_signal(signum, _frame):
     stop_event.set()
 
 
+# Issue #19 (review N1): the handlers go in before connect/loop_start. The paho
+# thread logs "subscribed to" from on_connect, and a SIGTERM during the broker
+# handshake or right after that line must take the same clean path as a later
+# one - the "signal N received" line and loop_stop/disconnect - not Python's
+# default action. A signalled subscriber exits 0, never 3: rc 3 would send the
+# bash wrapper into the polling fallback.
 signal.signal(signal.SIGTERM, _on_stop_signal)
 signal.signal(signal.SIGINT, _on_stop_signal)
+
+cfg = mqtt_common.broker_config_from_env()
+client = mqtt_common.make_client("monitor_sub", cfg)
+client.on_message = on_message
+client.on_connect = on_connect
+print(f"MQTT Monitor: connecting to {cfg.host}:{cfg.port} (TLS={cfg.tls})...", flush=True)
+
+# Connection failure → fall back to polling (review item 4).
+try:
+    client.connect(cfg.host, cfg.port, cfg.keepalive)
+except Exception as e:
+    if state['stop_signal'] is None:
+        print(f"MQTT Monitor: connect failed ({e}); falling back to polling", flush=True)
+        sys.exit(3)
+else:
+    client.loop_start()
+    _wait = time.time()
+    while (time.time() - _wait < 5 and not state['connected'] and not state['failed']
+           and not stop_event.is_set()):
+        stop_event.wait(0.1)
+    if not state['connected'] and state['stop_signal'] is None:
+        print("MQTT Monitor: broker did not accept connection; falling back to polling", flush=True)
+        client.loop_stop()
+        sys.exit(3)
+
 start = time.time()
 try:
     while True:
diff --git a/VERSIONS.md b/VERSIONS.md
index 214d17c..403ce79 100644
--- a/VERSIONS.md
+++ b/VERSIONS.md
@@ -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` 로 바꿔 시그널이 곧바로 처리된다(끊긴 대기의 `sleep` 자식은 남은 간격 안에 스스로 끝나고, 이미 돌고 있는 `--once` 조정 1회는 끝난 뒤 처리된다).
+3. **`SIGTERM`/`SIGINT` 즉시 전달 (`reconcile.sh`)**: python 구독자를 백그라운드 자식으로 두고 bash 가 `wait` 하도록 바꿔 시그널이 미뤄지지 않는다(종전엔 포그라운드 자식이 끝날 때까지 bash 가 시그널을 보류해 `SIGKILL` 만 통했다). 트랩이 자식에 `SIGTERM` 을 전달하고 rc 143 으로 끝나며(첫 백그라운드 자식 전의 시그널 포함), python 쪽 핸들러는 브로커 연결(`connect`/`loop_start`) 전에 설치되어 핸드셰이크·CONNACK 대기 중이나 구독 직후의 시그널도 이벤트를 깨워 `loop_stop`/`disconnect` 를 거친 뒤 rc 0 으로 끝난다(시그널을 받은 뒤에는 폴링 폴백으로 가지 않는다). 폴백 루프의 대기도 `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 레지스트리 가드와 같은 방식).
 
 #### ✅ 품질 검증
-- **신규 테스트 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).
+- **신규 테스트 16개** (`tests/test_issue19_subscriber_daemon_lifecycle.py`): 샌드박스 기본값에서 두 helper 본체 모두 데몬 미기동(2), 옵트아웃 중에도 기존 데몬 PID 보고, 리터럴 `1` 만 인정, `create_session.sh` 샌드박스 실행 후 데몬 없음, 옵트아웃 해제 시 누수 재현 + 회수기 제거, 회수기 단위 검증·argv 선정 기준 검증, 공백이 든 경로의 argv 선정·실제 데몬 회수(2), 폴링 폴백의 lock 소실 종료·`SIGTERM` 즉시 종료(2, 브로커 불필요), CONNACK 대기 중 `SIGTERM` 정상 종료(응답 없는 로컬 리스너, 브로커 불필요), 첫 백그라운드 자식 전의 시그널에도 `_sub_stop` 트랩 rc 143(bash 3.2 `set -u`), 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` 는 브리프를 실은 루프 드라이버·래퍼도 매치하므로 판정에 쓰지 않음).
 
diff --git a/tests/test_issue19_subscriber_daemon_lifecycle.py b/tests/test_issue19_subscriber_daemon_lifecycle.py
index 23e5d9f..7b37e0e 100644
--- a/tests/test_issue19_subscriber_daemon_lifecycle.py
+++ b/tests/test_issue19_subscriber_daemon_lifecycle.py
@@ -358,6 +358,58 @@ def test_poll_fallback_sigterm_exits_promptly(mam_sandbox, mock_herdr, monkeypat
             proc.kill()
 
 
+def test_sigterm_during_broker_handshake_takes_the_clean_path(mam_sandbox, mock_herdr, monkeypatch):
+    """Review N1: the python handlers are installed before connect/loop_start,
+    so a SIGTERM while the subscriber still waits for CONNACK (a local listener
+    that never answers) exits through the same clean path as a later one - the
+    "signal 15 received" line, rc 143 from the wrapper - and never drops into
+    the polling fallback."""
+    import socket
+
+    monkeypatch.delenv("MAM_NO_SUBSCRIBER_DAEMON", raising=False)
+    silent = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+    silent.bind(("127.0.0.1", 0))
+    silent.listen(1)  # the kernel completes the TCP handshake; no CONNACK ever comes
+    log = mam_sandbox / "sub.log"
+    proc = _start_subscriber(mam_sandbox, log, {
+        "MQTT_BROKER": "127.0.0.1", "MQTT_PORT": str(silent.getsockname()[1]), "MQTT_TLS": "0",
+    })
+    try:
+        assert _wait_for(log, r"MQTT Monitor: connecting to 127\.0\.0\.1", 30), log.read_text()
+        time.sleep(1.0)  # well inside the 5 s CONNACK wait
+        t0 = time.time()
+        proc.send_signal(signal.SIGTERM)
+        rc = _wait_exit(proc, 10)
+        assert rc is not None, "SIGTERM was deferred: daemon still running after 10s"
+        assert time.time() - t0 < 5
+        assert rc == 143, (rc, log.read_text())
+        text = log.read_text()
+        assert "signal 15 received, exiting" in text, text
+        assert "falling back to polling" not in text, text
+    finally:
+        if proc.poll() is None:
+            proc.kill()
+        silent.close()
+
+
+def test_sub_stop_trap_exits_143_before_the_first_background_child():
+    """Review B1: the wrapper's `_sub_stop` trap must end with rc 143 even when
+    the signal lands before the first `&` - bash 3.2 treats `$!` as unbound
+    under `set -u` there, and the trap died with rc 1. The function is taken
+    verbatim from reconcile.sh and run without and with a child to stop."""
+    src = (Path(__file__).resolve().parent.parent / ".agents" / "skills" / "multi-agent-mux-monitor"
+           / "scripts" / "reconcile.sh").read_text(encoding="utf-8")
+    m = re.search(r"^  _sub_stop\(\) \{\n.*?^  \}\n", src, re.S | re.M)
+    assert m, "_sub_stop not found in reconcile.sh"
+    # With a child, let it exec first: a TERM forwarded in the instant between
+    # its fork and its exec is swallowed by the child's copy of the trap - a
+    # separate window from B1, which is about reading `$!` at all.
+    for child in ("", "sleep 30 &\nsleep 0.5\n"):
+        script = "set -euo pipefail\n_sub_pid=''\n{}trap _sub_stop TERM INT\n{}kill -TERM $$\nsleep 5\n".format(m.group(0), child)
+        res = subprocess.run(["bash", "-c", script], capture_output=True, text=True, timeout=30)
+        assert res.returncode == 143, (child, res.returncode, res.stderr)
+
+
 # ---------------------------------------------------------------------------
 # 4. self-termination and signals - MQTT loop (needs a reachable broker)
 # ---------------------------------------------------------------------------