Skip to content

Commit 41fddb1

Browse files
glasstigerclaude
andcommitted
Guard against persisted-dict duplication on a failed publish
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>
1 parent 034d8f3 commit 41fddb1

1 file changed

Lines changed: 65 additions & 0 deletions

File tree

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626

2727
import io.questdb.client.Sender;
2828
import io.questdb.client.cutlass.line.LineSenderException;
29+
import io.questdb.client.cutlass.qwp.client.sf.cursor.PersistedSymbolDict;
2930
import io.questdb.client.std.Files;
3031
import io.questdb.client.test.cutlass.qwp.websocket.TestWebSocketServer;
3132
import io.questdb.client.test.tools.TestUtils;
@@ -226,6 +227,70 @@ public void testTornDictionaryFailsCleanlyInsteadOfCorrupting() throws Exception
226227
});
227228
}
228229

230+
@Test
231+
public void testFailedPublishDoesNotDuplicatePersistedSymbols() throws Exception {
232+
// Regression: persistNewSymbolsBeforePublish is a write-ahead -- it runs
233+
// BEFORE the frame is published (sealAndSwapBuffer -> appendBlocking). If
234+
// publish fails (here PAYLOAD_TOO_LARGE, a frame bigger than the SF
235+
// segment; a backpressure deadline in production), the frame's symbols are
236+
// already on disk but sentMaxSymbolId is NOT advanced and the rows stay
237+
// buffered -- so a retry re-runs the persist. Keying the persist range off
238+
// pd.size() (not sentMaxSymbolId+1) makes it idempotent. Before that fix,
239+
// the retry appended the symbol a SECOND time, breaking the dense
240+
// id->position invariant; on recovery every later global id shifts by one
241+
// and symbol column values are silently misattributed.
242+
assertMemoryLeak(() -> {
243+
try (TestWebSocketServer silent = new TestWebSocketServer(new SilentHandler())) {
244+
int port = silent.getPort();
245+
silent.start();
246+
Assert.assertTrue(silent.awaitStart(5, TimeUnit.SECONDS));
247+
248+
// Small segment; a heavily padded row's frame cannot fit, so
249+
// appendBlocking throws PAYLOAD_TOO_LARGE deterministically -- no
250+
// backpressure timing needed. The server never acks (SilentHandler).
251+
String cfg = "ws::addr=localhost:" + port + ";sf_dir=" + sfDir + ";sf_max_bytes=1024;";
252+
String pad = TestUtils.repeat("x", 2000); // frame >> 1024-byte segment
253+
Sender sender = Sender.fromConfig(cfg);
254+
try {
255+
// Buffer ONE new-symbol row, then flush it TWICE. Each flush
256+
// runs the write-ahead persist and then fails to publish; the
257+
// failed flush leaves the row buffered, so the second flush is
258+
// the retry that (pre-fix) duplicated the persisted symbol.
259+
sender.table("m").symbol("s", "s0").stringColumn("p", pad).longColumn("v", 1L).atNow();
260+
for (int attempt = 0; attempt < 2; attempt++) {
261+
try {
262+
sender.flush();
263+
Assert.fail("oversized frame must fail to publish");
264+
} catch (LineSenderException expected) {
265+
// frame too large -- expected on every attempt
266+
}
267+
}
268+
269+
// The persisted dictionary must hold "s0" EXACTLY ONCE.
270+
// Pre-fix, the retry duplicated it (size == 2).
271+
PersistedSymbolDict pd = PersistedSymbolDict.open(Paths.get(sfDir, "default").toString());
272+
Assert.assertNotNull(pd);
273+
try {
274+
Assert.assertEquals("failed-publish retry must not duplicate the persisted symbol",
275+
1, pd.size());
276+
Assert.assertEquals("s0", pd.readLoadedSymbols().getQuick(0));
277+
} finally {
278+
pd.close();
279+
}
280+
} finally {
281+
try {
282+
sender.close();
283+
} catch (LineSenderException ignored) {
284+
// close() re-flushes the still-buffered oversized row and
285+
// fails again (PAYLOAD_TOO_LARGE); expected here and not
286+
// what we assert. close() still runs its resource cleanup,
287+
// so no native memory leaks.
288+
}
289+
}
290+
}
291+
});
292+
}
293+
229294
private static void writeAckWatermark(java.nio.file.Path path, long fsn) throws IOException {
230295
byte[] buf = new byte[16];
231296
ByteBuffer bb = ByteBuffer.wrap(buf).order(ByteOrder.LITTLE_ENDIAN);

0 commit comments

Comments
 (0)