Skip to content

fix(traffic): support busybox flock and stop failing open on lock failure - #146

Open
rvalitov wants to merge 3 commits into
SamNet-dev:mainfrom
rvalitov:fix/flock-busybox
Open

rvalitov wants to merge 3 commits into
SamNet-dev:mainfrom
rvalitov:fix/flock-busybox

Conversation

@rvalitov

Copy link
Copy Markdown
Contributor

Problem

busybox ships a flock applet that has no -w option. Because it installs a flock symlink to /bin/busybox, command -v flock succeeds on Alpine — so the "flock is unavailable, skip locking" guards never fired, and it was the lock call itself that failed:

$ flock -w 5 9
flock: unrecognized option: w
$ flock -n 9          # -n IS supported

Five call sites pass -w 5. Three aborted with "Could not acquire traffic lock", making all of these impossible on Alpine:

  • secret_reset_trafficsecret reset-traffic, and monthly quota resets (which call it with output discarded, so they failed silently)
  • run_traffic_reset_global — global traffic reset
  • save_replication — replication config was never saved

The other two were worse: they had no guard at all and matched || { exec 9>&-; return 0; }, so they reported success while writing nothing.

  • flush_traffic_to_disk — counters silently discarded before stop/restart
  • save_traffic — the Telegram daemon persisted no traffic at all, with no error anywhere

Fix

Probe for -w support once, and fall back to flock -n, which busybox does support. The lock is still taken; only the 5-second grace period is lost. On a host with no flock at all, behaviour is unchanged (proceed unlocked).

_lock_fd centralises all five sites, and the two fail-open paths now return non-zero instead of 0. Those two write to stderr rather than log_error because save_traffic also runs inside the generated daemon script, which has no logging helpers.

Testing

New tests/test_flock_portability.sh places a busybox-style flock on PATH, so the regression is caught on every platform rather than only on Alpine. It also pins the fail-closed behaviour.

Verified on Alpine 3.20: tests/test_traffic_reset.sh goes from 21 failures to 0. No test result changes on Debian 12, Ubuntu 22.04/24.04 or Fedora 41.

…lure

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.
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.
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.
@rvalitov
rvalitov marked this pull request as ready for review September 17, 2026 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant