fix(windows): settle scheduler registration verification - #868
fix(windows): settle scheduler registration verification#868ventianima-lab wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughWindows Task Scheduler installation finalization now uses bounded, injectable verification retries. It stops immediately for non-transient failures and stops without mutation when attempt ownership is lost. Tests cover recovery, conflicts, exhaustion, rollback, and ownership loss. ChangesScheduler finalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ElevatedCreateRun
participant SchedulerVerification
participant InstallState
participant ParentRollback
ElevatedCreateRun->>SchedulerVerification: Reconcile scheduler registration
SchedulerVerification->>SchedulerVerification: Retry transient states with bounded delays
SchedulerVerification->>InstallState: Persist state after successful verification
SchedulerVerification->>ParentRollback: Roll back after non-retriable failure or exhaustion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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`:
- 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.
🪄 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: 2bc2d8c9-79b7-46f5-b3fc-94bff5ffc856
📒 Files selected for processing (2)
src/service.tstests/windows-elevation-spawn.test.ts
| "Installation state was not written.", | ||
| ]); | ||
| } | ||
| if (!attemptStillOwned(options)) return; |
There was a problem hiding this comment.
🗄️ 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:
- 1: https://www.nazarboyko.com/articles/how-async-await-actually-works-in-nodejs
- 2: https://sujeet.pro/articles/js-event-loop
- 3: https://deepwiki.com/oven-sh/bun/2.2-event-loop-and-async-operations
- 4:
setImmediateordering differs from Node oven-sh/bun#6069 - 5: https://jsprep.pro/javascript-event-loop-interview-questions
- 6: Event loop issue oven-sh/bun#8972
- 7: process: drain the microtask queue before nextTick callbacks queued from inside it oven-sh/bun#33088
- 8: Align
process.nextTickexecution order with Node oven-sh/bun#4409 - 9: node:vm
microtaskMode: 'afterEvaluate'doesn't seem to be respected oven-sh/bun#20145 - 10: https://github.com/oven-sh/bun/blob/50e478dc/src/bun.js/event_loop.zig
- 11: https://github.com/oven-sh/bun/blob/74e191b3/src/jsc/event_loop.rs
🏁 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.tsRepository: 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.tsRepository: 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:
- 1: https://bun.sh/docs/runtime/web-apis
- 2: https://bun.sh/docs/runtime/globals
- 3: https://bun.sh/docs
- 4: https://bun.sh/docs/runtime/nodejs-compat
- 5: https://bun.sh/docs/runtime
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
|
Thanks for tightening up the Windows scheduler verification path. Two remaining asks before this is ready:
happy to re-review quickly once both are in. |
2 participants
Summary
Verification
Summary by CodeRabbit
Bug Fixes
Tests