Move service teardown off the main dispatcher

[hawkff]

Summary

• await native, traffic, and network teardown under a non-cancellable I/O boundary
• preserve cleanup order and primary failures
• cover cancellation, dispatcher, and failure behavior

Validation

• Namespace CI run 29216479788
• Android stop/restart gate pending

Greptile Summary

This PR moves service teardown into suspend helpers and off the main dispatcher. The main changes are:

• A shared teardown helper that runs cleanup under NonCancellable.
• Suspend service and VPN teardown paths that preserve cleanup ordering.
• Proxy teardown split into suspend persistence and synchronous Closeable compatibility.
• Tests for cancellation, dispatcher use, and failure propagation.

Confidence Score: 4/5

The synchronous proxy close bridge can hang teardown when it runs on the Default dispatcher.

• The main suspend service teardown path preserves cleanup ordering and has focused tests.
• The compatibility close() path blocks Dispatchers.Default while waiting for looper shutdown work that can need the same dispatcher.
• A stuck close can block service shutdown or restart.

app/src/main/java/io/nekohasekai/sagernet/bg/proto/ProxyInstance.kt

Important Files Changed

Filename	Overview
app/src/main/java/io/nekohasekai/sagernet/bg/BaseService.kt	Changes service teardown to a suspend helper that closes and persists the proxy before releasing service resources.
app/src/main/java/io/nekohasekai/sagernet/bg/ServiceTeardownExecution.kt	Adds shared teardown helpers for non-cancellable cleanup and primary failure preservation.
app/src/main/java/io/nekohasekai/sagernet/bg/VpnService.kt	Updates VPN teardown to close the VPN connection before delegating to base service cleanup.
app/src/main/java/io/nekohasekai/sagernet/bg/proto/ProxyInstance.kt	Splits proxy teardown into suspend persistence and a synchronous close bridge, with a dispatcher self-blocking risk in the bridge.
app/src/test/java/io/nekohasekai/sagernet/bg/ServiceTeardownExecutionTest.kt	Adds tests for teardown completion, cancellation, dispatcher routing, and failure handling.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Stop as Service stop path
    participant Helper as runServiceTeardown
    participant Proxy as ProxyInstance
    participant Looper as TrafficLooper
    Stop->>Helper: killProcesses()
    Helper->>Proxy: closeAndPersist() on IO
    Proxy->>Proxy: super.close()
    Proxy->>Looper: stop() and persist traffic
    Helper->>Stop: release wake lock / stop listener
    participant Closeable as Closeable caller
    Closeable->>Proxy: close()
    Proxy->>Proxy: runBlocking(Dispatchers.Default)
    Proxy->>Looper: stop() waits for looper job

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Stop as Service stop path
    participant Helper as runServiceTeardown
    participant Proxy as ProxyInstance
    participant Looper as TrafficLooper
    Stop->>Helper: killProcesses()
    Helper->>Proxy: closeAndPersist() on IO
    Proxy->>Proxy: super.close()
    Proxy->>Looper: stop() and persist traffic
    Helper->>Stop: release wake lock / stop listener
    participant Closeable as Closeable caller
    Closeable->>Proxy: close()
    Proxy->>Proxy: runBlocking(Dispatchers.Default)
    Proxy->>Looper: stop() waits for looper job

Loading

_{Reviews (1): Last reviewed commit: "fix(service): suspend blocking teardown ..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 22 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9e8a4073-0f97-47e9-92df-cf502890b5d4

📥 Commits

Reviewing files that changed from the base of the PR and between 6e24c2d and 799996f.

📒 Files selected for processing (5)

• app/src/main/java/io/nekohasekai/sagernet/bg/BaseService.kt
• app/src/main/java/io/nekohasekai/sagernet/bg/ServiceTeardownExecution.kt
• app/src/main/java/io/nekohasekai/sagernet/bg/VpnService.kt
• app/src/main/java/io/nekohasekai/sagernet/bg/proto/ProxyInstance.kt
• app/src/test/java/io/nekohasekai/sagernet/bg/ServiceTeardownExecutionTest.kt

---

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Default Dispatcher Blocks Itself

When a synchronous Closeable.close() caller reaches this path from a Dispatchers.Default worker, runBlocking(Dispatchers.Default) can occupy the dispatcher while closeAndPersist() waits for the traffic looper job to cancel and join on the same dispatcher. That can leave teardown stuck indefinitely and block service shutdown or restart.

[hawkff]

No change. Normal service shutdown uses the suspending close path and never calls this compatibility override. The override preserves the prior behavior for unexpected Closeable callers; one caller cannot self-starve the shared Default scheduler because another worker remains available for the traffic job.

[greptile-apps]

That's a fair correction. Dispatchers.Default is a thread pool (minimum 2 threads), so a single runBlocking call on it won't starve itself — at least one other thread remains available to run the looper cancellation work. My original comment overstated the risk for the single-caller case.

The remaining (much narrower) scenario would be multiple concurrent unexpected Closeable callers saturating the Default pool simultaneously, but that's contrived given the suspend path is the normal route and the compatibility override is a last-resort bridge. Your point stands.

Would you like me to remember a rule along the lines of: "don't flag runBlocking(Dispatchers.Default) as a self-starvation risk unless pool exhaustion across concurrent callers is plausible, given Default is a multi-threaded pool"?

[hawkff]

Verified on Android: stop, restart, and profile reload completed without an ANR or teardown error; final traffic totals persisted and existing app data remained intact.