feat(qwp): stop resending the full symbol dictionary on every message#66
Open
glasstiger wants to merge 99 commits into
Open
feat(qwp): stop resending the full symbol dictionary on every message#66glasstiger wants to merge 99 commits into
glasstiger wants to merge 99 commits into
Conversation
Previously every QWP ingress message re-sent the entire symbol
dictionary, so a connection with many distinct symbols paid to
retransmit the whole dictionary on every message. The client now
sends each symbol id to the server only once per connection.
Memory mode:
- The producer keeps a monotonic "sent" watermark and each frame
carries only the ids above it (a delta section), instead of the
full dictionary from id 0.
- On reconnect or failover the fresh server has an empty dictionary,
so the I/O thread replays the whole dictionary as a catch-up frame
before any post-reconnect traffic, keeping the producer's monotonic
baseline valid across the wire boundary.
Store-and-forward (file mode):
- Each slot persists its dictionary to a dot-prefixed side-file
(PersistedSymbolDict) using write-ahead ordering: new symbols are
appended before the referencing frame is published, so a recovered
or orphan-drained slot on a fresh process can always rebuild the
dictionary that a delta frame references.
- The persistence does not fsync, matching the rest of
store-and-forward, which is process-crash durable (the page cache
survives) but not host-crash durable. A host crash that tears the
dictionary is caught at replay by a guard that fails the send
cleanly ("resend required") instead of transmitting a gapped frame
that would corrupt the table.
Catch-up split:
- The reconnect/recovery catch-up splits across as many frames as the
server's advertised batch cap requires, so a dictionary larger than
the cap is re-registered without any single frame exceeding it. The
frames carry contiguous id ranges and reassemble on the server
exactly as the original per-frame deltas would.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
glasstiger
added a commit
to questdb/questdb
that referenced
this pull request
Jul 9, 2026
Update the java-questdb-client submodule to de86197, which makes the QWP client register each symbol id with the server only once per connection (delta symbol dictionary) instead of re-sending the whole dictionary on every ingress message. The OSS server already parses delta symbol-dictionary frames, so this is the OSS half of a tandem pair with the client PR questdb/java-questdb-client#66 and needs no server change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Tandem OSS PR (submodule bump): questdb/questdb#7374 — merge together. |
The symbol-dictionary catch-up called fail() on a send error, but the catch-up runs inside connectLoop (via swapClient) and, on the initial connect, on the caller thread (via start() -> positionCursorForStart). Calling fail() there re-entered connectLoop. On a reconnect this corrupted the wire mapping: the outer setWireBaselineWithCatchUp overwrote fsnAtZero while nextWireSeq kept the nested attempt's value, so a later ACK translated through engine.acknowledge(fsnAtZero + wireSeq) and trimmed un-acked frames from the store-and-forward log -- silent data loss. A flapping connection recursed connectLoop until the stack overflowed into a terminal, turning a transient outage into a hard failure (breaking Invariant B). On the initial connect the same fail() ran connectLoop on the caller thread and blocked Sender construction forever. sendDictCatchUp and sendCatchUpChunk now throw CatchUpSendException instead of calling fail(). connectLoop's own retry catch handles the swapClient path (one non-re-entrant reconnect with backoff); trySendOne's orphan-retire re-anchor turns it into a fresh fail() from the I/O loop body; start() drops the dead client so the I/O thread reconnects and re-sends the catch-up off the caller thread. A single dictionary entry too large for the server batch cap is non-retriable, so it latches a terminal (recordFatal) rather than looping -- also removing the oversized-entry reconnect livelock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
persistNewSymbolsBeforePublish keyed the append range off sentMaxSymbolId+1. That watermark only advances after the whole frame is published, whereas PersistedSymbolDict.size() advances per persisted entry. If a mid-batch appendSymbol threw (a short write on a full disk), the symbols before the failing one were already durable but the frame was not published, so sentMaxSymbolId stayed put. A retry then re-keyed from sentMaxSymbolId+1 and re-appended that already-persisted prefix, duplicating entries and breaking the dense id->symbol mapping recovery relies on (entry i must be symbol id i) -- a torn dictionary that re-registers the wrong symbols on the fresh server, or diverges the producer's watermark from the I/O thread's mirror. Resume from pd.size() instead: it is exactly the count already durable, so the retry continues past the persisted prefix (the next append overwrites any torn trailing bytes) without duplicating. In the happy path pd.size() equals sentMaxSymbolId+1, so behaviour is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a regression test: a dictionary entry larger than the reconnect
server's per-chunk catch-up budget must latch a clean terminal, not
reconnect-loop. Connection 1 advertises no cap so a ~200-byte symbol
registers into the sent-dictionary mirror; the handler then shrinks the
advertised cap and drops the socket, so the reconnect's catch-up cannot
re-ship the entry. The test asserts the surfaced terminal names the
catch-up path ("... during catch-up").
Reverting the fix (entry-too-large calling fail() again) fails this test
with a StackOverflowError on the I/O thread -- the catch-up re-entering
connectLoop -- confirming the guard bites both ways.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
persistNewSymbolsBeforePublish appended each new symbol with its own PersistedSymbolDict.appendSymbol call, and each appendSymbol issues one positioned write. A high-cardinality batch -- one new symbol per row, which is exactly the store-and-forward workload delta encoding targets -- therefore stalled the producer thread with up to one pwrite syscall per row per flush. Add PersistedSymbolDict.appendSymbols(dict, from, to): it encodes the whole [from..to] entry region into scratch once and issues a single positioned write, so a flush that introduces N symbols costs one syscall instead of N. It keeps appendSymbol's durability and idempotency contract -- no fsync, and a short write throws without advancing size, so a retry keyed off size() re-encodes and overwrites at the same offset. PersistedSymbolDictTest.testAppendSymbolsBatchWritesDenseRange checks the batched write produces the same dense, id-ordered file (including an empty symbol mid-range), that an empty range is a no-op, and that a follow-on batch keyed off the recovered size continues without a gap or duplicate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On recovery / orphan-drain the CursorWebSocketSendLoop constructor seeds a native mirror (sentDictBytesAddr) from the slot's persisted dictionary so the first connection can re-register it. That mirror is freed only on ioLoop's exit path, so a loop that is constructed but never runs -- start() never called, or Thread.start() failing before the loop runs, or a close() racing an unstarted loop -- leaked it. close() already safety-nets the client for that same "loop never started" case; the mirror was missed. close() now frees the mirror when the loop never ran (ioThread was null on entry). It does NOT free it when the loop ran: ioLoop's exit owns the free there, and on the failed-stop path the thread may still be mid-send, so touching the mirror would race; a duplicate close observes a zero address and skips. CursorWebSocketSendLoopMirrorLeakTest populates a recoverable slot, then leak-checks constructing an engine + loop over it and closing WITHOUT start(). Reverting the free fails it with a 4096-byte NATIVE_DEFAULT leak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
testRecoveredSlotReplaysDeltaFramesAgainstFreshServer never acked in phase 1, so recovery replayed from the very first frame -- whose delta already starts at id 0. The replayed frames were thus self-sufficient from 0, and the reconstructed-dictionary assertions passed whether or not the seeded catch-up carried the right symbols (or any at all). Only the sawCatchUpFrame existence check was load-bearing. Stamp the ack watermark at FSN DISTINCT_SYMBOLS-1 between the phases so recovery replays from the first frame past the symbol-introducing cycle: a frame with deltaStart=DISTINCT_SYMBOLS carrying no new symbols. The early ids it references now exist only in the persisted dictionary, so the reconstructed dictionary is complete solely because the catch-up re-registered them. Verified both ways: with a catch-up that sends a table-less frame but no symbols, the pre-change test still passes (the head frames carry the dictionary) while the stamped test fails at "dictionary id 0 expected sym-0 but was null". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a disk-mode slot's .symbol-dict cannot be opened, the engine reports delta encoding as unavailable and the sender must fall back to self-sufficient frames -- every batch re-ships the whole dictionary from id 0 -- because a recovered slot would have no dictionary to rebuild non-self-sufficient deltas from. Nothing exercised that path. Add a test that plants a directory where the dictionary file belongs, so openRW / openCleanRW fail and open() returns null. It then asserts both batches ship deltaStart=0 and that batch 2 re-ships the whole dictionary (deltaCount=2), rather than the monotonic delta (deltaStart=1, deltaCount=1) the enabled path emits. Verified it bites: forcing isDeltaDictEnabled() to stay true regresses batch 2 to deltaStart=1 and the test fails. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
openExisting parsed complete entries and set appendOffset past the last one, but left the file at its full length. A crash mid-append leaves a torn trailing record; if the next append after recovery is SHORTER than that torn tail, it overwrites only the tail's prefix and leaves residue beyond its own end. A later recovery can then mis-parse that residue as a ghost symbol, shifting every subsequent dense id -- so the "self-healing tail" guarantee was not actually airtight. open() now truncates the file to the end of the last complete entry (ftruncate) so nothing survives past appendOffset. Best-effort: a failed truncate falls back to the prior overwrite-from-appendOffset behaviour. testTornTrailingEntrySelfHeals now asserts the file returns to its clean length after the reopen; reverting the truncate fails it (19 vs 16 bytes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The I/O thread's lifetime-monotonic symbol-dictionary mirror is sized with int math: accumulateSentDict passed sentDictBytesLen + regionBytes (an int sum) to ensureSentDictCapacity, and the grow step doubled capacity*2, also int. On a pathological, very-high-cardinality connection the sum overflows negative -- so the capacity check passes and copyMemory scribbles past the buffer (silent heap corruption) -- and capacity*2 overflows negative near 1 GB, degrading the doubling to exact-fit reallocs. Reaching this needs ~200M+ distinct symbols on one connection, far past any real workload, but the failure mode is silent corruption. ensureSentDictCapacity now takes a long, the caller passes a long sum, and the method throws a LineSenderException above an int-addressable ceiling (Integer.MAX_VALUE - 8) instead of overflowing, growing in long math clamped to that ceiling. Defensive only -- not reachable at realistic symbol cardinality, so there is no scale test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bluestreak01
requested changes
Jul 9, 2026
bluestreak01
left a comment
bluestreak01
left a comment
Member
There was a problem hiding this comment.
Review of PR #66 — feat(qwp): stop resending the full symbol dictionary on every message
Reviewing at level 3 (full mission-critical pass: all steps, all reviewer dimensions, per-finding source verification). Note: the subagent tool is unavailable in this environment, so the parallel-reviewer passes and per-finding verification were run inline by the parent session using read/bash against the source and a local build+test run — not delegated. Every finding below was verified against the cited source lines; false positives are listed in Downgraded.
Build/test evidence: mvn -pl core compile clean on JDK 25; DeltaDictCatchUpTest, DeltaDictRecoveryTest, PersistedSymbolDictTest, SelfSufficientFramesTest, ReconnectTest → 15 tests, 0 failures.
Committed-binary gate: PASS — git diff --numstat shows no binary files; all 10 changed files are .java with numeric line counts.
Critical
C1 — Persisted .symbol-dict accumulates duplicate entries when appendBlocking fails and a later flush succeeds → silent symbol corruption on recovery (file mode, delta enabled). [in-diff]
File: core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java:3660-3676 (persistNewSymbolsBeforePublish), triggered via flushPendingRows (3491/3498) and flushPendingRowsSplit (3574/3582).
Code-path trace (verified):
flushPendingRows runs, in order:
persistNewSymbolsBeforePublish(); // 3491 — appends [sentMaxSymbolId+1 .. currentBatchMaxSymbolId] to .symbol-dict (Files.write, no fsync)
activeBuffer.write(...); // 3494
sealAndSwapBuffer(); // 3495 — calls cursorEngine.appendBlocking(); CAN THROW
advanceSentMaxSymbolId(); // 3498 — SKIPPED on throw
...
resetTableBuffersAfterFlush(keys); // SKIPPED on throw → rows + currentBatchMaxSymbolId preservedsealAndSwapBuffer → appendBlocking throws LineSenderException("cursor SF append failed", …) on the two documented conditions (QwpWebSocketSender.java:3768,3783-3785): backpressure deadline (the SF ring hit sf_max_total_bytes and did not drain — i.e. exactly the store-and-forward stress scenario, server slow/down) and PAYLOAD_TOO_LARGE. The I/O loop is not failed, so cursorSendLoop.checkError() passes and the sender stays open and usable.
On the throw: the frame's new symbols are already durably on disk (persist ran before sealAndSwapBuffer), but sentMaxSymbolId was not advanced (advanceSentMaxSymbolId at 3498 skipped) and the table buffers/currentBatchMaxSymbolId are not reset (resetTableBuffersAfterFlush skipped — verified: currentBatchMaxSymbolId is reset only at 3607, 3686, and inside resetTableBuffersAfterFlush, none of which run on this path).
The next successful flush() (a transient backpressure clears the moment the server catches up) re-enters persistNewSymbolsBeforePublish with the same from = sentMaxSymbolId + 1 (3668) and to = currentBatchMaxSymbolId (3669) — because pd.appendSymbol has no dedup (PersistedSymbolDict.java:appendSymbol) and nothing rolled back the earlier append, the failed frame's symbols are written to the file a second time. The file's positional invariant ("symbol id i is the i-th entry", PersistedSymbolDict.java class doc) is now broken.
Impact on recovery/orphan-drain (a fresh process reads the file):
seedGlobalDictionaryFromPersisted(2243/3695) callsgetOrAddSymbol, which de-dupes → producerglobalSymbolDictionary.size()andsentMaxSymbolIdare below the file's entry count.- The send loop's constructor seeds the mirror directly from the raw file bytes with
sentDictCount = pd.size()(CursorWebSocketSendLoop.java:515-522), i.e. including the duplicate. sendDictCatchUpre-registers the duplicated mirror on the fresh server, so every global id above the duplicate is shifted by +1.- Symbol column cells are encoded as absolute global ids (
QwpColumnWriter.writeSymbolColumnWithGlobalIds, line 277buffer.putVarint(globalId)). The replayed frames carry the original ids, which now resolve against the shifted server dictionary → rows get the wrong symbol values, silently. The torn-dictionary guard does not catch this (deltaStartnever exceeds the now-largersentDictCount, sotrySendOneat 2223-2238 passes).
This is a store-and-forward data-integrity violation triggered by an ordinary transient outage — the exact failure class SF exists to survive.
Suggested fix: base the append range on the true persist watermark, not the wire baseline. pd.size() already tracks how many symbols are durably persisted at contiguous ids 0..size-1:
int from = pd.size(); // instead of sentMaxSymbolId + 1
int to = currentBatchMaxSymbolId;
if (to < from) return;
for (int id = from; id <= to; id++) pd.appendSymbol(globalSymbolDictionary.getSymbol(id));In the happy path pd.size() == sentMaxSymbolId + 1, so behavior is identical; after a failed append it skips the already-persisted ids, making the operation idempotent across retries. Add a regression test: file mode + delta, force an appendBlocking failure (small sf_max_bytes + silent server), then a successful flush, then assert .symbol-dict has no duplicate and a fresh-process recovery reconstructs the dictionary gap-free.
C2 — Required Enterprise failover tandem is missing/unlinked; the HA path this feature targets is UNTESTED in CI (Step 2.7 gate). [tandem]
Verification (commands recorded):
- OSS tandem:
gh pr list --repo questdb/questdb --head qwp-delta-symbol-dict→ #7374 present, matching branch, bidirectionally linked (body: "Tandem OSS half of #66"; a PR comment links back). It is a submodule bump only — "The OSS server already parses delta symbol-dictionary frames, so no server change is required." Its CI covers single-node QWP e2e. - Enterprise tandem:
gh pr list --repo questdb/questdb-enterprise --head qwp-delta-symbol-dict→ empty.ghcan reach the private enterprise repo (confirmed), and a scan of the 60 most-recent enterprise PRs shows no client-bump/qwp-symbol-dict PR.SqlFailoverQwpClientLosslessTestexists in enterprise (questdb-ent/src/test/java/com/questdb/lifecycle/), and the PR body claims it "passes end-to-end against a real server" — but with no enterprise PR bumping the client submodule, that test runs against the old client in enterprise CI, not this change.
Why this trips the gate: the change is squarely HA-facing — it rewrites the SF drainer's on-the-wire framing, adds reconnect/failover dictionary catch-up (swapClient → setWireBaselineWithCatchUp → sendDictCatchUp), and adds recovery/orphan-drain dictionary rebuild. The headline benefit (dictionary survives a reconnect/failover) is only proven end-to-end by the enterprise failover suite the PR itself names. Per Step 2.7, a required-but-missing tandem is Critical and every behavior it would cover is treated as UNTESTED. The client-local loopback tests (C-tier coverage below) are strong, but they cannot prove (a) a real server accepts and correctly registers a 0-table catch-up frame mid-stream, or (b) primary→replica failover preserves the dictionary.
Required action: open (or link) the enterprise tandem that bumps the client submodule to this SHA and runs SqlFailoverQwpClientLosslessTest (and, ideally, a kill-9 recovery variant in the enterprise e2e-python suite for the file-mode host-crash/torn-dict path, which the unit test only simulates by truncating the file). Also confirm OSS #7374's e2e actually drives a reconnect (so the catch-up frame is exercised against a real server), not just a single connected ingest.
Moderate
M1 — One Files.write syscall per new symbol on the producer thread. [in-diff]
persistNewSymbolsBeforePublish (3660-3676) loops pd.appendSymbol(...), and each appendSymbol (PersistedSymbolDict.java) issues its own Files.write(fd, …) (one pwrite). A frame that introduces K new symbols does K syscalls on the user/producer thread. This is per-new-symbol (not per-row), so it's bounded by dictionary growth, but a high-cardinality first batch will burst syscalls synchronously in the flush path. Batch the frame's whole new-symbol range into a single scratch buffer and one Files.write. Not zero-GC-blocking (no allocation), but avoidable syscall amplification on the ingestion path.
M2 — accumulateSentDict silently drops symbols on a partial-overlap delta. [in-diff]
CursorWebSocketSendLoop.java:1946-1960: the guard is if (deltaCount <= 0 || deltaStart != sentDictCount) return;. A delta with deltaStart < sentDictCount and deltaStart + deltaCount > sentDictCount (overlaps the tip and extends past it) is dropped entirely — the new tail symbols never enter the mirror, so a later catch-up would be incomplete (→ the same shifted-id corruption as C1). I verified this is currently unreachable: the producer emits strictly contiguous, non-overlapping deltas (beginMessage computes deltaStart = confirmedMaxId+1; advanceSentMaxSymbolId moves the baseline to exactly currentBatchMaxSymbolId), and recovery seeds sentDictCount from a superset, so deltaStart < sentDictCount ⇒ deltaStart+deltaCount ≤ sentDictCount. But it is load-bearing correctness resting on an invariant enforced elsewhere. Harden it: handle the partial overlap (accumulate only the [sentDictCount .. deltaStart+deltaCount) tail) or assert deltaStart + deltaCount <= sentDictCount so a future producer change fails loudly instead of silently corrupting the mirror.
Minor
m1 — Stale "self-sufficient / delta from id 0" comments now contradict delta mode.
QwpWebSocketSender.java:3392, 3398-3399, and 3777 still say cursor frames are "self-sufficient (every frame carries … a symbol-dict delta from id 0)". In delta mode frames are explicitly not self-sufficient (the whole point of the PR), and the 3777 comment ("next batch re-emits … symbol-dict delta from id 0") describes behavior that no longer happens. Update to match the new baseline semantics to avoid misleading a future reader on the recovery/retry path (which is exactly where C1 lives).
m2 — Memory-mode mirror double-stores the dictionary.
The I/O-thread mirror (sentDictBytes*) holds every symbol's UTF-8 bytes while globalSymbolDictionary already holds them as Java Strings. Bounded by distinct-symbol count (not per-row), so acceptable, but worth a comment that memory-mode steady-state native footprint is ~2× the dictionary size for the reconnect-catch-up capability.
Downgraded (false positives — verified against source)
- Negative
fsnAtZeroon fresh recovery (replayStart=0⇒fsnAtZero = -catchUpFrames) corrupts ack accounting — dismissed.SegmentRing.acknowledgeclamps topublishedFsnand no-ops whenseq ≤ ackedFsn(339-349); the catch-up frame maps to an already-acked/nonexistent low FSN and its ack is a harmless no-op.DeltaDictRecoveryTestexercises exactly this (silent server, nothing acked) and passes. pd.size()read race in the send-loop constructor vs producerappendSymbol— dismissed. The loop is constructed during sender build/startCursorSendLoop(or on the drainer thread with no producer at all), which happens-before the first user send; no concurrent append occurs, sosentDictCount == loadedEntriescount.- Catch-up frame double-advances the durable-ack watermark — dismissed. The catch-up frame's OK enqueues a
tableCount=0(trivially durable) pending entry mapping to an ≤ackedFsnFSN;drainPendingDurableacks a no-op. Cumulative ack semantics make a missing catch-up OK harmless too. - Catch-up (non-
DEFER_COMMIT) frame prematurely commits deferred WAL on reconnect — dismissed. It is the first frame on a fresh server connection, which holds no pending WAL state; committing nothing is a no-op before the deferred replay frames arrive. positionCursorForStartre-sends a catch-up when retiring an orphan tail — dismissed. That branch is guarded bynextWireSeq == 0(trySendOne2166-2175), which cannot hold aftersendDictCatchUpincrementednextWireSeq; whensentDictCount==0there is nothing to re-send.- A symbol larger than the batch cap breaks catch-up — dismissed. The original data frame carrying that symbol (plus row data) would already exceed the cap and fail; the catch-up (symbol only, less overhead) is strictly smaller, so
sendDictCatchUp'sentryBytes > budgetterminal is consistent, not a new failure. - Java 8 floor violations in new code — dismissed. No
var, text blocks,instanceofpatterns,List.of, etc. in the changed main files; the one->is a pre-existing lambda. Compiles clean on JDK 25. PersistedSymbolDictuses slf4j instead of QuestDBLog— dismissed. Its sibling SF-cursor classes (AckWatermark,SegmentRing,CursorSendEngine, the send loop) all use slf4j; this is consistent.
Coverage map
| # | Behavioral change | Test (local unless noted) | Failure link | Dimensions | Verdict |
|---|---|---|---|---|---|
| 1 | Memory-mode monotonic delta (symbolDeltaBaseline in beginMessage) |
SelfSufficientFramesTest.testMemoryModeShipsMonotonicDelta |
asserts batch-2 deltaStart=1,deltaCount=1 — fails if baseline reverts to -1 |
happy ✓; NULL N-A; boundary (2 symbols) ✓; concurrency N-A | TESTED |
| 2 | File-mode delta + write-ahead persist | SelfSufficientFramesTest.testFileModeShipsMonotonicDeltaAndPersistsDict |
asserts monotonic delta + .symbol-dict retains both symbols |
happy ✓; resource (dict file) ✓ | TESTED |
| 3 | Reconnect catch-up (memory) | DeltaDictCatchUpTest.testReconnectCatchUpRebuildsDictionary |
reconstructs conn-2 dict from wire; fails on null gap | happy ✓; reconnect ✓ (loopback) | TESTED |
| 4 | Split catch-up under batch cap | DeltaDictCatchUpTest.testReconnectCatchUpSplitsLargeDictionaryAcrossFrames |
asserts ≥2 zero-table frames + gap-free reassembly | boundary (cap) ✓ | TESTED |
| 5 | File-mode recovery replay to fresh server | DeltaDictRecoveryTest.testRecoveredSlotReplaysDeltaFramesAgainstFreshServer |
asserts catch-up frame seen + gap-free dict | recovery ✓ (loopback); memory-leak N-A | TESTED |
| 6 | Torn-dictionary guard (simulated host crash) | DeltaDictRecoveryTest.testTornDictionaryFailsCleanlyInsteadOfCorrupting |
asserts 0 frames replayed + terminal "incomplete" error | error path ✓ | TESTED |
| 7 | PersistedSymbolDict open/append/reopen/torn-tail/bad-magic/removeOrphan |
PersistedSymbolDictTest (5 tests, assertMemoryLeak) |
round-trip + self-heal asserts | happy/boundary/empty-symbol/resource ✓ | TESTED |
| 8 | appendBlocking failure → persist-then-retry dict duplication (file mode) |
none (recorded search: no test references appendBlocking/backpressure/dup + persisted dict) |
— | error+retry ✗; recovery-after-retry ✗ | UNTESTED → Critical (C1) |
| 9 | Real-server 0-table catch-up acceptance + primary→replica failover | OSS tandem #7374 (single-node only); Enterprise tandem missing | — | real-server/failover ✗ | UNTESTED → Critical (C2) |
| 10 | seedGlobalDictionaryFromPersisted id/baseline resume on recovery |
indirect via DeltaDictRecoveryTest #5 |
dict reconstructed gap-free implies correct seed | happy ✓; retry-dup interaction ✗ (see C1) | TESTED (partial) |
Summary
Verdict: REQUEST CHANGES.
The design is careful and the write-ahead/torn-dictionary reasoning is largely sound, but two blocking issues stand:
- C1 (data integrity): a transient
appendBlockingbackpressure failure followed by any successful flush duplicates the failed frame's symbols in the persisted.symbol-dict; a later recovery/orphan-drain then silently misattributes symbol values via shifted global ids. This is a store-and-forward correctness violation on the very outage class SF exists to survive, it has no regression test, and the fix is small (base the persist range onpd.size()). - C2 (test gate): the HA failover behavior the feature targets has no linked, CI-running enterprise tandem; the OSS tandem #7374 covers single-node only.
Test & tandem gate: FAILS — one UNTESTED-Critical bug-fix-worthy path (C1, no regression test) and a required-but-missing Enterprise tandem (C2). Cannot approve.
Zero-GC gate: PASSES — no steady-state per-row/per-producer-call allocation on the ingestion path; producer-side additions (symbolDeltaBaseline, advanceSentMaxSymbolId, persistNewSymbolsBeforePublish) allocate nothing (M1 is syscall amplification, not GC). Catch-up/mirror allocations are I/O-thread, reconnect-only.
Coverage map: 10 behavioral-change groups — 8 tested locally (loopback), 2 UNTESTED (dict-dup-on-retry; HA-failover tandem).
Tandem status: OSS e2e tandem linked (#7374, single-node); Enterprise failover tandem required and missing; enterprise e2e-python kill-recovery coverage for the host-crash/torn-dict path recommended.
Findings: 6 verified (2 Critical, 2 Moderate, 2 Minor); 8 draft findings dropped as false positives after source verification.
In-diff vs out-of-diff: 4 in-diff (C1, M1, M2, m1), 1 tandem/process (C2), 1 cross-cutting (m2). The C1 mechanism spans the new persistNewSymbolsBeforePublish (in-diff) and the pre-existing sealAndSwapBuffer/appendBlocking failure path (out-of-diff) it now interacts with — the classic "diff quietly changed a contract at an unchanged callsite" case.
trySendOne decoded a frame's delta header twice: the pre-send torn-dictionary guard called frameDeltaStart (magic/flags check + start-id varint), then post-send accumulateSentDict re-ran isDeltaFrame and re-read the start id before reading deltaCount. Both run on every delta frame on the I/O send path. Decode the start id once in the guard, hoist the frame address into a local, and pass the start id into accumulateSentDict, which now locates deltaCount just past the canonical start-id encoding (via NativeBufferWriter.varintSize) instead of re-parsing the header. The non-delta-frame case is carried by the same start id (-1), so the post- send mirror update runs exactly when it did before. Also move the accumulateSentDict javadoc onto accumulateSentDict: it had drifted above frameDeltaStart (which kept its own doc), leaving accumulateSentDict undocumented. The per-entry region walk (to size the mirror copy) remains; eliminating it needs a wire-level deltaBytes field, a server-side change out of scope for this client fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Several comments predated file-mode delta encoding and claimed every
cursor frame is self-sufficient with a "symbol-dict delta from id 0". That
is now only the fallback: in delta mode (memory mode, and file mode when
the persisted dictionary opened) frames carry monotonic deltas that are
NOT self-sufficient, and the fresh server's dictionary is re-established by
an I/O-thread catch-up frame before replay.
The worst offender was the deltaDictEnabled field doc ("Enabled only in
memory-mode ... File-mode keeps full self-sufficient frames"), which
directly contradicted the feature. Corrected it plus the two ensureConnected
call-site comments, the append-failed-path comment, and the
wasRecoveredFromDisk field doc (schema stays self-sufficient per frame; the
dictionary does not). No behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two robustness fixes to the delta symbol-dictionary tests. Deterministic synchronization (replaces fixed sleeps): - DeltaDictCatchUpTest waited a fixed 200 ms for the server to close connection 1 before sending batch 2. On a loaded machine that could under-wait and let batch 2 race into connection 1's pre-close window, changing which connection the catch-up lands on. The handler now sets a conn1Closed flag after it closes the socket, and the test waits on that. - DeltaDictRecoveryTest's torn-dictionary test slept a fixed 1 s to let the replay guard fire before close(). It now polls flush() for the latched terminal (close() remains the fallback), so it captures the terminal as soon as it fires -- the run dropped from ~1 s to ~0.3 s. Leak checks: the Sender-based tests allocate native memory (the send-loop mirror, persisted-dict buffers, segment mmaps) but were not wrapped in assertMemoryLeak, unlike the rest of the suite. Wrap all eight methods across the three classes; every one is balanced (they already cleaned up via try-with-resources -- the wrapper now guards against future leaks). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
flushPendingRowsSplit fires when one flush's encoded size exceeds the server's batch cap: it emits one frame per table. The first frame must carry the whole batch's symbol-dict delta and advance the baseline, and the remaining frames must carry an empty delta that only references ids the first frame already registered -- otherwise a fresh server would see dangling symbol ids. No test drove that producer-side split. Add a test that buffers two padded tables into one flush under a small advertised cap, so the batch splits, and asserts the first frame ships deltaStart=0/deltaCount=2 while the second ships deltaStart=2/deltaCount=0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
accumulateSentDict dropped a frame entirely whenever deltaStart != sentDictCount. A delta that overlaps the mirror tip and extends past it (deltaStart < sentDictCount < deltaStart+deltaCount) was therefore discarded whole -- the new tail symbols never entered the mirror, which would leave a later reconnect catch-up incomplete and shift server-side ids. The producer only ever emits strictly contiguous, non-overlapping deltas, so this is currently unreachable, but it is load-bearing correctness resting on an invariant enforced elsewhere. Handle the overlap: skip the already-held prefix [deltaStart, sentDictCount) and copy only the new tail [sentDictCount, deltaStart+deltaCount). The steady-state case (deltaStart == sentDictCount) has skip == 0, so it is unchanged and free. A gap (deltaStart > sentDictCount, which the torn-dictionary guard rejects before send) now bails explicitly rather than implicitly. Also document that the I/O-thread mirror is a second, native copy of the dictionary (the producer's GlobalSymbolDictionary already holds the same symbols as Java Strings) -- so a memory-mode connection's steady-state dictionary footprint is ~2x the symbol set, an intentional cost of the reconnect-catch-up capability. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regression test for the write-ahead persist path: persistNewSymbolsBefore- Publish runs before the frame is published (sealAndSwapBuffer -> appendBlocking). If publish fails after the persist -- here PAYLOAD_TOO_LARGE (a frame bigger than the SF segment), a backpressure deadline in production -- the symbols are already on disk but sentMaxSymbolId is not advanced and the rows stay buffered, so a retry re-runs the persist. The fix keys the persist range off pd.size() (idempotent); this pins it. The test drives one new-symbol row whose padded frame exceeds a 1 KB segment, flushes it twice (both fail to publish), then asserts the persisted .symbol-dict holds the symbol exactly once. Reverting the fix to sentMaxSymbolId+1 fails it with size 2 -- the duplicate that shifts every later global id and silently misattributes symbol values on recovery. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…va-questdb-client into qwp-delta-symbol-dict
setWireBaselineWithCatchUp anchors fsnAtZero = replayStart - catchUpFrames so every catch-up frame maps to an already-acked FSN. Dropping the - catchUpFrames term is silent data loss: a server ACK for a catch-up frame then translates to an FSN at or above replayStart and trims a not-yet-delivered data frame from the store-and-forward log. The existing catch-up tests reconstruct the dictionary from wire bytes and never assert ACK/trim accounting, so they were blind to this line; the enterprise SqlFailoverQwpClientLosslessTest ingests no symbols and never enters the catch-up path at all. CursorWebSocketSendLoopCatchUpAlignmentTest drives the catch-up against a stub client and asserts the catch-up frame's OK leaves the real engine's ackedFsn untouched, for both a single catch-up frame and a split (multi-frame) catch-up. Reverting the - catchUpFrames term fails both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sendCatchUpChunk throws CatchUpSendException on a transient wire failure instead of calling fail(). From inside the catch-up fail() re-enters connectLoop -- desyncing the fsnAtZero/nextWireSeq wire mapping (a later ACK then trims un-acked store-and-forward frames), or overflowing the stack on a flapping connection -- turning a transient outage into a hard failure. Only the oversized-entry (non-retriable) terminal was covered; the retriable path had no test. testTransientCatchUpSendFailureIsRetriableNotTerminal drives the catch-up against a stub whose sendBinary throws, and asserts the failure surfaces as a retriable CatchUpSendException and leaves the producer-facing error latch clear. Reverting the throw to fail() fails it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Four minor cleanups on the delta symbol-dictionary catch-up, all behaviour-preserving on every reachable path: - The sentDict* field comment said the catch-up mirror is memory-mode only; it is also seeded and used in disk mode on a recovered / orphan-drained slot. Corrected. - positionCursorAt's javadoc said it runs after nextWireSeq was reset to 0, but the catch-up path leaves nextWireSeq past the frames it emitted. Corrected to describe setWireBaselineWithCatchUp anchoring the wire baseline; the method only moves the byte cursor. - The recovery-seed constructor set sentDictCount = pd.size() outside the loadedEntriesLen > 0 block. A recovered slot always has entries when size > 0, so the result is unchanged, but coupling the count to the mirror bytes stops sentDictCount ever claiming symbols the mirror does not hold. - sendDictCatchUp used Integer.MAX_VALUE as the no-cap per-frame budget, so sendCatchUpChunk's int frameLen could overflow on a multi-GB dictionary. Bound it by MAX_SENT_DICT_BYTES, the same ceiling ensureSentDictCapacity enforces. Unreachable at real cardinality (~200M+ symbols); defensive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Close three ways the delta symbol-dictionary feature could lose or corrupt data on the reconnect and store-and-forward recovery paths. Run the torn-dictionary guard unconditionally. trySendOne gated the guard on deltaDictEnabled, which CursorSendEngine reports false when a recovered disk slot cannot open its persisted dictionary (fd exhaustion, a read-only remount, ENOSPC). The recorded frames are still delta frames, so replaying them against a fresh empty-dictionary server null-padded the missing ids and silently corrupted the table. The guard now decodes the delta start for every frame and fails terminally on a gap regardless of the flag; only the sent-dictionary mirror stays gated. Stop treating a catch-up frame as the head data frame. sendCatchUpChunk advances nextWireSeq, but onClose's poison-strike gate and handleServerRejection's pre-send gate read nextWireSeq > 0 as "a data frame was sent". A transient non-orderly close or NACK after the catch-up but before the first replay frame then charged a poison strike on a frame that never left, and after a few flaps escalated a transient outage to a PROTOCOL_VIOLATION terminal that quarantines an orphan drainer. A new dataFrameSentThisConnection flag, set only after a real ring frame sends, now gates both decisions, so the drainer keeps retrying as Invariant B requires. Bound the commit message's dictionary delta to the sent watermark. sendCommitMessage skips the write-ahead persist yet encoded a delta up to currentBatchMaxSymbolId, so a symbol left in the batch by a cancelled row (cancelRow rolls back neither currentBatchMaxSymbolId nor the global registration) rode out on the commit frame without being persisted. A recovered slot then under-seeded the producer against the surviving frame and misattributed the reused id. The commit now caps the delta at sentMaxSymbolId in delta mode, giving an empty delta. Each fix carries a regression test proven to fail when the fix is reverted: a directory-shadowed .symbol-dict (guard), a close after only the catch-up (poison gate), and a cancelled-row symbol on a transactional commit (delta bound). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On recovery the send loop copied the persisted dictionary's loaded-entries buffer into a fresh mirror allocation and left PersistedSymbolDict holding a second copy for the engine's lifetime -- roughly twice the dictionary size in native memory on a high-cardinality recovered slot, retained long after the one-time seed. The loop now adopts that buffer as its mirror backing via takeLoadedEntries(), which transfers ownership so the dictionary no longer retains or frees it. The producer's readLoadedSymbols() is the only other consumer and runs first (setCursorEngine seeds the producer before the loop is built; the drainer has no producer consumer), guarded by an assert. Add a recover-then-continue-ingest test. A file-mode sender writes symbols and crashes; a fresh sender recovers the slot and ingests a NEW symbol. It asserts the producer continues the dictionary from the recovered size instead of colliding at id 0, exercising seedGlobalDictionaryFromPersisted, which no prior test drove past recovery. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Retain source rows when split preflight rejects an oversized table. Allow full dictionary frames to re-anchor ACKed recovery gaps while preserving unacked-gap safety. Bump the segment format and install a legacy-reader rollback barrier.
Restore the previous QwpWebSocketSender connect overload and the BackgroundDrainer constructor. Delegate both to the new catch-up dwell defaults. Use a saturating conversion for catch-up dwell milliseconds so large valid values cannot wrap negative and quarantine slots prematurely.
Release the cached recovery symbol suffix after the foreground send loop copies it, while retaining orphan-drainer state across recycled sessions. Share a Java Unsafe slice-by-8 CRC implementation between dictionary and segment recovery so large backlogs avoid scalar checksum scans without moving mmap faults into JNI.
Remove the unused symbol-mirror append helper. Reuse shared wire parsing and deletion-assured temporary directory fixtures in recovery tests.
endpointPolicyFailureIsTerminal() latched a terminal only for an ORPHAN drainer, so a FOREGROUND sender retried auth, non-421 upgrade and durable-ack capability failures forever -- including before it had ever reached the server. An operator whose credentials are wrong then watched a mute sender buffer into store-and-forward until it filled and misreported the cause as "out of space". Two pre-existing tests pin that startup contract and went red: QuestDBFacadeCallbacksTest#testFacadeErrorHandlerReceivesAsyncIngestError asserts the errorHandler receives the async auth terminal, and QuestDBFacadeDrainerListenerTest asserts close() rethrows the foreground's latched capability-gap terminal. Key the terminal off hasEverConnected as well. Connectivity problems are the caller's problem during startup: SYNC/OFF startup reports them by throwing from build(); ASYNC has no caller left to throw at, so the latched terminal reaches the user through SenderErrorHandler and the close() rethrow. The constructor already seeds hasEverConnected = client != null, so SYNC/OFF arrives past initialization and never latches, and swapClient makes the flag sticky: once the wire has been up, store-and-forward owns the buffered data and every endpoint-policy failure is a transient to ride out (Invariant B), unchanged. A transport failure (a dead port) never consulted this predicate and still retries forever. Rewrite the three tests that asserted the opposite so they pin the restored contract, and cross-link testAsyncNoServerRetriesForeverNoTerminal as its transport-half twin. Reverting the predicate fails both pre-existing tests plus the two rewritten async-initial ones, while the post-start tests stay green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
testQuarantineRenameFailurePreservesOriginalSlot counted the slot's .sfa files BEFORE build(), then asserted the count was unchanged after the failed rename. Recovery legitimately unlinks an empty never-rotated hot-spare on the way in (SegmentRing drops a recovered segment whose frameCount() is 0 with no torn tail), and whether SegmentManager had provisioned one by then is a race -- so the assertion measured that race rather than the rename. It failed 3 of 3 standalone runs, and inverted (2 vs 3) under load. Snapshot inside the quarantineAfterCloseHook instead: after the engine closed, before the rename. That is exactly what "a failed rename preserves the slot" means, and it is immune to the hot-spare race because both counts observe the post-recovery state. Assert the snapshot is non-zero as well, so the equality cannot pass vacuously on an empty slot. The product was never at fault: the reclaimed segment holds no frames, and close() unlinks nothing -- instrumenting quarantineTornSlot shows the same segments before and after it. Mutating that method to delete a segment during the failed rename still fails the test (expected 2, was 1), so it continues to catch real destruction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Five review findings on the delta symbol dictionary. The three performance ones all sit on paths that scale with symbol cardinality -- the workload this feature exists for introduces a new symbol per ROW -- and the two guards were pinning nothing. RecoveredFrameAnalysis.foldDelta issued one bounds check, one capacity check and a ~12-byte copyMemory per SYMBOL. runningCoverage is loop-invariant (it only advances after the walk) and id ascends, so "id >= runningCoverage" is a step predicate: the entries a frame contributes are always ONE contiguous run at the tail of its delta section. Note where the run starts and copy it once. Recovery walks the whole backlog, so this traded millions of stub-dispatched small copies for one bulk copy per frame. It also drops the partial prefix the per-entry version left behind on a markGap bail-out; that residue was already unreachable (a gap pins coverage() at -1, and a later self-sufficient frame resets the buffer), so not writing it is equivalent and leaves less state to reason about. accumulateSentDict already solved the same problem this way one file over. Crc32c.updateUnsafe read each 8-byte slice-by-8 block with eight getByte calls: 16 loads per block where 9 do. Consume it with two getInt loads. getInt is an Unsafe intrinsic exactly as getByte is, so the catchable InternalError this method exists for is unaffected; the loop becomes little-endian, which costs nothing because the formats it checksums are already little-endian throughout. This is the whole recovery CRC. The differential test against the native intrinsic (8 alignments x 28 lengths x 3 seeds, plus the chaining fuzz) proves it bit-identical. The sent-dict entry-ends index was maintained incrementally by accumulateSentDict and built eagerly by the constructor, yet sendDictCatchUp is its ONLY reader. That put a store per new symbol on the per-frame I/O path and retained 4 bytes per symbol for the connection's whole life, to serve a reconnect that may never come. Build it lazily instead: ensureSentDictEntryIndex() already notices sentDictIndexedCount falling behind and rebuilds, at most once per reconnect, dwarfed by shipping the same bytes over the wire. A connection that never reconnects no longer allocates the index at all. testCtorFreesSeededMirrorWhenFrameSeedThrows could not fail. The seam threw BEFORE ensureSentDictCapacity -- the call that copy-on-writes the borrowed prefix into a loop-OWNED allocation -- so the cleanup it guards, gated on sentDictBytesOwned, freed nothing. Deleting that cleanup left the test green. Move the seam past the grow: deleting the cleanup now fails with a 4096-byte NATIVE_DEFAULT leak. The test's premise comment described the old copy-based design and is corrected. testPersistFailureSurfacesAsLineSenderException drove code production never runs. FilesFacade.isMmapAllowed() defaults to (this == INSTANCE), so installing any fault facade silently swapped the dictionary onto the positioned-write fallback -- the act of injecting the fault replaced the code under test. Fault a refused ff.allocate on the mmap append window instead, with the facade opting into isMmapAllowed(): that is both the real shape of ENOSPC here and the path production takes. Reverting the wrap now escapes as "could not grow mmap append region", from the mapped path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The full-dict-fallback branch discards a persisted .symbol-dict whose
frames carry their whole dictionary inline (maxSymbolDeltaStart == 0 and
recoveredMaxSymbolId >= size). But the recovery analysis was folded with
the dictionary's size as its baseline, and every consumer of it presents
the baseline it derived the same way: once pd is null,
seedGlobalDictionaryFromPersisted computes baseline 0, and
checkedRecoveryAnalysis rejects a baseline that disagrees with the fold.
Discarding a dictionary that held entries (size > 0) therefore threw
IllegalStateException("recovery symbol baseline mismatch") out of build().
That is not an UnreplayableSlotException, so build()'s quarantine handler
could not set the slot aside. With a stable senderId every restart
re-recovered the same slot and threw again, so the application could never
construct a Sender -- it could not even buffer new rows. Exactly the
outage quarantineTornSlot exists to prevent, and on a slot that is fully
recoverable, since its frames need no dictionary.
It takes one transient plus one crash: a session whose .symbol-dict fails
to open (EIO, fd exhaustion, a Windows share lock) runs full-dict fallback
and, per the never-recreate contract, leaves the previous session's
populated side-file intact; if that session then crashes, this recovery
opens a dictionary with size > 0 beside self-sufficient frames that
out-reach it. The prior regression test covered only the size == 0 case.
Re-fold at baseline 0 after the discard so producer, mirror and replay
guard all anchor at 0 -- the full-dict mode the slot was written in. The
new test plants a populated survivor beside full-dict frames; reverting
the re-fold throws "recovery symbol baseline mismatch [expected=3,
actual=0]".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Four fixes across the recovery, orphan-scan and drainer paths, all on inputs a real cluster can produce. Bound the oversized-batch throw. flushPendingRowsSplit's all-or-nothing pre-flight throws when a single table's frame exceeds the server cap, and keeps the batch buffered so a rejected flush does not silently drop the caller's rows -- correct, and better than the old behaviour that discarded every table's rows while reporting failure. But a batch that can never fit the reachable cap then re-rejects on every flush(), growing unbounded if the caller keeps appending. The batch cannot drain until a larger-cap node returns or the sender is closed, so the message now says so, rather than reading as a transient a caller would retry into an ever-larger batch. Stop OrphanScanner counting the legacy-reader guards as data. CursorSendEngine plants .qwp-v2-guard-a/b.sfa in every disk-mode slot, before recovery or any segment, and its failure path does not unlink them. A construction that then throws (ENOSPC on the initial segment) leaves a slot holding only guards, which the scanner reported as an orphan with unacked data: a drainer adopted it, failed its own build under the same disk pressure, and dropped a permanent .failed sentinel plus an ERROR on a slot that never held a byte. Exclude the guards by name (SegmentRing's own predicate, now shared) -- a real segment beside them still counts. Saturate the drainer's capability-gap budget. reconnect_max_duration_millis is validated only as > 0, so a large value (Long.MAX_VALUE is the natural "never give up") wrapped the raw millis*1_000_000 multiply negative, and the OR-gated elapsed check then quarantined the slot on the FIRST capability-gap sweep, skipping the whole 16-sweep settle budget -- asking for more tolerance bought none. Convert with TimeUnit.toNanos, which clamps at Long.MAX_VALUE. The keepalive-interval and poison-escalation windows in CursorWebSocketSendLoop's constructor had the identical raw multiply -- the poison one is the exact twin of the cap-gap dwell the PR already hardened -- and a negative there escalates a transient to a producer-fatal terminal on strike count alone; convert both the same way. Make the legacy-reader barrier durable and stop re-truncating it. Its content never changes, yet it was rewritten via openCleanRW (truncate) on every engine open and never synced, so every open re-opened a window in which a host crash leaves a zero-filled guard beside already-durable v2 segments -- and a rolled-back v1 reader then skips the guard (bad magic) and the segments (bad version) alike, reads the slot empty, and truncates the unacked log. Verify an existing guard and rewrite only a missing or damaged one, and msync the guard on creation (33 bytes, once per slot lifetime) so it is durable before the data it protects. The guard frame is not byte-reproducible, so the test discriminates kept from rewritten by byte-identity across a reopen and asserts only semantic validity after a repair. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
connectWithRetry set its deadline as startNanos + maxDurationMillis * 1_000_000L, a raw millis-to-nanos multiply. A large reconnect_max_duration_millis -- Long.MAX_VALUE is the natural "retry until the server boots" value, and the builder setter enforces no upper bound -- wraps that product negative, so the pre-condition retry loop runs zero iterations. The SYNC initial connect then throws "no attempts made" without ever calling the reconnect factory: the exact opposite of the requested patience. The prior commit hardened this same overflow class in the drainer's capabilityGapBudgetNanos and the loop dwells via TimeUnit.MILLISECONDS.toNanos, but missed connectWithRetry, the primary consumer of the value. connectWithRetry now compares elapsed time against a saturating budget instead of an absolute deadline: toNanos alone still overflows the startNanos + budget sum once it saturates to Long.MAX_VALUE, whereas the bounded nanoTime difference stays correct. A new regression test drives connectWithRetry with a Long.MAX_VALUE budget and asserts at least one attempt runs; it fails on the old code with "failed after 0ms / 0 attempts". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PersistedSymbolDict.appendSymbols encoded the mapped chunk in two passes over the id range: a first loop called Utf8s.utf8Bytes on every symbol just to size the header varint and the mmap reserve, then a second loop re-walked each symbol's UTF-8 length to write it. It now stages the entry region in the scratch buffer with a single walk per symbol and bulk-copies it into the append window after the header, dropping the redundant walk. The chunk it writes is byte-identical, so the exact-file-size "one chunk, one CRC" test still holds. A direct in-window encode cannot avoid the double walk: the back-to-back chunk format leaves no room to reserve and back-fill the header in place. The path is cold -- only a retry after a failed publish re-encodes from the dictionary; the steady state persists a frame's pre-encoded bytes through appendRawEntries. Add testMappedAppendSpansMultipleWindowsAndRecoversDenseIds: it appends a dictionary larger than one 4 MiB append window, asserts the window remaps (appendMapGrowthCount >= 2) without a positioned write, then reopens and checks every id recovers to its own symbol across the boundary. This exercises the ensureAppendMap remap arithmetic and the single-pass encode as chunks straddle the boundary, both of which the single-window happy path left untested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Production recovery runs one ordered fold (SegmentRing.analyzeRecovery -> MmapSegment.scanRecovery -> RecoveredFrameAnalysis) and seeds the producer through CursorSendEngine.addRecoveredSymbolsTo. The older per-metric walk -- collectReplaySymbolsAbove, maxSymbolDeltaEnd and maxSymbolDeltaStart on MmapSegment and SegmentRing, plus the CursorSendEngine.collectReplaySymbolsAbove wrapper -- had no production caller and diverged from the fold: it lacked the full-dict gap reset, the unacked-gap distinction and commit-boundary checkpointing, so the tests routing through it verified an algorithm production never runs. Delete all seven methods and the now-orphaned RecoveredFrameAnalysis.appendDecodedSymbols helper, then rewire the two tests that reached the walk (RecoveredFrameAnalysisTest, CursorWebSocketSendLoopOrphanTailTest) to the production addRecoveredSymbolsTo path. That is behavior-preserving -- same coverage, same decoded symbols -- and stronger, since the tests now drive the real seeding method. Drop the imports the deletion left unused. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two test-quality fixes in the send-loop cursor suite; no production behavior change. De-reflection. CatchUpAlignmentTest, PoisonFrameTest, TornDictGuardTest and MirrorLeakTest reached into CursorWebSocketSendLoop by reflection -- reading private constants and fields, invoking private methods, and writing private state -- so a rename broke them as a runtime ReflectiveOperationException instead of a compile error. Add @testonly accessors and thin ...ForTest wrappers on CursorWebSocketSendLoop, each delegating verbatim to the same private member, and make forceMirrorSeedFailureForTest public; then rewire the four tests to call them. The rewrites drive the identical production paths, and the mirror-overflow guard now catches the exact public LineSenderException type rather than unwrapping an InvocationTargetException. De-flake. PoisonFrameTest asserted absolute wall-clock bounds -- a recycle is "immediate" under 300 ms, and a recycle count fits a fixed sleep window -- that a CI GC pause or starved scheduler can trip. Replace them with predicates only a real regression fails: consecutive recycle gaps must clear 3/4 of the reconnect backoff (an unpaced loop's sub-millisecond gaps fail, while a slow test thread only yields more paced samples); the deterministic zeroProgressRecycles branch value (level 0, then 1, then reset on progress); a first*2 <= second relative bound; and the kept second >= backoff lower bound, which starvation can only lengthen. Split the dwell test into a huge-window case (threshold reached, window still open, so no escalation -- asserting poisonStrikes == 3) and a small-window case crossed by a sleep strictly longer than the window, which must escalate to the PROTOCOL_VIOLATION terminal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A cluster of small review follow-ups; only the first changes behavior. - The orphan-tail-retire re-anchor caught a CatchUpSendException and passed fail(e.getCause()), unwrapping the cap-gap marker so connectLoop's isCatchUpCapGap reset the orphan settle episode instead of preserving it. Route unwrap-aware: fail(isCatchUpCapGap(e) ? e : e.getCause()) keeps the attempt count and dwell anchor across the re-anchor recycle for a genuine cap gap, and unwraps to the raw cause otherwise, exactly as a normal send failure does. This only ever errs safe today -- more retries before an orphan drainer quarantines -- but the wrapper exists precisely to carry that signal. - Drop @testonly from BackgroundDrainer.connectWithDurableAckRetry: run() calls it in production, so the annotation was inaccurate. - Rename CatchUpSendException.capGap to isCapGap (boolean naming). - Restore the CursorWebSocketSendLoop final-field group and a QwpWebSocketSender import to alphabetical order. - DeltaDictCatchUpTest: fix a comment naming a test that does not exist. - DeltaDictRecoveryTest: delete the never-instantiated CountingHandler and the orphaned javadoc stacked above FullDiskDictFacade. - CursorSendEngineTest: rename the legacy-guard test to describe what it asserts (the guards force an FSN gap; current recovery adopts the v2 slot) -- it never drives a legacy reader to throw. - PoisonFrameTest: the two catch-up-only "does not strike" tests now assert poisonStrikes() == 0 explicitly instead of relying only on checkError() not throwing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
testLogicalLockReportsLockDirectoryCreationFailure built the lock directory path with Paths.get(parentDir, ".slot-locks"), which yields a backslash separator on Windows. SlotLock.acquireLogical builds it as parentDir + "/" + ".slot-locks" (forward slash), so the injected LockDirectoryFailureFacade -- which fails mkdir only when lockDir.equals(path) -- never matched the production path on Windows. The mkdir then succeeded, acquireLogical did not throw, and the test hit its fail() guard. It passed on Linux and macOS only because Paths.get uses a forward slash there. Build the test's lock-dir (and slot) path with the same "/" concatenation SlotLock uses, so the facade matches on every platform. Test-only change; no production behavior is affected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
[PR Coverage check]😍 pass : 1455 / 1571 (92.62%) file detail
|
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
3 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.
Tandem
This change lands together with its submodule-bump counterparts (merge as a set):
java-questdb-clientsubmodule. The OSS server already parses delta symbol-dictionary frames, so no server change is required.SqlFailoverQwpClientLosslessTest, file-mode failover) runs end-to-end against this change.Summary
Every QWP ingress message used to carry the entire symbol dictionary, so a connection that ingests many distinct symbols re-transmitted the whole dictionary on every message. This change makes the client register each symbol id with the server only once per connection and send only new ids (a delta) thereafter, re-registering the full dictionary when a connection is replaced.
The bandwidth saving grows with symbol cardinality and message count; for low-cardinality or short-lived connections it is negligible, and the change adds the costs described under Tradeoffs.
What changed
Memory mode
Store-and-forward (file mode)
PersistedSymbolDict) so a recovered or orphan-drained slot on a fresh process -- which has no in-memory dictionary -- can rebuild what its (non-self-sufficient) delta frames reference.Catch-up split
Durability
The persisted dictionary intentionally does not fsync, matching the rest of store-and-forward: it is process-crash durable (the OS page cache survives a JVM crash) but not host-crash durable. Rather than fsync only the dictionary -- which would not make the frame data itself host-crash durable -- a host crash that tears the dictionary is caught rather than silently trusted. Each side-file chunk carries a CRC-32C over its header and batched entry bytes (the same checksum the SF segment frames use), so recovery stops at the first torn or mismatched chunk and trusts only the intact prefix; the send loop then detects any surviving delta frame whose start id exceeds that prefix and fails cleanly with a "resend required" error instead of transmitting a gapped frame that would corrupt the table.
Tradeoffs
Test plan
DeltaDictCatchUpTest-- reconnect catch-up rebuilds the dictionary (memory mode); a large dictionary splits across multiple catch-up frames under a small advertised batch cap and reassembles gap-free.DeltaDictRecoveryTest-- a recovered file-mode slot replays its delta frames against a fresh server; a torn (host-crash) dictionary fails cleanly instead of corrupting.PersistedSymbolDictTest-- side-file append/read/orphan-removal round trips.SelfSufficientFramesTest,ReconnectTest-- full-dict fallback and reconnect replay still hold.SqlFailoverQwpClientLosslessTest(file-mode failover) passes end-to-end against a real server.🤖 Generated with Claude Code