Fix N-API error builders returning an unwritten error value

[kriszyp]

⊙ Problem

createRocksDBError and createJSError (src/binding/napi/helpers.cpp) return their result through an error out-param. When one of their own N-API calls failed, they returned without writing it. About 17 call sites never initialise that local, so on that failure it reached napi_throw or a promise reject unwritten. The async completion sites (backup, backup stream, checkpoint, commit) would then never settle their promise.

❓ Your call: I fixed the builders, not the call sites. The dispatch task preferred changing the builders to return a napi_value and updating all 22 callers; I did not, because a napi_value return can also be null, so it gives no guarantee the out-param lacks once the builder always writes. Reviewers agreed in plan review. Say if you want the return-value form anyway.

💡 Solution

When an internal N-API call fails, the builder hands back the exception that call left pending. If none is pending, it synthesizes one first, so the value is always a real JS value. Every path now writes error, so no caller changes. Successful builds gain no N-API calls.

• Recovery: takeFailedCallException checks napi_is_exception_pending before it synthesizes, and reads the extended error first, because the pending check resets it.
• Every failing N-API call in createRocksDBError and createJSError goes through NAPI_STATUS_TAKES_ERROR.

Invariant: after either builder returns, error is a valid napi_value. The contract is in src/binding/napi/DESIGN.md.

⚖️ Alternatives

• Return napi_value and migrate 22 call sites. Not chosen: same guarantee as the out-param once the builder always writes it; 22 files changed for no added guarantee.
• Initialise and check at each call site. Not chosen: the async sites have no napi_throw fallback, so each would need its own recovery; the builder contract would stay "may leave the out-param unwritten", so the next caller repeats the bug.

Framing-Verdict: better-alternative-exists (34abdcc9efcd) from the planning review, adopted: the centralized out-param repair, as the reviewer proposed.

🔧 Changes

• Builders: src/binding/napi/helpers.cpp, recovery above. Header signatures unchanged.
• Regression fixture: test/fixtures/fork-error-object-failure.mts. Runs in a child process, patches Object.create (the global the builders call), and checks: a thrown value comes back as the same value (async backups.list), a non-callable factory yields an Error (sync transactionSync with setTimestamp(-1), and async).
• Test driver: test/error-object-failure.test.ts. Kills the child at 10 s, waits for it to exit, then removes the database directory.
• Design note: src/binding/napi/DESIGN.md and its index entry in DESIGN.md. Records which regression case discriminates (async only; see Verification).
• README: Node.js requirement now reads ^22.18.0 || >=24.0.0, matching package.json engines.

❓ Your call: Not fixed here, recorded as a follow-up. Commit-metadata decorations (hasLog at transaction.cpp:856 and its sync siblings, appliedInMemoryOnly at database.cpp) set a property on the recovered value without checking the result. If that value is null or undefined, the setter throws and the commit promise is never settled. A builder returns undefined only when it cannot create an exception (out-of-memory during napi_create_error) or when user code makes Object.create throw null or undefined. Fixing it means guarding four decoration sites, which is a separate change.

✅ Verification

• pnpm build (bundle + native binding): passes; no compiler warnings in helpers.cpp.
• pnpm check (type-check, lint, format): passes.
• pnpm test on the rebased head bd9ff25f: 78 files passed, 1 skipped; 1152 tests passed, 10 skipped. The branch was rebased onto main a6260366 after Make optimistic commit lock buckets and validation policy configurable #897, Stop verification-table slot collisions from answering FRESH for another key #901, Run a database's async commits on up to four concurrent commit threads #902 and Read every entry of a transaction-log segment that one transaction pushed past transactionLogMaxSize #890 merged; the only conflict was the DESIGN.md index, resolved by keeping both entries.
• Fails on the pre-fix builder: with src/binding/napi/helpers.cpp taken from 1b165e09, the fixture exits non-zero (the thrown-value case's reject never runs, because the builder's pending exception blocks it). It passes on this branch.
• Not reproducible from JS without patching a JS global. The sync createJSError case passes on the pre-fix builder too (the synthesized exception is already pending, so the caller's napi_throw is a no-op), so it checks the new path's result, not the regression.
• Not covered: commit-decoration failures (see Your call).

Pre-push review: 7 rounds on the pre-rebase head, then one full round on the rebased head. The open findings are the decoration hazard above, plus follow-ups recorded in the dispatch file: a lost RocksDB status message when the builder itself fails under OOM, the duplicated Object.create/Error.prototype lookup, and the now-dead error == nullptr guards in database.cpp.

🤖 Generated with Claude Code

Related PRs: #767 independent, #905 overlaps (DESIGN.md index line), #897 overlaps (README and DESIGN.md index lines), #742 independent, #902 overlaps (README and DESIGN.md index lines), #901 overlaps (DESIGN.md index line), #890 overlaps (DESIGN.md index line)
Complexity: medium

_{Dispatch: task rocksdb-js-createrocksdberror-callsites · queued by unknown · ran by claude/sonnet/xhigh · worker kzyp-xps-1}

_{Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse; rounds=8; full=4 @ bd9ff25}

_{Review-Attention: read ~3m (decisions: recovered-value-shape, synthesized-message, fix-layer) @ bd9ff25}

[gemini-code-assist]

Code Review

This pull request updates the N-API error builders (createRocksDBError and createJSError in helpers.cpp) to ensure they always write to their error out-parameter, even when internal N-API calls fail. It introduces a new helper takeFailedCallException and macro NAPI_STATUS_TAKES_ERROR to retrieve or synthesize pending exceptions correctly. Additionally, the Node.js version requirement in README.md is updated to ^22.18.0 || >=24.0.0, design documentation is added, and integration tests are introduced to verify error handling behavior. No review comments were provided, so there is no feedback to address.

[github-actions]

📊 Benchmark Results

get-sync.bench.ts

getSync() > random keys - small key size (100 records)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	24.48K ops/sec	40.84	39.45	1,992.929	0.141	122,424
🥈 rocksdb	2	10.42K ops/sec	96.01	92.44	23,841.715	0.962	52,076

getSync() > sequential keys - small key size (100 records)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	28.41K ops/sec	35.20	34.15	605.421	0.105	142,039
🥈 rocksdb	2	9.53K ops/sec	104.974	101.304	3,653.076	0.150	47,631

ranges.bench.ts

getRange() > small range (100 records, 50 range)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	23.77K ops/sec	42.08	37.04	1,874.665	0.286	118,831
🥈 rocksdb	2	15.33K ops/sec	65.22	56.72	1,070.235	0.121	76,665

realistic-load.bench.ts

Realistic write load with workers > write variable records with transaction log

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	461.38 ops/sec	2,167.425	44.28	68,795.19	15.82	923
🥈 lmdb	2	25.91 ops/sec	38,590.428	441.313	1,206,872.942	136.277	64.00

transaction-log.bench.ts

Transaction log > read 100 iterators while write log with 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	34.69K ops/sec	28.82	13.76	20,840.486	0.845	173,465
🥈 lmdb	2	436.75 ops/sec	2,289.641	127.27	13,709.366	1.31	2,184

Transaction log > read one entry from random position from log with 1000 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	602.63K ops/sec	1.66	1.41	5,068.653	0.210	3,013,140
🥈 lmdb	2	482.69K ops/sec	2.07	1.18	8,478.029	0.552	2,413,474

worker-put-sync.bench.ts

putSync() > random keys - small key size (100 records, 10 workers)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	842.38 ops/sec	1,187.118	1,051.391	4,046.753	0.391	1,685
🥈 lmdb	2	1.14 ops/sec	877,483.44	818,499.958	987,701.877	3.72	10.00

worker-transaction-log.bench.ts

Transaction log with workers > write log with 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	21.14K ops/sec	47.31	30.04	20,580.291	2.08	42,272
🥈 lmdb	2	830.47 ops/sec	1,204.143	181.512	20,995.98	5.64	1,661

---

Results from commit d7f09a8

[cb1kenobi]

Good catch!