Skip to content

refactor(service): decompose startVpn into ordered stage functions - #47

Merged
Hidden-Node merged 1 commit into
mainfrom
advisor/018-service-startvpn-decompose
Jul 24, 2026
Merged

refactor(service): decompose startVpn into ordered stage functions#47
Hidden-Node merged 1 commit into
mainfrom
advisor/018-service-startvpn-decompose

Conversation

@Hidden-Node

Copy link
Copy Markdown
Owner

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).

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).
@Hidden-Node
Hidden-Node merged commit 6e494bb into main Jul 24, 2026
2 checks passed
@Hidden-Node
Hidden-Node deleted the advisor/018-service-startvpn-decompose branch July 24, 2026 20:22
zeretrelle pushed a commit to zeretrelle/MasterDnsVPN-AndroidClient that referenced this pull request Jul 26, 2026
…-Node#47)

Lifts the regex-based scan-line scraping out of the private stateful
`VpnManager.parseScanLine` into an `internal` pure
`(prev: ScanStateBundle, line: String) -> ScanStateBundle` function in
a new `ScanStateReducer.kt`. The singleton's public API is unchanged;
`parseScanLine` becomes a 10-line wrapper that threads its three
StateFlows (`_scanStatus`, `_activeResolvers`, `_connectionWarning`)
through the reducer and emits new values only when they change.

## Why

`parseScanLine` ran up to 10 regex matches per Go-core log line and
mutated 9 `MutableStateFlow` fields. It was private and stateful, so
the regex contract could not be unit-tested without driving the
singleton through `appendLog` and observing StateFlows via Turbine.
This made every change to the log-scraping logic risky and
uncharacterizable. Extracting a pure reducer creates a trivially
table-testable seam (the cheapest seam for the future god-object
decompose of `VpnManager`, finding 9) at zero behavior change.

## What changed

### `android/app/src/main/java/com/masterdns/vpn/util/ScanStateReducer.kt` (new, +155)

- `internal data class ScanStateBundle` -- immutable input/output bundle
  holding `scanStatus: VpnManager.ScanStatus`, `activeResolvers: List<String>`,
  `connectionWarning: String?`.
- `internal object ScanStateReducer` -- owns the 10 regex constants moved
  verbatim from `VpnManager.kt` (patterns byte-identical, same
  `RegexOption.IGNORE_CASE` / `DOT_MATCHES_ALL` where applicable):
  `INDEXED_PROGRESS`, `TOTAL_CANDIDATES`, `SCAN_TOTALS`, `ACTIVE_RESOLVERS`,
  `TOTAL_ACTIVE`, `REMAINING`, `SYNCED_MTU`, `RESOLVER_ADDED`,
  `RESOLVER_REMOVED`, `SESSION_INIT_BACKOFF`.
- `internal fun reduce(prev, line)` -- pre-computes the 6 early-return
  matches (`scanMatch`, `activeMatch`, `totalActiveMatch`, `remainingMatch`,
  `syncedMtuMatch`, `testingMtu`) before a `when` cascade, then a single
  `if (anyMatched) return` short-circuits the trailing `MTU Testing
  Completed` / `Session Initialized Successfully` triggers and
  `SESSION_INIT_BACKOFF` blocks. The 4 non-returning pre-blocks
  (`RESOLVER_ADDED`, `RESOLVER_REMOVED`, `INDEXED_PROGRESS`,
  `TOTAL_CANDIDATES`) run before the cascade, unchanged.

### `android/app/src/main/java/com/masterdns/vpn/util/VpnManager.kt` (+14/-131)

- `parseScanLine` body (was 93 lines, ended at line 451) replaced with
  a 10-line wrapper: snapshot the 3 StateFlows into a `ScanStateBundle`,
  call `ScanStateReducer.reduce(prev, line)`, emit each changed field
  back via `if (next.X != prev.X) _X.value = next.X` guards.
- The 10 `private val *_REGEX` constants (previously at lines 101-140)
  removed from `VpnManager` -- they now live in `ScanStateReducer`.
