From 5f800542049cf6a97e41913a17b05a8513461762 Mon Sep 17 00:00:00 2001 From: Ramil Valitov Date: Thu, 17 Sep 2026 15:42:36 +0300 Subject: [PATCH 1/3] fix(traffic): support busybox flock and stop failing open on lock failure busybox's flock applet has no -w option, but because it installs a flock symlink to /bin/busybox, `command -v flock` succeeds on Alpine. The "flock is unavailable, skip locking" guards therefore never fired and the lock call itself was what failed. Two of the five call sites then matched `|| { ...; return 0; }` and reported success while writing nothing: flush_traffic_to_disk() dropped the counters, and save_traffic() left the Telegram daemon persisting no traffic at all. Both were completely silent. The other three sites aborted with "Could not acquire traffic lock", which made traffic resets, replication saves and monthly quota resets impossible on Alpine. Probe for -w support once and fall back to a non-blocking acquire, so the lock is still taken; and make the two fail-open paths report failure instead of claiming success. Verified on Alpine 3.20: tests/test_traffic_reset.sh goes from 21 failures to 0, and the new tests/test_flock_portability.sh pins both the busybox behaviour and the fail-closed behaviour on every platform. --- mtproxymax.sh | 73 +++++++++++++++++---- tests/test_flock_portability.sh | 110 ++++++++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 12 deletions(-) create mode 100644 tests/test_flock_portability.sh diff --git a/mtproxymax.sh b/mtproxymax.sh index b0ac44b..2365645 100644 --- a/mtproxymax.sh +++ b/mtproxymax.sh @@ -1777,6 +1777,38 @@ _load_all_cumulative_user_stats() { done } +# util-linux flock takes `-w SECS` to wait for a lock; busybox's flock applet does not, +# and `command -v flock` cannot tell the two apart because busybox installs a `flock` +# symlink to /bin/busybox. Testing for the binary therefore succeeds on Alpine while the +# lock call itself fails. Probe the capability once instead of assuming it. +_FLOCK_WAIT_SUPPORTED="" +_flock_supports_wait() { + if [ -z "$_FLOCK_WAIT_SUPPORTED" ]; then + if command -v flock >/dev/null 2>&1 && + (exec 9>/dev/null; flock -w 0 9) 2>/dev/null; then + _FLOCK_WAIT_SUPPORTED="yes" + else + _FLOCK_WAIT_SUPPORTED="no" + fi + fi + [ "$_FLOCK_WAIT_SUPPORTED" = "yes" ] +} + +# Take an exclusive lock on the already-open file descriptor $1. +# Returns 0 when the lock is held, non-zero when it could not be taken. A host with no +# flock at all proceeds unlocked, which is the long-standing behaviour in that case. +_lock_fd() { + local _fd="${1:?file descriptor required}" + command -v flock >/dev/null 2>&1 || return 0 + if _flock_supports_wait; then + flock -w 5 "$_fd" 2>/dev/null + else + # busybox: -w is unavailable, so take the lock without waiting rather than not + # locking at all. Losing the 5s grace period is far better than skipping the lock. + flock -n "$_fd" 2>/dev/null + fi +} + # One-shot flush of traffic counters to disk (for use before stop/restart) # Works standalone — loads cumulative from disk, computes delta from live metrics, saves back flush_traffic_to_disk() { @@ -1787,7 +1819,14 @@ flush_traffic_to_disk() { mkdir -p "$_stats_dir" 2>/dev/null # Acquire lock to prevent race with daemon's save_traffic exec 9>"${_stats_dir}/.traffic.lock" - flock -w 5 9 2>/dev/null || { exec 9>&- 2>/dev/null; return 0; } + # Report failure rather than returning 0: this used to claim success while writing + # nothing, which silently discarded counters. Plain stderr, not log_error(), because + # this also runs from the daemon, which has no logging helpers of its own. + if ! _lock_fd 9; then + echo "mtproxymax: could not acquire traffic lock — counters not flushed" >&2 + exec 9>&- 2>/dev/null + return 1 + fi # Load existing cumulative totals local cum_in=0 cum_out=0 @@ -2608,12 +2647,10 @@ secret_reset_traffic() { # disk update with save_traffic() and leave it a reset command to consume # before its next write, otherwise it would restore the pre-reset values. exec 9>"${STATS_DIR}/.traffic.lock" - if command -v flock &>/dev/null; then - flock -w 5 9 2>/dev/null || { - log_error "Could not acquire traffic lock. Telegram daemon may be busy." - exec 9>&- - return 1 - } + if ! _lock_fd 9; then + log_error "Could not acquire traffic lock. Telegram daemon may be busy." + exec 9>&- + return 1 fi if [ "$label" = "all" ]; then @@ -6627,8 +6664,10 @@ run_traffic_reset_global() { log_info "Resetting cumulative counters..." exec 9>"${_stats_dir}/.traffic.lock" - if command -v flock &>/dev/null; then - flock -w 5 9 2>/dev/null || { log_error "Could not acquire traffic lock. Daemon may be busy."; exec 9>&-; return 1; } + if ! _lock_fd 9; then + log_error "Could not acquire traffic lock. Daemon may be busy." + exec 9>&- + return 1 fi local _tmp @@ -11550,7 +11589,14 @@ save_traffic() { mkdir -p "$_tdir" 2>/dev/null # Acquire lock to prevent race with flush_traffic_to_disk exec 9>"${_tdir}/.traffic.lock" - flock -w 5 9 2>/dev/null || { exec 9>&- 2>/dev/null; return 0; } + # Fail loudly rather than returning 0: this used to report success while writing + # nothing, so on a host whose flock lacked -w the daemon never persisted counters and + # no error was ever surfaced. stderr rather than log_error() — see _lock_fd callers. + if ! _lock_fd 9; then + echo "mtproxymax: could not acquire traffic lock — traffic not saved" >&2 + exec 9>&- 2>/dev/null + return 1 + fi apply_pending_traffic_resets local _tmp=$(mktemp "${_tdir}/.traffic.XXXXXX" 2>/dev/null) || { exec 9>&-; return; } chmod 600 "$_tmp" @@ -12474,8 +12520,11 @@ save_replication() { chmod 600 "$tmp" # Serialise with sync-timer flock to prevent lost-update races with save_sync_status() exec 201>"${INSTALL_DIR:-/opt/mtproxymax}/.mtproxymax-sync.lock" 2>/dev/null || true - if command -v flock &>/dev/null; then - flock -w 5 201 2>/dev/null || { log_error "Could not acquire lock for replication config"; rm -f "$tmp"; exec 201>&- 2>/dev/null; return 1; } + if ! _lock_fd 201; then + log_error "Could not acquire lock for replication config" + rm -f "$tmp" + exec 201>&- 2>/dev/null + return 1 fi mv "$tmp" "$REPLICATION_FILE" exec 201>&- 2>/dev/null || true diff --git a/tests/test_flock_portability.sh b/tests/test_flock_portability.sh new file mode 100644 index 0000000..e94a359 --- /dev/null +++ b/tests/test_flock_portability.sh @@ -0,0 +1,110 @@ +#!/bin/bash +# Regression tests for lock acquisition on hosts whose flock is busybox's applet. +# +# busybox ships a `flock` that has no -w option. Because it is present on PATH, +# `command -v flock` succeeds and the "flock is unavailable, skip locking" guard never +# fires — but `flock -w 5 9` itself fails. On Alpine that made every traffic reset abort +# with a misleading "Could not acquire traffic lock" message, and made two save paths +# report success while writing nothing at all. +# +# The busybox behaviour is reproduced with a stub on PATH rather than by requiring an +# Alpine host, so this regression test runs on every platform in the CI matrix. +set -o pipefail + +if [ "${BASH_VERSINFO[0]:-0}" -lt 4 ]; then + echo "SKIP: bash 4+ required (got ${BASH_VERSION:-unknown})" >&2 + exit 0 +fi + +TEST_TMPDIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'mtp_flock_XXXXXX') +INSTALL_DIR="$TEST_TMPDIR/install" +mkdir -p "$INSTALL_DIR/relay_stats" + +# A flock that behaves like busybox's: it accepts -n/-s/-x/-u but rejects -w outright. +# FAKE_FLOCK_HOLD=1 additionally makes every acquire fail, to exercise the other branch. +FAKEBIN="$TEST_TMPDIR/bin" +mkdir -p "$FAKEBIN" +cat >"$FAKEBIN/flock" <<'FLOCK_EOF' +#!/bin/bash +for _a in "$@"; do + case "$_a" in + -w*) echo "flock: unrecognized option: w" >&2; exit 1 ;; + esac +done +[ "${FAKE_FLOCK_HOLD:-0}" = "1" ] && exit 1 +exit 0 +FLOCK_EOF +chmod +x "$FAKEBIN/flock" +PATH="$FAKEBIN:$PATH" +export PATH + +MTPROXYMAX_SOURCE_ONLY=true source "$(dirname "${BASH_SOURCE[0]}")/../mtproxymax.sh" +set +e +trap 'rm -rf "$TEST_TMPDIR"' EXIT + +TESTS_RUN=0 +TESTS_FAILED=0 +LAST_ERROR="" + +assert_eq() { + local name="$1" want="$2" got="$3" + TESTS_RUN=$((TESTS_RUN + 1)) + if [ "$got" = "$want" ]; then + printf ' PASS %s\n' "$name" + else + printf ' FAIL %s (got=%q want=%q)\n' "$name" "$got" "$want" + TESTS_FAILED=$((TESTS_FAILED + 1)) + fi +} + +check_root() { :; } +load_settings() { :; } +reload_proxy_config() { :; } +log_info() { :; } +log_success() { :; } +log_error() { LAST_ERROR="$*"; } +audit_log() { :; } + +SECRETS_LABELS=(alice) +SECRETS_ENABLED=(true) + +METRICS='# HELP test test +telemt_user_octets_from_client{user="alice"} 120 +telemt_user_octets_to_client{user="alice"} 340' + +_fetch_metrics() { printf '%s\n' "$METRICS"; } +curl() { printf '%s\n' "$METRICS"; } + +echo "flock portability tests" + +# The stub must really be the flock that resolves, or the test proves nothing. +assert_eq "busybox-style flock is the one on PATH" "$FAKEBIN/flock" "$(command -v flock)" + +# --- a busybox flock must not prevent the lock from being taken ----------------- +printf 'alice|1000|2000\n' >"$STATS_DIR/user_traffic" +printf 'alice|10|20\n' >"$STATS_DIR/user_traffic_snapshot" + +secret_reset_traffic alice no_reload +assert_eq "traffic reset succeeds despite flock lacking -w" "0" "$?" +assert_eq "reset work actually happened" "present" \ + "$([ -f "$STATS_DIR/.traffic_reset_pending" ] && echo present || echo absent)" + +# --- an unavailable lock must fail loudly, never silently succeed --------------- +# Both save paths used to `return 0` here, reporting success while writing nothing. +FAKE_FLOCK_HOLD=1 +export FAKE_FLOCK_HOLD +printf 'alice|1000|2000\n' >"$STATS_DIR/user_traffic" +rm -f "$STATS_DIR/.traffic_reset_pending" + +secret_reset_traffic alice no_reload +assert_eq "held lock makes the reset fail" "1" "$?" +assert_eq "held lock does not silently succeed" "absent" \ + "$([ -f "$STATS_DIR/.traffic_reset_pending" ] && echo present || echo absent)" + +flush_traffic_to_disk +assert_eq "flush_traffic_to_disk fails when the lock is unavailable" "1" "$?" + +unset FAKE_FLOCK_HOLD + +printf '\n%d tests, %d failures\n' "$TESTS_RUN" "$TESTS_FAILED" +[ "$TESTS_FAILED" -eq 0 ] From 927b7915967b0b79083147c00027b78fe1839092 Mon Sep 17 00:00:00 2001 From: Ramil Valitov Date: Thu, 17 Sep 2026 18:53:45 +0300 Subject: [PATCH 2/3] fix(traffic): stop silencing stderr in the lock paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fail-closed paths this PR adds could not be heard. `exec` with only redirections applies to the whole shell, so `exec 9>&- 2>/dev/null` leaves fd 2 pointing at /dev/null for the rest of the process — not just for that line. In save_replication the log_error added here writes to >&2 and was therefore swallowed by the line immediately above it. Three of these were introduced by this PR (two fd 9 closes in the new fail-closed branches, one fd 201 close); the other three are the same idiom already on main, in functions this PR rewrites. All six are fixed so the failure paths actually report something. Verified: after `exec 201>file || true`, fd 2 still points at the caller's stderr rather than /dev/null, and the flock suite still passes on both Debian 12 and Alpine 3.20. --- mtproxymax.sh | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/mtproxymax.sh b/mtproxymax.sh index 2365645..02820bd 100644 --- a/mtproxymax.sh +++ b/mtproxymax.sh @@ -1824,7 +1824,7 @@ flush_traffic_to_disk() { # this also runs from the daemon, which has no logging helpers of its own. if ! _lock_fd 9; then echo "mtproxymax: could not acquire traffic lock — counters not flushed" >&2 - exec 9>&- 2>/dev/null + exec 9>&- return 1 fi @@ -11594,7 +11594,7 @@ save_traffic() { # no error was ever surfaced. stderr rather than log_error() — see _lock_fd callers. if ! _lock_fd 9; then echo "mtproxymax: could not acquire traffic lock — traffic not saved" >&2 - exec 9>&- 2>/dev/null + exec 9>&- return 1 fi apply_pending_traffic_resets @@ -12519,15 +12519,18 @@ save_replication() { chmod 600 "$tmp" # Serialise with sync-timer flock to prevent lost-update races with save_sync_status() - exec 201>"${INSTALL_DIR:-/opt/mtproxymax}/.mtproxymax-sync.lock" 2>/dev/null || true + # No 2>/dev/null here: `exec` with only redirections applies to the whole shell, so it + # would silence stderr for the rest of the process — including the log_error below, + # which is exactly the failure this function is supposed to report. + exec 201>"${INSTALL_DIR:-/opt/mtproxymax}/.mtproxymax-sync.lock" || true if ! _lock_fd 201; then log_error "Could not acquire lock for replication config" rm -f "$tmp" - exec 201>&- 2>/dev/null + exec 201>&- return 1 fi mv "$tmp" "$REPLICATION_FILE" - exec 201>&- 2>/dev/null || true + exec 201>&- || true } # Load replication.conf @@ -12849,7 +12852,7 @@ do_sync() { main() { # Prevent overlapping sync runs - exec 200>"${LOCK_FILE}" 2>/dev/null || true + exec 200>"${LOCK_FILE}" || true if command -v flock &>/dev/null; then flock -n 200 || { echo "[$(date '+%Y-%m-%d %H:%M:%S')] SKIP: Another sync already running" >> "${REPLICATION_LOG}" From 04ef8b74d2cd0e4c55aee0d91ed3e69f0a935c1a Mon Sep 17 00:00:00 2001 From: Ramil Valitov Date: Thu, 17 Sep 2026 23:12:11 +0300 Subject: [PATCH 3/3] fix(traffic): define the lock helpers inside the generated daemon The helpers added for busybox flock were defined only in the manager script, but the bot daemon is emitted from a single-quoted heredoc and never sources the manager -- as its own header comment states. The generated daemon therefore called an undefined _lock_fd: save_traffic() got 127 back, took the new fail-closed branch and returned 1 without writing a single counter. Parsing the heredoc out of mtproxymax.sh yields 1052 lines with one call site and no definition of either helper. Because an undefined command fails on every host, this did not merely leave the Alpine bug unfixed: it broke accounting on util-linux too, where the previous `flock -w 5 9 ... || { ...; return 0; }` had worked. The daemon is the only writer of accounting in production, so counters stopped being persisted everywhere. Copy both helpers into the heredoc next to save_traffic(), and stop redirecting the update_traffic call to /dev/null so that a lock failure reaches the log instead of being discarded. tests/test_flock_portability.sh only ever exercised the manager, which is why CI stayed green. It now extracts the generated daemon, asserts that it defines the helpers it calls, and runs the daemon's own save_traffic under a busybox-style flock. Verified: the new daemon cases fail 7/16 against the previous revision of mtproxymax.sh and pass 16/16 with it; the util-linux, held-lock and no-flock branches all behave as before. --- mtproxymax.sh | 37 +++++++++++++++++- tests/test_flock_portability.sh | 69 +++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/mtproxymax.sh b/mtproxymax.sh index 02820bd..a2cbd86 100644 --- a/mtproxymax.sh +++ b/mtproxymax.sh @@ -11584,6 +11584,39 @@ load_traffic() { fi } +# Duplicated from the manager script: this heredoc has a quoted delimiter, so it is +# written out literally and the daemon never sources the manager. Without these two +# definitions save_traffic() below calls an undefined _lock_fd, gets 127 back, and +# returns before writing any counters — which is exactly the accounting loss this +# change set out to fix. Keep identical to _flock_supports_wait/_lock_fd above. +_FLOCK_WAIT_SUPPORTED="" +_flock_supports_wait() { + if [ -z "$_FLOCK_WAIT_SUPPORTED" ]; then + if command -v flock >/dev/null 2>&1 && + (exec 9>/dev/null; flock -w 0 9) 2>/dev/null; then + _FLOCK_WAIT_SUPPORTED="yes" + else + _FLOCK_WAIT_SUPPORTED="no" + fi + fi + [ "$_FLOCK_WAIT_SUPPORTED" = "yes" ] +} + +# Take an exclusive lock on the already-open file descriptor $1. +# Returns 0 when the lock is held, non-zero when it could not be taken. A host with no +# flock at all proceeds unlocked, which is the long-standing behaviour in that case. +_lock_fd() { + local _fd="${1:?file descriptor required}" + command -v flock >/dev/null 2>&1 || return 0 + if _flock_supports_wait; then + flock -w 5 "$_fd" 2>/dev/null + else + # busybox: -w is unavailable, so take the lock without waiting rather than not + # locking at all. Losing the 5s grace period is far better than skipping the lock. + flock -n "$_fd" 2>/dev/null + fi +} + save_traffic() { local _tdir="${INSTALL_DIR}/relay_stats" mkdir -p "$_tdir" 2>/dev/null @@ -12208,7 +12241,9 @@ while true; do _now=$(date +%s) if [ $((_now - _last_traffic_update)) -ge 60 ] && is_running; then _last_traffic_update=$_now - update_traffic 2>/dev/null + # Deliberately not silenced: save_traffic() reports a failed lock on stderr, and + # discarding that is what let unpersisted counters go unnoticed in production. + update_traffic [ "${PORTAL_ENABLED:-false}" = "true" ] && "${INSTALL_DIR}/mtproxymax" portal generate &>/dev/null # Connection log: append per-user activity (delta = current cumulative - previous cumulative) diff --git a/tests/test_flock_portability.sh b/tests/test_flock_portability.sh index e94a359..aebaf58 100644 --- a/tests/test_flock_portability.sh +++ b/tests/test_flock_portability.sh @@ -106,5 +106,74 @@ assert_eq "flush_traffic_to_disk fails when the lock is unavailable" "1" "$?" unset FAKE_FLOCK_HOLD +# --- the generated daemon must carry its own copy of the lock helpers ------------ +# The bot daemon is emitted from a single-quoted heredoc and never sources the +# manager, so every helper it calls has to be defined inside that heredoc. These two +# helpers were added only to the manager, which left the generated daemon calling an +# undefined _lock_fd: save_traffic() then returned 1 and persisted no counters at all. +# Everything above passes either way, because it exercises the manager, not the daemon. +TEST_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +DAEMON_FULL="$TEST_TMPDIR/daemon_full.sh" +awk "/<<[[:space:]]*'TELEGRAM_SCRIPT'/{f=1;next} /^TELEGRAM_SCRIPT\$/{f=0} f" \ + "$TEST_ROOT/mtproxymax.sh" >"$DAEMON_FULL" +assert_eq "generated daemon captured" "yes" \ + "$([ -s "$DAEMON_FULL" ] && echo yes || echo no)" + +# Declarations and function bodies only: stop before the cleanup trap so that +# sourcing this cannot start the daemon's main loop. +DAEMON_PRELUDE="$TEST_TMPDIR/daemon_prelude.sh" +awk '/^trap /{exit} {print}' "$DAEMON_FULL" >"$DAEMON_PRELUDE" + +assert_eq "daemon defines _lock_fd" "yes" \ + "$(grep -qE '^_lock_fd\(\)' "$DAEMON_FULL" && echo yes || echo no)" +assert_eq "daemon defines _flock_supports_wait" "yes" \ + "$(grep -qE '^_flock_supports_wait\(\)' "$DAEMON_FULL" && echo yes || echo no)" + +# Exercise the daemon's own save_traffic(), not the manager's. +DAEMON_HARNESS="$TEST_TMPDIR/daemon_save_traffic.sh" +cat >"$DAEMON_HARNESS" <<'HARNESS_EOF' +#!/bin/bash +source "$2" +INSTALL_DIR="$1" +TRAFFIC_FILE="${INSTALL_DIR}/relay_stats/cumulative_traffic" +USER_TRAFFIC_FILE="${INSTALL_DIR}/relay_stats/user_traffic" +mkdir -p "${INSTALL_DIR}/relay_stats" +_cum_in=700; _cum_out=900 +_cum_user_in=([alice]=11); _cum_user_out=([alice]=22) +_prev_user_in=([alice]=1); _prev_user_out=([alice]=2) +_prev_total_in=5; _prev_total_out=6 +save_traffic +echo "rc=$?" +HARNESS_EOF + +DAEMON_INSTALL="$TEST_TMPDIR/daemon_install" +daemon_run() { + rm -rf "$DAEMON_INSTALL" + bash "$DAEMON_HARNESS" "$DAEMON_INSTALL" "$DAEMON_PRELUDE" 2>&1 +} +daemon_file() { cat "$DAEMON_INSTALL/relay_stats/$1" 2>/dev/null; } + +# The daemon is the only writer of accounting in production, and this is the path +# that silently stopped persisting on every distro, not just Alpine. +_dout=$(daemon_run) +assert_eq "daemon save_traffic succeeds under busybox flock" "rc=0" \ + "$(printf '%s\n' "$_dout" | grep '^rc=')" +assert_eq "daemon wrote cumulative_traffic" "700|900" "$(daemon_file cumulative_traffic)" +assert_eq "daemon wrote user_traffic" "alice|11|22" "$(daemon_file user_traffic)" +assert_eq "daemon wrote user_traffic_snapshot" "alice|1|2" \ + "$(daemon_file user_traffic_snapshot)" +assert_eq "daemon wrote global_traffic_snapshot" "5|6" \ + "$(daemon_file global_traffic_snapshot)" + +# A lock that is genuinely held must fail loudly rather than persist nothing. +FAKE_FLOCK_HOLD=1 +export FAKE_FLOCK_HOLD +_dout=$(daemon_run) +assert_eq "daemon save_traffic fails when the lock is held" "rc=1" \ + "$(printf '%s\n' "$_dout" | grep '^rc=')" +assert_eq "daemon reports the failure on stderr" "yes" \ + "$(printf '%s\n' "$_dout" | grep -q 'traffic not saved' && echo yes || echo no)" +unset FAKE_FLOCK_HOLD + printf '\n%d tests, %d failures\n' "$TESTS_RUN" "$TESTS_FAILED" [ "$TESTS_FAILED" -eq 0 ]