bg: soften sidecar readiness gate for non-MasterDnsVPN sidecars#27
Conversation
📝 WalkthroughWalkthroughIn ChangesMasterDnsVPN Fatal Timeout Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - name: Gradle Build | ||
| env: | ||
| BUILD_PLUGIN: none | ||
| run: | | ||
| echo "sdk.dir=${ANDROID_HOME}" > local.properties | ||
| echo "ndk.dir=${ANDROID_HOME}/ndk/25.0.8775105" >> local.properties | ||
| export LOCAL_PROPERTIES="${{ secrets.LOCAL_PROPERTIES }}" | ||
| ./run init action gradle | ||
| ./gradlew app:assembleOssDebug | ||
| APK=$(find app/build/outputs/apk -name '*arm64-v8a*.apk') | ||
| APK=$(dirname $APK) | ||
| echo "APK=$APK" >> $GITHUB_ENV |
There was a problem hiding this comment.
The "Gradle Build" step appears twice in this job (once at the original location around line 143, and again here after "Save Gradle cache"). This means ./gradlew app:assembleOssDebug will run twice on every CI run, doubling build time and compute cost. The duplicate was likely introduced when the "Save Gradle cache" step was inserted: the existing block was accidentally re-added below it. The other three workflows (build.yml, preview.yml, release.yml) correctly have only one "Gradle Build" followed by "Save Gradle cache" — the same pattern should apply here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.github/workflows/build.yml:
- Around line 43-49: The current workflow relies on cache to pass the
libcore.aar artifact between jobs, but on pull_request events the Save LibCore
Cache step skips saving when there is a cache miss, causing downstream jobs that
depend on this artifact to fail deterministically. Replace the cache-based
handoff with GitHub Actions artifact upload and download mechanisms: in the
producer job that builds libcore.aar, add an upload-artifact action to persist
the artifact regardless of cache hit status, and in downstream consumer jobs,
add a download-artifact action to retrieve it before use, with an optional
fallback rebuild step if the artifact is not found. Apply this same pattern to
all other producer-consumer artifact handoffs mentioned in the workflow file.
In @.github/workflows/ci.yml:
- Around line 161-172: The Gradle Build step that performs the
app:assembleOssDebug build is duplicated in the CI workflow, causing unnecessary
builds and wasted resources. Remove this entire duplicate Gradle Build step from
the workflow file since the same build operation is already performed in another
step earlier in the workflow. Keep only one instance of the step that builds the
OSS debug APK to avoid redundant builds.
In `@app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt`:
- Around line 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.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 770cb0ed-f62b-40cd-97ff-7b45e8677e88
📒 Files selected for processing (5)
.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.ymlapp/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt
| - name: Save LibCore Cache | ||
| if: github.event_name != 'pull_request' && steps.cache.outputs.cache-hit != 'true' | ||
| uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 | ||
| with: | ||
| path: | | ||
| app/libs/libcore.aar | ||
| key: ${{ steps.cache.outputs.cache-primary-key }} |
There was a problem hiding this comment.
PR cache-miss path can break downstream APK builds.
On pull_request, producer jobs can build artifacts on cache miss but skip saving them; the downstream build job is restore-only and then hard-fails if sidecar artifacts are absent. This creates a deterministic failure mode when cache keys change in PRs.
A PR-safe intra-workflow handoff (e.g., producer upload + consumer download artifacts) or a consumer-side build fallback is needed.
Also applies to: 82-89, 111-126
🤖 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 @.github/workflows/build.yml around lines 43 - 49, The current workflow
relies on cache to pass the libcore.aar artifact between jobs, but on
pull_request events the Save LibCore Cache step skips saving when there is a
cache miss, causing downstream jobs that depend on this artifact to fail
deterministically. Replace the cache-based handoff with GitHub Actions artifact
upload and download mechanisms: in the producer job that builds libcore.aar, add
an upload-artifact action to persist the artifact regardless of cache hit
status, and in downstream consumer jobs, add a download-artifact action to
retrieve it before use, with an optional fallback rebuild step if the artifact
is not found. Apply this same pattern to all other producer-consumer artifact
handoffs mentioned in the workflow file.
| - name: Gradle Build | ||
| env: | ||
| BUILD_PLUGIN: none | ||
| run: | | ||
| echo "sdk.dir=${ANDROID_HOME}" > local.properties | ||
| echo "ndk.dir=${ANDROID_HOME}/ndk/25.0.8775105" >> local.properties | ||
| export LOCAL_PROPERTIES="${{ secrets.LOCAL_PROPERTIES }}" | ||
| ./run init action gradle | ||
| ./gradlew app:assembleOssDebug | ||
| APK=$(find app/build/outputs/apk -name '*arm64-v8a*.apk') | ||
| APK=$(dirname $APK) | ||
| echo "APK=$APK" >> $GITHUB_ENV |
There was a problem hiding this comment.
Duplicate Gradle build step runs OSS debug build twice.
This second Gradle Build block repeats the same commands and unnecessarily doubles CI time/cost.
Proposed fix
- - name: Gradle Build
- env:
- BUILD_PLUGIN: none
- run: |
- echo "sdk.dir=${ANDROID_HOME}" > local.properties
- echo "ndk.dir=${ANDROID_HOME}/ndk/25.0.8775105" >> local.properties
- export LOCAL_PROPERTIES="${{ secrets.LOCAL_PROPERTIES }}"
- ./run init action gradle
- ./gradlew app:assembleOssDebug
- APK=$(find app/build/outputs/apk -name '*arm64-v8a*.apk')
- APK=$(dirname $APK)
- echo "APK=$APK" >> $GITHUB_ENV🤖 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 @.github/workflows/ci.yml around lines 161 - 172, The Gradle Build step that
performs the app:assembleOssDebug build is duplicated in the CI workflow,
causing unnecessary builds and wasted resources. Remove this entire duplicate
Gradle Build step from the workflow file since the same build operation is
already performed in another step earlier in the workflow. Keep only one
instance of the step that builds the OSS debug APK to avoid redundant builds.
| 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)") | ||
| } |
There was a problem hiding this comment.
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.
awaitExternalProcessesReady previously threw IOException (hard VPN-start failure) for any sidecar that didn't bind within maxOf(1s, connectionTestTimeout). MasterDnsVPN genuinely needs that gate (it crashed if dialed before binding), but Mieru/Naive/TrojanGo/Hysteria were historically fire-and-forget with first-dial retry. Now only MasterDnsVPN throws on timeout; the others log a warning and continue. (Greptile, PR #18 follow-up)
hawkff
force-pushed
the
chore/ci-and-sidecar-hardening
branch
from
51cda3a to
3a4b855
Compare
hawkff
changed the title
1 participant
Summary
Follow-up to #18.
awaitExternalProcessesReadypreviously threwIOException(hard VPN-start failure) for any sidecar that didn't bind withinmaxOf(1s, connectionTestTimeout).MasterDnsVPN genuinely needs that gate (it crashed if dialed before its listener was up), but Mieru/Naïve/TrojanGo/Hysteria were historically fire-and-forget — the first sing-box dial retries. So a slow bind on a low-end device shouldn't hard-fail VPN start. Now only MasterDnsVPN throws on timeout; the others log a warning and continue.
Note on cache-poisoning hardening
This branch originally also split
actions/cacheinto gated restore/save (the deferred #18 item). I dropped that change: this repo's CI uses the cache as the cross-job artifact bus (thebuildjobneeds:libcore+sidecars and restores their caches to assemble the APK). Gatingsaveto non-pull_requestevents breaks that bus on every PR run — CI confirmed the APK job failed withsidecar cache miss. The cache keys are already content-addressed (hashFilesof workflows + status files + pinned versions), so poisoning risk is low. A proper fix would migrate intra-run job passing toupload/download-artifact— that's a separate, larger change, tracked separately.Verification
Greptile Summary
This PR softens the sidecar readiness gate in
awaitExternalProcessesReady: previously any pending sidecar at timeout would hard-fail VPN startup with anIOException; now only a timeout on a config that includes MasterDnsVPN is fatal, while all other sidecar types log a warning and allow startup to proceed.IOException, matching their historical fire-and-forget startup contract.IOExceptionon timeout — is preserved for configs that include MasterDnsVPN, which crashes if dialed before its listener is up.Confidence Score: 4/5
Safe to merge for pure non-MasterDnsVPN configs; a mixed config (MasterDnsVPN + any other sidecar) where only the non-critical sidecar misses the deadline will still throw a fatal IOException despite MasterDnsVPN being ready.
The change correctly softens the gate for standalone non-critical sidecars, but the guard condition (hasMasterDnsVpn) is derived from the full config rather than from which ports remain in pending at timeout. In a mixed setup, MasterDnsVPN binding successfully does not remove hasMasterDnsVpn as true, so a slow Mieru/Naïve/etc. sidecar still triggers the fatal path — the exact failure mode the PR intends to eliminate.
app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt — the hasMasterDnsVpn guard at the timeout site needs to check whether any MasterDnsVPN port specifically is still in pending, not whether MasterDnsVPN exists anywhere in the config.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[awaitExternalProcessesReady called] --> B{processes initialized\nand ports non-empty?} B -- No --> C[return early] B -- Yes --> D[Compute hasMasterDnsVpn\nfrom config] D --> E{hasMasterDnsVpn?} E -- Yes --> F[timeout = max 60s, connTestTimeout] E -- No --> G[timeout = max 1s, connTestTimeout] F --> H[Poll all ports every 50ms\nuntil bound or deadline] G --> H H --> I{pending empty\nat deadline?} I -- No --> J{hasMasterDnsVpn?} I -- Yes --> K[return — all sidecars ready] J -- Yes --> L[throw IOException\nfatal VPN start failure] J -- No --> M[Logs.w warning\ncontinue — sing-box retries]%%{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"}}}%% flowchart TD A[awaitExternalProcessesReady called] --> B{processes initialized\nand ports non-empty?} B -- No --> C[return early] B -- Yes --> D[Compute hasMasterDnsVpn\nfrom config] D --> E{hasMasterDnsVpn?} E -- Yes --> F[timeout = max 60s, connTestTimeout] E -- No --> G[timeout = max 1s, connTestTimeout] F --> H[Poll all ports every 50ms\nuntil bound or deadline] G --> H H --> I{pending empty\nat deadline?} I -- No --> J{hasMasterDnsVpn?} I -- Yes --> K[return — all sidecars ready] J -- Yes --> L[throw IOException\nfatal VPN start failure] J -- No --> M[Logs.w warning\ncontinue — sing-box retries]Reviews (2): Last reviewed commit: "bg: soften sidecar readiness gate for no..." | Re-trigger Greptile