Skip to content

fix(service): restart Windows service wrapper on unexpected bun termination - #5938

Closed
codingbooo wants to merge 1 commit into
lidge-jun:devfrom
codingbooo:fix/issue-5913-windows-service-restart
Closed

codingbooo wants to merge 1 commit into
lidge-jun:devfrom
codingbooo:fix/issue-5913-windows-service-restart

Conversation

@codingbooo

@codingbooo codingbooo commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Windows wrapper exit retry: In src/service/windows-taskxml.ts, replaced the brittle if %ERRORLEVEL% NEQ 0 check 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).
  • Wrapper protocol coordination: Added src/service/windows-wrapper-exit.ts defining OCX_WINDOWS_WRAPPER_PROTOCOL=1 and exit code 42. handleStart and chooseListenPort in src/cli/index.ts now return 42 only when running under an active service wrapper that advertises the protocol, preserving backward compatibility for legacy service installs.
  • Tests & Docs: Added unit tests in tests/windows/windows-service-wrappers.test.ts and CLI dispatch pins; updated documentation in lifecycle.md and docs-and-release.md.

Verification

  • bun run typecheck passed 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.ts passed (106 pass).

Checklist

  • I have tested my changes locally.
  • I have updated relevant documentation / tests.
  • I have followed the project's code style and contributing guidelines.

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

  • Bug Fixes
    • Windows scheduled services now restart the proxy after it exits, including after a clean exit. They stop restarting when another proxy already owns the port or when the service is explicitly stopped.
    • Added guidance to stop the service and refresh its wrapper after an upgrade.

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7c257d6a-9fc0-43d5-bf97-67a3d96690d2

📥 Commits

Reviewing files that changed from the base of the PR and between bb3f3c2 and fa3a131.

📒 Files selected for processing (10)
  • docs-site/src/content/docs/reference/cli/lifecycle.md
  • src/cli/dispatch.ts
  • src/cli/index.ts
  • src/service/windows-taskxml.ts
  • src/service/windows-wrapper-exit.ts
  • structure/ops/docs-and-release.md
  • structure/runtime.md
  • tests/cli/cli-dispatch.test.ts
  • tests/cli/cli-ready.test.ts
  • tests/windows/windows-service-wrappers.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Windows service restart flow

Layer / File(s) Summary
Service stay-out exit contract
src/service/windows-wrapper-exit.ts, src/cli/index.ts:2, 334-336, 447-449, 512, src/cli/dispatch.ts:1062, tests/cli/cli-dispatch.test.ts:439-440, tests/cli/cli-ready.test.ts:847-864, structure/runtime.md:201-202
The helper returns code 42 when both service and wrapper-protocol environment values are "1". Three CLI live-owner paths use that result. CLI assertions and runtime documentation reflect the stay-out behavior.
Windows wrapper retry flow
src/service/windows-taskxml.ts:1, 68-74, 116-125, tests/windows/windows-service-wrappers.test.ts:129-186, docs-site/src/content/docs/reference/cli/lifecycle.md:403-408, structure/ops/docs-and-release.md:183-190
The generated wrapper retries child exits after a five-second delay unless the child returns code 42, which ends the wrapper successfully. Windows tests check the exit cases. Documentation describes the restart and stop behavior.

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
Loading

Merge Risk: ⚪ Minimal · up to fa3a1

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 Review

Security architecture risk: 🔵 Low · up to fa3a1

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed behavior affects the lifetime of the scheduled CLI child and therefore the availability of the proxy it runs. It does not, on the examined paths, add a network entrypoint or change the generated task’s principal.

Trust Boundaries and Controls

  • inferred — The environment marker coordinates wrapper and child behavior; it is not an authentication check. A child exit of 42 ends the wrapper regardless of why the child returned it. No evidence establishes that a less-privileged actor can control that exit or modify the installed child, and installed-task and file permissions remain unverified.

Resilience and Maintainability Implications

  • observed — On a pre-bind live-owner stay-out, startup raises its exit before publishing ownership state; the ownership-publication path releases its lease on that exit. The stop path separately checks for a proxy that survives or respawns after scheduler shutdown.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR meets the coding objective in issue #5913. src/service/windows-taskxml.ts now treats only WINDOWS_WRAPPER_STAY_OUT_EXIT_CODE (42) as an intentional stay-out result. It logs other child exit…
Out of Scope Changes check ✅ Passed The changed files stay within issue #5913. The source changes implement the Windows wrapper restart protocol. The CLI changes provide the intentional stay-out signal required by that protocol. The Win…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: restarting the Windows service wrapper after unexpected Bun termination. It is concise and specific.
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions

github-actions Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ 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.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

@github-actions
github-actions Bot marked this pull request as draft September 26, 2026 12:59
@github-actions github-actions Bot added the bug Something isn't working label Sep 26, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 64 / 80

