fix(platform): stop on SIGTERM when already CLOSED, and exit when tunnel setup aborts - #346
Open
mastercoding wants to merge 2 commits into
Open
mastercoding wants to merge 2 commits into
mastercoding wants to merge 2 commits into
Conversation
on_signal() set shutting_down, called mqvpn_client_disconnect() and left the loop to cb_state_changed's CLOSED branch. mqvpn_client_disconnect() returns early when the state is already CLOSED or IDLE (mqvpn_client.c:3134), so a client that reached CLOSED on its own got no transition, cb_state_changed never ran, and the process went on ticking a dead client: SIGTERM and SIGINT did nothing and only SIGKILL stopped it. The shipped mqvpn-client@.service sets Restart=on-failure, which never sees the process exit, and `systemctl stop` falls back to systemd's default TimeoutStopSec; scripts/ci_e2e/run_test.sh hangs the same way, in stop_and_check_sanitizer's unbounded `wait` (sanitizer_check.sh:64-65). Break the loop in the handler, as svr_on_signal() already does. Darwin's on_signal is the same shape and gets the same line. The new scenario reaches CLOSED without root, a server or a TUN: --no-reconnect against a port nothing answers on. It fails on the parent commit — "client still running 5s after SIGTERM (state CLOSED) / the handler ran — the event loop was not broken" — and passes here, the process gone 500ms after the signal. Run in a debian:trixie aarch64 container (glibc 2.41), as root and as an unprivileged user. That the client sits in the loop at CLOSED there is the scenario's way in, not a behaviour this asserts is right: run_killswitch_test.sh's Test 3 expects a --no-reconnect client to exit instead, so the test fails saying so rather than passing empty if that ever becomes true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cb_tunnel_config_ready()'s fail: path — TUN creation, addressing, MTU, routes, the kill switch — called mqvpn_client_disconnect() and returned into the event loop. The library reads that as a user disconnect (mqvpn_client.c:3136 sets shutting_down), so cb_h3_conn_close's retry branch never fires (mqvpn_client.c:1331) although Reconnect defaults to true (config.c:1280), and nothing else ended the loop: no tunnel, no retry, no exit, and the shipped unit's Restart=on-failure with nothing to act on. platform_windows.c:232-238 already decided this — "local-side problems that reconnecting won't fix" — and reads the flag at :703. Same three statements in the same order, and the same rc line, on Linux and on Darwin. The one behaviour change: a setup failure on a *later* reconnect now ends a client that has been up for days, where before it idled on with no tunnel. Measured both ways, kill switch up and then iptables removed under a forced reconnect — on 6b80ddc "STILL ALIVE 20 s later ... SIGTERM did NOT stop it; needed SIGKILL", here "EXITED on its own, status 1". On the reported trigger (--kill-switch, no iptables binary, two netns, debian:trixie aarch64), 30 of 30 aborted starts exited by themselves with status 1 and none logged "reconnecting in"; on 6b80ddc the same 30 all needed SIGKILL after SIGTERM. The new netns suite drives fail: with --tun-name on a device that already exists in the namespace, so TUNSETIFF returns EINVAL inside the callback after the handshake. It runs in both e2e jobs; its Test 1 control proves an ordinary client on the same rig still comes up and stays up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughLinux and Darwin clients now exit after fatal tunnel setup failures and respond to signals in CLOSED or IDLE states. New tests cover SIGTERM handling, setup-abort exit status, setup-failure logging, and sanitizer results. ChangesClient shutdown behavior
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The shutdown and fatal-setup paths are covered by the stated implementation and integration checks, with no actionable merge risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation Issue Full details: Docstring CoverageExplanation Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 5 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #345. Two commits, both in the Linux platform layer — the library is untouched, so
there is no ABI or shared-behaviour change and the Android/iOS/API consumers are unaffected.
on_signal()breaks the loop itself instead of relying on a CLOSED transition thatmqvpn_client_disconnect()does not deliver when the state is already CLOSED(
mqvpn_client.c:3134). That early return is a documented idempotence guarantee(
tests/test_api.c:802,:814), so the loop exit belongs to the platform — which is theshape
svr_on_signal()already uses in the same file. One statement and a comment.The
fail:path marks fatal and exits non-zero — the decisionplatform_windows.c:232-238already made ("local-side problems that reconnecting won't fix"), with the same
rc = ctx.fatal_error ? 1 : 0;at:703.platform_darwin.ccarries both shapes verbatim.Before / after, kill-switch trigger, 30 aborted starts and 30 controls per run, same binary
and stop procedure within each run: 30 of 30 aborted starts needed SIGKILL -> 30 of 30 exit by
themselves with status 1. The controls are unchanged: still come up, still exit 0 on SIGTERM.
How to verify:
The first fails on
6b80ddc0and onb9227aa9("client still running 5s after SIGTERM (state CLOSED) / thehandler ran — the event loop was not broken") and passes here in 500 ms. The second has a
control that comes up and pings through the tunnel, then drives
fail:with--tun-nameon anexisting device: "client still running 20s after the abort" before, "exited 1 after 1s" here.
What this does not fix:
Reconnectstill does not cover setup failures. Making it do so is alibrary-side change — closing the connection as a failed attempt rather than a user disconnect,
so the gate at
mqvpn_client.c:1331fires — which alters shared behaviour for one platformcaller and puts POSIX at odds with Windows. Say the word and I will send that version instead.
One behaviour change, measured both ways: a setup failure on a later reconnect now ends a
client that had been up for days. What it replaces is a process holding the tunnel's name with no
tunnel, that a supervisor can neither see nor stop.
Summary by CodeRabbit
Bug Fixes
Tests