fix: keep unhandled service-thread errors from killing the root service #37

Merged

mysticalsoap merged 4 commits from fix/guard-service-threads into trunk

2026-08-17 21:09:13 -04:00

mysticalsoap commented

2026-08-17 18:38:53 -04:00

Owner

Closes #17.

Problem

TunnelThread.run's only except KeyError was the OpenVPN dispatch fallback, not a guard: anything else raised while establishing a tunnel reached the top of the QThread and PyQt5 aborted the root D-Bus service, dropping every active tunnel. The fallback also rerouted a KeyError raised anywhere inside wireguard() into openvpn() with a WireGuard server dict. 14dd13a fixed this pattern for the importer only.

Fix

One pattern, as the issue proposed: GuardedThread (aqomui/threads.py) owns run(), subclasses implement work() and report_failure(), and nothing reaches the top of the thread. Both QThreads the service owns are on it -- AddServers migrated with no behavior change, TunnelThread newly guarded. A failed tunnel attempt now logs the cause, deletes the outbound allow rule(s) it inserted (previously a failed attempt left a permanent ACCEPT hole in the killswitch), and reports conn_attempt_failed(_bypass) -- statuses the GUI already answers with cleanup and a notification.

Along the way: wg()'s local handler emitted "fail", which the GUI cannot dispatch (getattr on the status name -- no such method), and logged nothing; deleted in favor of the guard. The two related swallows from the issue are narrowed: allow_dest_ip no longer labels every failure "not a valid ip address", and the hop-ip bare except catches only what the no-hop case produces.

Verification

tests/test_tunnel_failure.py (no root, network or account) -- verified to fail 8 checks against the unfixed code, including the wireguard-to-openvpn reroute, all passing after. tests/test_import_failure.py unchanged and still passing.

Known gaps (pre-existing, out of scope here)

The plain threading.Thread helpers openvpn() starts (hop/ssl/ssh) are not QThreads: an exception there doesn't abort the service, but the hop case leaves work() waiting on connect_status forever with no status reported.
The check() harness is now copy-pasted across five test scripts; worth extracting if a sixth appears.

🤖 Generated with Claude Code

Closes #17. **Problem** `TunnelThread.run`'s only `except KeyError` was the OpenVPN dispatch fallback, not a guard: anything else raised while establishing a tunnel reached the top of the QThread and PyQt5 aborted the root D-Bus service, dropping every active tunnel. The fallback also rerouted a `KeyError` raised anywhere inside `wireguard()` into `openvpn()` with a WireGuard server dict. 14dd13a fixed this pattern for the importer only. **Fix** One pattern, as the issue proposed: `GuardedThread` (aqomui/threads.py) owns `run()`, subclasses implement `work()` and `report_failure()`, and nothing reaches the top of the thread. Both QThreads the service owns are on it -- `AddServers` migrated with no behavior change, `TunnelThread` newly guarded. A failed tunnel attempt now logs the cause, deletes the outbound allow rule(s) it inserted (previously a failed attempt left a permanent ACCEPT hole in the killswitch), and reports `conn_attempt_failed`(`_bypass`) -- statuses the GUI already answers with cleanup and a notification. Along the way: `wg()`'s local handler emitted `"fail"`, which the GUI cannot dispatch (`getattr` on the status name -- no such method), and logged nothing; deleted in favor of the guard. The two related swallows from the issue are narrowed: `allow_dest_ip` no longer labels every failure "not a valid ip address", and the hop-ip bare `except` catches only what the no-hop case produces. **Verification** `tests/test_tunnel_failure.py` (no root, network or account) -- verified to fail 8 checks against the unfixed code, including the wireguard-to-openvpn reroute, all passing after. `tests/test_import_failure.py` unchanged and still passing. **Known gaps** (pre-existing, out of scope here) - The plain `threading.Thread` helpers `openvpn()` starts (hop/ssl/ssh) are not QThreads: an exception there doesn't abort the service, but the hop case leaves `work()` waiting on `connect_status` forever with no status reported. - The `check()` harness is now copy-pasted across five test scripts; worth extracting if a sixth appears. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

mysticalsoap added 4 commits

2026-08-17 18:38:53 -04:00

change: move the import guard onto a shared thread base 9f10fabe0a

14dd13a guarded AddServers.run in place. The same abort-on-escape rule
applies to any QThread the root service owns, so the guard becomes a base
class: subclasses implement work() and report_failure(), and GuardedThread
guarantees nothing reaches the top of run(). No behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

fix: keep an unhandled tunnel error from killing the service d0bee2fd15

TunnelThread.run caught only KeyError, and that except was the OpenVPN
dispatch fallback, not a guard -- anything else raised while establishing
a tunnel reached the top of the QThread and PyQt5 aborted the root
service, dropping every active tunnel. The fallback also rerouted a
KeyError raised anywhere inside wireguard() into openvpn() with a
WireGuard server dict.

TunnelThread now runs on GuardedThread: the dispatch keys on
server_dict.get("tunnel"), so a missing key still means OpenVPN, and an
escaped error ends as conn_attempt_failed -- the status the GUI already
answers with cleanup and a notification. report_failure also deletes the
outbound allow rule(s) the attempt inserted, which nothing else takes
back on this path.

tests/test_tunnel_failure.py covers it (no root, network or account) and
fails against the unfixed code, including the wireguard-to-openvpn reroute.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

fix: let a WireGuard setup error reach the guard's report 569dc42525

wg()'s own except emitted "fail", a status the GUI cannot dispatch --
it resolves status strings to methods of the same name via getattr and
has none called fail, so reporting the failure raised AttributeError in
the GUI instead of telling the user anything. It also logged nothing, so
the log the notification points at had no cause in it.

GuardedThread.report_failure already does this right: the cause is
logged, the attempt's firewall rules come back out, and the GUI gets
conn_attempt_failed. Delete the local handler and let it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

change: stop mislabeling what broad exception handlers swallow 0b6f2b67b5

allow_dest_ip caught BaseException and reported every failure as an
invalid ip address -- a missing iptables binary, or SystemExit, got the
same line. It now catches Exception and logs what actually happened.

The bare except around the hop ip is a real fallback (no hop configured),
so it stays one, narrowed to the KeyError/TypeError that case produces.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

mysticalsoap merged commit d263fd0838 into trunk

2026-08-17 21:09:13 -04:00

mysticalsoap deleted branch fix/guard-service-threads

2026-08-17 21:09:13 -04:00

mysticalsoap referenced this pull request

2026-08-17 21:21:41 -04:00

change: convert the test scripts to pytest #38