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
This commit is contained in:
1 parent
8f9745b128
commit
134fe9704c
4 files changed
+97
-27
No files matched your search
@@ -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;
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in new issue
Block a user