Repository navigation
fix(v18): throw errors from fingerprint, normalize and scan instead of returning them - #186
Closed
pyramation wants to merge 1 commit into
Closed
pyramation wants to merge 1 commit into
pyramation wants to merge 1 commit into
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. No blocking issues — approved ✅; 1 nitpick below. 🧹 Nitpicks (1) — 🟢 1 low
This PR hardens the libpg-query WASM bindings: the C wrapper now detects WASM heap exhaustion via NULL returns, reports protocol failures through an "ERROR: "-prefixed buffer, and rewrites
Reviewed commit: ca67fa6 |
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
The v18 wrappers for
fingerprint,normalizeandscanreturned libpg_query error text as an ordinary result string. The JS side then only threw on messages starting withsyntax error. Every other error was either returned as the result or broke JSON parsing. Out-of-memory insidebuild_scan_jsonsilently returned an empty token list. Reproduced on published 18.2.0 and 18.2.1:The cause is that
wasm_scanreturns{"version":0,"tokens":[]}when its upfrontmalloc(n_tokens * ~200)fails (from about 10.4 MB of short statements). It also skips any token whose copy fails to allocate.The fix, in
templates/full(regenerated intoversions/18/srcwithcopy:templates):wasm_wrapper.c:wasm_fingerprint,wasm_normalize_queryandwasm_scannow return errors as"ERROR: <message>"via a newerror_result(). Every allocation failure returnsNULLinstead of a placeholder string or an empty result.index.ts: a newresultOrThrow(ptr)replaces thestartsWith('syntax error')check in all six entry points.NULLthrowslibpg_query returned no result: out of WASM memory ….ERROR:is stripped and the message is thrown as anError, so existing messages such assyntax error at end of inputare unchanged.Tests in
versions/18/testcover lexer errors and empty input infingerprint,normalizeandscan, plus an 11 MBscanSync/scanthat must throw and then recover. The 4 new tests fail against the published 18.2.1 WASM and pass after the rebuild (emscripten/emsdk:6.0.3). The v18 suite passes 147/147.Not fixed here: around 50 MB,
pg_query_scanaborts withAborted(Assertion failed: <binary>)inside libpg_query and writes about 4 MB of binary to stderr. That is a clean JSRuntimeError, and later async calls recover. The behaviour is the same in 18.2.0.Link to Devin session: https://app.devin.ai/sessions/68876ce060664609ab6b63c8bed91e78
Open in Devin Desktop: https://app.devin.ai/desktop/session/68876ce060664609ab6b63c8bed91e78?variant=devin
Requested by: @pyramation