Skip to content

feat(qwp): stop resending the full symbol dictionary on every message - #66

Open
glasstiger wants to merge 157 commits into
mainfrom
qwp-delta-symbol-dict
Open

feat(qwp): stop resending the full symbol dictionary on every message#66
glasstiger wants to merge 157 commits into
mainfrom
qwp-delta-symbol-dict

Conversation

@glasstiger

@glasstiger glasstiger commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Tandem

This change lands together with its counterparts (merge as a set):

  • OSS: feat(qwp): stop resending the full symbol dictionary on every message questdb#7374 -- bumps the java-questdb-client submodule, and adds one server-side change: the ingress decoder now rejects a delta symbol dictionary whose start id runs past the connection dictionary, atomically and with a dedicated retriable error, instead of null-padding the hole. See "Server-side gap rejection" below.
  • Enterprise: questdb/questdb-enterprise#1122 -- bumps the client so the failover suite (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

  • The producer keeps a monotonic "sent" watermark; each frame's dictionary section carries only the ids above it instead of the full dictionary from id 0.
  • On reconnect or failover the fresh server starts with an empty dictionary, so the I/O thread replays the whole dictionary as a catch-up frame before any post-reconnect traffic. The producer's monotonic baseline is deliberately preserved across the wire boundary rather than reset.

