Stabilize root proxy routing state - #215
Open
grosserknallkopf wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughIntroduces a centralized ChangesProtection orchestration refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR stabilizes BlockAds’ protection lifecycle by centralizing “desired vs actual” routing/service state and routing all start/stop/reconcile operations through a single controller, reducing cases where UI reports protection while the wrong (or unhealthy) backend is active.
Changes:
- Introduces a unified protection health/state model (
ProtectionHealth,ActualProtectionState/Mode) and wires Home UI warnings/status to actual service + health signals. - Consolidates all entry points (boot, workers, widget, tile, settings) to start/stop/reconcile through
ServiceController, enforcing mutual exclusivity between Root Proxy and Local VPN. - Hardens Root Proxy runtime behavior: real readiness probing, health checks, partial IPv6 reporting, dynamic reloads, and Private DNS preservation/restore.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/res/values/strings.xml | Adds new UI strings for protection issues, partial IPv6 warning, and HTTPS filtering constraints. |
| app/src/main/res/values-de/strings.xml | German translations for the newly introduced strings. |
| app/src/main/java/app/pwhs/blockads/worker/VpnResumeWorker.kt | Resumes protection via controller only if protection is still desired. |
| app/src/main/java/app/pwhs/blockads/worker/RootProxyResumeWorker.kt | Resumes protection via controller only if protection is still desired. |
| app/src/main/java/app/pwhs/blockads/widget/WidgetToggleReceiver.kt | Routes widget toggles through ServiceController instead of direct service intents. |
| app/src/main/java/app/pwhs/blockads/ui/settings/SettingsViewModel.kt | Reconciles routing mode changes via controller and disables HTTPS filtering in Root mode. |
| app/src/main/java/app/pwhs/blockads/ui/onboarding/OnboardingViewModel.kt | Persists onboarding selection by activating the matching preset profile. |
| app/src/main/java/app/pwhs/blockads/ui/httpsfiltering/HttpsFilteringViewModel.kt | Prevents enabling HTTPS filtering in Root mode; exposes routing mode as StateFlow. |
| app/src/main/java/app/pwhs/blockads/ui/httpsfiltering/HttpsFilteringScreen.kt | Shows Root-mode warning UI and snackbar for unsupported HTTPS filtering. |
| app/src/main/java/app/pwhs/blockads/ui/home/HomeViewModel.kt | Drives UI from centralized ProtectionHealth rather than preferences-only assumptions. |
| app/src/main/java/app/pwhs/blockads/ui/home/HomeScreen.kt | Surfaces protection problems and partial IPv6 warnings; adds clickable module link. |
| app/src/main/java/app/pwhs/blockads/ui/filter/detail/FilterDetailScreen.kt | Localizes the “test domain”/lookup UI strings. |
| app/src/main/java/app/pwhs/blockads/service/TrustedNetworkManager.kt | Uses pause semantics (requestPause) instead of fully disabling protection. |
| app/src/main/java/app/pwhs/blockads/service/ServiceController.kt | Implements unified start/stop/restart/reconcile logic and protection health aggregation. |
| app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt | Adds runtime reloads, detailed health reporting, readiness/health checks, and improved teardown behavior. |
| app/src/main/java/app/pwhs/blockads/service/ProtectionState.kt | New centralized protection state/health domain model. |
| app/src/main/java/app/pwhs/blockads/service/IptablesManager.kt | Adds detailed rule setup status, IPv6 status reporting, and Private DNS snapshot/restore. |
| app/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.kt | Replaces fake readiness delay with UDP DNS probe; adds health/last-activity tracking. |
| app/src/main/java/app/pwhs/blockads/service/BootReceiver.kt | Auto-starts protection via controller using “protectionDesired” intent state. |
| app/src/main/java/app/pwhs/blockads/service/AdBlockVpnService.kt | Aligns lifecycle with “protectionDesired”, mutual exclusivity, and controller-based reconciliation. |
| app/src/main/java/app/pwhs/blockads/service/AdBlockTileService.kt | Uses controller for tile start/stop actions. |
| app/src/main/java/app/pwhs/blockads/MainActivity.kt | Uses controller for start/stop toggles to avoid resurrecting stale services. |
| app/src/main/java/app/pwhs/blockads/di/AppModule.kt | Injects ProfileManager into onboarding view model. |
| app/src/main/java/app/pwhs/blockads/data/entities/ProfileManager.kt | Adds onboarding-level profile switching and “reapply active profile” after filter syncs. |
| app/src/main/java/app/pwhs/blockads/data/datastore/AppPreferences.kt | Introduces protectionDesired intent concept mapped onto legacy vpn_enabled storage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+223
to
+227
| private fun isRootRoutingAllowed(): Boolean { | ||
| return runBlocking { | ||
| appPrefs.getRoutingModeSnapshot() == AppPreferences.ROUTING_MODE_ROOT | ||
| } | ||
| } |
Comment on lines
+389
to
+394
| if (markProtectionDisabled) { | ||
| ServiceController.cancelResumeWork(this) | ||
| runBlocking { | ||
| appPrefs.setProtectionDesired(false) | ||
| } | ||
| } |
Comment on lines
+333
to
+337
| private fun isVpnRoutingAllowed(): Boolean { | ||
| return runBlocking { | ||
| appPrefs.getRoutingModeSnapshot() != AppPreferences.ROUTING_MODE_ROOT | ||
| } | ||
| } |
| <string name="protection_problem_root_routing">Root Proxy is running, but DNS redirection rules are not active.</string> | ||
| <string name="protection_partial_title">Partial protection</string> | ||
| <string name="protection_partial_ipv6">IPv4 DNS is protected, but IPv6 DNS interception is not active on this device. Some requests may bypass BlockAds. Disable IPv6 on your router, use Local VPN mode, or, for advanced Magisk users, consider a third-party module such as magisk-disable-ipv6.</string> | ||
| <string name="protection_partial_ipv6_magisk_link">magisk-disable-ipv6</string> |
| <string name="protection_problem_root_routing">Root Proxy läuft, aber die DNS-Umleitungsregeln sind nicht aktiv.</string> | ||
| <string name="protection_partial_title">Teilweiser Schutz</string> | ||
| <string name="protection_partial_ipv6">IPv4-DNS ist geschützt, aber IPv6-DNS-Umleitung ist auf diesem Gerät nicht aktiv. Einige Anfragen können BlockAds umgehen. Deaktiviere IPv6 am Router, nutze den lokalen VPN-Modus oder prüfe als erfahrener Magisk-Nutzer ein Drittanbieter-Modul wie magisk-disable-ipv6.</string> | ||
| <string name="protection_partial_ipv6_magisk_link">magisk-disable-ipv6</string> |
Comment on lines
+343
to
+352
| text = when (actualProtectionMode) { | ||
| ActualProtectionMode.ROOT -> "Root Proxy Mode" | ||
| ActualProtectionMode.WIREGUARD -> "WireGuard Mode" | ||
| ActualProtectionMode.VPN -> "Local VPN Mode" | ||
| ActualProtectionMode.CONFLICT -> stringResource(R.string.protection_mode_conflict) | ||
| ActualProtectionMode.NONE -> when (routingMode) { | ||
| AppPreferences.ROUTING_MODE_ROOT -> "Root Proxy Mode" | ||
| AppPreferences.ROUTING_MODE_WIREGUARD -> "WireGuard Mode" | ||
| else -> "Local VPN Mode" | ||
| } |
Comment on lines
+803
to
+812
| val body = stringResource(R.string.protection_partial_ipv6) | ||
| val linkText = stringResource(R.string.protection_partial_ipv6_magisk_link) | ||
| val linkColor = MaterialTheme.colorScheme.primary | ||
| val annotatedText = remember(body, linkText, linkColor) { | ||
| buildAnnotatedString { | ||
| val linkStart = body.indexOf(linkText) | ||
| if (linkStart < 0) { | ||
| append(body) | ||
| return@buildAnnotatedString | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt (1)
377-412: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMove
stopProxyteardown off the main thread.onStartCommand(...ACTION_STOP)andpauseProxy()call this synchronously, and it still doesrunBlocking,IptablesManager.teardownRules()(Shell.exec()), andgoTunnelAdapter.stop()inline. Offload the shutdown toserviceScope.launch(Dispatchers.IO)like the VPN service does.🤖 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 `@app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt` around lines 377 - 412, The stopProxy teardown is still running synchronously on the caller thread, including runBlocking, IptablesManager.teardownRules, and goTunnelAdapter.stop, which can block the main thread when ACTION_STOP or pauseProxy invokes it. Update stopProxy in RootProxyService to perform the shutdown work inside serviceScope.launch(Dispatchers.IO), matching the VPN service pattern, while keeping the state updates and notification cleanup in the same stop flow.app/src/main/java/app/pwhs/blockads/widget/WidgetToggleReceiver.kt (1)
64-72: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStart-failure fallback is unreachable.
ServiceController.requestStart(context)now justscope.launch { ... }, so thistry/catchcan’t catch the actual foreground-service start failure that used to openMainActivity. When start is denied, the widget will fail silently; move the fallback into the controller or haverequestStartreport the outcome back to the receiver.🤖 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 `@app/src/main/java/app/pwhs/blockads/widget/WidgetToggleReceiver.kt` around lines 64 - 72, The fallback in WidgetToggleReceiver is unreachable because ServiceController.requestStart(context) only launches work asynchronously, so the try/catch cannot observe the actual start failure. Move the failure handling into ServiceController.requestStart or change it to return/report success so WidgetToggleReceiver can open MainActivity when VPN start is denied. Use the requestStart method and the widget receiver flow to ensure the fallback is triggered on the real failure path, not around the coroutine launch.
🧹 Nitpick comments (5)
app/src/main/java/app/pwhs/blockads/service/ProtectionState.kt (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel layer references controller constants — inverts dependency direction.
ActualProtectionState.Failedstores a rawStringreason, andProtectionHealth.problem(Line 54-56) pattern-matches againstServiceController.FAIL_REASON_ROOT_ENGINE/FAIL_REASON_ROOT_ROUTING. This makes the state/model file (ProtectionState.kt) depend on the orchestration file (ServiceController.kt), which is backwards — models should be consumed by controllers, not the other way around. It also means any typo or renamed constant inServiceControllersilently falls into theelse -> ROOT_ENGINE_UNHEALTHYdefault, masking a possible new failure category.Consider moving the failure-reason constants into
ProtectionState.kt(or replacing theStringwith a small enum/sealed type owned by this model file) soServiceControllerdepends on the model, not vice versa.♻️ Suggested direction
-sealed interface ActualProtectionState { - ... - data class Failed(val reason: String) : ActualProtectionState -} +enum class ProtectionFailureReason { ROOT_ENGINE_UNHEALTHY, ROOT_ROUTING_INACTIVE } + +sealed interface ActualProtectionState { + ... + data class Failed(val reason: ProtectionFailureReason) : ActualProtectionState +}Also applies to: 49-59
🤖 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 `@app/src/main/java/app/pwhs/blockads/service/ProtectionState.kt` at line 22, The failure reason handling in ProtectionState/ProtectionHealth is backwards because the model is matching on ServiceController constants. Move the failure-reason definitions into ProtectionState.kt (or replace the raw String in ActualProtectionState.Failed with a model-owned enum/sealed type), then update ProtectionHealth.problem to switch on that model-owned type instead of referencing ServiceController.FAIL_REASON_ROOT_ENGINE and FAIL_REASON_ROOT_ROUTING. Keep ServiceController consuming the model rather than defining the canonical failure reasons.app/src/main/java/app/pwhs/blockads/MainActivity.kt (1)
162-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth
routingModebranches are now identical.After delegating to
ServiceController.requestStop, the root and non-root branches perform exactly the same logic (if (RootProxyService.isRunning || AdBlockVpnService.isRunning) requestStop else handleVpnToggle). The outerroutingModecheck can be collapsed.♻️ Proposed simplification
- if (routingMode == AppPreferences.ROUTING_MODE_ROOT) { - if (RootProxyService.isRunning || AdBlockVpnService.isRunning) { - ServiceController.requestStop(this) - } else { - handleVpnToggle() - } - } else { - if (AdBlockVpnService.isRunning || RootProxyService.isRunning) { - ServiceController.requestStop(this) - } else { - handleVpnToggle() - } - } + if (RootProxyService.isRunning || AdBlockVpnService.isRunning) { + ServiceController.requestStop(this) + } else { + handleVpnToggle() + }🤖 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 `@app/src/main/java/app/pwhs/blockads/MainActivity.kt` around lines 162 - 174, The routingMode branch in MainActivity is duplicated and can be simplified because both paths in the toggle logic do the same thing. Collapse the outer if/else around the service check into a single flow in MainActivity.handleVpnToggle-related logic, keeping the existing ServiceController.requestStop and AdBlockVpnService/RootProxyService running check behavior unchanged.app/src/main/java/app/pwhs/blockads/service/AdBlockTileService.kt (1)
36-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant stop branches.
isRootProxyRunningandisVpnRunningbranches now both callServiceController.requestStop(this); they can be merged.♻️ Proposed simplification
- if (isRootProxyRunning) { - ServiceController.requestStop(this) - } else if (isVpnRunning) { - ServiceController.requestStop(this) - } else { + if (isRootProxyRunning || isVpnRunning) { + ServiceController.requestStop(this) + } else {🤖 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 `@app/src/main/java/app/pwhs/blockads/service/AdBlockTileService.kt` around lines 36 - 40, The stop logic in AdBlockTileService is redundant because both the isRootProxyRunning and isVpnRunning branches call ServiceController.requestStop(this) with the same behavior. Simplify the conditional by merging these checks into a single branch in the service’s stop handling, keeping the same stop call and leaving the fallback branch unchanged.app/src/main/java/app/pwhs/blockads/ui/home/HomeScreen.kt (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ClickableTextis deprecated in Compose Foundation.Per current AndroidX documentation,
ClickableText"has been deprecated" in favor ofText/BasicTextwith anAnnotatedStringcontainingLinkAnnotation. Since this file diff pulls inandroidx.compose.ui:ui/androidx.compose.material3:material3at "latest", the newPartialIpv6WarningTextcomposable is built on a deprecated API on day one. Not blocking, but worth migrating towithLink(LinkAnnotation.Url(...))to avoid a future forced migration.Also applies to: 798-843
🤖 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 `@app/src/main/java/app/pwhs/blockads/ui/home/HomeScreen.kt` at line 6, `ClickableText` in `HomeScreen.kt` is using a deprecated Compose Foundation API, so update `PartialIpv6WarningText` to use `Text` or `BasicText` with an `AnnotatedString` and `LinkAnnotation` instead. Remove the `ClickableText` import, build the clickable link content with `withLink(LinkAnnotation.Url(...))`, and keep the existing click behavior and styling in the new text implementation.app/src/main/java/app/pwhs/blockads/ui/home/HomeViewModel.kt (1)
81-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
vpnStoppingbypasses the centralizedprotectionHealthmodel.Unlike
vpnEnabled/vpnConnecting,vpnStoppingstill directly combinesAdBlockVpnService.state/RootProxyService.stateand inline-qualifiesapp.pwhs.blockads.service.VpnState.STOPPINGinstead of importingVpnState. This appears to be becauseActualProtectionState(inProtectionState.kt) has no "stopping" variant, so the central model can't represent it — but it means Home's stop-in-progress UI can still diverge from the reconciled state model this PR is trying to centralize. Consider importingVpnStatefor readability, and consider whether the protection-state model should eventually be extended with aStoppingstate so all transient states are attributable to one source of truth.- state1 == app.pwhs.blockads.service.VpnState.STOPPING || state2 == app.pwhs.blockads.service.VpnState.STOPPING + state1 == VpnState.STOPPING || state2 == VpnState.STOPPING🤖 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 `@app/src/main/java/app/pwhs/blockads/ui/home/HomeViewModel.kt` around lines 81 - 86, The vpnStopping flow in HomeViewModel still reads the raw service states directly and inline-qualifies VpnState.STOPPING, which keeps it outside the centralized protectionHealth model. Update HomeViewModel to import VpnState for clarity, and review whether the stop-in-progress state should be represented through the shared protection-state model in ProtectionState.kt/ActualProtectionState so vpnEnabled, vpnConnecting, and vpnStopping all come from the same source of truth.
🤖 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 `@app/src/main/java/app/pwhs/blockads/service/IptablesManager.kt`:
- Around line 302-308: The Private DNS snapshot is only stored in-memory via
previousPrivateDns inside savePrivateDnsIfNeeded(), so a process restart can
overwrite the user’s original setting after setupRulesDetailed() forces
private_dns_mode to off. Update savePrivateDnsIfNeeded(), restorePrivateDns(),
and the PrivateDnsSnapshot flow so the original mode/specifier survives
restarts, either by persisting the snapshot across process death or by
preventing re-snapshotting once the app has already forced Private DNS off.
In `@app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt`:
- Around line 210-227: The early return in RootProxyService’s service-start
branch stops the service before it is promoted to foreground, which can trigger
the foreground-service timeout. In the else branch that checks
isRootRoutingAllowed(), make sure the service is already started in the
foreground before calling stopSelf(), or restructure the start path so
startForegroundService() is only used when root routing is allowed; keep the fix
localized around RootProxyService.startProxy/startForeground handling and the
non-root routing guard.
In `@app/src/main/java/app/pwhs/blockads/service/ServiceController.kt`:
- Around line 201-252: The reconcileToMode transition logic is racing because it
reads service state and then starts/stops services without any serialization, so
concurrent callers can interleave and violate mutual exclusivity. Add a single
shared guard in ServiceController (for example around reconcileToMode, and by
extension callers like requestStartNow/requestStopNow/requestRestartNow) so only
one reconciliation runs at a time, and keep the existing state checks and
start/stop decisions inside that critical section.
- Around line 139-199: The fire-and-forget launch paths in
ServiceController.requestStart, requestStop, requestPause, and requestRestart
can throw uncaught exceptions because the launched coroutines are not protected.
Add exception handling inside the scope.launch blocks or wrap the suspend calls
in try/catch within requestStartNow, requestStopNow, and requestRestartNow so
failures are logged and do not escape asynchronously; make sure the fix covers
the async work invoked from WidgetToggleReceiver.toggleVpn as well.
- Around line 220-252: The VPN-to-root switch in ServiceController.switchService
currently uses a fixed delay after calling AdBlockVpnService.stop(), which can
let RootProxyService.start() run while AdBlockVpnService is still STOPPING.
Update the root-transition branch to wait for AdBlockVpnService to reach STOPPED
before starting or reloading the replacement service, matching the existing
root-to-VPN stop behavior; use the service state checks around
AdBlockVpnService.stop(), RootProxyService.start(), and
RootProxyService.requestReload() to place the wait in the right spot.
---
Outside diff comments:
In `@app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt`:
- Around line 377-412: The stopProxy teardown is still running synchronously on
the caller thread, including runBlocking, IptablesManager.teardownRules, and
goTunnelAdapter.stop, which can block the main thread when ACTION_STOP or
pauseProxy invokes it. Update stopProxy in RootProxyService to perform the
shutdown work inside serviceScope.launch(Dispatchers.IO), matching the VPN
service pattern, while keeping the state updates and notification cleanup in the
same stop flow.
In `@app/src/main/java/app/pwhs/blockads/widget/WidgetToggleReceiver.kt`:
- Around line 64-72: The fallback in WidgetToggleReceiver is unreachable because
ServiceController.requestStart(context) only launches work asynchronously, so
the try/catch cannot observe the actual start failure. Move the failure handling
into ServiceController.requestStart or change it to return/report success so
WidgetToggleReceiver can open MainActivity when VPN start is denied. Use the
requestStart method and the widget receiver flow to ensure the fallback is
triggered on the real failure path, not around the coroutine launch.
---
Nitpick comments:
In `@app/src/main/java/app/pwhs/blockads/MainActivity.kt`:
- Around line 162-174: The routingMode branch in MainActivity is duplicated and
can be simplified because both paths in the toggle logic do the same thing.
Collapse the outer if/else around the service check into a single flow in
MainActivity.handleVpnToggle-related logic, keeping the existing
ServiceController.requestStop and AdBlockVpnService/RootProxyService running
check behavior unchanged.
In `@app/src/main/java/app/pwhs/blockads/service/AdBlockTileService.kt`:
- Around line 36-40: The stop logic in AdBlockTileService is redundant because
both the isRootProxyRunning and isVpnRunning branches call
ServiceController.requestStop(this) with the same behavior. Simplify the
conditional by merging these checks into a single branch in the service’s stop
handling, keeping the same stop call and leaving the fallback branch unchanged.
In `@app/src/main/java/app/pwhs/blockads/service/ProtectionState.kt`:
- Line 22: The failure reason handling in ProtectionState/ProtectionHealth is
backwards because the model is matching on ServiceController constants. Move the
failure-reason definitions into ProtectionState.kt (or replace the raw String in
ActualProtectionState.Failed with a model-owned enum/sealed type), then update
ProtectionHealth.problem to switch on that model-owned type instead of
referencing ServiceController.FAIL_REASON_ROOT_ENGINE and
FAIL_REASON_ROOT_ROUTING. Keep ServiceController consuming the model rather than
defining the canonical failure reasons.
In `@app/src/main/java/app/pwhs/blockads/ui/home/HomeScreen.kt`:
- Line 6: `ClickableText` in `HomeScreen.kt` is using a deprecated Compose
Foundation API, so update `PartialIpv6WarningText` to use `Text` or `BasicText`
with an `AnnotatedString` and `LinkAnnotation` instead. Remove the
`ClickableText` import, build the clickable link content with
`withLink(LinkAnnotation.Url(...))`, and keep the existing click behavior and
styling in the new text implementation.
In `@app/src/main/java/app/pwhs/blockads/ui/home/HomeViewModel.kt`:
- Around line 81-86: The vpnStopping flow in HomeViewModel still reads the raw
service states directly and inline-qualifies VpnState.STOPPING, which keeps it
outside the centralized protectionHealth model. Update HomeViewModel to import
VpnState for clarity, and review whether the stop-in-progress state should be
represented through the shared protection-state model in
ProtectionState.kt/ActualProtectionState so vpnEnabled, vpnConnecting, and
vpnStopping all come from the same source of truth.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6eb20c5a-a19d-4ae4-b7e7-22f93a175a69
📒 Files selected for processing (25)
app/src/main/java/app/pwhs/blockads/MainActivity.ktapp/src/main/java/app/pwhs/blockads/data/datastore/AppPreferences.ktapp/src/main/java/app/pwhs/blockads/data/entities/ProfileManager.ktapp/src/main/java/app/pwhs/blockads/di/AppModule.ktapp/src/main/java/app/pwhs/blockads/service/AdBlockTileService.ktapp/src/main/java/app/pwhs/blockads/service/AdBlockVpnService.ktapp/src/main/java/app/pwhs/blockads/service/BootReceiver.ktapp/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.ktapp/src/main/java/app/pwhs/blockads/service/IptablesManager.ktapp/src/main/java/app/pwhs/blockads/service/ProtectionState.ktapp/src/main/java/app/pwhs/blockads/service/RootProxyService.ktapp/src/main/java/app/pwhs/blockads/service/ServiceController.ktapp/src/main/java/app/pwhs/blockads/service/TrustedNetworkManager.ktapp/src/main/java/app/pwhs/blockads/ui/filter/detail/FilterDetailScreen.ktapp/src/main/java/app/pwhs/blockads/ui/home/HomeScreen.ktapp/src/main/java/app/pwhs/blockads/ui/home/HomeViewModel.ktapp/src/main/java/app/pwhs/blockads/ui/httpsfiltering/HttpsFilteringScreen.ktapp/src/main/java/app/pwhs/blockads/ui/httpsfiltering/HttpsFilteringViewModel.ktapp/src/main/java/app/pwhs/blockads/ui/onboarding/OnboardingViewModel.ktapp/src/main/java/app/pwhs/blockads/ui/settings/SettingsViewModel.ktapp/src/main/java/app/pwhs/blockads/widget/WidgetToggleReceiver.ktapp/src/main/java/app/pwhs/blockads/worker/RootProxyResumeWorker.ktapp/src/main/java/app/pwhs/blockads/worker/VpnResumeWorker.ktapp/src/main/res/values-de/strings.xmlapp/src/main/res/values/strings.xml
Comment on lines
+139
to
+199
| fun requestStart(context: Context, startedFromBoot: Boolean = false) { | ||
| scope.launch { | ||
| requestStartNow(context.applicationContext, startedFromBoot) | ||
| } | ||
| } | ||
|
|
||
| suspend fun requestStartNow(context: Context, startedFromBoot: Boolean = false) { | ||
| val appContext = context.applicationContext | ||
| val appPrefs = AppPreferences(appContext) | ||
| appPrefs.setProtectionDesired(true) | ||
| appPrefs.setPausedByTrusted(false) | ||
| cancelResumeWork(appContext) | ||
| reconcileToMode(appContext, appPrefs.routingMode.first(), startIfDesired = true, startedFromBoot = startedFromBoot) | ||
| } | ||
|
|
||
| /** | ||
| * Stop whichever service is currently running. | ||
| */ | ||
| fun requestStop(context: Context) { | ||
| if (RootProxyService.isRunning) { | ||
| RootProxyService.stop(context) | ||
| scope.launch { | ||
| requestStopNow(context.applicationContext, markProtectionDisabled = true) | ||
| } | ||
| if (AdBlockVpnService.isRunning) { | ||
| AdBlockVpnService.stop(context) | ||
| } | ||
|
|
||
| fun requestPause(context: Context) { | ||
| scope.launch { | ||
| requestStopNow(context.applicationContext, markProtectionDisabled = false) | ||
| } | ||
| } | ||
|
|
||
| suspend fun requestStopNow(context: Context, markProtectionDisabled: Boolean) { | ||
| val appContext = context.applicationContext | ||
| val appPrefs = AppPreferences(appContext) | ||
| cancelResumeWork(appContext) | ||
| if (markProtectionDisabled) { | ||
| appPrefs.setProtectionDesired(false) | ||
| appPrefs.setPausedByTrusted(false) | ||
| } | ||
| stopServices(appContext, markProtectionDisabled = markProtectionDisabled) | ||
| } | ||
|
|
||
| fun requestRestart(context: Context) { | ||
| scope.launch { | ||
| requestRestartNow(context.applicationContext) | ||
| } | ||
| } | ||
|
|
||
| suspend fun requestRestartNow(context: Context) { | ||
| val appContext = context.applicationContext | ||
| val appPrefs = AppPreferences(appContext) | ||
| val desiredMode = appPrefs.routingMode.first() | ||
| val shouldBeRunning = appPrefs.protectionDesired.first() || | ||
| AdBlockVpnService.state.value.isActiveState() || | ||
| RootProxyService.state.value.isActiveState() | ||
|
|
||
| if (!shouldBeRunning) return | ||
|
|
||
| reconcileToMode( | ||
| context = appContext, | ||
| targetMode = desiredMode, | ||
| startIfDesired = true, | ||
| restartTarget = true, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ServiceController scope and methods ==\n'
grep -n "CoroutineScope\|SupervisorJob\|CoroutineExceptionHandler\|fun requestStart\|fun requestStop\|fun requestPause\|fun requestRestart\|suspend fun requestStartNow\|suspend fun requestStopNow\|suspend fun requestRestartNow" -n app/src/main/java/app/pwhs/blockads/service/ServiceController.kt
printf '\n== Relevant surrounding lines ==\n'
sed -n '1,240p' app/src/main/java/app/pwhs/blockads/service/ServiceController.kt
printf '\n== WidgetToggleReceiver references ==\n'
grep -R -n "toggleVpn\|requestStart(context)\|requestStop(context)\|requestPause(context)\|requestRestart(context)" app/src/main/java || trueRepository: pass-with-high-score/blockads-android
Length of output: 11585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the call site implementation.
file=$(grep -R -n "toggleVpn" app/src/main/java | head -n 1 | cut -d: -f1)
echo "FILE=$file"
sed -n '1,240p' "$file"Repository: pass-with-high-score/blockads-android
Length of output: 3195
Catch exceptions in the fire-and-forget jobs requestStart/requestStop/requestPause/requestRestart launch on scope without a CoroutineExceptionHandler, so failures in the suspend bodies can surface as unhandled coroutine exceptions and crash the app. WidgetToggleReceiver.toggleVpn’s try/catch only wraps the immediate call and cannot intercept those async failures.
🤖 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 `@app/src/main/java/app/pwhs/blockads/service/ServiceController.kt` around
lines 139 - 199, The fire-and-forget launch paths in
ServiceController.requestStart, requestStop, requestPause, and requestRestart
can throw uncaught exceptions because the launched coroutines are not protected.
Add exception handling inside the scope.launch blocks or wrap the suspend calls
in try/catch within requestStartNow, requestStopNow, and requestRestartNow so
failures are logged and do not escape asynchronously; make sure the fix covers
the async work invoked from WidgetToggleReceiver.toggleVpn as well.
Comment on lines
+201
to
+252
| suspend fun reconcileToMode( | ||
| context: Context, | ||
| targetMode: String, | ||
| startIfDesired: Boolean = true, | ||
| restartTarget: Boolean = false, | ||
| startedFromBoot: Boolean = false, | ||
| ) { | ||
| val appContext = context.applicationContext | ||
| val appPrefs = AppPreferences(appContext) | ||
| appPrefs.setRoutingMode(targetMode) | ||
| cancelResumeWork(appContext) | ||
|
|
||
| val protectionDesired = appPrefs.protectionDesired.first() | ||
| val shouldRun = startIfDesired && ( | ||
| protectionDesired || | ||
| AdBlockVpnService.state.value.isActiveState() || | ||
| RootProxyService.state.value.isActiveState() | ||
| ) | ||
|
|
||
| if (targetMode == AppPreferences.ROUTING_MODE_ROOT) { | ||
| if (AdBlockVpnService.state.value.isActiveState()) { | ||
| AdBlockVpnService.stop(appContext, markProtectionDisabled = false) | ||
| delay(SERVICE_SWITCH_DELAY_MS) | ||
| } | ||
|
|
||
| if (shouldRun) { | ||
| if (RootProxyService.state.value == VpnState.RUNNING && restartTarget) { | ||
| RootProxyService.requestReload(appContext) | ||
| } else if (!RootProxyService.state.value.isActiveState()) { | ||
| RootProxyService.start(appContext, startedFromBoot = startedFromBoot) | ||
| } | ||
| } else if (RootProxyService.state.value.isActiveState()) { | ||
| RootProxyService.stop(appContext, markProtectionDisabled = false) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if (RootProxyService.state.value.isActiveState()) { | ||
| RootProxyService.stop(appContext, markProtectionDisabled = false) | ||
| delay(SERVICE_SWITCH_DELAY_MS) | ||
| } | ||
|
|
||
| if (shouldRun) { | ||
| if (AdBlockVpnService.state.value == VpnState.RUNNING && restartTarget) { | ||
| AdBlockVpnService.requestRestart(appContext) | ||
| } else if (!AdBlockVpnService.state.value.isActiveState()) { | ||
| AdBlockVpnService.start(appContext, startedFromBoot = startedFromBoot) | ||
| } | ||
| } else if (AdBlockVpnService.state.value.isActiveState()) { | ||
| AdBlockVpnService.stop(appContext, markProtectionDisabled = false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
No synchronization guarding concurrent reconcileToMode invocations.
reconcileToMode reads AdBlockVpnService.state.value / RootProxyService.state.value, then acts on them (classic check-then-act). It's reachable concurrently from requestStartNow, requestStopNow (via stopServices), and requestRestartNow, all launched fire-and-forget on the shared scope. Rapid successive calls (e.g., a widget tap immediately followed by a boot-triggered resume, or a trusted-network pause racing a manual toggle) can interleave: both calls might observe "not active" and independently start a service, or one stop and one start could race past each other's delay(SERVICE_SWITCH_DELAY_MS) window. This directly undermines the PR's stated goal that Root Proxy and Local VPN "remain mutually exclusive."
Consider serializing all reconciliation through a Mutex so only one transition runs at a time:
🔒 Suggested fix
+ private val reconcileMutex = kotlinx.coroutines.sync.Mutex()
+
suspend fun reconcileToMode(
context: Context,
targetMode: String,
startIfDesired: Boolean = true,
restartTarget: Boolean = false,
startedFromBoot: Boolean = false,
- ) {
+ ) = reconcileMutex.withLock {
val appContext = context.applicationContext
...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| suspend fun reconcileToMode( | |
| context: Context, | |
| targetMode: String, | |
| startIfDesired: Boolean = true, | |
| restartTarget: Boolean = false, | |
| startedFromBoot: Boolean = false, | |
| ) { | |
| val appContext = context.applicationContext | |
| val appPrefs = AppPreferences(appContext) | |
| appPrefs.setRoutingMode(targetMode) | |
| cancelResumeWork(appContext) | |
| val protectionDesired = appPrefs.protectionDesired.first() | |
| val shouldRun = startIfDesired && ( | |
| protectionDesired || | |
| AdBlockVpnService.state.value.isActiveState() || | |
| RootProxyService.state.value.isActiveState() | |
| ) | |
| if (targetMode == AppPreferences.ROUTING_MODE_ROOT) { | |
| if (AdBlockVpnService.state.value.isActiveState()) { | |
| AdBlockVpnService.stop(appContext, markProtectionDisabled = false) | |
| delay(SERVICE_SWITCH_DELAY_MS) | |
| } | |
| if (shouldRun) { | |
| if (RootProxyService.state.value == VpnState.RUNNING && restartTarget) { | |
| RootProxyService.requestReload(appContext) | |
| } else if (!RootProxyService.state.value.isActiveState()) { | |
| RootProxyService.start(appContext, startedFromBoot = startedFromBoot) | |
| } | |
| } else if (RootProxyService.state.value.isActiveState()) { | |
| RootProxyService.stop(appContext, markProtectionDisabled = false) | |
| } | |
| return | |
| } | |
| if (RootProxyService.state.value.isActiveState()) { | |
| RootProxyService.stop(appContext, markProtectionDisabled = false) | |
| delay(SERVICE_SWITCH_DELAY_MS) | |
| } | |
| if (shouldRun) { | |
| if (AdBlockVpnService.state.value == VpnState.RUNNING && restartTarget) { | |
| AdBlockVpnService.requestRestart(appContext) | |
| } else if (!AdBlockVpnService.state.value.isActiveState()) { | |
| AdBlockVpnService.start(appContext, startedFromBoot = startedFromBoot) | |
| } | |
| } else if (AdBlockVpnService.state.value.isActiveState()) { | |
| AdBlockVpnService.stop(appContext, markProtectionDisabled = false) | |
| } | |
| } | |
| private val reconcileMutex = kotlinx.coroutines.sync.Mutex() | |
| suspend fun reconcileToMode( | |
| context: Context, | |
| targetMode: String, | |
| startIfDesired: Boolean = true, | |
| restartTarget: Boolean = false, | |
| startedFromBoot: Boolean = false, | |
| ) = reconcileMutex.withLock { | |
| val appContext = context.applicationContext | |
| val appPrefs = AppPreferences(appContext) | |
| appPrefs.setRoutingMode(targetMode) | |
| cancelResumeWork(appContext) | |
| val protectionDesired = appPrefs.protectionDesired.first() | |
| val shouldRun = startIfDesired && ( | |
| protectionDesired || | |
| AdBlockVpnService.state.value.isActiveState() || | |
| RootProxyService.state.value.isActiveState() | |
| ) | |
| if (targetMode == AppPreferences.ROUTING_MODE_ROOT) { | |
| if (AdBlockVpnService.state.value.isActiveState()) { | |
| AdBlockVpnService.stop(appContext, markProtectionDisabled = false) | |
| delay(SERVICE_SWITCH_DELAY_MS) | |
| } | |
| if (shouldRun) { | |
| if (RootProxyService.state.value == VpnState.RUNNING && restartTarget) { | |
| RootProxyService.requestReload(appContext) | |
| } else if (!RootProxyService.state.value.isActiveState()) { | |
| RootProxyService.start(appContext, startedFromBoot = startedFromBoot) | |
| } | |
| } else if (RootProxyService.state.value.isActiveState()) { | |
| RootProxyService.stop(appContext, markProtectionDisabled = false) | |
| } | |
| return | |
| } | |
| if (RootProxyService.state.value.isActiveState()) { | |
| RootProxyService.stop(appContext, markProtectionDisabled = false) | |
| delay(SERVICE_SWITCH_DELAY_MS) | |
| } | |
| if (shouldRun) { | |
| if (AdBlockVpnService.state.value == VpnState.RUNNING && restartTarget) { | |
| AdBlockVpnService.requestRestart(appContext) | |
| } else if (!AdBlockVpnService.state.value.isActiveState()) { | |
| AdBlockVpnService.start(appContext, startedFromBoot = startedFromBoot) | |
| } | |
| } else if (AdBlockVpnService.state.value.isActiveState()) { | |
| AdBlockVpnService.stop(appContext, markProtectionDisabled = false) | |
| } | |
| } |
🤖 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 `@app/src/main/java/app/pwhs/blockads/service/ServiceController.kt` around
lines 201 - 252, The reconcileToMode transition logic is racing because it reads
service state and then starts/stops services without any serialization, so
concurrent callers can interleave and violate mutual exclusivity. Add a single
shared guard in ServiceController (for example around reconcileToMode, and by
extension callers like requestStartNow/requestStopNow/requestRestartNow) so only
one reconciliation runs at a time, and keep the existing state checks and
start/stop decisions inside that critical section.
grosserknallkopf
force-pushed
the
root-proxy-reconciliation
branch
from
July 5, 2026 18:23
904c628 to
e22d216
Compare
grosserknallkopf
marked this pull request as draft
July 5, 2026 18:23
grosserknallkopf
force-pushed
the
root-proxy-reconciliation
branch
from
July 5, 2026 18:26
e22d216 to
904c628
Compare
grosserknallkopf
marked this pull request as ready for review
July 5, 2026 18:26
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@app/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.kt`:
- Around line 527-549: The probeStandaloneDns helper currently creates the
DatagramSocket before the guarded failure path, so a socket-construction
exception can escape instead of returning false. Move the DatagramSocket
allocation inside the try/finally in probeStandaloneDns so any constructor
failure is caught by the existing Exception handling and the probe fails closed;
keep the rest of the send/receive logic and socket.close cleanup unchanged.
- Around line 514-585: The readiness check is too strict in
isValidHealthResponse, so waitForStandaloneReady can fail on upstream DNS errors
even when the local engine is already up. Update the standalone probe flow in
waitForStandaloneReady/probeStandaloneDns to treat any well-formed response with
the matching HEALTH_QUERY_ID as readiness, rather than requiring rcode == 0, or
handle this health query locally in the engine so startup only depends on
liveness.
In `@app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt`:
- Around line 223-227: The RootProxyService main-thread path is blocking because
isRootRoutingAllowed() wraps the suspend DataStore read in runBlocking, and
stopProxy() likely does the same for setProtectionDesired(false). Update
RootProxyService to cache the routing mode in memory instead of querying
AppPreferences synchronously, and move the protection-state write onto
serviceScope so onStartCommand() and stopProxy() return without waiting for
DataStore I/O.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0483372-b4de-49d9-af00-959bbb755d8b
📒 Files selected for processing (25)
app/src/main/java/app/pwhs/blockads/MainActivity.ktapp/src/main/java/app/pwhs/blockads/data/datastore/AppPreferences.ktapp/src/main/java/app/pwhs/blockads/data/entities/ProfileManager.ktapp/src/main/java/app/pwhs/blockads/di/AppModule.ktapp/src/main/java/app/pwhs/blockads/service/AdBlockTileService.ktapp/src/main/java/app/pwhs/blockads/service/AdBlockVpnService.ktapp/src/main/java/app/pwhs/blockads/service/BootReceiver.ktapp/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.ktapp/src/main/java/app/pwhs/blockads/service/IptablesManager.ktapp/src/main/java/app/pwhs/blockads/service/ProtectionState.ktapp/src/main/java/app/pwhs/blockads/service/RootProxyService.ktapp/src/main/java/app/pwhs/blockads/service/ServiceController.ktapp/src/main/java/app/pwhs/blockads/service/TrustedNetworkManager.ktapp/src/main/java/app/pwhs/blockads/ui/filter/detail/FilterDetailScreen.ktapp/src/main/java/app/pwhs/blockads/ui/home/HomeScreen.ktapp/src/main/java/app/pwhs/blockads/ui/home/HomeViewModel.ktapp/src/main/java/app/pwhs/blockads/ui/httpsfiltering/HttpsFilteringScreen.ktapp/src/main/java/app/pwhs/blockads/ui/httpsfiltering/HttpsFilteringViewModel.ktapp/src/main/java/app/pwhs/blockads/ui/onboarding/OnboardingViewModel.ktapp/src/main/java/app/pwhs/blockads/ui/settings/SettingsViewModel.ktapp/src/main/java/app/pwhs/blockads/widget/WidgetToggleReceiver.ktapp/src/main/java/app/pwhs/blockads/worker/RootProxyResumeWorker.ktapp/src/main/java/app/pwhs/blockads/worker/VpnResumeWorker.ktapp/src/main/res/values-de/strings.xmlapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values-de/strings.xml
🚧 Files skipped from review as they are similar to previous changes (22)
- app/src/main/java/app/pwhs/blockads/ui/httpsfiltering/HttpsFilteringScreen.kt
- app/src/main/java/app/pwhs/blockads/ui/filter/detail/FilterDetailScreen.kt
- app/src/main/java/app/pwhs/blockads/ui/onboarding/OnboardingViewModel.kt
- app/src/main/java/app/pwhs/blockads/service/BootReceiver.kt
- app/src/main/java/app/pwhs/blockads/data/datastore/AppPreferences.kt
- app/src/main/java/app/pwhs/blockads/service/TrustedNetworkManager.kt
- app/src/main/java/app/pwhs/blockads/worker/VpnResumeWorker.kt
- app/src/main/java/app/pwhs/blockads/worker/RootProxyResumeWorker.kt
- app/src/main/res/values/strings.xml
- app/src/main/java/app/pwhs/blockads/service/ProtectionState.kt
- app/src/main/java/app/pwhs/blockads/MainActivity.kt
- app/src/main/java/app/pwhs/blockads/service/AdBlockTileService.kt
- app/src/main/java/app/pwhs/blockads/ui/settings/SettingsViewModel.kt
- app/src/main/java/app/pwhs/blockads/di/AppModule.kt
- app/src/main/java/app/pwhs/blockads/widget/WidgetToggleReceiver.kt
- app/src/main/java/app/pwhs/blockads/data/entities/ProfileManager.kt
- app/src/main/java/app/pwhs/blockads/ui/home/HomeScreen.kt
- app/src/main/java/app/pwhs/blockads/ui/httpsfiltering/HttpsFilteringViewModel.kt
- app/src/main/java/app/pwhs/blockads/service/AdBlockVpnService.kt
- app/src/main/java/app/pwhs/blockads/ui/home/HomeViewModel.kt
- app/src/main/java/app/pwhs/blockads/service/IptablesManager.kt
- app/src/main/java/app/pwhs/blockads/service/ServiceController.kt
Comment on lines
+514
to
+585
| private suspend fun waitForStandaloneReady( | ||
| port: Int, | ||
| startFailure: AtomicReference<Throwable?>, | ||
| ): Boolean { | ||
| val deadline = System.currentTimeMillis() + 2_500L | ||
| while (System.currentTimeMillis() < deadline) { | ||
| if (startFailure.get() != null) return false | ||
| if (probeStandaloneDns(port, "127.0.0.1")) return true | ||
| delay(100L) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| private suspend fun probeStandaloneDns(port: Int, host: String): Boolean = withContext(Dispatchers.IO) { | ||
| val query = buildHealthQuery() | ||
| val socket = DatagramSocket() | ||
| try { | ||
| socket.soTimeout = 700 | ||
| socket.send( | ||
| DatagramPacket( | ||
| query, | ||
| query.size, | ||
| InetAddress.getByName(host), | ||
| port | ||
| ) | ||
| ) | ||
| val response = ByteArray(512) | ||
| val packet = DatagramPacket(response, response.size) | ||
| socket.receive(packet) | ||
| isValidHealthResponse(response, packet.length) | ||
| } catch (e: Exception) { | ||
| false | ||
| } finally { | ||
| socket.close() | ||
| } | ||
| } | ||
|
|
||
| private fun buildHealthQuery(): ByteArray { | ||
| val out = ByteArrayOutputStream() | ||
| out.write((HEALTH_QUERY_ID shr 8) and 0xff) | ||
| out.write(HEALTH_QUERY_ID and 0xff) | ||
| out.write(0x01) | ||
| out.write(0x00) | ||
| out.write(0x00) | ||
| out.write(0x01) | ||
| out.write(0x00) | ||
| out.write(0x00) | ||
| out.write(0x00) | ||
| out.write(0x00) | ||
| out.write(0x00) | ||
| out.write(0x00) | ||
| HEALTH_DOMAIN.split('.').forEach { label -> | ||
| out.write(label.length) | ||
| out.write(label.toByteArray(Charsets.US_ASCII)) | ||
| } | ||
| out.write(0x00) | ||
| out.write(0x00) | ||
| out.write(0x01) | ||
| out.write(0x00) | ||
| out.write(0x01) | ||
| return out.toByteArray() | ||
| } | ||
|
|
||
| private fun isValidHealthResponse(response: ByteArray, length: Int): Boolean { | ||
| if (length < 12) return false | ||
| val id = ((response[0].toInt() and 0xff) shl 8) or (response[1].toInt() and 0xff) | ||
| if (id != HEALTH_QUERY_ID) return false | ||
| val flags = ((response[2].toInt() and 0xff) shl 8) or (response[3].toInt() and 0xff) | ||
| val isResponse = flags and 0x8000 != 0 | ||
| val rcode = flags and 0x000f | ||
| return isResponse && rcode == 0 | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'local\.pwhs\.app|HEALTH_DOMAIN'
rg -nP -C3 'startStandalone' --type kotlinRepository: pass-with-high-score/blockads-android
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files.
git ls-files | rg 'GoTunnelAdapter\.kt|Health|Standalone|Tunnel|DNS|go.*adapter|blockads'
echo '--- GoTunnelAdapter outline ---'
ast-grep outline app/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.kt --view expanded || true
echo '--- symbol search in GoTunnelAdapter.kt ---'
rg -n 'HEALTH_DOMAIN|HEALTH_QUERY_ID|waitForStandaloneReady|probeStandaloneDns|isValidHealthResponse|startStandalone|engine.stop|engine.start|DatagramSocket|local\.pwhs\.app' app/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.kt || true
echo '--- surrounding lines ---'
sed -n '450,650p' app/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.ktRepository: pass-with-high-score/blockads-android
Length of output: 25739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- health / standalone references ---'
rg -n -C 3 'local\.pwhs\.app|HEALTH_DOMAIN|HEALTH_QUERY_ID|isStandaloneHealthy|startStandalone|waitForStandaloneReady|probeStandaloneDns|rcode == 0|SERVFAIL|NXDOMAIN' \
app blockadstv .agent/skills -g '!**/build/**' || true
echo '--- GoTunnelAdapter startStandalone section ---'
sed -n '360,460p' app/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.kt
echo '--- architecture docs ---'
sed -n '1,260p' .agent/skills/blockads-dev/references/architecture.md
echo '--- root proxy docs ---'
sed -n '1,260p' .agent/skills/blockads-dev/references/root-proxy.md
echo '--- gomobile interop docs ---'
sed -n '1,260p' .agent/skills/blockads-dev/references/gomobile-interop.mdRepository: pass-with-high-score/blockads-android
Length of output: 28337
Relax standalone readiness to detect liveness, not upstream success.
waitForStandaloneReady only accepts rcode == 0, so a transient upstream DNS failure can make root-proxy startup abort even when the local engine is already listening. Accept any well-formed response with the matching ID as readiness, or answer this health query locally in the engine.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 544-544: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 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 `@app/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.kt` around lines
514 - 585, The readiness check is too strict in isValidHealthResponse, so
waitForStandaloneReady can fail on upstream DNS errors even when the local
engine is already up. Update the standalone probe flow in
waitForStandaloneReady/probeStandaloneDns to treat any well-formed response with
the matching HEALTH_QUERY_ID as readiness, rather than requiring rcode == 0, or
handle this health query locally in the engine so startup only depends on
liveness.
Comment on lines
+527
to
+549
| private suspend fun probeStandaloneDns(port: Int, host: String): Boolean = withContext(Dispatchers.IO) { | ||
| val query = buildHealthQuery() | ||
| val socket = DatagramSocket() | ||
| try { | ||
| socket.soTimeout = 700 | ||
| socket.send( | ||
| DatagramPacket( | ||
| query, | ||
| query.size, | ||
| InetAddress.getByName(host), | ||
| port | ||
| ) | ||
| ) | ||
| val response = ByteArray(512) | ||
| val packet = DatagramPacket(response, response.size) | ||
| socket.receive(packet) | ||
| isValidHealthResponse(response, packet.length) | ||
| } catch (e: Exception) { | ||
| false | ||
| } finally { | ||
| socket.close() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
DatagramSocket() is constructed outside the try/finally.
If the constructor throws (e.g. resource/FD exhaustion), the exception escapes probeStandaloneDns and propagates up through waitForStandaloneReady, isStandaloneHealthy, and isStandaloneIpv6Healthy instead of the intended false. Move the allocation inside the guarded block so the probe fails closed.
🛡️ Proposed fix
private suspend fun probeStandaloneDns(port: Int, host: String): Boolean = withContext(Dispatchers.IO) {
- val query = buildHealthQuery()
- val socket = DatagramSocket()
- try {
+ val query = buildHealthQuery()
+ var socket: DatagramSocket? = null
+ try {
+ socket = DatagramSocket()
socket.soTimeout = 700
socket.send(
DatagramPacket(
query,
query.size,
InetAddress.getByName(host),
port
)
)
val response = ByteArray(512)
val packet = DatagramPacket(response, response.size)
socket.receive(packet)
isValidHealthResponse(response, packet.length)
} catch (e: Exception) {
false
} finally {
- socket.close()
+ socket?.close()
}
}🧰 Tools
🪛 detekt (1.23.8)
[warning] 544-544: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 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 `@app/src/main/java/app/pwhs/blockads/service/GoTunnelAdapter.kt` around lines
527 - 549, The probeStandaloneDns helper currently creates the DatagramSocket
before the guarded failure path, so a socket-construction exception can escape
instead of returning false. Move the DatagramSocket allocation inside the
try/finally in probeStandaloneDns so any constructor failure is caught by the
existing Exception handling and the probe fails closed; keep the rest of the
send/receive logic and socket.close cleanup unchanged.
Comment on lines
+223
to
+227
| private fun isRootRoutingAllowed(): Boolean { | ||
| return runBlocking { | ||
| appPrefs.getRoutingModeSnapshot() == AppPreferences.ROUTING_MODE_ROOT | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e kt AppPreferences.kt --exec sed -n '1,200p' {}
echo '==== snapshot/desired impls ===='
rg -nP -C4 '(getRoutingModeSnapshot|setProtectionDesired)\b' app/src/main/javaRepository: pass-with-high-score/blockads-android
Length of output: 29461
🏁 Script executed:
ast-grep outline app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt --view expanded
sed -n '1,260p' app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt
echo '==== stopProxy area ===='
sed -n '360,420p' app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt
echo '==== AppPreferences routing/protection impl ===='
sed -n '560,620p' app/src/main/java/app/pwhs/blockads/data/datastore/AppPreferences.kt
sed -n '380,410p' app/src/main/java/app/pwhs/blockads/data/datastore/AppPreferences.ktRepository: pass-with-high-score/blockads-android
Length of output: 17016
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 1 <= i <= 260 or 360 <= i <= 420:
print(f"{i:4d}: {line}")
PYRepository: pass-with-high-score/blockads-android
Length of output: 14036
🏁 Script executed:
sed -n '1,220p' app/src/main/java/app/pwhs/blockads/service/RootProxyService.ktRepository: pass-with-high-score/blockads-android
Length of output: 8752
Avoid blocking the service main thread here. getRoutingModeSnapshot() and setProtectionDesired(false) are suspend DataStore calls, so wrapping them in runBlocking blocks onStartCommand()/stopProxy() until the read or write completes. Cache routing mode in memory and move the protection-state write onto serviceScope.
🤖 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 `@app/src/main/java/app/pwhs/blockads/service/RootProxyService.kt` around lines
223 - 227, The RootProxyService main-thread path is blocking because
isRootRoutingAllowed() wraps the suspend DataStore read in runBlocking, and
stopProxy() likely does the same for setProtectionDesired(false). Update
RootProxyService to cache the routing mode in memory instead of querying
AppPreferences synchronously, and move the protection-state write onto
serviceScope so onStartCommand() and stopProxy() return without waiting for
DataStore I/O.
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.
Summary
This PR hardens the Root Proxy / Local VPN routing lifecycle and fixes several UI/state mismatches that could make BlockAds report protection while the wrong service, no routing rules, or an unhealthy root DNS engine was actually active.
What changed
Validation
./gradlew :app:testDebugUnitTest :app:assembleDebugadb install -r.