Skip to content

fix(windows): settle scheduler registration verification #868

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ventianima-lab wants to merge 1 commit into from
Closed

fix(windows): settle scheduler registration verification #868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion src/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,8 @@ type FinalizeHooks = {
deleteArgs: string[],
) => Promise<ElevatedSchtasksCreateAndRunResult>;
verify?: () => WindowsSchedulerInstallVerification;
/** Test-only replacement for the bounded post-registration settle delay. */
settleDelay?: (milliseconds: number) => Promise<void>;
writeInstallState?: () => void;
/** Preferred tri-state probe for security-sensitive reconciliation. */
probeTask?: () => WindowsSchedulerTaskProbe;
Expand Down Expand Up @@ -736,6 +738,36 @@ function attemptStillOwned(options: ApplyElevatedOptions): boolean {
return !check || check(options.attemptId);
}

const WINDOWS_SCHEDULER_VERIFICATION_SETTLE_DELAYS_MS = [50, 150, 300, 600] as const;

function schedulerVerificationMaySettle(
verification: WindowsSchedulerInstallVerification,
): boolean {
return verification.assetsHealthy
&& verification.nativeServiceAbsent
&& !verification.nativeStatusUnknown
&& !verification.conflict
&& (!verification.taskInstalled || !verification.registrationHealthy);
}

async function verifyWindowsSchedulerInstallAfterSettle(
options: ApplyElevatedOptions,
): Promise<WindowsSchedulerInstallVerification | null> {
const verify = finalizeHooks?.verify ?? verifyWindowsSchedulerInstall;
const delay = finalizeHooks?.settleDelay
?? ((milliseconds: number) => new Promise<void>(resolve => setTimeout(resolve, milliseconds)));
if (!attemptStillOwned(options)) return null;
let verification = verify();
for (const milliseconds of WINDOWS_SCHEDULER_VERIFICATION_SETTLE_DELAYS_MS) {
if (verification.ok || !schedulerVerificationMaySettle(verification)) break;
if (!attemptStillOwned(options)) return null;
await delay(milliseconds);
if (!attemptStillOwned(options)) return null;
verification = verify();
}
return attemptStillOwned(options) ? verification : null;
}

async function applyElevatedSchedulerResult(
result: ElevatedSchtasksCreateAndRunResult,
options: ApplyElevatedOptions,
Expand Down Expand Up @@ -765,7 +797,11 @@ async function applyElevatedSchedulerResult(
await reconcileUnknownElevatedOutcome(result.exitCode);
}

const verification = (finalizeHooks?.verify ?? verifyWindowsSchedulerInstall)();
// Task Scheduler can acknowledge elevated creation before the non-elevated
// query/XML view is coherent. Settle only that narrow visibility window;
// conflicts, missing assets, and unknown SCM state still fail immediately.
const verification = await verifyWindowsSchedulerInstallAfterSettle(options);
if (!verification) return;
if (!verification.ok) {
// Preserve a healthy elevated task when WinSW absence cannot be proven (unknown SCM status).
// Unknown is not a confirmed dual-backend conflict; install state is still withheld.
Expand All @@ -782,6 +818,7 @@ async function applyElevatedSchedulerResult(
"Installation state was not written.",
]);
}
if (!attemptStillOwned(options)) return;

@coderabbitai coderabbitai Bot Aug 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Does Bun's async/await and Promise microtask scheduling follow the same ordering guarantees as V8/Node.js (at least one microtask tick between an async function's internal return and the caller's await resuming)?

💡 Result:

Bun's async/await and Promise microtask scheduling is fundamentally driven by the JavaScriptCore (JSC) engine's microtask queue, which necessitates a microtask tick between an async function's await suspension and its resumption [1][2][3]. While Bun is designed to be highly compatible with Node.js, it does not guarantee identical microtask ordering guarantees in all edge cases because it must integrate Node.js-compatible concepts (like process.nextTick) into the JSC event loop model [3][4]. In Node.js (V8), the await keyword effectively wraps the awaited value in a promise and schedules the function's continuation as a microtask [1][5]. This ensures that the remainder of the async function runs after the current synchronous execution has completed and the microtask queue begins to drain [1][2][5]. Bun follows this core mechanism: await suspends the function, and resumption occurs via the microtask queue [2][3]. However, discrepancies exist in how Bun and Node.js manage the broader queue priority: 1. Queue Priority: In Node.js, process.nextTick callbacks are generally executed before microtasks (Promise reactions) [2][6]. Bun has made significant efforts to align with this behavior, including implementing specialized hooks to ensure process.nextTick and microtask ordering matches expectations in most common scenarios [7][8][3]. 2. Engine Differences: Bun uses JSC, while Node.js uses V8 [3]. V8 has specific optimizations for await (e.g., reducing the number of microtask ticks required for already-resolved promises) [1]. While Bun's JSC integration also manages microtasks, the underlying implementation and timing of "draining" these queues (often involving vm.drainMicrotasks() or specific event loop task wrappers) can differ from V8's internal task scheduling [9][10][11]. Developers should not rely on strict cross-runtime microtask ordering, as the exact sequence—especially involving the interaction between process.nextTick and promise reactions—can vary between V8 and JSC, and even across different versions of Bun [6][4]. While Bun aims for drop-in compatibility, it maintains a unique event loop architecture [3][4].

