Repository navigation
fix(wasm): replace the module after an exception escapes a WASM call - #175
Open
pyramation wants to merge 5 commits into
Open
pyramation wants to merge 5 commits into
pyramation wants to merge 5 commits into
Conversation
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
…3 tests cover src/
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.
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟡 2 medium 💬 Inline comments (2)
This PR wires a recovery path into the WASM wrappers:
Reviewed commit: 01c4459 |
There was a problem hiding this comment.
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
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
RangeError(stack overflow on deeply nested SQL), a trap, or Emscripten'sexit()when the 1 GiB heap limit is hit.SELECT 1+1+…, 100k terms, 25 ms each) and the instance traps on every later call, includingSELECT 1.ExitStatus, which is not anError, and silently setsprocess.exitCode = 1in the host process.Change: JS wrapper only. No WASM rebuild and no memory-flag change.
finallyblocks skip_freeonce the instance has been dropped.while (!wasmModule) await (initPromise ?? instantiate()). That includes a call already queued behind the failing one, so it waits for the fresh instance.*Synccall made during the 10–25 ms reload throws the existing "CallloadModule()first" error.initPromiseis cleared, so the next call retries. The failed promise always has a handler, so it never becomes an unhandled rejection that kills the process.SqlErrorfrom a normal parse error is built in JS after the call returns, so it does not.Reproduced first: the new
test/wasm-recovery.test.jsruns in every version (13–18).parseSync('SELECT 1'). On the current build, call 34 ends inRuntimeError: table index is out of bounds, and so doesSELECT 1afterwards.'SELECT 1;\n'input must reject with anError, leaveprocess.exitCodeunchanged, and still parseSELECT 1. Before the fix it threwRuntimeError(after the first test) orExitStatuswithprocess.exitCode = 1.parse(deep)andparse('SELECT 1')run concurrently, and the second must succeed. Before the fix it got "CallloadModule()first".These are the two wrapper bugs the memory-defaults research found; that research's harness is on the
research/wasm-memorybranch. Separately, the prebuilt v13 WASM still uses Emscripten's 64 KiB stack and traps on a 1000-term1+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