Store-and-forward (file mode)

  • Each slot persists its dictionary to a dot-prefixed side-file (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.
  • Write-ahead ordering: new symbols are appended to the side-file before the frame that references them is published to the ring.

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. When the server advertises no cap, or the whole dictionary fits, the behaviour is unchanged (a single frame).

Server-side gap rejection (OSS half)

Delta framing makes a non-zero start id reachable on the wire for the first time, so the decoder's handling of one now matters. QwpMessageCursor.parseDeltaSymbolDict grew the connection dictionary with nulls up to deltaStartId + deltaCount, which inflated size() -- the very bound QwpSymbolColumnCursor checks an incoming symbol index against. A row referencing a padded id therefore passed the bounds check, read back null, and landed a NULL symbol value with no error.

The decoder now rejects deltaStartId > size() with its own error code, DELTA_DICT_GAP, surfaced to the sender as a new wire status byte, STATUS_DICTIONARY_GAP (0x0D). The gap verdict depends on this connection's dictionary coverage -- server state, not the frame's bytes -- so unlike a parse error it is retriable: the server sends the NACK and keeps the connection open, and the sender recycles the wire and re-registers from an id the server actually holds. A contiguous append (deltaStartId == size()) and a lower start that re-registers or remaps existing ids both stay allowed. The parse is atomic on failure: a rejected delta restores every entry it overwrote and nulls the slots it grew into, so the connection dictionary is exactly what it was before the frame and can never hold a null.

Wire-compat note: the server now rejects a frame shape it previously (wrongly) accepted, and 0x0D is a status byte no earlier server emitted, under an unchanged protocol version. QWP is experimental and unreleased, and the bundled client moves in lockstep with the server, which is what the tandem labels assert; this client maps an unknown status byte to a retriable category, so an older bundled client against a newer server degrades to retry rather than failing. This client cannot emit a gapped frame -- its send loop refuses to -- so the guard exists for a client bug, a torn store-and-forward dictionary, or a third-party implementation.

Symbol dictionary capacity

The server caps a connection's symbol dictionary at 1,000,000 distinct values (MAX_SYMBOL_DICTIONARY_SIZE, pre-existing). Before this change the practical ceiling was far lower: every message re-shipped the dictionary prefix from id 0, so per-message cost grew with lifetime cardinality and a large dictionary outgrew the frame budget long before the cap. Delta encoding removes that per-message cost, which makes the protocol cap the binding constraint for the first time — and because the producer's baseline is lifetime-monotonic, the reconnect catch-up would trip the server's rejection on every reconnect, including recovered slots and orphan drainers, stranding an already-buffered store-and-forward backlog with no error ever reaching the producer.

The client therefore enforces the cap at registration: creating the 1,000,001st distinct symbol value throws a LineSenderException from symbol() naming the limit and the recovery, before the row is buffered. Rows using already-registered values are unaffected. Everything buffered stays deliverable — the server's check is >, so a dictionary of exactly the cap still catches up cleanly. To reset the id space, close the sender and build a new one: a fully drained close removes the slot's dictionary side-file, so the rebuilt sender starts fresh. Reaching a million distinct values in symbol columns usually means the data belongs in varchar.

The server-side rejection itself keeps its parse-error (terminal) classification: with the registration guard, this client cannot reach it, the same unreachability argument the gap status relies on for old clients.

Slot quarantine: deterministic recovery failures set the slot aside

A recovery failure that is deterministic -- a torn slot whose surviving frames cannot be replayed without corrupting data, an unreadable interior segment, a corrupt segment chain -- no longer aborts Sender.build() forever or spins the orphan drainer. Sender.build() and BackgroundDrainer catch the typed exceptions (UnreplayableSlotException, SfRecoveryException, MmapSegmentCorruptionException), rename the whole slot directory aside for operator attention, dispatch a synchronous SenderError, and continue on a fresh slot. Renaming the whole directory guarantees the replacement starts empty and cannot fail the same way twice. Operational failures -- e.g. a drained-slot leftover whose unlink fails -- deliberately stay plain aborts that retry, rather than quarantining data that is still deliverable.

Mmap faults on the dictionary path degrade instead of killing the sender

MmapSegment.isMmapAccessFault recognizes the InternalError HotSpot raises for an access to an unbacked page (delivered asynchronously before JDK 21, JDK-8283699). The dictionary-side consumers (persistNewSymbolsBeforePublish, healPersistedDictionary) treat a recognized fault as a persist failure and degrade the sender to full self-sufficient frames (disableDeltaDict) instead of propagating an untyped Error; an unrecognized InternalError still propagates. Segment recovery itself validates every page through positioned reads before mapping, so the recovery scan cannot hit a late-delivered fault on pages it has not already read.

Reconnect policy: post-connect endpoint rejections retry instead of killing the producer

Once a foreground sender has completed its first connection (including the dictionary catch-up), a later WebSocket upgrade rejection or durable-ack capability mismatch no longer latches a producer-fatal terminal: the send loop retries with backoff while store-and-forward keeps buffering, and the failure is reported through SenderError dispatch. At build/initialization time these failures still surface loudly. Auth failures on the orphan drainer, and initialization-time failures in all modes, keep their previous terminal behaviour. hasEverConnected latches only after the catch-up succeeds, so a first connection that fails inside the catch-up still counts as never-connected and keeps endpoint-policy failures terminal.

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

  • Each reconnect/failover now replays the full dictionary as a catch-up frame, so a reconnect on a very high-cardinality connection ships the whole dictionary once (previously every message did).
  • File mode writes a per-slot dictionary side-file (extra disk I/O and one small file per slot).
  • Without fsync, a host/power crash can still lose recently persisted symbols, and the affected data must be re-sent. Every detectable tear now fails clean rather than corrupting: the per-chunk CRC-32C catches an interior page lost out of order (or a stale chunk left by a failed best-effort truncate) that would otherwise shift the dense id->symbol mapping, so recovery trusts only the intact prefix and the send loop forces a "resend required" for the rest. A tail truncate that itself fails makes the file untrusted; recovery leaves it intact and falls back to full-dictionary frames rather than exposing stale bytes. The one residual is a tear that happens to leave a CRC-matching byte run -- a 1-in-2^32-per-chunk collision, no weaker than the SF frames' own checksum.
  • On failover to a node advertising a smaller batch cap, a symbol accepted under a larger or absent cap can exceed the new cap during the catch-up. A foreground sender retries that indefinitely and recovers on its own once a larger-cap node returns, so store-and-forward contains the window instead of surfacing it to the producer. Only an orphan drainer gives up, and only after both 16 consecutive cap gaps and a minimum wall-clock dwell (catch_up_cap_gap_min_escalation_window_millis, 5 minutes by default); it then sets its slot aside for an operator and that slot's data must be re-sent. This cannot happen on a homogeneous cluster -- a symbol that fit inside a data frame under a given cap always fits the smaller catch-up frame under the same cap -- so it only affects heterogeneous/rolling-cap clusters or an operator lowering the cap below existing data.
  • The server-side gap rejection turns a previously silent (and silently wrong) frame into a NACK. The rejection is retriable by design -- a gap is a statement about per-connection server state, and re-registering from a held id resolves it -- but a sender that persistently re-sends the same gapped frame escalates through the poison-frame detector to a terminal error rather than looping forever.
  • Quarantine trades availability of one slot's data for the rest of the pipeline: a slot set aside must be re-sent (or inspected and restored by an operator), and the sender continues on a fresh slot instead of blocking.

Follow-ups (known, deliberately not in this PR)

  • Sender.build()'s rollback closes the cursor engine without the failed-stop check the close-delegation protocol requires, and PersistedSymbolDict makes the send loop's mirror a borrower of the engine's native memory — so a throw landing in the narrow window after the I/O thread starts, combined with a thread that outlives the 30 s stop (in practice an OOM), could free memory a live I/O thread still reads. The reachable window is effectively theoretical, and the fix (move the rollback close into QwpWebSocketSender.connect's catch, which owns the engine and honours the protocol) touches teardown paths not worth destabilizing here. It must land together with narrowing ensureConnected's blanket exception wrap, which currently masks the worse variant of the same defect: fixing either alone makes the other worse.
  • In full-dictionary fallback mode accumulateSentDict still runs per frame: it re-walks the already-held dictionary prefix varint-by-varint on the I/O thread and, when the loop was constructed with the delta dict already disabled, accumulates a native mirror nothing ever reads. Both are constant-factor costs on a mode that already re-sends the whole dictionary per frame, so the fix (carrying the encoder's entries-length as sideband on ring entries, plus offset arithmetic for the identical prefix) waits for profiling evidence rather than adding plumbing here.
  • The Rust client (c-questdb-client) already enforces a producer-side dictionary cap (SymbolGlobalDict::intern errors at the cap), but its constant MAX_CONN_SYMBOL_DICT_SIZE = 8_388_608 was taken from the egress/result-batch direction, not the ingress server's 1,000,000 — so its guard cannot fire before the server rejection. One-line constant fix (plus comment correction) needed in that repo.

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 is caught by the per-chunk CRC and only the intact prefix is trusted.
  • PersistedSymbolDictTest -- side-file append/read/orphan-removal round trips, and a multi-byte UTF-8 round trip across reopen (every other symbol in these suites is ASCII, where a symbol's UTF-8 byte length and its char count agree, so a confusion between the two would otherwise go unnoticed).
  • GlobalSymbolDictionaryTest, DeltaDictCeilingTest -- the 1,000,000-entry protocol cap: the boundary entry is accepted, the next is refused without mutation, cancelRow() recovers the row, the sender keeps working with registered values, and the refused symbol never reaches the wire.
  • CursorWebSocketSendLoopCatchUpAlignmentTest -- the split catch-up's chunks must tile [0, n) exactly: the captured frames are reassembled through the same decoder the end-to-end tests use and compared per id, so an overlap, a gap or a shift all fail. Also covers a reconnect with an empty dictionary (no catch-up frame at all) and a split over entries of differing widths.
  • SelfSufficientFramesTest, ReconnectTest -- full-dict fallback and reconnect replay still hold.
  • MmapFaultDegradesTest -- a recognized mmap access fault on the dictionary persist path degrades the sender to full-dict frames; an unrecognized InternalError still propagates.
  • MmapSegmentRecoveryFaultTest -- single-segment recovery fault shapes: read errors, short reads, size changes and unbacked pages fail closed before mapping or skip the unbacked tail.
  • SegmentSkipQuarantineTest, SegmentRecoveryIntegrityTest, BackgroundDrainerUnreplayableSlotQuarantineTest -- deterministic recovery failures quarantine the whole slot and the replacement starts empty; a drained-slot leftover whose unlink fails aborts and retries instead of quarantining.
  • CursorWebSocketSendLoopForegroundReconnectPolicyTest -- post-connect endpoint rejections retry on a foreground sender; initialization-time failures stay terminal; a first connect that fails inside the catch-up does not latch hasEverConnected.
  • SlotLockTest -- lock lifecycle, including the pid-sidecar-before-lock unlink order on retirement.
  • OSS QwpSymbolDecoderTest -- a gapped delta is rejected with DELTA_DICT_GAP (routed to the DICTIONARY_GAP status), the deltaStartId == size() boundary is still accepted, a rejected delta restores every overwritten entry and leaves no nulls -- including when the rejected frame was the connection's first.
  • The enterprise SqlFailoverQwpClientLosslessTest (file-mode failover) passes end-to-end against a real server, asserting per row that every surviving SYMBOL is the value its id implies.

🤖 Generated with Claude Code

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 glasstiger added the enhancement New feature or request label Jul 9, 2026
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>
@glasstiger

Copy link
Copy Markdown
Contributor Author

Tandem OSS PR (submodule bump): questdb/questdb#7374 — merge together.

glasstiger and others added 10 commits July 9, 2026 17:10
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 bluestreak01 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glasstiger

Review of PR #66feat(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, ReconnectTest15 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 preserved

sealAndSwapBufferappendBlocking 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) calls getOrAddSymbol, which de-dupes → producer globalSymbolDictionary.size() and sentMaxSymbolId are 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.
  • sendDictCatchUp re-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 277 buffer.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 (deltaStart never exceeds the now-larger sentDictCount, so trySendOne at 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-dictempty. gh can reach the private enterprise repo (confirmed), and a scan of the 60 most-recent enterprise PRs shows no client-bump/qwp-symbol-dict PR. SqlFailoverQwpClientLosslessTest exists 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 (swapClientsetWireBaselineWithCatchUpsendDictCatchUp), 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 fsnAtZero on fresh recovery (replayStart=0fsnAtZero = -catchUpFrames) corrupts ack accountingdismissed. SegmentRing.acknowledge clamps to publishedFsn and no-ops when seq ≤ ackedFsn (339-349); the catch-up frame maps to an already-acked/nonexistent low FSN and its ack is a harmless no-op. DeltaDictRecoveryTest exercises exactly this (silent server, nothing acked) and passes.
  • pd.size() read race in the send-loop constructor vs producer appendSymboldismissed. 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, so sentDictCount == loadedEntries count.
  • Catch-up frame double-advances the durable-ack watermarkdismissed. The catch-up frame's OK enqueues a tableCount=0 (trivially durable) pending entry mapping to an ≤ackedFsn FSN; drainPendingDurable acks 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 reconnectdismissed. 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.
  • positionCursorForStart re-sends a catch-up when retiring an orphan taildismissed. That branch is guarded by nextWireSeq == 0 (trySendOne 2166-2175), which cannot hold after sendDictCatchUp incremented nextWireSeq; when sentDictCount==0 there is nothing to re-send.
  • A symbol larger than the batch cap breaks catch-updismissed. 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's entryBytes > budget terminal is consistent, not a new failure.
  • Java 8 floor violations in new codedismissed. No var, text blocks, instanceof patterns, List.of, etc. in the changed main files; the one -> is a pre-existing lambda. Compiles clean on JDK 25.
  • PersistedSymbolDict uses slf4j instead of QuestDB Logdismissed. 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 appendBlocking backpressure 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 on pd.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.

glasstiger and others added 8 commits July 9, 2026 22:09
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>
glasstiger and others added 5 commits July 10, 2026 00:43
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>
Sergei Minaev and others added 29 commits July 28, 2026 21:22
The split pre-flight moved from discard-then-throw to retain-then-throw,
which is the better semantic: the batch survives until a larger-cap node
appears. close() was not updated to match.

flushPendingRows is the first statement in close()'s try, so its throw
skipped sendCommitMessage, the final sealAndSwapBuffer and drainOnClose.
Rows deferred by earlier successful flushes were never committed and
close() returned without waiting for acks of already-published frames.
One oversized final batch abandoned the whole session, while the message
that path emits tells the caller to close the sender to discard it.

Give the pre-flight rejection its own type so close() can recognise it
without matching on message text -- flushPendingRows also throws plain
LineSenderException for unrelated reasons, and discarding the batch on
those would lose data silently. Catch it, reset the invalid batch, stash
the error and let the rest of close() run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
openFresh returned null on an openCleanRW failure without truncating
anything, so a fresh-start session could run full-dictionary from id 0
while the previous generation's .symbol-dict stayed on disk. The next
recovery then seeds from a side-file describing a different id space from
the surviving frames, and nothing detects it: no gap is reported, the CRC
is valid, and both the client and server bounds checks pass. The catch-up
registers the old generation's strings under ids the new generation's
rows reference, so a resumed producer writes silently wrong SYMBOL values.

Distinguish the two callers. open() may still proceed without a dictionary
when the file is absent or a sub-header stub -- there is nothing to lose.
openClean, the fresh-slot path, now refuses when an existing file cannot
be truncated.

This closes the one reachable path. The wider class -- nothing compares
persisted strings against the frames' own at overlapping ids -- remains
open and is tracked separately.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
Add a CursorSendEngineTest case that plants a real directory at the
fresh slot's .symbol-dict path and constructs the engine with the
plain FilesFacade.INSTANCE, asserting the refusal (LineSenderException
containing "cannot be truncated") reaches the caller through the
constructor's own catch-and-rethrow. The prior round only exercised
the refusal via PersistedSymbolDict directly and via stubbed facades,
so nothing proved the propagation claim in CursorSendEngine's comment
against a genuine EISDIR from openCleanRW.

Also reorder phase1Engine construction in DeltaDictRecoveryTest's two
full-dict-fallback tests to happen after the test server is confirmed
up, matching the file's own established pattern, so a server-start
failure cannot leak the engine's slot lock and native memory past the
try block.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
catchup_cap_gap_min_escalation_window_millis spelled catchup as one word
while every neighbouring key separates its words, and the builder method
is already catchUpCapGapMinEscalationWindowMillis. Connect-string keys are
permanent API, and this one has not shipped, so rename it now rather than
carry the inconsistency forever. No alias is needed.

Also make wsConfigSnapshotForTest put the raw field instead of the
resolved default, matching every sibling key -- resolving the unset
sentinel made the snapshot unable to tell "unset" from "explicitly set to
the default".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
The per-file recovery catch was widened from MmapSegmentException to
Throwable plus isMmapAccessFault, but the FSN contiguity check only fails
closed for an interior segment.

A skipped OLDEST segment moves firstSealed() up, and the ack seed is
lowestBase - 1 on the premise that anything below the ring's bottom was
trimmed and trim is ack-driven. A segment can now leave the bottom
because of an mmap fault, so every frame it held was declared
server-acked and never replayed -- silently, and identically on every
restart since the file is not unlinked.

A skipped NEWEST segment lets nextSeq reuse the FSNs it owns, so the next
recovery sorts two segments onto overlapping baseSeqs and throws
MmapSegmentException, which is not UnreplayableSlotException: build() is
not quarantined, it just fails the same way forever.

Refusing only when the skipped range cannot be shown acked is not
implementable -- baseSeq lives at header offset 8, inside the read that
failed, and the filename does not carry it -- so any skip now refuses the
slot. Rename the file to .corrupt so a reused FSN range cannot collide
with it later, and raise UnreplayableSlotException so the slot is set
aside with its bytes intact instead of losing frames silently. The same
refusal also covers the case where every segment in the directory was
skipped, which previously fell through to a silent fresh start at
baseSeq=0.

This is not a revert of the mmap-fault change: before it, one bad page
aborted recovery with an untyped exception and build() failed forever.
Now the producer starts on a fresh slot and the data is recoverable by
hand. What is given up is replaying the sibling segments, which cannot be
done safely without knowing what was lost.

Updates every existing test whose premise was "one file is skipped, the
healthy sibling still recovers" (a wider blast radius than a single test),
plus two new dedicated skipped-oldest / skipped-newest regression tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
Task 13 (25d253f) fixed a second, distinct defect beyond its own
brief: when every .sfa in a slot's directory is unreadable, the
pre-existing opened.size() == 0 branch returned null before ever
reaching the post-contiguity refusal, so the caller read null as "no
prior slot" and started a fresh ring at baseSeq=0 -- silently
discarding every frame every skipped file held, with no
operator-visible signal beyond a log line. That commit already closed
the gap (a refuseIfSegmentsSkipped call in the opened.size() == 0
branch), but shipped without a dedicated test proving it, since the
three tests it added each leave at least one segment readable.

Verified this test is independently load-bearing, not incidentally
covered by the other refusal: temporarily disabling only the
opened.size() == 0 call site (keeping the post-contiguity one active)
fails exactly this one test and none of the other three refusal
tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
CRITICAL, distinct from the prior two commits: build() constructed
CursorSendEngine outside its own catch (UnreplayableSlotException),
which wrapped only connect(). A skip detected during recovery (fixed
two commits ago) could refuse from the constructor, and that refusal
escaped build() entirely, uncaught. Worse, because the skipped
segment is renamed to .corrupt, a second recovery attempt against the
same directory no longer matches ".sfa" at all, and would silently
recover a ring missing the oldest segment -- the original silent-loss
bug, deferred by exactly one restart.

Fixed by reusing quarantineTornSlot, already proven for the symbol-
dictionary refusal: made its live-engine parameter nullable (nothing
to close when the constructor never returned one) and wrapped the
CursorSendEngine construction in the same catch. This renames the
whole slot directory aside before building the replacement at the
original path, which is then genuinely empty -- nothing left to
skip, so it cannot refuse a second time. Rejected the alternative of
making the refusal itself survive the rename: that alone trades
silent loss for a permanent brick, since every later restart against
the same tainted directory would refuse again forever.

Also closes a second, separate hole in the same area: the
empty-with-torn-tail branch (frame[0] itself corrupt) already
quarantined a segment to .corrupt but never counted it toward the
skip tally, so a torn OLDEST segment could still seed the ack cursor
past frames nothing shows were delivered. Counts it now when the
tail is non-empty; a genuinely empty, never-appended hot-spare
(tail empty) is unaffected, since nothing was lost there.

Running the whole test module (not just the files touched so far)
turned up a real regression outside the targeted list:
PrReviewRedTests' single-segment torn-frame test now hits the
"every segment skipped" case and must expect the typed refusal
instead of a silent return. Updated it accordingly.

Investigated and declined a filter tightening the review also
suggested (requiring "sf-" as well as ".sfa" in the recovery scan,
matching production's own naming convention): verified it is safe
against real deployments, but it would make dozens of pre-existing
tests across this suite recover an empty ring instead of the
segments they create, since many use non-conforming test file names.
Left the filter as is.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
IMPORTANT, found by probing af513a9 rather than reading it: a skipped
INTERIOR segment still bricks the slot permanently. The typed refusal
ran after the FSN contiguity check, but skipping a middle segment also
opens a gap between its surviving neighbours, so the untyped
MmapSegmentException from that check fires first and escapes both the
constructor-time and connect()-time UnreplayableSlotException catches
in Sender.build(). Because the interior file is already renamed to
.corrupt, every retry recomputes the skip count as zero and hits the
identical gap again -- not an escaped exception once, but forever.

Moved the skip-count refusal to run immediately after sorting, before
the contiguity loop: the count is already known at that point, so any
skip refuses before a gap it may have opened is ever examined. Added
an interior-segment test (three segments, the middle one corrupted)
alongside the existing oldest/newest/all/torn-oldest/stray cases.

Also: added assertMemoryLeak to SegmentSkipQuarantineTest, the only
test covering the constructor's mid-construction self-cleanup path
before quarantineTornSlot builds the replacement engine.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
The only check that a recovered .symbol-dict described the same id space
as the surviving frames was a size heuristic; nothing compared the
persisted strings against the strings the frames carry at overlapping
ids. When the surviving frames are self-sufficient and reference fewer
ids than the side-file holds, the discard did not fire, and the catch-up
registered a previous generation's symbols under ids this generation's
rows reference. Every guard passed: no gap, valid CRC, both bounds checks
satisfied, and the resumed producer wrote silently wrong SYMBOL values.

Stamp a generation id into the segment header, mirror it into the
dictionary header, and refuse to seed across a mismatch. Putting it in
the segment rather than beside it states the actual invariant -- the
dictionary belongs to the same producer generation as the frames
referencing its ids -- and catches segments from two lineages sharing a
directory, which a slot-level file cannot see.

This needed the segment header to grow 24 to 32 and its version to go to
2. That was previously rejected because it would owe a rule to segments
already written; QWP is experimental in the 9.x releases and
store-and-forward is reachable only through it, so no such rule is owed.

The dictionary format is numbered from 1. It is new on this branch, so
the 3 it carried described revisions that never left the branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
SegmentManager.serviceRing read e.ring.getActive().generation() with
no null check. SegmentRing.close() nulls active under its own
monitor with no coordination with the manager thread, so a
deregister/close racing a spare-provisioning tick could NPE. The
outcome was already benign (installHotSpare rejects on a closed ring
either way), but the NPE landed in a broad catch-all as a misleading
"will retry next tick" warning for a ring that will never accept a
spare again. Guard with a null-checked local instead, skipping spare
creation cleanly for a ring that has already closed.

CursorSendEngine.generation was assigned at construction and never
read back, since SegmentManager derives a rotation spare's generation
from the ring's active segment instead. Added a @testonly accessor and
a test that forces a real rotation and reopens the resulting spare
straight off disk to confirm its stamped generation matches the
engine's -- checking the persisted bytes, not just the in-memory field
the same create() call also sets.

Also replaced four test helpers' hardcoded offset 24 for the on-disk
generation field with MmapSegment.HEADER_SIZE - 8, so they can't drift
silently from the constant on a future header change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
Several comments and javadocs in the store-and-forward delta-dictionary
path still described the null-padding fallback and the source/binary
compatibility shims this branch removed earlier, plus stale 24-byte
segment-header arithmetic left over from the header growing to 32
bytes. Update them to match current behaviour and strip reasoning that
appealed to an install base this branch no longer supports:

- CursorWebSocketSendLoop/CursorSendEngine/UnreplayableSlotException:
  the torn-dictionary guard and related javadocs now describe the
  server rejecting a gapped frame with STATUS_DICTIONARY_GAP instead
  of null-padding it.
- CursorSendEngine's dictionary re-fold comment no longer argues from
  "every frame the shipped client ever wrote" / "the FIRST restart
  after upgrading"; it stands on the transient-plus-crash case alone.
- CursorWebSocketSendLoop/Sender: "legacy immediate escalation" ->
  "immediate escalation" (0 is a config value, not legacy behaviour);
  dropped a stale "retained for source and binary compatibility"
  sentence for a shim already deleted.
- MmapSegmentRecoveryFaultTest: replaced two false claims that
  reverting the mmap-fault guard "errors or aborts the fork" with a
  disclosure that on pre-21 JDKs the test degrades to tolerating any
  raw fault, so it does not pin the regression there; added a similar
  disclosure to the in-flight-holder test, which only exercises the
  pre-assignment throw.
- Files.java: corrected two places claiming Linux strips S_ISVTX via
  mkdir -- Linux honours it regardless of umask; the documented
  mode & ~umask & 0777 formula covers only the permission bits.
- SfDirPermissionsTest: rewrote the end-to-end test's javadoc, which
  overclaimed catching a regression the component-level test cannot;
  PosixFilePermission cannot observe the sticky bit on any platform,
  so the exact-mode guard remains the component-level test.
- CursorWebSocketSendLoopCatchUpAlignmentTest/OrphanTailTest,
  SegmentRingTest, SegmentManagerTest: corrected a deleted-fast-path
  comment and stale 24-byte header arithmetic (now 32).
- SegmentRingTest: all four single-segment-skip tests now assert the
  exact "skipped N unreadable segment(s)" message instead of a bare
  substring match.
- PersistedSymbolDictTest: added a missing // VERSION comment to
  match its twin test.

No production behaviour changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
Paragraph 1 said 01777 and 0755 collapse to an identical 0755 under
mode & ~umask. In full precision umask only clears the permission
bits, so the sticky bit survives (01777 & ~022 is 01755, not 0755) --
consistent with the Files.java fix earlier in this branch stating
Linux honours S_ISVTX regardless of umask. Restate the real reason
the end-to-end test cannot distinguish the two modes: the permission
bits do come out identical, but PosixFilePermission has no bit for
S_ISVTX on any platform, so the surviving sticky bit is simply
unobservable through it. The test's conclusion was already correct;
only the explanation was wrong.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
Final-review fix wave for the qwp-delta-symbol-dict branch: one
critical silent-data-loss bug plus several tests that could not fail.

- BackgroundDrainer now catches UnreplayableSlotException from
  CursorSendEngine's constructor and quarantines the slot (.failed
  sentinel), instead of falling through to a generic catch whose
  null-engine gate skipped the sentinel because the local engine
  reference is still null when this throw fires. Without this fix, a
  second orphan scan recovers cleanly past a skipped/corrupted
  segment and reports success over frames that were never sent. New
  test BackgroundDrainerUnreplayableSlotQuarantineTest, proven by
  mutation (reverting the new catch turns it red).

- PersistedSymbolDict.openFresh's untruncatable-file branch now
  throws UnreplayableSlotException instead of a plain
  LineSenderException, so Sender.build()'s existing constructor-time
  catch quarantines a fresh, dataless slot instead of bricking every
  restart under a stable senderId.

- SfDirPermissionsTest.recordedModeFor re-implemented the production
  sfDirShared ternary instead of observing it, so reverting the
  ternary left all three tests green. Added a swappable
  sfDirFilesFacade seam to Sender.build() (mirroring the existing
  quarantineFilesFacade pattern) and rewrote the test to build a real
  Sender and record the actual mkdir mode. Proven by mutation.

- MmapFaultDegradesTest only asserted isMmapAccessFault in isolation,
  never the two guards it exists to cover. Added a test that injects
  a recognised mmap-access-fault InternalError through
  persistNewSymbolsBeforePublish (via a FilesFacade seam on the
  persisted dictionary's mmap growth, the same deterministic
  technique MmapSegmentRecoveryFaultTest uses) and asserts the sender
  degrades to full self-sufficient frames instead of the fault
  escaping raw. Proven by mutation.

- CursorWebSocketSendLoopCatchUpAlignmentTest's
  testEmptyDictionaryReconnectSendsNoCatchUpFrame asserted only
  values that stay identical whether or not the sentDictCount > 0
  guard runs. Added an assertion on the existing onCapRead hook,
  which does discriminate it. Proven by mutation.

- Corrected stale comments left behind by earlier fix rounds:
  UnreplayableSlotException's javadoc (it is not the one verdict
  seedGlobalDictionaryFromPersisted reaches, and BackgroundDrainer
  also treats it as recoverable now), DeltaDictRecoveryTest's
  null-padding claims (the server rejects a dictionary gap, it does
  not null-pad it), and PersistedSymbolDictTest's header-size claim
  (16 bytes, not 8).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAW9kmZseGjpAUC1yqT2N6
Upstream 5bbdfb8 (#67) rewrote the same store-and-forward recovery core
this branch is built on, so this is a semantic reconciliation rather than
a textual merge. 17 files conflicted; the dispositions are:

Exception taxonomy. Adopt upstream's corruption/operational split whole.
Positively-identified corruption (MmapSegmentCorruptionException) is
quarantined; a plain MmapSegmentException is operational and aborts
startup without quarantining, so a transient EMFILE/ENOMEM can never
silently drop durable frames. Our rename-on-any-MmapSegmentException is
gone. UnreplayableSlotException survives -- it carries a verdict none of
upstream's types do, that the symbol dictionary cannot be rebuilt from
any source -- and every catch site that quarantines now orders the arms
SfSanitizedResidueException (retry), SfOperationalException (never
quarantine), UnreplayableSlotException, SfRecoveryException /
MmapSegmentCorruptionException, MmapSegmentException. Sender.build()
gained the same lattice: upstream's terminal recovery types would
otherwise escape it and re-brick the producer on every restart, which is
the failure the constructor-time quarantine arm exists to prevent.

Lineage id. Our per-producer generation stays in the segment header
(HEADER_SIZE 32, VERSION 2) but is renamed lineageId throughout, because
SfManifest.generation is a monotonic boundary-record counter and the two
meanings cannot share a word in one package. The id has to travel in the
same bytes as the frames whose symbol ids it validates, which is why it
is not in the manifest. SegmentRing.recover checks agreement across the
admitted chain and exposes recoveredLineageId().

Ack seeding and skipped segments. Upstream's SfManifest boundary checks
subsume our skip-tally refusal and are strictly stronger: a recorded
headBase/activeBase proves whether a set-aside file was load-bearing,
where the tally could only count. The one gap left was legacy migration,
which has no boundary to check against, so recover() refuses there when
the surviving chain is based above zero and a file was set aside during
that same recovery. A chain based at zero needs no check, and a chain
based above zero with nothing set aside is the ordinary steady state of
an ack-trimmed slot.

Dropped as premise-removed: MmapSegment's inFlight holder and
SegmentRing's per-file mmap-fault skip. Recovery now scans through
positioned reads into a bounded buffer and maps only after validating,
so no recovery byte is dereferenced through mmap and the async
unsafe-access fault they guarded cannot arise. isMmapAccessFault stays
for PersistedSymbolDict's append and heal paths, which are still folded
over Crc32c.updateUnsafe.

Also taken: sf_max_bytes -> sf_max_segment_bytes (#70), and upstream's
sealedHead deque, successor-linked nextSealedAfter and binary-search
findSegmentContaining, which supersede the equivalents this branch had
grown independently.

Two tests are disabled with the reason in their javadoc. Both tear a slot
by deleting sf-initial.sfa to model an ack-driven trim; a real trim now
durably advances the manifest head before unlinking, so a raw delete
reads as a head segment that vanished outside the protocol and recovery
fails closed before the dictionary-gap verdict they pin is reached. The
guards themselves are unchanged.

The local darwin-aarch64 native library was rebuilt: upstream added
openRWExclusive0, fsyncDir0 and SlotLock.release0.

Suite: 2943 pass, 0 fail, 4 skipped.
… not terminal

Use MmapSegmentException instead of SfRecoveryException: an unlink can fail
for a transient reason (share lock, antivirus/backup handle), same as the
manifest unlink four lines below in the same branch already assumes. The
narrower type quarantined the slot on the first failure via
BackgroundDrainer's OrphanScanner.markFailed instead of aborting startup
for a retry.
MmapSegmentException is a shared base for SfRecoveryException,
MmapSegmentCorruptionException and SfSanitizedResidueException, and the
substring "could not remove" also matches the sibling manifest-unlink
failure four lines below in the same branch. Assert on the full message
so the test can only pass on the leftover-unlink throw it targets.
…nary

PersistedSymbolDict no longer takes or checks a lineageId: the header
shrinks from 16 to 8 bytes (magic + version + reserved only), and
open()/openClean() and their FilesFacade-aware/private overloads drop
the trailing long parameter. VERSION stays 1 -- the on-disk format is
otherwise unchanged, so a recovered file that predates this change (or
one written by the Rust client, which uses the same magic under a
different, incompatible VERSION=2 body) is still handled identically.

CursorSendEngine's two call sites drop the argument; lineageIdInProgress
itself stays (it still feeds MmapSegment.create until a later task).
SegmentRing's stale @link target is fixed so Javadoc keeps resolving.

Test fallout: deleted the one test whose subject was the lineage check
(testRecoveryDiscardsADictionaryFromAnotherGeneration) and swept every
other call site across six test files to drop the trailing generation
argument and the now-unused generation-reading helpers/fields.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….open

Adds testExactHeaderSizeFileIsCompleteWhileOneByteShorterIsARecreatedStub,
covering a routing-semantics change that shipped untested in the lineage
removal: a file of exactly HEADER_SIZE (8) bytes now loads as a complete,
valid, empty dictionary via openExisting rather than being treated as a
crash-left sub-header stub and recreated -- while one byte shorter is
still recreated exactly as before.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ecreate

Side 1 of testExactHeaderSizeFileIsCompleteWhileOneByteShorterIsARecreatedStub
asserted only non-null / size() / file length, all of which openFresh would
reproduce byte-for-byte if it wrongly recreated an exactly-HEADER_SIZE file
(Files.openCleanRW truncates-and-recreates, and openFresh writes the same
magic/version/reserved bytes the fixture hand-writes) -- so a routing
regression from `existing >= HEADER_SIZE` to `existing > HEADER_SIZE` would
have passed silently.

Add usedMappedRecoveryInput() assertions on both sides: true only through
openExisting's mmap recovery, always false from openFresh, so it actually
distinguishes "loaded" from "recreated". Confirmed with a throwaway mutation
probe (reverted, not committed): flipping the routing comparison to `>`
fails exactly this test's new Side-1 assertion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SegmentRing.recover no longer compares lineage ids across the admitted
chain: the check made sense only while a recovered dictionary also had
to be matched against that lineage (Task 2 removed that side), and a
pre-manifest slot fed segments from two producer generations is no
longer reachable in any current build (MANIFEST_REQUIRED_FLAG rules it
out). recoveredLineageId (field + accessor) is deleted along with it;
CursorSendEngine drops the read that fed lineageIdInProgress on the
recovery path and updates the two comments that described the removed
check.

Deleted SegmentRingTest#testRecoveryRefusesSegmentsFromTwoLineages,
which constructed two single-frame segments with hand-picked lineage
ids and no manifest -- a state only a pre-manifest slot could reach.
That slot now migrates and replays instead of being refused.
…ites

The class Javadoc still described a "segment files on disk cannot be
trusted" cause family (an unreadable .sfa skip tally, and the
generation-disagreement check the prior commit deleted) that no
SegmentRing code path throws any more, and never mentioned the
fresh-slot truncate-failure throw in PersistedSymbolDict.openFresh at
all. Rewritten against the five live throw sites
(QwpWebSocketSender, CursorSendEngine, PersistedSymbolDict x3) so the
doc names only causes the code can actually produce.
Drops the lineageId producer-generation stamp from MmapSegment: the
32-byte/version-2 header returns to the 24-byte/version-1 layout that
agrees with java/main and rust/main. Removes MISSING_LINEAGE_ID,
lineageId(), the lineageId field/constructor parameter, the three
now-duplicate @testonly create() overloads, and the matching
CursorSendEngine/SegmentManager plumbing (derivation, pass-through,
accessor). Sweeps ~170 MmapSegment.create/createInMemory call sites
across main and test to drop the trailing lineage argument, and
removes now-unused GEN/GENERATION test fixtures and the stale comments
that named methods and tests earlier tasks already deleted.
comment, and cover manifestRequired on rotation spares

Final-review fix wave on the header-shrink change (three Minors, 0
Critical/Important):

- Three test comments still quoted pre-revert header sizes the lineage
  feature had rewritten (32-byte/version-2 MmapSegment header,
  16-byte PersistedSymbolDict header). Recomputed each from the
  current constants and the test's own arithmetic; both inequalities
  the tests rely on still hold, so no test behaviour changes.

- SegmentRing's clean-drain-crash-window comment overstated why a torn
  leftover whose quarantine rename fails "can never be mistaken for
  this generation's": quarantineFile only logs on a failed rename, so
  the file really can survive under its original name. The safety
  property comes from the NEXT recovery's FSN boundary checks (the
  active-boundary throw, validateContiguous's expected-next-base
  check, and the below-headBase exclusion/requarantine), not from the
  rename. Rewrote the comment to name the actual mechanism.

- Added coverage for the manifestRequired flag SegmentManager stamps
  on every rotation spare (SegmentManager.java:935), which had zero
  test coverage after the header-shrink task deleted the one test that
  happened to also touch a spare's on-disk bytes. Proved the new test
  binding by temporarily flipping true->false at that call site: the
  test fails (expected:<1> but was:<0>), then passes again once
  reverted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Multi-uid sharing of one sf_dir never worked: the merge base already
created sf_dir 0755, so a second uid could not create its slot
directory with or without the flag. sf_dir_shared therefore enabled a
scenario with no existing user, while adding a public config key, a
builder method, mode threading through SlotLock, BackgroundDrainer and
startOrphanDrainers, and a silent no-op on any pre-existing sf_dir
(mkdir is the only mode consumer and never touches an existing
directory).

Every directory now uses DIR_MODE_DEFAULT unconditionally, matching
the merge base. The .slot-locks logical lock itself is untouched: it
still arbitrates slot retirement, quarantine renames and orphan
adoption. The removal also deletes the sfDirFilesFacade test seam,
whose only consumer was the feature's own permission test.

If multi-uid sharing is ever needed, it wants a real design: a chmod
binding, mode verification against an existing tree, and docs.
removeOrphanLogical removed <name>.lock first. In the window between
that unlink and the pid unlink, a racing acquirer can create a fresh
lock inode at the freed pathname and write its own <name>.lock.pid;
the retiring process then deletes the successor's sidecar, so a third
contender's diagnostics report holder=unknown for the lifetime of the
successor's ownership.

Removing the sidecar first closes that variant at zero cost: after the
.lock unlink there is no successor whose sidecar the second remove
could hit. A recording-facade test pins the order.
swapClient set hasEverConnected = true before running the newly added
setWireBaselineWithCatchUp, so an ASYNC initial connect that completed
the WebSocket upgrade and then failed inside the dictionary catch-up
latched the flag with no connection ever fully established. From then
on endpointPolicyFailureIsTerminal() returned false for the sender's
whole life: a later auth, upgrade or durable-ack-capability rejection
dispatched as RETRIABLE, recordFatal never ran, close() had nothing to
rethrow, and the operator watched a mute sender buffer into
store-and-forward instead of learning the credentials were wrong.

swapClient now latches the flag after positionCursorAt, once the
connection is fully established. The catch-up failure path leaves the
sender INITIALIZING, so the next endpoint-policy rejection still
surfaces as the startup terminal the ASYNC contract promises.
snapshotReplayTarget, the only other reader, gates on the flag plus
nextWireSeq > 0 and only tightens with the move.

The new regression test drives the exact shape: a first reconnect
returns a client whose sends fail (the catch-up dies mid-flight), the
second attempt presents an auth failure, and the test asserts the
startup terminal latches with hasEverConnected still false.
The server rejects a delta or catch-up whose deltaStartId + deltaCount
exceeds MAX_SYMBOL_DICTIONARY_SIZE (1,000,000) and the sender classifies
that rejection as terminal, so in store-and-forward mode the 1,000,001st
distinct symbol value would strand the whole buffered backlog: every
reconnect, recovered slot and orphan drainer dies on its first catch-up
frame. Refuse the symbol at registration instead, before the row is
buffered, with an error naming the limit and the recovery. Rows using
already-registered values are unaffected, and everything buffered stays
deliverable (the server check is >, so an exactly-at-cap dictionary
still catches up cleanly).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFV4mYtzZqxr8gD9xSuJSu
symbol() past the cap throws before buffering, cancelRow() recovers the
row, the sender keeps working with registered values, and the wire never
carries the refused symbol. Adds a TestOnly accessor for the producer
dictionary so the fill does not need a million rows through the row API.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFV4mYtzZqxr8gD9xSuJSu
@mtopolnik

Copy link
Copy Markdown
Contributor

[PR Coverage check]

😍 pass : 1482 / 1644 (90.15%)

file detail

path covered line new line coverage
🔵 io/questdb/client/cutlass/qwp/client/WebSocketResponse.java 0 1 00.00%
🔵 io/questdb/client/Sender.java 72 96 75.00%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java 30 37 81.08%
🔵 io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java 147 175 84.00%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java 18 21 85.71%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/OrphanScanner.java 6 7 85.71%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/PersistedSymbolDict.java 387 439 88.15%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java 87 96 90.62%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java 382 412 92.72%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java 19 20 95.00%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/RecoveredFrameAnalysis.java 162 168 96.43%
🔵 io/questdb/client/cutlass/qwp/client/QwpWebSocketEncoder.java 54 54 100.00%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java 7 7 100.00%
🔵 io/questdb/client/cutlass/http/client/WebSocketClient.java 11 11 100.00%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/SlotLock.java 50 50 100.00%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/UnreplayableSlotException.java 2 2 100.00%
🔵 io/questdb/client/std/FilesFacade.java 1 1 100.00%
🔵 io/questdb/client/std/Crc32c.java 27 27 100.00%
🔵 io/questdb/client/cutlass/qwp/client/NativeBufferWriter.java 8 8 100.00%
🔵 io/questdb/client/cutlass/qwp/client/GlobalSymbolDictionary.java 8 8 100.00%
🔵 io/questdb/client/impl/ConfigSchema.java 1 1 100.00%
🔵 io/questdb/client/cutlass/qwp/client/BatchTooLargeForCapException.java 2 2 100.00%
🔵 io/questdb/client/SenderError.java 1 1 100.00%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request tandem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants