fix(olcrtc): validate profiles and recover sidecars - #157
Merged
Conversation
This was referenced Jul 14, 2026
📝 WalkthroughWalkthroughThis PR adds guarded process restart policies and OLCRTC readiness markers, centralizes stricter OLCRTC profile validation, updates the OLCRTC wrapper lifecycle, and retargets build workflows to a new pinned repository revision. ChangesRuntime restart and sidecar readiness
OLCRTC profile validation
OLCRTC build integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant BoxInstance
participant GuardedProcessPool
participant olcrtc-socks
participant ReadyMarker
BoxInstance->>GuardedProcessPool: start OLCRTC with restart policy
GuardedProcessPool->>olcrtc-socks: launch with ready-marker path
olcrtc-socks->>ReadyMarker: publish ready marker after WaitReady
BoxInstance->>ReadyMarker: poll marker and external port
GuardedProcessPool->>olcrtc-socks: clear marker before restart
olcrtc-socks->>ReadyMarker: republish marker after recovery
Possibly related PRs
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: 1
🧹 Nitpick comments (2)
app/src/main/java/io/nekohasekai/sagernet/fmt/olcrtc/OlcrtcFmt.kt (1)
140-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnrecognized payload keys are silently dropped instead of rejected.
The
when (key)block has noelsebranch, so a typo likevp8-fp=30(missing hyphen) or any future/unsupported key silently falls through, leavingvp8Fpsat its default with no error. Every other malformed-payload path in this file (missing=, empty key, non-integer vp8 values, delimiter-bearing client id) fails fast — this is the one case that fails silently.♻️ Proposed fix
payloadValues.forEach { (key, value) -> when (key) { "vp8-fps" -> vp8Fps = value.toIntOrNull() ?: throw IllegalArgumentException("olcRTC: VP8 FPS must be an integer") "vp8-batch" -> vp8BatchSize = value.toIntOrNull() ?: throw IllegalArgumentException("olcRTC: VP8 batch size must be an integer") // Our non-standard pairing-token carrier. "cid", "client-id", "clientid" -> { require(value.none { it in DELIMITERS }) { "olcRTC: client id contains a reserved delimiter" } clientId = value } + + else -> throw IllegalArgumentException("olcRTC: unrecognized transport payload key") } }If unknown keys are meant to be ignored on purpose (e.g. forward-compatibility with a newer wrapper), please confirm — otherwise this should fail fast like the rest of the parser.
🤖 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/fmt/olcrtc/OlcrtcFmt.kt` around lines 140 - 158, Add an `else` branch to the payload-key `when` inside the parser so any key other than `vp8-fps`, `vp8-batch`, and the supported client-id aliases throws an `IllegalArgumentException` with a clear unsupported-key message before `validateOlcrtcProfile()` runs.app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt (1)
312-329: 🩺 Stability & Availability | 🔵 TrivialNo permanent circuit breaker for olcRTC restarts.
With
GuardedProcessRestartPolicy()andshouldFailAfterProcessExitbypassing the uptime fast-fail whenever a policy is present, a persistently-broken olcRTC carrier config will restart forever (capped at a 30s delay) for the life of the VPN connection, with no total-attempt/circuit-breaker limit. Worth considering a max-attempt or max-duration cutoff to bound background retries on a permanently misconfigured profile, though this isn't a regression versus the other sidecars' pre-existing no-backoff restart-forever behavior.🤖 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 312 - 329, Bound olcRTC recovery retries by adding a circuit-breaker limit to the GuardedProcessRestartPolicy path in the enableOlcrtcRecovery branch. Track either total restart attempts or elapsed recovery duration and stop restarting once the configured limit is reached, while preserving the existing readiness wait and normal no-restart behavior when recovery is disabled.
🤖 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 344-367: Update pendingExternalPorts to check the pool’s
active/liveness state inside its polling loop, before probing ports and while
delaying between iterations, and return promptly when the pool becomes inactive.
Preserve the existing timeout and pending-port behavior for active pools, and
ensure the caller’s pool-inactive handling remains effective during superseded
starts.
---
Nitpick comments:
In `@app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt`:
- Around line 312-329: Bound olcRTC recovery retries by adding a circuit-breaker
limit to the GuardedProcessRestartPolicy path in the enableOlcrtcRecovery
branch. Track either total restart attempts or elapsed recovery duration and
stop restarting once the configured limit is reached, while preserving the
existing readiness wait and normal no-restart behavior when recovery is
disabled.
In `@app/src/main/java/io/nekohasekai/sagernet/fmt/olcrtc/OlcrtcFmt.kt`:
- Around line 140-158: Add an `else` branch to the payload-key `when` inside the
parser so any key other than `vp8-fps`, `vp8-batch`, and the supported client-id
aliases throws an `IllegalArgumentException` with a clear unsupported-key
message before `validateOlcrtcProfile()` runs.
🪄 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: 815710b8-879e-423c-b3c1-96fada2ff3cc
📒 Files selected for processing (18)
.depot/workflows/build-apk.yml.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.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/SidecarReadinessPolicy.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/TestInstance.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/olcrtc/OlcrtcFmt.ktapp/src/main/java/io/nekohasekai/sagernet/ui/profile/OlcrtcSettingsActivity.ktapp/src/test/java/io/nekohasekai/sagernet/bg/GuardedProcessRestartPolicyTest.ktapp/src/test/java/io/nekohasekai/sagernet/bg/proto/SidecarReadinessPolicyTest.ktapp/src/test/java/io/nekohasekai/sagernet/fmt/OlcrtcFmtTest.ktbuildScript/lib/olcrtc-src/main.gobuildScript/lib/olcrtc-src/main_test.gobuildScript/lib/olcrtc.sh
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
Review follow-upCommit
Review decisions:
Namespace CI run 29337125648 passed at the final SHA. CodeRabbit CLI reported no findings on the semantic changes. |
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
hawkff/olcrtc@ad5cc1e3d60b657b15ccd26f8db91395bf9630d0.Verification
23b292d846e4803959f4621f0dbeb9fca766f06e.Scope
This verifies Jitsi
datachannel. It does not claim VP8 or network-handoff coverage.Greptile Summary
This PR tightens olcRTC build, validation, and sidecar recovery behavior. The main changes are:
Confidence Score: 4/5
This is close, but the URL-test timeout fix should be corrected before merging.
sidecar listener not ready.app/src/main/java/io/nekohasekai/sagernet/bg/proto/SidecarReadinessPolicy.kt
Important Files Changed
Comments Outside Diff (1)
app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt, line 407-412 (link)When an olcRTC URL test takes more than 15 seconds to publish its readiness marker, this branch gives up even though the sidecar was started with the longer olcRTC readiness window. A slow carrier join that would succeed within the 60-second sidecar timeout is reported as
sidecar listener not ready, making URL tests fail incorrectly.Reviews (3): Last reviewed commit: "style: format readiness timeout helper" | Re-trigger Greptile