Citations:

🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- service.ts relevant region ---'
sed -n '740,835p' src/service.ts

printf '%s\n' '--- focused test region ---'
sed -n '300,485p' tests/windows-elevation-spawn.test.ts

printf '%s\n' '--- ownership and verification references ---'
rg -n -C 3 'attemptStillOwned|verifyWindowsSchedulerInstallAfterSettle|stillOwnsAttempt|rollbackElevatedSchedulerTask|writeInstallState' src/service.ts tests/windows-elevation-spawn.test.ts

Repository: lidge-jun/opencodex

Length of output: 29411

🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- available runtimes ---'
command -v bun || true
bun --version 2>/dev/null || true
command -v node || true
node --version 2>/dev/null || true

printf '%s\n' '--- async/await microtask ordering probe ---'
probe='
const events = [];
let owned = true;

async function verifyAfterSettle() {
  const verify = () => {
    events.push("verify");
    queueMicrotask(() => {
      owned = false;
      events.push("flip");
    });
    return { ok: false };
  };
  const value = verify();
  events.push(`inner-check:${owned}`);
  return owned ? value : null;
}

async function apply() {
  const verification = await verifyAfterSettle();
  events.push(`outer-resume:${owned}:${verification !== null}`);
  if (!verification || owned) return "no-rollback";
  events.push("rollback");
  return "rollback";
}

apply().then(result => {
  events.push(`result:${result}`);
  console.log(JSON.stringify(events));
});
'
if command -v bun >/dev/null 2>&1; then
  bun -e "$probe"
fi
if command -v node >/dev/null 2>&1; then
  node -e "$probe"
fi

printf '%s\n' '--- relevant test setup and later ownership test ---'
sed -n '1,120p' tests/windows-elevation-spawn.test.ts
sed -n '700,755p' tests/windows-elevation-spawn.test.ts

Repository: lidge-jun/opencodex

Length of output: 6815

🌐 Web query:

site:bun.sh/docs queueMicrotask Promise microtask await Bun JavaScriptCore

💡 Result:

In Bun, which is powered by the JavaScriptCore engine, the execution order of microtasks is consistent with standard ECMAScript specifications and the requirements of the Web-standard APIs it implements [1][2][3]. When working with queueMicrotask, Promise resolutions, and async/await, the following principles apply: 1. Microtask Queue: Both Promise callbacks (e.g., .then, .catch, .finally) and tasks queued via queueMicrotask() are placed into the microtask queue [2][4]. 2. Execution Order: Microtasks are executed after the currently executing script and before control is returned to the event loop [1]. When an await expression is encountered, the remainder of the async function is effectively scheduled as a microtask when the awaited promise resolves. 3. Bun and JavaScriptCore: Bun uses the Apple-developed JavaScriptCore engine [5][3]. Because Bun implements queueMicrotask() as a standard Web API, its behavior is designed to be fully compatible with environments where these mechanisms are expected [1][4]. The engine processes the microtask queue exhaustively before moving on to the next macrotask (such as timers, I/O callbacks, or DOM events). In practice, this means that if you have a mix of await expressions and queueMicrotask calls, they will all be processed in the order they were scheduled as part of the same microtask checkpoint [1][2]. There are no specialized deviations in Bun's handling of these primitives compared to other modern JavaScript runtimes [3][4].

Citations:

Document and test the post-verification ownership race.

At src/service.ts:821, queueMicrotask can change ownership after the check at line 768 but before the await at line 803 resumes. Keep this guard and add a comment explaining this microtask boundary. Add a focused test in tests/windows-elevation-spawn.test.ts that flips ownership from the final verify hook and asserts that rollback and writeInstallState are skipped.

Suggested comment 
+    // Ownership can change during the microtask that resumes this function
+    // after the await above, so keep this check before rollback.
     if (!attemptStillOwned(options)) return;
