diff --git a/.github/workflows/chdb-node-test.yml b/.github/workflows/chdb-node-test.yml index 0364900..81bab5c 100644 --- a/.github/workflows/chdb-node-test.yml +++ b/.github/workflows/chdb-node-test.yml @@ -10,9 +10,10 @@ on: # update_libchdb.sh pins a published chdb-core libchdb across all four target # platforms, so the full v2 + v3 suite is expected green here. The pin is -# currently v26.7.3, the first stable release to export the Durable V1 ABI -# (chdb_backup_database_n / chdb_restore_database_n / chdb_classify_query_n); -# v26.7.2-rc.2 was the candidate that introduced them. +# currently the v26.11.0-rc.2 release candidate. v26.7.3 was the first stable +# release to export the Durable V1 ABI (chdb_backup_database_n / +# chdb_restore_database_n / chdb_classify_query_n); v26.7.2-rc.2 was the +# candidate that introduced them. # Harden the registry fetch so a transient reset (e.g. ECONNRESET during # install) retries instead of reddening the job. Combined with the per-job diff --git a/README.md b/README.md index 98358e3..95f8e04 100644 --- a/README.md +++ b/README.md @@ -200,7 +200,7 @@ and the sync policy with `@clickhouse/client`. > is injected; Node callers get a default engine over this package's own addon > from `chdb/durable/node`. > -> Requires an engine exporting the durable ABI — currently `26.7.3`. +> Requires an engine exporting the durable ABI — currently `26.11.0-rc.2`. > Compatibility is a floor rather than an equality: an object records > `min_reader` and `backup_format`, and any engine at or above that floor opens > it. An object written by `26.7.2-rc.2` stays readable on `26.7.3` and later. diff --git a/docs/design/durable-control-plane.md b/docs/design/durable-control-plane.md index 710477d..e4efdc0 100644 --- a/docs/design/durable-control-plane.md +++ b/docs/design/durable-control-plane.md @@ -724,7 +724,7 @@ this package: 1. Publish `@chdb/lib-` packages built from a tree that binds the durable ABI. `optionalDependencies` and `update_libchdb.sh` already name - `26.7.3-stable.1`, and the release workflow derives the subpackage version + `26.11.0-rc.2.1`, and the release workflow derives the subpackage version from the latter — but nothing has been tagged since, so that version is not on npm at all. The newest published is `26.7.0-stable.1` (main `chdb@3.3.0` pins it), which carries an addon from before this work and exports none of diff --git a/index.js b/index.js index ef94836..950d896 100644 --- a/index.js +++ b/index.js @@ -6,6 +6,7 @@ const chdbNode = require('./dist/loader.js').loadNative(); const { mkdtempSync, rmSync, realpathSync } = fs; const { join, resolve: resolvePath } = require('path'); const os = require('os'); +const { isMainThread } = require('worker_threads'); const { ChdbConnectionError, ChdbClosedError, @@ -568,12 +569,33 @@ let exitHookInstalled = false; function ensureExitHook() { if (exitHookInstalled) return; exitHookInstalled = true; - process.on('exit', () => { + const teardown = () => { for (const s of openSessions) { try { s.close(); } catch (_) { /* best effort */ } } - }); + // Then what the sessions do not cover: the standalone default connection, + // which the addon opens on first use, and any connection close() had to + // defer. The addon closes the idle ones and stops the engine here, before + // C exit(), because its atexit backstop runs after statics the engine + // still needs are destroyed: a process.exit() with MergeTree merges in + // flight on the default connection crashed there. As with the sessions, an + // 'exit' listener that runs after this one finds the engine closed. Main + // thread only: the connections and the engine are process-wide, and a + // worker that exits must not close them under the main thread. + if (isMainThread && typeof chdbNode.ExitTeardown === 'function') { + try { chdbNode.ExitTeardown(); } catch (_) { /* best effort */ } + } + }; + process.on('exit', teardown); + // Deno.exit() skips the process 'exit' event and dispatches 'unload' + // instead. Running both is harmless: the second finds nothing left to do. + if (typeof Deno !== 'undefined' && typeof globalThis.addEventListener === 'function') { + globalThis.addEventListener('unload', teardown); + } } +// Installed up front rather than with the first Session: the standalone +// functions reach the engine through that default connection without one. +ensureExitHook(); // Session class with connection-based path handling const SESSION_SIGNALS = ['SIGINT', 'SIGTERM']; diff --git a/lib/chdb_node.cpp b/lib/chdb_node.cpp index 858de56..35c49b4 100644 --- a/lib/chdb_node.cpp +++ b/lib/chdb_node.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -201,8 +202,9 @@ class ConnGuard { static void erase_arrow_tables_for_conn(chdb_connection *conn); static void clear_all_arrow_tables(); -// Close every live connection. atexit backstop (the JS layer closes sessions on -// 'exit' too); also used so a leaked connection never outlives the process. +// Close every live connection. atexit backstop (the JS layer closes sessions and +// calls ExitTeardownWrapper on 'exit' first); also used so a leaked connection +// never outlives the process. static void hard_close_all() { std::lock_guard lk(g_reg_mu); clear_all_arrow_tables(); @@ -214,20 +216,50 @@ static void hard_close_all() { g_reg.boundKey.clear(); } -// Node's environment cleanup closes connections, but the engine may still own -// background threads when C++ static destructors run. On macOS with core -// v26.9.0, a CLI that queried once could hang indefinitely inside exit(). -// Run this process-wide backstop before the engine's own exit handlers. +// The atexit backstop: close whatever is still open when the process reaches C +// exit(), then stop the engine, so it owns no background threads while C++ +// static destructors run. On macOS with core v26.9.0, a CLI that queried once +// could hang indefinitely inside exit() without it. +// +// It is only the fallback. index.js calls ExitTeardownWrapper (below) from the +// process 'exit' event, which closes everything it safely can before exit(). +// atexit handlers and static destructors run in reverse order of registration, +// and this one is registered once, at the first connect, so it runs after two +// things registered later: the exit teardown the engine registers on every +// start (since chdb-core v26.11.0-rc.2), so after an engine restart that runs +// first, and every function-local static constructed after the first connect, +// including ones a query or a MergeTree merge constructed. Closing a connection +// here tears the server down onto those destroyed statics; with merges in +// flight, a process.exit() on the default connection crashed in this handler. +// +// chdb_shutdown() is one of them: it keeps a function-local static mutex, +// constructed by its first call. Once ExitTeardownWrapper has called it, a +// second call from here locks that mutex after its destructor ran, which macOS +// reports as "mutex lock failed: Invalid argument" and aborts. So the exit +// paths call it once between them. +static std::atomic g_exit_shutdown_called{false}; + +static void shutdown_engine_once() { + if (!g_exit_shutdown_called.exchange(true)) (void)chdb_shutdown(); +} + static void shutdown_on_process_exit() { hard_close_all(); - (void)chdb_shutdown(); + shutdown_engine_once(); } +// Set at the first connect. Guarded by g_reg_mu: both callers hold it. +static bool g_atexit_registered = false; + +// Environments that loaded the addon and have not been torn down: the main +// thread, and each worker_threads Worker that required it. Init counts one in, +// its env cleanup hook counts it out. ExitTeardownWrapper reads it. +static std::atomic g_live_envs{0}; + static void ensure_atexit() { - static bool done = false; - if (!done) { + if (!g_atexit_registered) { std::atexit(shutdown_on_process_exit); - done = true; + g_atexit_registered = true; } } @@ -253,8 +285,9 @@ static chdb_connection *get_default_conn(char **error_message) { g_reg.defaultConn = c; g_reg.boundKey = ""; g_reg.conns.insert(c); - // Register only after chdb_connect initialized the engine: atexit handlers - // run in reverse registration order, so shutdown must precede its teardown. + // Register only after chdb_connect started the engine, so the backstop runs + // before the exit teardown that start registered (see shutdown_on_process_exit + // for what it still runs after). ensure_atexit(); return c; } @@ -1760,6 +1793,56 @@ Napi::Value EngineVersionWrapper(const Napi::CallbackInfo &info) { return Napi::String::New(env, version); } +// Exit teardown, called by index.js from the main thread's process 'exit' +// event: the last point before Node calls C exit(), where every static is still +// alive (shutdown_on_process_exit explains why the atexit backstop is too late). +// index.js has closed its sessions by then. This closes what is left, the +// standalone default connection above all, and then stops the engine, so +// nothing of chDB runs into Node's own teardown. +// +// A connection an async call is still running on (the in-flight count) stays +// open: closing it under the call aborts the engine, and Node joins that libuv +// thread only after this returns. For those, the backstop is registered once +// more. exit() then runs it first: after Node has joined the thread, and before +// the statics that the first registration runs after. +// +// While another environment has the addon loaded (a worker_threads Worker), +// nothing is closed: that thread may be using any connection, its own sessions +// or the shared default, and synchronous calls are not in the in-flight count. +// Node stops it only after this returns, so closing one here freed it under +// the worker's next query. The backstop is registered again instead; by the +// time exit() runs it, Node has stopped and joined every worker. +Napi::Value ExitTeardownWrapper(const Napi::CallbackInfo &info) { + bool started; + bool busy = g_live_envs.load() > 1; + { + std::lock_guard lk(g_reg_mu); + started = g_atexit_registered; + for (auto it = g_reg.conns.begin(); !busy && it != g_reg.conns.end();) { + chdb_connection *c = *it; + if (c && inflight_locked(*c) > 0) { + busy = true; + ++it; + continue; + } + if (c) { + erase_arrow_tables_for_conn(c); + chdb_close_conn(c); + } + if (c == g_reg.defaultConn) g_reg.defaultConn = nullptr; + it = g_reg.conns.erase(it); + } + if (g_reg.conns.empty()) g_reg.boundKey.clear(); + } + if (started) { + if (busy) + std::atexit(shutdown_on_process_exit); + else + shutdown_engine_once(); + } + return info.Env().Undefined(); +} + Napi::Object Init(Napi::Env env, Napi::Object exports) { // Export the functions exports.Set("Query", Napi::Function::New(env, QueryWrapper)); @@ -1796,10 +1879,19 @@ Napi::Object Init(Napi::Env env, Napi::Object exports) { exports.Set("ArrowRegisterColumns", Napi::Function::New(env, ArrowRegisterColumnsWrapper)); exports.Set("ArrowUnregister", Napi::Function::New(env, ArrowUnregisterWrapper)); - // Most-reliable exit cleanup: close every live connection on env teardown, - // ahead of (and complementary to) the std::atexit backstop. Both call the - // idempotent hard_close_all, so running twice is harmless. - env.AddCleanupHook([] { hard_close_all(); }); + // For index.js's process 'exit' hook. + exports.Set("ExitTeardown", Napi::Function::New(env, ExitTeardownWrapper)); + + // Close every live connection on env teardown, ahead of the std::atexit + // backstop. Both call the idempotent hard_close_all, so running twice is + // harmless. Node runs no cleanup hooks on process.exit(), which is one reason + // index.js closes on the 'exit' event instead (ExitTeardownWrapper). The + // hook also counts this environment out of g_live_envs. + g_live_envs.fetch_add(1); + env.AddCleanupHook([] { + g_live_envs.fetch_sub(1); + hard_close_all(); + }); return exports; } diff --git a/package.json b/package.json index ad849c9..125808d 100644 --- a/package.json +++ b/package.json @@ -121,10 +121,10 @@ "node-gyp-build": "^4.6.0" }, "optionalDependencies": { - "@chdb/lib-darwin-arm64": "26.7.3-stable.1", - "@chdb/lib-darwin-x64": "26.7.3-stable.1", - "@chdb/lib-linux-arm64-gnu": "26.7.3-stable.1", - "@chdb/lib-linux-x64-gnu": "26.7.3-stable.1" + "@chdb/lib-darwin-arm64": "26.11.0-rc.2.1", + "@chdb/lib-darwin-x64": "26.11.0-rc.2.1", + "@chdb/lib-linux-arm64-gnu": "26.11.0-rc.2.1", + "@chdb/lib-linux-x64-gnu": "26.11.0-rc.2.1" }, "peerDependencies": { "@clickhouse/client": ">=1.23.0", diff --git a/test/v3/engine-exit.test.ts b/test/v3/engine-exit.test.ts index 5eca9a4..f3f0cd4 100644 --- a/test/v3/engine-exit.test.ts +++ b/test/v3/engine-exit.test.ts @@ -1,5 +1,7 @@ import { describe, expect, it } from 'vitest' import { spawnSync } from 'node:child_process' +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' import { join } from 'node:path' describe('engine process teardown (chdb-node#97)', () => { @@ -21,4 +23,130 @@ describe('engine process teardown (chdb-node#97)', () => { expect(child.stdout.trim()).toBe('42') expect(child.stderr).toBe('') }, 20_000) + + // Up to chdb-core v26.11.0-rc.1, chdb_shutdown() refused once a MergeTree + // table had been used: the background executors outlived the last + // connection, and rc.1 printed "chDB global pool shutdown refused" and + // "chDB shutdown failed" on every such exit. rc.2 destroys them on shutdown + // (chdb-core#312). The session stays open, so the exit hook closes it. + it('exits silently after using a MergeTree table', () => { + const entry = join(process.cwd(), 'index.js') + const script = ` + const { Session } = require(${JSON.stringify(entry)}); + const s = new Session(); + s.query('CREATE TABLE t (id UInt64) ENGINE = MergeTree ORDER BY id'); + s.query('INSERT INTO t SELECT number FROM numbers(1000)'); + process.stdout.write(s.query('SELECT count() FROM t', 'CSV')); + ` + const child = spawnSync(process.execPath, ['-e', script], { + encoding: 'utf8', + timeout: 15_000, + }) + expect(child.error).toBeUndefined() + expect(child.status).toBe(0) + expect(child.stdout.trim()).toBe('1000') + expect(child.stderr).toBe('') + }, 20_000) + + // The standalone functions' default connection had no 'exit' hook, so a + // process.exit() left it to the addon's atexit backstop, inside C exit(), + // after statics the server teardown still uses were destroyed. With merges + // in flight that crashed: SIGSEGV, or "mutex lock failed: Invalid argument", + // each after chDB's crash handler waited about 30 s. + it('exits cleanly from process.exit() with merges in flight on the default connection', () => { + const entry = join(process.cwd(), 'index.js') + const script = ` + const { query } = require(${JSON.stringify(entry)}); + query('CREATE TABLE t (id UInt64, s String) ENGINE = MergeTree ORDER BY id'); + let merging = 0; + for (let i = 0; i < 200 && merging === 0; i++) { + query('INSERT INTO t SELECT number, toString(number) FROM numbers(' + i * 20000 + ', 20000)'); + merging = Number(query('SELECT count() FROM system.merges', 'CSV').trim()); + } + process.stdout.write(String(merging)); + process.exit(0); + ` + const child = spawnSync(process.execPath, ['-e', script], { + encoding: 'utf8', + timeout: 20_000, + killSignal: 'SIGKILL', + }) + expect(child.error, child.stderr).toBeUndefined() + expect(child.signal, child.stderr).toBeNull() + expect(child.status, child.stderr).toBe(0) + // Only meaningful if a merge was running when the exit began. + expect(Number(child.stdout)).toBeGreaterThan(0) + expect(child.stderr).toBe('') + }, 30_000) + + // The same with a query still running on the default connection, which the + // 'exit' hook must not close under it. It is closed in exit() instead, by + // the backstop registered again from the hook so that it runs before the + // destroyed statics. Without that this crashed or hung in exit(). + it('exits cleanly from process.exit() with an insert still running on the default connection', () => { + const entry = join(process.cwd(), 'index.js') + const script = ` + const { query, queryAsync } = require(${JSON.stringify(entry)}); + query('CREATE TABLE t (id UInt64, s String) ENGINE = MergeTree ORDER BY id'); + let merging = 0; + for (let i = 0; i < 200 && merging === 0; i++) { + query('INSERT INTO t SELECT number, toString(number) FROM numbers(' + i * 20000 + ', 20000)'); + merging = Number(query('SELECT count() FROM system.merges', 'CSV').trim()); + } + let done = false; + queryAsync('INSERT INTO t SELECT number, toString(number) FROM numbers(10000000)', { format: 'CSV' }) + .then(() => { done = true; }, () => { done = true; }); + setTimeout(() => { + process.stdout.write(merging + ' ' + (done ? 'done' : 'running')); + process.exit(0); + }, 20); + ` + const child = spawnSync(process.execPath, ['-e', script], { + encoding: 'utf8', + timeout: 60_000, + killSignal: 'SIGKILL', + }) + expect(child.error, child.stderr).toBeUndefined() + expect(child.signal, child.stderr).toBeNull() + expect(child.status, child.stderr).toBe(0) + // Only meaningful if merges ran and the insert had not finished at exit. + const [merging, insert] = child.stdout.split(' ') + expect(Number(merging)).toBeGreaterThan(0) + expect(insert).toBe('running') + expect(child.stderr).toBe('') + }, 70_000) + + // A worker_threads Worker holds connections of its own, and Node stops it + // only after the main thread's 'exit' event. Closing them from that event + // freed the worker's Session under its next synchronous query (SIGSEGV). + it('exits cleanly from process.exit() while a worker runs synchronous Session queries', () => { + const entry = join(process.cwd(), 'index.js') + const dir = mkdtempSync(join(tmpdir(), 'chdb-exit-worker-')) + try { + const workerCode = ` + const { Session } = require(${JSON.stringify(entry)}); + const s = new Session(${JSON.stringify(join(dir, 'data'))}); + s.query('SELECT 1'); + require('node:worker_threads').parentPort.postMessage('started'); + for (;;) s.query('SELECT sum(number) FROM numbers(100000000)'); + ` + const script = ` + const { Worker } = require('node:worker_threads'); + require(${JSON.stringify(entry)}); + const w = new Worker(${JSON.stringify(workerCode)}, { eval: true }); + w.on('message', () => setTimeout(() => process.exit(0), 100)); + ` + const child = spawnSync(process.execPath, ['-e', script], { + encoding: 'utf8', + timeout: 20_000, + killSignal: 'SIGKILL', + }) + expect(child.error, child.stderr).toBeUndefined() + expect(child.signal, child.stderr).toBeNull() + expect(child.status, child.stderr).toBe(0) + expect(child.stderr).toBe('') + } finally { + rmSync(dir, { recursive: true, force: true }) + } + }, 30_000) }) diff --git a/tests/clickhouse-js/skip_list.json b/tests/clickhouse-js/skip_list.json index 02a2167..31ff365 100644 --- a/tests/clickhouse-js/skip_list.json +++ b/tests/clickhouse-js/skip_list.json @@ -226,6 +226,26 @@ "file": "packages/client-node/__tests__/integration/node_streaming_e2e.test.ts", "test": "[Node.js] streaming e2e > should stream a Parquet file", "reason": "Streams a Parquet file into chdb via `INSERT FORMAT Parquet`; chdb's `ParquetV3BlockInputFormat` rejects this specific fixture with `apache::thrift::protocol::TProtocolException: invalid TType`. The server's older Parquet input format accepts it. Engine-version divergence in Parquet decoder." + }, + { + "file": "packages/client-common/__tests__/integration/error_parsing.test.ts", + "test": "ClickHouse server errors parsing > returns \"unknown identifier\" error", + "reason": "Engine-version divergence, not a chdb gap. Since chdb-core v26.9.0 (ClickHouse 26.9.2.1; v26.7.3 is unaffected) the analyzer's message ends the identifier with `. In scope ...` (capital I) where this tag's case-sensitive regex expects ` in scope`. Code 47 and type UNKNOWN_IDENTIFIER still parse correctly; only the message match misses. Upstream accepts both spellings since ClickHouse/clickhouse-js#1025 (client-1.24.0). Drop this entry when syncedAgainst moves to that release or later." + }, + { + "file": "packages/client-common/__tests__/integration/error_parsing.test.ts", + "test": "ClickHouse server errors parsing > returns \"unknown table\" error", + "reason": "Same `. In scope` message change as the entry above. Code 60 and type UNKNOWN_TABLE still parse correctly. Fixed upstream by the same clickhouse-js#1025." + }, + { + "file": "packages/client-common/__tests__/integration/date_time.test.ts", + "test": "DateTime > DateTime64(3) > should insert DateTime64(3) and get it back (different timezone)", + "reason": "Engine-version divergence, not a chdb gap. Since chdb-core v26.9.0 (ClickHouse 26.9.2.1; v26.7.3 is unaffected) an epoch-milliseconds JSON number inserted into DateTime64(3) saturates to 9999-12-31 23:59:59.000 instead of being read as ticks; the same value sent as a JSON string still parses. chdb-node never sends a Date this way (it formats Date values as strings), so this only reaches callers who put raw epoch-ms numbers into JSONEachRow themselves. Upstream skips this test on such servers since clickhouse-js#1025 (isDateTime64FromJSONNumberBroken)." + }, + { + "file": "packages/client-common/__tests__/integration/data_types.test.ts", + "test": "data types > should work with custom JSON handling (BigInt and Date)", + "reason": "Same epoch-ms DateTime64(3) saturation as the entry above: the test's custom JSON.stringify sends a Date as getTime(). Upstream skips it on such servers since clickhouse-js#1025." } ] } diff --git a/update_libchdb.sh b/update_libchdb.sh index 2dd8753..8f491ec 100755 --- a/update_libchdb.sh +++ b/update_libchdb.sh @@ -11,20 +11,21 @@ cd "$(dirname "$0")" # Fail fast so a bad download never silently leaves a stale/partial libchdb. set -e -# Pre-release engine carrying the Durable V1 ABI: chdb_backup_database_n, +# The engine has to export the Durable V1 ABI: chdb_backup_database_n, # chdb_restore_database_n and the chdb_classify_query_n that reports statement # count and target-database proof. chdb/durable's engine adapter cannot be built -# without them, and no stable release exports them yet. +# without them. v26.7.3 is the first stable release that exports them. # -# Note chdb_version() on this build returns the full "26.7.2-rc.2", suffix and -# all. Durable records that string in head.engine.version and opens only on an -# exact match, so an object written against this RC is not readable by the -# eventual 26.7.2 stable. That is the contract working as designed (V1 pins the -# engine exactly), but it means RC-era objects are throwaway. +# The pin is currently a release candidate, and chdb_version() on it returns the +# full "26.11.0-rc.2", suffix and all. Durable records that string in +# head.engine.version and raises min_reader to it on every write. A pre-release +# sorts below the release it leads to, so an object written against this RC +# stays readable by the eventual stable, but every earlier engine (26.11.0-rc.1, +# 26.9.x and before) is refused it with engine_incompatible from then on. # # Keep the pin on its own line and literal: the release check greps for it when # it proposes a bump. -CHDB_ENGINE_PIN=v26.7.3 +CHDB_ENGINE_PIN=v26.11.0-rc.2 # CHDB_ENGINE_VERSION overrides the pin, which is how the release check runs the # suite against an engine this repository has not adopted yet. It is a different @@ -52,7 +53,7 @@ LATEST_RELEASE="${CHDB_ENGINE_VERSION:-$CHDB_ENGINE_PIN}" # ERR_DLOPEN_FAILED instead. # # The publish and cleanroom workflows read CHDB_LIB_VERSION from here. -LIBCHDB_NPM_VERSION=26.7.3-stable.1 +LIBCHDB_NPM_VERSION=26.11.0-rc.2.1 # Download the correct version based on the platform case "$(uname -s)" in