fix: stop VPN Bypass's own route churn destabilising GlobalProtect (#65) - #66
Conversation
A re-route was a full teardown-and-rebuild: removeAllRoutes() followed by applyAllRoutesInternal(). Because removeAllRoutes() clears activeRoutes, the shouldSkipReapply no-op guard was structurally unable to fire on that path — activePairs was always empty, so it never equalled desiredPairs — and every re-route re-issued the ENTIRE route set (~3N kernel mutations: N deletes plus N delete-before-adds). In Bypass mode those routes egress via the LOCAL gateway, so the usual trigger (a VPN interface renumber) changes no destination|gateway pair at all: the whole storm rebuilt routes to the values they already had. Every mutation raises a kernel route-change event, and GlobalProtect re-validates its own gateway route on each one, tearing down and re-negotiating the tunnel when that lookup transiently fails — measured on a live machine as 757 route-change events and 48 tunnel teardowns in one day, ~27s of downtime each. performReroute() now calls applyAllRoutesInternal() directly, which computes the desired set, skips entirely when it already matches (zero mutations), and drops genuinely stale destinations via commitAppliedRoutes' orphan cleanup. A real gateway change is still applied, in place, without a window where the route is absent — which also closes a brief VPN-Only leak window the teardown opened. Also release the route-operation gate via defer. performReroute was the only one of nine gate holders releasing without it, so an unwind at any suspension point leaked the gate permanently: acquireRouteOperation() try-acquires, so every later apply, re-route and DNS refresh would give up silently for the life of the process. The leak-critical latch-clear-at-START ordering is unchanged. New RerouteChurnTests: 3 tests, proven non-vacuous (re-injecting the teardown fails the regression test). Full suite shows no new failures against baseline.
The helper issued a blind `route -n delete` before every `route -n add`. That
cost two kernel routing-table writes per route even when the route was already
correct, and it briefly left the destination with NO route at all.
Both matter because every routing-table write raises a kernel route-change event,
and GlobalProtect's connection monitor re-validates its own gateway route on each
one. Its teardowns are preceded by `Failed to find route for <gateway>` — exactly
what a transient removal produces. Measured on a live machine: 757 route-change
events and 48 tunnel teardowns in a day, ~27s of downtime each. The blind delete
could also remove a route this app never installed.
installRoute() now walks a ladder that never opens a gap:
1. `route change` — rewrite in place (one write, no gap); the re-apply case.
2. `route add` — only when nothing was there to change.
3. `delete`+`add` — last resort, only when add reports the route exists (a race
with another writer). The only remaining path that can open a gap, and now
rare rather than universal.
Also:
- Serialise route/hosts mutations across XPC connections. HelperToolDelegate makes
a fresh HelperTool per connection and the app recreates its connection on an XPC
deadline, so handlers could previously mutate the table concurrently — the
condition behind the simultaneous /sbin/route processes reported in #65.
getVersion is deliberately NOT serialised: it is the app's liveness probe, and
queueing it behind a long batch would time it out and force a helper reinstall.
- Add os_log to the helper, which previously produced no diagnostics whatsoever:
log stream --predicate 'subsystem == "com.geiserx.vpnbypass.helper"' --info
- Bump helperVersion 1.8.0 -> 1.9.0 (and Helper/Info.plist to match, enforced by
HelperVersionDriftTests) so installed helpers actually pick this up.
- Guard RerouteLatchTimingTests with the same free-gate precondition used by the
new churn tests: RouteManager is a private-init singleton that loads the real
on-disk config on first touch, so on a machine with a populated config unrelated
startup work can hold the route gate and make those assertions vacuous.
855 tests, 0 failures.
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe helper now serializes route mutations, logs route operations, and uses change-first installation. ChangesRouting stability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Tests/VPNBypassTests/RerouteChurnTests.swift (1)
69-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack the hermeticity work with
bd.This comment uses
.goal-loop/GOAL-WORKLOG.mdas the task tracker. Replace the worklog reference with the applicablebdissue identifier.As per coding guidelines, “Use bd for all task tracking.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/VPNBypassTests/RerouteChurnTests.swift` around lines 69 - 70, Update the explanatory comment near the reroute churn tests to replace the `.goal-loop/GOAL-WORKLOG.md` worklog reference with the applicable `bd` issue identifier, while preserving the existing context about making the suite hermetic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/CHANGELOG.md`:
- Line 11: Correct the “GlobalProtect disconnect loop” changelog entry so the
reported disconnect frequency matches the stated 48 teardowns over a 24-hour
day: use approximately one disconnect every 30 minutes, or explicitly identify
the shorter active measurement window that justifies the existing eight-minute
figure.
In `@Helper/HelperTool.swift`:
- Around line 116-122: Update updateHostsFile so its complete hosts-file read,
filtering, and write transaction executes synchronously on the existing static
routeQueue. Preserve the current read-modify-write behavior and error handling
while ensuring concurrent XPC connections cannot interleave the transaction.
- Around line 245-251: The route replacement fallback in the exists-handling
branch must stop when deletion fails. In the visible `executeRoute` call within
`HelperTool`, retain its result, return failure with the deletion error when
`success` is false, and only execute the replacement add after successful
removal.
In `@ROADMAP.md`:
- Around line 3-10: Update the audit-token verification version attribution in
the Current State section of ROADMAP.md from 1.9.0 to 1.7.0, matching the
history documented by HelperProtocol.swift; leave the separate 1.9.0 route,
serialization, and logging attribution unchanged.
---
Nitpick comments:
In `@Tests/VPNBypassTests/RerouteChurnTests.swift`:
- Around line 69-70: Update the explanatory comment near the reroute churn tests
to replace the `.goal-loop/GOAL-WORKLOG.md` worklog reference with the
applicable `bd` issue identifier, while preserving the existing context about
making the suite hermetic.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d0bfcfcd-f7e3-4532-876b-0bac1f64701c
📒 Files selected for processing (9)
.gitignoreHelper/HelperTool.swiftHelper/Info.plistROADMAP.mdSources/VPNBypassCore/HelperProtocol.swiftSources/VPNBypassCore/RouteManager.swiftTests/VPNBypassTests/RerouteChurnTests.swiftTests/VPNBypassTests/RerouteDeciderTests.swiftdocs/CHANGELOG.md
| ## [3.1.7] - 2026-08-04 | ||
|
|
||
| ### Fixed | ||
| - **VPN Bypass no longer destabilises other VPN clients (GlobalProtect disconnect loop).** Two habits made the app rewrite the routing table constantly even when nothing had changed, and every routing-table write is an event that enterprise VPN clients react to — one measured machine logged 757 route-change events and 48 tunnel teardowns in a single day, roughly one disconnect every eight minutes with ~27 seconds of downtime each. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'README availability:'
[ -f README.md ] && echo 'README.md exists' || echo 'README.md missing'
echo 'CHANGELOG availability:'
[ -f docs/CHANGELOG.md ] && echo 'docs/CHANGELOG.md exists' || echo 'docs/CHANGELOG.md missing'
if [ -f README.md ]; then
echo 'README.md first 80 lines:'
sed -n '1,80p' README.md | cat -n
fi
echo 'CHANGELOG relevant lines:'
sed -n '1,80p' docs/CHANGELOG.md | cat -n
echo 'Search for related terms:'
rg -n "disconnect|tunnel teardown|routing table|VPN Bypass|route-change|757|48|eight minutes|30 minutes" . || trueRepository: GeiserX/VPN-Bypass
Length of output: 1922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Working directory:'
pwd
echo 'Git remote/config:'
git remote -v 2>/dev/null || true
git status --short 2>/dev/null || true
echo 'README availability:'
[ -f README.md ] && echo 'README.md exists' || echo 'README.md missing'
echo 'CHANGELOG availability:'
[ -f docs/CHANGELOG.md ] && echo 'docs/CHANGELOG.md exists' || echo 'docs/CHANGELOG.md missing'
if [ -f README.md ]; then
echo 'README.md first 80 lines:'
sed -n '1,80p' README.md | cat -n
fi
if [ -f docs/CHANGELOG.md ]; then
echo 'CHANGELOG relevant lines:'
sed -n '1,80p' docs/CHANGELOG.md | cat -n
fi
echo 'Search for related terms:'
rg -n "disconnect|tunnel teardown|routing table|VPN Bypass|route-change|757|48|eight minutes|30 minutes" . || trueRepository: GeiserX/VPN-Bypass
Length of output: 1922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'PWD:'
pwd
echo 'LS top-level files:'
ls -la | sed -n '1,60p'
echo 'Git config/remote:'
git config --list --show-scope 2>/dev/null || true
git remote -v 2>/dev/null || true
echo 'README existence:'
if [ -f README.md ]; then
echo 'README.md bytes:'
wc -c README.md
sed -n '1,80p' README.md | cat -n
else
echo 'README.md missing'
fi
echo 'CHANGELOG existence:'
if [ -f docs/CHANGELOG.md ]; then
echo 'docs/CHANGELOG.md bytes:'
wc -c docs/CHANGELOG.md
sed -n '1,80p' docs/CHANGELOG.md | cat -n
else
echo 'docs/CHANGELOG.md missing'
fi
echo 'Search related terms:'
rg -n "disconnect|tunnel teardown|routing table|VPN Bypass|route-change|757|48|eight minutes|30 minutes" . || trueRepository: GeiserX/VPN-Bypass
Length of output: 1922
Correct the disconnect-rate wording.
If “a single day” means 24 hours, 48 teardowns equal one disconnect every 30 minutes, not every eight minutes. Correct the rate or state the shorter active measurement window that supports eight minutes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/CHANGELOG.md` at line 11, Correct the “GlobalProtect disconnect loop”
changelog entry so the reported disconnect frequency matches the stated 48
teardowns over a 24-hour day: use approximately one disconnect every 30 minutes,
or explicitly identify the shorter active measurement window that justifies the
existing eight-minute figure.
…correct docs CodeRabbit on #66: - MAJOR: updateHostsFile did a read-modify-write of /etc/hosts outside routeQueue, so two concurrent XPC connections could read the same content and the later write would silently discard the earlier update — while the queue's own comment claimed it serialised hosts mutations. The transaction now runs on routeQueue. - installRoute's last-resort replace discarded the delete result, so a failed delete surfaced as a secondary 'exists' error from the follow-up add, hiding the cause. It now returns the deletion error. - CHANGELOG: 48 teardowns was over ~6.5h of active use, not a 24h day; stated the measurement window that supports 'every 8 minutes'. - ROADMAP: audit-token verification shipped in 1.7.0 and the cdhash pin in 1.8.0; 1.9.0 is the current helper. Attribution corrected. 855 tests, 0 failures; universal helper builds.
|
@coderabbitai All four addressed — thanks, the hosts-file one was a real hole and my own comment was over-claiming. ✅ MAJOR — ✅ Delete result discarded in the replace path. Fixed — a failed delete now returns its own error instead of letting the follow-up ✅ CHANGELOG disconnect rate. You're right: 48 teardowns over 24h is one per 30 minutes. The "every 8 minutes" figure comes from ~6.5 hours of active use (the log window contained a ~4.7h sleep gap with no logging). Reworded to state that window explicitly. ✅ ROADMAP version attribution. Corrected: audit-token verification shipped in 1.7.0, the fail-closed cdhash pin in 1.8.0, and 1.9.0 is the current helper (route change-in-place, serialisation, os_log). The sentence now attributes each to its own version. Full suite green (855 tests, 0 failures) and the universal helper builds. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
1 participant
Stop VPN Bypass destabilising other VPN clients (#65)
Fixes #65.
The problem, measured
On a live machine running GlobalProtect,
PanGPS.logrecorded 757 route-change events and 48 full tunnel teardowns in one day — about one disconnect every 8 minutes, ~27s of downtime each. 83% of the teardowns were preceded, within ~0.02s, byFailed to find route for <gateway>.A/B: with the app running, ~18 GP route-change events/60s and 17–18 concurrent
/sbin/routeprocesses; with the helper booted out, 2 events and 0 processes. ~90% of the route-table churn was ours.Every routing-table write raises a kernel route-change event. GlobalProtect re-validates its own gateway route on each one and tears the tunnel down when that lookup fails. So the fix is not to make our writes faster — it is to stop writing when nothing changed, and to never leave a destination momentarily without a route.
Two churn generators, both removed
1. Re-routing rebuilt everything instead of reconciling.
performReroute()didremoveAllRoutes()+applyAllRoutesInternal(). BecauseremoveAllRoutes()clearsactiveRoutes, theshouldSkipReapplyno-op guard was structurally unable to fire on that path —activePairswas always empty, so it never equalleddesiredPairs. Every re-route re-issued the entire set (~3N writes).In Bypass mode the routes egress via the local gateway, so the usual trigger — a VPN interface renumber — changes no
destination|gatewaypair at all. The whole storm rebuilt routes to the values they already had. It now reconciles: applies only genuine differences, does nothing when already correct.2. The helper deleted every route before adding it. A blind
route -n deletepreceded everyroute -n add: two writes per route even when correct, and a brief window with no route for that destination — exactly what makes another client's gateway check fail. It could also delete a route this app never installed. Now:changein place →addif nothing was there →delete+addonly on a genuine race.Also fixed
HelperToolDelegatecreates a freshHelperToolper connection, and the app recreates its connection on an XPC deadline — the condition behind the simultaneousrouteprocesses).getVersionis deliberately not serialised: it is the liveness probe, and queueing it behind a batch would time it out and force a spurious helper reinstall.performReroutewas the only one of nine gate holders releasing the route-operation gate withoutdefer. Since the body now suspends during a real apply, an unwind there would leak it permanently — andacquireRouteOperation()try-acquires, so every later apply, re-route and DNS refresh would silently give up for the life of the process.log stream --predicate 'subsystem == "com.geiserx.vpnbypass.helper"' --info.Verification
swift buildclean;make build-helperuniversal; 855 tests, 0 failures.RerouteChurnTests, proven non-vacuous: re-injecting the old teardown makes the regression test fail; restoring the fix makes it pass. The detector isrouteEpoch, whichremoveAllRoutes()bumps as its unconditional first statement and which the apply path never touches, with a negative-control test proving the detector moves.ClassicRouteCompilerTests,RouteCompilerTests,ReapplySkipTestsgreen).RunAtLoadunchanged.Note on test hermeticity
RouteManageris aprivate initsingleton that loads the real on-disk config on first touch, so on a developer machine with a populated config, unrelated startup work can hold the route gate and makeperformRerouteassertions vacuous. Tests that drive it now skip loudly rather than report a false pass or failure. CI has no user config, so they execute normally there. Making the suite hermetic is tracked as separate work.Deliberately not in this PR
The routing-socket (
PF_ROUTE) rewrite, kernel-truth diffing and contention backoff. Research showed diff-before-write without backoff would make this bug worse — we are currently blind to another client clobbering our routes, so diffing would start that fight at socket speed — and a hand-packedrt_msghdrwriter inside the root daemon is a new brick class. Those land separately, measured against this baseline.Helper upgrade
Helper version 1.8.0 → 1.9.0 so installed helpers actually pick up the fix. Expect one admin prompt on first launch after upgrading.
Summary by CodeRabbit
Release Notes v3.1.7
New Features
Bug Fixes
Tests