fix(service): restart Windows service wrapper on unexpected bun termination - #5938
codingbooo wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (10)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Windows service wrapper now retries after child exits except for the protocol stay-out code. Service-context CLI live-owner paths return code 42 when the wrapper protocol is enabled. Tests and documentation describe the exit contract. ChangesWindows service restart flow
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Wrapper as Windows service wrapper
participant CLI as CLI child
CLI->>Wrapper: Exit with child status
alt status is the stay-out code
Wrapper->>Wrapper: Exit successfully
else any other status
Wrapper->>Wrapper: Wait five seconds and restart child
end
Merge Risk: ⚪ Minimal · up to The Windows service wrapper should restart the proxy after unexpected exits, including exit code 0. No identified issue currently prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new wrapper distinguishes an intentional decision not to start from an unexpected child exit, while retaining the existing service ownership checks. No material security regression was identified, but behavior and permissions of installed tasks could not be fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 7 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
리뷰 · 우선순위 64 / 80Windows 작업 스케줄러로 켜 둔 opencodex가, VPN이나 TUN 프로그램에 프록시가 죽어도 다시 살아나게 하는 수정입니다. 이슈 #5913입니다. 래퍼 배치 파일은 지금까지 종료 코드가 0이 아닐 때만 5초 뒤 다시 시작했습니다. 바깥 프로그램이 bun.exe를 죽이면 코드가 0으로 보이는 일이 있습니다. 래퍼는 정상 종료로 보고 멈췄고, 스케줄러도 성공으로 적어서 자동 재시작이 돌지 않았습니다. 사용자는 이 PR은 멈추는 코드를 하나 정합니다. 새 래퍼는 바탕은 라인 - 라인 - 라인 - 메인테이너의 판단이 필요한 지점 종료 코드 0을 사고로 볼지 정해 주세요. VPN에 죽은 프로세스를 살리는 데는 맞습니다. 정상 종료까지 5초 뒤 되살리는 것도 같은 규칙입니다. 이미 깔린 서비스는 머지만으로 바뀌지 않습니다. 업데이트가 repair로 cmd를 항상 다시 쓰는지, 사용자가 이 PR은 아직 초안입니다. 준비 체크리스트 네 칸이 비어 있습니다. 너의 추천 방향은 맞습니다. 닫거나 중복으로 볼 PR은 아닙니다. 머지 전에 Windows에서 164행 시험을 한 번 돌려 주세요. 0, 1, 42가 재시작과 정지로 갈라져야 합니다. 업그레이드 안내에는 이 댓글은 grok-bot이 작성했습니다 |
) Six focused fixes from the assigned bug batch remain as separate attributed commits. | PR | Change | Author | | --- | --- | --- | | #5969 | Preserve Meta Muse tool-choice semantics and reject unsupported selectors before dispatch. | shawnkim | | #5944 | Remove unsupported hosted web-search declarations for Xiaomi MiMo destinations. | codingbo | | #5938 | Restart the Windows service child after unexpected exits, including exit 0, while reserving the intentional stay-out code. | codingbo | | #5935 | Reject Claude message-thread state on translated routes so the client resends full history. | kaladinhonor | | #5939 | Rewrite standalone `\\0` escapes in Meta tool-schema patterns to equivalent `\\x00`. | boblob6969 | | #5951 | Preserve Kiro-reported credits across stream attempts and in the usage ledger. | codingbo | A separate integration commit keeps upstream-controlled Kiro event-type text out of opt-in debug logs. The Kiro stream retains the previously landed bounded HTTP-error text when combined with credit metering. Left out: #5977. Independent security review found that its local read capability authenticates the request but not the HTTP response. A substituted listener could return a shape-valid forged `protected` verdict. A correct server proof bound to the nonce, endpoint, and body is outside this batch. Both its source commit and status-validation follow-up were reverted in new commits; its test and layout entries are gone. The source PR remains open. Co-authored-by: shawnkim <shawnkim@markncompany.co.kr> Co-authored-by: codingbo <cnsdbo@163.com> Co-authored-by: kaladinhonor <266145786+kaladinhonor@users.noreply.github.com> Co-authored-by: boblob6969 <boblob6969@icloud.com>
Summary
Fixes #5913 by ensuring the Windows Task Scheduler batch wrapper (
opencodex-service.cmd) restarts the proxy child process after a 5s cooldown upon unexpected termination (including when killed with code 0 by external tools like TUN drivers/VPNs), while reserving a dedicated exit code (42) for intentional CLI "stay-out" decisions.Changes
src/service/windows-taskxml.ts, replaced the brittleif %ERRORLEVEL% NEQ 0check with an explicit check for the intentional stay-out exit code (42). Any other exit code (including 0 from external kills) logs the exit and loops back after a 5s cooldown (ping -n 6 127.0.0.1 >nul).src/service/windows-wrapper-exit.tsdefiningOCX_WINDOWS_WRAPPER_PROTOCOL=1and exit code42.handleStartandchooseListenPortinsrc/cli/index.tsnow return42only when running under an active service wrapper that advertises the protocol, preserving backward compatibility for legacy service installs.tests/windows/windows-service-wrappers.test.tsand CLI dispatch pins; updated documentation inlifecycle.mdanddocs-and-release.md.Verification
bun run typecheckpassed cleanly.bun test tests/service/service.test.ts tests/windows/passed (638 pass, 1 skip on non-Windows).bun test tests/cli/cli-dispatch.test.ts tests/cli/cli-ready.test.tspassed (106 pass).Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Required local validation passed; commands, results, and any full-suite exception are documented.
I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit