Skip to content

Commit fdd7141

Browse files
glasstigerclaude
andcommitted
Tidy delta symbol-dict dead code and edges
Minor follow-ups from the review. Remove the unused CursorSendEngine.isMemoryMode() -- it had no callers; isDeltaDictEnabled() tests sfDir == null directly. Sort the NativeBufferWriter import into its alphabetical place among the qwp.client imports in CursorWebSocketSendLoop. Harden PersistedSymbolDict.openExisting: hoist entriesAddr / entriesLen so the catch frees the loaded-entries buffer too. It is unreachable today (nothing between that malloc and the return throws, and on success the buffer is transferred to the returned dict), but the error path no longer leaks if a future edit adds a throwing step. Return a defensive copy from DeltaDictCatchUpTest's dictFor() instead of the live inner list: the caller iterates it unlocked while the server thread may still be appending, matching the sibling dictSnapshot(). Add testTornDictionaryOneIdGapFailsCleanly -- the tightest torn-dictionary boundary (deltaStart == recoveredSize + 1), the common one-entry-short host-crash tail. The existing torn test uses a 3-id gap, so it does not pin the guard's false-negative edge; a "deltaStart > size + 1" mutation passes it but ships the gapped frame here (the new test fails on it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 6d14727 commit fdd7141

5 files changed

Lines changed: 92 additions & 16 deletions

File tree

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

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -629,17 +629,6 @@ public long getTotalBackpressureStalls() {
629629
return backpressureStallCount.get();
630630
}
631631

632-
/**
633-
* True when this engine has no store-and-forward directory: the ring lives
634-
* only in malloc'd memory, so it cannot be recovered after a crash and no
635-
* orphan drainer ever replays it. Only in-process reconnect/failover replays
636-
* its frames, which is what makes send-time symbol-dict catch-up (rather than
637-
* fully self-sufficient frames) safe.
638-
*/
639-
public boolean isMemoryMode() {
640-
return sfDir == null;
641-
}
642-
643632
/**
644633
* Whether the sender may delta-encode symbol dictionaries on this engine.
645634
* Always true in memory mode (the send loop keeps an in-process catch-up

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,12 @@
3131
import io.questdb.client.cutlass.http.client.WebSocketFrameHandler;
3232
import io.questdb.client.cutlass.http.client.WebSocketUpgradeException;
3333
import io.questdb.client.cutlass.line.LineSenderException;
34+
import io.questdb.client.cutlass.qwp.client.NativeBufferWriter;
3435
import io.questdb.client.cutlass.qwp.client.QwpAuthFailedException;
3536
import io.questdb.client.cutlass.qwp.client.QwpDurableAckMismatchException;
3637
import io.questdb.client.cutlass.qwp.client.QwpIngressRoleRejectedException;
3738
import io.questdb.client.cutlass.qwp.client.QwpRoleMismatchException;
3839
import io.questdb.client.cutlass.qwp.client.QwpVersionMismatchException;
39-
import io.questdb.client.cutlass.qwp.client.NativeBufferWriter;
4040
import io.questdb.client.cutlass.qwp.client.WebSocketResponse;
4141
import io.questdb.client.cutlass.qwp.protocol.QwpConstants;
4242
import io.questdb.client.cutlass.qwp.websocket.WebSocketCloseCode;

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,8 @@ private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
353353
return null;
354354
}
355355
long buf = 0L;
356+
long entriesAddr = 0L;
357+
int entriesLen = 0;
356358
try {
357359
int len = (int) fileLen;
358360
buf = Unsafe.malloc(len, MemoryTag.NATIVE_DEFAULT);
@@ -388,8 +390,7 @@ private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
388390
pos = next + (int) entryLen;
389391
count++;
390392
}
391-
int entriesLen = pos - HEADER_SIZE;
392-
long entriesAddr = 0L;
393+
entriesLen = pos - HEADER_SIZE;
393394
if (entriesLen > 0) {
394395
entriesAddr = Unsafe.malloc(entriesLen, MemoryTag.NATIVE_DEFAULT);
395396
Unsafe.getUnsafe().copyMemory(buf + HEADER_SIZE, entriesAddr, entriesLen);
@@ -417,6 +418,13 @@ private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
417418
if (buf != 0L) {
418419
Unsafe.free(buf, (int) fileLen, MemoryTag.NATIVE_DEFAULT);
419420
}
421+
// entriesAddr is transferred to the returned dict on the success path,
422+
// and the catch only runs before that return, so freeing it here cannot
423+
// double-free. Unreachable today (nothing between its malloc and the
424+
// return throws), but keeps the error path leak-free against a future edit.
425+
if (entriesAddr != 0L) {
426+
Unsafe.free(entriesAddr, entriesLen, MemoryTag.NATIVE_DEFAULT);
427+
}
420428
Files.close(fd);
421429
LOG.warn("symbol dict {} recovery failed ({}); recreating", filePath, String.valueOf(t));
422430
return null;

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,9 @@ private static class CatchUpHandler implements TestWebSocketServer.WebSocketServ
272272

273273
synchronized List<String> dictFor(int connNumber) {
274274
return connNumber <= dictsByConn.size()
275-
? dictsByConn.get(connNumber - 1)
275+
// Copy under the lock: the caller iterates it unlocked while the
276+
// server thread may still be appending to the live inner list.
277+
? new ArrayList<>(dictsByConn.get(connNumber - 1))
276278
: new ArrayList<>();
277279
}
278280

@@ -406,7 +408,9 @@ private static class SplitCatchUpHandler implements TestWebSocketServer.WebSocke
406408

407409
synchronized List<String> dictFor(int connNumber) {
408410
return connNumber <= dictsByConn.size()
409-
? dictsByConn.get(connNumber - 1)
411+
// Copy under the lock: the caller iterates it unlocked while the
412+
// server thread may still be appending to the live inner list.
413+
? new ArrayList<>(dictsByConn.get(connNumber - 1))
410414
: new ArrayList<>();
411415
}
412416

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

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,81 @@ public void testTornDictionaryFailsCleanlyInsteadOfCorrupting() throws Exception
227227
});
228228
}
229229

230+
@Test
231+
public void testTornDictionaryOneIdGapFailsCleanly() throws Exception {
232+
// Boundary variant of testTornDictionaryFailsCleanlyInsteadOfCorrupting: the
233+
// recovered dictionary is short by EXACTLY ONE id -- the tightest gap the
234+
// guard must still reject (deltaStart == recoveredSize + 1). A one-entry-short
235+
// tail is the common host-crash outcome, so this pins the guard's
236+
// false-negative edge: a "deltaStart > size + 1" mutation would let this
237+
// single-id gap through and null-pad the missing symbol on the server.
238+
assertMemoryLeak(() -> {
239+
// Phase 1: each row introduces a new symbol (frame i carries deltaStart=i).
240+
try (TestWebSocketServer silent = new TestWebSocketServer(new SilentHandler())) {
241+
int port = silent.getPort();
242+
silent.start();
243+
Assert.assertTrue(silent.awaitStart(5, TimeUnit.SECONDS));
244+
String cfg = "ws::addr=localhost:" + port + ";sf_dir=" + sfDir
245+
+ ";close_flush_timeout_millis=0;";
246+
try (Sender s1 = Sender.fromConfig(cfg)) {
247+
for (int i = 0; i < 6; i++) {
248+
s1.table("m").symbol("s", "sym-" + i).longColumn("v", i).atNow();
249+
s1.flush();
250+
}
251+
}
252+
}
253+
254+
// Lose the whole dictionary (truncate to the 8-byte header, size 0) but
255+
// stamp the watermark at FSN 0, so recovery replays from FSN 1 -- a frame
256+
// with deltaStart=1. The dictionary covers no ids, so id 0 is the single
257+
// missing entry: deltaStart(1) == recoveredSize(0) + 1. Because size is 0
258+
// no catch-up is sent, so any counted frame would be the gapped data frame.
259+
java.nio.file.Path slot = Paths.get(sfDir, "default");
260+
java.nio.file.Path dict = slot.resolve(".symbol-dict");
261+
byte[] header = Arrays.copyOf(java.nio.file.Files.readAllBytes(dict), 8);
262+
java.nio.file.Files.write(dict, header);
263+
writeAckWatermark(slot.resolve(".ack-watermark"), 0);
264+
265+
// Phase 2: recover against a fresh counting server. The guard must fire on
266+
// the very first replay frame (deltaStart 1 > recovered size 0) and fail
267+
// terminally rather than send a frame that null-pads id 0.
268+
CountingHandler handler = new CountingHandler();
269+
try (TestWebSocketServer good = new TestWebSocketServer(handler)) {
270+
int port = good.getPort();
271+
good.start();
272+
Assert.assertTrue(good.awaitStart(5, TimeUnit.SECONDS));
273+
274+
String cfg = "ws::addr=localhost:" + port + ";sf_dir=" + sfDir + ";";
275+
LineSenderException terminal = null;
276+
Sender s2 = Sender.fromConfig(cfg);
277+
try {
278+
long deadline = System.currentTimeMillis() + 10_000;
279+
while (System.currentTimeMillis() < deadline && terminal == null) {
280+
try {
281+
s2.flush();
282+
Thread.sleep(20);
283+
} catch (LineSenderException e) {
284+
terminal = e;
285+
}
286+
}
287+
} finally {
288+
try {
289+
s2.close();
290+
} catch (LineSenderException e) {
291+
if (terminal == null) {
292+
terminal = e;
293+
}
294+
}
295+
}
296+
Assert.assertEquals("a one-id gap must still block replay to a fresh server",
297+
0, handler.frames.get());
298+
Assert.assertNotNull("a one-id-short dictionary must surface a terminal error", terminal);
299+
Assert.assertTrue(terminal.getMessage(),
300+
terminal.getMessage().contains("delta start 1 exceeds recovered dictionary size 0"));
301+
}
302+
});
303+
}
304+
230305
@Test
231306
public void testRecoveredSenderContinuesIngestingNewSymbols() throws Exception {
232307
// M2 regression: seedGlobalDictionaryFromPersisted resumes the producer's

0 commit comments

Comments
 (0)