Conversation
Reviewer's GuideThis refactor replaces the distributed launcher, persisted-status, session-token, and broadcast lifecycle protocol with a single mutex-serialized RuntimeCoordinator exposing RuntimeState via StateFlow, while simplifying startup probes and config compilation and tightening VPN/root shutdown and recovery behavior. Sequence diagram for serialized runtime start and state publicationsequenceDiagram
participant Caller
participant C as RuntimeCoordinator
participant M as mutex
participant Host as TunService or RootRuntime
participant Core
participant State as StateFlow<RuntimeState>
Caller->>C: start(mode, source)
C->>M: runOp(startLocked)
M-->>C: lock acquired
C->>State: publish Starting
C->>Host: start or launch
Host->>Core: start process and load config
Core-->>Host: controller ready
Host-->>C: start complete
C->>State: publish Running
C-->>Caller: return
Sequence diagram for coordinated stop and VPN teardownsequenceDiagram
participant Caller
participant C as RuntimeCoordinator
participant M as mutex
participant Session as VpnSession
participant Service as TunService
participant Core
participant State as StateFlow<RuntimeState>
Caller->>C: stop(reason)
C->>M: runOp(stopLocked)
M-->>C: lock acquired
C->>State: publish Stopping
C->>Session: stop()
Session->>Core: stop and wait for process exit
C->>Service: finish()
Service-->>C: onDestroy
C->>State: publish Idle
C-->>Caller: return
State diagram for RuntimeCoordinator lifecyclestateDiagram-v2
[*] --> Idle
Idle --> Starting: start(mode, source)
Starting --> Running: controller ready
Starting --> Failed: startup error
Starting --> Idle: cancellation or cleanup
Running --> Reloading: reload()
Reloading --> Running: reload accepted
Reloading --> Running: reload rejected and restored
Reloading --> Failed: runtime unavailable
Running --> Stopping: stop(reason)
Running --> Idle: verify() detects vanished core
Stopping --> Idle: teardown complete
Failed --> Starting: start(mode, source)
Failed --> Idle: stop(reason)
Flow diagram for runtime readiness probingflowchart TD
Start[Core launched] --> Probe[ControllerReadiness.await]
Probe --> Alive{Core alive?}
Alive -- No --> Fail[Fail startup]
Alive -- Yes --> Group[GET /group]
Group --> Ready{Groups ready or config has no groups?}
Ready -- Yes --> Running[Publish Running]
Ready -- No --> Backoff[Wait 25ms, 50ms, then 100ms]
Backoff --> Probe
Probe --> Timeout{2s budget exceeded?}
Timeout -- Yes --> Fail
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
YumeLira
added this pull request to stack #190
September 27, 2026 10:44
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="runtime/service/src/runtime/service/RuntimeCoordinator.kt" line_range="169" />
<code_context>
+
+ internal fun onVpnServiceDestroyed(service: TunService) {
+ if (vpnService === service) vpnService = null
+ vpnDestroy?.complete(Unit)
+ if (activeVpn === service) {
+ scope.launch {
</code_context>
<issue_to_address>
**issue (bug_risk):** A late `onVpnServiceDestroyed` callback from an old `TunService` can complete the `vpnDestroy` deferred belonging to a newer service because the callback completes `vpnDestroy` before checking the service identity, and `stopVpnLocked` stores only an unassociated deferred. The coordinator then stops waiting early and can proceed while the current VPN service is still being destroyed.
**Triggers:** When a VPN service destruction exceeds the 3-second timeout and a replacement service is started before the old `onDestroy` callback arrives.
**Suggested fix:** Associate the destruction deferred with its expected service and complete it only when `service === expectedService`.
</issue_to_address>
### Comment 2
<location path="runtime/service/src/runtime/service/RuntimeCoordinator.kt" line_range="320-321" />
<code_context>
+ private suspend fun startVpnLocked(source: RuntimeStartSource) {
+ RuntimeLog.writer(context, RuntimeLog.Source.LocalTun)
+ .beginSession(RuntimeLog.Type.Launcher, "request start source=$source mode=VpnService")
+ val service = vpnService ?: awaitVpnService()
+ activeVpn = service
+ val spec = withContext(Dispatchers.IO) { SessionRuntimeSpecFactory(context).createVpnSpec() }
+ service.session.start(spec)
</code_context>
<issue_to_address>
**issue (bug_risk):** `startVpnLocked` reuses any non-null `vpnService` without verifying that the instance is still alive. After `stopVpnLocked` times out waiting for destruction, `vpnService` remains set until the delayed callback, so a new start can call `session.start` on the old service instance while its teardown is still pending.
**Triggers:** When `TunService.onDestroy` is delayed beyond `VPN_DESTROY_TIMEOUT_MS` and another start follows immediately.
**Suggested fix:** Clear or invalidate `vpnService` before allowing a replacement start, and await or explicitly reject starts while the previous service is still being destroyed.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and a lifecycle error in the new coordinator could leave the VPN or root daemon running incorrectly, fail to stop it, or route traffic through an unintended runtime; that can cause an outage or security-relevant traffic exposure. Reverting restores the previous code, but it may not immediately undo an already-started daemon, VPN session, or traffic impact.
Blocking findings: runtime/service/src/runtime/service/RuntimeCoordinator.kt:169, runtime/service/src/runtime/service/RuntimeCoordinator.kt:321
|
|
||
| internal fun onVpnServiceDestroyed(service: TunService) { | ||
| if (vpnService === service) vpnService = null | ||
| vpnDestroy?.complete(Unit) |
There was a problem hiding this comment.
issue (bug_risk): A late onVpnServiceDestroyed callback from an old TunService can complete the vpnDestroy deferred belonging to a newer service because the callback completes vpnDestroy before checking the service identity, and stopVpnLocked stores only an unassociated deferred. The coordinator then stops waiting early and can proceed while the current VPN service is still being destroyed.
Triggers: When a VPN service destruction exceeds the 3-second timeout and a replacement service is started before the old onDestroy callback arrives.
Suggested fix: Associate the destruction deferred with its expected service and complete it only when service === expectedService.
Comment on lines
+320
to
+321
| val service = vpnService ?: awaitVpnService() | ||
| activeVpn = service |
There was a problem hiding this comment.
issue (bug_risk): startVpnLocked reuses any non-null vpnService without verifying that the instance is still alive. After stopVpnLocked times out waiting for destruction, vpnService remains set until the delayed callback, so a new start can call session.start on the old service instance while its teardown is still pending.
Triggers: When TunService.onDestroy is delayed beyond VPN_DESTROY_TIMEOUT_MS and another start follows immediately.
Suggested fix: Clear or invalidate vpnService before allowing a replacement start, and await or explicitly reject starts while the previous service is still being destroyed.
Replace the launcher/status-store/broadcast protocol with a single in-process coordinator that serializes start, stop, reload and verify behind one mutex and publishes a StateFlow<RuntimeState>. - UI, tile, auto-restart, Wi-Fi automation and config reloads all call the coordinator; the MMKV phase slot, session token and lifecycle broadcasts are gone. - Readiness probes GET /group with 25/50/100ms backoff instead of full /proxies queries every 200ms; group names are parsed only on demand. - Drop dead work on the start path: config fingerprints, the unused post-start snapshot refresh, the service log stream, no-op observers and the transport fallback compile. - VPN stop waits for TunService.onDestroy and the core pid; connection tracking polls only while the app is in the foreground.
YumeYuka
force-pushed
the
refactor-simplify-runtime-start-stop
branch
from
September 27, 2026 12:09
f501e8b to
bd14377
Compare
YumeYuka
approved these changes
Sep 27, 2026
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
2 participants
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.
Replace the launcher/status-store/broadcast protocol with a single
in-process coordinator that serializes start, stop, reload and verify
behind one mutex and publishes a StateFlow.
the coordinator; the MMKV phase slot, session token and lifecycle
broadcasts are gone.
/proxies queries every 200ms; group names are parsed only on demand.
post-start snapshot refresh, the service log stream, no-op observers
and the transport fallback compile.
tracking polls only while the app is in the foreground.
Summary by Sourcery
Route every local runtime start, stop, reload, and verification operation through one serialized coordinator with a single observable state source.
New Features:
Bug Fixes:
Enhancements:
Chores: