Skip to content

Commit 6e494bb

Browse files
authored
refactor(service): decompose startVpn into ordered stage functions (#47)
Plan 018 (plans/018-service-startvpn-decompose). Extract the 314-line startVpn monolith in MasterDnsVpnService.kt into five private stage functions called by a short orchestrator: - loadProfileAndSettings(profileId): ConnectInputs (stage: profile + port + DNS flag) - prepareConfigFiles(inputs): ConfigPaths (stage: MTU export + DNS port fallback + config + resolvers + log-file + logTailJob launch) - launchGoCoreAndWait(configPaths) (stage: goClientJob launch + waitForSocksProxyReady) - establishVpnInterface(inputs) (stage: vpnDnsServers + VpnService.Builder + establish + startTunBridge/startTun) - registerNetworkCallback() (stage: ConnectivityManager.NetworkCallback register) Plus two private data classes (ConnectInputs, ConfigPaths) to thread derived values across stages. startVpn() itself is reduced to a 63-line orchestrator (52 lines excluding the catch block) that reads as a numbered recipe. stopVpn() is intentionally left intact (its decomposition is deferred per plan 018 maintenance notes; the closeStaleVpnInterface / plan 014 KDoc invariants must stay). Behavior is byte-identical to the pre-refactor shape: same VpnManager state transitions, same mobile.Mobile.startClient/startTunBridge/ startTun call sites, same ordering, same error handling. The user- visible log sequence is preserved exactly (32 VpnManager.appendLog calls + 2 Log.e calls before == 32 + 2 after; order unchanged). Data-dependency threads (per plan's STOP-condition analysis): - localDnsEnabled is computed in loadProfileAndSettings from the original profile's advancedJson and threaded via ConnectInputs; the MTU-export path only mutates MTU_SERVERS_FILE_NAME, never the DNS flag, so the value seen in establishVpnInterface matches the pre-refactor localDnsEnabled that was re-derived from runtimeProfile.advancedJson. (Verified by re-reading lines 199-238.) - mtuExportTargetUri / mtuConfigDir stay as service fields (already declared at lines 95-96); they are set in prepareConfigFiles and read by stopVpn's exportMtuResultsIfNeeded() at line 559. Unchanged cross-stage coupling. - protocolOverride ("SOCKS5") and listenIpOverride (always null) were inline locals in the original startVpn; moved into prepareConfigFiles where they are consumed by ConfigGenerator.generateConfig. Three documented deviations from plan 018's snippet code: 1. "SOCKS5 proxy is ready on 127.0.0.1:PORT" log in launchGoCoreAndWait uses activeLocalSocksPort instead of inputs.socksPort. loadProfileAndSettings sets activeLocalSocksPort = socksPort before returning, so the two are provably equal and the log string is identical at runtime. Done because launchGoCoreAndWait takes ConfigPaths (not ConnectInputs) per the plan's signature. 2. "Proxy mode active on port PORT" in the orchestrator's proxyMode branch uses inputs.socksPort. The plan's Step 6 snippet had a latent bug (socksPort referenced a bare local that no longer existed after stage 1 extraction); corrected to inputs.socksPort per reviewer override to the executor. 3. In prepareConfigFiles and launchGoCoreAndWait the bare launch() calls were changed to serviceScope.launch(). The original bare launch() resolved against the outer connectJob coroutine scope receiver (a structured-concurrency child of connectJob). Once extracted into free-standing suspend functions, that implicit receiver is gone and bare launch() does not compile. The serviceScope.launch form keeps goClientJob and logTailJob as children of serviceScope (siblings of connectJob) instead of children of connectJob. Functional impact is nil: stopVpn() explicitly cancels goClientJob and logTailJob (lines 536, 539), ensureGoCoreStopped() calls mobile.Mobile.stopClient() at the top of the next startVpn, and onDestroy() also calls stopClient() -- three independent cancellation surfaces retain the shutdown contract. Flagging here so a future structured-concurrency audit can re-evaluate if a tighter scope is required. Comments were removed per AGENTS.md no-comments rule. One loss worth noting: the "ponytail: IPv6 NOT routed into the TUN..." design rationale comment in establishVpnInterface is gone. The non-routing behavior is preserved (no addRoute for ::/0) and the rationale lives in git history; a future ponytail-audit may want to re-mark it. Verification: - gradle / assembleDebug / compileDebugKotlin: SKIPPED per user constraint (no local Android build env; CI on push is the gate). Plan's done criteria adapted to grep + brace/paren balance checks. - grep for each of the 5 new private fun signatures -> 1 each - startVpn body: 63 lines (incl outer braces + catch block); 52 lines excluding the 9-line catch block (under plan's 60-line limit). - Brace balance: 279/279 across whole file. Paren balance: 656/656. - Scope: only one file modified (MasterDnsVpnService.kt, +290/-281). - log-sequence equivalence: 32 VpnManager.appendLog + 2 Log.e before == 32 + 2 after; top-to-bottom order preserved. - Zero bare launch() calls remain in extracted functions. Executor dispatched via /improve execute 018 bailed before producing commits; reviewer applied the refactor directly in the isolated worktree as permitted by closing-the-loop.md (worktree is disposable; user's main checkout untouched).
1 parent b8c44d0 commit 6e494bb

1 file changed

Lines changed: 290 additions & 281 deletions

File tree

0 commit comments

Comments
 (0)