Skip to content

Commit 2ee6e8f

Browse files
glasstigerclaude
andcommitted
Close two test-gate holes; report the third as not reproducible
C20. BackgroundDrainer's capabilityGapBudgetNanos overflow fix had no test: restoring the raw millis * 1_000_000L left all 38 drainer tests green. reconnect_max_duration_millis is validated only as > 0, and Long.MAX_VALUE is the natural way to ask for "never give up" -- but a raw multiply wraps it negative, and since the escalation gate ORs against the attempt cap, an elapsed of 0 on the very first sweep already clears a negative budget. The slot quarantines on sweep ONE, skipping the whole 16-sweep settle budget, so asking for more tolerance bought none. The new test asserts the full budget is spent; reverted it reports expected:<16> but was:<1>. The CursorWebSocketSendLoop twin of the same conversion was already pinned, which is what made this gap visible. C18. testScanFaultOnMapPastEofIsHandledAnyFilesystem nested every assertion in `if (ring != null)`, under a comment saying the null case was "also handled" -- so on a late-delivered fault, where SegmentRing skips the file and openExisting returns null, the test ran ZERO assertions and passed. It now writes a healthy sibling contiguous below the faulting segment, so the ring is non-null on every delivery path and the invariant can be asserted unconditionally: one bad .sfa must never take the slot down, and the skip must be per-FILE, with the sibling keeping all of its frames. Two limits are worth stating rather than papering over. MmapSegment's own in-class fault conversion still cannot be pinned on the shipping JDK 8 target -- delivery there is asynchronous, so a test cannot require the precise-delivery outcome. And the scanFrames CRC choice cannot be regression-tested at all: reverting it to the native Crc32c.update means a SIGBUS inside JNI, which aborts the JVM rather than failing a test. C19 is NOT fixed and its finding is downgraded. ensureConnected's `cursorSendLoop.close()` is genuinely unguarded, as the review proved, but it is no longer demonstrably load-bearing: the foreground loop now BORROWS its mirror rather than owning it, so an orphaned one holds almost nothing, and the catch-up frame buffer is allocated only once a catch-up is actually sent. A test written against an injected start() failure passed with the fix reverted. The window is also unreachable without a seam -- a null error handler is coerced to a default, the inbox capacity is validated at its setter, and with no reachable server ensureConnected fails before the loop is ever constructed. Rather than add production test-surface for a test that proves nothing, the seam and the test were both removed. The fix stays as defensive hygiene. Full client suite: 2698 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 52a9473 commit 2ee6e8f

3 files changed

Lines changed: 48 additions & 2 deletions

File tree

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -708,6 +708,32 @@ public void testRoleRejectAndCapabilityGapLandOnSeparateStreams() throws Excepti
708708
});
709709
}
710710

711+
@Test
712+
public void testSaturatingCapabilityGapBudgetDoesNotQuarantineOnTheFirstSweep() throws Exception {
713+
assertMemoryLeak(() -> {
714+
// reconnect_max_duration_millis is validated only as > 0, and Long.MAX_VALUE
715+
// is the natural way to ask for "never give up". A raw millis * 1_000_000L
716+
// wraps that NEGATIVE, and because the escalation gate is an OR against the
717+
// attempt cap, capabilityGapElapsedNanos (0 on the first sweep) already
718+
// clears a negative budget -- so the slot quarantines on sweep ONE, skipping
719+
// the whole 16-sweep settle budget. Asking for more tolerance bought none.
720+
//
721+
// The CursorWebSocketSendLoop twin of this conversion is pinned by
722+
// CursorWebSocketSendLoopZeroBackoffTest; this site had no test, despite the
723+
// commit that fixed it citing that one as prior art.
724+
int cap = BackgroundDrainer.DEFAULT_MAX_DURABLE_ACK_MISMATCH_ATTEMPTS;
725+
ScriptedFactory factory = ScriptedFactory.alwaysFailing(
726+
() -> new QwpDurableAckMismatchException("h", 1234, "primary"));
727+
BackgroundDrainer drainer = newDrainerWithBudgets(factory, Long.MAX_VALUE, 1L, 4L);
728+
WebSocketClient out = drainer.connectWithDurableAckRetry();
729+
assertNull(out);
730+
assertEquals(BackgroundDrainer.DrainOutcome.FAILED, drainer.outcome());
731+
assertEquals("a saturated budget must leave the attempt cap as the only gate, "
732+
+ "so the full settle budget is spent before quarantine",
733+
cap, factory.attempts());
734+
});
735+
}
736+
711737
@Test
712738
public void testTransportErrorResetsCapabilityGapEpisode() throws Exception {
713739
assertMemoryLeak(() -> {

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

Lines changed: 1 addition & 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.QwpWebSocketSender;
2930
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine;
3031
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorWebSocketSendLoop;
3132
import io.questdb.client.cutlass.qwp.client.sf.cursor.PersistedSymbolDict;

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

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -276,18 +276,37 @@ private void scanFaultOnMapPastEofBody() throws Exception {
276276
truncateTo(path, page);
277277
// Report twice the real length so openExisting maps a second,
278278
// beyond-EOF page; the scan faults reading it on any filesystem.
279+
// A HEALTHY sibling, contiguous below the faulting segment (baseSeq 0..4,
280+
// faulting one at 5). Without it the whole slot is one file, so a fault
281+
// converted to a per-file skip leaves openExisting returning null and the
282+
// assertions below -- nested in `if (ring != null)` -- ran ZERO times, under
283+
// a comment claiming that outcome was "also handled". Nothing was checked.
284+
// With the sibling the ring is non-null on EVERY delivery path, so the
285+
// recovery invariant can be asserted unconditionally: one bad file must
286+
// never take the slot down with it.
287+
writeSegment(tmpDir + "/seg-sibling.sfa", 0L, new int[]{8, 8, 8, 8, 8});
288+
279289
FilesFacade ff = new MapPastEofFacade(path, 2 * page);
280290
// Must not throw, on any JDK or JIT tier. Revert the fault handling
281291
// and a raw InternalError propagates out of here instead.
282292
try (SegmentRing ring = SegmentRing.openExisting(ff, tmpDir, SEGMENT_BYTES)) {
283-
if (ring != null) {
293+
assertNotNull("one faulting .sfa must never take down the whole slot", ring);
294+
assertNotNull("the healthy sibling must recover", ring.firstSealed() != null
295+
? ring.firstSealed() : ring.getActive());
296+
if (ring.getActive().baseSeq() == 5L) {
284297
// Interpreter / C1, or precise delivery on 21+: scanFrames'
285298
// own catch fired and the frame below the tear survived.
286299
assertEquals("the frame below the beyond-EOF page must be recovered",
287300
1L, ring.getActive().frameCount());
288301
assertEquals("scan must stop at the beyond-EOF boundary",
289302
page, ring.getActive().publishedOffset());
290-
} // else: the fault was converted to a per-file skip -- also handled.
303+
} else {
304+
// Late delivery: SegmentRing skipped the faulting file entirely, so
305+
// the sibling is all that is left -- and it must be intact.
306+
assertEquals("the skip must be per-FILE: the sibling keeps every frame",
307+
5L, ring.getActive().frameCount());
308+
assertEquals(0L, ring.getActive().baseSeq());
309+
}
291310
}
292311
});
293312
}

0 commit comments

Comments
 (0)