- `TIMESTAMP_CANDIDATES` and `TimestampCandidate` (timestamp
  normalization, not scan-state) stay in `VpnManager`, untouched.
- The remaining `_scanStatus.value = _scanStatus.value.copy(...)` site
  inside `updateState` (the `VpnState.CONNECTED` reset, line 147) is
  unrelated to scan-line parsing and is intentionally preserved.

### `android/app/src/test/java/com/masterdns/vpn/util/ScanStateReducerTest.kt` (new, +120)

- 14 plain JUnit4 `@Test` methods (no new test dependencies -- reuses the
  existing `testImplementation("junit:junit:4.13.2")` and
  `kotlinx-coroutines-test:1.9.0` orchestrated by plan 005). Pattern
  follows the existing `GlobalSettingsPortRangeTest.kt` skeleton
  (same package `com.masterdns.vpn.util`, plain `assertEquals`/`assertTrue`).
- Cases cover: empty/non-matching lines, the `INDEXED_PROGRESS` happy
  path and non-numeric-total skip, `SCAN_TOTALS` Accepted/Rejected,
  `RESOLVER_ADDED` idempotence, `RESOLVER_REMOVED` removal,
  `"Testing MTU sizes"` / `"MTU Testing Completed"` /
  `"Session Initialized Successfully"` triggers, `SESSION_INIT_BACKOFF`,
  the precedence invariant (case 13: a line matching both
  `SCAN_TOTALS` and `ACTIVE_RESOLVERS` fires only the first), and
  `SYNCED_MTU`.

## Behavior preservation

Public API of `VpnManager` (`state`, `scanStatus`, `activeResolvers`,
`connect`, `disconnect`, `appendLog`, `appendCoreLog`, etc.) is
unchanged -- `MasterDnsVpnService.kt`, `VpnTileService.kt`,
`ResolversScreen.kt`, and `HomeStatusCards.kt` continue to consume
the same StateFlows with no edits. The `if (next != prev)` emission
guards are an additive optimization: observers receive exactly the
same sequence of distinct StateFlow values as before; the no-match
case no longer fires a redundant emission.

## Scope

3 files in `android/app/src/main/java/com/masterdns/vpn/util/` and
`android/app/src/test/java/com/masterdns/vpn/util/`. No out-of-scope
edits; no `mobile/`, no `cmd/`/`internal/`, no `go.mod`/`go.sum`, no
other UI files. The `plans/` folder (locally gitignored via
`.git/info/exclude`) is not committed and stays on the local
checkout for reference.

## Verification

Gradle `compileDebugKotlin` + `testDebugUnitTest` were not run locally
per the user's no-local-build constraint; this PR is the gate --
`android-ci.yml` runs `assembleDebug` which compiles and runs the
unit-test suite on push. The 14 new tests are JVM-runnable (no
Robolectric, no device) and target the pure reducer directly.
Plan 005's `ResolverAnalyzerTest` and the existing
`GlobalSettingsPortRangeTest` prove the testDebugUnitTest task is
wired and runs in CI.

## Maintenance notes

- **Adding a new scan-state field**: extend `ScanStatus` and
  `ScanStateBundle`, then add a new `?.let { match -> ... }` block in
  `ScanStateReducer.reduce`. Do NOT re-mutate `_scanStatus` from
  `parseScanLine` directly -- that is the anti-pattern this refactor
  removed.
- **Performance**: the `if (next != prev)` guards are pure
  optimizations; if a StateFlow-observer race appears under heavy
  load, removing the guards is safe (emitting an identical value is
  legal and cheap).
- **`ScanStateReducer` is the first Hilt-injectable seam for
  `VpnManager`**: a follow-up can make it a `@Singleton` injected
  interface so an `androidTest` can swap a fake reducer that asserts
  on the lines it received.

Squash-merges planner commits 985dfb0 (extraction) and 8b02a0c
(precedence-cascade bugfix discovered in self-review).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant