Skip to content

Commit 0520dc4

Browse files
glasstigerclaude
andcommitted
Don't quarantine a fully-acked torn-dict slot
seedGlobalDictionaryFromPersisted walked collectReplaySymbolsAbove from FSN 0 and threw UnreplayableSlotException on the first symbol-dictionary gap regardless of ack state. For a recovered slot whose committed frames were all acked -- so nothing was left to replay -- a gap in that already delivered data still threw. (A host crash tore the unsynced side-file, or it could not be opened, while the low segments that registered the missing ids were trimmed away.) build() then set the slot aside as <sender>.unreplayable-<n>, its fully-drained close deleted the delivered bytes the quarantine promises to preserve, and it logged a false "resend required" for data the server had already acknowledged. That gap only matters for frames that will REPLAY. When every committed frame has been acked (ackedFsn >= recoveredCommitBoundaryFsn), the seed now suppresses the throw: it seeds the intact prefix only, leaving the frame-derived symbols unused exactly as the send loop's mirror does on a -1, so the producer baseline and the mirror's sentDictCount still agree by construction, and resumes on the same slot. A genuine gap in UNACKED committed frames still throws, so the real-unreplayable quarantine path is unchanged. Add testFullyAckedTornSlotResumesInPlaceWithoutQuarantine, the acked counterpart to the existing unacked quarantine test: the same on-disk tear, but with the ack watermark stamped at the last committed frame, asserting no set-aside copy exists and the sender resumes in place. The test fails without the production change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c0c2ce2 commit 0520dc4

2 files changed

Lines changed: 83 additions & 0 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3933,6 +3933,33 @@ private void seedGlobalDictionaryFromPersisted(PersistedSymbolDict pd) {
39333933
ObjList<String> fromFrames = new ObjList<>();
39343934
long coverage = cursorEngine.collectReplaySymbolsAbove(baseline, fromFrames);
39353935
if (coverage < 0) {
3936+
// A gap: the surviving frames reference ids below their own delta start,
3937+
// introduced by frames since acked and trimmed away, and the persisted
3938+
// dictionary no longer holds them (a host crash tore its unsynced tail, or it
3939+
// could not be opened). That gap only matters for frames that will REPLAY.
3940+
// When every recovered committed frame is already acked
3941+
// (ackedFsn >= recoveredCommitBoundaryFsn), NOTHING replays: the gap is in
3942+
// data the server already has, and the retired orphan-deferred tail above the
3943+
// commit boundary is never transmitted. Throwing here would raise a false
3944+
// "resend required" for delivered data AND -- because such a slot is fully
3945+
// drained -- let build()'s connect-path close unlink the (already-delivered)
3946+
// bytes the quarantine claims to preserve. So DON'T throw: seed the intact
3947+
// prefix only, leaving fromFrames unused exactly as the send loop's mirror
3948+
// does on a -1 (it adds nothing), so the producer baseline and the mirror's
3949+
// sentDictCount still agree by construction. The producer resumes above the
3950+
// prefix and the fully-drained slot is cleaned up on close.
3951+
long ackedFsn = cursorEngine.ackedFsn();
3952+
long commitBoundaryFsn = cursorEngine.recoveredCommitBoundaryFsn();
3953+
if (ackedFsn >= commitBoundaryFsn) {
3954+
sentMaxSymbolId = globalSymbolDictionary.size() - 1;
3955+
LOG.info("recovered store-and-forward slot has a torn/incomplete symbol dictionary, "
3956+
+ "but every committed frame was already acked so nothing needs replaying; "
3957+
+ "resuming on the intact prefix without quarantine and without data loss "
3958+
+ "[recoveredPrefixSize={}, ackedFsn={}, commitBoundaryFsn={}]",
3959+
baseline, ackedFsn, commitBoundaryFsn);
3960+
return;
3961+
}
3962+
// Genuine loss: unacked committed frames reference ids nothing still holds.
39363963
// Typed, because Sender.build() sets such a slot aside instead of failing: this
39373964
// is the point at which every source of truth has been tried and none of them
39383965
// holds the missing ids. See UnreplayableSlotException.

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,62 @@ public void testTrimmedRegisteringFramesAreUnreplayableAndTheSlotIsSetAside() th
249249
});
250250
}
251251

252+
@Test
253+
public void testFullyAckedTornSlotResumesInPlaceWithoutQuarantine() throws Exception {
254+
// M1 regression -- the ACKED counterpart to
255+
// testTrimmedRegisteringFramesAreUnreplayableAndTheSlotIsSetAside. The on-disk
256+
// tear is IDENTICAL (earliest segment trimmed, .symbol-dict torn to its header,
257+
// so the surviving frames' deltas start above ids nothing on disk still holds),
258+
// but here every committed frame was already ACKED before the crash. Nothing is
259+
// left to replay, so the "gap" is entirely in data the server already has.
260+
//
261+
// seedGlobalDictionaryFromPersisted must therefore NOT raise
262+
// UnreplayableSlotException. Quarantining a fully-delivered slot would fire a
263+
// false "resend required" alarm AND -- because such a slot is fully drained --
264+
// let build()'s connect-path close unlink the (already-delivered) bytes the
265+
// quarantine claims to preserve. The slot must resume IN PLACE.
266+
//
267+
// Before the fix the gap detector ignored ack state and threw, so this slot was
268+
// set aside as default.unreplayable-0 with a "resend required" error for data the
269+
// server had already acknowledged.
270+
assertMemoryLeak(() -> {
271+
writeAndTearUnreplayableSlot();
272+
// Mark every committed frame acked. writeAndTearUnreplayableSlot writes 12
273+
// frames (FSNs 0..11), so stamping the watermark at 11 makes
274+
// ackedFsn == recoveredCommitBoundaryFsn: a torn dictionary with nothing left
275+
// to replay. (Not higher than 11 -- that would make the resuming producer's
276+
// next frame look pre-acked.)
277+
java.nio.file.Path slot = Paths.get(sfDir, "default");
278+
writeAckWatermark(slot.resolve(".ack-watermark"), 11);
279+
280+
DictReconstructingHandler handler = new DictReconstructingHandler();
281+
try (TestWebSocketServer good = new TestWebSocketServer(handler)) {
282+
int port = good.getPort();
283+
good.start();
284+
Assert.assertTrue(good.awaitStart(5, TimeUnit.SECONDS));
285+
String cfg = "ws::addr=localhost:" + port + ";sf_dir=" + sfDir + ";";
286+
// build() must succeed WITHOUT setting the slot aside, and the producer
287+
// must keep working on the SAME slot.
288+
try (Sender s2 = Sender.fromConfig(cfg)) {
289+
s2.table("m").symbol("s", "after-recovery").longColumn("v", 99).atNow();
290+
s2.flush();
291+
long deadline = System.currentTimeMillis() + 10_000;
292+
while (System.currentTimeMillis() < deadline && handler.maxDictSize() < 1) {
293+
Thread.sleep(20);
294+
}
295+
}
296+
}
297+
// The fully-acked slot was NOT quarantined: no set-aside copy exists (the
298+
// inverse of assertUnreplayableSlotSetAside), and the sender resumed on the
299+
// original slot.
300+
Assert.assertFalse("a fully-acked torn slot must NOT be quarantined -- its data "
301+
+ "was already delivered, so there is nothing to resend",
302+
java.nio.file.Files.isDirectory(Paths.get(sfDir, "default.unreplayable-0")));
303+
Assert.assertTrue("the sender must resume on the original slot",
304+
java.nio.file.Files.isDirectory(slot));
305+
});
306+
}
307+
252308
private static int countSegmentFiles(java.nio.file.Path dir) {
253309
java.io.File[] files = dir.toFile().listFiles();
254310
int n = 0;

0 commit comments

Comments
 (0)