Skip to content

Commit 1cb4c65

Browse files
mtopolnikclaude
andcommitted
Add multi-window scan test, reword comments
The recovery boundary suite only covered files that fit in a single pread window, leaving the scan's window repositioning and the chunk-by-chunk CRC verification untested. The new test recovers a 4 MiB segment holding a 3 MiB frame followed by a small one: checking the large frame reloads the window four times, feeding each chunk into the running CRC, and the small frame is then verified out of a repositioned window. Recovery must be total -- both frames kept, the cursor at the last appended byte, no torn tail. A mutation check confirmed the test has teeth: reseeding the CRC on each window reload fails exactly this test while the rest of the suite stays green. writeSegment gains an explicit-segment-size variant to build the oversized file. The comments in MmapSegment and the test suite also drop the "fold the CRC" phrasing: the scan computes the CRC incrementally, feeding each window-sized chunk into the running value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent ac8166d commit 1cb4c65

2 files changed

Lines changed: 60 additions & 10 deletions

File tree

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,8 @@ public final class MmapSegment implements QuietCloseable {
6767
private static final Logger LOG = LoggerFactory.getLogger(MmapSegment.class);
6868
// Recovery reads the file through a pread window of this size (or the
6969
// whole file when smaller). Sized so a typical segment scans in a handful
70-
// of preads; frames larger than the window CRC-fold across refills.
70+
// of preads; the scan checksums a frame larger than the window in
71+
// window-sized chunks.
7172
private static final long RECOVERY_BUF_BYTES = 1L << 20;
7273

7374
private final String path;
@@ -591,10 +592,11 @@ private static RecoveryScan scanFile(FilesFacade ff, int fd, String path, long f
591592
if (payloadLen < 0 || pos + FRAME_HEADER_SIZE + payloadLen > fileSize) {
592593
break;
593594
}
594-
// CRC over the (payloadLen, payload) pair, folded window by
595-
// window for frames larger than the buffer. Chaining
596-
// Crc32c.update calls is bit-identical to one call over the
597-
// contiguous range (tryAppend writes the CRC the same way).
595+
// CRC over the (payloadLen, payload) pair, computed
596+
// incrementally: each Crc32c.update call feeds the next
597+
// window-sized chunk into the running value, which is
598+
// bit-identical to one call over the contiguous range
599+
// (tryAppend writes the CRC the same way).
598600
long crcPos = pos + 4;
599601
long remaining = 4L + payloadLen;
600602
int crc = Crc32c.INIT;

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

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,10 @@
6060
* point under a JIT-compiled caller — the JDK 8 CI flake this suite guards
6161
* against reintroducing.)
6262
* <p>
63+
* The suite also guards the fully-readable side of the same machinery: a file
64+
* larger than the scan's pread window, whose recovery slides the window
65+
* forward and checks one frame's CRC in chunks spanning several window loads.
66+
* <p>
6367
* These tests drive the production entry point ({@code openExisting}), not
6468
* private scan methods via reflection, so they exercise the real recovery
6569
* path end to end.
@@ -114,10 +118,11 @@ public void testRecoveryKeepsFramesBeforeUnbackedTail() throws Exception {
114118
* The harder case: a frame whose 8-byte header sits on a backed page but
115119
* whose payload reaches into the unbacked hole (a torn write leaves a real
116120
* positive {@code payloadLen} with the payload spanning the boundary). The
117-
* CRC therefore folds across the backed-to-hole edge: the backed bytes are
118-
* real, the hole preads as zeros, the CRC mismatches, and recovery rejects
119-
* that frame while keeping the one below it. The surviving non-zero header
120-
* bytes at the bail-out position are flagged as a torn tail.
121+
* CRC check therefore reads bytes on both sides of the backed-to-hole
122+
* edge: the backed bytes are real, the hole preads as zeros, the CRC
123+
* mismatches, and recovery rejects that frame while keeping the one
124+
* below it. The surviving non-zero header bytes at the bail-out position
125+
* are flagged as a torn tail.
121126
*/
122127
@Test
123128
public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception {
@@ -260,13 +265,56 @@ public void testReadErrorDuringScanEndsRecoveryAtBoundary() throws Exception {
260265
});
261266
}
262267

268+
/**
269+
* The multi-window scan: a segment larger than the recovery read window
270+
* (1 MiB), holding a frame whose CRC span -- the (payloadLen, payload)
271+
* pair -- is itself larger than the window, followed by a small frame.
272+
* Verifying the large frame forces the scan to compute its CRC
273+
* incrementally -- reloading the window several times and feeding each
274+
* chunk into the running value, which chained {@code Crc32c.update} calls
275+
* make bit-identical to {@code tryAppend}'s one-pass CRC -- and the small
276+
* frame is then verified out of a repositioned window. Every byte is
277+
* readable, so recovery must be total: both frames kept, the cursor at
278+
* the last appended byte, no torn tail.
279+
*/
280+
@Test
281+
public void testScanRecoversFrameLargerThanReadWindow() throws Exception {
282+
TestUtils.assertMemoryLeak(() -> {
283+
final String path = tmpDir + "/seg-multi-window.sfa";
284+
// 3 MiB of payload in a 4 MiB segment: checking the large frame's
285+
// CRC reloads the window four times, and the scan as a whole
286+
// takes multiple preads. (Sized against
287+
// MmapSegment.RECOVERY_BUF_BYTES = 1 MiB; keep the payload larger
288+
// than that constant or this test degrades to a single-window
289+
// scan.)
290+
final int largeLen = 3 * (1 << 20);
291+
final long segmentBytes = 4L * (1 << 20);
292+
long used = writeSegment(path, 17L, new int[]{largeLen, 64}, segmentBytes);
293+
294+
try (MmapSegment seg = MmapSegment.openExisting(path)) {
295+
assertEquals("both frames must be recovered", 2L, seg.frameCount());
296+
assertEquals("scan must recover up to the last appended byte", used, seg.publishedOffset());
297+
assertEquals("a fully recovered segment has no torn tail", 0L, seg.tornTailBytes());
298+
}
299+
});
300+
}
301+
263302
/**
264303
* Creates a segment at {@code path} and appends one frame per entry in
265304
* {@code payloadLens} (each filled with non-zero bytes so recovery can tell
266305
* written data from an unwritten/zeroed hole). Returns the used byte count
267306
* (the published offset after the last append).
268307
*/
269308
private static long writeSegment(String path, long baseSeq, int[] payloadLens) {
309+
return writeSegment(path, baseSeq, payloadLens, SEGMENT_BYTES);
310+
}
311+
312+
/**
313+
* Variant of {@link #writeSegment(String, long, int[])} with an explicit
314+
* segment size, for tests whose file must exceed the recovery scan's
315+
* 1 MiB pread window.
316+
*/
317+
private static long writeSegment(String path, long baseSeq, int[] payloadLens, long segmentBytes) {
270318
int maxLen = 0;
271319
for (int len : payloadLens) {
272320
maxLen = Math.max(maxLen, len);
@@ -276,7 +324,7 @@ private static long writeSegment(String path, long baseSeq, int[] payloadLens) {
276324
for (int i = 0; i < maxLen; i++) {
277325
Unsafe.getUnsafe().putByte(buf + i, (byte) (i | 1)); // all non-zero
278326
}
279-
try (MmapSegment seg = MmapSegment.create(path, baseSeq, SEGMENT_BYTES)) {
327+
try (MmapSegment seg = MmapSegment.create(path, baseSeq, segmentBytes)) {
280328
for (int len : payloadLens) {
281329
assertTrue("append must fit", seg.tryAppend(buf, len) >= 0);
282330
}

0 commit comments

Comments
 (0)