Windows 작업 스케줄러로 켜 둔 opencodex가, VPN이나 TUN 프로그램에 프록시가 죽어도 다시 살아나게 하는 수정입니다. 이슈 #5913입니다.

래퍼 배치 파일은 지금까지 종료 코드가 0이 아닐 때만 5초 뒤 다시 시작했습니다. 바깥 프로그램이 bun.exe를 죽이면 코드가 0으로 보이는 일이 있습니다. 래퍼는 정상 종료로 보고 멈췄고, 스케줄러도 성공으로 적어서 자동 재시작이 돌지 않았습니다. 사용자는 ocx stop 다음에 ocx start를 해야 했습니다.

이 PR은 멈추는 코드를 하나 정합니다. 새 래퍼는 OCX_WINDOWS_WRAPPER_PROTOCOL=1을 넣습니다. 자식이 42로 끝나면 "이미 다른 프록시가 포트를 쓰고 있으니 일부러 빠진다"는 뜻입니다. 래퍼는 그때만 끝냅니다. 0을 포함한 다른 코드는 로그를 남기고 5초 쉬었다가 다시 시작합니다. 옛 래퍼에는 이 변수가 없습니다. 그때 CLI는 예전처럼 0으로 끝납니다. 그래서 이미 설치된 PC는 ocx service repair로 래퍼 파일을 다시 만들어야 이 동작이 켜집니다. Bun이나 CLI 파일이 없을 때의 종료 코드 3은 루프 밖에서 그대로 끝납니다.

바탕은 dev입니다. types.ts와 config.ts를 나누는 변경은 아닙니다. 같은 이슈를 고치는 다른 열린 PR은 없습니다.

라인 - tests/windows/windows-service-wrappers.test.ts 164행. 실제 cmd.exe로 0은 재시작, 42는 정지를 보는 시험은 Windows가 아니면 건너뜁니다. PR 본문도 그 1건을 skip했다고 적습니다. 130행 시험은 만들어진 배치 문장만 비교합니다. set "ERRORLEVEL="(src/service/windows-taskxml.ts 69행)이 환경변수에 가려진 ERRORLEVEL을 지우는지도 그 skip된 시험만 봅니다.

라인 - src/service/windows-taskxml.ts 117행, src/cli/index.ts 655행. 42가 아니면 0도 5초 뒤 재시작입니다. 서비스 안 프록시가 신호를 받고 마무리가 되면 655행에서 0으로 끝납니다. 예전 래퍼는 그 0에서 스스로 멈췄습니다. 이제는 ocx service stop이 래퍼 프로세스를 죽여야 멈춥니다. 그 죽임이 늦거나 실패하면, 깨끗이 꺼진 프록시가 다시 뜹니다.

라인 - structure/runtime.md 201행. OCX_SERVICE=1이면 Windows 래퍼 규약을 쓴다고 적혀 있습니다. 42는 OCX_SERVICE와 OCX_WINDOWS_WRAPPER_PROTOCOL이 둘 다 1일 때만 나갑니다. macOS, Linux, 그리고 아직 repair하지 않은 Windows는 그대로 0입니다.

메인테이너의 판단이 필요한 지점

종료 코드 0을 사고로 볼지 정해 주세요. VPN에 죽은 프로세스를 살리는 데는 맞습니다. 정상 종료까지 5초 뒤 되살리는 것도 같은 규칙입니다.

이미 깔린 서비스는 머지만으로 바뀌지 않습니다. 업데이트가 repair로 cmd를 항상 다시 쓰는지, 사용자가 ocx service repair를 해야 하는지 릴리스에 적을지 정해 주세요.

이 PR은 아직 초안입니다. 준비 체크리스트 네 칸이 비어 있습니다.

너의 추천

방향은 맞습니다. 닫거나 중복으로 볼 PR은 아닙니다. 머지 전에 Windows에서 164행 시험을 한 번 돌려 주세요. 0, 1, 42가 재시작과 정지로 갈라져야 합니다. 업그레이드 안내에는 ocx service repair를 적는 쪽이 안전합니다. structure/runtime.md 201행은 프로토콜 변수가 켜진 Windows 래퍼만 42로 끝나게 고치면 됩니다. 체크리스트를 채운 뒤에 초안을 푸세요.

이 댓글은 grok-bot이 작성했습니다

@codingbooo
codingbooo marked this pull request as ready for review September 26, 2026 13:27
@github-actions
github-actions Bot marked this pull request as draft September 26, 2026 13:27
@codingbooo
codingbooo marked this pull request as ready for review September 26, 2026 13:28
lidge-jun added a commit that referenced this pull request Sep 26, 2026
)

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>
@lidge-jun

Copy link
Copy Markdown
Owner

Thanks! This landed on dev through bug-PR merge train batch 9B, #5985 (merge bf04176). Your change is one commit on dev with you as the author and a Co-authored-by trailer. The cmd wrapper regression ran and passed on a hosted Windows shard. Closing since the content is now on dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants