fix(service): make Windows scheduler stop actually stop the proxy - #780
Conversation
Fixes #764
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR validates proxy PIDs, hardens Windows scheduler and native-service transitions, prevents non-interactive WinSW prompts, and verifies proxy termination before reporting a successful service stop. ChangesWindows service lifecycle safeguards
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant stopWindows
participant findLiveProxy
participant stopProxy
stopWindows->>findLiveProxy: discover live proxy
findLiveProxy-->>stopWindows: identity-verified PID
stopWindows->>stopProxy: terminate verified proxy
stopWindows->>findLiveProxy: verify proxy termination
findLiveProxy-->>stopWindows: no live proxy or active proxy
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 085be23e08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
[shipping-github] StatusNot merge-ready yet. Required CI is green except |
Wait through the scheduler wrapper restart window when schtasks /end fails, and verify health-reported PIDs before stopProxy during service cleanup.
CI merges PR heads with dev; both sides added the proxy-liveness import, which broke typecheck with duplicate identifiers.
Restores cross-platform typecheck when both branches added the same proxy-liveness import.
…oundary Windows CI died with `panic(...): Internal assertion failure` and no `(fail)` line — four runs across `dev`, #771, #780 and #781. The crash header names the shape: `workers_spawned(9) workers_terminated(8)`, one worker never reclaimed, and it lands exactly at the `api-storage-policy` -> `api-storage` file boundary. That suite is the only one that spawns policy workers. `Worker.terminate()` returns void and does not wait for the thread to go away. Under `bun test --isolate` the harness reclaims a file's realm at the boundary, so a worker still exiting at that instant trips a Bun-internal assertion and takes the whole run down. macOS and Linux tolerate the same race, which is why it reads as a Windows-only flake. `worker-lifecycle.ts` tracks live workers and terminates them through the `close` event, so callers can await the thread actually being gone rather than a timer they hope is long enough; a 5s cap keeps a wedged worker from hanging teardown. Both storage jobs settle their run promise after that await, and the policy suite's `afterEach` now drains. devlog `_plan/260730_remote_issue_merge_round/150` reproduced this panic twice against an unchanged tree, confirmed the same tree passed in PR context, and left this defence as the follow-up if it recurred. It recurred. Evidence: 6190 pass / 0 fail across 445 files, typecheck clean, privacy scan green. The regression test asserts a worker was really spawned before checking it is gone, so it cannot pass vacuously.
Verdict: gated
|
…#764) `ocx service stop` reported success and restored native Codex while a proxy was still listening, leaving both pointing at each other. The cause is that `ops.stop()` reports the outcome of the STOP COMMAND, not of the process. A Windows scheduler task whose wrapper survives `schtasks /end` respawns its child a few seconds later, so a stop that returned success can still leave a live proxy. The tracked-pid cleanup does not catch it either: the respawned child writes a pid this process never recorded, and `stopTrackedProxyIfRunning` returns "none" the moment the pid file is absent. Stop is now verified instead of inferred. After the manager and the tracked proxy are stopped, the command polls for a live proxy across the observed restart window; if one answers, it reports the port, leaves native Codex alone, and exits non-zero. Restoring the config on top of a running proxy is the part that turns a failed stop into a broken installation, so that is the step that must not happen on an unverified stop. Deliberately different from PR #780, which waits only when `schtasks /end` returns an ERROR. The reported case is an /end that SUCCEEDS while the wrapper lives — on that path there is nothing for the command to report, so waiting on its exit status cannot help. That PR's other work (orphan discovery, PID verification, Microsoft-account preflight) is untouched here and still worth reviewing separately. Four tests on an injected clock, so no wall-clock sleeping. Ablation removing the restart-window polling in favour of a single probe: 1 pass / 3 fail, including the respawn case itself; restored 4/4. One test asserts a genuinely stopped proxy returns null — without it the check could report a survivor every time and strand native Codex unrestored, which is worse than the bug. Another asserts a throwing probe is not read as absence, since an unreachable health endpoint means unknown, not stopped. 87 pass across service, proxy-liveness and both tray suites. Windows behavior needs the matrix CI; the logic here is clock-injected and platform-neutral.
|
Half of what this PR addresses landed on Where the approaches differ. This PR waits ~6.5 seconds when A correction I had to make there, which is relevant if you revisit this: my first version ran that wait on every platform. Review caught it. launchd and systemd do not bring a child back after unload/stop, so every macOS and Linux user would have paid a Windows-specific delay on every stop — a worse everyday bug than the one being fixed. It is now scoped to supervisors that can actually respawn. Still open here and not duplicated: the orphan discovery, PID verification against the process table rather than trusting health-reported values, the Microsoft-account preflight, and the interactive WinSW prompting. Those are independent improvements; the One note if you rebase: several of the service checks are source-text assertions rather than behavior tests, so they pass whether or not the behavior holds. The stop verification that landed uses an injected clock and asserts probe counts, which may be a useful pattern for the PID work. #764 stays open for its |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/proxy-liveness.ts (1)
106-118: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThree copies of the same PID-safety gate across two files.
killablePidandverifiedReportedPidinsrc/server/proxy-liveness.ts, andverifiedKillTargetinsrc/service.ts, all implement the same logic: reject non-safe/non-positive PIDs, call a verifier, and require the verifier to echo back the exact candidate before treating it as killable. This is exactly the kind of security-sensitive check (gate on what can be terminated) that should live in one place so a future hardening (e.g., an extra check against process start-time) doesn't get applied to only one or two of the three copies.
src/server/proxy-liveness.ts#L106-L118: extract a singleverifiedSafePid(candidate, verifyPidFn)helper (e.g., export it fromsrc/config.tsnext toverifyPidIdentity) and havekillablePidandverifiedReportedPidboth call it.src/service.ts#L1694-L1698: replaceverifiedKillTarget's body with a call to the same shared helper, passingverifyPidIdentity.🤖 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 `@src/server/proxy-liveness.ts` around lines 106 - 118, Centralize the PID safety gate in a shared verifiedSafePid(candidate, verifyPidFn) helper, located with verifyPidIdentity in src/config.ts and exported for reuse. In src/server/proxy-liveness.ts lines 106-118, update killablePid and verifiedReportedPid to delegate to this helper; in src/service.ts lines 1694-1698, replace verifiedKillTarget’s duplicated body with the same helper using verifyPidIdentity.
🤖 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 `@src/service.ts`:
- Around line 2037-2038: Wrap the entire case "stop": body in its own block so
schedulerEndOk is block-scoped to that case. Preserve the existing stop-case
logic and behavior while ensuring the let declaration cannot leak across the
switch body.
- Around line 1443-1459: Update readWindowsPrincipalSource to replace
execFileSync with the repository’s Bun-native subprocess pattern, enforce a 5000
ms timeout, and await or otherwise handle its completion as required by that
API. Return the trimmed principal source on successful output, and return null
for timeouts, non-zero exits, missing PowerShell, or other probe failures.
In `@tests/proxy-liveness.test.ts`:
- Around line 148-149: Update the test invoking findLiveProxy for the
healthz-reported PID case to pass an explicit verifyPidFn that rejects the PID,
such as a function returning null. Keep the existing expectation and
configuration unchanged, matching the deterministic override pattern used by the
sibling tests.
---
Outside diff comments:
In `@src/server/proxy-liveness.ts`:
- Around line 106-118: Centralize the PID safety gate in a shared
verifiedSafePid(candidate, verifyPidFn) helper, located with verifyPidIdentity
in src/config.ts and exported for reuse. In src/server/proxy-liveness.ts lines
106-118, update killablePid and verifiedReportedPid to delegate to this helper;
in src/service.ts lines 1694-1698, replace verifiedKillTarget’s duplicated body
with the same helper using verifyPidIdentity.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da399858-474f-418a-ba5c-1e29d1ad5a26
📒 Files selected for processing (6)
src/lib/winsw.tssrc/server/proxy-liveness.tssrc/service.tstests/proxy-liveness.test.tstests/service.test.tstests/winsw.test.ts
| function readWindowsPrincipalSource(): string | null { | ||
| if (process.platform !== "win32") return null; | ||
| const ps = join(process.env.SystemRoot ?? "C:\\Windows", "System32", "WindowsPowerShell", "v1.0", "powershell.exe"); | ||
| if (!existsSync(ps)) return null; | ||
| try { | ||
| const out = execFileSync(ps, [ | ||
| "-NoLogo", | ||
| "-NoProfile", | ||
| "-NonInteractive", | ||
| "-Command", | ||
| "(Get-LocalUser -Name $env:USERNAME -ErrorAction SilentlyContinue).PrincipalSource", | ||
| ], { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"], windowsHide: true }).trim(); | ||
| return out || null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Inspect the implementation, its call sites, and the project runtime metadata.
printf '%s\n' '--- source context ---'
sed -n '1410,1475p' src/service.ts
printf '%s\n' '--- call sites ---'
rg -n -C 3 'readWindowsPrincipalSource|execFileSync' src/service.ts
printf '%s\n' '--- runtime metadata ---'
rg -n -i 'bun|engines|runtime' package.json bunfig.toml README.md .github 2>/dev/null | head -120Repository: lidge-jun/opencodex
Length of output: 14627
🌐 Web query:
Bun 1.3.14 execFileSync timeout option child_process documentation
💡 Result:
In Bun 1.3.14, the child_process.execFileSync method supports the timeout option within its options object [1][2][3]. When provided, the timeout option (specified in milliseconds) instructs the parent process to send the signal defined by the killSignal property (which defaults to SIGTERM) if the child process runs for longer than the specified duration [4][5]. While the API documentation indicates support for this option [1][2][3], users have reported issues in specific environments, such as on Windows, where execFileSync and spawnSync may hang despite a timeout being set [6]. In such cases, the process may not terminate as expected, leading to a hang rather than a timed-out error [6]. Bun development has been actively working on improving the compatibility and stability of child_process methods, including explicitly adding and refining support for timeout in synchronous child process methods in recent updates [7].
Citations:
- 1: https://bun.sh/reference/node/child_process/ExecSyncOptions/timeout
- 2: https://bun.sh/reference/node/child_process/ExecSyncOptions
- 3: https://bun.com/reference/node/child_process/ExecFileSyncOptions
- 4: https://bun.sh/reference/node/child_process
- 5: https://bun.sh/reference/node/child_process/execSync
- 6: child_process cannot spawn native Windows executables (Rust-compiled) on Windows — hangs / ETIMEDOUT oven-sh/bun#32011
- 7: Pass more node child process tests oven-sh/bun#16863
🏁 Script executed:
# Inspect the native-install call path and the declared Bun version.
printf '%s\n' '--- native install references ---'
rg -n -C 5 'installWindowsNative|assertWindowsNativeServiceAccountSupported|service install|--native' src/service.ts src/cli.ts src/cli 2>/dev/null | head -240
printf '%s\n' '--- package runtime declarations ---'
sed -n '20,70p' package.json
printf '%s\n' '--- local runtime availability ---'
command -v bun || true
bun --version 2>/dev/null || trueRepository: lidge-jun/opencodex
Length of output: 16543
🌐 Web query:
site:github.com/oven-sh/bun/issues/32011 execFileSync timeout Windows details
💡 Result:
In Node.js, the execFileSync method supports a timeout option which specifies, in milliseconds, how long a child process is allowed to run [1][2][3]. If the process exceeds this time, it is killed using the signal specified by killSignal (defaulting to SIGTERM) [1][2][3]. Key details regarding the timeout behavior on Windows include: 1. Process Exit Requirement: execFileSync will not return until the child process has completely exited, even after a timeout has occurred [1][3]. If the child process ignores the kill signal and continues running, the parent process will remain waiting until the child finally exits [1][4]. 2. Error Handling: If a process times out, execFileSync will throw an error [1][3]. This error object contains the full result of the underlying spawnSync operation [1][3]. 3. Known Behavioral Issues: In some runtime environments (such as Bun, which implements these child_process APIs), attempting to spawn certain native Windows executables directly may result in an ETIMEDOUT error or a hang, even with a timeout specified [5]. In these cases, wrapping the execution using shell: true or explicitly calling cmd /c is often required to achieve the expected behavior [5].
Citations:
- 1: https://nodejs.org/download/rc/v5.0.0-rc.1/docs/api/child_process.html
- 2: https://r2.nodejs.org/docs/v4.0.0/api/child_process.html
- 3: https://nodejs.org/download/release/v4.2.5/docs/api/child_process.html
- 4: https://r2.nodejs.org/docs/v0.11.14/api/child_process.html
- 5: child_process cannot spawn native Windows executables (Rust-compiled) on Windows — hangs / ETIMEDOUT oven-sh/bun#32011
🌐 Web query:
Bun 1.3.14 child_process execFileSync timeout Windows implementation issue
💡 Result:
In Bun 1.3.14, issues related to timeout behavior in child_process.execFileSync on Windows typically stem from how Bun's underlying subprocess implementation handles process spawning and timer management on the Windows platform [1][2]. Specific identified issues and behaviors include: 1. Stale Clock/Timer Issues: A known issue involves the synchronous spawn implementation (spawnSync, which execFileSync uses) relying on a cached per-VM libuv loop for the timeout timer [1]. If the loop's cached clock remains idle and goes stale, a timeout can trigger prematurely, resulting in an immediate termination of the child process, an exit code of null, and empty stdout/stderr [1]. 2. Spawning Native Executables: Users have reported that execFileSync and spawnSync may hang indefinitely—eventually throwing an ETIMEDOUT error—when attempting to spawn certain native Windows executables directly without a shell wrapper [2]. In these cases, using { shell: true } often serves as a workaround, as it bypasses the specific spawn path causing the hang [2]. 3. Resource Contention: Under high load, particularly in CI/CD environments on Windows (which lack native fork support and are more expensive for process creation), synchronous child process spawning can occasionally exceed expected time limits due to OS-level scheduler starvation or resource constraints (e.g., Windows Defender scanning) [3]. Bun implements node:child_process by wrapping its native Zig/Rust-based Bun.spawnSync primitives [4]. Developers encountering these issues are often advised to verify if the behavior persists when using { shell: true } (if applicable to their command) or to ensure that the environment is not oversubscribed during process-heavy tasks [2][3]. Fixes for these spawning and timing mechanisms are managed via the oven-sh/bun repository's issue tracker [1][5][6].
Citations:
- 1: Bun.spawnSync({ timeout }) returns exitCode: null with empty stdout/stderr for a real ssh.exe child on Windows — not reproduced with a synthetic closed-port case oven-sh/bun#33932
- 2: child_process cannot spawn native Windows executables (Rust-compiled) on Windows — hangs / ETIMEDOUT oven-sh/bun#32011
- 3: ci: windows test timeouts cluster on subprocess-spawning tests under 'bun --filter --parallel' coleam00/Archon#2306
- 4: https://deepwiki.com/oven-sh/bun/5.4-subprocess-management
- 5: execSync doesn't work on Windows oven-sh/bun#8588
- 6: unknown error in
execFileSyncfromnode:child_processoven-sh/bun#31710
🏁 Script executed:
# Check the exact dependency declaration and whether tests cover the Windows principal probe.
printf '%s\n' '--- package header and dependencies ---'
sed -n '1,75p' package.json
printf '%s\n' '--- principal probe tests and mocks ---'
rg -n -C 4 'PrincipalSource|MicrosoftAccount|assertWindowsNativeServiceAccountSupported|execFileSync|process.platform' tests src | head -260Repository: lidge-jun/opencodex
Length of output: 20208
🏁 Script executed:
# Inspect the repository's Bun-native Windows subprocess wrapper and its timeout behavior.
printf '%s\n' '--- Windows ACL subprocess implementation ---'
sed -n '70,125p' src/lib/windows-secret-acl.ts
printf '%s\n' '--- timeout-related subprocess patterns ---'
rg -n -C 5 'Bun\.spawnSync|timeoutMs|timeout:|ETIMEDOUT|execFileSync.*hang|hung' src tests | head -280Repository: lidge-jun/opencodex
Length of output: 20592
Bound the Windows principal probe and avoid execFileSync
At src/service.ts:1448-1454, add timeout: 5000 so ordinary PowerShell stalls use the existing null fallback. Bun can still hang in synchronous node:child_process Windows paths despite this option. Use the repository’s Bun-native subprocess pattern with an explicit timeout, and map timeout or non-zero exit to null.
🤖 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 `@src/service.ts` around lines 1443 - 1459, Update readWindowsPrincipalSource
to replace execFileSync with the repository’s Bun-native subprocess pattern,
enforce a 5000 ms timeout, and await or otherwise handle its completion as
required by that API. Return the trimmed principal source on successful output,
and return null for timeouts, non-zero exits, missing PowerShell, or other probe
failures.
Owner review: #764 is an /end that succeeds while the wrapper respawns, so waiting only when /end errors cannot catch it. Keep orphan/PID cleanup and the immediate live-proxy refuse-success guard; restart-window verification stays on the separate stop-verification path.
|
[shipping-github] Addressed owner feedback — removed the schtasks /end-error wait (wrong for the succeed-then-respawn #764 case); kept orphan discovery, PID verification, MS-account preflight, WinSW prompting, and the immediate live-proxy refuse-success guard. Restart-window polling remains on the separate stop-verification path. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/service.ts`:
- Around line 2044-2048: Update the service stop flow around
stopTrackedProxyForServiceCommand and findLiveProxy to poll for the complete
scheduler restart window after stopWindows(), rather than relying on
findLiveProxy’s single 1500ms health request. Keep the stop command unsuccessful
if a respawned proxy becomes live during that window, and add a regression test
covering delayed Windows-wrapper respawn.
In `@tests/service.test.ts`:
- Around line 675-682: The test “Windows scheduler stop does not wait on
schtasks /end failure” should execute serviceCommand("stop") instead of
inspecting source text. Mock the scheduler /end operation to fail and inject a
fake delay or clock, then assert that proxy cleanup runs and the stop command
completes without waiting; keep the regression test near the existing service
stop tests.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4f657f9-a6e1-446a-82b6-ab44596bfb59
📒 Files selected for processing (3)
src/service.tstests/proxy-liveness.test.tstests/service.test.ts
Resolve the service.ts conflict by retaining verifiedKillTarget/orphan cleanup from this PR and adopting proxyStillLiveAfterStop from tip for the succeed-then-respawn #764 path.
|
Merging this. Why it helps: finishes the remaining Windows service-stop hardening on top of tip — verified PID/orphan cleanup, Microsoft-account preflight, and interactive WinSW prompting — without re-landing the restart-window verification already on |
2 participants
Summary
Partial progress on #764 (scheduler stop hardening: orphan discovery, verified PID kills, MS-account preflight, WinSW prompting). Restart-window stop verification already landed separately on
dev.The
--nativeswitch half of #764 stays open — do not treat this as a full close.Test plan