🤖 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` at line 821, Keep the attemptStillOwned guard in the
post-verification path, and add a concise comment explaining that queueMicrotask
may change ownership between the earlier check and the await resumption. Add a
focused test in windows-elevation-spawn.test.ts that changes ownership from the
final verify hook, then asserts rollback and writeInstallState are not invoked.

Source: Path instructions

const rollbackError = await rollbackElevatedSchedulerTask();
const parts = [
"Elevated Task Scheduler registration did not produce a conflict-free install.",
Expand Down
150 changes: 150 additions & 0 deletions tests/windows-elevation-spawn.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,156 @@ describe("finalizeWindowsSchedulerServiceRegistration", () => {
expect(parentRollbackLaunches).toBe(0);
});

test("settles a transient post-create registration view before writing install state", async () => {
let verifies = 0;
const delays: number[] = [];
setFinalizeWindowsSchedulerHooksForTests({
elevateCreateAndRun: async () => ({
outcome: "success",
exitCode: OCX_ELEVATED_SUCCESS,
stdout: "",
stderr: "",
}),
verify: () => {
verifies += 1;
if (verifies < 3) {
return {
taskInstalled: verifies === 2,
registrationHealthy: false,
assetsHealthy: true,
nativeServiceAbsent: true,
nativeStatusUnknown: false,
conflict: false,
ok: false,
detail: verifies === 1
? "Task Scheduler task is not installed."
: "Task Scheduler registration is present but unhealthy.",
};
}
return okVerify();
},
settleDelay: async milliseconds => { delays.push(milliseconds); },
writeInstallState: () => { writeCount += 1; },
});

await expect(finalizeWindowsSchedulerServiceRegistration()).resolves.toEqual({ kind: "done" });
expect(verifies).toBe(3);
expect(delays).toEqual([50, 150]);
expect(writeCount).toBe(1);
expect(parentRollbackLaunches).toBe(0);
});

test("does not settle a confirmed scheduler conflict", async () => {
let verifies = 0;
let delays = 0;
mockParentRollbackSpawn();
setFinalizeWindowsSchedulerHooksForTests({
elevateCreateAndRun: async () => ({
outcome: "success",
exitCode: OCX_ELEVATED_SUCCESS,
stdout: "",
stderr: "",
}),
verify: () => {
verifies += 1;
return {
taskInstalled: true,
registrationHealthy: true,
assetsHealthy: true,
nativeServiceAbsent: false,
nativeStatusUnknown: false,
conflict: true,
ok: false,
detail: "CONFLICT: Task Scheduler and native WinSW are both present.",
};
},
settleDelay: async () => { delays += 1; },
writeInstallState: () => { writeCount += 1; },
taskInstalled: () => false,
});

await expect(finalizeWindowsSchedulerServiceRegistration()).rejects.toThrow(/CONFLICT/);
expect(verifies).toBe(1);
expect(delays).toBe(0);
expect(writeCount).toBe(0);
expect(parentRollbackLaunches).toBe(1);
});

test("bounds settling when the registration remains unhealthy", async () => {
let verifies = 0;
const delays: number[] = [];
mockParentRollbackSpawn();
setFinalizeWindowsSchedulerHooksForTests({
elevateCreateAndRun: async () => ({
outcome: "success",
exitCode: OCX_ELEVATED_SUCCESS,
stdout: "",
stderr: "",
}),
verify: () => {
verifies += 1;
return {
taskInstalled: true,
registrationHealthy: false,
assetsHealthy: true,
nativeServiceAbsent: true,
nativeStatusUnknown: false,
conflict: false,
ok: false,
detail: "Task Scheduler registration is present but unhealthy.",
};
},
settleDelay: async milliseconds => { delays.push(milliseconds); },
writeInstallState: () => { writeCount += 1; },
taskInstalled: () => false,
});

await expect(finalizeWindowsSchedulerServiceRegistration()).rejects.toThrow(/present but unhealthy/);
expect(verifies).toBe(5);
expect(delays).toEqual([50, 150, 300, 600]);
expect(writeCount).toBe(0);
expect(parentRollbackLaunches).toBe(1);
});

test("stops settling without rollback or state write after attempt ownership is lost", async () => {
let verifies = 0;
let owned = true;
const delays: number[] = [];
setFinalizeWindowsSchedulerHooksForTests({
elevateCreateAndRun: async () => ({
outcome: "success",
exitCode: OCX_ELEVATED_SUCCESS,
stdout: "",
stderr: "",
}),
verify: () => {
verifies += 1;
return {
taskInstalled: false,
registrationHealthy: false,
assetsHealthy: true,
nativeServiceAbsent: true,
nativeStatusUnknown: false,
conflict: false,
ok: false,
detail: "Task Scheduler task is not installed.",
};
},
settleDelay: async milliseconds => {
delays.push(milliseconds);
owned = false;
},
stillOwnsAttempt: () => owned,
writeInstallState: () => { writeCount += 1; },
});

await expect(finalizeWindowsSchedulerServiceRegistration()).resolves.toEqual({ kind: "done" });
expect(verifies).toBe(1);
expect(delays).toEqual([50]);
expect(writeCount).toBe(0);
expect(parentRollbackLaunches).toBe(0);
});

test("create failure does not write install state or parent-rollback", async () => {
setFinalizeWindowsSchedulerHooksForTests({
elevateCreateAndRun: async () => {
Expand Down
Loading