fix: stop telegram listener crash-loop from failed background commands
gates / consistency-and-conventions (push) Failing after 13s
gates / consistency-and-conventions (push) Failing after 13s
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
This commit is contained in:
Your Name
@@ -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 <cmdline> <msg_id> [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')"
|
||||
|
||||
@@ -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) |
|
||||
@@ -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" <<STUB
|
||||
#!/usr/bin/env bash
|
||||
printf 'curl %s\n' "\$*" >> "$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" <<STUB
|
||||
#!/usr/bin/env bash
|
||||
printf 'curl %s\n' "\$*" >> "$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"
|
||||
}
|
||||
Reference in New Issue
Block a user