Skip to content

bg: soften sidecar readiness gate for non-MasterDnsVPN sidecars #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hawkff merged 1 commit into from
Jun 18, 2026
Merged

bg: soften sidecar readiness gate for non-MasterDnsVPN sidecars #27

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,16 @@ abstract class BoxInstance(
if (pending.isNotEmpty()) delay(50)
}
if (pending.isNotEmpty()) {
throw IOException("sidecar listener not ready on port(s): ${pending.joinToString()}")
// MasterDnsVPN must have its listener up before the first dial (it crashed
// otherwise), so a timeout there is fatal. Other sidecars (Mieru/Naïve/
// TrojanGo/Hysteria) were historically fire-and-forget: the first sing-box
// dial retries, so a slow bind shouldn't hard-fail VPN start — log and continue.
val message = "sidecar listener not ready on port(s): ${pending.joinToString()}"
if (hasMasterDnsVpn) {
throw IOException(message)
} else {
Logs.w("$message; continuing (sing-box will retry the connection)")
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Comment on lines 296 to +306

@coderabbitai coderabbitai Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Condition checks whether MasterDnsVPN is configured, not whether it failed to bind.

The hasMasterDnsVpn flag is computed at lines 269-271 based on whether MasterDnsVPN is in the configuration. However, when deciding to throw at line 302, the code should check if MasterDnsVPN's port is among the pending ports—not just whether MasterDnsVPN is configured.

Current behavior: If MasterDnsVPN is configured and binds successfully, but another sidecar (e.g., Mieru) is slow, the code still throws because hasMasterDnsVpn == true. This causes unnecessary startup failures and contradicts the comment's stated intent.

🐛 Proposed fix to check pending ports specifically 
             if (pending.isNotEmpty()) {
-                // MasterDnsVPN must have its listener up before the first dial (it crashed
-                // otherwise), so a timeout there is fatal. Other sidecars (Mieru/Naïve/
-                // TrojanGo/Hysteria) were historically fire-and-forget: the first sing-box
-                // dial retries, so a slow bind shouldn't hard-fail VPN start — log and continue.
+                // MasterDnsVPN must have its listener up before the first dial (it crashes
+                // otherwise), so a timeout on its port is fatal. Other sidecars (Mieru/Naïve/
+                // TrojanGo/Hysteria) were historically fire-and-forget: sing-box retries,
+                // so a slow bind shouldn't hard-fail VPN start — log and continue.
+                val masterDnsVpnPorts = config.externalIndex.flatMap { idx ->
+                    idx.chain.entries
+                        .filter { it.value.requireBean() is MasterDnsVpnBean }
+                        .map { it.key }
+                }.toSet()
+                val hasPendingMasterDnsVpn = pending.any { it in masterDnsVpnPorts }
                 val message = "sidecar listener not ready on port(s): ${pending.joinToString()}"
-                if (hasMasterDnsVpn) {
+                if (hasPendingMasterDnsVpn) {
                     throw IOException(message)
                 } else {
                     Logs.w("$message; continuing (sing-box will retry the connection)")
                 }
             }
🤖 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/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt` around
lines 296 - 306, The condition at line 302 checks if MasterDnsVPN is configured
using the hasMasterDnsVpn flag, but it should instead check if MasterDnsVPN's
specific port is in the pending list of failed port bindings. Replace the
hasMasterDnsVpn condition with a check that verifies whether MasterDnsVPN's
actual listening port appears in the pending ports, so that an exception is only
thrown if MasterDnsVPN itself failed to bind, not when it's merely configured
but bound successfully while another sidecar is slow.

}
}
}
Expand Down
Loading