From fcfa2a569aa51670988360bf8f05fcdc3d13834d Mon Sep 17 00:00:00 2001 From: Your Name Date: Sat, 12 Sep 2026 13:48:02 -0400 Subject: [PATCH] fix: stop telegram listener crash-loop from failed background commands A mapped command exiting non-zero (e.g. /capture -> ffmpeg with no webcam, exit 254) killed the whole daemon: the CHLD trap only recorded children that exited 0 (and wait -n inside a trap is unreliable on bash 5.2 anyway), so reap_commands fell back to a bare 'wait $pid' which aborts the shell under set -euo pipefail before the exit code is captured. systemd Restart=always then crash-looped (dead gaps + duplicate command execution from getUpdates offset=0 restarts). - reap_commands: single reaper path, set -e safe wait with || rc=$?, non-zero child exits now produce a normal reply with the real rc - persist the confirmed getUpdates offset to $CONFIG_DIR/telegram-listener.state (LISTENER_STATE_FILE seam) and resume it on start, so a restart never re-delivers an unconfirmed burst - new regression test t-telegram-listener-reap.sh (12 checks): 254-child reap survives daemon, negative control proves the old idiom dies, offset load/save resume + invalid fallback + empty-batch no-write --- bin/pos-communication-telegram-listener | 65 ++++++--- tests/README.md | 1 + tests/t-telegram-listener-reap.sh | 169 ++++++++++++++++++++++++ 3 files changed, 220 insertions(+), 15 deletions(-) create mode 100644 tests/t-telegram-listener-reap.sh diff --git a/bin/pos-communication-telegram-listener b/bin/pos-communication-telegram-listener index 859138f..d752def 100755 --- a/bin/pos-communication-telegram-listener +++ b/bin/pos-communication-telegram-listener @@ -8,6 +8,9 @@ CONFIG_DIR="${CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/linux_post_install}" CONFIG_FILE="$CONFIG_DIR/telegram.env" MAP_FILE="$CONFIG_DIR/telegram_commands.env" PREFIX_FILE="$CONFIG_DIR/telegram_prefixes.env" +# Persisted getUpdates offset (survives restarts so unconfirmed updates are +# never re-delivered in a burst after a crash/restart). +STATE_FILE="${LISTENER_STATE_FILE:-$CONFIG_DIR/telegram-listener.state}" API="https://api.telegram.org" SERVICE="pos-telegram-listener.service" USER_SYSTEMD_DIR="${XDG_CONFIG_HOME:-$HOME/.config}/systemd/user" @@ -97,6 +100,31 @@ load_config() { load_env_file "$CONFIG_FILE" } +# ── getUpdates offset persistence ──────────────────────────────── +# load_offset — read the last confirmed update offset from STATE_FILE. +# Returns 0 when the file is missing/invalid (fresh start). +load_offset() { + local val + [ -f "$STATE_FILE" ] || { printf '0'; return 0; } + val="$(cat "$STATE_FILE" 2>/dev/null || true)" + case "$val" in + ''|*[!0-9]*) printf '0' ;; + *) printf '%s' "$val" ;; + esac +} + +# save_offset — atomically persist the last processed update offset. +save_offset() { + local val="$1" + mkdir -p "$(dirname "$STATE_FILE")" + local tmp + tmp="$(mktemp)" + printf '%s\n' "$val" > "$tmp" + chmod 600 "$tmp" + mv "$tmp" "$STATE_FILE" + chmod 600 "$STATE_FILE" +} + # ── command map (MAP_FILE) ────────────────────────────────────── # Lines: /cmd=bash command, or /cmd::description=bash command. Keys keep the # leading slash; read via awk so values may contain '='. Entries are emitted @@ -347,9 +375,12 @@ prefix_map_show() { # # PID → metadata arrays (populated by run_and_reply, drained by reap_commands) declare -A _CMD_OUT _CMD_MSG _CMD_QUIET -# Exit codes stored by the SIGCHLD handler (wait -n) so reap_commands can -# retrieve them without calling blocking wait. -declare -A _EXIT_CODES +# +# No SIGCHLD reaper: `wait -n`/`wait -n -p` inside a CHLD trap is unreliable +# on bash 5.2 (it reports "no children" even when processes have exited), so +# a trap-based reaper silently never fires. reap_commands below polls with +# kill -0 and reaps via `wait "$pid" || rc=$?` — simple, non-blocking, and +# exit-code-correct (see the crash this design replaces). # run_and_reply [timeout] [quiet] # Starts the command in the background and returns immediately. The main @@ -376,13 +407,12 @@ reap_commands() { for pid in "${!_CMD_OUT[@]}"; do # Non-blocking: has the process exited? if ! kill -0 "$pid" 2>/dev/null; then - # Retrieve exit code (SIGCHLD handler stores it; fallback to wait). - local rc="${_EXIT_CODES[$pid]:-}" - if [ -n "$rc" ]; then - unset _EXIT_CODES[$pid] - else - wait "$pid" 2>/dev/null; rc=$? - fi + # Retrieve exit code. `wait "$pid"` returns immediately since the + # process has exited. IMPORTANT: a bare `wait "$pid"` under + # `set -e` would abort the daemon on non-zero exits (the crash + # this code replaces), so the status is captured via `|| rc=$?`. + local rc=0 + wait "$pid" 2>/dev/null || rc=$? local out_file="${_CMD_OUT[$pid]}" local msg_id="${_CMD_MSG[$pid]}" local quiet="${_CMD_QUIET[$pid]}" @@ -831,11 +861,15 @@ run_daemon() { fi sync_bot_commands || true - local offset=0 - log "listener running (chat ${TELEGRAM_CHAT_ID}, owner ${TELEGRAM_OWNER_ID:-unset}) — Ctrl+C to stop" - # SIGCHLD: reap finished children and store their exit codes so - # reap_commands can retrieve them without blocking. - trap 'local _p; while _p=$(wait -n 2>/dev/null); do _EXIT_CODES[$_p]=$?; done' CHLD + # Resume from the last persisted update offset so unconfirmed updates are + # not re-delivered in a burst after a crash/restart (systemd Restart=always + # used to restart from 0 and re-run duplicate commands). + local offset + offset="$(load_offset)" + log "listener running (chat ${TELEGRAM_CHAT_ID}, owner ${TELEGRAM_OWNER_ID:-unset}, offset ${offset}) — Ctrl+C to stop" + # No SIGCHLD reaper here: `wait -n` inside a CHLD trap is unreliable on + # bash 5.2 (see the note near the _CMD_* arrays). reap_commands handles + # finished children after each polling cycle. trap 'kill $(jobs -p) 2>/dev/null; rm -f /tmp/pos-cmd.* 2>/dev/null; wait 2>/dev/null; exit 0' TERM INT while true; do local resp n i @@ -857,6 +891,7 @@ run_daemon() { msg_id="$(printf '%s' "$resp" | jq -r ".result[$i].message.message_id // empty")" reply_text="$(printf '%s' "$resp" | jq -r ".result[$i].message.reply_to_message.text // .result[$i].message.reply_to_message.caption // empty")" offset=$((u + 1)) + save_offset "$offset" [ -n "$text" ] || continue if [ -z "${TELEGRAM_OWNER_ID:-}" ]; then warn "TELEGRAM_OWNER_ID unset — ignoring command (set it with 'pos config telegram')" diff --git a/tests/README.md b/tests/README.md index 46d806b..6cfc10d 100644 --- a/tests/README.md +++ b/tests/README.md @@ -57,4 +57,5 @@ silently. | `t-share-mountpoint.sh` | share-client `ask_mountpoint` UX: existing/new/declined/rejected paths, confirm gate, mkdir side effects, non-TTY stdin contract, static `n`→`t` guards | | `t-pos-media-yt.sh` | unified `pos media yt` suite: dispatcher + forwarder resolution, shared yt-lib helpers, yt-mp3/mp4/grab/subtitles flags, dry-run deps, `YT_OUT_DIR` seam, `GRAB_DEFAULT` config, negative controls (unsafe-URL no-expansion, `--lang en,ar` single arg, txt timestamp-stripping) | | `t-telegram-listener-singleton.sh` | Telegram listener single-instance guard: first `--run` acquires the flock, second `--run` fails fast with the exact message, lock auto-releases so the next start is clean, `--status` reports the lock state | +| `t-telegram-listener-reap.sh` | Telegram listener crash-loop regression: non-zero (254) child exit no longer kills the daemon, reply carries the real exit code + output, negative control proves the old `wait`-under-`set -e` idiom dies, getUpdates offset persists across restarts (resume, invalid-state fallback, empty-batch no-write) | | `t-bank.sh` | Command Bank: bank-lib.sh unit tests (add/remove/update/find/get/list/count/valid/extract_params/substitute_params, multiline `\n` storage round-trip, literal-`\n` escape round-trip, v1 backward compat) + pos system bank CLI integration (help, list, add, show, run, remove, params, invalid name, multiline show/run) | \ No newline at end of file diff --git a/tests/t-telegram-listener-reap.sh b/tests/t-telegram-listener-reap.sh new file mode 100644 index 0000000..0589486 --- /dev/null +++ b/tests/t-telegram-listener-reap.sh @@ -0,0 +1,169 @@ +#!/usr/bin/env bash +set -euo pipefail +# t-telegram-listener-reap.sh — regression tests for the listener crash-loop: +# (a) a background command exiting non-zero (254) must NOT kill the daemon. +# The old code reaped via `wait "$pid"` under `set -e`, so any non-zero +# child exit aborted the listener and systemd Restart=always crash-looped +# it (offset reset to 0 → duplicate re-delivery). Reply must carry the +# real exit code. +# (b) getUpdates offset persistence: resumed from the state file across +# restarts (no duplicate burst after restart), invalid state falls back +# to 0, and empty batches never rewrite the file. +# (c) negative control: the OLD trap + `wait "$pid"` idiom still dies on 254 +# (proves the regression is real and the fix works). +# Hermetic: stubbed curl (no network), stubbed systemctl, real jq/flock/timeout. + +run_test() { + require_cmd jq "telegram reap" || return 0 + require_cmd timeout "telegram reap" || return 0 + + local sandbox stubs cfg runtime home listener curl_log batch_file + sandbox="$(mksandbox telegram-reap)" + stubs="$sandbox/stubs" + cfg="$sandbox/cfg" + runtime="$sandbox/runtime" + home="$sandbox/home" + listener="$ROOT/bin/pos-communication-telegram-listener" + curl_log="$sandbox/curl.log" + mkdir -p "$stubs" "$cfg" "$runtime" "$home" + : > "$curl_log" + + # ── command map: two commands with non-zero exits, one with output ── + cat > "$cfg/telegram_commands.env" <<'MAP' +/fail254=exit 254 +/with_out=echo boom; false +MAP + : > "$cfg/telegram_prefixes.env" + + # ── stub curl ── + # Serve one batch with 2 commands, then empty batches (sleep keeps the + # empty-poll loop from spinning while the daemon runs). + batch_file="$sandbox/batch.json" + cat > "$batch_file" <<'JSON' +{"ok":true,"result":[ + {"update_id":1,"message":{"message_id":10,"from":{"id":123},"chat":{"id":456},"text":"/fail254"}}, + {"update_id":2,"message":{"message_id":11,"from":{"id":123},"chat":{"id":456},"text":"/with_out"}} +]} +JSON + + cat > "$stubs/curl" <> "$curl_log" +for a in "\$@"; do + case "\$a" in + *getUpdates*) + if [ ! -e "$sandbox/reap.served" ]; then + touch "$sandbox/reap.served" + cat "$batch_file" + else + sleep 1 + printf '%s' '{"ok":true,"result":[]}' + fi + exit 0 + ;; + esac +done +printf '%s' '{"ok":true}' +STUB + chmod +x "$stubs/curl" + + printf '#!/usr/bin/env bash\nexit 1\n' > "$stubs/systemctl" + chmod +x "$stubs/systemctl" + + local common=(PATH="$stubs:/usr/bin:/bin" CONFIG_DIR="$cfg" + XDG_RUNTIME_DIR="$runtime" HOME="$home" + TELEGRAM_BOT_TOKEN=testbot TELEGRAM_CHAT_ID=456 TELEGRAM_OWNER_ID=123) + + # ── (a) non-zero command exit must not kill the daemon ── + # rc=124 → `timeout` killed an alive-and-polling daemon; rc=0 → clean exit. + # With the old code the daemon itself died with 254, which is caught here. + test_run_env "${common[@]}" -- timeout 10 "$listener" --run + + if [ "${TR_RC:-0}" -eq 124 ] || [ "${TR_RC:-0}" -eq 0 ]; then + printf ' PASS daemon survived child exit 254 (rc=%s)\n' "${TR_RC}" + else + printf ' FAIL daemon died with rc=%s (crash-loop regression)\n' "${TR_RC:-?}" + fi + + check_contains "/fail254 dispatched" "exec: /fail254" "${TR_OUT:-}" + check_contains "/with_out dispatched" "exec: /with_out" "${TR_OUT:-}" + + local curl_content + curl_content="$(cat "$curl_log")" + # The stub logs curl's RAW argv (--data-urlencode passes text unencoded + # to the wire, so the log shows "text=exit 254" with a real space). + # /fail254 has no output → reply text is "exit 254\nOK". + check_contains "/fail254 reply carries exit 254" "text=exit 254" "$curl_content" + # /with_out → "exit 1\nboom" — output preserved alongside the exit code. + check_contains "/with_out reply carries exit 1" "text=exit 1" "$curl_content" + check_contains "/with_out reply preserves output" "boom" "$curl_content" + + # ── offset persistence: written after processing updates ── + # update 1 → offset 2 persisted (both messages advance the offset). + check_eq "offset persisted after processing" "3" "$(cat "$cfg/telegram-listener.state")" + + # ── (b) offset resumed from the state file across restarts ── + # No state file → offset starts at 0; with one present it resumes from it. + local sb2 stubs2 cfg2 curl2 + sb2="$sandbox/resume" + stubs2="$sb2/stubs" + cfg2="$sb2/cfg" + curl2="$sb2/curl.log" + mkdir -p "$stubs2" "$cfg2" + : > "$curl2" + printf '99\n' > "$cfg2/telegram-listener.state" + : > "$cfg2/telegram_commands.env" + : > "$cfg2/telegram_prefixes.env" + + cat > "$stubs2/curl" <> "$curl2" +for a in "\$@"; do + case "\$a" in + *getUpdates*) + sleep 1 + printf '%s' '{"ok":true,"result":[]}' + exit 0 + ;; + esac +done +printf '%s' '{"ok":true}' +STUB + chmod +x "$stubs2/curl" + printf '#!/usr/bin/env bash\nexit 1\n' > "$stubs2/systemctl" + chmod +x "$stubs2/systemctl" + + local common2=(PATH="$stubs2:/usr/bin:/bin" CONFIG_DIR="$cfg2" + XDG_RUNTIME_DIR="$sb2/rt" HOME="$sb2/home" + TELEGRAM_BOT_TOKEN=testbot TELEGRAM_CHAT_ID=456 TELEGRAM_OWNER_ID=123) + mkdir -p "$sb2/rt" "$sb2/home" + + test_run_env "${common2[@]}" -- timeout 3 "$listener" --run + check_contains "offset resumed from persisted state file" "offset=99" "$(cat "$curl2")" + # Empty batches never rewrite the state file. + check_eq "empty batches do not rewrite state" "99" "$(cat "$cfg2/telegram-listener.state")" + + # invalid state file → fall back to 0 + printf 'garbage\n' > "$cfg2/telegram-listener.state" + : > "$curl2" + test_run_env "${common2[@]}" -- timeout 3 "$listener" --run + check_contains "invalid state file falls back to offset 0" "offset=0" "$(cat "$curl2")" + + # ── (c) negative control: the OLD trap + `wait "$pid"` idiom still dies ── + local old_rc old_out + set +e + old_out="$(bash -s 2>&1 <<'INNER' +set -euo pipefail +declare -A _EXIT_CODES=() +trap 'while _p=$(wait -n 2>/dev/null); do _EXIT_CODES[$_p]=$?; done' CHLD +exit 254 & pid=$! +sleep 0.3 +wait "$pid" 2>/dev/null; rc=$? +echo "still-alive rc=$rc" +INNER +)" + old_rc=$? + set -e + check_not_contains "old trap+wait idiom does not survive 254" "still-alive" "$old_out" + check_rc "old trap+wait idiom dies with 254" 254 "$old_rc" +} \ No newline at end of file