Skip to content

fix(v18): throw errors from fingerprint, normalize and scan instead of returning them - #186

Closed
pyramation wants to merge 1 commit into
mainfrom
fix/v18-scan-fingerprint-normalize-errors
Closed

pyramation wants to merge 1 commit into
mainfrom
fix/v18-scan-fingerprint-normalize-errors

Conversation

@pyramation

Copy link
Copy Markdown
Collaborator

Summary

The v18 wrappers for fingerprint, normalize and scan returned libpg_query error text as an ordinary result string. The JS side then only threw on messages starting with syntax error. Every other error was either returned as the result or broke JSON parsing. Out-of-memory inside build_scan_json silently returned an empty token list. Reproduced on published 18.2.0 and 18.2.1:

fingerprintSync("SELECT 'abc")  // => "unterminated quoted string at or near \"'abc\""  (as the fingerprint)
normalizeSync('SELECT "x')      // => "unterminated quoted identifier at or near \"\"x\""
fingerprintSync('')             // => "Invalid input: query cannot be null or empty"
scanSync("SELECT 'abc")         // SyntaxError: Unexpected token 'u', "unterminat"... is not valid JSON
scanSync('SELECT a, b FROM t WHERE c = 1;\n'.repeat(340000))  // 10.4 MB => { version: 0, tokens: [] }, no error

The cause is that wasm_scan returns {"version":0,"tokens":[]} when its upfront malloc(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 into versions/18/src with copy:templates):

  • wasm_wrapper.c: wasm_fingerprint, wasm_normalize_query and wasm_scan now return errors as "ERROR: <message>" via a new error_result(). Every allocation failure returns NULL instead of a placeholder string or an empty result.
  • index.ts: a new resultOrThrow(ptr) replaces the startsWith('syntax error') check in all six entry points.
    • NULL throws libpg_query returned no result: out of WASM memory ….
    • ERROR: is stripped and the message is thrown as an Error, so existing messages such as syntax error at end of input are unchanged.

Tests in versions/18/test cover lexer errors and empty input in fingerprint, normalize and scan, plus an 11 MB scanSync/scan that 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_scan aborts with Aborted(Assertion failed: <binary>) inside libpg_query and writes about 4 MB of binary to stderr. That is a clean JS RuntimeError, 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

@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. No blocking issues — approved ✅; 1 nitpick below.

🧹 Nitpicks (1) — 🟢 1 low
  • 🟢 Unpack failure misreported as WASM heap exhaustion (wasm_wrapper.c:402) — The PR changed wasm_scan to return NULL when pg_query__scan_result__unpack fails (previously it returned an "ERROR: Failed to unpack scan result" string), while the new resultOrThrow maps every NULL pointer to a message claiming the 1 GiB WASM heap was exhausted and suggesting smaller input pieces (versions/18/src/index.ts:258).

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 build_scan_json to serialize scan results with proper escaping and per-token cleanup. The TypeScript entry points in templates/full and versions/18 map NULL results to OOM throws, and the v18 test suite gains fingerprint, normalize, and scan coverage exercising the new paths.

Files Change
templates/full/wasm_wrapper.c, versions/18/src/wasm_wrapper.c Rewrite error/OOM contract: NULL-on-heap-exhaustion sentinel, "ERROR: "-prefixed error buffer, leak-free build_scan_json
templates/full/index.ts, versions/18/src/index.ts Map NULL wrapper results to explicit heap-exhaustion throws on parse/normalize/fingerprint/scan paths
versions/18/test/fingerprint.test.js, versions/18/test/normalize.test.js, versions/18/test/scan.test.js Add tests for fingerprint, normalization, and scan error reporting behavior

Reviewed commit: ca67fa6

@devin-ai-integration

Copy link
Copy Markdown

Closing in favour of #183, which keeps the structured SqlError design. The parts of this PR that #183 lacked are now on #183 in 6610a93: scan out of memory throws instead of returning partial or empty tokens, plus the 11 MB test and the lexer-error and empty-input tests.

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