fix(olcrtc): supervise sidecar recovery - #156
Closed
hawkff wants to merge 4 commits into
Closed
Conversation
📝 WalkthroughWalkthroughAdds guarded process restart backoff and lifecycle handling, integrates olcRTC recovery with asynchronous listener readiness checks, monitors runtime shutdown health, adds Kotlin and Go tests, runs Go tests in the wrapper script, and updates the sidecar cache checksum inputs. ChangesolcRTC lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BoxInstance
participant GuardedProcessPool
participant olcRTC
participant OS
BoxInstance->>GuardedProcessPool: start olcRTC with restart policy
GuardedProcessPool->>olcRTC: start process
olcRTC-->>BoxInstance: publish readiness marker and listener readiness
BoxInstance-->>GuardedProcessPool: complete restart readiness
olcRTC-->>GuardedProcessPool: report process exit
GuardedProcessPool->>GuardedProcessPool: calculate restart backoff
OS-->>olcRTC: deliver SIGTERM or SIGINT
olcRTC-->>BoxInstance: return shutdown result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
buildScript/lib/olcrtc-src/main.go (1)
95-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: extract the ticker interval into a named constant.
The
15 * time.Secondliteral would read better as a named constant near the top of the file, especially since it defines the unhealthy-detection granularity.Proposed refactor
+const healthCheckInterval = 15 * time.Second + func main() { ... - ticker := time.NewTicker(15 * time.Second) + ticker := time.NewTicker(healthCheckInterval)🤖 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 `@buildScript/lib/olcrtc-src/main.go` around lines 95 - 106, The ticker interval in the main runtime flow should use a named constant instead of the inline 15-second duration. Define the constant near the top of the file and update the time.NewTicker call in main to reference it, preserving the existing unhealthy-detection interval.
🤖 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 `@app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt`:
- Around line 420-422: Update the sidecar readiness failure condition around the
pending-port message to base fatality on which sidecars are actually present in
pending, rather than the configuration-wide hasMasterDnsVpn or hasOlcrtc flags.
Ensure unrelated sidecar timeouts remain non-fatal when the olcRTC and
MasterDnsVPN ports are ready, while preserving strict-mode failures.
- Around line 304-315: The olcRTC restart flow around
GuardedProcessRestartPolicy and awaitExternalPortReady must prevent another
local process from impersonating the sidecar. Rotate SOCKS credentials for each
restart generation and make readiness validate a sidecar-specific authenticated
handshake rather than merely accepting any listener on 127.0.0.1:port;
alternatively use a pre-bound app-private socket while preserving restart
behavior.
---
Nitpick comments:
In `@buildScript/lib/olcrtc-src/main.go`:
- Around line 95-106: The ticker interval in the main runtime flow should use a
named constant instead of the inline 15-second duration. Define the constant
near the top of the file and update the time.NewTicker call in main to reference
it, preserving the existing unhealthy-detection interval.
🪄 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: 71630511-190d-4f28-b057-2c914570e598
📒 Files selected for processing (9)
.github/workflows/ci.ymlapp/src/main/java/io/nekohasekai/sagernet/bg/GuardedProcessPool.ktapp/src/main/java/io/nekohasekai/sagernet/bg/GuardedProcessRestartPolicy.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/TestInstance.ktapp/src/test/java/io/nekohasekai/sagernet/bg/GuardedProcessRestartPolicyTest.ktbuildScript/lib/olcrtc-src/main.gobuildScript/lib/olcrtc-src/main_test.gobuildScript/lib/olcrtc.sh
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
buildScript/lib/olcrtc-src/main.go (1)
109-112: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove the readiness marker before
log.Fatal
log.Fatalexits viaos.Exit, so the deferredremoveReadyMarkeris skipped on runtime-loss shutdowns. Clear the marker first to avoid leaving the app marked ready after exit.🤖 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 `@buildScript/lib/olcrtc-src/main.go` around lines 109 - 112, Update the runtime-loss branch after waitAfterReady to remove the readiness marker before calling log.Fatal. Invoke the existing removeReadyMarker cleanup directly in that branch so the marker is cleared before process termination.
🧹 Nitpick comments (1)
buildScript/lib/olcrtc-src/main_test.go (1)
60-80: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid a deadlock on the test failure path.
If
waitAfterReadyunexpectedly returns after the healthy tick, the unbufferedsignals <- syscall.SIGTERMsend blocks forever instead of allowing the assertion to fail. Use buffered channels (or a timeout) for this test.Proposed fix
- signals := make(chan os.Signal) - ticks := make(chan time.Time) + signals := make(chan os.Signal, 1) + ticks := make(chan time.Time, 1)🤖 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 `@buildScript/lib/olcrtc-src/main_test.go` around lines 60 - 80, Update TestWaitAfterReadyHealthyTicksContinue to avoid blocking on the cleanup signal when waitAfterReady exits unexpectedly: make the signals channel buffered, or otherwise add a bounded send/timeout, while preserving the existing tick and result assertions.
🤖 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
`@app/src/main/java/io/nekohasekai/sagernet/bg/proto/SidecarReadinessPolicy.kt`:
- Around line 6-10: Update shouldFailSidecarReadiness so strict mode only causes
failure when pendingPorts is non-empty, while preserving failure for pending
required ports and returning false when all ports are ready.
---
Outside diff comments:
In `@buildScript/lib/olcrtc-src/main.go`:
- Around line 109-112: Update the runtime-loss branch after waitAfterReady to
remove the readiness marker before calling log.Fatal. Invoke the existing
removeReadyMarker cleanup directly in that branch so the marker is cleared
before process termination.
---
Nitpick comments:
In `@buildScript/lib/olcrtc-src/main_test.go`:
- Around line 60-80: Update TestWaitAfterReadyHealthyTicksContinue to avoid
blocking on the cleanup signal when waitAfterReady exits unexpectedly: make the
signals channel buffered, or otherwise add a bounded send/timeout, while
preserving the existing tick and result assertions.
🪄 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: d1051ce9-7f9a-4d75-bd26-012c9c60280c
📒 Files selected for processing (6)
app/src/main/java/io/nekohasekai/sagernet/bg/GuardedProcessPool.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/SidecarReadinessPolicy.ktapp/src/test/java/io/nekohasekai/sagernet/bg/proto/SidecarReadinessPolicyTest.ktbuildScript/lib/olcrtc-src/main.gobuildScript/lib/olcrtc-src/main_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt
Owner
Author
|
Superseded by #157, which includes this recovery work with validation, the tested runtime pin, and the combined follow-up fixes. |
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
1 participant
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
Validation
29280508001: passedRollout
Android end-to-end verification is still required before merge.
Greptile Summary
This PR adds supervised recovery for the olcRTC sidecar. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "fix: handle empty sidecar readiness sets" | Re-trigger Greptile