Skip to content

fix(wasm): replace the module after an exception escapes a WASM call - #175

Open
pyramation wants to merge 5 commits into
mainfrom
fix/wasm-recover-after-throw
Open

pyramation wants to merge 5 commits into
mainfrom
fix/wasm-recover-after-throw

Conversation

@pyramation

@pyramation pyramation commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

Merge #173 first. This branch includes #173, because on main the v13 CI job tests the wrapper bundled in @libpg-query/v13@13.5.7, not this repo's source. Without #173 the v13 recovery test ran against that old wrapper and hung. Once #173 is in main, its commits drop out of this diff.

Summary

Before: in every published build (17.7.4, 18.1.5 and the lowmem tags), one bad input could permanently break the parser for the rest of the process.

  • If an exception escapes a WASM call, libpg_query's cleanup never runs. The exception can be a V8 RangeError (stack overflow on deeply nested SQL), a trap, or Emscripten's exit() when the 1 GiB heap limit is hit.
  • Each such exception leaks the C shadow stack and the whole parse memory context.
  • Repeat a deep input about 35 times (SELECT 1+1+…, 100k terms, 25 ms each) and the instance traps on every later call, including SELECT 1.
  • The heap-limit path is worse: it throws ExitStatus, which is not an Error, and silently sets process.exitCode = 1 in the host process.

Change: JS wrapper only. No WASM rebuild and no memory-flag change.

resultPtr = callWasm(() => wasmModule._wasm_parse_query_raw(queryPtr));

function callWasm(fn) {
  const exitCode = process?.exitCode;
  try { return fn(); }
  catch (error) {
    wasmModule = undefined;          // the instance's state is unknown; drop it
    initPromise = instantiate();     // load a fresh one (10–25 ms)
    process.exitCode = exitCode;     // undo exit()'s side effect
    if (error instanceof Error) throw error;
    throw new Error('libpg_query terminated (…), most likely out of WASM memory; …', { cause: error });
  }
}
  • finally blocks skip _free once the instance has been dropped.
  • Async entry points loop while (!wasmModule) await (initPromise ?? instantiate()). That includes a call already queued behind the failing one, so it waits for the fresh instance.
  • A *Sync call made during the 10–25 ms reload throws the existing "Call loadModule() first" error.
  • If the reload itself fails, initPromise is cleared, so the next call retries. The failed promise always has a handler, so it never becomes an unhandled rejection that kills the process.
  • Only throws that unwind out of WASM trigger a reload. A SqlError from a normal parse error is built in JS after the call returns, so it does not.

Reproduced first: the new test/wasm-recovery.test.js runs in every version (13–18).

  • Stays usable: 60 deep-chain failures, then parseSync('SELECT 1'). On the current build, call 34 ends in RuntimeError: table index is out of bounds, and so does SELECT 1 afterwards.
  • Heap limit: a 50 MB 'SELECT 1;\n' input must reject with an Error, leave process.exitCode unchanged, and still parse SELECT 1. Before the fix it threw RuntimeError (after the first test) or ExitStatus with process.exitCode = 1.
  • Queued call: parse(deep) and parse('SELECT 1') run concurrently, and the second must succeed. Before the fix it got "Call loadModule() first".
  • Failed reload: tested by hand with a stub loader that fails on its 2nd call. Before the fix, the process died with an unhandled rejection, or every later call rejected forever. Now the next call reloads and parses.
  • After the fix: v18 95/95, v17 44/44, v13 44/44 pass locally.

These are the two wrapper bugs the memory-defaults research found; that research's harness is on the research/wasm-memory branch. Separately, the prebuilt v13 WASM still uses Emscripten's 64 KiB stack and traps on a 1000-term 1+1+…. With this PR it recovers instead of staying broken.

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

stringToPtr sized the buffer with lengthBytesUTF8 and wrote with stringToUTF8,
which disagree on unpaired surrogates (4 bytes per two units vs 3 bytes each),
so the write stopped short and the tail of the query was silently dropped. An
embedded NUL likewise ended the C string early.

Encode once with TextEncoder (unpaired surrogates become U+FFFD, the bytes a
Postgres driver sends), allocate exactly that many bytes plus the terminator,
and throw on an embedded NUL.

Fixes #169
A RangeError (V8 stack overflow on deeply nested SQL), a trap, or exit()
when the heap limit is hit unwinds out of libpg_query without running its
cleanup, leaking the shadow stack and the parse memory context. After ~35
such errors the instance trapped on every call, even SELECT 1. The FATAL
path also threw a non-Error ExitStatus and set process.exitCode=1.

Discard the instance and re-instantiate it, restore process.exitCode, and
turn non-Error throws into an Error.
@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 21, 2026 •

Copy link
Copy Markdown

Review complete. 🟡 2 medium

💬 Inline comments (2)

  • 🟡 Concurrent callers fail spuriously during WASM recovery window — index.ts:144
  • 🟡 Recovery instantiate() rejection is left unhandled — index.ts:145

This PR wires a recovery path into the WASM wrappers: callWasm catches native aborts/OOM, nulls the module, re-instantiates it, and retries or rethrows, and each version gains a wasm-recovery test suite exercising OOM and stack-overflow recovery.

Files Change
templates/index.ts, templates/full/index.ts Add callWasm guard/recovery logic used to regenerate the per-version wrappers.
versions/{13..18}/src/index.ts Regenerated from templates; identical recovery semantics across versions.
versions/{13..18}/package.json Append wasm-recovery suite to test scripts (18 keeps its extra suites).
versions/{13..18}/test/wasm-recovery.test.js New resource-exhaustion tests: large multi-statement OOM input and repeated stack-overflow loop asserting recovery via loadModule().

Reviewed commit: 01c4459

@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 (re-instantiation on OOM/abort) and recovery tests across the six pg_query versions, with two medium-severity gaps in the recovery window left to address.

Key findings

  • 🟡 Concurrent callers fail spuriously during WASM recovery window — index.ts:144
  • 🟡 Recovery instantiate() rejection is left unhandled — index.ts:145

Comment thread templates/index.ts Outdated
Comment thread templates/index.ts Outdated
…a failed reload

Async entry points now loop until an instance exists instead of awaiting
initPromise once, so a call queued behind a failing one waits for the fresh
instance. A rejected load clears initPromise (and is handled), so it no
longer surfaces as an unhandled rejection and the next call retries.
…o's wrapper

On main the v13 job runs the wrapper bundled in @libpg-query/v13@13.5.7, so
the recovery test exercised the old wrapper and hung. Also bound the
recovery suite with a 120 s timeout so a regression fails instead of hanging.
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