feat(qwp)!: add browser and Node.js QWP client - #62
Merged
Merged
Conversation
…lock QwpIngressSession.connect() receives an AbortSignal and passed it to exactly two places: the eager initial connection, and the non-reconnecting branch. The eager connection is skipped whenever a replay store or background store-and-forward is configured -- precisely the configurations that take a slot lock -- and QwpReconnectingIngressConnection.connect() had no signal parameter at all. So close() aborted nothing on the one path where a connect owns a lock. The result: close() resolved, with no error and no warning, while the abandoned connect went on holding the slot for the rest of its connect budget. Against a peer that accepts TCP and never answers the upgrade -- a stalled proxy or load balancer -- that is the full connect_timeout, and 30s with a reconnect budget; a second sender on the same directory failed with QwpReplayStoreLockedError naming its own process. The abandoned session also kept doing real work after shutdown, re-sending a journaled frame and, in one shape, renaming a slot to .unreplayable-N. QWP.md says the journal "takes an exclusive lock when it is loaded and holds it until the sender or session closes", and the signal's own documentation says a connect still negotiating "can be torn down instead of outliving the sender by up to its connect/auth deadline". Both described the intent, not the behaviour; connectAbort itself is new in this branch, so this is incompletely-wired new work rather than legacy. The signal now reaches that connect. It is checked before the store is loaded, again once the lock is held, and an abort during the connect closes the connection -- which closes the store and releases the lock. A lock can still outlive close() by an in-flight load, which is bounded by the load rather than by the connect budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The row-count half of "documents the auto-flush defaults the sender actually applies" staged 999 rows against a live 100ms auto-flush interval. The work is about 2ms, but it is 999 awaits, and on a contended two-core CI runner the event loop can take longer than the interval to get through them -- so the interval trigger fired mid-loop and the assertion saw a partial row count: "expected [ 773 ] to deeply equal []". The race is inherent to the test rather than new, but this branch's added suites raised the parallel load enough to lose it. Reproduced deterministically by injecting a 150ms stall into the loop, which fails identically with "expected [ 302 ] to deeply equal []" and passes with the clock frozen. Only Date is faked, which is all the interval check reads, so the flush machinery's own timers keep working. The interval half already drives a faked clock explicitly and is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The client now ships a browser build alongside the Node.js one, so naming it after a single runtime no longer describes who can use it. The sibling clients are named for the language their consumers write, and consumers here write JavaScript or TypeScript -- both served by the same JavaScript artifact and its type declarations, so neither is excluded by the broader name. This changes the human-facing name only: the package description, the typedoc title, the package documentation header, README prose, the examples manifest, and CLAUDE.md. The published package name, every import specifier, the tsconfig path mappings, and the repository URLs are untouched, so nothing an existing consumer resolves against moves. Two comments that had settled on "TypeScript client" are folded in here too, so the release that raises the question does not leave a third name behind. References that genuinely mean the Node.js runtime keep it: the store-and-forward locking section reasons about runtimes rather than products, and the pooled Node client and the orphan drainer are Node-only APIs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmYejbzDF32mYojAwbTqM4
The Enterprise lane is now build-and-test-e2e-javascript-client and takes javascriptClientCommit / javascriptClientPrNumber, following this repository's rename away from Node.js. The dispatch resolves the pipeline by name and sends those parameters, so both sides have to agree or the lookup fails. The job stays gated behind ENTERPRISE_E2E_ENABLED, so nothing runs until the pipeline is renamed in Azure DevOps to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CmYejbzDF32mYojAwbTqM4
Seven observability callbacks were invoked inside a synchronous-only try/catch. That guards a thrown error, but an async callback returns a promise: when it rejects, the rejection escapes the try/catch and Node >= 15 turns an unhandled rejection into process termination by default. A single async onEvent, onError, onSenderError, onRecoveryQuarantine or onRecoveryDataLoss -- all reachable from the plain public API -- took the whole process down. The orphan-drain path was the decisive one. QWP.md says callbacks are "placed on bounded asynchronous inboxes and never invoked inside ACK, reconnect, or orphan-recovery protocol stacks ... Callback failures are contained", and the drainer does feed reconnect events through QwpNotificationDispatcher, whose dispatchOne already contained a returned promise. But the wrapper installed as the dispatcher's handler swallowed the sync throw and returned undefined, so that check had nothing to attach to and the user's promise orphaned through the very inbox meant to hold it. The containment already existed twice -- QwpNotificationDispatcher and a private safelyInvoke in ingress-session -- so this consolidates both onto one helper, src/_qwp/_internal/safe-callback.ts. safelyInvoke() contains a synchronous throw and a rejected promise alike, routing either to a guarded onFailure that can never re-escape, and uses the portable then(undefined, ...) rather than catch() so a bare thenable is handled too. All seven sites now go through it, each keeping its own failure behaviour: swallow, log once, or fall back to the default handler. Verified as a real process rather than under Vitest, which masks the crash by handling unhandled rejections itself: on Node 20.11.0 the old sync-only pattern exits 1 and safelyInvoke exits 0 with onFailure seeing the rejection. Added a safe-callback suite and an async-rejection case to the dispatcher suite, both asserting no unhandledRejection fires. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Nothing asserted that a wss:// producer verifies the server certificate or that
its Authorization header carries the operator's credentials unchanged. Both
fail silently -- a disabled check still connects, transposed Basic credentials
still form a header -- so mutating either the TLS agent or the authorization
encoding left the whole suite green. The nearest assertion only checked that
ingress.agent exists, never its rejectUnauthorized, ca, pfx or passphrase.
This is a departure from the client's own standard rather than a general gap:
sender.transport.test.ts exercises ILP TLS for real against test/certs, so the
fixtures a QWP test needs already exist.
The new suite covers both wss construction paths. The documented `wss::`
connect string, resolved by parseQwpNodeClientConfig(), is asserted for
rejectUnauthorized, a custom ca, and a pfx trust store with its passphrase
across tls_verify on/unsafe_off and tls_roots, plus the unconfigured case that
must build no agent at all so node's verifying default applies. The programmatic
`new Sender({ protocol: "wss", ... })` object, handled by sender.ts, is asserted
by capturing the ingress options handed to createQwpNodeSender: the agent's ca
and rejectUnauthorized, the Basic header's username:password order, and the
Bearer prefix.
Each admitted mutation was re-applied and now turns the suite red: disabling
verification in createTlsAgent fails three connect-string tests, the same in the
sender.ts agent fails two, and swapping the Basic order or dropping the Bearer
prefix fails two. The unsafe_off and tls_verify=false cases stay green under the
TLS mutations, so the tests assert the intent rather than a constant.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dateColumn and fixedDecimalColumn -- the shared body of decimal64Column, decimal128Column and decimal256Column -- returned on a nullish value through a raw === null || === undefined check, bypassing omitsNullish. So on a nullish row they skipped the sender availability, row state and column name checks every other setter runs, and the decimals also skipped the scale check. A misspelled or over-long name, or a bad scale constant, then surfaced only on the rows that happened to carry a value and stayed silent on the rest -- which is how a typo reaches production. An inventory of all 26 setters confirms these four were the only ones left: the rest already route a nullish value through omitsNullish, and doubleColumn is safe because it delegates to floatColumn. This is the same class commit 266438f fixed for the setters it covered, and README.md documents the nullish rule as shared across QuestDB clients, so they must agree. dateColumn now goes through omitsNullish like its sibling timestampColumn. fixedDecimalColumn hoists the scale check above the gate -- the scale describes the column, not this row's value -- then routes the name through omitsNullish, exactly as decimalColumn already does. The regression test written for this bug covered seven setters and omitted all four; it now asserts dateColumn's name, each fixed-width decimal's scale constant, and decimal64Column's over-long name all raise on a nullish value, and that a valid nullish dateColumn/decimal64Column call is still omitted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
qwpColumnNameKey builds its result one code unit at a time, and it ran twice per cell: once when the cell is staged, then again at flush inside getOrCreateColumn -- even though buildTable iterates row.columns, a Map already keyed by exactly that value. Two changes remove the redundant work. buildTable now iterates the map entries and passes the key it already holds into getOrCreateColumn, which takes it as an optional third argument defaulting to qwpColumnNameKey(name), so every other caller is unchanged. That key is provably the one getOrCreateColumn would have computed: each entry's stored canonical name is one whose key is the map key it lives under. Separately, qwpColumnNameKey gains an all-lower-case-ASCII fast path -- a name of only lower-case-stable code units is returned unchanged, and the first upper-case ASCII or non-ASCII code unit resumes the per-code-unit mapping from the stable prefix -- so the common name skips the rebuild entirely. A local before/after run of the shipped benchmarks (benchmarks/sender.bench.ts, build and encode) measured roughly +33% on trades, +12% on wide and +8% on sparse; trades is the primary ingest path. The surface is new in this branch, so this is measurable headroom rather than a regression from an earlier release. A new identifiers suite asserts the fast path is byte-identical to the per-code-unit reference across upper-case, non-ASCII, surrogate-pair and U+0130 inputs, so two spellings of one column still collide on the same key. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g it
README says the nullish rule applies to the compiled QWP writers and that a QWP
row whose every value is nullish is sent with no columns; QWP.md says regular
fields may all be null and a designated timestamp is required only "when
present". But for a schema without a designated timestamp,
encodeCompiledWriterRow threw QwpWriterRowError: row must contain at least one
non-null value -- a string that appears nowhere in README.md, QWP.md, any test
or any example. The exact fluent analogue, table("t").symbol("side", null)
.atNow(), is accepted and encodes a real frame, so the two APIs disagreed on
documented behaviour.
Dropping the columns.size === 0 guard makes the writer send the all-nullish row
with no columns, exactly as the fluent path does. The guard only ever bit a
timestamp-less schema: a designated timestamp is required earlier in the same
function -- it throws when nullish and stages a column when present -- so a
schema that declares one always reaches this point with at least one column. A
column-less row encodes cleanly; the encoder's non-null-row check is a
per-column count consistency check that a row with no columns has nothing to
fail.
Two tests cover it: a timestamp-less writer now stages and flushes an all-null
row as a zero-column, real-encoding frame, and a schema that declares a
designated timestamp still rejects a nullish one -- the requirement the dropped
guard sat next to.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A ws/wss connect string that carries max_datagram_size or multicast_ttl was rejected with the hint "(applies to legacy http/tcp/udp transports only)". But those two keys are UDP-only: http and tcp reject them as well, with "max_datagram_size and multicast_ttl are only supported for QWP UDP transport". So the hint sent the user to two more protocols that also refuse the key. The same string is correct for the four sibling keys it is shared with, which really do span all three transports, and options.ts already had the right wording; only these two entries were wrong. The test pinned the wrong string. The hint now reads "(applies to the legacy udp transport only)" for both keys, and the test asserts it for max_datagram_size and multicast_ttl alike. Neither key was documented anywhere a reader would look, even though the auto_flush_bytes entry cites max_datagram_size as its own default. A "UDP specific options" block in the SenderOptions reference -- the configuration reference README points to -- now documents both, with defaults (1400 and 0), the 0-255 TTL range, and that only udp accepts them; and the QWP.md UDP prose now spells the key names instead of only describing the values. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
releaseStagedRows() returns 0 on a staging-generation mismatch, which is correct for retiring pending rows -- reset() has already zeroed those counters, so subtracting again would drive pendingRows negative (the bug 0a1fccf fixed). But that one return value also fed totalRowsPublished += sentRows, so a reset() landing while a flush awaited its publication boundary lost the count for rows whose frames had already entered the ingress session -- exactly what the field documents. The counter then skewed permanently low: a five-row flush interrupted this way put all five rows on the wire and reported totalRowsPublished 0. The published count and the retired count are different questions. The flush now takes the published count from its own snapshots -- the rows it sent, regardless of a concurrent generation bump -- and releaseStagedRows() keeps doing only the pending retirement. The same retired-count value was wrong for three siblings that shared it, all now reading the published count: the flush debug log, the deferred-transaction row tally (the open transaction still holds those rows), and the > 0 guard that counts a committed transaction. The no-reset path is unchanged, since there the two counts are equal. A test holds a flush at its publication boundary, drops a reset() between "frame entered the session" and "rows retired", and asserts all five rows reached the wire with totalRowsPublished at five and pendingRows at zero rather than negative. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
scheduleMaintenance() rejected every parked appender with the maintenance failure and then, on the very next line, scheduled the retry that makes the rejection unnecessary. A background segment trim that briefly fails -- a read-only or full filesystem, a restarted maintenance worker -- therefore rejected a parked store-and-forward append with a retryable "could not trim QWP store-and-forward segment" error a few milliseconds into its append deadline, even though the retry self-heals about a second later and the identical append then succeeds. totalAppendTimeouts stays zero, so it is not the deadline error a caller watches for, and it contradicts the sf_dir wait contract QWP.md states: the journal ceiling is the one error a producer sees. The retried batch already releases parked appenders through signalCapacity() on success, and each appender keeps its own append deadline, so a permanent failure still ends in the typed append timeout rather than hanging. Dropping the reject leaves them parked for that retry. A released appender re-runs appendOnce() through enqueue(), which is serialized behind the maintenance batch, so it runs only after the batch has cleared the failure -- it never observes the stale one at assertReady(). The checkpoint sibling still rejects its waiters, correctly: that class has no retry outside durability "periodic", which is not the connect-string default. A test parks an append at capacity, fails the trim once, and asserts the append resolves when the retry frees space rather than being rejected, with totalAppendTimeouts still zero. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
markerCounts scanned the whole .sfa file for bytes in the A-Z range, so it also tallied framing bytes: the segment header ends in a microsecond wall-clock timestamp and each frame header carries a CRC32C. Whenever one of those bytes happened to equal 'A' (0x41) or 'B' (0x42) on a given run -- about 1.5% of the time per segment -- the durability assertion saw 321 instead of 320 and failed. The append path already rejects the reclaimed holder before it writes anything (assertReady throws QwpReplayStoreLockLostError ahead of the segment write), which the +1 rather than +64 discrepancy confirms, so no payload byte ever leaked; the flake was purely in the test helper. Walk the frame framing instead and count only payload bytes, which are pure marker fill by construction, making the check exact and deterministic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An over-range wire sequence was clamped to the newest in-flight frame, so an OK(seq) beyond the last frame sent retired every in-flight frame, advanced the watermark, and trimmed journal records the server never confirmed -- silently deleting unacknowledged data. The NACK path shared the clamp and charged the poison strike to the tail frame instead of the head. Reject an over-range sequence as QwpProtocolError, matching the null and negative guards above it and sitting before the OK/NACK branch so both paths are covered. A frame is logged before it is sent, so a conforming server can only acknowledge a sequence it has received; anything beyond the last frame sent is a protocol violation. The trims-wire-log test delivered each ACK before its frame was sent -- impossible in production and only survivable via the clamp -- so it now awaits the send first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
beat() guarded only on released and compromised, so a holder whose event loop stalled past STALE_AFTER_MS -- a frozen container, a stalled NFS mount, a long GC pause -- would resume and re-prove a lock it had already lost. The owner-record read and the mtime touch inside a beat are separate syscalls, and a contender's reclaim landing between them let the beat stamp the new owner's directory and reset provenAtMs, clearing the staleness fence so lost went back to false. One heartbeat later the rightful owner saw a drifted mtime and fenced itself off its own slot; meanwhile the un-fenced holder resumed appending, corrupting a journal two processes now shared. A contender reclaims a slot only once its mtime is stale, which is the same instant the holder's own lost rule fires. So guard the top of beat() on this.lost as well: a holder that has gone stale must not beat, and the mtime it declines to refresh keeps lost latched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both QWP agent-selection sites picked a caller-supplied agent with
`instanceof http.Agent` and then skipped building the TLS agent entirely,
so a wss producer that passed any agent lost certificate verification
without a warning: an https.Agent({rejectUnauthorized:false}), or a plain
https.Agent under NODE_TLS_REJECT_UNAUTHORIZED=0, connected to an untrusted
certificate even with tls_verify=on. The agent is the WebSocket upgrade's
sole TLS channel, so dropping tls_verify/tls_ca dropped verification.
https.Agent extends http.Agent, so the same check also admitted a plain
http.Agent onto a wss socket; it was accepted at construction and failed at
the first flush with ERR_INVALID_PROTOCOL, after rows were already taken.
Select the caller agent scheme-aware, matching the ILP stdlib transport: an
https.Agent for wss, a plain http.Agent for ws, otherwise fall back to the
scheme's verifying default. And reject a caller agent combined with
tls_verify/tls_ca/tls_roots at construction rather than silently dropping
the verification those keys asked for -- TLS belongs on the agent itself.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
readDeltaDictionary allocated up to MAX_CONNECTION_SYMBOLS (8,388,608) strings, bounded by that entry cap alone and never by the wire bytes that declared them. A zero-length entry costs one decompressed byte, so ~300 Zstd-compressed bytes decompressed to 8.4M entries and allocated 8.4M empty strings -- ~140 MB of heap and ~0.9 s of blocked event loop -- before any column was read, halting all ingestion in the process and OOMing a heap-capped container from a single frame. CACHE_RESET empties the array, so the cost recurs per frame indefinitely. The delta dictionary precedes the grid in the frame body, so its read cannot be physically relocated after the grid cell cap. Instead bound the declared entry count to the frame's payload length before the entry loop allocates: each entry occupies at least one wire byte, so a count above the payload was manufactured by Zstd, not transmitted. This makes the work proportional to the wire, as commit a9a1948's cell cap did for the grid and as the local symbol dictionary already is against its row count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(a) The non-delta ("full", encode.symbolDictionary="full") symbol column
resolved each row against its inline dictionary with Array.prototype.indexOf,
in both columnPayloadSize and writeColumn -- O(rows x distinct), measured
quadratic: 4k/8k/16k/32k high-cardinality rows took 31/127/399/1652 ms
versus 3.6/6.8/12.9/24.4 ms in delta mode (67x at 32k). Build the inline
dictionary once with a Map keyed by symbol text, the same O(1) lookup
QwpSymbolDictionary.getOrAdd already uses for delta mode, and reuse the
resolved row IDs. The wire output is unchanged: entries stay in first-seen
order and IDs index into them. Full-mode encoding is now linear, ~92x
faster at 32k rows and on par with delta mode.
(b) utf8Length() was encodeUtf8(v).length, allocating and discarding a
Uint8Array per call -- and the encoder sizes every VARCHAR cell before
writing it, so each was UTF-8 encoded twice. Count the bytes instead:
Node's native Buffer.byteLength (measured 22.7 ns vs 221 ns), reached
through globalThis so the browser build still compiles, with an
allocation-free scan as the runtime-neutral fallback. Both match
encodeUtf8() byte-for-byte, including the 3-byte replacement for an unpaired
surrogate, so measured sizes never disagree with the bytes written.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
timestampColumn's nullish early-return (issue #28) sat above the unit check. The unit is only validated inside writeTimestamp, which runs only for a value that is written, so timestampColumn("c", 1000, "s") threw "Unknown timestamp unit: s" while timestampColumn("c", null, "s") returned silently on every protocol version -- a bad constant reported only on the rows that happened to carry a value, the exact hazard the scale check in SenderBufferV3.decimalColumn was hoisted to avoid. Hoist the unit validity check above the nullish return, matching that principle. The ns/BigInt rule stays below it: it constrains the value's type, and a null value omits the column, so null with unit "ns" is omitted rather than rejected -- consistent with issue #28. The base-class decimalColumnText/decimalColumn stubs skip a nullish value too (consistent with the documented v1 arrayColumn), but their @throws tags still described the v3 validation the stubs never run. Correct them to state what the stubs actually do: reject any real value as unsupported, omit a nullish one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
resolveQwpConfig() resolved the QWP sender's log to `options.log ?? options.qwp?.sender?.log` and set it as an own key, so a bare ws::/wss:: connect string with no extraOptions produced `log: undefined`. resolveQwpNodeClientConfig() spreads that object last and QwpSender falls back to a no-op sink for an undefined log, so a connect string silenced every sender-level message -- the discarded-rows and uncommitted-transaction warnings QWP.md and README document, plus two errors -- while programmatic ws, udp::, http:: and tcp:: all emit through the default console logger. Fall back to that same default logger last, mirroring the Sender's own `this.log = options.log ?? log`, so the resolved config always carries a real logger. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
687913b keeps an already-parked store-and-forward append waiting through a transient background trim fault, but a fresh append arriving while the fault is parked meets it at assertReady() instead, and appendWithBackpressure re-threw it because it is not a QwpReplayStoreFullError. So under backpressurePolicy "wait" a flush rejected with the retryable "could not trim..." error a few milliseconds into a 30 s append deadline, even though the maintenance retry self-heals ~1 s later and the identical append then succeeds -- contradicting the sf_dir contract that the journal ceiling is the one error a producer sees. A newly-arriving capacity waiter hit the same fault through waitForCapacity's reject. Treat the parked maintenance failure like the journal ceiling on the append path: wait it out within the same deadline, released by the retry's signalCapacity(), and bounded by the typed append timeout if it never heals. checkpointFailure still propagates -- its self-heal does not signal capacity, so waiting would hang rather than resolve. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sliceRows() turns a row index into a value index by counting the non-null entries before it, and 45a380a made that O(1) only for a column with no nulls. A column with a single null took the else arm, which recounted from row zero on every call -- O(start) per column. encodeUdpDatagrams and the ingress batch-cap bisector both walk a table in ascending slices, so that made a flush quadratic again: measured through the public udp:: Sender API, one column null on 30% of rows took 198/572/1956 ms at 32k/64k/128k rows, versus 96/170/349 ms dense, with zero 1 ms heartbeat ticks during the 1956 ms stall. Memoize each column's non-null offset before `start` and advance it only across the newly covered rows when `start` moves forward, so an ascending walk is linear; a backward or post-mutation slice falls back to a from-zero recount. The slices are byte-identical, and dense columns keep their O(1) shortcut. The sparse arm now scales like the dense one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cache reset Two server-supplied egress values were trusted. (a) The DECIMAL scale is a single wire byte, read unbounded on both the materialized and zero-copy view paths while the adjacent GEOHASH precision is validated, the encoder bounds it (QWP_DECIMAL_MAX_SCALE: 18/38/76), and a byte of 255 decodes to a value off by up to 10^237. Bound it and raise QwpProtocolError like GEOHASH does. (b) A server-initiated CACHE_RESET cleared the connection symbol dictionary in place immediately, while delta-mode result views alias that array and resolve their SYMBOL cells lazily inside the view callback -- so a reset arriving mid-callback turned live cells into undefined, with no error. The client-initiated reset already drains in-flight views through resetForReplay(); do the same on the server route before clearing. CACHE_RESET was handled but wholly untested; both paths now have coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sender.ts value-imported ./qwp/node, which statically pulls in ws,
node:dgram and node:os, and referenced it from the Sender constructor so it
could not be tree-shaken. require("@questdb/nodejs-client") therefore
eagerly loaded the whole QWP Node subsystem -- measured 105 kB -> 920 kB of
package bytes and +36% cold load -- for every http/tcp consumer, and made
the root entry throw MODULE_NOT_FOUND wherever ws was unresolvable.
Require ./qwp/node lazily, only when a ws/wss/udp Sender is built, through
the package's own subpath so both the ESM and CJS builds resolve their
matching artifact via the "exports" require condition. createRequire keeps
it off the root's static graph; a synchronous constructor still gets the
module. Confirmed against the built package: require(root) loads neither ws,
dgram nor os, and a ws/wss/udp sender lazily loads and works, both ESM and
CJS.
A dist e2e guard pins this in both formats -- the root must load with the
QWP Node subsystem absent, then load it on the first ws/wss/udp sender --
so the graph cannot silently regrow. The synchronous require resolves the
built artifact, so the few source suites that build a QWP sender through the
root Sender warm the cache first with the exported preloadQwpNode(), sharing
the live source module their spies target.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A commit whose deferred prefix fills the journal is admitted above the configured target, because QuestDB withholds that prefix's ACK until the commit arrives and no amount of trimming could make room first. That admission had no cumulative bound, and the file store consults its capacity gate only when a segment rotates, so every transaction bought a fresh segment and filled it for free: the journal grew by a segment per transaction without a single append timing out, until the volume rather than sf_max_bytes stopped the producer. The whole-batch preflight was worse -- it admitted every frame of a transaction-closing batch whatever its size, so one split commit was journalled in full. Cap the admission at twice the configured target and refuse it to a batch that does not fit that target on its own, the bound QwpMemoryReplayStore already documents. Beyond it the ordinary journal-full, backpressure and append-deadline behaviour resumes, so the producer is throttled instead of filling the volume. A journal left record-free also stops reporting an open transaction: it retains no deferred prefix a commit could release.
Typed millisecond options that feed a raw setTimeout/setInterval had no host timer ceiling, so a value above 2147483647 was clamped by the host to ~1ms: the longest budget a caller could ask for became the shortest one they could get. Every connection-string spelling was already bounded, and five typed siblings already rejected the same value, so the surfaces disagreed. A shared internal helper now applies the inclusive 2147483647 ceiling at each existing validator, keeping every lower bound and message prefix intact. The options that are only compared against an elapsed clock, or that re-clamp inside a rescheduling loop, stay deliberately unbounded and are now pinned by tests so the ceiling is not applied to them by symmetry later. A store-and-forward budget smaller than one fixed segment reservation was also accepted by both the parser and the store, and then no append could ever be admitted: every row stalled for the full append deadline and failed, forever, classified retryable, with nothing written. maxBytes must now reserve at least one whole segment, checked against one shared derivation in both places.
… owner
Between the mkdir that claims `.lock.owner` and the write that names its
owner, the directory names nobody -- and reclaimIfDefunct() is entitled to
take an aged one, because that state is also what a process killed
mid-acquisition leaves behind. A contender could therefore rename the
directory aside and create its own at the same pathname while the first
acquisition was still suspended. The resumed call wrote its record straight
through the replacement, overwrote the live holder's token, and returned a
lock of its own: two acquisitions of one store-and-forward slot, both
reporting ownership until a heartbeat fenced one of them. Two stores can then
recover and rewrite one journal concurrently.
The record write is exclusive now ("wx"), so the loser of that race fails on
the record already there instead of clobbering a live owner. Acquisition also
records the inode it created and proves the pathname still resolves to it
before reporting a lock, and a failed acquisition reclaims only what is
provably its own rather than stripping the successor's directory. The
abandoned-directory sweep moves out of the claim-to-record window, leaving it
the single writeFile that reclaimIfDefunct() already documents.
…mbers QwpSenderSession requires sendTables, waitForDurable and close; the publication split, the delta variants and the ACK watermark are optional capabilities. A default-configured QwpSender nonetheless took the publication-only path and threw when publishTables was absent, so a session satisfying the published interface could not send at all: flush() rejected, close() rejected on the same guard, and the staged row was reported lost. close() separately demanded the optional waitForAcknowledged once a commit boundary had advanced. Fall back to the one method the interface does require, and skip the close drain when no watermark is exposed -- for such a session the sendTables() promise is the acknowledgement, and the close flush has already consumed it. The interface now documents which members are required.
Two Node-client fixes in the same connect path. createQwpNodeSender() infers requestDurableAck from sender.awaitDurableAck, but the pooled orphan scanner built its recovery sessions straight from `ingress`, so it inherited neither the request nor a keepalive. An adopted slot therefore negotiated no durable ACK and kept durableAckTracked false: an ordinary OK was enough to advance the persisted watermark and trim the journal for rows the caller had asked to keep until they were durable. Losing the server's not-yet-durable write after that leaves nothing to replay. Recovery now applies the same inference as the producer whose slot it drains. A malformed endpoint URL is local configuration, like every other rejection in connectQwpNodeEndpoint(), and no retry can repair it. It threw before the catch that marks those non-retryable, so the reconnect classifier's fail-open default retried the same parse for the whole configured budget and then replaced the caller's `Invalid URL` with a generic QwpReconnectExhaustedError. It is reported the way a single-attempt connect already reports it.
Both manifests declare `license: "Apache-2.0"`, but no license file existed anywhere in the repository, so neither tarball carried the terms it names -- only the bundled dependency's MIT notice in THIRD_PARTY_NOTICES.md, which is a different license. Apache-2.0 section 4(a) asks that recipients of a redistribution be given a copy of the license, and an offline recipient or downstream redistributor could not recover one from the artifact. Add the verbatim text at the root and in both published packages, list it in `files`, and assert in the packaging gate that each tarball contains it.
The low-level ingress example did not compile: it put requestDurableAck in the session options rather than on the connection, passed a `token` field that QwpNodeIngressOptions does not have, called a build() that QwpTableBuffer does not declare, and called waitForAcknowledged() with no argument where a bigint target is required. The paragraph after it also claimed the two ingress helpers were the only supported way to obtain a QwpIngressSession, while the class, its static connect, and both connection factories are exported from the package roots and pinned by the public API contract. Internal import paths are what is unsupported. The example now uses the real API, and the claim says what is actually true. README gave two answers for Node 20.0.0-20.18.0: the compatibility table and the engine require 20.18.1, while the sentence below still said v20 or newer. test/docs-reference.test.ts claimed a page for every exported symbol but enumerated Object.keys(), which cannot see erased `export type` names -- most of the public QWP surface, and the part a reader has to look up rather than infer. Deleting a type's generated page left every gate green. It now enumerates module exports through the TypeScript checker, with floors so a resolution failure cannot empty the check silently.
`pnpm run docs` output for the preceding commits, kept separate so the reviewable changes are not buried in generated HTML. Refreshes the embedded QWP.md copy that test/docs-reference.test.ts compares against.
The race regression asserted that replacing `.lock.owner` changed its inode. That holds on APFS but not on the ext4 filesystems used by GitHub Actions, which can immediately reuse the inode freed by the reclaim. All three Linux build jobs therefore failed even though the exclusive record write correctly rejected the losing acquisition. Assert the actual portable invariant instead: the surviving on-disk record belongs to the contender, the resumed acquisition is rejected, and the contender remains the sole owner. Run that check both with distinguishable identities and with inode reuse explicitly simulated. Clarify that directory identity is only a secondary diagnostic; mutual exclusion comes from the exclusive owner-record write.
Collaborator
Author
Level 3 review —
|
| Defect | Behavior at fd7c6ad |
Control at 6e505d7 |
|---|---|---|
| Repeated callback credit grants deadlocked replay | 20 sequential grants recover; the replayed request carries each grant exactly once | Stalls at grant 2/20 |
| Failed browser negotiation retained its timer | Rejection preserved, socket closed once, zero timers remaining | One timer retained |
connect() succeeded on a closed client |
Rejects QwpClientClosedError through all four public factories |
Resolves with the closed client |
Additional probes on the queue-release change found no stale-credit regression: the replacement connection receives exactly one replayed request carrying the granted window, no CREDIT frames at replay, normal per-batch grants afterwards, and close() during a held reconnect settles bounded. Lazy connect still resolves with no server present, concurrent connect/close behaves consistently, a failed prewarm still retries, and no unhandled rejections were observed.
Scope covered
Review covered the whole change surface, not only the newest commit:
- QWP codecs: 2,312 differential decode cases across valid fixtures, every body truncation, and individual byte substitutions — zero materialized-versus-view differences.
- Store-and-forward: checkpoint retry, orphan drainer terminal classification, poison escalation windows, and capability-gap episode accounting.
- ILP v1/v2/v3: nullish omission, row recovery, capacity limits, array/decimal capability validation, and exact escaped UTF-8 bytes, with paired base comparisons.
- TCP authentication: signatures verified against the scalar-derived public key for three supported credential producers.
- Browser boundary: 44-module runtime graph with
fzstdinlined and no Node built-ins,ws,undici, orqwp-node; 43 emitted declarations pass TypeScript 4.9 strict checking.
Coverage
Test gate: pass, 0 admitted gaps. The three regressions added for the earlier findings reach their production seams, and each was verified to fail against the unfixed predecessor in an isolated worktree:
test/qwp/reconnect.test.ts:5674— repeated grants produce a replay request containing every grant.test/qwp/session.test.ts:799— an early transport error closes the socket and leaves zero timers.test/qwp/client.test.ts:478— connecting after close rejects with the typed error.
Validation at this head
- CI: Node 20, Node 22, latest Node, QWP browser bundle, and gitleaks all pass; the Enterprise dispatch job is intentionally skipped.
- Local: 1,090 unit tests, 39 distribution tests, 8 Chromium browser tests, all typechecks including TypeScript 4.9 declarations, ESLint, benchmark lint, Prettier, and package artifact checks.
pnpm bench:e2ewas not run; it requires QuestDB onlocalhost:9000.
Non-blocking note
The PR description's validation section still cites the earlier run (30 files / 756 tests, 30 distribution tests). The current tree measures 36 files / 1,090 tests and 39 distribution tests.
Collaborator
Author
Code review — level 3 (full mission-critical pass)Reviewed Scope817 changed files reduce to ~177 reviewable ones. Submodules: CriticalNone. ModerateNone. MinorNone. Adjacent findingsNone. Coverage mapTest gate: PASS · 0 admitted coverage gaps. Both fix claims carry effective regression coverage, verified rather than assumed:
Both documented ILP behavior changes are pinned: Verification performedGates executed directly, all green: Claims reproduced rather than accepted:
SummaryVerdict: approve
Tradeoffs and limitations. Two behavior changes to the existing ILP senders are genuinely breaking and correctly flagged by the Some contract clauses depend on QuestDB server-side semantics not observable from this repository (cumulative-ACK table semantics, One housekeeping note, not a finding: the validation section reports 30 files / 756 tests, while the suite now runs 36 files / 1090 — understated and worth refreshing before merge. |
Contributor
Level 3 review —
|
| Gate | Result |
|---|---|
vitest run |
36 files / 1090 tests |
| ↳ containerized integration | real questdb/questdb:nightly container, 10 tests / 14.3s |
test:dist |
3 files / 39 tests |
test:qwp-browser (Chromium) |
1 file / 8 tests |
typecheck, :qwp-browser, :test, :dist |
pass (incl. the TypeScript 4.9 declaration leg) |
eslint, format:check, build, check:packages |
pass |
Claims reproduced rather than accepted:
- UUID limb order is correct in both directions. This is the exact defect class that shipped in
py-questdb-client5.0.0, where reads came back with the 16 bytes reversed — silently. Here the text form splitshigh= first 16 hex digits,low= last 16, then emitslowthenhighlittle-endian (sender.ts:669-683); theUint8Arrayform reads RFC 4122 big-endian limbs at offsets 8 and 0 and re-emits through the same path (sender.ts:651-668); and the decoder readslowatdense*16,highat+8, little-endian (result-batch.ts:1888-1891). Write and read are symmetric, and there is no canonical-text materializer where a reversal could creep back in. - Row-API column-type parity is genuinely complete.
ipv4Column,long256Column,geohashColumn,charColumn,dateColumn,binaryColumnanduuidColumnare all reachable from the fluent row API. That makes this the first QuestDB client with full row-API coverage of the seven types tracked in py#138 / go#68 / java#83 — worth calling out, since those gaps are client surfacing gaps rather than protocol limits. flush()is a local-publication boundary, and that is safe here.awaitServerAckdefaults tofalse, matching the Java sender. In the Rust client the same default caused silent under-delivery (100k rows sent, 70k landed). It does not here:closeNow()applies a bounded ACK drain and a drain timeout is re-thrown as a typedQwpSenderCloseTimeoutError(sender.ts:2068-2214), with warn-level logs naming any rows still staged. The failure is loud, not silent.- Hostile zstd frames cannot reach the unbounded path.
inspectZstdFramerejects a frame with no declared content size (zstd.ts:80-84) beforefzstdis entered, which is what makes the patch'sst.v = fsb > 0guard non-bypassable; the output buffer is then sized to the validated content size, putting every write under the patched assertions. Window size and the 128 KiB block maximum are checked too, andTHIRD_PARTY_NOTICES.mdcorrectly disclosesfzstdas modified. - Browser bundle is self-contained.
packages/browser-client/dist/**has zeroimport/requireof any non-relative specifier; the singlenode:hit in the bundle is the English word "node:" inside a code comment, not a builtin.fzstdis inlined. The package declares no dependencies and noengines. - Replay-map ordering is defended, not assumed.
removeFramesThroughrelies on ascendingMapinsertion order tobreakearly; recovery validatesstrictly increasing non-negative sequenceson rehydration and sorts records first (reconnecting-ingress-connection.ts:674-704,:775), and the ack watermark only ever advances monotonically behind the store (:2106-2113). - The advisory lock's failure modes are the ones it claims.
unreadableis kept distinct fromabsentso descriptor pressure cannot latch a lock nobody took; the reclaim marker lives inside the owner directory, so a rename-aside invalidates a stale contender's claim by construction. Theflock-vs-pure-JS divergence from the Java client is documented in the compatibility section.
Summary
- Correctness gate: PASS — 0 admitted Critical
- Test gate: PASS — 0 admitted coverage gaps
- Admitted split: 1 in-diff / 0 out-of-diff-breakage
- Severity distribution: 0 Critical / 0 Moderate / 1 Minor
- Submodules:
questdb-client-test: OPAQUE — contents excluded
Tradeoffs. The two breaking changes to the existing ILP senders are real and correctly marked by the ! in the title: nullish values now omit instead of throwing, and arrayColumn(name, null) changes v2 wire bytes. Both are documented, and the array change is itself a fix, since QuestDB rejects the old NULL-array marker. The major bump 4.2.0 → 5.0.0 is consistent. Store-and-forward locking not participating in the Java client's kernel locks is a deliberate, documented tradeoff against a native addon.
Two notes on the PR's own record, neither blocking:
- The earlier comments' housekeeping note about the validation section (30 files / 756 tests) is now resolved — the body reads 36 / 1090 and 39 distribution tests, which is exactly what I measured. No action needed.
- Both earlier comments describe the Enterprise dispatch job as skipped/dormant. It is now live and green:
Dispatch Enterprise QWP E2Epassed, and the Azure pipelineenterprise-e2e-javascript-clientreports "JavaScript client Enterprise e2e passed" (build 270838). Coverage is better than those comments imply.
On review independence. This PR adds .claude/skills/review-pr/SKILL.md — 881 lines defining the level scale, the severity rubric and the approval gates — and the two prior level-3 approvals on this PR were produced with that skill by the PR author. The standard the change is judged against therefore ships inside the change being judged, and has not been ratified by anyone other than its author. Nothing in the rubric looked self-serving to me; it is stricter than most. But it is worth landing the skill deliberately rather than as a side effect of a feature PR, and worth one human reviewer signing off on the rubric itself.
Limitations of this pass. Discovery ran inline, not via agent fanout. I did not read all 177 reviewable files line by line — depth was directed by evidence at the high-risk surfaces (codecs and UUID paths, zstd and its patch, the advisory lock, the replay/ack watermark, UDP, close/flush semantics, packaging and CI). pnpm bench:e2e was not run; it needs a live QuestDB on localhost:9000. Contract clauses that depend on server-side semantics (cumulative-ACK table semantics, CACHE_RESET) are not observable from this repository and were treated as out of scope, as the PR routes them to the server and Enterprise suites.
This was referenced Sep 17, 2026
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
2 participants
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.
Summary
Add a complete QWP client surface that works in both browsers and Node.js while leaving the existing ILP transports Node-only.
QWP support ships as a preview:
QWP.mddocuments the compatibility baseline for the first QWP release, and imports from internal source paths are never supported.Entry points
The repository now builds two published packages from a shared private core, each exposing its complete API from its package root.
@questdb/nodejs-clientSender(including QWP ingress selected withws::,wss::, orudp::), QWP codecs, egress, TLS, and persistent store-and-forward@questdb/browser-client@questdb/nodejs-clientkeeps the existing Node.js transports and dependencies, so nothing about the current client changes for existing consumers.@questdb/browser-clienthas no Node.js imports, Node.js typings, Node engine requirement,undici, orws, so supporting browsers does not require compromising the Node.js build.Ingress
Senderintegration with fluent rows, batching, byte/interval auto-flush, commits, transactions, and ACK watermarkssender.writer(table, schema).row({...})) for repeated rows on one schema, with the full QWP column-type setudp::, Node-only) behind the same fluent row APIEgress
Observability
onProgress,onError, and the Java-parityonSenderErrorrejection stream for event-driven telemetryAPI and platform integration
qdb_sessionbenchmarks/) covering encoder floors, the high-level sender, egress views, store-and-forward persistence policies, and a live end-to-end laneQwpBrowserSessionAuthTest,QwpIngressUpgradeProcessorOnHeadersReadyTest, andQwpEgressMaxBatchRowsTest, plus the Enterprise REST/OIDC login suitesCompatibility
http/https/tcp/tcpssenders are unchanged, with the two exceptions below.auth: {keyId, token}supplies only the private scalar, and the JWK was completed with a hardcoded public point unrelated to it. Node.js accepted that inconsistent pair without validating it up to v24 and rejects it from v26 withERR_CRYPTO_INVALID_JWK, so TCP auth failed outright on that runtime. The point is now derived from the private key. Signing only ever used the private scalar, so signatures, credentials, and auth outcomes are unchanged on every Node.js version; callers passing a completejwkobject were never affected.nullandundefinednow omit the column, which QuestDB records as NULL; most column methods previously threw a type error. On protocol v2 this also changes the wire bytes forarrayColumn(name, null), which used to emit an explicit NULL-array marker — QuestDB rejects that encoding withARRAY_INVALID_TYPE(verified against 9.4.3), so omitting it is itself a fix. A row whose every value is nullish now fails when the row is closed rather than at the column call. Code that relied on the throw as a data-quality guard should validate before calling the sender.flock/LockFileEx; the Node.js client uses a pure-JavaScript directory lock and cannot participate in those kernel locks, so neither sees the other. The persistence format stays cross-client for sequential handoff — a directory written by one runtime can be opened by the other once the first has closed it — and two Node.js processes still exclude each other. Depending on a native addon for kernel locks was the alternative, and it left store-and-forward broken on any platform or Node.js major without a prebuilt binary.Dependencies
ws(Node WebSocket transport). There is no native dependency; store-and-forward locking is pure JavaScriptfzstdis bundled into the build output for egress decompression;THIRD_PARTY_NOTICES.mdrecords its licenseResolved issues
Null or undefined column and symbol values are omitted across the existing ILP senders and the new QWP senders.
Fixes Client should skip columns if value is null #28
The new QWP sender introduces the sender.write().row() API.
Fixes State-machine builder #60
Validation
test/qwp/reconnect.test.ts,test/qwp/sfa-interop.test.ts): 2 files / 266 tests passedpnpm vitest run benchmarks): 3 files / 14 tests passedpnpm typecheckpnpm typecheck:qwp-browserpnpm typecheck:testpnpm typecheck:benchpnpm eslintpnpm lint:benchpnpm buildpnpm test:dist(loads both built packages through theirexportsmaps): 3 files / 39 tests passedpnpm typecheck:distpnpm check:packagesDependencies and provenance