[vuanhphung]

Prefetch one kernel batch for pending query handles; completed default-path queries remain on demand. Priming is single-flight, and synchronous fetch throws are deferred correctly. Close disposes results only after the RPC succeeds; failed closes preserve unread batches, and concurrent closes share one outcome. Cancellation still blocks further reads. The binding cannot abort a fetch already in progress, which may keep Node alive after close/cancel.

Live pecotesting Medium serverless, Node 24.14.0/kernel 1.4.0; three fresh processes per build/query/mode, alternating build order, result cache off, row materialization disabled. Compared 60fa17e → 9bc4047 with identical ResultSlicer code. Elapsed includes execute + finished() + full drain; drain starts after finished(). RSS is the Linux whole-process high-water mark, including native buffers. Some samples overlapped CI on the same warehouse; timings were not isolated.

Execution / query	Elapsed s, main → branch	Drain s, main → branch	Peak RSS MiB, main → branch
async / boolean (80M)	3.949 → 3.579	2.489 → 2.130	344.9 → 352.5
async / float (50M)	5.200 → 4.919	1.561 → 1.420	647.4 → 676.7
async / double (30M)	4.793 → 4.666	0.974 → 0.937	743.6 → 684.1
async / catalog_sales (42.7M)	27.378 → 25.473	21.022 → 18.930	1666.7 → 1871.6
default / range (10K)	0.235 → 0.235	0.012 → 0.012	100.9 → 100.8
default / boolean (80M)	3.373 → 3.411	2.420 → 2.426	360.3 → 351.2
default / catalog_sales (42.7M)	26.679 → 30.879	20.373 → 25.250	1845.3 → 1765.8

All default range/boolean samples returned completed statements and fetched on demand. Default catalog_sales returned mixed handles: main had 1 pending / 2 completed, branch had 2 pending / 1 completed. The default large-result comparison therefore mixes prefetch-enabled and on-demand execution. CloudFetch and matching row counts were confirmed in every non-range sample.

Async extended-type drain medians improved 4–14%. Large-result performance remains inconsistent: the async catalog_sales drain median improved 10%, but the default median regressed 24%; the earlier five-sample async comparison showed no gain. Async catalog_sales peak RSS increased 12% in this rerun. These results do not establish a general large-result speedup or a fixed memory overhead.

Queries: SELECT val from main.driver_ext_types_bench.t_{boolean,float,double}; SELECT * FROM main.tpcds_sf100_delta.catalog_sales WHERE cs_ship_mode_sk <= 14 AND cs_sold_date_sk BETWEEN 2450815 AND 2451635; SELECT id FROM range(0, 10000).

Validation: 1,555 unit tests pass with kernel 1.4.0; build, type-check, lint, and Prettier pass. Four regressions cover failed close, concurrent closes, cancel during close, and failed fetch-error cleanup. Both live CloudFetch lifecycle tests pass on pecotesting with the native fetch blocked. Unit and E2E CI pass on Node 22/24/26; the real Kernel E2E run passes 23 tests against the binding built from KERNEL_REV.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Medium

The prefetch design looks sound (failed prefetches can't reject unhandled, and prefetch is limited to async handles), with one medium concern: overlapping prime() calls can overwrite the in-flight prefetch and drop a batch. Lifecycle and error-path tests are thorough.

[peco-review-bot]

🟡 Medium — When two prime() calls overlap (for example hasMore() and fetchNext() called together, or two fetchChunk calls with no upstream lock — I found no fetch mutex in lib/), both await the same this.inflight promise. The first continuation sets inflight = undefined and, with prefetch on, calls startFetch(), which assigns a new promise P2. The second continuation then runs this.inflight = undefined without checking whether the field still holds the promise it awaited. That overwrites P2, and its own startFetch() starts P3. P2's batch is then lost: no caller ever reads it, so a result batch is silently dropped. Before this PR, each concurrent call made its own fetchNextBatch() and no batch was lost.

Fix: only clear the field if it still holds the promise this call awaited, e.g. const current = this.inflight!; const settled = await current; if (this.inflight === current) this.inflight = undefined;. Add a unit test that calls hasMore() and fetchNext() concurrently with prefetch enabled.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Medium

Looks good overall, with one medium issue: if a close RPC fails, the results provider stays disposed while the operation still reports as open, so later fetches silently return no rows. The prefetch logic itself (the shared prime promise, error reflection on abandoned fetches, at most one fetch in flight) is sound and well tested.

[peco-review-bot]

🟡 Medium — close() calls this.resultsProvider?.dispose() before kernelClose, and dispose() can't be undone: it sets exhausted = true and drops the buffered batch. If the close RPC then fails, kernelClose sets isClosed back to false and rethrows. The DBSQLOperation facade never sets its closed flag, so from the caller's side the operation is still active.

If the caller catches the close error and keeps reading, failIfNotActive passes. hasMore() then returns false and fetchChunk() returns [] with no error, so the result set ends early without any signal. This applies to both handle kinds, including the terminal Statement path, where prefetch is disabled. Before this change, a failed close left the stream readable.

The disposal should only become permanent once close succeeds. Alternatively, a provider disposed by a close that later failed should throw when read instead of reporting exhaustion. A unit test where lifecycleHandle.close rejects and the caller then calls hasMore()/fetchChunk() would cover this. (cancel() doesn't have this problem, because kernelCancel keeps isCancelled set even when the RPC fails.)

[peco-review-bot]

Verdict: 1 Medium

The prefetch design looks sound, with one medium issue: close() disposes the results provider before the close RPC succeeds, so a failed close leaves an active operation that silently reports no more rows. The single-flight prime, the deferred synchronous throws and the error handling for abandoned prefetches are correct, and the tests cover them.

[peco-review-bot]

🟡 Medium — close() calls resultsProvider.dispose() before it knows whether kernelClose succeeds. If the native close() rejects, kernelClose rolls isClosed back to false, so the operation counts as active again and failIfNotActive passes. The provider stays disposed for good, though: dispose() set exhausted = true and dropped the buffered batch. So after a failed close(), hasMore() returns false and fetchChunk() returns [] with no error. fetchAll() and streaming callers would read that as the normal end of the result set and silently lose the remaining rows. Before this change, a failed close left fetching working.

cancel() doesn't have this problem, because isCancelled stays set when the RPC fails. On the fetchChunk error path the original error is already thrown, so it's also fine there. The gap is a user-initiated close() that fails.

Possible fixes: once the provider is disposed, make later hasMore()/fetchNext() calls throw instead of reporting exhaustion. Or, if kernelClose rejects, mark the operation unusable (for example, keep a separate disposed flag that failIfNotActive checks). Either way, add a unit test where the lifecycle close stub rejects, then call fetchChunk and hasMore.

[peco-review-bot]

✅ No issues identified by the review bot.