Repository navigation
fix: honor vm deadlines after native SQLite writes - #207
Conversation
|
Hi! I'm the It looks like you correctly set up a CI job that uses the autofix.ci GitHub Action, but the autofix.ci GitHub App has not been installed for this repository. This means that autofix.ci unfortunately does not have the permissions to fix this pull request. If you are the repository owner, please install the app and then restart the CI workflow! 😃 |
autofix.ci
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 897b5f42cd
ℹ️ 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".
| function capture(_: any, _1?: any) {} | ||
|
|
||
| describe("elapsed deadlines with delayed VM trap delivery", () => { | ||
| const waitPastDeadline = () => Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 10); |
There was a problem hiding this comment.
All six new regressions use this fixed-duration Atomics.wait to advance past the VM timeout. This violates the explicit constraint in test/AGENTS.md: “never wait for time to pass in tests” and instead wait for the condition to be met. Replace the arbitrary 10 ms sleep with bounded monotonic-clock polling that establishes the deadline has elapsed while preserving the blocked timer-delivery setup.
AGENTS.md reference: test/AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
A
node:vmtimeout can miss a synchronous SQLite write when the native call returns before the timer queue delivers its callback. The elapsed deadline is cancelled withdidFire() == false, and the VM run returns successfully. Withiterate(), the cursor may also escape before the VM boundary can clean it up.Retain the absolute monotonic deadline in the VM run owner, poll it after
sqlite3_step()whileStatementRunstill owns cleanup, and check again before cancelling the timer. Re-post termination for already-fired deadlines too, covering the timer's publication-to-notification window. SQLite still completes its native step atomically.A test-only queue barrier deterministically delays timer delivery. Six new regressions cover a blocking native callback and SQLite all/values/get/run/iterate, checking timeout errors, cursor release and trigger/FTS consistency. The original SQLite test is unchanged; no timeout widening or retries.
The original symptom fell from 25 failing S7 processes (30 failed cases) among 272 attempts to zero assertion failures among 268 completed patched processes. Four of the 272 patched attempts hit the unchanged 60-second harness cutoff in the background cell; these remain inconclusive and are not counted as passes. All 188 before and 188 after Node controls passed, 564 timeout checks per side.
Each Bun process runs all ten original timeout cases. The diagnostic cell adds an outer timing wrapper; its 21 S7 normal returns exceeded the 1 ms limit (5.6–25.2 ms). Node 26.12.1 is the oracle for get/run/iterate. Node's iterate cursor cleanup differs independently; parity here concerns the timeout error.
A sampled slow background run spent all 718 main-thread samples inside SQLite commit/fsync. A separate control with no VM contexts or timeouts, the same SQL and background policy, and four CPU burners also reached the 60-second cutoff in all four processes: two S7 and two patched. Both patched controls stopped in the same trigger/FTS values step; S7 controls stopped later in schema creation. This independently reproduces the background native I/O/scheduling limit. It does not convert the four incomplete matrix attempts into passes. No timeout was widened and no failed or incomplete test attempt was rerun.
Final exact-head native CI 40167681456 passed on Linux (27 selected files / 29 rows) and macOS (21 selected files / 23 rows). PR CI 40167683873 passed on Linux (19 rows) and macOS (16 rows), including the full VM file on both. Six deterministic regressions passed locally on the verified final-head Mac artifact. All ten surrounding SQLite/VM files passed earlier with identical production sources: 1,100 tests, eight existing skips and 60 existing todos. Codex P2 review is scoped-clean for the runtime change and the import-order follow-up.
Exact head:
897b5f42cd3cf0a9cd481a6a77307dad155552d8. Exact-head native CI · PR CI.Upstream search found oven-sh#41907 for clock-source conversion, but no matching fix for cancellation of an elapsed, undelivered deadline. This patch changes no workflow or WebKit pin. Release-note and Node compatibility entries are included.