Repository navigation
fix(wasm): replace a failed instance synchronously so the next call never runs on a broken one - #189
pyramation wants to merge 3 commits into
Conversation
…ever runs on a broken one
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟠 1 high 💬 Inline comments (1)
🧹 Nitpicks (2) — 🟢 2 low
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.
Reviewed commit: 564154f |
There was a problem hiding this comment.
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
| 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)); |
There was a problem hiding this comment.
🟠 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.
|
Folded into #190 |
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 gotWASM module not initialized. Call loadModule() first.Both behaviours give errors that don't describe the real cause:In fuzzing, stale-instance sync calls also produced
RuntimeError: table index is out of boundsandAborted(Assertion failed: <garbage>).Change (all majors, via
templates/):.wasmURL through the loader'slocateFileand keeps a compiledWebAssembly.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,fetchelsewhere.callWasm()catch →replaceInstance(). This callsPgQueryModule({ instantiateWasm }), whose hook runsreceive(new WebAssembly.Instance(compiled, imports))synchronously. Exports and memory views are set insidereceive, so the new instance can be used beforecallWasmrethrows. The loader's remaining start-up runs on the next microtask; that is stdioFS.initplus constant stores into libc's main-thread struct. Each replacement uses a fresh loader closure because the loader caches table entries (wasmTableMirror).WASM memory limit hit repeatedly in one synchronous run; yield (await) to free memorywith that error ascause, until the code awaits. Small failures, such as stack overflows on deep nesting (~18 MiB of growth each), are not limited.causeis now a shallow copy, because Emscripten's exit object keeps the whole 1 GiB instance alive.stale,staleFailures,MAX_STALE_FAILURESand the async reload path for failures.ensureLoaded()retries on the next sync call, and otherwise throwsCould not start a fresh WASM instance after a failed call; await loadModule() to retrywithcause.loadModule()does a full reload. The 5 inlineif (!wasmModule) throw …checks in the full API now callensureLoaded().Tests
test/wasm-recovery.test.jsis the same file in every version:keeps sync calls working through 60 overflows in one synchronous loop(real 100k-term1+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 ascause, peak RSS under 2.5 GiB, and thatSELECT 1works afterawait.names the cause when a fresh instance cannot start…andretries a fresh instance that failed to start on the next sync call, both with a simulated instantiate failureparse…scanSync): the call right after a failure must run on a new instance (used === loads), with noawaitin between60 overflows: RangeError ×10, thenWASM module not initialized…;memory limit:Failed to allocate memory for query; both fail on v18 and v13build:js)Speed and memory
Published 18.2.1 / 13.8.1 wasm:
loadModule()parseSync, 3,000 corpus statementsSELECT 1externalafter the failuresFailed to allocate memory for query)cause)externalafter that loop yieldsRelated
parsePlPgSQLaborting onassert(proc_source != NULL)forCREATE FUNCTION/PROCEDUREwithout anASbody (including validLANGUAGE 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