Skip to content

fix(wasm): replace a failed instance synchronously so the next call never runs on a broken one - #189

Open
pyramation wants to merge 3 commits into
mainfrom
fix/wasm-sync-recovery
Open

pyramation wants to merge 3 commits into
mainfrom
fix/wasm-sync-recovery

Conversation

@pyramation

@pyramation pyramation commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

When a WASM call throws, callWasm() used to mark the instance stale and load a fresh one asynchronously. Sync callers in the same tick kept using the broken instance, and after 10 failures they got WASM module not initialized. Call loadModule() first. Both behaviours give errors that don't describe the real cause:

// (a) 11th sync call in one tick
for (let i = 0; i < 60; i++) { try { parseSync(DEEP) } catch {} ; parseSync('SELECT 1') }
// main: RangeError ×10, then "WASM module not initialized. Call loadModule() first." for every call
// (b) after the 1 GiB limit is hit, the stale instance's heap is full or corrupt
parseSync('SELECT 1;\n'.repeat(5_000_000))   // libpg_query terminated (out of WASM memory) — correct
parseSync('SELECT 1;\n'.repeat(1_000_000))   // main: "Failed to allocate memory for query" (fits fine in a fresh instance)

In fuzzing, stale-instance sync calls also produced RuntimeError: table index is out of bounds and Aborted(Assertion failed: <garbage>).

Change (all majors, via templates/):

  • On first load, the wrapper records the .wasm URL through the loader's locateFile and keeps a compiled WebAssembly.Module. The second compile is a V8 cache hit, about 4 ms. It reads the file the same way the Emscripten loader does: await import('node:fs') in Node, fetch elsewhere.
  • callWasm() catch → replaceInstance(). This calls PgQueryModule({ instantiateWasm }), whose hook runs receive(new WebAssembly.Instance(compiled, imports)) synchronously. Exports and memory views are set inside receive, so the new instance can be used before callWasm rethrows. The loader's remaining start-up runs on the next microtask; that is stdio FS.init plus constant stores into libc's main-thread struct. Each replacement uses a fresh loader closure because the loader caches table entries (wasmTableMirror).
  • Out-of-memory cap. A replaced instance is freed only after the current synchronous run yields. At most one replaced instance whose memory grew past 512 MiB (half the heap limit) may be waiting per run; the count resets on the next microtask. After that, the failing call still throws its own error, and later sync calls throw WASM memory limit hit repeatedly in one synchronous run; yield (await) to free memory with that error as cause, until the code awaits. Small failures, such as stack overflows on deep nesting (~18 MiB of growth each), are not limited.
  • A non-Error throw is wrapped as before, but its cause is now a shallow copy, because Emscripten's exit object keeps the whole 1 GiB instance alive.
  • Removed stale, staleFailures, MAX_STALE_FAILURES and the async reload path for failures.
  • If the new instance can't be created: ensureLoaded() retries on the next sync call, and otherwise throws Could not start a fresh WASM instance after a failed call; await loadModule() to retry with cause. loadModule() does a full reload. The 5 inline if (!wasmModule) throw … checks in the full API now call ensureLoaded().

Tests

test/wasm-recovery.test.js is the same file in every version:

  • keeps sync calls working through 60 overflows in one synchronous loop (real 100k-term 1+1+…)
  • gives the next sync call a fresh instance after the memory limit is hit (the repro above)
  • stops replacing after repeated out-of-memory failures in one synchronous run, and recovers after an await: a child process runs 10 OOMs in one loop. It asserts 2 real OOM errors, then 8 cap errors with the OOM as cause, peak RSS under 2.5 GiB, and that SELECT 1 works after await.
  • names the cause when a fresh instance cannot start… and retries a fresh instance that failed to start on the next sync call, both with a simulated instantiate failure
  • every entry point (parse…scanSync): the call right after a failure must run on a new instance (used === loads), with no await in between
run result
main wrapper + published 18.2.1 / 13.8.1 wasm 60 overflows: RangeError ×10, then WASM module not initialized…; memory limit: Failed to allocate memory for query; both fail on v18 and v13
this branch (published wasm, build:js) v18 143/143; v13–v17 92 tests, 0 fail (full-API tests skip)

Speed and memory

Published 18.2.1 / 13.8.1 wasm:

main this PR
loadModule() 9–16 ms 11–22 ms
parseSync, 3,000 corpus statements 10–14 µs/stmt 10–13 µs/stmt
replacing an instance after a failure async reload about 1–2 ms, synchronous
200 deep failures, each followed by SELECT 1 180/200 ok (v18) 200/200 ok
idle external after the failures 42 MB 42 MB
10 OOMs in one synchronous loop: peak RSS 1,124 MB (calls 2–10: wrong error, Failed to allocate memory for query) 2,112 MB (calls 3–10: the cap error, with the OOM as cause)
RSS/external after that loop yields 141 MB / — 135 MB / 2 MB

Related

parsePlPgSQL aborting on assert(proc_source != NULL) for CREATE FUNCTION/PROCEDURE without an AS body (including valid LANGUAGE sql BEGIN ATOMIC … END) is fixed in the fork: constructive-io/libpg_query#14. The node pin for it will be combined later with the other fork changes.

Link to Devin session: https://app.devin.ai/sessions/b32426e30a71409594f003cf94c9e560
Open in Devin Desktop: https://app.devin.ai/desktop/session/b32426e30a71409594f003cf94c9e560?variant=devin
Requested by: @pyramation

@devin-ai-integration

Copy link
Copy Markdown

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@tenki-reviewer

tenki-reviewer Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Review complete. 🟠 1 high

💬 Inline comments (1)

  • 🟠 New require('node:fs') breaks the ESM build — index.ts:143
🧹 Nitpicks (2) — 🟢 2 low
  • 🟢 Renderer exclusion forces fetch of a file:// URL (index.ts:142) — The new Node detection in compileWasm() excludes proc.type === 'renderer' (templates/index.ts:142), so in an Electron renderer with nodeIntegration the code falls through to fetch(url) — but there the Emscripten glue's locateFile yields a file:// URL, which fetch cannot retrieve, so the initial instantiate() rejects and every entry point fails with WASM module not initialized.
  • 🟢 Redundant second wasm load gates init on its success (index.ts:157) — instantiate() lets the Emscripten glue fetch, compile and instantiate the .wasm, then independently re-reads and re-compiles the same file via compileWasm(wasmUrl) before assigning wasmModule (templates/index.ts:157-158).

This PR reworks WASM instantiation and failure handling in the parser template sources (templates/index.ts, templates/full/index.ts) and all generated copies under versions/13-18/src/index.ts, plus matching wasm-recovery tests per version.

A compileWasm() helper pre-fetches and compiles the .wasm bytes (fs on Node, fetch elsewhere) so a cached WebAssembly.Module can be reused when replacing a crashed instance, and replaceInstance() synchronously installs a fresh instance via instantiateWasm + PgQueryModule while the failed call returns a JSON error. The failure-counter logic skips cleanup for the instance that threw, and each version gains tests simulating WASM crashes and recovery.

Files Change
templates/index.ts, templates/full/index.ts New compileWasm()/replaceInstance() recovery path, cached compiled module, failure-counter cleanup skip
versions/13-18/src/index.ts Generated copies of the same recovery logic for each PG version
versions/13-18/test/wasm-recovery.test.js New tests covering crash simulation and instance replacement

Reviewed commit: 564154f

@tenki-reviewer tenki-reviewer Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adds WASM crash recovery across all libpg_query version bundles: a shared compiled-module cache, a compileWasm() pre-compile step, and a replaceInstance() path that swaps in a fresh instance when a WASM call throws.

Key findings

  • 🟠 New require('node:fs') breaks the ESM build — index.ts:143

Comment thread templates/index.ts Outdated
async function compileWasm(url: string): Promise<WebAssembly.Module> {
const proc = (globalThis as any).process;
if (proc?.versions?.node && proc.type !== 'renderer') {
return WebAssembly.compile(require('node:fs').readFileSync(url.startsWith('file://') ? new URL(url) : url));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 bug · high

New require('node:fs') breaks the ESM build

The new compileWasm() helper reads the .wasm file with a bare require('node:fs') on the Node path (templates/index.ts:143, replicated in templates/full/index.ts and versions/13-18/src/index.ts). The same source is compiled to ESM (wasm/index.js) and exported under the import condition in parser/package.json, so Node ESM consumers of @pgsql/parser execute the bare require and get ReferenceError: require is not defined on the only instantiation path, failing every parse/fingerprint call; bundlers targeting the module entry likewise fail to resolve it for browser builds. The swallowed error in instantiate().catch(() => {}) leaves later calls failing with WASM module not initialized.

📋 Prompt for AI Agents

In templates/index.ts line 143 (and the identical copies in templates/full/index.ts and versions/13 through versions/18 src/index.ts), compileWasm() calls require('node:fs'), which is undefined in the emitted ESM build (wasm/index.js, exported under the import condition of @pgsql/parser). Replace the bare require with a module-system-agnostic load — e.g. const fs = await import('node:fs') inside the Node branch, or (await import('node:module')).createRequire(import.meta.url) — so both the CJS and ESM builds can read the .wasm file in Node. Apply the change to all copies, and confirm parser/test/esm.test.mjs passes afterward.

@devin-ai-integration

Copy link
Copy Markdown

Folded into #190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant