feat(qwp): QWP ingest over WebSocket with store-and-forward durability + benchmarks - #61
Draft
nwoolmer wants to merge 121 commits into
Draft
feat(qwp): QWP ingest over WebSocket with store-and-forward durability + benchmarks#61nwoolmer wants to merge 121 commits into
nwoolmer wants to merge 121 commits into
Conversation
Adds the approved design for porting QuestDB Wire Protocol ingest to the Node.js client, using java-questdb-client 1.3.7-SNAPSHOT (8f5ed4f9) as the reference. Covers module layout, wire format, error policy, store-and-forward, config surface, testing strategy and a 13-PR stack. Records two source-provenance findings so implementers do not repeat them: docs/qwp/*.md in the parent repo were deleted by #7200 and still document a removed schema_id field, and design/qwp-nack-policy-v2.md predates the ABANDONED policy and the DATA_LOSS/PROTOCOL_VIOLATION categories. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-verified the spec against java-questdb-client 1.3.7 (8f5ed4f9) rather than against prose. Seven wire-format errors found, several of which would have produced frames the server rejects or, worse, silently accepts as wrong data: - every column payload starts with a 1-byte null header; the spec omitted it - values are compacted to non-null rows (valueCount, not rowCount) everywhere - null bitmap semantics documented: bit=1 means NULL, LSB-first per byte - DECIMAL scale is written in the column payload, not the schema; the QwpConstants javadoc saying "in schema" is wrong and the spec had copied it - DATE is never Gorilla-encoded and carries no encoding byte - the timestamp encoding byte exists only when FLAG_GORILLA is set, and is still emitted (as 0x00) for columns of <= 2 values - GEOHASH precision is a single per-column varint; array shape is per value Also: SF boundary records use an alternating two-record CRC32C scheme at offsets 0 and 4096, not the "bytes before manifest entry" ordering previously asserted; CRC32C is Castagnoli, so zlib.crc32 cannot be used. X-QWP-Client-Id follows Java's <lang>/<protocol-version> convention, not package.json. Added QWP-specific auto-flush defaults and the missing sender-level config keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third pass over java-questdb-client 1.3.7 (8f5ed4f9), checking the areas the first two passes had asserted rather than verified. Config: ConfigSchema assigns every key a Side, and six of the spec's assignments were wrong. max_batch_rows, initial_credit, compression, compression_level and client_id are Side.EGRESS (query client), not ingest; sender_id is Side.INGRESS, not a pooling key. There is no `zstd` key at all -- zstd is an enum value of the egress-side `compression` key, so the spec had invented a connect-string option. Pool keys are now accept-and-ignore rather than reject, matching Java, so a shared connect string does not break the sender. username/password are canonical with user/pass as aliases. Poison detector: escalation requires a strike count AND a minimum wall-clock dwell (poison_min_escalation_window_millis, default 5000), not the count alone. Java's rationale is explicit -- with pacing, four strikes can accrue in under a second behind a load balancer whose backend is briefly down, so a count-only rule escalates transients to producer-fatal terminals. The catch-up cap gap has the same two-condition shape (16 attempts + dwell). Also defines the wire primitives the spec had used ~15 times without ever specifying them: varint is unsigned LEB128 (max 10 bytes, no implicit zig-zag), zigzag is (n<<1)^(n>>63), and string is varint length + UTF-8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth pass, focused on section 8, which had the least line-level verification. Adds the persisted symbol dictionary (<slot>/.symbol-dict, SYD1), which the spec had omitted entirely. It is load-bearing, not an optimisation: delta SF frames carry only the symbols they introduce, so recovery and orphan adoption must re-register the whole dictionary before replay, and a surviving frame referencing a missing id is unrecoverable. Records the chunk layout, the implicit dense id numbering, the deliberate per-chunk (not per-entry) CRC32C, the write-ahead-but-not-fsynced ordering, and the rule that open() never destroys the file. Corrects slot locking: Java has TWO locks, not one. The slot .lock plus a parent-anchored logical lock under .slot-locks/, deliberately outside the slot directory so it survives a rename, which the four-step orphan adoption sequence depends on. Only the primitive is replaced in Node; the structure is not. Adds the SF01 segment file format (24-byte header, per-frame u32 crc32c + u32 payloadLen, CRC covering both), the publishedCursor publish barrier, the memoryBacked flag shared by memory and disk modes, and the four sf_durability modes (memory, periodic, flush, append), which are WebSocket-only. Splits symbol-dict persistence into its own PR; stack is now 14. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fifth pass, over CursorWebSocketSendLoop's reconnect/replay sequencing.
Adds the symbol-dictionary catch-up, previously absent. The delta dictionary
is connection-scoped on the server, so after a reconnect the fresh server's
dictionary is empty while every surviving SF frame references ids from the old
connection. The send loop must emit a catch-up frame re-registering from id 0
before replaying any data frame. The spec had named the outcome ("re-register
and replay") without the mechanism, which is not implementable.
Records the chunking rules, including that an unadvertised batch cap is NOT
unbounded -- pack against 64 KiB, because the transport closes oversized
frames with 1009 and a catch-up-only close is non-terminal, so an unchunked
catch-up reconnects into the identical frame forever. Also the cap-gap
asymmetry: a foreground sender retries forever, only an orphan drainer latches,
after 16 attempts AND 300s dwell.
Adds the recovery no-dedup rule for .symbol-dict replay: entries are appended
unconditionally by position, because the file, the wire delta and the catch-up
mirror all key on id, not string. Flags that Java's colliding case (lone UTF-16
surrogates -> '?') diverges in Node (-> U+FFFD), so those inputs must be
excluded from byte-equality golden vectors.
Corrects catch_up_cap_gap_min_escalation_window_millis to 300000 and adds the
reconnect backoff defaults.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sixth pass, over SegmentManager and the close path. Adds hot-spare provisioning, absent until now. The segment manager keeps every ring supplied with a pre-created spare so segment create (open+allocate+map) and trim (unmap+unlink) never run on the producer or I/O path; rotation just swaps in the spare. Records the 1 ms poll tick, MIN_LIVE_SEGMENTS=2, and the staged trim retry. Omitting this fails no test -- it silently moves a file create onto the producer at every rotation. Records the .symbol-dict liveness-floor deadlock, which a naive port reintroduces: segment bytes are reclaimable by ACK-driven trim, so refusing to provision on them is backpressure that clears itself, but .symbol-dict bytes are lifetime-monotonic. If side-file bytes alone push a ring under sf_max_total_bytes, no ack can ever free the shortfall and the producer stalls permanently, across restarts, while the disk-full warning points at a trim that cannot help. Enforcing the cap as a directory-byte sum is the bug. Separates the two ring append sentinels, previously conflated as "backpressure": BACKPRESSURE_NO_SPARE clears itself and should wait, PAYLOAD_TOO_LARGE never clears and must fail immediately rather than burn the append deadline and report a timeout. Adds close() ordering: a pre-flight rejection of the final batch must not escape before commit/seal/drain, or it abandons rows an earlier successful flush already published; and close() must surface latched terminal errors, since a caller who only closes would otherwise never see a server rejection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…WP spec Seventh pass, over the flush split path and WebSocketClient's control frames. Adds the cap-split flush, previously absent -- section 5 claimed all dirty tables always become ONE frame. When the encoded frame exceeds serverMaxBatchSize the flush splits per table, all but the last carrying FLAG_DEFER_COMMIT. Records the two rules that make it safe: pre-flight every split frame before publishing any (otherwise an oversized later frame strands the published prefix and a later commit delivers a partial batch), and snapshot the cap exactly once per flush (the I/O side lowers it on failover to a smaller-cap node, and in Node every await inside the flush is that window). States the delivery contract explicitly: a split flush failing partway leaves a deferred prefix on the ring and the next flush re-emits the whole batch, so those rows are delivered at-least-once, duplicated. This is deliberate and within store-and-forward's contract, but the spec previously implied exactly-once by omission. Adds the two symbol-dictionary modes. Delta is not simply on/off: full-dict mode ships the whole dictionary from id 0 in every frame so replay to a fresh server can never dangle an id, and delta mode is only safe once .symbol-dict exists to reseed recovery. The mode follows from available durable state, not from a user toggle -- which is what lets PR 6 ship before PR 12. Adds WebSocket control-frame obligations: PING/PONG, the RFC 6455 5.5.1 close echo, per-frame CSPRNG masking, and the separate control-frame send buffer, so a pong is never interleaved into a partially written data frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eighth pass, over reset() and the listener/dispatcher surface. reset() must do more than discard buffered rows. A later flush encodes its delta as [sentMaxSymbolId+1 .. currentBatchMaxSymbolId], so a watermark left behind by the discarded batch makes even a single-row follow-up carry the whole abandoned symbol range -- hitting the very cap rejection reset() exists to clear and wedging the sender permanently. It must also reset the batch watermark to -1 and reclaim never-shipped symbol ids, which is what 1.3.7's "Return never-shipped symbol ids on reset()" does. Adds the three async callback surfaces; the spec had shown only an error callback. Records the shared delivery contract: never invoked on the I/O or producer path (a queued setImmediate dispatch in Node, since there is no dispatcher thread), bounded inbox with counted drops and a minimum capacity of 16, handler exceptions caught and logged, success connection events guaranteed per transition while failure events may coalesce, and AUTH_FAILED firing before the producer-side error is observable. Also records that the progress watermark advances only on server OK frames and that a plain OK means server-side commit, not object-store durability. Fixes non-monotonic section numbering introduced by earlier passes: 3.3.1 preceded 3.3, and the 8.1.0.x block sorted before 8.1.1. Cross-references updated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ninth pass, aimed at the one wire codec the spec had left as "port QwpGorillaEncoder". Records the full delta-of-delta layout: the five DoD buckets and their bit widths, that the first two timestamps ship uncompressed with only t[2] onward entering the bitstream, the exact encoded-size formula for counts 0/1/2/>2, LSB-first packing with zero padding to a byte boundary, and the single-pass pre-validation that returns -1 when a DoD leaves signed int32 and forces the uncompressed fallback. The trap worth the pass: because packing is LSB-first, the prefix constants are bit-reversed relative to how they read. The logical prefix '10' is written as 0b01, '110' as 0b011, '1110' as 0b0111. Writing 0b10 for '10' is the obvious mistake and yields a stream that decodes into plausible-but-wrong timestamps instead of failing loudly. Java carries a javadoc table saying exactly this, which suggests it has caught people before. Confirmed the client and server bucket constants are identical. Adds the matching golden-vector requirements: every bucket boundary (0, +/-64, +/-256, +/-2048, int32 edges), a raw-fallback stream, and columns of exactly 0, 1, 2 and 3 values, since sub-3 counts take a different path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tenth pass, over section 3.5 -- claims made in the first pass about the existing Node code and never re-verified since. Checked against current main; "all additive" understated it. options.ts branches on protocol in four places, not one: the protocol token switch, parseProtocolVersion, parseAddress's port defaulting, and the doc comment. Two of them carry their own copy of the "accepted protocols" error string, which sender.config.test.ts almost certainly asserts verbatim. ws/wss default to port 9000, not 9009. Records a silent-failure hazard: parseProtocolVersion's default arm assigns PROTOCOL_VERSION_V1 to any non-HTTP protocol, and createBuffer switches on protocol_version alone, so a ws:: sender would receive SenderBufferV1 -- the ILP text buffer -- and emit ILP with no error raised. No protocol_version value denotes QWP, so createBuffer must branch on protocol before reaching that switch. Added as a risk with a required PR 3 test. Notes that resolveAuto needs no guard only by accident (it returns early on a non-auto version before building a ws://.../settings URL), so that deserves a regression test. Corrects 9.1: auto_flush_bytes is not a different default, it does not exist in the Node client at all, and auto_flush_interval is a hardcoded 1s module constant with no per-transport hook. Both are new functionality in PR 3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…laims Eleventh pass, reading the document as a document rather than checking it against sources. Ten passes of accreted patching had left three internal contradictions that source-comparison could not surface. Section 1.1 excludes multi-host failover, yet five later sections lean on endpoint rotation: RETRIABLE_OTHER's whole definition, two connection event kinds, the cap-snapshot rationale, the catch-up cap gap, and addr described as a host:port list. Adds 1.2 stating the stack targets a single endpoint and giving each behaviour's single-host meaning, keeping the enum and event shapes intact so the later HA spec stays additive. Cross-references added at each site. Fixes two stale claims in section 10 that later passes had invalidated. Tier 2 still described poison escalation as firing at 4 strikes, contradicting 7.4, where escalation needs the strike count AND the dwell window; it now requires both conditions be exercised, including a case that accrues 4 strikes inside the window and asserts escalation does not fire. Tier 4 still asserted rows land "exactly once", contradicting 5.1, where replay and cap-split retry legitimately duplicate; the assertion is now every row present, with duplicates explicitly not a failure. Also adds the liveness-floor case to the crash-recovery tier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twelfth pass, read as an implementer walking PR 1 and asking what the spec could not answer. Two gaps, both PR-1 blocking. Section 6.5 described the handshake request and response headers but said nothing about a non-101 response. Java classifies three ways and conflating them inverts the retry behaviour: 421 carrying X-QuestDB-Role is a role reject retried indefinitely (the connect-time half of the read-only case), 401/403 is a terminal credential failure that emits AUTH_FAILED before the producer-side error, and everything else including 404 falls through unclassified. Treating 401 as retriable spins forever; treating 421 as terminal kills a sender during an ordinary failover window. Records that tls_roots does not port. Java takes a JVM keystore path plus password; Node's tls.connect takes PEM via ca or PKCS#12 via pfx, and cannot read JKS at all. Specifies accepting PEM and PKCS#12, detecting JKS by its 0xFEEDFEED magic and failing with a message naming the conversion rather than a parse error. Also carries over Java's rule that a custom trust store may not be combined with disabled validation. Both added to the risks, and PR 1's row now names them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirteenth pass, walking PR 2 and PR 3 as an implementer from the ground up. Adds the commit frame (5.1.1), an entire wire message the spec had never described: tableCount 0, no rows, FLAG_DEFER_COMMIT cleared, and no symbols. The empty delta must be built by construction, pinning both bounds to the baseline, because the commit path does not write-ahead-persist the dictionary -- shipping a symbol there puts an id on the wire that a recovered slot cannot rebuild, silently misattributing reused ids after a crash. Deriving the bound from batch state instead is a bug Java already fixed: the batch watermark is not reliably reset after an empty flush or a cancelled row, and a commit reaching that window re-shipped the entire dictionary in a frame no chunker covers. Adds the row-rollback invariant (4.1.1), which has no ILP analogue. In a row-oriented buffer a half-written row is trailing bytes; in a columnar one a setter that throws mid-row leaves columns at unequal lengths, so every later frame is malformed while still looking structurally valid. Java wraps every column setter in rollbackRow; the port must too. Notes that Node's Sender has no cancelRow, so that parity is optional while the rollback is not. Adds the wire limits the spec was missing: DEFAULT_MAX_ROWS_PER_TABLE 1,000,000 and DEFAULT_MAX_TABLES_PER_CONNECTION 10,000, plus the u16 structural ceiling on tableCount. Clarifies that payloadLen excludes the header and that one QWP message is exactly one WebSocket binary frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourteenth pass, cross-referencing the Java client class by class against the spec and filling the PR 4/5 surface, which had no coverage at all. Adds 6.5.3, the per-column accumulation rules from QwpTableBuffer: type locked on first sight, duplicate column within a row silently first-value-wins, row completion back-filling nulls so columns stay equal length, and a per-row size guard distinct from the per-frame split. Per type: geohash precision locked 1-60, BINARY nulls only via the bitmap, jagged arrays rejected, 2 GiB string data cap per batch, and no mixing global symbol ids with a local dictionary in one column. The decimal rule is the trap: scale is locked on the first value and later values are automatically RESCALED to it, throwing only on precision loss or capacity overflow. Mis-porting it as lock-and-reject rejects data Java accepts, order-dependently. Adds two post-101 upgrade failures 6.5.1 did not cover, with opposite handling. An unsupported X-QWP-Version is transient at every layer and retried indefinitely -- never a security error -- because a rolling upgrade can leave one node ahead. A durable-ack capability gap is terminal and fails fast, since retrying cannot turn a non-primary into a primary. Records that a batch fitting no split needs its own error class, not a message match: it is retained for a larger-cap node, close() must recognise and discard it rather than abandon already-published rows, and reset() discards it. Notes QwpServerInfo and QwpBatchBuffer are egress-only, so they are not ported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…atrix Fifteenth pass, over the last unreferenced ingest classes. Adds 5.3, the staging buffer. Sections 5.1 and 8.3.1 both said "seal and swap the buffer" without ever saying what is swapped. Java stages encoded messages in a MicrobatchBuffer between the encoder and the ring, keeps two of them, and cycles FILLING -> SEALED -> SENDING -> RECYCLED. Records the six-step swap, including the early return on an empty buffer (which is part of why the commit frame cannot trust batch state) and the 30s recycle wait that 5.1 already referenced as a mid-split failure cause without defining. Makes the Node decision explicit rather than leaving it implied: the second buffer exists because the first stays pinned while read asynchronously after handoff, and in Node that hazard recurs at every await inside append. Either copy on append and drop the swap entirely, or port the two-buffer wait. Recommends copy-on-append, since Node must copy into a Buffer for the write regardless, and notes that choosing it makes the 30s timeout unreachable and that 5.1 should then stop naming it. Completes the coverage matrix in 1.1. RowView, ColumnView, RowCallback, QwpColumnBatch, QwpColumnLayout, QwpBindValues, QwpBindSetter, QueryEvent, QwpEgressIoThread and QwpResultBatchDecoder are result-batch decode and belong to the query spec. QwpSpscQueue, NativeBufferWriter, SegmentedNativeBufferWriter, NativeSegmentList and OffHeapAppendMemory are threading and off-heap allocation with no Node analogue and no protocol semantics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sixteenth pass, over SenderErrorDispatcher and SenderConnectionDispatcher, which section 4.2 had captured only at javadoc level. The inbox drops the OLDEST entry, not the newest. Section 4.2 said only "surplus dropped", and the intuitive bounded-queue implementation -- reject the newcomer when full -- is backwards. Watermarks are monotonic, so the newest entry is always the most informative and dropping the head compresses information rather than losing it; the spec mandates it, and it needs a deque rather than a plain queue. Under load a drop-newest port would retain stale notifications and discard current state. Added to the risks with a test that fills the inbox and asserts the newest survives. Corrects the capacity: default is 256, minimum 16. The spec had quoted the minimum as though it were the whole story and never gave a default. Records that the dispatcher starts lazily on first delivery, that handlers are swappable after connect by design, and that close drains under a short deadline rather than discarding. Narrows the close() suppression test in 8.3.1. Java tracks both "a custom handler ever received any error" and "it received THE latched terminal error", and close() consults only the second. Gating on the first would let a routine RETRIABLE rejection delivered earlier suppress the close-time report of a later, genuinely unsurfaced TERMINAL error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seventeenth pass, over SenderConnectionDispatcher and SenderProgressDispatcher. Fixes an error introduced by the previous pass. Collapsing error_inbox_capacity and connection_listener_inbox_capacity into one table row asserted a shared default of 256. The three dispatchers have different defaults: errors 256, progress 256, connection events 64 -- deliberately smaller because connection events are sparse next to per-batch server errors. The connect-string minimum of 16 applies to the configurable pair; the dispatcher constructor itself only requires >= 1. Completes the connection event list. Kind has seven members and section 4.2 omitted DISCONNECTED. Records the event payload, which the spec had reduced to a bare kind: host and port, the previous host and port, an attempt number, a round number, a cause and a timestamp. The attempt/round pair is what makes a reconnect storm diagnosable. AUTH_FAILED carries the upgrade auth failure as its cause, which is the path by which the terminal credential case reaches the listener before the producer-side throw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eighteenth pass, over the per-key value parsing that ConfigSchema defers to Sender's own helpers. The spec mirrored key names without their grammars. Corrects auto_flush_bytes. The spec claimed a default of 8 MiB, taken from QwpWebSocketSender.DEFAULT_AUTO_FLUSH_BYTES, but the builder's WebSocket default is 0 -- which is exactly what auto_flush_bytes=off sets. Byte-based auto-flush is therefore OFF by default and the trigger is rows and interval only; defaulting to 8 MiB would flush on a trigger Java does not use. Records that an explicit off is preserved even once the server advertises a cap, and that the per-row guard is what makes opting out safe. Adds 9.1.1, the value grammars. Byte counts accept k/m/g and also t (which the javadoc omits but the code handles), with an optional trailing b, case-insensitive, 1024-based. A port reaching for parseInt reads auto_flush_bytes=64m as 64 bytes -- a flush per row, with no error raised. Added to the risks as the most likely silent misconfiguration in the config surface. Enum values are case-insensitive. Adds the SF size defaults the spec lacked: sf_max_segment_bytes 4 MiB, sf_max_total_bytes mode-dependent at 128 MiB memory and 10 GiB disk, sf_sync_interval_millis 5000, max_background_drainers 4, max_name_len 127. Records slot naming in 8.3: a slot is <sf_dir>/<sender_id>/ with sender_id defaulting to "default", so a second sender sharing sf_dir without setting it fails with "sf slot already in use". The error must name sender_id as the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nineteenth pass, over LineSenderBuilder.build()'s validation, which the spec had none of. Corrects sf_durability. Section 8.2 said it selects one of exactly four modes. Four values parse, but build() rejects flush and append as "not yet supported (use sf_durability=memory or periodic)", so only two are usable. The parser should still accept all four so the error names the right cause. Adds 9.2. Every Side.INGRESS key is WebSocket-only and throws when combined with http:: or tcp::, not only the two keys 8.2 mentioned. Three keys are rejected the other way, for WebSocket: protocol_version, a disabled auto-flush interval, and ILP max_backoff. The protocol_version rule is stronger than 3.5 assumed -- Java makes an explicitly supplied value an error under ws::, which turns the silent ILP-fallback hazard into a loud failure, so 3.5 now points at it. Records the dependency rules: periodic durability requires sf_dir, sf_sync_interval_millis requires periodic, drain_orphans requires sf_dir, tls_roots cannot combine with tls_verify=unsafe_off, and WebSocket requires at least one host:port pair. Records that mode selection is implicit: there is no store_and_forward key, sf_dir present means disk mode and absent means memory mode. That single fact drives memory-vs-disk throughout section 8, including the mode-dependent sf_max_total_bytes default, and the spec never stated it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twentieth pass, over InitialConnectMode and SfDurability. Adds 4.3, connect timing, which the spec never addressed -- it described flush and reconnect without ever saying when the initial connection happens. Java has OFF, SYNC and ASYNC, and the default is DERIVED: setting any reconnect_* key implicitly upgrades construction from non-connecting to connecting-with-retry. The reason is that the reconnect_* knobs read as a generic retry budget while the underlying path governs only reconnects from an established connection, so a user who sets a budget and gets no retry on the first connect has hit what Java calls the canonical footgun. Porting the three modes with a fixed default would reintroduce it. Notes that initial_connect_mode is builder-only while initial_connect_retry is the connect-string key. Replaces 8.2's vague "sf_durability governs when fdatasync runs" with the actual semantics: memory never fsyncs explicitly and survives a process crash but not an OS or power crash; periodic checkpoints in the background at a target cadence that is not a bound, since scheduler and storage latency add to the real power-loss window; flush and append are reserved. Records the consequence the spec had obscured: disk mode alone is not power-loss durability. sf_dir selects disk mode but durability still defaults to memory, so files are written and never explicitly synced. Power-loss survival needs sf_durability=periodic, which itself requires sf_dir. This is the same property 8.1.5 records for .symbol-dict, and under the default it applies to the segments too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…defaults Twenty-first pass, over the remaining builder constants. Corrects 8.4's account of quarantine. Setting a torn slot aside is two steps, not one: the slot is RENAMED with a quarantine infix -- deliberately not the sender's own slot name, so a restarting sender cannot re-adopt it as its working slot -- and then marked .failed so the orphan drainer skips it too. The spec had only the sentinel, which stops the drainer but not the owner. Adds the cap: at most 64 quarantined copies of one slot before construction refuses another, since each is an unreplayable slot a human must inspect and unbounded accumulation turns a disk-space problem into a second incident. Adds 9.1.2. Java threads a not-set-explicitly sentinel through every numeric option rather than pre-seeding defaults, so that an explicitly supplied value equal to the default still fails fast. This is a constraint on the port, not a Java idiom: the natural JS shape collapses "unset" and "set to the default", and both the ws:: rejection rules in 9.2 and the connect-mode derivation in 4.3 key on whether a value was supplied rather than on what it is. Records that close bounds the wait and not the connect -- a close racing an in-flight connect cancels it rather than waiting it out -- and adds the defaults the spec lacked: durable_ack_keepalive_interval_millis 200, close shutdown await 30000, quarantine cap 64. Notes the inbox minimum of 16 is sized to exceed the ten error categories so drop-oldest cannot erase the trailing category distribution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-second pass, over build()'s shared prologue. Section 6.5.2 described how tls_verify, tls_roots and tls_roots_password map onto Node's tls.connect but never said they are wss-only. Supplying any of them with plain ws:: throws "tls_verify/tls_roots/tls_roots_password require the wss:: schema" rather than being ignored, and tls_roots_password additionally requires tls_roots. Section 6.5 described the Authorization header as derived from user/password/token without saying how the three interact. They are validated, not inferred: username and password must be supplied together, token is mutually exclusive with both, and the setters are one-shot so configuring either mechanism twice throws "already configured" rather than last-write-wins. Records a cross-client constraint worth honouring: Java deliberately emits the same message text as the egress query client for the username/password rule, so a connect string shared between a sender and a query client fails identically on both sides. Matching those strings keeps a user debugging a shared ws:: string from getting two different diagnoses. Adds the new dependency rules to 9.2 and corrects the key names there and in 6.5 from user to username, the canonical spelling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scope correction from the user: multiple addresses are required for failover, so section 1.2's single-endpoint restriction is wrong and is replaced. Multi-host addressing and failover are now in scope. Every rotation-flavoured behaviour elsewhere in the spec is live rather than vestigial: RETRIABLE_OTHER's rotation, the FAILED_OVER and ALL_ENDPOINTS_UNREACHABLE events, the 421 role reject retried until a primary appears, the mid-stream cap change behind the snapshot-once rule, and the catch-up cap gap on a smaller-cap node. Adds the addr grammar from ConfigView.parseEntry: comma-separated, IPv6-aware, duplicates rejected on (host, port). A custom port on IPv6 requires brackets, since an unbracketed multi-colon entry is read as a bare IPv6 host on the default port. Java's "IPv6 addresses are not supported" throw is UDP-only and does not constrain WebSocket. Adds endpoint selection from QwpHostHealthTracker: rounds with pickNext and beginRound, priority as the lexicographic (state, zoneTier) tuple with state outranking zone, and the state order HEALTHY, UNKNOWN, TRANSIENT_REJECT, TRANSPORT_ERROR, TOPOLOGY_REJECT. Records a finding that narrows the ask: the ingest sender constructs the tracker with the single-argument form, which collapses every zone tier to SAME, so ingest selection is state-only and zone-aware ranking is genuinely an egress feature. zone and target therefore stay accept-and-ignore, and porting zone ranking into the sender would build something Java's sender does not have. Records that background drainers must use a private round cursor with health-only recording so they cannot consume the foreground round, and adds that to the risks. Stack grows to sixteen PRs with 9a and 9b. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-fourth pass, reading sections 1 to 4 and 10 in order. Eight defects, most of them introduced by the scope change itself. 1.1: the QwpHostHealthTracker line was glued to the preceding bullet list and rendered as part of it. 1.2: the selection rule led with Java's (state, zoneTier) tuple and only then said the ingest sender is zone-blind, leaving it ambiguous whether to implement zone tiers. It now leads with the state-only ranking the port needs and says plainly not to implement zone tiers, while asking that the ranking function stay shaped for a later addition. 2: the sources table listed two reference implementations while 1.2 cites a third; .NET is now listed, since Java's tracker javadoc says it mirrors it and that makes it the best cross-check for endpoint selection. 3.2: the module layout had no component for endpoint selection at all after failover came into scope; adds endpoints.ts and hostTracker.ts with a description. 3.2: subsection 3.2.1 sat between two bullets of the module-layout list, interrupting it. Moved after the list. 3.4: the runtime-model table had no row for the connect walk, which is now a real concurrency constraint -- a single in-flight connect, and drainers with their own cursors. 4: the flagship example used a single address, so the spec's headline snippet did not exercise the newly in-scope capability. It now shows a list including an IPv6 literal, and points at 4.3 since construction may or may not connect. 4: the new-surface block showed no callbacks despite 4.2 defining three; adds all three and a pointer to 4.3. 10: mock-server tier described only single-endpoint scenarios; adds the multi-endpoint matrix that 9b needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-fifth pass, continuing the linear read from section 5. 5.3 told the implementer to "choose one and state it" between copy-on-append and porting the two-buffer swap, then recommended copy-on-append without deciding. A spec that defers its own fork leaves PR 3 with an open question, so copy-on-append is now the decision, with the two-buffer swap recorded as a rejected alternative and why. 5.3 also instructed that the 30s buffer-recycle timeout be dropped from 5.1 rather than left as dead prose -- and then 5.1 still listed it as a mid-split failure cause. That is exactly the dead prose 5.3 predicted. 5.1 now names the append deadline as the Node cause and says explicitly that Java's other cause has no analogue here. 5's data-flow diagram listed auto_flush_bytes as a trigger without qualification, contradicting 9.1, where it is off by default. Annotated. 11's table had sixteen rows numbered 1 to 14 with 9a and 9b wedged in, an artifact of inserting failover mid-stack. Renumbered 1 to 16 and corrected the phase description, plus the four cross-references that named PR numbers: the dictionary-mode note, the memory/disk ring note, and the publish-semantics risk, which spanned a now-shifted range. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-sixth pass, linear read of section 6 onward. Section 6.1 listed FLAG_ZSTD and section 6.2 never said which region it covers, which PR 8 could not have answered. Chasing that found the flag does not belong to the ingest path at all. The server-side constant is explicit: FLAG_ZSTD is "set only on RESULT_BATCH frames and only after the handshake negotiated zstd". Every reference in the Java client is on the decode side -- QwpResultBatchDecoder, QwpQueryClient, and a comment in WebSocketClient. The ingest encoder sets FLAG_GORILLA and ORs in FLAG_DELTA_SYMBOL_DICT and never sets FLAG_ZSTD. The negotiation is about the response direction too: the client sends X-QWP-Accept-Encoding to tell the server how to compress result batches, and the echoed X-QWP-Content-Encoding is parsed only so callers can observe the level applied. This corrects a decision taken on a false premise. The spec required zstd on ingest, feature-detected against Node's zstdCompress so the Node 20 floor could be kept. None of that is needed: the ingest sender performs no compression, PR 8 carries only defer-commit and the commit frame, and the version-floor question disappears. The compression and compression_level keys being Side.EGRESS is corroborating evidence rather than coincidence. Compression moves to the out-of-scope list alongside the other egress features. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-seventh pass, linear read of sections 7, 8 and 12. Section 1.2 claimed RETRIABLE_OTHER's endpoint rotation was made live by the failover scope change. Reading section 7 against it shows that is an over-claim: RETRIABLE_OTHER's only category is NOT_WRITABLE (0x0C), which 7.1 already records as reserved and not emitted by any current server. The policy is therefore still unreachable, and rotation is driven by something else entirely -- the tracker recording TRANSPORT_ERROR for a failed connect and TOPOLOGY_REJECT for a 421 role reject, both of which demote a host in the next pick. 1.2 now separates the two, saying plainly that connect-time failure drives rotation and that RETRIABLE_OTHER should still be mapped and implemented but not expected to fire, since today's servers signal the same condition with a reconnect-eligible close. 7.2's row carries the same caveat so the two sections read consistently, and the test matrix and PR 11 no longer name a policy that cannot be triggered -- a mock-server test written against RETRIABLE_OTHER would have needed the server to emit a reserved status byte. Sections 7.1, 7.3, 7.4 and 12 verified consistent: the category count of ten with seven wire-mapped matches Java's own framing, and none of the twenty risk entries referenced the now-removed ingest zstd. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… recovery Twenty-eighth pass, reading section 9 straight through -- it had heavy targeted work across passes 18 to 22 but no linear read since it grew to five subsections. The defaults table was missing six entries, three of which change behaviour rather than tuning: drain_orphans off, request_durable_ack off, transaction off, plus sf_durability defaulting to memory and sf_dir unset meaning memory mode. drain_orphans defaulting to off is the significant one. Combined with sf_dir set, a crashed process's slot is written to disk, survives, and is never drained automatically -- nothing replays it until an operator enables the flag or another sender adopts the slot. The default is defensible, since draining opens background connections at startup, but a user who configures sf_dir expecting crash recovery gets durability without recovery. Recorded in 9.1 and added to the risks, flagged for the README. Two key names do not say what they do, so section 9 now says it: transaction is the defer-commit switch, and sf_dir is what selects disk mode. Resolves an apparent conflict between 6.5 and 9. client_id is Side.EGRESS and accept-and-ignore, which reads as though the sender omits X-QWP-Client-Id. It does not -- the sender always emits its own constant, and the key only lets a query client override its identifier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mplate Twenty-ninth pass, aimed at the one assumption in section 10 that had never been checked: that a Java-side emitter could drive QwpWebSocketEncoder standalone. Tier 1 is the linchpin of the wire-correctness strategy, so an unverified premise there was the largest remaining risk. It holds. QwpWebSocketEncoderTest already drives the encoder with no server, and every class the harness needs is public. Records the concrete shape, including two things that would otherwise cost the implementer a cycle: encode(buffer) is a one-call path for a single table, with the beginMessage / addTable / finishMessage sequence needed only for multi-table frames and for controlling the delta-dictionary bounds; and the buffer is native memory, so the harness must copy out and free rather than leak per fixture, as the existing tests do via assertMemoryLeak. The test also exposed an API-level fact section 6.2.1 had missed. Nullability is a per-column construction choice, not a per-value one: getOrCreateColumn takes useNullBitmap as a parameter and a column created without it cannot represent a null at all. The Node port needs the same decision point at a column's first write. Notes QwpWebSocketSenderMultiEndpointTest as the Java reference for the multi-endpoint test matrix that PR 11 needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirtieth pass, walking PR 1's framing as an implementer writing ws/frame.ts. Pass twelve applied this exercise to the handshake and TLS only; the frame codec had never been walked. Adds 3.2.2. The headline correction: "never fragmented" in 3.2 describes what we SEND, and reading it as an inbound rule produces a client that rejects valid traffic. Java maintains a dedicated fragment buffer and accumulates inbound continuation frames into it, doubling and capped at the maximum receive size, with an explicit error rather than silent truncation on overflow. A server response or an intermediary may fragment even though our data frames do not. Records the rest of what the file needs and the spec did not state: parsing is an incremental state machine resumed across reads, since TCP delivers arbitrary boundaries and one data event is not one frame; the receive buffer is 64 KiB by default and grows when the write position comes within 1 KiB of the end; control frames carry at most 125 payload bytes and are never fragmented; RSV bits must be zero as we negotiate no extensions; inbound frames are never masked and a masked one is a protocol error; and Java's strict mode rejecting non-minimal length encodings is off by default, so accept them on receive while always emitting minimal lengths. Disambiguates a receive-buffer figure in 7.5 that silently referred to the server's 128 KiB buffer while 3.2.2 now documents the client's 64 KiB one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…delta Read every code block. Three defects, one of which invalidates a fix made in the second pass. confirmedMaxId lives on the buffer, starts at -1, and only advances via confirmDeltaPublished(), which the transport's publish path calls rather than sealFrames(). A fresh QwpBuffer with a fully primed dictionary therefore still ships every symbol, because entriesFrom(-1 + 1) returns everything. Both the Task 4 delta assertion and the Task 6 primed arm added in the second pass were measuring the cold case while claiming the steady state. Both now set the baseline too, and Task 4 gains a guard test asserting the cold case is not smaller, so dropping the baseline call fails a test rather than quietly degrading a number. The Gorilla sanity check expected a raw fallback that cannot occur. Every workload uses BASE_TS + i * 1000n, so every delta-of-delta is zero and Gorilla always takes the one-bit path; the fallback needs a delta-of-delta beyond signed int32, about a 35-minute jump at microsecond resolution. Rewritten, and the suite now states plainly that it does not cover that path. WORKLOADS was declared with a literal-keyed Record in the interfaces block but implemented as Record<string, Workload>, so a misspelled workload name would compile and fail at runtime. Now keyed on an exported WorkloadName union. Task 4's build helper also dropped the strings family, the same trap fixed in Tasks 5 and 6 last pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six defects, two of which would have produced confidently wrong numbers. os.tmpdir() is frequently tmpfs, so the disk guard failed in the most reassuring possible direction: the dd baseline would report an excellent figure and every store-and-forward number would be a RAM measurement, with the guard confirming a false conclusion rather than catching it. The same applies to the e2e sf-on arm, whose headline claim is local durability. Both now honour QWP_BENCH_DIR, print the path chosen, and tell the reader to check df -T. Task 7 created an engine inside the bench body, putting acquireSlot, recovery, a dict fd open and a setInterval start inside the timed window. That setup dwarfs 100 appends, so the benchmark reported engine-open cost under an append label. Engines are now opened once in beforeAll with acknowledge(publishedFsn) per iteration to keep trim running, and the comment explains why hoisting is required rather than merely tidier. The e2e sf-on arm wrote to a fixed path and never cleaned up, so a second run would recover the first run's segments and replay stale frames into the measurement. Fresh mkdtemp per run, removed in a finally. Also: warmup and measured rows were the same rows, double-ingesting the first 500 timestamps; arm() had no finally, so a mid-loop throw leaked the sender and left the barrier timer holding the process open; and the warmup size was an inline literal rather than a named constant beside the other knobs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five defects, one of them a spec requirement with no implementing task. benchmarks/README.md had gone stale against six passes of fixes. It is the artefact most likely to be read in isolation before someone quotes a number, and it mentioned none of QWP_BENCH_DIR, that hz is callbacks per second rather than rows per second, rme, or the uncovered Gorilla fallback. All four now appear, with the tmpfs check given its own section. The spec promised output the harness does not produce. Section 4 claimed the bench layers report rows/s, bytes/s, bytes/row and appends/s; vitest bench emits hz and no bytes column at all. Corrected with the conversion spelled out. Section 8 listed "SF append does not dominate whole-flush cost" as an assertion, but it appears nowhere in the plan and cannot meaningfully — append and flush are benchmarked separately at different granularities, so the ratio would be a number without a meaning. Dropped with the reason recorded rather than faked into a task. The spec's bytes/row band of 40 to 80 contradicted the plan's 20 to 120 and neither had been measured; the spec now describes the wide-then-tighten approach. The two assertion lists had diverged in both directions and are reconciled to the five the plan implements. Sections 5 and 7 also updated for the varchar family, the regular-timestamp scope limit, and the tmpfs guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-read Tasks 5 and 6 after the fifth pass changed them, and audited the Self-Review itself. Six defects. Neither the encoder nor its floor consumed its result, so both could be optimised away — and unequally. A floor that gets eliminated while the encoder does not yields a ratio that looks like a finding, which defeats the entire purpose of having a floor. Every arm in Tasks 5 and 6 now accumulates into a module-level sink, built in rather than offered as a remedy after the fact. Task 5 encoded a different table name than Tasks 4 and 6, passing the workload name rather than row.table, so frame sizes differed between tasks measuring nominally the same workload. Buffer was imported and unused. Task 6 pays a bigint to number conversion Task 5 does not, so part of the gap between them is representation rather than buffer overhead; documented at fill(). The File Structure table and the README still said four assertions after the fifth pass added a fifth. The Self-Review's coverage claim was a flat list asserting section 8 wholesale, which is precisely how the missing SF-ratio assertion stayed hidden for six passes. It is now a clause-by-clause table, plus a fourth check for cross-document consistency — the class that produced five of the defects found. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ninth, constraints versus reality: "no new dependencies" was untrue since bench:e2e runs through npx tsx; "benchmarks live outside test/ so pnpm test stays fast" contradicted Task 3, which deliberately includes three test files from that directory; "every generator takes a fixed seed" was false for highCardSymbol; and "tinybench's iteration count covers it" conflated within-run noise with between-run variance. Tenth, adversarial: Task 6's sink read sealFrames().length, which is the frame count rather than bytes — always 1 under CAP — leaving the encoded contents unobserved, while the identical expression in Task 5 was bytes because encodeFrame returns a Buffer. The same code meant two different things in two files. Both e2e arms also wrote the same table, so the sf-on arm ingested into a table already holding 15k rows from sf-off, an asymmetry the single-server guard exists to remove. Eleventh, format compliance: seven of nine tasks had no Interfaces block. That matters because this plan recommends subagent-driven execution, where each task goes to a fresh agent that sees only that task and needs the block to learn neighbouring names and types. Twelfth, spec standalone: the goal claimed the suite shows what SF adds after the ratio assertion had been removed; section 9 implied tinybench reports some GC information when it reports none, unlike Java's GCProfiler arm; and the Gorilla fallback and the per-arm table split were missing from their sections. Thirteenth verified the review log's own claims against the shipped code — eight assertions, all holding. First clean pass in thirteen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourteenth, the e2e script read as a program: the three repeats were not equivalent runs. Workload rows are deterministic, so every repeat resent the same timestamp range into the same table, and after repeat 1 those rows are older than what is already committed — putting QuestDB on its out-of-order commit path. Repeat 1 measured append while repeats 2 and 3 measured O3, and the spread-across-repeats guard would have read that structural difference as machine noise. Each repeat now carries a timestamp stride so all three append forward. This needed QuestDB knowledge rather than TypeScript reading. Fifteenth, the dependency graph: every declared Consumes is satisfied by an earlier task. Tasks 1 and 2 declared Produces without Consumes so the graph could not be read mechanically; both now state it, with Task 2 explaining why consuming nothing is deliberate for a floor. Also notes the SF benchmark's frame is a synthetic 4 KiB buffer while a real trades frame is tens of KiB, so the disk number is the cost of a 4 KiB append rather than a realistic flush. Sixteenth and seventeenth: spec section 6 listed four floors and the plan implemented two. floorWriteStrings was written and called by nothing, so the varchar floor was specified and never exercised; Task 5 now has a varchar arm. The whole-frame floor is dropped from the spec instead, since summing per-column floors ignores the schema, table header and null bitmaps a real frame writes. The Self-Review's coverage table had said three floors when the spec listed four — the same miscount failure the table was rewritten to catch two passes earlier, so counts are now explicit. Eighteenth verified the review log's own numbering: contiguous 1 to 46, no duplicates. No defects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nineteenth, the generators read statistically rather than structurally: all three seeds emitted a near-zero first output, a weakly-seeded xorshift whose state had not diffused, so sparse made row 0's leading columns deterministically null and trades opened at its minimum price. The generator now discards sixteen rounds. Also notes that wide's strings are two to three bytes, so the varchar floor arm mainly measures offset-table overhead. Twentieth, the commands: the tsx justification still claimed the plan adds no dependency, contradicting the Global Constraint the ninth pass had corrected — the fix had not propagated to the paragraph repeating the claim. One Run line also carried a machine-specific absolute path. Twenty-first, whether the assertions prove what they claim. A test titled "gorilla shrinks regularly spaced timestamps" built a LONG column and asserted the encoding is identical with the flag on and off; the body was right and the title wrong, so anyone scanning test names would believe the suite proved compression it never tested. Worse, nothing asserted Gorilla compresses at all, so the suite would pass against an encoder that never compressed — a hole under the one codec the design spec spent most space on. Added a positive assertion. The cold-delta guard was also too loose at half the full-dict size, which would pass if the baseline were partially advancing; tightened to ninety percent. Twenty-second and twenty-third: adding the sixth assertion desynchronised the count in three places, the drift class the seventeenth pass made counts explicit to catch. Reconciled. The compaction assertion was re-derived and does discriminate, since a broken encoder would make sparse larger than dense. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…is unchecked The finding that matters: benchmarks/ is neither type-checked nor linted. tsconfig.json includes only src and the lint script is eslint src/**, so the entire benchmark tree is the only unchecked TypeScript in the repo and a type error would surface as a crash mid-benchmark rather than at build time. Task 3 now adds a separate tsconfig.bench.json — separate so the bunchee build keeps compiling only src — plus typecheck:bench and lint:bench scripts, propagated to the spec's harness section and the README's command block. That falsified two earlier entries in this review log, both corrected in place. Defect 26 claimed pnpm eslint would have flagged an unused import; it would not, since eslint never sees benchmarks/. Defect 13 claimed the WorkloadName union makes a misspelled workload name fail to compile; without typecheck:bench it fails in an editor only. A log that asserts protections which do not exist is worse than no log. Also: every Task N reference was resolved against what that task now is after the fourth pass's renumbering — all nine correct, but two were stale against a later fix, both still describing floorWriteStrings as uncalled after the sixteenth pass had wired it into Task 5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
First batch where the defect rate collapsed. What was checked is recorded so the absence of findings reads as evidence rather than fatigue. Twenty-ninth verified that lint:bench, added in the previous batch, actually works: eslint.config.mjs is a flat config with no files restriction, so it applies to whatever paths are passed. The plan's benchmark code was then checked against the recommended rule set — no any, no non-null assertions, no unused bindings. Clean. Thirtieth found the one defect: strict is unset repo-wide, so the new tsconfig.bench.json catches wrong types and unknown WORKLOADS keys but not a possibly-undefined access such as rows[0] on an empty array. Recorded at the config with an instruction not to raise strict there, since that is a repo-wide decision and enabling it for src through this config would surface pre-existing errors unrelated to benchmarks. Thirty-first rechecked the spec's reference-client claims, asserted in the first pass and never revisited: GCProfiler, both JMH modes, ports 9000 and 8812, criterion, Throughput Elements and Bytes, and extend_from_slice as the floor — all six verified against source. Clean. Thirty-second verified every numeric claim: four workloads, three floors, six assertions, four guards, nine tasks. Clean. Thirty-third checked structure and naming: code fences balanced after roughly thirty edits, and every defined script is referenced and every referenced script defined. Clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every prior pass reviewed Plan B in isolation. Widening to its sibling documents found four defects that in-document review structurally could not. The plans index does not list Plan B: README.md opens "Four plans implement QWP ingest" while five plan documents exist, so the file someone reads to orient themselves omits the newest. Added as a separate track with its own trap list, since benchmarks fail differently from protocol code — they produce a confident number rather than an error. The design spec never pointed at the benchmark work, so a reader of its testing section would not learn that validate.test.ts asserts the wire-format rules that document spends most of its length on. Plan 1 carried the same machine-specific absolute path fixed as defect 50 in Plan B — the class recurs across documents, which single-document review cannot reveal. Both early handoffs pointed at a different Java checkout than the spec pins: questdb-enterprise-4's java-questdb-client is 1.3.3-SNAPSHOT, four behind the pinned 1.3.7. That is exactly the trap design spec section 2 exists to prevent. Severity was checked rather than assumed — that checkout's server-side qwp codecs are post-#7200 with no schema_id, so nobody following it would have encoded the removed field. Both handoffs now carry the version caution. Sibling plans were also checked for Plan B's other defect classes: Plan 4's use of tmpdir is acceptable for crash-recovery tests, and no bare tsx invocations exist anywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Widened review from the plan documents to the handoff chain: the files an executing agent actually opens first. That chain turned out to be misleading in three ways. No handoff mentions Plan B. There are eight handoff files, not four, each superseding the last and recording work that continued well past Plan 4. None of the five deferred handoffs names Plan B or benchmarks, so an agent orienting from the current handoff would conclude the deferred-items list is all that remains, when Plan B is the only planned work actually left. Fixed in HANDOFF-plan8, the head of the chain. Plan B contradicts every handoff about how to run anything: it instructs pnpm bench throughout, while every earlier handoff records that pnpm scripts fail here on the ignored-builds gate. The executing agent would hit a failure on their first command with nothing telling them it was environmental. Plan B and the benchmark spec now carry the caveat and the direct binary invocations. Supersession was discoverable only forwards. Each superseding file declared what it replaced, but no superseded file said it had been replaced, so anyone landing on one directly reads four-generation-stale guidance as current. This is not hypothetical — it happened during this pass, when the first fix for the Plan B omission was written into HANDOFF-plan4-deferred, four generations dead, and was caught only by listing the directory afterwards. All four superseded handoffs now open with a banner naming their successor and the head of the chain. Version consistency was checked and is clean: package.json is 5.0.0 and every surviving 4.3.0 reference explicitly records the change. README's "Four plans implement QWP ingest" was deliberately left as accurate, since Plan B is not ingest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan B was drafted against the post-Plan-4 tree. Handoffs 5 through 8 then landed the delta wiring, orphan drainers, durable-ack and the SFM1 manifest. This batch re-read the plan against src/ as it stands now. The disk arm spikes every tenth iteration and nothing said so. APPENDS times 4 KiB is 400 KiB per body against a 4 MiB segment size, putting a segment roll on roughly every tenth iteration, and the C2 work added an 8 KiB sf-manifest.bin rewrite to that path after this plan was drafted. A reader would see a bimodal distribution and reasonably blame noise or GC. Now documented: read p75 as steady-state append cost and max/p999 as roughly the cost of a roll. That has a second consequence, also now recorded in both the plan and the spec's goal section: memory mode never reaches persistFrame, so it never rolls and never writes a manifest. The gap between the two arms is segment management plus writing, not writing alone, and must not be quoted as the cost of durability. Three things were verified rather than assumed, and are clean. The docs carry no stale unimplemented claims — drain_orphans no longer throws, sf_sync_interval_millis is a valid config key and request_durable_ack reaches the drainer, none of which the docs contradicted. C2 does not break the hand-built segments in Task 7, since scanSegment never reads the flags byte and recovery treats an absent manifest as a clean fallback, throwing only when the manifest sits ahead of the scanned head. And the delta assertions survive B1 because they drive QwpBuffer directly with an explicit setConfirmedMaxId. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…deferred" Audited HANDOFF-plan8's assertion that nothing else is deferred against src/, item by item. The assertion holds, but establishing that required reading source, which is itself the defect. The handoff chain closes items silently. C4 and B1b are listed in HANDOFF-plan4-deferred and plan5-deferred, then simply stop being mentioned in 6, 7 and 8. Both are in fact fixed — transport.connect joins an in-flight connectPromise rather than opening the engine twice, and sendDictCatchUp returns its frame count for acks.onConnected to consume — but "fixed" was indistinguishable from "forgotten" without going to the code. B1b in particular is a data-loss-shaped bug, a mis-attributed ACK over-trimming the ring, so a reader assuming the worst would redo work that was already done. plan8 now carries a closure ledger naming each carried item and where it landed. The e2e repeats are comparable by accident, and the accident is deletable. An earlier pass fixed timestamp contamination across repeats with REPEAT_TS_STRIDE. The identical hazard exists through the symbol dictionary: repeat 1 would pay full-dict encoding plus write-ahead symbol persistence while later repeats recover a dictionary and run delta. It does not currently bite, because each repeat builds a fresh Sender and the sf-on arm varies sender_id per repeat. But that id was chosen for slot-lock isolation, nothing recorded it as load-bearing for comparability, and collapsing it to one id is an obvious tidy-up. Delta wiring went live in handoff 6, so this is a real difference now rather than a latent one. Three items were verified clean against source: C1 no longer writes the watermark per ACK, C3 reaches the wire via the drainer, and B1a uses varintSize(symbolId) with a regression test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…shipped Reviewed spec section 9.1, config keys and defaults, against src/. Two defects, one of them the most consequential finding of the review so far. The mode-dependent sf_max_total_bytes default was never implemented. Spec 9.1 specifies 128 MiB in memory mode and 10 GiB in disk mode, and section 8 leans throughout on sf_dir presence driving memory-versus-disk defaults. The shipped code defines only MEMORY_MAX_TOTAL_BYTES at 128 MiB and applies it unconditionally regardless of sf_dir; the drainer hardcodes the same value. 10 GiB appears in no source file and no test. A disk-mode user who does not set the key explicitly therefore gets roughly 80x less retention than designed: the ring caps 80x sooner during an outage and begins shedding unacked frames, which is precisely the guarantee store-and-forward exists to provide. Recorded as OPEN in HANDOFF-plan8 and marked in the spec's defaults table, but not fixed, because this review is document-only. segmentBytes was checked and needs no branch, since spec 9.1 gives one value for both modes. The spec also named a config key that does not exist. Its accepted-key list and defaults table both said sf_max_segment_bytes while ValidConfigKeys accepts sf_segment_bytes, so a config string copied out of the spec is rejected as an unknown option. Both occurrences now carry the shipped name, with the divergence recorded rather than silently resolved: the shipped name is asymmetric with its neighbour sf_max_total_bytes, and a config key is public API, so renaming is much cheaper before 5.0.0 ships than after. The package README was checked and is current. Four other apparent key mismatches were checked and are not defects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… shipped code Continued the previous batch's unit, spec 9.1 against src/, because it was the highest-yield one so far. Four more defects, all recorded in HANDOFF-plan8 and none fixed, since this review is document-only. Poison-frame detection is not wired at all. PoisonDetector is never constructed anywhere in src/ — grep finds it only in its own unit test. The client therefore has no poison escalation: a repeatedly rejected frame never reaches quarantine, and the strikes-and-dwell rule the spec and every handoff trap list emphasise is inert. Both keys that tune it are accepted, validated, then silently ignored, which is worse than rejecting them. sf_append_deadline_millis is in the same state. A sweep of all 38 accepted keys found exactly these three QWP keys with no consumer outside options.ts. That is the failure mode the previous batch predicted, actually occurring. HANDOFF-plan3-to-plan4 stated the gap plainly. The Dispatcher half of that same warning was wired; the poison half was not, and it then dropped out of handoffs 4 through 8 in silence. Because the chain stopped mentioning items once they were resolved, fixed and forgotten were indistinguishable — and here one was genuinely forgotten. Three spec-9.1 keys are not accepted at all: durable_ack_keepalive_interval_millis, auth_timeout_ms and catch_up_cap_gap_min_escalation_window_millis are absent from ValidConfigKeys and from src/ under any name, so setting any of them throws as an unknown option. The keepalive has teeth, since with request_durable_ack on, nothing bounds a durable-ack wait. max_background_drainers defaults to 1 rather than the specified 4, so orphan slots drain four times less concurrently than designed after a crash. auto_flush_interval is still 1 s rather than 100 ms. Rows delegate through transport.getDefaultAutoFlushRows, but no matching interval hook exists, so the sender falls back to a hardcoded module constant. This is exactly the change spec 9.1's own prose says must be added, never made. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e unenforced limit Continued on spec-versus-shipped, moving from the 9.1 defaults table to the normative wire and limits sections. Yield dropped sharply, which is informative in itself: the protocol encoding is in good shape, and the gaps found so far are in configuration and lifecycle wiring rather than the format. MAX_ROWS_PER_TABLE is declared but never enforced. constants.ts defines it at one million and nothing reads it, while all three of its neighbours are enforced. Reachability is narrow, since auto_flush_rows defaults to 1000, so it needs auto-flush raised or disabled or a long defer-commit transaction. The failure is not narrow: the server rejects the oversized frame, under store-and-forward that frame is already durable and replays forever, and with PoisonDetector unwired nothing ever escalates it. The two gaps compose into a permanent stall, precisely what the poison design exists to prevent. Recorded with a caution about the fix, since the spec calls it DEFAULT_MAX_ROWS_PER_TABLE, a server-side default an operator may raise, so a hardcoded client throw could reject frames a correctly configured server would accept. Verified correct: the frame header magic, version and size match spec 6.1; the three flag bits match and FLAG_ZSTD is correctly absent as egress-only; all 24 column type codes match spec 6.3 including the irregular ones; the error taxonomy has exactly the specified 10 categories and 4 policies; and quarantine is implemented with the 64-copy cap correctly scoped per slot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- sf_max_total_bytes is now mode-dependent: 128 MiB in memory mode, 10 GiB in disk mode (transport + orphan drainer), so a disk user who omits the key gets ~80x the retention and does not shed unacked frames early. - max_background_drainers defaults to 4 (was 1) per spec 9.1. - QWP auto_flush_interval defaults to 100 ms via a new per-transport getDefaultAutoFlushInterval() hook (was a hardcoded 1000 ms for all). - PoisonDetector is now wired into the transport: a RETRIABLE NACK keys a strike on the rejected frame's FSN, a non-orderly close after a send keys a strike on the head-of-line frame, and OK-at-or-beyond clears — escalation (strikes AND dwell) latches a terminal PROTOCOL_VIOLATION and stops replay. - MAX_ROWS_PER_TABLE is enforced at seal time with a clear diagnostic, before any byte is published. - The three spec-9.1 keys that were rejected as unknown are now accepted and wired: durable_ack_keepalive_interval_millis (keepalive PING, <= 0 disables), auth_timeout_ms (ws connect+handshake timeout), and catch_up_cap_gap_min_escalation_window_millis (orphan-drainer dwell).
nwoolmer
commented
Aug 8, 2026
nwoolmer
left a comment
nwoolmer
left a comment
Contributor
Author
There was a problem hiding this comment.
Overview
This PR adds QWP WebSocket ingestion, store-and-forward durability, failover, protocol codecs, configuration, documentation, benchmarks, and tests. Five reachable correctness, data-loss, and security defects block the draft.
Concerns
- Cross-process slot locking can let two live senders mutate the same durable state.
- Symbol persistence can corrupt recovered symbol IDs after a failed publication.
- Durable-ACK mode trims replay state before the durable acknowledgement.
- NACK policy is classified but not enforced, allowing rejected frames to be skipped.
- Malformed inbound traffic can escape the socket callback as an uncaught exception.
Validation
Direct tool execution passed TypeScript checks, source lint, benchmark type checks, and 361 non-integration tests across 51 files. Benchmark lint completed with only the existing ignored-README warning. The pnpm script wrapper itself was blocked by pnpm 11's ignored dependency build-script precheck.
Verdict
Found: 5 critical, 0 important, 0 suggestions
Request changes
| if (code !== "EEXIST") throw e; | ||
| const [pidStr, boot] = (await readFile(lockPath, "utf8")).split("\n"); | ||
| const pid = Number.parseInt(pidStr, 10); | ||
| const stale = boot !== bootId() || !isAlive(pid); |
Contributor
Author
There was a problem hiding this comment.
🚨 [CRITICAL] Foreign live holders are classified as stale. BOOT_ID is this process's start timestamp, not a machine boot identifier, so a second live process computes a different value and unlinks the first process's lock even while isAlive(pid) is true. Concurrent senders can then corrupt the same segments, manifest, watermark, and dictionary. Use a kernel-backed lock or a genuine machine boot ID plus the foreign PID's start identity; never steal a lock solely because its owner started at another time.
| // confirmDeltaPublished() on that ring-append success. | ||
| const delta = this.dict !== undefined; | ||
| if (delta) { | ||
| const fresh = this.dict!.entriesFrom(this.confirmedMaxId + 1); |
Contributor
Author
There was a problem hiding this comment.
🚨 [CRITICAL] Persisted symbols use the wire-confirmation cursor. Persistence completes before publication, but confirmedMaxId advances only after ring append. If publication fails, the next seal writes those symbols again; positional recovery can turn [A] then [A,B] into IDs [A,A,B] while live frames encoded B as ID 1, silently replaying B as A. Track a separate persisted cursor and advance it only after a complete side-file append.
| } | ||
| return; | ||
| } | ||
| if (r.status === STATUS.DURABLE_ACK) return; |
Contributor
Author
There was a problem hiding this comment.
🚨 [CRITICAL] Durable acknowledgements never control retention. The ordinary OK path already calls engine.acknowledge() and trims replay state, while DURABLE_ACK is discarded here. With request_durable_ack=on, a server failure between those responses can lose acknowledged data. Retain each FSN until the durable table transaction watermarks cover it; only non-durable mode may trim on OK.
| // counts: it is a verdict on the node, not the bytes. Escalation needs | ||
| // strikes AND dwell (the detector enforces both); a brief outage answered | ||
| // with pacing must not become producer-fatal. | ||
| if (policy === Policy.RETRIABLE) { |
Contributor
Author
There was a problem hiding this comment.
🚨 [CRITICAL] NACK policies are classified but never applied. Non-escalated retryable, other-node, and terminal outcomes only emit an event; they neither replay nor block later acknowledgements. A subsequent OK can map to a later FSN and trim through the rejected head, permanently losing that batch. Reconnect and replay retryable NACKs, latch deterministic terminals, and prevent the ACK watermark from crossing an unresolved rejection.
| }); | ||
| } | ||
|
|
||
| private onData(chunk: Buffer): void { |
Contributor
Author
There was a problem hiding this comment.
🚨 [CRITICAL] Malformed inbound traffic can escape the socket callback. FrameParser.next() throws for invalid WebSocket framing, and the BINARY callback can throw while decoding QWP responses. Because onData runs directly inside an EventEmitter data handler without a catch, a bad peer can terminate the Node process. Catch parser/application failures, destroy the socket, and notify the transport exactly once.
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
1 participant
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.
QWP ingest over WebSocket (
ws://) with store-and-forward durability + benchmark suiteThis branch ships the QuestDB Wire Protocol (QWP) ingest path for the Node.js client — a columnar, binary WebSocket protocol — alongside durable store-and-forward and a benchmark suite that validates the implementation.
~118 commits, +8.2k/−91 lines. This PR is a draft for review.
The internal planning/spec documents (
docs/superpowers/) that drove this work are intentionally not part of this public branch — they were removed in the final commit. User-facing documentation ships viaREADME.md.1. What this adds
A fully parallel ingest transport for
ws:///wss://, selected automatically when a connection string uses thews:scheme. HTTP/ILP (v1/v2) and TCP transports are unchanged and remain the default.Protocol engine (
src/qwp/)protocol/)VARCHAR,BINARY, arrays, decimals (64/128/256 + rescale), geohash,UUID,LONG256, with per-type column locks and a row-rollback guard if any setter throws mid-rowResilience & failover
wss://addr=parsing (incl. IPv6), state-ranked host tracker, reconnect with endpoint rotation, and dictionary catch-up after reconnectStore-and-forward (
src/qwp/sf/) — durable ingestion that survives a client crash/process restart.ack-watermark), persisted symbol dictionary (SYD1), and a chain-head manifest (sf-manifest.bin/ SFM1) that detects a lost tail segment during recoveryrequest_durable_ack)Version: bumped to 5.0.0 (the
ws://surface and new config keys are additive to the client API).2. Benchmarks (
benchmarks/)A self-contained, ad-hoc benchmark suite (no CI job, no new dependencies) — encoder throughput vs hand-written floors, row-building overhead, store-and-forward append cost, and end-to-end flush latency. Run with
pnpm bench/pnpm bench:e2e; also addsvalidate.test.ts(6 wire-format invariants) that runs underpnpm test.benchmarks/files are type-checked (tsconfig.bench.json+typecheck:bench) and linted (lint:bench).Current numbers (this machine, ext4
/tmp, live QuestDB on:9000)hz= callbacks/s; ×10,000 rows = rows/s.encodeFrametrades (gorilla off / on)encodeFramewide (50 cols)encodeFramesparseSymbolDict.getOrAddvs naive interningQwpBufferbuild+seal trades / sparse / widedddisk baseline 2.8 GB/s)scanSegment1000 framesEnd-to-end
flush()latency (5000 rows × 3 repeats):3. Testing & gates
tsc --noEmitclean;eslint src/**clean;typecheck:bench/lint:benchcleanvitest run: 361 passed / 13 skipped (+7 tests added in the spec-gap closure below); the benchmarkvalidate.test.tsruns in the normal suitetest/sender.integration.test.ts(TestContainers) — Docker is unavailable in this environment; a pre-existing limitation unrelated to these changeslocalhost:9000): green4. Documentation
README.mddocumentsws://, the durable-ack flow, and store-and-forward caveats (including thesf_durability=periodicbarrier anddrain_orphans/request_durable_ack)benchmarks/README.mddocuments how to run and read the benchmark suite5. Spec gaps noted in review — now closed (commit
215b44f)The five gaps recorded under this PR's earlier "Known limitations" have all been addressed:
sf_max_total_bytesdefault — now 128 MiB memory / 10 GiB disk (transport + orphan drainer), so a disk-mode user who omits the key gets ~80× the retention and does not shed unacked frames during an outage.PoisonDetectoris now wired into the transport lifecycle — aRETRIABLENACK keys a strike on the rejected frame's FSN; a non-orderly close after at least one send keys a strike on the head-of-line frame; OK-at-or-beyond clears. Escalation (strikes and dwell) latches a terminalPROTOCOL_VIOLATIONand stops the replay loop, so a poisoned frame can no longer replay forever. Both tuning keys (max_frame_rejections,poison_min_escalation_window_millis) are read.MAX_ROWS_PER_TABLEis enforced at seal time with a clear diagnostic naming the table and the remedy, before any byte is published.durable_ack_keepalive_interval_millis(keepalive PING, ≤0 disables),auth_timeout_ms(ws connect+handshake timeout), andcatch_up_cap_gap_min_escalation_window_millis(orphan-drainer dwell).max_background_drainersnow defaults to 4 (was 1), and QWPauto_flush_intervaldefaults to 100 ms via a new per-transportgetDefaultAutoFlushInterval()hook (was a hardcoded 1000 ms).