Skip to content

Commit 2fdbbf9

Browse files
glasstigerclaude
andcommitted
Fix catch-up terminal on a homogeneous batch cap
The reconnect symbol-dictionary catch-up latched a terminal for any entry exceeding the packing budget (cap - HEADER_SIZE - 16). That reserve is larger than the minimal data-frame overhead, so a symbol the producer had already shipped under a given cap could exceed the budget and trip the terminal on reconnect -- permanently hard-failing a running producer on a homogeneous single-cap cluster, the exact failure the store-and-forward path exists to survive. sendDictCatchUp now tests the actual solo catch-up frame (header + the entry's delta varints + the entry bytes) against the cap. That frame is always smaller than the data frame that already carried the entry, so an accepted symbol always re-registers on the same cap; a genuinely oversized entry on a shrunk (heterogeneous) cap still terminates. The conservative packing budget stays, used only for multi-entry chunk splitting. Add a fixed-cap regression test: a 173-char symbol under a fixed cap of 200 is accepted, then re-registers gap-free across a reconnect. It fails before this change, where the reverted condition terminates a frame that fits the cap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent fdd7141 commit 2fdbbf9

2 files changed

Lines changed: 91 additions & 8 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2171,12 +2171,21 @@ private long readVarintAt(long p, long limit) {
21712171
*/
21722172
private int sendDictCatchUp() {
21732173
int cap = client.getServerMaxBatchSize();
2174-
// Symbol-bytes budget per frame, leaving room for the 12-byte header and
2175-
// the two delta-section varints. cap <= 0 means the server advertised no
2176-
// limit -- send the whole dictionary in one frame, but still bound the
2177-
// budget by MAX_SENT_DICT_BYTES so sendCatchUpChunk's int frameLen
2178-
// (HEADER_SIZE + varints + symbolsLen) cannot overflow on a pathological
2179-
// multi-GB dictionary (unreachable at real cardinality; defensive).
2174+
// The frame ceiling a catch-up chunk must not exceed: the server's
2175+
// advertised cap, or -- when the server advertises none (cap <= 0) --
2176+
// MAX_SENT_DICT_BYTES so sendCatchUpChunk's int frameLen (HEADER_SIZE +
2177+
// varints + symbolsLen) cannot overflow on a pathological multi-GB
2178+
// dictionary (unreachable at real cardinality; defensive). Used by the
2179+
// single-entry terminal below, which measures the real solo frame.
2180+
int frameLimit = cap > 0 ? cap : MAX_SENT_DICT_BYTES;
2181+
// Symbol-bytes budget for PACKING several entries into one chunk, leaving
2182+
// room for the 12-byte header and the two delta-section varints. Kept
2183+
// deliberately conservative (reserving 16 for the varints): it only makes a
2184+
// multi-entry chunk split marginally earlier, never over the cap. It must
2185+
// NOT gate the single-entry terminal -- that reserve is larger than the
2186+
// minimal data-frame overhead, so an entry the producer already shipped
2187+
// under this cap could exceed the reserve yet still fit its own catch-up
2188+
// frame; the terminal tests the real solo frame against frameLimit instead.
21802189
int budget = cap > 0
21812190
? Math.max(1, cap - QwpConstants.HEADER_SIZE - 16)
21822191
: MAX_SENT_DICT_BYTES - QwpConstants.HEADER_SIZE - 16;
@@ -2192,7 +2201,20 @@ private int sendDictCatchUp() {
21922201
long len = readVarintAt(p, limit); // entry length prefix; varintEnd -> just past prefix
21932202
long entryEnd = varintEnd + len; // just past [len varint][utf8 bytes]
21942203
long entryBytes = entryEnd - entryStart;
2195-
if (entryBytes > budget) {
2204+
// The exact table-less frame sendCatchUpChunk would build for THIS entry
2205+
// alone: header + deltaStart varint (the entry's own global id) +
2206+
// deltaCount varint (1) + the entry bytes. Terminal only when even that
2207+
// solo frame exceeds the cap -- i.e. the entry genuinely cannot be
2208+
// re-registered. Testing the real solo frame (not the conservative
2209+
// packing budget above) is what keeps a HOMOGENEOUS cluster
2210+
// livelock-free: an entry the producer already shipped in a data frame
2211+
// under this cap (header + delta varints + entry + schema + >=1 row) is
2212+
// strictly larger than its bare catch-up frame, so it always fits here.
2213+
long soloFrameLen = QwpConstants.HEADER_SIZE
2214+
+ NativeBufferWriter.varintSize(chunkStartId + chunkSymbols)
2215+
+ NativeBufferWriter.varintSize(1)
2216+
+ entryBytes;
2217+
if (soloFrameLen > frameLimit) {
21962218
// Non-retriable: the entry will not shrink and the same cluster
21972219
// re-advertises the same cap, so reconnecting would livelock.
21982220
// Latch a terminal (the data must be resent after the cap is
@@ -2209,7 +2231,7 @@ private int sendDictCatchUp() {
22092231
// the terminal keeps the homogeneous common case livelock-free.
22102232
LineSenderException err = new LineSenderException(
22112233
"symbol dictionary entry too large for the server batch cap during catch-up ["
2212-
+ "entryBytes=" + entryBytes + ", budget=" + budget + ']');
2234+
+ "frameLen=" + soloFrameLen + ", cap=" + cap + ']');
22132235
recordFatal(err);
22142236
throw new CatchUpSendException(err);
22152237
}

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/DeltaDictCatchUpTest.java

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,67 @@ public void testReconnectCatchUpRebuildsDictionary() throws Exception {
9999
});
100100
}
101101

102+
@Test
103+
public void testFixedCapNearBoundarySymbolCatchesUpWithoutTerminal() throws Exception {
104+
// Regression (homogeneous single cap): a symbol whose length sits just below
105+
// the advertised cap is ACCEPTED into a data frame (messageSize <= cap) and
106+
// enters the sent-dictionary mirror. On reconnect the catch-up must
107+
// re-register it under the SAME cap -- the bare catch-up frame (header + two
108+
// varints + the entry) is smaller than the data frame that already shipped
109+
// it (which also carried the table schema + a row), so it fits.
110+
//
111+
// Pre-fix, the single-entry terminal used the conservative PACKING budget
112+
// (cap - HEADER_SIZE - 16), which is stricter than the producer's publish
113+
// gate (messageSize <= cap) by more than the minimal data-frame overhead. So
114+
// a symbol accepted onto the wire under cap C could exceed that budget and
115+
// trip a spurious "during catch-up" terminal, permanently hard-failing a
116+
// running producer on its first transient reconnect. Concretely at cap=200:
117+
// table("t").symbol("s", <173 chars>).atNow() encodes to 198 bytes (<=200,
118+
// accepted), its dict entry is 2+173=175 bytes (> old budget 172 -> old
119+
// terminal), while the real solo catch-up frame is 12+1+1+175=189 (<=200 ->
120+
// fits). Unlike testCatchUpEntryTooLargeForCapFailsTerminally (a genuinely
121+
// oversized entry on a shrunk cap, which MUST still terminate), this entry
122+
// is legally shippable and must NOT terminate.
123+
final int cap = 200;
124+
final String nearCapSymbol = TestUtils.repeat("x", 173);
125+
assertMemoryLeak(() -> {
126+
CatchUpHandler handler = new CatchUpHandler();
127+
try (TestWebSocketServer server = new TestWebSocketServer(handler)) {
128+
server.setAdvertisedMaxBatchSize(cap); // same cap on every handshake (homogeneous)
129+
int port = server.getPort();
130+
server.start();
131+
Assert.assertTrue(server.awaitStart(5, TimeUnit.SECONDS));
132+
133+
try (Sender sender = Sender.fromConfig("ws::addr=localhost:" + port + ";")) {
134+
// Symbol-only row so the near-cap symbol drives the frame size.
135+
sender.table("t").symbol("s", nearCapSymbol).atNow();
136+
sender.flush();
137+
waitFor(() -> handler.dictFor(1).size() >= 1, 5_000);
138+
waitFor(() -> handler.conn1Closed, 5_000);
139+
140+
// The reconnect runs the catch-up under the SAME cap. Pre-fix
141+
// this latched a terminal (surfacing on this flush); post-fix the
142+
// catch-up ships the near-cap symbol and the flush goes through.
143+
sender.table("t").symbol("s", "beta").atNow();
144+
sender.flush();
145+
waitFor(() -> handler.connectionsAccepted.get() >= 2
146+
&& handler.dictFor(2).size() >= 2, 5_000);
147+
}
148+
149+
// Connection 2's dictionary, rebuilt purely from the frames it
150+
// received, must hold the near-cap symbol (re-registered by the
151+
// catch-up) and beta, gap-free -- proving the catch-up SHIPPED rather
152+
// than terminated.
153+
List<String> conn2 = handler.dictFor(2);
154+
Assert.assertTrue("2nd connection saw a catch-up frame with 0 tables",
155+
handler.sawZeroTableFrameOnConn2);
156+
Assert.assertEquals("2nd connection dictionary size", 2, conn2.size());
157+
Assert.assertEquals(nearCapSymbol, conn2.get(0));
158+
Assert.assertEquals("beta", conn2.get(1));
159+
}
160+
});
161+
}
162+
102163
@Test
103164
public void testCatchUpEntryTooLargeForCapFailsTerminally() throws Exception {
104165
// A dictionary entry that exceeds the reconnect server's per-chunk budget

0 commit comments

Comments
 (0)