fix(android): deliver default-interface updates synchronously#8890
Merged
garmr-ulfr merged 4 commits intoJul 1, 2026
Conversation
checkDefaultInterfaceUpdate dispatched each updateDefaultInterface call to GlobalScope.launch(Dispatchers.IO), so successive default-network changes could be applied out of order, pinning a stale interface index (Go side is last-writer-wins). The golang/go#68760 stack workaround that the launch was duplicating is already handled on the Go side (libbox SetupOptions.FixAndroidStack, enabled by radiance), so delivering synchronously preserves the network-change ordering that DefaultNetworkListener already guarantees. Remove the now-unused Bugs.fixAndroidStack helper.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDefaultNetworkMonitor's default-interface update logic is refactored to run on a dedicated single-thread executor instead of coroutine-based dispatch, replacing conditional Bugs.fixAndroidStack branching with sentinel constants and extracted retry-based interface resolution helpers. The now-unused Bugs utility object and its fixAndroidStack property are deleted. ChangesDefault network interface update refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant checkDefaultInterfaceUpdate
participant updateExecutor
participant resolveDefaultInterface
participant getInterfaceIndex
participant listener
checkDefaultInterfaceUpdate->>updateExecutor: execute update task
updateExecutor->>resolveDefaultInterface: resolve interface for network
loop retry up to INTERFACE_RESOLUTION_RETRY_COUNT
resolveDefaultInterface->>resolveDefaultInterface: read interfaceName from LinkProperties
resolveDefaultInterface->>getInterfaceIndex: get index via NetworkInterface or Os.if_nametoindex
getInterfaceIndex-->>resolveDefaultInterface: return index or null
alt resolution failed
resolveDefaultInterface->>resolveDefaultInterface: sleepBeforeRetry
end
end
resolveDefaultInterface-->>updateExecutor: return ResolvedInterface or null
updateExecutor->>listener: notifyDefaultInterface with resolved name/index
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses out-of-order delivery of Android default-interface updates to the Go/libbox side by removing the asynchronous GlobalScope.launch(Dispatchers.IO) dispatch and delivering updateDefaultInterface calls synchronously from DefaultNetworkMonitor.checkDefaultInterfaceUpdate. It also removes the now-unused Bugs.fixAndroidStack workaround helper.
Changes:
- Deliver
updateDefaultInterfacesynchronously to preserve the ordering guaranteed byDefaultNetworkListener. - Remove coroutine-based dispatch (
GlobalScope.launch(Dispatchers.IO)) and the unusedBugs.fixAndroidStackflag. - Replace magic “no interface” literals with constants (
NO_INTERFACE_NAME,NO_INTERFACE_INDEX).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| android/app/src/main/kotlin/org/getlantern/lantern/utils/Bugs.kt | Removes the unused Android stack workaround flag helper. |
| android/app/src/main/kotlin/org/getlantern/lantern/service/DefaultNetworkMonitor.kt | Makes default-interface updates synchronous and removes coroutine dispatch to preserve update ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Default-network callbacks are delivered on the main looper, so resolving the interface (which sleeps between retries and calls into Go over JNI) and delivering the update ran on the UI thread. Move that work onto a single-thread executor: it stays off the UI thread while still applying updates in network-change order, which a shared pool would not guarantee. Extract the resolution into resolveDefaultInterface/getInterfaceIndex/ sleepBeforeRetry helpers, snapshot the listener on the caller thread and mark it @volatile so the worker reliably observes setListener writes, and recover via if_nametoindex when getByName throws (not just when it returns null). Add logging when the interface is resolved, cleared, or fails to resolve.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@android/app/src/main/kotlin/org/getlantern/lantern/service/DefaultNetworkMonitor.kt`:
- Around line 82-103: The queued work in
DefaultNetworkMonitor.checkDefaultInterfaceUpdate can still call
updateDefaultInterface on a stale InterfaceUpdateListener after
closeDefaultInterfaceMonitor clears the field. Re-check the listener inside the
updateExecutor block before calling notifyDefaultInterface, or otherwise cancel
pending tasks, so only a live listener is notified. Use the listener variable in
checkDefaultInterfaceUpdate and the notifyDefaultInterface path to locate the
fix.
🪄 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 Plus
Run ID: 72249e7f-8c57-4f49-86fe-a7d9e7b81aea
📒 Files selected for processing (1)
android/app/src/main/kotlin/org/getlantern/lantern/service/DefaultNetworkMonitor.kt
notifyDefaultInterface re-checks that the snapshotted listener is still the active one before calling into libbox, so an update queued or being resolved across a setListener teardown can no longer invoke a closed handler. Also tidy the resolution logging: split the getByName attempt from the if_nametoindex fallback so the warning names the actual failing call and the fallback runs once, and drop the fixed attempt-count from the failure log since resolution can bail early on interruption.
atavism
reviewed
Jul 1, 2026
atavism
approved these changes
Jul 1, 2026
Contributor
|
Just one comment |
Os.if_nametoindex was added in API 26, but the app supports API 24+. Skip the name-based fallback on API 24/25 with an SDK_INT check instead of letting the call fail, and drop the runCatching that was masking it.
Contributor
Author
|
Thanks for the review @atavism! |
garmr-ulfr
merged commit 940b673
into
garmr/fix/android-default-interface-index
2 checks passed
atavism
added a commit
that referenced
this pull request
Jul 2, 2026
…8889) * fix(android): recover default-interface index via Os.if_nametoindex NetworkInterface.getByName relies on the interface-enumeration syscall, which is restricted on some Android versions/ROMs (returns null or throws there). When it failed, checkDefaultInterfaceUpdate exhausted its retries without ever calling updateDefaultInterface, so sing-box never received a default interface and every outbound failed with "no available network interface". Fall back to Os.if_nametoindex when getByName cannot resolve the index, mirroring the already-hardened getInterfaces sibling, and retry only when the index is still unresolvable. Also early-return on a null interface name instead of burning the retry budget on getByName(null). * move `Os.if_nametoindex` inside the `try` and yield 0 on throw Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(android): resolve default interface inside the retry loop getLinkProperties (and its interfaceName) can briefly return null right after a network change. Resolving it once before the loop meant a transient null abandoned the update entirely, leaving the default interface unset for that network change. Resolve inside the retry loop instead, matching upstream, so a transient null is retried rather than dropped. * fix(android): deliver default-interface updates synchronously (#8890) * fix(android): deliver default-interface updates synchronously checkDefaultInterfaceUpdate dispatched each updateDefaultInterface call to GlobalScope.launch(Dispatchers.IO), so successive default-network changes could be applied out of order, pinning a stale interface index (Go side is last-writer-wins). The golang/go#68760 stack workaround that the launch was duplicating is already handled on the Go side (libbox SetupOptions.FixAndroidStack, enabled by radiance), so delivering synchronously preserves the network-change ordering that DefaultNetworkListener already guarantees. Remove the now-unused Bugs.fixAndroidStack helper. * refactor(android): serialize interface updates on a background thread Default-network callbacks are delivered on the main looper, so resolving the interface (which sleeps between retries and calls into Go over JNI) and delivering the update ran on the UI thread. Move that work onto a single-thread executor: it stays off the UI thread while still applying updates in network-change order, which a shared pool would not guarantee. Extract the resolution into resolveDefaultInterface/getInterfaceIndex/ sleepBeforeRetry helpers, snapshot the listener on the caller thread and mark it @volatile so the worker reliably observes setListener writes, and recover via if_nametoindex when getByName throws (not just when it returns null). Add logging when the interface is resolved, cleared, or fails to resolve. * fix(android): skip interface updates for a torn-down listener notifyDefaultInterface re-checks that the snapshotted listener is still the active one before calling into libbox, so an update queued or being resolved across a setListener teardown can no longer invoke a closed handler. Also tidy the resolution logging: split the getByName attempt from the if_nametoindex fallback so the warning names the actual failing call and the fallback runs once, and drop the fixed attempt-count from the failure log since resolution can bail early on interruption. * fix(android): guard if_nametoindex fallback behind API 26 Os.if_nametoindex was added in API 26, but the app supports API 24+. Skip the name-based fallback on API 24/25 with an SDK_INT check instead of letting the call fail, and drop the runCatching that was masking it. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: atavism <atavism@users.noreply.github.com>
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
Default-network changes are delivered to libbox through
DefaultNetworkMonitor. EachupdateDefaultInterfacecall was dispatched toGlobalScope.launch(Dispatchers.IO), so successive network changes could beapplied out of order and pin a stale interface index on the Go side
(last-writer-wins) — making outbounds bind to the wrong interface and fail.
This PR delivers interface updates serialized on a single background thread,
preserving network-change order while keeping the resolution work — which
sleeps between retries and calls into Go over JNI — off the main-looper
callback thread.
Changes
updateDefaultInterfaceonto a single-thread executor: updatesapply in network-change order (a shared pool like
Dispatchers.IOcouldreorder them) and off the UI thread (the callback is delivered on the main
looper).
Bugs.fixAndroidStackhelper (Bugs.kt); thex/mobile: bind with go1.22.5,1.22.6 leads to: fatal error: runtime: stack split at bad time golang/go#68760 stack workaround it duplicated is handled on the Go side via
libbox
SetupOptions.FixAndroidStack(enabled by radiance).resolveDefaultInterface/getInterfaceIndex/sleepBeforeRetry.Os.if_nametoindexwhenNetworkInterface.getByNamethrows, not only when it returns null.@Volatileso theworker thread reliably observes
setListenerwrites.Stacking
Stacked on #8889 — base is
garmr/fix/android-default-interface-index, so thisPR's diff is only the ordering changes. Retarget to
mainonce #8889 merges.Summary by CodeRabbit