fix(windows): preflight scheduler registration before teardown - #1465
fix(windows): preflight scheduler registration before teardown#1465Ingwannu wants to merge 1 commit into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughFresh Windows service installation now stages and verifies Task Scheduler registration before stopping the proxy or changing service assets. UAC cancellation preserves the running setup. CLI installation no longer times out during the elevated transaction. ChangesWindows scheduler installation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/service.ts (1)
2975-2991: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe catch prefix now misreports registration failures as cleanup failures.
Line 2989 prints "❌ Service install cleanup failed: …". On the new path the most common failure is UAC denial during registration, which happens before any cleanup runs. The message tells the operator that cleanup broke, while the PR guarantees the opposite: nothing was cleaned up. Use a neutral prefix.
🐛 Proposed fix
- console.error(`❌ Service install cleanup failed: ${error instanceof Error ? error.message : String(error)}`); + console.error(`❌ Service install failed: ${error instanceof Error ? error.message : String(error)}`);🤖 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 2975 - 2991, Update the catch handler surrounding the service installation flow to replace the misleading “Service install cleanup failed” prefix with a neutral service installation failure message, while preserving the existing error-detail formatting, process.exitCode assignment, and loop control.
🤖 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/server/startup-action-control.ts`:
- Around line 167-179: Replace the zero Windows install timeout in
src/server/startup-action-control.ts lines 167-179 with an exported named
long-bound constant such as WINDOWS_ELEVATED_INSTALL_TIMEOUT_MS, and classify
timeout expiry through the existing indeterminate install state rather than
leaving installState running. Update
tests/startup-action-control-elevation.test.ts lines 81-86 to assert the
exported constant instead of literal 0 and add coverage verifying an expired
elevated install ends in indeterminate.
In `@src/service.ts`:
- Around line 2543-2570: Update installFreshWindowsSchedulerSafely to record the
owned config path and remove/re-verify any installed WinSW service before
registration, preferably by reusing or extracting the shared preparation helper
from installWindows. Ensure both fresh and existing installation paths perform
the native-service switch, while preserving the transactional ordering before
prepare() and task startup.
- Around line 1510-1513: Update buildWindowsSchtasksCreateArgs to delegate to
buildWindowsSchtasksCreateArgsForXml, passing its generated XML instead of
constructing the fixed argument array itself. Keep
buildWindowsSchtasksCreateArgsForXml as the single definition of the
scheduler-create argument shape.
In `@tests/service.test.ts`:
- Around line 765-791: Make the routing-preservation test assertion meaningful
by changing the prepare stub in installFreshWindowsSchedulerSafely to delete
routingPath when invoked, while retaining the existing routingBefore setup and
final file-content assertion. Keep the calls assertion verifying prepare is not
reached, so the test observes both ordering and filesystem preservation.
---
Outside diff comments:
In `@src/service.ts`:
- Around line 2975-2991: Update the catch handler surrounding the service
installation flow to replace the misleading “Service install cleanup failed”
prefix with a neutral service installation failure message, while preserving the
existing error-detail formatting, process.exitCode assignment, and loop control.
🪄 Autofix
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: c5978a0c-fc81-421b-9e0d-680bb6d0d48e
📒 Files selected for processing (6)
docs-site/src/content/docs/reference/cli/lifecycle.mdsrc/server/startup-action-control.tssrc/service.tsstructure/05_gui-and-management-api.mdtests/service.test.tstests/startup-action-control-elevation.test.ts
| const timeout = process.platform === "win32" && action === "install-service" | ||
| ? 0 | ||
| : 60_000; | ||
| execFile(bun, argv, { | ||
| encoding: "utf8", | ||
| env: process.env, | ||
| timeout: 60_000, | ||
| // A fresh Windows scheduler install now owns its UAC prompt inside this CLI | ||
| // transaction. Killing only the CLI at 60s can orphan its elevated schtasks child, | ||
| // which may register the task after the Dashboard has reported failure. Keep the | ||
| // async request/attempt lock alive until Windows returns approval or cancellation. | ||
| timeout, | ||
| windowsHide: true, | ||
| maxBuffer: 256 * 1024, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The disabled timeout has no upper bound, and the test pins that value. Both sites follow from one decision: replacing the 60-second bound with 0 rather than with a longer bound. An unanswered UAC prompt therefore leaves installState in running forever and blocks every later install attempt, and the test locks the literal 0 in place so the source cannot be corrected without touching it.
src/server/startup-action-control.ts#L167-L179: replace0with a named long bound, for exampleWINDOWS_ELEVATED_INSTALL_TIMEOUT_MS, and classify its expiry as the existingindeterminatestate defined at lines 27-34.tests/startup-action-control-elevation.test.ts#L81-L86: assert against the exported bound constant instead of the literal0, and add a case that an expired elevated install ends inindeterminaterather thanrunning.
📍 Affects 2 files
src/server/startup-action-control.ts#L167-L179(this comment)tests/startup-action-control-elevation.test.ts#L81-L86
🤖 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/startup-action-control.ts` around lines 167 - 179, Replace the
zero Windows install timeout in src/server/startup-action-control.ts lines
167-179 with an exported named long-bound constant such as
WINDOWS_ELEVATED_INSTALL_TIMEOUT_MS, and classify timeout expiry through the
existing indeterminate install state rather than leaving installState running.
Update tests/startup-action-control-elevation.test.ts lines 81-86 to assert the
exported constant instead of literal 0 and add coverage verifying an expired
elevated install ends in indeterminate.
| /** Build the fixed scheduler-create command from an explicit staged XML document. */ | ||
| export function buildWindowsSchtasksCreateArgsForXml(xml: string): string[] { | ||
| return ["/create", "/tn", TASK, "/xml", xml, "/f"]; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Let buildWindowsSchtasksCreateArgs delegate to the new helper.
Lines 1505-1508 and lines 1511-1513 now build the same argument array. Two copies of the fixed create shape can drift, and the security argument in the code comments depends on the shape staying identical everywhere. Keep one definition.
♻️ Proposed refactor
export function buildWindowsSchtasksCreateArgs(script = windowsServiceScriptPath()): string[] {
const xml = script === windowsServiceScriptPath() ? windowsTaskXmlPath() : `${script}.xml`;
- return ["/create", "/tn", TASK, "/xml", xml, "/f"];
+ return buildWindowsSchtasksCreateArgsForXml(xml);
}Move buildWindowsSchtasksCreateArgsForXml above buildWindowsSchtasksCreateArgs or rely on function hoisting.
🤖 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 1510 - 1513, Update
buildWindowsSchtasksCreateArgs to delegate to
buildWindowsSchtasksCreateArgsForXml, passing its generated XML instead of
constructing the fixed argument array itself. Keep
buildWindowsSchtasksCreateArgsForXml as the single definition of the
scheduler-create argument shape.
| export async function installFreshWindowsSchedulerSafely( | ||
| deps: FreshWindowsSchedulerInstallDeps = {}, | ||
| ): Promise<void> { | ||
| const stage = deps.stageRegistrationXml ?? stageWindowsSchedulerRegistrationXml; | ||
| const register = deps.register ?? registerFreshWindowsSchedulerTask; | ||
| const prepare = deps.prepare ?? (() => prepareServiceInstall("scheduler")); | ||
| const publishAssets = deps.publishAssets ?? writeWindowsSchedulerAssets; | ||
| const runTask = deps.runTask ?? startWindows; | ||
| const writeState = deps.writeState ?? (() => writeServiceInstallState("scheduler")); | ||
| const rollbackTask = deps.rollbackTask ?? (() => rollbackElevatedSchedulerTask(TASK)); | ||
| const removeStagedXml = deps.removeStagedXml ?? ((path: string) => { | ||
| if (existsSync(path)) unlinkSync(path); | ||
| }); | ||
|
|
||
| let stagedXml: string | null = null; | ||
| let registered = false; | ||
| let started = false; | ||
| try { | ||
| stagedXml = stage(); | ||
| await register(stagedXml); | ||
| registered = true; | ||
|
|
||
| // The destructive boundary begins only after Task Scheduler accepted the definition. | ||
| await prepare(); | ||
| publishAssets(); | ||
| runTask(); | ||
| started = true; | ||
| writeState(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
The fresh path skips the native (WinSW) backend switch and the owned-path record.
installWindows() at lines 1910-1932 does three things before it creates the task:
recordOwnedConfigPath(getConfigDir(), serviceStatePath())(line 1911).- Removal and re-verification of an installed WinSW service (lines 1914-1924).
- Asset write, create, run, state write.
The new dispatch at lines 2980-2981 calls installFreshWindowsSchedulerSafely() instead of ops.install, so steps 1 and 2 no longer run. Failure mode: a machine that has the native WinSW service installed and no scheduler task takes the fresh path. The installer then registers and starts a scheduler task while the WinSW service is still registered. Two live managers respawn the proxy, which is exactly the conflict the comment on lines 1912-1913 exists to prevent. The install-state ownership record is also lost.
Add the WinSW switch and the ownership record to the transactional path, before prepare() and ideally before registration, or call the shared preparation helper that performs them.
🐛 Sketch of the missing phase
let stagedXml: string | null = null;
let registered = false;
let started = false;
try {
+ recordOwnedConfigPath(getConfigDir(), serviceStatePath());
+ // Two live managers would both respawn the proxy; remove the native backend
+ // before the scheduler backend is registered.
+ removeNativeServiceBeforeSchedulerSwitch();
stagedXml = stage();
await register(stagedXml);
registered = true;Expose the WinSW removal block of installWindows() as removeNativeServiceBeforeSchedulerSwitch() and call it from both paths.
#!/bin/bash
# Check whether prepareServiceInstall already removes the WinSW service and records the owned path.
ast-grep run --pattern 'export async function prepareServiceInstall($$$) { $$$ }' --lang typescript src/service.ts
rg -nP -C 5 'uninstallWinswService|statusWinswRaw|recordOwnedConfigPath' --type=ts src🤖 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 2543 - 2570, Update
installFreshWindowsSchedulerSafely to record the owned config path and
remove/re-verify any installed WinSW service before registration, preferably by
reusing or extracting the shared preparation helper from installWindows. Ensure
both fresh and existing installation paths perform the native-service switch,
while preserving the transactional ordering before prepare() and task startup.
| test("UAC cancellation removes only staged XML and never enters cleanup or asset publication", async () => { | ||
| const calls: string[] = []; | ||
| mkdirSync(TEST_DIR, { recursive: true }); | ||
| const routingPath = join(TEST_DIR, "config.toml"); | ||
| const routingBefore = 'openai_base_url = "http://127.0.0.1:10100/v1"\nmodel_catalog_json = "keep.json"\n'; | ||
| writeFileSync(routingPath, routingBefore, "utf8"); | ||
| await expect(installFreshWindowsSchedulerSafely({ | ||
| stageRegistrationXml: () => { calls.push("stage"); return "attempt.xml"; }, | ||
| register: async path => { | ||
| calls.push(`register:${path}`); | ||
| throw new Error("UAC prompt was cancelled"); | ||
| }, | ||
| prepare: async () => { calls.push("prepare"); }, | ||
| publishAssets: () => { calls.push("publish-assets"); }, | ||
| runTask: () => { calls.push("run-task"); }, | ||
| writeState: () => { calls.push("write-state"); }, | ||
| rollbackTask: async () => { calls.push("rollback-task"); return null; }, | ||
| removeStagedXml: path => { calls.push(`remove:${path}`); }, | ||
| })).rejects.toThrow("UAC prompt was cancelled"); | ||
|
|
||
| expect(calls).toEqual([ | ||
| "stage", | ||
| "register:attempt.xml", | ||
| "remove:attempt.xml", | ||
| ]); | ||
| expect(readFileSync(routingPath, "utf8")).toBe(routingBefore); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The routing-preservation assertion cannot fail.
Lines 768-770 write config.toml, and line 790 asserts it is unchanged. Every destructive dependency is stubbed in this call: prepare on line 777 only pushes a string and never touches routing. The file therefore cannot change, no matter what installFreshWindowsSchedulerSafely does. The real guarantee is already proved by the calls assertion on lines 785-789, which shows prepare was never invoked.
Either delete the file setup and the line 790 assertion, or make prepare a stub that actually deletes routingPath so the ordering guarantee is observable through the filesystem.
♻️ Option: make the stub destructive so the assertion has meaning
- prepare: async () => { calls.push("prepare"); },
+ prepare: async () => { calls.push("prepare"); rmSync(routingPath, { force: true }); },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("UAC cancellation removes only staged XML and never enters cleanup or asset publication", async () => { | |
| const calls: string[] = []; | |
| mkdirSync(TEST_DIR, { recursive: true }); | |
| const routingPath = join(TEST_DIR, "config.toml"); | |
| const routingBefore = 'openai_base_url = "http://127.0.0.1:10100/v1"\nmodel_catalog_json = "keep.json"\n'; | |
| writeFileSync(routingPath, routingBefore, "utf8"); | |
| await expect(installFreshWindowsSchedulerSafely({ | |
| stageRegistrationXml: () => { calls.push("stage"); return "attempt.xml"; }, | |
| register: async path => { | |
| calls.push(`register:${path}`); | |
| throw new Error("UAC prompt was cancelled"); | |
| }, | |
| prepare: async () => { calls.push("prepare"); }, | |
| publishAssets: () => { calls.push("publish-assets"); }, | |
| runTask: () => { calls.push("run-task"); }, | |
| writeState: () => { calls.push("write-state"); }, | |
| rollbackTask: async () => { calls.push("rollback-task"); return null; }, | |
| removeStagedXml: path => { calls.push(`remove:${path}`); }, | |
| })).rejects.toThrow("UAC prompt was cancelled"); | |
| expect(calls).toEqual([ | |
| "stage", | |
| "register:attempt.xml", | |
| "remove:attempt.xml", | |
| ]); | |
| expect(readFileSync(routingPath, "utf8")).toBe(routingBefore); | |
| }); | |
| test("UAC cancellation removes only staged XML and never enters cleanup or asset publication", async () => { | |
| const calls: string[] = []; | |
| mkdirSync(TEST_DIR, { recursive: true }); | |
| const routingPath = join(TEST_DIR, "config.toml"); | |
| const routingBefore = 'openai_base_url = "http://127.0.0.1:10100/v1"\nmodel_catalog_json = "keep.json"\n'; | |
| writeFileSync(routingPath, routingBefore, "utf8"); | |
| await expect(installFreshWindowsSchedulerSafely({ | |
| stageRegistrationXml: () => { calls.push("stage"); return "attempt.xml"; }, | |
| register: async path => { | |
| calls.push(`register:${path}`); | |
| throw new Error("UAC prompt was cancelled"); | |
| }, | |
| prepare: async () => { calls.push("prepare"); rmSync(routingPath, { force: true }); }, | |
| publishAssets: () => { calls.push("publish-assets"); }, | |
| runTask: () => { calls.push("run-task"); }, | |
| writeState: () => { calls.push("write-state"); }, | |
| rollbackTask: async () => { calls.push("rollback-task"); return null; }, | |
| removeStagedXml: path => { calls.push(`remove:${path}`); }, | |
| })).rejects.toThrow("UAC prompt was cancelled"); | |
| expect(calls).toEqual([ | |
| "stage", | |
| "register:attempt.xml", | |
| "remove:attempt.xml", | |
| ]); | |
| expect(readFileSync(routingPath, "utf8")).toBe(routingBefore); | |
| }); |
🤖 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 `@tests/service.test.ts` around lines 765 - 791, Make the routing-preservation
test assertion meaningful by changing the prepare stub in
installFreshWindowsSchedulerSafely to delete routingPath when invoked, while
retaining the existing routingBefore setup and final file-content assertion.
Keep the calls assertion verifying prepare is not reached, so the test observes
both ordering and filesystem preservation.
Inventory all seven bug-labelled issue/PR pairs against dev 87e3ff9, record each reviewer blocker and red-CI root cause, and write one decade doc per pair. Audit findings beyond the filed reviews: - #1462 (no review yet) still resurrects a deleted provider or custom model when the stale process also edited that row: a persisted MISSING_CONFIG_VALUE is not a plain record, so src/config.ts:2884-2896 skips recursion and live wins. Probed at the exact head, both entity types came back. - #1465 (no review yet) gets its two-phase ordering right, but rollback deletes the scheduler task by name without proving this attempt created it, so a concurrent registration can be destroyed by our cleanup. - #1441's three blockers are already addressed on current head aa256f6; the reviewer judged the older, replaced head a8087d3. - #1461 and #1441 red CI is infrastructure noise (Bun 1.3.14 segfault and epoll_ctl EEXIST, plus the #1302 shard hang), not a contributor regression. Refs #1459 #1453 #1273 #1454 #1449 #1439 #1429
|
Landed on On The two-phase transaction audits clean: phase 1 stages and registers only, and nothing in that path stops a manager or the tracked proxy ( The follow-up closes a gap in the cleanup claim. Rollback deleted the scheduler task by name, and the absence probe is not atomic with the fixed-name Stated honestly: the query and the elevated delete are still not one atomic operation, so a replacement registered in that window can be deleted. Closing that needs an attempt-unique task name or an elevated attempt-bound transaction, which changes how the service registers on Windows and needs a real Windows host to validate — Verified: |
2 participants
Summary
This fixes the real ordering regression reported in #1454: cancelling or denying UAC during a fresh scheduler install is now a pre-commit failure and leaves the working proxy/routing untouched.
Closes #1454
Verification
origin/devat87e3ff9f6.taskset -c 0-1 nice -n 10 bun testacross 18 Windows service/UAC and adjacent CLI files: 406 pass, 0 fail.taskset -c 0-1 nice -n 10 bun run typecheck: passed.taskset -c 0-1 nice -n 10 bun run privacy:scan: passed.cd docs-site && bun install --frozen-lockfile: no changes.cd docs-site && taskset -c 0-1 nice -n 10 bun run build: 221 pages built.Codex autostart shim > Unix shim exports persisted service API token before running Codex, reproduces on cleandevand is unrelated to this Windows scheduler change. After rebasing, the exact-head focused suite, typecheck, privacy scan, and docs build above were rerun successfully.Checklist
Because this changes Windows elevation and service lifecycle boundaries, please keep the PR in draft until an independent maintainer completes the required security review and Windows smoke test.
Summary by CodeRabbit