Skip to content

Commit 4f78df2

Browse files
puzpuzpuzclaude
andcommitted
test(qwp): portably regression-guard recovery's mmap-fault path (#64 M1)
The three MmapSegmentRecoveryFaultTest cases only fault on ZFS: on ext4/xfs a within-EOF sparse hole zero-fills, so the scan stops via the CRC-mismatch / bad-magic branch and every test stays green even with the recovery mmap-fault guard fully reverted -- zero fail-on-revert protection on the primary CI runners (PR #64 review, item M1). Proven by reverting both production files to the pre-branch base: 3/3 still passed on ext4. Add a portable seam and two tests that fault deterministically on any filesystem. A read of a mapped page beyond the file's real EOF raises SIGBUS everywhere (POSIX), which HotSpot translates to a catchable InternalError at an Unsafe intrinsic site -- the same fault an unbacked ZFS page raises. openExisting gains a FilesFacade overload (mirroring create) whose file length flows through the facade; a test facade reports a length larger than the real (truncated-down) file so the mapping extends past EOF. - FilesFacade.length(String) + DefaultFilesFacade delegate; openExisting(String) now delegates to openExisting(FilesFacade, String), which also routes openRW and close through the facade. - testScanFaultOnMapPastEofIsHandledAnyFilesystem and testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem fault on ext4 and fail on revert (verified: raw InternalError escapes when the guard is neutralized, while the three sparse-hole tests stay green -- exactly the M1 gap). The scan test accepts both handled outcomes: under the interpreter/C1 the fault is caught in scanFrames (graceful partial recovery), but once C2 inlines scanFrames into openExisting the async unsafe-access InternalError is delivered to openExisting's outer catch instead, converting the file to a skippable MmapSegmentException. Both are safe (no JVM abort, no raw error into recovery); a revert lets a raw InternalError through and fails the test either way. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 16df6f6 commit 4f78df2

5 files changed

Lines changed: 315 additions & 3 deletions

File tree

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

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -258,11 +258,31 @@ public static MmapSegment createInMemory(long baseSeq, long sizeBytes) {
258258
* last valid frame) do not log and report {@code 0}.
259259
*/
260260
public static MmapSegment openExisting(String path) {
261-
long fileSize = Files.length(path);
261+
return openExisting(FilesFacade.INSTANCE, path);
262+
}
263+
264+
/**
265+
* Facade-aware variant of {@link #openExisting(String)} that takes the file
266+
* length (and open/close) through a {@link FilesFacade} instead of straight
267+
* off {@link Files}. Production uses {@link FilesFacade#INSTANCE}; the seam
268+
* exists so recovery's mmap-fault guard can be regression-tested on any
269+
* filesystem.
270+
* <p>
271+
* The mapping is sized to {@code ff.length(path)} and every recovery read
272+
* runs straight out of it, so a facade that reports a length <em>larger</em>
273+
* than the real file makes the mapping extend past end-of-file. A read of a
274+
* page beyond real EOF raises SIGBUS on <em>every</em> filesystem — the same
275+
* fault an unbacked/sparse page raises on ZFS, but reproduced
276+
* deterministically on ext4/xfs, where a within-EOF hole would instead
277+
* zero-fill and never exercise the guard. See
278+
* {@link FilesFacade#length(String)}.
279+
*/
280+
public static MmapSegment openExisting(FilesFacade ff, String path) {
281+
long fileSize = ff.length(path);
262282
if (fileSize < HEADER_SIZE) {
263283
throw new MmapSegmentException("file shorter than header: " + path + " size=" + fileSize);
264284
}
265-
int fd = Files.openRW(path);
285+
int fd = ff.openRW(path);
266286
if (fd < 0) {
267287
throw new MmapSegmentException("openRW failed for " + path);
268288
}
@@ -309,7 +329,7 @@ public static MmapSegment openExisting(String path) {
309329
if (addr != Files.FAILED_MMAP_ADDRESS) {
310330
Files.munmap(addr, fileSize, MemoryTag.MMAP_DEFAULT);
311331
}
312-
Files.close(fd);
332+
ff.close(fd);
313333
// The header reads above (magic/version/baseSeq) run before
314334
// scanFrames and are otherwise unguarded. An unbacked page 0 --
315335
// an unflushed header on a truncate-based-allocate filesystem

core/src/main/java/io/questdb/client/std/DefaultFilesFacade.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,11 @@ public long length(int fd) {
9191
return Files.length(fd);
9292
}
9393

94+
@Override
95+
public long length(String path) {
96+
return Files.length(path);
97+
}
98+
9499
@Override
95100
public int lock(int fd) {
96101
return Files.lock(fd);

core/src/main/java/io/questdb/client/std/FilesFacade.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,22 @@ public interface FilesFacade {
8787

8888
long length(int fd);
8989

90+
/**
91+
* Stat length of the file at {@code path}, in bytes. Default delegates to
92+
* {@link Files#length(String)}.
93+
*
94+
* <p>Test injection point: {@code MmapSegment.openExisting} maps the file to
95+
* this length and scans straight out of the mapping, so a wrapping facade
96+
* that returns a value <em>larger</em> than the real file makes the mapping
97+
* extend past end-of-file. A read of a page beyond real EOF raises SIGBUS on
98+
* <em>every</em> filesystem (which HotSpot translates to a catchable
99+
* {@code InternalError} at an {@code Unsafe} intrinsic site) — the same fault
100+
* a genuinely unbacked/sparse page raises on ZFS, but reproduced
101+
* deterministically on ext4/xfs too. That is what lets recovery's mmap-fault
102+
* guard be regression-tested on any CI runner rather than only on ZFS.
103+
*/
104+
long length(String path);
105+
90106
int lock(int fd);
91107

92108
int mkdir(String path, int mode);

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

Lines changed: 266 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment;
2828
import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegmentException;
2929
import io.questdb.client.std.Files;
30+
import io.questdb.client.std.FilesFacade;
3031
import io.questdb.client.std.MemoryTag;
3132
import io.questdb.client.std.Unsafe;
3233
import io.questdb.client.test.tools.TestUtils;
@@ -70,6 +71,18 @@
7071
* the JVM. A hole-zero-filling filesystem (ext4) instead reads the hole back as
7172
* zeroes, which fails the frame CRC; either way recovery must stop at the same
7273
* boundary and recover the same frames.
74+
* <p>
75+
* <b>Fail-on-revert on any filesystem.</b> The sparse-hole tests above only
76+
* fault on ZFS: on ext4/xfs the within-EOF hole zero-fills, so the scan stops
77+
* via the CRC-mismatch / bad-magic branch and they stay green even with the
78+
* mmap-fault guard reverted -- no regression protection on the ext4/xfs CI
79+
* runners. The two {@code MapPastEof} tests below close that gap portably.
80+
* They truncate the file <em>down</em> (freeing the tail blocks) and hand
81+
* {@code openExisting} a {@link FilesFacade} that reports the original, larger
82+
* length, so the mapping extends past real end-of-file. A read of a page beyond
83+
* real EOF raises SIGBUS on <em>every</em> filesystem -- the same catchable
84+
* {@code InternalError} an unbacked ZFS page raises -- so they exercise the
85+
* real fault path (and fail on revert) on ext4/xfs too, not only on ZFS.
7386
*/
7487
public class MmapSegmentRecoveryFaultTest {
7588

@@ -187,6 +200,96 @@ public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception {
187200
});
188201
}
189202

203+
/**
204+
* Portable fail-on-revert guard for the recovery mmap-fault handling on the
205+
* scan path. Unlike {@link #testRecoveryKeepsFramesBeforeUnbackedTail}
206+
* (which only faults on ZFS), this maps the file past real EOF via the
207+
* length-injecting facade, so the scan's read of the beyond-EOF page faults
208+
* on ext4/xfs too. The fix must <em>recognize</em> that fault and keep
209+
* recovery safe -- never a JVM abort, never a raw {@code InternalError}
210+
* escaping into {@code SegmentRing}'s recovery loop. Revert the
211+
* {@code scanFrames}/{@code openExisting} mmap-fault guard (or fold the CRC
212+
* back through native {@code Crc32c}) and this errors or aborts the fork.
213+
* <p>
214+
* Two handled outcomes are accepted, because which one occurs depends on
215+
* whether the recovery methods are JIT-compiled at fault time:
216+
* <ul>
217+
* <li><b>Interpreter / C1:</b> {@code scanFrames}'s own
218+
* {@code catch (InternalError)} fires, so the frame below the tear is
219+
* recovered and a usable segment is returned.</li>
220+
* <li><b>C2:</b> once {@code scanFrames} is inlined into
221+
* {@code openExisting}, HotSpot delivers the async unsafe-access
222+
* {@code InternalError} to {@code openExisting}'s outer
223+
* {@code catch (Throwable)} instead of the inlined inner one, which
224+
* converts the file to a skippable {@link MmapSegmentException}.
225+
* Still fully handled -- {@code SegmentRing} skips just this
226+
* {@code .sfa} rather than aborting the slot.</li>
227+
* </ul>
228+
* (The C2 delivery imprecision is a property of HotSpot's async
229+
* unsafe-access fault handling, not of this seam; the seam only makes it
230+
* reproducible off ZFS.)
231+
*/
232+
@Test
233+
public void testScanFaultOnMapPastEofIsHandledAnyFilesystem() throws Exception {
234+
TestUtils.assertMemoryLeak(() -> {
235+
final String path = tmpDir + "/seg-mappasteof-scan.sfa";
236+
final long page = Files.PAGE_SIZE;
237+
// One frame that ends exactly on the first page boundary.
238+
final int payloadLen = (int) (page - MmapSegment.HEADER_SIZE - MmapSegment.FRAME_HEADER_SIZE);
239+
long boundary = writeSegment(path, 5L, new int[]{payloadLen});
240+
assertEquals("frame must fill exactly one page", page, boundary);
241+
// Free every block past the first page: the file is now exactly one
242+
// (fully backed) page, with nothing beyond it on disk.
243+
truncateTo(path, page);
244+
// Report twice the real length so openExisting maps a second,
245+
// beyond-EOF page; the scan faults reading it on any filesystem.
246+
FilesFacade ff = new MapPastEofFacade(path, 2 * page);
247+
try (MmapSegment seg = MmapSegment.openExisting(ff, path)) {
248+
// Interpreter / C1: graceful partial recovery.
249+
assertEquals("the frame below the beyond-EOF page must be recovered", 1L, seg.frameCount());
250+
assertEquals("scan must stop at the beyond-EOF boundary", page, seg.publishedOffset());
251+
assertEquals("a beyond-EOF page is not a torn write", 0L, seg.tornTailBytes());
252+
} catch (MmapSegmentException skippedUnderC2) {
253+
// C2: the inlined fault escaped to openExisting's outer catch and
254+
// was converted to a per-file skip. Assert it is the recognized
255+
// mmap fault (not some other data error) so a revert -- which
256+
// lets a raw InternalError through instead -- still fails here.
257+
assertTrue(skippedUnderC2.getMessage(),
258+
skippedUnderC2.getMessage().contains("unsafe memory access operation"));
259+
}
260+
});
261+
}
262+
263+
/**
264+
* Portable fail-on-revert guard for the {@code openExisting} header-block
265+
* guard. The file is truncated to empty and the facade reports a full page,
266+
* so the very first header read (magic) lands on a beyond-EOF page and
267+
* faults on any filesystem. {@code openExisting} must convert that to a
268+
* {@link MmapSegmentException} -- the per-file signal {@code SegmentRing}
269+
* skips on -- not let the raw {@code InternalError} escape and abort recovery
270+
* of every sibling. Revert the header-block conversion and this throws
271+
* {@code InternalError} instead of {@code MmapSegmentException}.
272+
*/
273+
@Test
274+
public void testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem() throws Exception {
275+
TestUtils.assertMemoryLeak(() -> {
276+
final String path = tmpDir + "/seg-mappasteof-header.sfa";
277+
final long page = Files.PAGE_SIZE;
278+
writeSegment(path, 9L, new int[]{64});
279+
// Free every block: the file is now empty, so even page 0 (the
280+
// header) is beyond EOF under the reported one-page mapping.
281+
truncateTo(path, 0L);
282+
FilesFacade ff = new MapPastEofFacade(path, page);
283+
try {
284+
MmapSegment.openExisting(ff, path).close();
285+
fail("expected MmapSegmentException for a beyond-EOF header page");
286+
} catch (MmapSegmentException expected) {
287+
// ok -- SegmentRing's per-file catch skips just this file
288+
// instead of aborting recovery of the whole slot.
289+
}
290+
});
291+
}
292+
190293
/**
191294
* Creates a segment at {@code path} and appends one frame per entry in
192295
* {@code payloadLens} (each filled with non-zero bytes so recovery can tell
@@ -231,4 +334,167 @@ private static void punchSparseTail(String path, long keepBytes) {
231334
Files.close(fd);
232335
}
233336
}
337+
338+
/**
339+
* Shrinks the file to {@code keepBytes}, freeing every block past it, and
340+
* leaves it there (no re-extend). Combined with a facade that reports a
341+
* larger length, the freed region becomes a beyond-EOF part of the mapping
342+
* that faults on read on any filesystem.
343+
*/
344+
private static void truncateTo(String path, long keepBytes) {
345+
int fd = Files.openRW(path);
346+
assertTrue("openRW failed", fd >= 0);
347+
try {
348+
assertTrue("truncate failed", Files.truncate(fd, keepBytes));
349+
} finally {
350+
Files.close(fd);
351+
}
352+
}
353+
354+
/**
355+
* A {@link FilesFacade} that reports an inflated stat length for one target
356+
* path so {@code openExisting} maps that file past end-of-file (see
357+
* {@link FilesFacade#length(String)}); every other call, including
358+
* {@code length} for any other path, delegates to the production
359+
* {@link FilesFacade#INSTANCE}.
360+
*/
361+
private static final class MapPastEofFacade implements FilesFacade {
362+
private final long reportedLength;
363+
private final String targetPath;
364+
365+
MapPastEofFacade(String targetPath, long reportedLength) {
366+
this.targetPath = targetPath;
367+
this.reportedLength = reportedLength;
368+
}
369+
370+
@Override
371+
public boolean allocate(int fd, long size) {
372+
return INSTANCE.allocate(fd, size);
373+
}
374+
375+
@Override
376+
public long allocNativePath(String path) {
377+
return INSTANCE.allocNativePath(path);
378+
}
379+
380+
@Override
381+
public int close(int fd) {
382+
return INSTANCE.close(fd);
383+
}
384+
385+
@Override
386+
public boolean exists(String path) {
387+
return INSTANCE.exists(path);
388+
}
389+
390+
@Override
391+
public void findClose(long findPtr) {
392+
INSTANCE.findClose(findPtr);
393+
}
394+
395+
@Override
396+
public long findFirst(String dir) {
397+
return INSTANCE.findFirst(dir);
398+
}
399+
400+
@Override
401+
public long findName(long findPtr) {
402+
return INSTANCE.findName(findPtr);
403+
}
404+
405+
@Override
406+
public int findNext(long findPtr) {
407+
return INSTANCE.findNext(findPtr);
408+
}
409+
410+
@Override
411+
public int findType(long findPtr) {
412+
return INSTANCE.findType(findPtr);
413+
}
414+
415+
@Override
416+
public void freeNativePath(long pathPtr) {
417+
INSTANCE.freeNativePath(pathPtr);
418+
}
419+
420+
@Override
421+
public int fsync(int fd) {
422+
return INSTANCE.fsync(fd);
423+
}
424+
425+
@Override
426+
public long length(int fd) {
427+
return INSTANCE.length(fd);
428+
}
429+
430+
@Override
431+
public long length(long pathPtr) {
432+
return INSTANCE.length(pathPtr);
433+
}
434+
435+
@Override
436+
public long length(String path) {
437+
return targetPath.equals(path) ? reportedLength : INSTANCE.length(path);
438+
}
439+
440+
@Override
441+
public int lock(int fd) {
442+
return INSTANCE.lock(fd);
443+
}
444+
445+
@Override
446+
public int mkdir(String path, int mode) {
447+
return INSTANCE.mkdir(path, mode);
448+
}
449+
450+
@Override
451+
public int openCleanRW(String path) {
452+
return INSTANCE.openCleanRW(path);
453+
}
454+
455+
@Override
456+
public int openCleanRW(long pathPtr) {
457+
return INSTANCE.openCleanRW(pathPtr);
458+
}
459+
460+
@Override
461+
public int openRW(String path) {
462+
return INSTANCE.openRW(path);
463+
}
464+
465+
@Override
466+
public int openRW(long pathPtr) {
467+
return INSTANCE.openRW(pathPtr);
468+
}
469+
470+
@Override
471+
public long read(int fd, long addr, long len, long offset) {
472+
return INSTANCE.read(fd, addr, len, offset);
473+
}
474+
475+
@Override
476+
public boolean remove(String path) {
477+
return INSTANCE.remove(path);
478+
}
479+
480+
@Override
481+
public boolean remove(long pathPtr) {
482+
return INSTANCE.remove(pathPtr);
483+
}
484+
485+
@Override
486+
public int rename(String oldPath, String newPath) {
487+
return INSTANCE.rename(oldPath, newPath);
488+
}
489+
490+
@Override
491+
public boolean truncate(int fd, long size) {
492+
return INSTANCE.truncate(fd, size);
493+
}
494+
495+
@Override
496+
public long write(int fd, long addr, long len, long offset) {
497+
return INSTANCE.write(fd, addr, len, offset);
498+
}
499+
}
234500
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,11 @@ public long length(int fd) {
563563
return INSTANCE.length(fd);
564564
}
565565

566+
@Override
567+
public long length(String path) {
568+
return INSTANCE.length(path);
569+
}
570+
566571
@Override
567572
public int lock(int fd) {
568573
return INSTANCE.lock(fd);

0 commit comments

Comments
 (0)