fix(vpn): close stale vpnInterface on reconnect and throw on go-core … - #41
Merged
Merged
Conversation
…stop timeout Reconnecting while the prior Go core hadn't fully stopped leaked the old ParcelFileDescriptor: ensureGoCoreStopped() only waited 4s, logged a warning, and continued, so the next builder.establish() overwrote vpnInterface at line 360 without closing the previous one. After enough reconnects the process hit its fd soft limit and establish() threw. Two-part fix: 1. Add closeStaleVpnInterface() and call it at the top of startVpn() AFTER ensureGoCoreStopped(). The ordering preserves the existing invariant (see MasterDnsVpnService.kt:491 comment): the fd is closed only once the Go core has stopped, so tun2socks goroutines mid-read on the fd don't hit EBADF. Also clears tunBridgeActive so a later stopClient() doesn't assume a live bridge on the stale fd. 2. Make ensureGoCoreStopped() throw IllegalStateException on timeout instead of logging-and-continuing. The throw propagates to the catch-all in startVpn(), which calls setError(message) and runs the proven stopVpn() cleanup path (Mobile.stopClient is Go-side idempotent; stopVpn itself is wrapped in try/catch). The user now sees a "Go core did not stop cleanly within 4 seconds" error instead of a silent CONNECTING-forever or cascading fd leaks. No recursion risk: stopVpn() does not call startVpn() or ensureGoCoreStopped() (grep-confirmed single call site at line 150). Scope: MasterDnsVpnService.kt only; no Go bridge, go.mod, or go.sum changes. Composes with plan 014 (isStopping lifecycle) cleanly. Verification: gradle build skipped per no-local-build constraint; CI on push is the gate. Grep-based done criteria pass: - closeStaleVpnInterface() x2 matches (def + call site) - throw "Go core did not stop cleanly within 4 seconds" present - "Warning: Go core may still be running" removed (0 matches)
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
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.
…stop timeout
Reconnecting while the prior Go core hadn't fully stopped leaked the old ParcelFileDescriptor: ensureGoCoreStopped() only waited 4s, logged a warning, and continued, so the next builder.establish() overwrote vpnInterface at line 360 without closing the previous one. After enough reconnects the process hit its fd soft limit and establish() threw.
Two-part fix:
Add closeStaleVpnInterface() and call it at the top of startVpn() AFTER ensureGoCoreStopped(). The ordering preserves the existing invariant (see MasterDnsVpnService.kt:491 comment): the fd is closed only once the Go core has stopped, so tun2socks goroutines mid-read on the fd don't hit EBADF. Also clears tunBridgeActive so a later stopClient() doesn't assume a live bridge on the stale fd.
Make ensureGoCoreStopped() throw IllegalStateException on timeout instead of logging-and-continuing. The throw propagates to the catch-all in startVpn(), which calls setError(message) and runs the proven stopVpn() cleanup path (Mobile.stopClient is Go-side idempotent; stopVpn itself is wrapped in try/catch). The user now sees a "Go core did not stop cleanly within 4 seconds" error instead of a silent CONNECTING-forever or cascading fd leaks.
No recursion risk: stopVpn() does not call startVpn() or ensureGoCoreStopped() (grep-confirmed single call site at line 150).
Scope: MasterDnsVpnService.kt only; no Go bridge, go.mod, or go.sum changes. Composes with plan 014 (isStopping lifecycle) cleanly.
Verification: gradle build skipped per no-local-build constraint; CI on push is the gate. Grep-based done criteria pass: