Stop teardown landing on a stream fetch that is still running - #77
Merged
Conversation
The v3 suite fails intermittently on macOS with "a session (path=…) is active; close it before using standalone query()" — a registry-state error in a file that never opened that session. Three of four CI runs on #76 hit it, always on a macOS shard, never on Linux. index.js states the invariant that explains it: because the C ABI has no interrupt, a native op runs on its libuv thread to completion no matter when the JS promise settles, so every native op registers in pendingNativeOps and nothing destroys a connection while one is outstanding. Releasing a connection mid-op aborts the shared in-process engine for the rest of the process, and on some platforms leaves the worker blocked inside libchdb so its promise never settles. Streaming was the one path that never registered. queryAsync and Session.queryAsync go through withAbortTimeout, insert and raw insert through runInsert and wrapRawNative, all of which track; StreamFetch tracked nothing. So _drainPendingOps() reported quiet while a fetch was mid-flight and the suite's global afterEach closed the connection under it. Two more holes on the same principle came out of reading around it. cancel() destroys the stream handle synchronously via StreamCancel, and the fetch worker reads that handle with no lock — its `if (!st_ || st_->finished || !st_->handle)` guard can be invalidated between the check and chdb_stream_fetch_result. And close() ignored _activeStream entirely, so the handle outlived the connection it points into and the iterator's own finally would later cancel against a connection that no longer existed. All three now go through the one mechanism: the fetch is tracked, cancel() defers the destroy behind an in-flight fetch, and close() cancels the active stream before releasing the connection. The last two compose without extra ordering logic — cancel() registers its deferred destroy in pendingNativeOps, which close()'s own deferral already waits for, so the stream handle is always destroyed before the connection. The two new tests pick a query that needs a real per-row computation. The obvious choice, count() over numbers(), returns in ~15ms, which is short enough that both tests pass whether or not the races are handled — so they assert the precondition (a fetch is in flight when teardown lands) rather than trusting it, and a future change that makes the fetch fast fails the test instead of silently losing the coverage. Verified by running the full v3 suite with and without the index.js change: the same nine failures either way (Layer 3 arrow-input and parametrized streaming, which need a different engine build than this machine has), plus the two new tests failing only without it. Also corrects a comment in async-stress.test.ts claiming plain queryAsync is not tracked for drain. withAbortTimeout has tracked every async query since #53; the stale note sent this investigation down the wrong path for a while. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@wudidapaopao, please review this |
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.
The v3 suite fails intermittently on macOS with
a session (path=…) is active; close it before using standalone query()— a registry-state error raised in a filethat never opened that session. Three of four CI runs on #76 hit it, always on a
macOS shard, never on Linux.
index.jsstates the invariant that explains it. The C ABI has no interrupt, so anative op runs on its libuv thread to completion regardless of when the JS promise
settles; every native op therefore registers in
pendingNativeOps, and nothingdestroys a connection while one is outstanding. Releasing a connection mid-op
aborts the shared in-process engine for the rest of the process, and on some
platforms leaves the worker blocked inside libchdb so its promise never settles.
Streaming was the one path that never registered.
queryAsyncandSession.queryAsyncgo throughwithAbortTimeout,insertand raw insert throughrunInsertandwrapRawNative, all of which track.StreamFetchtracked nothing,so
_drainPendingOps()reported quiet while a fetch was mid-flight and the suite'sglobal
afterEachclosed the connection under it.Reading around that turned up two more holes on the same principle:
cancel()destroys the stream handle synchronously throughStreamCancel, andthe fetch worker reads that handle with no lock — its
if (!st_ || st_->finished || !st_->handle)guard can be invalidated between thecheck and
chdb_stream_fetch_result.close()ignored_activeStream, so the handle outlived the connection it pointsinto and the iterator's own
finallywould later cancel against a connection thatno longer existed.
All three now go through the one mechanism: the fetch is tracked,
cancel()defersthe destroy behind an in-flight fetch, and
close()cancels the active stream beforereleasing the connection. The last two compose without extra ordering logic — the
deferred destroy lands in
pendingNativeOps, whichclose()'s own deferral alreadywaits for, so the stream handle is always destroyed before the connection.
The tests would have been useless with the obvious query
count()overnumbers()returns in ~15ms even at 2e8 rows, short enough that bothtests pass whether or not the races are handled. They use a query that needs a real
per-row computation instead, and they assert the precondition — a fetch is in flight
when teardown lands — rather than trusting it, so a change that makes the fetch fast
fails the test instead of silently dropping the coverage.
What was checked
Full v3 suite with and without the
index.jschange: the same nine failures eitherway (Layer 3 arrow-input and parametrized streaming, which want a different engine
build than the machine had), plus the two new tests failing only without the fix.
The v2 mocha half could not run locally — mocha's yargs dependency breaks under Node
26 — so CI is the first run of it.
This does not come with a local reproduction of the original flake. The chain is
circumstantial: the failure is a registry-state error, the code documents that
mid-flight teardown aborts the engine and can hang on macOS, streaming was the one
untracked path, and the flake is macOS-only and crosses files. Every piece fits, but
the proof is that the failure stops coming back.
Also corrects a comment in
async-stress.test.tsclaiming plainqueryAsyncis nottracked for drain.
withAbortTimeouthas tracked every async query since #53; thestale note sent this investigation down the wrong path for a while.
🤖 Generated with Claude Code
Note
Fix teardown race when
Session.closeorStreamCancelruns during an in-flight stream fetchChdbQueryStreamnow tracks the in-flight nativeStreamFetchin_inflight; the async iterator skips new fetches if the stream is already closed, and each fetch is registered as a pending native operation.cancel()defersStreamCanceluntil any in-flight fetch settles, preventing destruction of the stream handle while the worker is still reading it.Session.close()now callscancel()on any active stream before tearing down the connection, deferringCloseConnectionuntil the stream's in-flight fetch completes.cancel()-during-fetch andclose()-during-fetch races.Macroscope summarized 05d2db1.