Refuse to open a session while the default connection is busy - #78
Merged
Conversation
main went red right after #77 with the failure that PR was meant to end, so the stream teardown was not the cause. The full log said what the earlier ones had not: Error: Hook timed out in 30000ms. ChdbQueryError: chdb: a session (path=…) is active; … Error: Worker exited unexpectedly The hook is the suite-wide afterEach, whose first statement is `await _drainPendingOps()`. It sat for the full 30s, so `_closeAllSessions()` never ran, the session stayed open, and every later test in the file reported it as active. Six failures, one cause: a native op that never settled. Only one teardown can produce that and has no gate. `new Session()` calls CreateConnection, which reaches acquire_session_conn, which — because libchdb binds one data directory per process and the in-memory default holds the slot with an empty path — calls chdb_close_conn on the default connection outright. The whole pendingNativeOps discipline, close()'s deferral and #77's stream tracking included, protects session connections. Nothing protected this one, and the constructor never consulted the registry at all. The test that failed does exactly this race sixty times per run, and its name claimed the race was safe. The constructor is synchronous and cannot wait, so it refuses and names what to await. That trades a hang thirty seconds and one test file away for an error at the call site that caused it. Counted per connection, not globally: a session-bound query in flight is no reason to reject a second session on the same path, which is allowed. Only the three standalone entry points mark the default connection busy. The decrement is registered before any caller's handler, so code that awaits its own query sees zero on the next line instead of racing the bookkeeping. The test asserted something that was never exercised. HEAVY(60_000_000) is `count() FROM numbers(6e7)`, which the engine answers from the range without scanning — 5ms, against sleeps of 0-3ms. The query had usually finished before the session was opened, so most runs never entered the race at all, which is why a genuinely broken invariant looked intermittent. It now uses a query that forces per-row work (~120ms), asserts the refusal happened rather than tolerating either outcome, and checks the query is unaffected and a session opens the moment it drains. Full v3 suite before and after: the same nine failures either way (Layer 3 arrow-input and parametrized streaming, which want a different engine build than this machine has). Nothing else in the suite opens a session while a standalone query is outstanding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The refusal told callers to await the operation. Two situations have nothing left to await, and probing found both. An aborted or timed-out call rejects immediately while the engine keeps computing — the C ABI has no interrupt. The caller's promise is already settled, so "await it first" is advice they cannot follow, and the only way through was to retry the constructor until it stopped throwing. The second is the answer to whether this is only about the default connection. It is not. Moving between data directories never destroys anything — the engine rejects a second directory rather than evicting the first — so there is no corruption there. But close() returns before the connection is really gone when an operation is still running on it, and the next session at a different path then gets "only one active data directory per process; close the current session", which is precisely what the caller just did. drainPending() waits for both: native operations still on a libuv thread, and connections whose destruction close() deferred behind them. It loops rather than snapshotting, because a deferred teardown can be registered while the drain is already running. _drainPendingOps keeps working as the name the test harness uses. The deferred-teardown case is now refused in JS, ahead of the engine, so the misleading advice never reaches the caller. The teardown counter had the bug the count exists to prevent. Decrementing it in a .finally on the outer promise put it one microtask behind the teardown it guards, so a caller resuming in that window was told a connection was still alive after it had already gone — which failed the 100x close-during-query stress case on the first full run. It now decrements in the same tick as the teardown. The same hazard in the other direction is why the default-connection counter releases before any caller's handler. README documents the constraint, both waits, and the behaviour change: code that opened a session without awaiting its standalone queries used to close the busy connection and now gets an error instead. Full v3 suite: the nine pre-existing failures, unchanged, and three new contract tests covering await-then-open, abort-then-drain, and close-then-switch-directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The teardown guard rejected every new session while any deferred teardown was draining, which is wrong for the case the engine explicitly supports: several connections to one data directory coexist, and a deferred teardown releases a connection, not the directory. Opening a second session at the same path was refused for no reason. Tracked by normalized directory now, and only a different one is refused. A fresh temp directory counts as different, which it is. A sweep over the microtask and timer boundaries around both counters found no false positive or false negative in 132 checks per run, over five runs including one under four-way CPU contention — but its close-then-open case only ever used a different path, so it went straight past this. The same-path case is now covered there and in the contract tests. The sweep can rule things out only along the dimensions it varies. Also audited what could produce a false negative, which is the failure that matters: a native op on the default connection that nothing counts. Every chdbNode call lives in index.js, so Layer 3 and the connection surface cannot bypass the bookkeeping, and of the five native paths that reach get_default_conn only QueryAsync and InsertRawAsync are asynchronous. Both are counted. The rest hold the JS thread, so no session can open underneath them. The README abort example referenced an AbortController and a drainPending import that were not in it. Both snippets now run as written; both were executed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@wudidapaopao please review this PR |
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.
main went red right after #77 with the failure that PR was meant to end, so the
stream teardown was not the cause. The full log said what the earlier ones had not:
The hook is the suite-wide
afterEach, whose first statement isawait _drainPendingOps(). It sat for the full 30s, so_closeAllSessions()neverran, the session stayed open, and every later test in the file reported it as
active. Six failures, one cause: a native op that never settled.
Where
Only one teardown can produce that and has no gate:
libchdb binds one data directory per process and the in-memory default holds the
slot with an empty path, so opening a session has to evict it. The whole
pendingNativeOpsdiscipline —close()'s deferral, and #77's stream tracking —protects session connections. Nothing protected this one, and the constructor
never consulted the registry at all.
The test that failed does exactly this race sixty times per run, and its name
claimed the race was safe.
What changes
The constructor is synchronous and cannot wait, so it refuses:
A hang thirty seconds and one test file away becomes an error at the call site
that caused it.
Counted per connection, not globally: a session-bound query in flight is no reason
to reject a second session on the same path, which is allowed. Only the three
standalone entry points mark the default connection busy. The decrement is
registered before any caller's handler, so code that awaits its own query sees
zero on the next line instead of racing the bookkeeping.
The test never exercised what it claimed
HEAVY(60_000_000)iscount() FROM numbers(6e7), which the engine answers fromthe range without scanning — 5ms, against sleeps of 0-3ms. The query had
usually finished before the session was opened, so most runs never entered the
race, which is why a genuinely broken invariant looked intermittent.
It now uses a query that forces per-row work (~120ms), asserts the refusal
happened rather than tolerating either outcome, and checks that the query is
unaffected and a session opens the moment it drains.
Checked
Full v3 suite before and after: the same nine failures either way (Layer 3
arrow-input and parametrized streaming, which want a different engine build than
this machine has). Nothing else in the suite opens a session while a standalone
query is outstanding. The v2 mocha half cannot run locally — mocha's yargs
dependency breaks under Node 26 — so CI is the first run of it.
🤖 Generated with Claude Code
Note
Refuse to open a Session while the default connection has in-flight operations
new Session()now throws aChdbConnectionErrorif stateless default-connection queries or inserts are still running, or if a previously closed session at a different path is still releasing its connection.drainPending()function that resolves once all in-flight native operations and deferred connection teardowns have settled, giving callers a clean synchronization point before opening a session.pendingDefaultOpscounter tracks in-flight stateless operations;pendingTeardownPathstracks directories with deferred teardowns afterSession.close().awaitthe operation or calldrainPending()first.Macroscope summarized 20e7bfb.