Skip to content

Commit 7880b7a

Browse files
glasstigerclaude
andcommitted
Cover defensive dict guards and tidy test helpers
Round of minor cleanups on the delta symbol-dictionary work. Tests for previously-uncovered defensive guards (all proven to fail without their guard): PersistedSymbolDict.open recreates empty on a bad version byte (a valid entry behind a corrupted version must not parse) and on a > Integer.MAX_VALUE length (a >2GB dictionary can't map into one int buffer -- driven by a fault facade); and the send loop's ensureSentDictCapacity latches a terminal instead of overflowing the int capacity math when the mirror would exceed MAX_SENT_DICT_BYTES. openExisting nulls buf after freeing it on the bad-magic/bad-version path, so the catch block cannot double-free if ff.close ever threw. PersistedSymbolDict.appendSymbol is marked @testonly -- production persists a frame's whole new-symbol range via appendSymbols / appendRawEntries; only tests append one symbol at a time. Documented that symbol-dict catch-up frames sit outside the poison-frame detector: a deterministically-NACKed catch-up recycles forever (paced), which is fine since a catch-up only re-registers already-accepted symbols. Consolidated four reinvented recursive rmDir/rmDirRec test helpers into one TestUtils.removeTmpDirRec (the existing removeTmpDir is flat and can't clean the store-and-forward slot subdirectory layout), and removed the imports that freed up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 50fc218 commit 7880b7a

8 files changed

Lines changed: 188 additions & 93 deletions

File tree

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2911,6 +2911,13 @@ private void handleServerRejection(long wireSeq) {
29112911
// path, where engine.acknowledge() no-ops at or below ackedFsn. A
29122912
// real replayed data frame is at fsn > ackedFsn, so it is never
29132913
// caught here.
2914+
//
2915+
// Catch-up frames therefore sit OUTSIDE the poison detector: a
2916+
// deterministically-NACKed catch-up recycles forever (paced, so no
2917+
// busy-loop). That is acceptable -- a catch-up only re-registers
2918+
// symbols the cluster already accepted, so a persistent NACK of one
2919+
// is a server bug, not a poison-frame the client can quarantine, and
2920+
// Invariant B's "retry a transient outage forever" applies.
29142921
handlePreSendRejection(wireSeq, status, category, policy);
29152922
return;
29162923
}

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import io.questdb.client.std.QuietCloseable;
3434
import io.questdb.client.std.Unsafe;
3535
import io.questdb.client.std.str.Utf8s;
36+
import org.jetbrains.annotations.TestOnly;
3637
import org.slf4j.Logger;
3738
import org.slf4j.LoggerFactory;
3839

@@ -317,7 +318,12 @@ public synchronized void appendRawEntries(long addr, int len, int count) {
317318
* (the entry's position). Writes {@code [len varint][utf8][crc32c]}, the CRC
318319
* covering the {@code [len][utf8]} bytes so a torn/stale entry is detected on
319320
* recovery.
321+
* <p>
322+
* Test-only: production persists a frame's whole new-symbol range in one write
323+
* via {@link #appendSymbols} / {@link #appendRawEntries}. Tests use this to
324+
* build a dictionary one entry at a time.
320325
*/
326+
@TestOnly
321327
public synchronized void appendSymbol(CharSequence symbol) {
322328
if (closed) {
323329
return;
@@ -516,6 +522,7 @@ private static PersistedSymbolDict openExisting(FilesFacade ff, String filePath,
516522
|| Unsafe.getUnsafe().getByte(buf + 4) != VERSION) {
517523
LOG.warn("symbol dict {} unreadable, bad magic or unknown version; recreating", filePath);
518524
Unsafe.free(buf, len, MemoryTag.NATIVE_DEFAULT);
525+
buf = 0L; // null after free so the catch below cannot double-free if ff.close throws
519526
ff.close(fd);
520527
return null;
521528
}

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

Lines changed: 1 addition & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import io.questdb.client.Sender;
2828
import io.questdb.client.cutlass.line.LineSenderException;
2929
import io.questdb.client.cutlass.qwp.client.sf.cursor.PersistedSymbolDict;
30-
import io.questdb.client.std.Files;
3130
import io.questdb.client.test.cutlass.qwp.websocket.TestWebSocketServer;
3231
import io.questdb.client.test.tools.TestUtils;
3332
import org.junit.After;
@@ -76,9 +75,7 @@ public void setUp() {
7675

7776
@After
7877
public void tearDown() {
79-
if (sfDir != null) {
80-
rmDirRec(sfDir);
81-
}
78+
TestUtils.removeTmpDirRec(sfDir);
8279
}
8380

8481
@Test
@@ -879,31 +876,6 @@ private static int readVarint(byte[] buf, int[] pos) {
879876
throw new IllegalStateException("varint truncated");
880877
}
881878

882-
private static void rmDirRec(String dir) {
883-
if (!Files.exists(dir)) {
884-
return;
885-
}
886-
long find = Files.findFirst(dir);
887-
if (find > 0) {
888-
try {
889-
int rc = 1;
890-
while (rc > 0) {
891-
String name = Files.utf8ToString(Files.findName(find));
892-
if (name != null && !".".equals(name) && !"..".equals(name)) {
893-
String child = dir + "/" + name;
894-
if (!Files.remove(child)) {
895-
rmDirRec(child);
896-
}
897-
}
898-
rc = Files.findNext(find);
899-
}
900-
} finally {
901-
Files.findClose(find);
902-
}
903-
}
904-
Files.remove(dir);
905-
}
906-
907879
/**
908880
* Reconstructs the per-connection symbol dictionary from delta sections,
909881
* mirroring the server's {@code setQuick(deltaStart + i)} + null-padding.

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

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import io.questdb.client.Sender;
2828
import io.questdb.client.cutlass.line.LineSenderException;
2929
import io.questdb.client.test.cutlass.qwp.websocket.TestWebSocketServer;
30+
import io.questdb.client.test.tools.TestUtils;
3031
import org.junit.Assert;
3132
import org.junit.Test;
3233

@@ -35,10 +36,8 @@
3536
import java.nio.ByteOrder;
3637
import java.nio.file.Files;
3738
import java.nio.file.Path;
38-
import java.util.Comparator;
3939
import java.util.concurrent.TimeUnit;
4040
import java.util.concurrent.atomic.AtomicLong;
41-
import java.util.stream.Stream;
4241

4342
import static io.questdb.client.test.tools.TestUtils.assertMemoryLeak;
4443

@@ -348,25 +347,7 @@ private static int readVarint(byte[] buf, int offset) {
348347
}
349348

350349
private static void rmDir(Path dir) {
351-
try {
352-
if (dir == null || !Files.exists(dir)) {
353-
return;
354-
}
355-
// try-with-resources: Files.walk returns a Stream backed by an open
356-
// directory handle that must be closed, or each rmDir leaks a descriptor.
357-
try (Stream<Path> walk = Files.walk(dir)) {
358-
walk.sorted(Comparator.reverseOrder())
359-
.forEach(p -> {
360-
try {
361-
Files.deleteIfExists(p);
362-
} catch (IOException ignored) {
363-
// best-effort
364-
}
365-
});
366-
}
367-
} catch (IOException ignored) {
368-
// best-effort
369-
}
350+
TestUtils.removeTmpDirRec(dir == null ? null : dir.toString());
370351
}
371352

372353
private static void waitFor(BoolCondition cond, long timeoutMillis) {

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopCatchUpAlignmentTest.java

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,47 @@ public void testCatchUpCapGapRetriesUntilBudgetThenLatches() throws Exception {
321321
});
322322
}
323323

324+
@Test
325+
public void testMirrorOverflowFailsLoud() throws Exception {
326+
// ensureSentDictCapacity must latch a terminal -- not silently overflow the
327+
// int capacity math into a heap-corrupting copyMemory -- when the sent-dict
328+
// mirror would exceed MAX_SENT_DICT_BYTES. Unreachable at real cardinality
329+
// (~200M+ symbols on one connection), so drive the guard directly with an
330+
// oversized required, mirroring testCatchUpChunkFrameSizeOverflowFailsLoud.
331+
TestUtils.assertMemoryLeak(() -> {
332+
Field maxField = CursorWebSocketSendLoop.class.getDeclaredField("MAX_SENT_DICT_BYTES");
333+
maxField.setAccessible(true);
334+
long overCeiling = (long) maxField.getInt(null) + 1L;
335+
CatchUpCapturingClient client = new CatchUpCapturingClient(0);
336+
try (CursorSendEngine engine = newEngine()) {
337+
CursorWebSocketSendLoop loop = newLoop(engine, client);
338+
try {
339+
Method m = CursorWebSocketSendLoop.class.getDeclaredMethod("ensureSentDictCapacity", long.class);
340+
m.setAccessible(true);
341+
try {
342+
m.invoke(loop, overCeiling);
343+
fail("a mirror capacity past MAX_SENT_DICT_BYTES must fail loud, not overflow");
344+
} catch (InvocationTargetException e) {
345+
assertEquals("overflow must surface as LineSenderException",
346+
"LineSenderException", e.getCause().getClass().getSimpleName());
347+
assertTrue("message must name the mirror ceiling: " + e.getCause().getMessage(),
348+
e.getCause().getMessage().contains("mirror exceeds the maximum size"));
349+
}
350+
// recordFatal (not a bare throw) latched the terminal, so the loop
351+
// winds down instead of reconnecting into the same overflow.
352+
try {
353+
loop.checkError();
354+
fail("mirror overflow must latch a terminal");
355+
} catch (LineSenderException terminal) {
356+
assertTrue(terminal.getMessage().contains("mirror exceeds the maximum size"));
357+
}
358+
} finally {
359+
loop.close();
360+
}
361+
}
362+
});
363+
}
364+
324365
private static void appendFrames(CursorSendEngine engine, int count) {
325366
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
326367
try {

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopMirrorLeakTest.java

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,15 @@
2929
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorWebSocketSendLoop;
3030
import io.questdb.client.cutlass.qwp.client.sf.cursor.PersistedSymbolDict;
3131
import io.questdb.client.test.cutlass.qwp.websocket.TestWebSocketServer;
32+
import io.questdb.client.test.tools.TestUtils;
3233
import org.junit.Assert;
3334
import org.junit.Test;
3435

3536
import java.io.IOException;
3637
import java.lang.reflect.Field;
3738
import java.nio.file.Files;
3839
import java.nio.file.Path;
39-
import java.util.Comparator;
4040
import java.util.concurrent.TimeUnit;
41-
import java.util.stream.Stream;
4241

4342
import static io.questdb.client.test.tools.TestUtils.assertMemoryLeak;
4443

@@ -197,21 +196,7 @@ private static int readInt(CursorWebSocketSendLoop loop, String name) throws Exc
197196
}
198197

199198
private static void rmDir(Path dir) throws IOException {
200-
if (dir == null || !Files.exists(dir)) {
201-
return;
202-
}
203-
// try-with-resources: Files.walk returns a Stream backed by an open
204-
// directory handle that must be closed, or each rmDir leaks a descriptor.
205-
try (Stream<Path> walk = Files.walk(dir)) {
206-
walk.sorted(Comparator.reverseOrder())
207-
.forEach(p -> {
208-
try {
209-
Files.deleteIfExists(p);
210-
} catch (IOException ignored) {
211-
// best-effort
212-
}
213-
});
214-
}
199+
TestUtils.removeTmpDirRec(dir == null ? null : dir.toString());
215200
}
216201

217202
private static class SilentHandler implements TestWebSocketServer.WebSocketServerHandler {

0 commit comments

Comments
 (0)