From 0a345a16a84fa5e548321eb7a14f47ee910d60ba Mon Sep 17 00:00:00 2001 From: Marko Topolnik Date: Mon, 20 Jul 2026 16:33:48 +0200 Subject: [PATCH 1/5] fix(qwp): read the SF recovery scan via pread instead of the mapping Recovery-time reads of a segment file go through pread into a private buffer: the header block, the frame scan, the CRC fold, and the torn-tail probe. Unreadable regions are then ordinary read results with one deterministic outcome per input on every filesystem, JDK, and JIT state: a sparse hole reads back as zeros and fails the frame CRC, a region past real end-of-file gives a short read, and a media error gives a failed read -- each is the boundary of recoverable data. The mapping is created only after the scan, for the append path, whose writes materialize pages rather than faulting. Reading the same regions through the mapping raises SIGBUS, which HotSpot converts to an InternalError delivered at an imprecise point under a JIT-compiled caller -- not reliably catchable by any try/catch placement, which is the MmapSegmentRecoveryFaultTest flake on the JDK 8 CI (#69). With no fault to catch, the catchable-InternalError machinery (isMmapAccessFault, the table-CRC fallback, the per-method catch arms) is gone, and the frame CRC uses the native Crc32c again -- safe on a process-private buffer. The scan is one pass through a sliding 1 MiB pread window (header fields, lastGood, frame count, torn-tail probe together); frames larger than the window CRC-fold across refills. The recovery tests become single-outcome and gain a bad-sector case via the facade's read seam (renamed MmapSegmentRecoveryBoundaryTest -- there is no fault delivery left to branch on). Fixes #69. Co-Authored-By: Claude Fable 5 --- .../qwp/client/sf/cursor/MmapSegment.java | 451 +++++++++--------- .../qwp/client/sf/cursor/SegmentRing.java | 7 +- .../io/questdb/client/std/FilesFacade.java | 16 +- ...a => MmapSegmentRecoveryBoundaryTest.java} | 253 +++++----- .../qwp/client/sf/cursor/MmapSegmentTest.java | 4 +- .../client/sf/cursor/PrReviewRedTests.java | 6 +- 6 files changed, 347 insertions(+), 390 deletions(-) rename core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/{MmapSegmentRecoveryFaultTest.java => MmapSegmentRecoveryBoundaryTest.java} (57%) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java index 78e5db9f..13f3a618 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java @@ -64,8 +64,11 @@ public final class MmapSegment implements QuietCloseable { public static final int FRAME_HEADER_SIZE = 8; // u32 crc + u32 payloadLen public static final int HEADER_SIZE = 24; public static final byte VERSION = 1; - private static final int[] CRC32C_TABLE = buildCrc32cTable(); private static final Logger LOG = LoggerFactory.getLogger(MmapSegment.class); + // Recovery reads the file through a pread window of this size (or the + // whole file when smaller). Sized so a typical segment scans in a handful + // of preads; frames larger than the window CRC-fold across refills. + private static final long RECOVERY_BUF_BYTES = 1L << 20; private final String path; private final long sizeBytes; @@ -242,12 +245,14 @@ public static MmapSegment createInMemory(long baseSeq, long sizeBytes) { } /** - * Opens an existing segment file for recovery. mmaps it RW, validates the - * header magic / version, then scans frames forward verifying each CRC. - * The first bad CRC (or a frame whose declared length runs past the file - * end) is treated as a torn tail; both cursors are positioned at the - * start of that frame. Returns the segment ready for further appends. - * Throws {@link MmapSegmentException} on header validation failure. + * Opens an existing segment file for recovery. Validates the header magic + * / version and scans frames forward verifying each CRC — all through + * {@code pread} into a private buffer — then mmaps the file RW for further + * appends. The first bad CRC (or a frame whose declared length runs past + * the file end, or an unreadable region) is treated as the boundary of + * recoverable data; both cursors are positioned at the start of that + * frame. Returns the segment ready for further appends. Throws + * {@link MmapSegmentException} on header validation failure. *

* If recovery observes a torn tail (the bytes at the bail-out position * are non-zero, indicating an attempted-but-failed frame write rather @@ -263,19 +268,24 @@ public static MmapSegment openExisting(String path) { /** * Facade-aware variant of {@link #openExisting(String)} that takes the file - * length (and open/close) through a {@link FilesFacade} instead of straight - * off {@link Files}. Production uses {@link FilesFacade#INSTANCE}; the seam - * exists so recovery's mmap-fault guard can be regression-tested on any - * filesystem. + * length, open/close, and every recovery read through a {@link FilesFacade} + * instead of straight off {@link Files}. Production uses + * {@link FilesFacade#INSTANCE}; the seam exists so recovery's handling of + * unreadable regions (short reads past a truncation, I/O errors from bad + * sectors) can be regression-tested on any filesystem. *

- * The mapping is sized to {@code ff.length(path)} and every recovery read - * runs straight out of it, so a facade that reports a length larger - * than the real file makes the mapping extend past end-of-file. A read of a - * page beyond real EOF raises SIGBUS on every filesystem — the same - * fault an unbacked/sparse page raises on ZFS, but reproduced - * deterministically on ext4/xfs, where a within-EOF hole would instead - * zero-fill and never exercise the guard. See - * {@link FilesFacade#length(String)}. + * Every recovery-time read goes through {@code pread} + * ({@link FilesFacade#read(int, long, long, long)}) into a private buffer, + * never through the mapping. This keeps recovery free of SIGBUS: a sparse + * hole reads back as zeros (which fails the frame CRC and ends the scan at + * that boundary), a region past real end-of-file gives a short read, and a + * media error gives a failed read — all three are ordinary return values + * with one deterministic outcome per input on every filesystem, JDK, and + * JIT state. Reading the same regions through the mapping instead would + * turn them into SIGBUS, which HotSpot converts to an {@code InternalError} + * whose delivery point under a JIT-compiled caller is imprecise — not + * reliably catchable by any {@code try/catch} placement. The mapping is + * created only after the scan completes, for the append path. */ public static MmapSegment openExisting(FilesFacade ff, String path) { long fileSize = ff.length(path); @@ -286,67 +296,24 @@ public static MmapSegment openExisting(FilesFacade ff, String path) { if (fd < 0) { throw new MmapSegmentException("openRW failed for " + path); } - long addr = Files.FAILED_MMAP_ADDRESS; try { - addr = Files.mmap(fd, fileSize, 0, Files.MAP_RW, MemoryTag.MMAP_DEFAULT); - if (addr == Files.FAILED_MMAP_ADDRESS) { - throw new MmapSegmentException("mmap failed for " + path); - } - int magic = Unsafe.getUnsafe().getInt(addr); - if (magic != FILE_MAGIC) { - throw new MmapSegmentException( - "bad magic in " + path + ": 0x" + Integer.toHexString(magic)); - } - byte version = Unsafe.getUnsafe().getByte(addr + 4); - if (version != VERSION) { - throw new MmapSegmentException("unsupported version in " + path + ": " + version); - } - long baseSeq = Unsafe.getUnsafe().getLong(addr + 8); - // FSNs are non-negative by construction (see SegmentRing). - // A negative baseSeq on disk means bit-rot or a malicious file — - // refuse the segment so SegmentRing.openExisting's narrow catch - // skips it like any other unreadable .sfa rather than feeding - // the bad value into Long.compareUnsigned-based contiguity - // checks (which would place the segment last in baseSeq order - // and trip the FSN-gap throw, taking the whole recovery down). - if (baseSeq < 0L) { - throw new MmapSegmentException( - "bad baseSeq in " + path + ": " + baseSeq); - } - long lastGood = scanFrames(addr, fileSize); - long count = countFrames(addr, lastGood); - long tornTail = detectTornTail(addr, lastGood, fileSize); - if (tornTail > 0) { + RecoveryScan scan = scanFile(ff, fd, path, fileSize); + if (scan.tornTailBytes > 0) { LOG.warn("SF segment {}: torn tail of {} bytes at offset {} " + "(file size {}, frames recovered {}). " + "Recovery will overwrite this region on next append; " + "frames past the tear (if any) are discarded. " + "Investigate disk health or unexpected writer crash.", - path, tornTail, lastGood, fileSize, count); + path, scan.tornTailBytes, scan.lastGood, fileSize, scan.frameCount); } - return new MmapSegment(path, fd, addr, fileSize, baseSeq, lastGood, count, false, tornTail); - } catch (Throwable t) { - if (addr != Files.FAILED_MMAP_ADDRESS) { - Files.munmap(addr, fileSize, MemoryTag.MMAP_DEFAULT); + long addr = Files.mmap(fd, fileSize, 0, Files.MAP_RW, MemoryTag.MMAP_DEFAULT); + if (addr == Files.FAILED_MMAP_ADDRESS) { + throw new MmapSegmentException("mmap failed for " + path); } + return new MmapSegment(path, fd, addr, fileSize, scan.baseSeq, + scan.lastGood, scan.frameCount, false, scan.tornTailBytes); + } catch (Throwable t) { ff.close(fd); - // The header reads above (magic/version/baseSeq) run before - // scanFrames and are otherwise unguarded. An unbacked page 0 -- - // an unflushed header on a truncate-based-allocate filesystem - // after a crash (create() does not msync), or a file truncated - // under the mapping -- faults at the Unsafe intrinsic site as a - // catchable InternalError. Convert it to a MmapSegmentException so - // SegmentRing's per-file catch skips just this .sfa, instead of - // letting the raw InternalError escape to SegmentRing's outer catch - // and abort recovery of every sibling segment. scanFrames and - // detectTornTail already handle their own in-mapping faults; this - // covers the header block and any future reader placed ahead of the - // scan. - if (isMmapAccessFault(t)) { - throw new MmapSegmentException( - "unreadable mapped header page in " + path - + " (unbacked/sparse page 0): " + t.getMessage(), t); - } throw t; } } @@ -541,201 +508,213 @@ public long frameCount() { * truncation (corruption) from a normal partial fill (no incident). *

* One case this does NOT count: when the scan stops because the bail-out - * region is itself an unbacked mapped page (an in-mapping fault, not a - * backed torn header), that region cannot be probed, so this returns - * {@code 0} even though frames may have been discarded. That outcome is - * surfaced by the {@code WARN} in {@link #scanFrames} instead -- see it for - * the benign-tail-vs-media-error caveat. + * region is itself unreadable (short read past a truncation, or an I/O + * error), that region cannot be probed, so this returns {@code 0} even + * though frames may have been discarded. That outcome is surfaced by the + * {@code WARN} in {@link #scanFile} instead -- see it for the + * truncation-vs-media-error caveat. */ public long tornTailBytes() { return tornTailBytes; } /** - * True when {@code t} is the JVM's recoverable signal for a fault while - * accessing a memory-mapped region -- a SIGBUS/SIGSEGV that HotSpot - * translates into an {@code InternalError} at an {@code Unsafe} intrinsic - * site instead of aborting the process. It surfaces when a mapped page is - * not backed by real file blocks: a sparse {@code .sfa} tail on a - * filesystem whose pre-allocation leaves holes (e.g. ZFS, where a - * truncate-based {@code allocate} does not materialize blocks), or a file - * truncated under the mapping. Recovery treats this as an I/O boundary -- - * the same way MappedByteBuffer readers do -- not a fatal VM error. + * One-pass recovery scan of the file via {@code pread}: validates the + * header, walks frames verifying each CRC, and probes the bail-out region + * for a torn tail. Returns the header fields plus {@code lastGood} (offset + * just past the last CRC-verified frame), the frame count, and the + * torn-tail byte count. *

- * The message is matched on the fragment {@code "unsafe memory access - * operation"}, which is common to both HotSpot wordings and NOT - * version-stable as a whole: pre-21 JDKs (including the shipping/CI JDK 8) - * emit {@code "a fault occurred in a recent unsafe memory access operation - * in compiled Java code"}, while JDK 21+ shortened it to {@code "a fault - * occurred in an unsafe memory access operation"}. Matching the shared - * fragment keeps the guard effective on JDK 8/11/17 as well as 21+, while - * still being specific enough that a genuine VirtualMachineError (real OOM, - * StackOverflow) is never swallowed. + * Reading through {@code pread} instead of the mapping keeps recovery free + * of SIGBUS by construction: a sparse hole reads back as zeros and fails + * the frame CRC, a region past real end-of-file gives a short read, and a + * media error gives a failed read. Each is an ordinary return value with a + * single deterministic outcome — the boundary of recoverable data — on + * every filesystem, JDK, and JIT state. The scan is cold (recovery only), + * so the extra copy over mapped access is immaterial; frame CRCs still go + * through the native {@link Crc32c}, which is safe here because the buffer + * is process-private malloc'd memory, never a mapped page. */ - private static boolean isMmapAccessFault(Throwable t) { - if (!(t instanceof InternalError)) { - return false; - } - String msg = t.getMessage(); - return msg != null && msg.contains("unsafe memory access operation"); - } - - /** - * Forward scan that returns the offset just past the last frame whose - * CRC verifies. A torn-tail frame (declared length runs past EOF, or - * CRC mismatch) leaves both cursors at the start of that frame; the - * next {@link #tryAppend} will overwrite it. The scan only reads from - * the mapping — no syscalls. - */ - private static long scanFrames(long addr, long fileSize) { - long pos = HEADER_SIZE; + private static RecoveryScan scanFile(FilesFacade ff, int fd, String path, long fileSize) { + long bufSize = Math.min(fileSize, RECOVERY_BUF_BYTES); + long buf = Unsafe.malloc(bufSize, MemoryTag.NATIVE_DEFAULT); try { + PreadWindow w = new PreadWindow(ff, fd, buf, bufSize, fileSize); + if (!w.require(0, HEADER_SIZE)) { + throw new MmapSegmentException( + "unreadable header in " + path + " (short read or I/O error at offset 0)"); + } + int magic = Unsafe.getUnsafe().getInt(w.addrOf(0)); + if (magic != FILE_MAGIC) { + throw new MmapSegmentException( + "bad magic in " + path + ": 0x" + Integer.toHexString(magic)); + } + byte version = Unsafe.getUnsafe().getByte(w.addrOf(4)); + if (version != VERSION) { + throw new MmapSegmentException("unsupported version in " + path + ": " + version); + } + long baseSeq = Unsafe.getUnsafe().getLong(w.addrOf(8)); + // FSNs are non-negative by construction (see SegmentRing). + // A negative baseSeq on disk means bit-rot or a malicious file — + // refuse the segment so SegmentRing.openExisting's narrow catch + // skips it like any other unreadable .sfa rather than feeding + // the bad value into Long.compareUnsigned-based contiguity + // checks (which would place the segment last in baseSeq order + // and trip the FSN-gap throw, taking the whole recovery down). + if (baseSeq < 0L) { + throw new MmapSegmentException( + "bad baseSeq in " + path + ": " + baseSeq); + } + + // Forward scan: lastGood ends up just past the last frame whose + // CRC verifies. A torn-tail frame (declared length runs past the + // file end, CRC mismatch, or an unreadable region) leaves both + // cursors at the start of that frame; the next tryAppend will + // overwrite it. + long pos = HEADER_SIZE; + long count = 0; + boolean unreadable = false; + scan: while (pos + FRAME_HEADER_SIZE <= fileSize) { - int crcRead = Unsafe.getUnsafe().getInt(addr + pos); - int payloadLen = Unsafe.getUnsafe().getInt(addr + pos + 4); + if (!w.require(pos, FRAME_HEADER_SIZE)) { + unreadable = true; + break; + } + int crcRead = Unsafe.getUnsafe().getInt(w.addrOf(pos)); + int payloadLen = Unsafe.getUnsafe().getInt(w.addrOf(pos + 4)); // Defensive: a corrupt length field could be enormous or negative, - // both of which would otherwise overrun the mapping. + // both of which would otherwise overrun the file bounds. if (payloadLen < 0 || pos + FRAME_HEADER_SIZE + payloadLen > fileSize) { - return pos; + break; } - // CRC over the contiguous (payloadLen, payload) pair, folded - // via Unsafe reads rather than the native Crc32c.update. - // Recovery maps to the file's stat length, but a page inside - // that range can be unbacked: a sparse pre-allocation tail (a - // truncate-based allocate that never materialized blocks, as on - // ZFS), or -- via a torn write, since tryAppend writes the - // length field before copying the payload -- a real positive - // payloadLen whose payload spans into an unwritten hole. A raw - // read of an unbacked page raises SIGBUS; HotSpot translates - // that into a catchable InternalError ONLY at an Unsafe - // intrinsic site, NEVER inside JNI native code, so a native - // Crc32c.update over such a page aborts the whole JVM (and, - // empirically on pre-21 JDKs, a preceding Unsafe pre-touch does - // not reliably fault first once an earlier native CRC ran in - // the same scan). Folding over Unsafe keeps every fault - // catchable -- handled below as the boundary of recoverable - // data; a page that instead reads back as zeroes just fails the - // CRC check and ends the scan. Recovery is cold, so the slower - // table CRC here is immaterial. - int crcCalc = crc32cRecovery(addr + pos + 4, 4L + payloadLen); - if (crcCalc != crcRead) { - return pos; + // CRC over the (payloadLen, payload) pair, folded window by + // window for frames larger than the buffer. Chaining + // Crc32c.update calls is bit-identical to one call over the + // contiguous range (tryAppend writes the CRC the same way). + long crcPos = pos + 4; + long remaining = 4L + payloadLen; + int crc = Crc32c.INIT; + while (remaining > 0) { + int m = (int) Math.min(remaining, bufSize); + if (!w.require(crcPos, m)) { + unreadable = true; + break scan; + } + crc = Crc32c.update(crc, w.addrOf(crcPos), m); + crcPos += m; + remaining -= m; + } + if (crc != crcRead) { + break; } pos += FRAME_HEADER_SIZE + payloadLen; + count++; + } + if (unreadable) { + LOG.warn("SF segment recovery: unreadable region at offset {} (file size {}); " + + "treating it as the end of recoverable data -- any frames beyond this " + + "offset are discarded. The usual causes are a file shorter than its " + + "recorded length (truncated under recovery, or size metadata that " + + "survived a crash its data blocks did not) or a media error (bad " + + "sector); check disk health if this segment was expected to be fully " + + "written or if this recurs.", + pos, fileSize); } - } catch (InternalError e) { - // The read at `pos` hit a mapped page that is not backed by real - // file blocks: the JVM translates the underlying SIGBUS into a - // recoverable InternalError instead of aborting the process. This - // happens when a prior session left a sparse segment tail (a - // truncate-based pre-allocation that does not materialize blocks, - // as on ZFS) or the file was truncated under the mapping. Every - // frame below `pos` already verified; treat the unreadable region - // exactly like unwritten space or a torn tail -- the boundary of - // recoverable data -- rather than letting the error abort recovery - // of the whole slot. Anything that is not the documented mmap - // access fault is a genuine VM error, so rethrow it. - if (!isMmapAccessFault(e)) { - throw e; + + // Torn-tail probe: distinguishes "writer attempted a write past the + // last valid frame and failed" (partial write, mid-stream + // corruption, bit rot) from clean unwritten space. create() + // truncates the file to size, leaving the tail zero-filled, and the + // writer only writes non-zero bytes via tryAppend (CRC and length + // together) — so a non-zero byte at the failed-frame position + // implies an attempted write, exactly the case operators want + // flagged. An unreadable probe region was never written, so it is + // clean unwritten space, not a torn write. + long torn = 0L; + if (pos < fileSize && !unreadable) { + int probe = (int) Math.min(FRAME_HEADER_SIZE, fileSize - pos); + if (w.require(pos, probe)) { + for (int i = 0; i < probe; i++) { + if (Unsafe.getUnsafe().getByte(w.addrOf(pos + i)) != 0) { + torn = fileSize - pos; + break; + } + } + } } - LOG.warn("SF segment recovery: unreadable mapped page at offset {} (file size {}); " - + "treating it as the end of recoverable data -- any frames beyond this " - + "offset are discarded. The usual cause is a benign sparse pre-allocation " - + "tail (e.g. a truncate-based allocate on ZFS) left by a prior session, " - + "but a mid-file media error (bad sector) is indistinguishable here; " - + "check disk health if this segment was expected to be fully written or " - + "if this recurs.", - pos, fileSize); + return new RecoveryScan(baseSeq, pos, count, torn); + } finally { + Unsafe.free(buf, bufSize, MemoryTag.NATIVE_DEFAULT); } - return pos; } - /** - * CRC-32C (Castagnoli) of {@code [addr, addr + len)} read through - * {@link Unsafe}, seeded like {@code Crc32c.update(Crc32c.INIT, addr, len)} - * and bit-identical to it (verified) -- but every byte load is an Unsafe - * intrinsic, so a fault on an unbacked mapped page is a catchable - * {@link InternalError} instead of the uncatchable JNI SIGBUS the native - * {@link Crc32c} would raise. Byte-at-a-time via a precomputed table - * ({@code ~0.5 GiB/s}); used only on the cold recovery scan, never on the - * append hot path (which stays on the native, hardware-friendly path). - */ - private static int crc32cRecovery(long addr, long len) { - int crc = ~Crc32c.INIT; - for (long i = 0; i < len; i++) { - crc = (crc >>> 8) ^ CRC32C_TABLE[(crc ^ Unsafe.getUnsafe().getByte(addr + i)) & 0xFF]; + /** Result of {@link #scanFile}: header fields plus scan outcomes. */ + private static final class RecoveryScan { + final long baseSeq; + final long frameCount; + final long lastGood; + final long tornTailBytes; + + RecoveryScan(long baseSeq, long lastGood, long frameCount, long tornTailBytes) { + this.baseSeq = baseSeq; + this.lastGood = lastGood; + this.frameCount = frameCount; + this.tornTailBytes = tornTailBytes; } - return ~crc; } /** - * Standard reflected CRC-32C byte table (polynomial {@code 0x82F63B78}), - * matching {@code crc32c_table[0]} in the native {@code crc32c.c}. Computed - * at class init to avoid 256 hand-transcribed literals; drives - * {@link #crc32cRecovery}. + * Sliding {@code pread} window over one file for the recovery scan. + * {@link #require} repositions the window when the requested span is not + * already buffered; {@link #addrOf} translates a file offset to the + * buffered copy's address. A span is unavailable — {@code require} returns + * {@code false} — when the file ends short of it or a read fails; the + * caller treats both as the boundary of recoverable data. */ - private static int[] buildCrc32cTable() { - int[] table = new int[256]; - for (int n = 0; n < 256; n++) { - int c = n; - for (int k = 0; k < 8; k++) { - c = (c & 1) != 0 ? (0x82F63B78 ^ (c >>> 1)) : (c >>> 1); - } - table[n] = c; + private static final class PreadWindow { + private final long bufAddr; + private final long bufSize; + private final int fd; + private final FilesFacade ff; + private final long fileSize; + private long winLen; + private long winOff; + + PreadWindow(FilesFacade ff, int fd, long bufAddr, long bufSize, long fileSize) { + this.ff = ff; + this.fd = fd; + this.bufAddr = bufAddr; + this.bufSize = bufSize; + this.fileSize = fileSize; } - return table; - } - /** - * Distinguishes "torn tail" (writer attempted a write past the last valid - * frame and failed — partial write, mid-stream corruption, bit rot) from - * clean unwritten space (manager-allocated segment with zero-filled tail). - * Returns the byte count from {@code lastGood} to {@code fileSize} when - * the bytes at the bail-out frame header are non-zero, else {@code 0}. - *

- * Heuristic but robust for the common cases: {@link #create} truncates the - * file to size, leaving the tail zero-filled; the writer only writes - * non-zero bytes via {@link #tryAppend}, which writes the CRC and length - * fields together. So a non-zero byte at the failed-frame position - * implies an attempted write — exactly the case operators want flagged. - */ - private static long detectTornTail(long addr, long lastGood, long fileSize) { - if (lastGood >= fileSize) { - return 0L; + /** Address of the buffered copy of file offset {@code pos}; call only after {@link #require} returned true for a span covering it. */ + long addrOf(long pos) { + return bufAddr + (pos - winOff); } - long probe = Math.min(FRAME_HEADER_SIZE, fileSize - lastGood); - try { - for (long i = 0; i < probe; i++) { - if (Unsafe.getUnsafe().getByte(addr + lastGood + i) != 0) { - return fileSize - lastGood; - } + + /** + * Ensures {@code [pos, pos + n)} is buffered, repositioning the window + * if needed. {@code n} must be {@code <=} the buffer size. Returns + * false when the span cannot be read fully (short read at EOF, or a + * read error). + */ + boolean require(long pos, int n) { + if (pos >= winOff && pos + n <= winOff + winLen) { + return true; } - } catch (InternalError e) { - // The bail-out region is an unbacked (sparse) mapped page -- see - // scanFrames for the mechanism. An unbacked hole was never written, - // so it is clean unwritten space, not a torn write. Rethrow any - // error that is not the recoverable mmap access fault. - if (!isMmapAccessFault(e)) { - throw e; + long want = Math.min(bufSize, fileSize - pos); + long got = 0; + while (got < want) { + long r = ff.read(fd, bufAddr + got, want - got, pos + got); + if (r <= 0) { + break; + } + got += r; } - return 0L; - } - return 0L; - } - - /** - * Counts frames in {@code [HEADER_SIZE, lastGood)}. Walks the framing in - * lockstep with {@link #scanFrames} (which already validated CRCs); so - * this is just length-driven traversal, no CRC re-check. - */ - private static long countFrames(long addr, long lastGood) { - long pos = HEADER_SIZE; - long count = 0; - while (pos < lastGood) { - int payloadLen = Unsafe.getUnsafe().getInt(addr + pos + 4); - pos += FRAME_HEADER_SIZE + payloadLen; - count++; + winOff = pos; + winLen = got; + return n <= got; } - return count; } } diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java index c8516c4c..1e1d1aba 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java @@ -197,7 +197,7 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { // CAUTION: only unlink when the file is genuinely // empty past the header. If frame[0] failed CRC // (bit-rot, partial-page-write at crash, etc.) but - // valid frames followed, scanFrames returns + // valid frames followed, the recovery scan returns // lastGood=HEADER_SIZE and frameCount=0 -- yet // tornTailBytes is non-zero. Treating that as // "empty hot-spare" would silently destroy every @@ -269,8 +269,9 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { // FSNs after recovery. A gap means a segment went missing (a // manual deletion) or a sealed segment under-recovered -- its tail // was cut short by a sparse/unbacked page or a mid-file media error - // (bad sector), the same class of fault scanFrames tolerates on the - // active segment but which corrupts the range on a sealed one. + // (bad sector), the same unreadable-region boundary the recovery + // scan tolerates on the active segment but which corrupts the + // range on a sealed one. for (int i = 1, n = opened.size(); i < n; i++) { MmapSegment prev = opened.get(i - 1); MmapSegment curr = opened.get(i); diff --git a/core/src/main/java/io/questdb/client/std/FilesFacade.java b/core/src/main/java/io/questdb/client/std/FilesFacade.java index 1b408cf4..80e9dedb 100644 --- a/core/src/main/java/io/questdb/client/std/FilesFacade.java +++ b/core/src/main/java/io/questdb/client/std/FilesFacade.java @@ -91,15 +91,13 @@ public interface FilesFacade { * Stat length of the file at {@code path}, in bytes. Default delegates to * {@link Files#length(String)}. * - *

Test injection point: {@code MmapSegment.openExisting} maps the file to - * this length and scans straight out of the mapping, so a wrapping facade - * that returns a value larger than the real file makes the mapping - * extend past end-of-file. A read of a page beyond real EOF raises SIGBUS on - * every filesystem (which HotSpot translates to a catchable - * {@code InternalError} at an {@code Unsafe} intrinsic site) — the same fault - * a genuinely unbacked/sparse page raises on ZFS, but reproduced - * deterministically on ext4/xfs too. That is what lets recovery's mmap-fault - * guard be regression-tested on any CI runner rather than only on ZFS. + *

Test injection point: {@code MmapSegment.openExisting} treats this + * length as the file's extent and preads its recovery scan against it, so a + * wrapping facade that returns a value larger than the real file + * makes the scan read past end-of-file. Those reads come back short, which + * recovery must treat as the boundary of recoverable data — the same + * outcome as a file whose size metadata survived a crash its data blocks + * did not, reproduced deterministically on any filesystem and CI runner. */ long length(String path); diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java similarity index 57% rename from core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java rename to core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java index 861f761c..28b3d263 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java @@ -40,59 +40,31 @@ import static org.junit.Assert.fail; /** - * Regression guard for the recovery-time SIGBUS hazard in {@link MmapSegment}. + * Regression guard for recovery when part of a persisted {@code .sfa} is not + * readable data: a sparse hole (a truncate-based pre-allocation that never + * materialized the tail blocks, as on ZFS), a file shorter than its recorded + * length (size metadata that survived a crash its data blocks did not, or a + * truncation under recovery), or a failing read (bad sector). *

- * On recovery, {@link MmapSegment#openExisting} maps a persisted {@code .sfa} - * to its stat length and scans frames straight out of the mapping. When a prior - * session left a sparse segment tail -- a truncate-based pre-allocation that - * never materialized the tail blocks, as happens on ZFS -- a read of an - * unbacked page raises the JVM's recoverable - * {@code InternalError("...unsafe memory access operation...")} (a translated - * SIGBUS). Recovery must treat that page as the boundary of recoverable data, - * keep every frame below it, and hand back a usable segment -- not let the - * error abort recovery of the whole slot (the reported ZFS-CI flake). + * {@link MmapSegment#openExisting} reads its entire recovery scan through + * {@code pread} into a private buffer — never through the mapping — so each of + * those cases is an ordinary read result with one deterministic outcome per + * input: a hole reads back as zeros and fails the frame CRC, a region past + * real end-of-file gives a short read, and an I/O error gives a failed read. + * Recovery must treat all three as the boundary of recoverable data — keep + * every frame below it and either hand back a usable segment or throw the + * per-file {@link MmapSegmentException} that {@code SegmentRing} skips on — + * and the outcome must be the same on every filesystem, JDK, and JIT state. + * (Reading the same regions through the mapping would instead raise SIGBUS, + * which HotSpot converts to an {@code InternalError} delivered at an imprecise + * point under a JIT-compiled caller — the JDK 8 CI flake this suite guards + * against reintroducing.) *

- * These tests drive the production entry point ({@code openExisting}), - * not the private scan methods via reflection. That matters for two reasons: - *

- * The fault-delivery mechanism the fix rests on was verified directly on the - * shipping/CI Java floor -- JDK 8 (Temurin 1.8.0_492) -- not merely inferred - * from the adjacent pre-21 LTS releases: the whole class passes there in both - * interpreter ({@code -Xint}) and JIT modes, HotSpot emits the exact pre-21 - * message above, and a direct {@code try/catch} catches the fault in - * interpreter, C1, and C2 modes. {@code isMmapAccessFault}'s shared - * {@code "unsafe memory access operation"} fragment matches that message while - * the JDK 21+-only needle it replaced does not -- the guard is live on JDK 8. - * The unbacked tail is produced portably by truncating the file down (dropping - * the tail blocks) and back up to the mapping size (leaving a sparse hole). A - * hole-faulting filesystem (ZFS) then faults on the read exactly as in - * production -- the case the fix must survive rather than fold the CRC through - * the native, JNI-side {@code Crc32c} where a SIGBUS is uncatchable and aborts - * the JVM. A hole-zero-filling filesystem (ext4) instead reads the hole back as - * zeroes, which fails the frame CRC; either way recovery must stop at the same - * boundary and recover the same frames. - *

- * Fail-on-revert on any filesystem. The sparse-hole tests above only - * fault on ZFS: on ext4/xfs the within-EOF hole zero-fills, so the scan stops - * via the CRC-mismatch / bad-magic branch and they stay green even with the - * mmap-fault guard reverted -- no regression protection on the ext4/xfs CI - * runners. The two {@code MapPastEof} tests below close that gap portably. - * They truncate the file down (freeing the tail blocks) and hand - * {@code openExisting} a {@link FilesFacade} that reports the original, larger - * length, so the mapping extends past real end-of-file. A read of a page beyond - * real EOF raises SIGBUS on every filesystem -- the same catchable - * {@code InternalError} an unbacked ZFS page raises -- so they exercise the - * real fault path (and fail on revert) on ext4/xfs too, not only on ZFS. + * These tests drive the production entry point ({@code openExisting}), not + * private scan methods via reflection, so they exercise the real recovery + * path end to end. */ -public class MmapSegmentRecoveryFaultTest { +public class MmapSegmentRecoveryBoundaryTest { private static final long SEGMENT_BYTES = 1L << 20; @@ -110,8 +82,9 @@ public void tearDown() { /** * Clean unbacked tail: a single frame ends exactly on a page boundary and - * everything above it is a sparse hole. Recovery must keep the frame and - * stop at the boundary, reporting no torn tail (an unwritten hole is not a + * everything above it is a sparse hole. The hole preads back as zeros, + * which fails the next frame's CRC, so recovery keeps the frame and stops + * at the boundary, reporting no torn tail (an unwritten hole is not a * torn write). */ @Test @@ -126,7 +99,7 @@ public void testRecoveryKeepsFramesBeforeUnbackedTail() throws Exception { long boundary = writeSegment(path, 7L, new int[]{payloadLen}); assertEquals("frame must fill exactly one page", page, boundary); // Drop the tail blocks, then re-extend logically so [page, SEGMENT_BYTES) - // is an unbacked hole under the recovery mapping. + // is an unbacked hole inside the file's recorded length. punchSparseTail(path, page); try (MmapSegment seg = MmapSegment.openExisting(path)) { @@ -141,12 +114,10 @@ public void testRecoveryKeepsFramesBeforeUnbackedTail() throws Exception { * The harder case: a frame whose 8-byte header sits on a backed page but * whose payload reaches into the unbacked hole (a torn write leaves a real * positive {@code payloadLen} with the payload spanning the boundary). The - * CRC fold therefore reads across the backed-to-unbacked edge. Recovery - * must reject that frame and keep the one below it -- and, crucially, must - * do so via {@code Unsafe} reads: the native, JNI-side {@code Crc32c} over - * an unbacked page raises a SIGBUS that HotSpot cannot translate, aborting - * the whole JVM (verified: an {@code hs_err} in - * {@code Java_io_questdb_client_std_Crc32c_update}). + * CRC therefore folds across the backed-to-hole edge: the backed bytes are + * real, the hole preads as zeros, the CRC mismatches, and recovery rejects + * that frame while keeping the one below it. The surviving non-zero header + * bytes at the bail-out position are flagged as a torn tail. */ @Test public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception { @@ -156,7 +127,7 @@ public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception { final long boundary = 2 * page; // Frame 2's header ends 8 bytes below the boundary (backed); its // payload starts 8 bytes below and runs a full page past -- across - // the backed->unbacked edge. + // the backed->hole edge. final long frame2Offset = boundary - 16; final int payloadLen2 = (int) page; final int payloadLen1 = (int) (frame2Offset - MmapSegment.HEADER_SIZE - MmapSegment.FRAME_HEADER_SIZE); @@ -178,18 +149,12 @@ public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception { } /** - * M1 regression: the header block (magic/version/baseSeq) is read before - * {@code scanFrames}, so an unbacked page 0 faults ahead of the guarded - * scan. {@link MmapSegment#openExisting} must surface that as a - * {@link MmapSegmentException} -- the per-file signal {@code SegmentRing} - * catches to skip just this {@code .sfa} -- and never let the raw - * {@code InternalError} escape and abort recovery of every sibling segment. - *

- * Portable across filesystems: on a hole-faulting FS (ZFS) the fault is - * converted to a {@code MmapSegmentException} in {@code openExisting}'s - * catch; on a hole-zero-filling FS (ext4) page 0 reads back as zeroes, so - * the magic check fails and throws {@code MmapSegmentException} directly. - * Either way the file is skippable, not fatal. + * The header block (magic/version/baseSeq) is read before the frame scan, + * so a file whose page 0 is a hole must fail cleanly: the header preads + * back as zeros, the magic check fails, and {@link MmapSegment#openExisting} + * throws {@link MmapSegmentException} -- the per-file signal + * {@code SegmentRing} catches to skip just this {@code .sfa} -- rather + * than anything that could abort recovery of every sibling segment. */ @Test public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception { @@ -209,38 +174,19 @@ public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception { } /** - * Portable fail-on-revert guard for the recovery mmap-fault handling on the - * scan path. Unlike {@link #testRecoveryKeepsFramesBeforeUnbackedTail} - * (which only faults on ZFS), this maps the file past real EOF via the - * length-injecting facade, so the scan's read of the beyond-EOF page faults - * on ext4/xfs too. The fix must recognize that fault and keep - * recovery safe -- never a JVM abort, never a raw {@code InternalError} - * escaping into {@code SegmentRing}'s recovery loop. Revert the - * {@code scanFrames}/{@code openExisting} mmap-fault guard (or fold the CRC - * back through native {@code Crc32c}) and this errors or aborts the fork. - *

- * Two handled outcomes are accepted, because which one occurs depends on - * whether the recovery methods are JIT-compiled at fault time: - *

- * (The C2 delivery imprecision is a property of HotSpot's async - * unsafe-access fault handling, not of this seam; the seam only makes it - * reproducible off ZFS.) + * A file shorter than its recorded length: the facade reports twice the + * real length, so the scan's pread of the second page comes back short. + * Recovery must treat the short read as the boundary of recoverable data + * -- keep the frame below it, report no torn tail (nothing readable was + * ever written there), and hand back a usable segment. One deterministic + * outcome on every filesystem; this is the shape a crash leaves when the + * size metadata survives but the tail's data blocks do not, or a + * concurrent truncation during recovery. */ @Test - public void testScanFaultOnMapPastEofIsHandledAnyFilesystem() throws Exception { + public void testScanPastRealEofStopsAtBoundaryAnyFilesystem() throws Exception { TestUtils.assertMemoryLeak(() -> { - final String path = tmpDir + "/seg-mappasteof-scan.sfa"; + final String path = tmpDir + "/seg-pasteof-scan.sfa"; final long page = Files.PAGE_SIZE; // One frame that ends exactly on the first page boundary. final int payloadLen = (int) (page - MmapSegment.HEADER_SIZE - MmapSegment.FRAME_HEADER_SIZE); @@ -249,45 +195,34 @@ public void testScanFaultOnMapPastEofIsHandledAnyFilesystem() throws Exception { // Free every block past the first page: the file is now exactly one // (fully backed) page, with nothing beyond it on disk. truncateTo(path, page); - // Report twice the real length so openExisting maps a second, - // beyond-EOF page; the scan faults reading it on any filesystem. - FilesFacade ff = new MapPastEofFacade(path, 2 * page); + // Report twice the real length so the scan preads a second, + // beyond-EOF page; the read comes back short on any filesystem. + FilesFacade ff = new RecoverySeamFacade(path, 2 * page, -1L); try (MmapSegment seg = MmapSegment.openExisting(ff, path)) { - // Interpreter / C1: graceful partial recovery. assertEquals("the frame below the beyond-EOF page must be recovered", 1L, seg.frameCount()); assertEquals("scan must stop at the beyond-EOF boundary", page, seg.publishedOffset()); - assertEquals("a beyond-EOF page is not a torn write", 0L, seg.tornTailBytes()); - } catch (MmapSegmentException skippedUnderC2) { - // C2: the inlined fault escaped to openExisting's outer catch and - // was converted to a per-file skip. Assert it is the recognized - // mmap fault (not some other data error) so a revert -- which - // lets a raw InternalError through instead -- still fails here. - assertTrue(skippedUnderC2.getMessage(), - skippedUnderC2.getMessage().contains("unsafe memory access operation")); + assertEquals("a beyond-EOF region is not a torn write", 0L, seg.tornTailBytes()); } }); } /** - * Portable fail-on-revert guard for the {@code openExisting} header-block - * guard. The file is truncated to empty and the facade reports a full page, - * so the very first header read (magic) lands on a beyond-EOF page and - * faults on any filesystem. {@code openExisting} must convert that to a - * {@link MmapSegmentException} -- the per-file signal {@code SegmentRing} - * skips on -- not let the raw {@code InternalError} escape and abort recovery - * of every sibling. Revert the header-block conversion and this throws - * {@code InternalError} instead of {@code MmapSegmentException}. + * The degenerate shorter-than-recorded case: the file is truncated to + * empty and the facade reports a full page, so the very first header pread + * comes back empty. {@link MmapSegment#openExisting} must surface that as + * a {@link MmapSegmentException} -- the per-file signal {@code SegmentRing} + * skips on -- so one lost header never aborts recovery of every sibling. */ @Test - public void testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem() throws Exception { + public void testHeaderPastRealEofIsSkippableAnyFilesystem() throws Exception { TestUtils.assertMemoryLeak(() -> { - final String path = tmpDir + "/seg-mappasteof-header.sfa"; + final String path = tmpDir + "/seg-pasteof-header.sfa"; final long page = Files.PAGE_SIZE; writeSegment(path, 9L, new int[]{64}); // Free every block: the file is now empty, so even page 0 (the - // header) is beyond EOF under the reported one-page mapping. + // header) is beyond real EOF under the reported one-page length. truncateTo(path, 0L); - FilesFacade ff = new MapPastEofFacade(path, page); + FilesFacade ff = new RecoverySeamFacade(path, page, -1L); try { MmapSegment.openExisting(ff, path).close(); fail("expected MmapSegmentException for a beyond-EOF header page"); @@ -298,6 +233,33 @@ public void testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem() throws Excepti }); } + /** + * A failing read mid-scan (the bad-sector case): reads below the failure + * offset succeed, reads at or past it fail. Recovery must treat the + * failure as the boundary of recoverable data -- keep the frames below + * it, report no torn tail (the region cannot be probed), and hand back a + * usable segment rather than aborting the slot. + */ + @Test + public void testReadErrorDuringScanEndsRecoveryAtBoundary() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final String path = tmpDir + "/seg-read-error.sfa"; + final long page = Files.PAGE_SIZE; + final int payloadLen = (int) (page - MmapSegment.HEADER_SIZE - MmapSegment.FRAME_HEADER_SIZE); + long boundary = writeSegment(path, 13L, new int[]{payloadLen, 64}); + assertTrue("second frame must sit past the first page", boundary > page); + // Reads below `page` succeed (truncated at the bad region's edge, + // like a real pread up against a bad sector); reads at or past + // `page` fail. + FilesFacade ff = new RecoverySeamFacade(path, -1L, page); + try (MmapSegment seg = MmapSegment.openExisting(ff, path)) { + assertEquals("the frame below the unreadable region must be recovered", 1L, seg.frameCount()); + assertEquals("scan must stop at the unreadable region", page, seg.publishedOffset()); + assertEquals("an unreadable region cannot be probed for a torn write", 0L, seg.tornTailBytes()); + } + }); + } + /** * Creates a segment at {@code path} and appends one frame per entry in * {@code payloadLens} (each filled with non-zero bytes so recovery can tell @@ -329,8 +291,8 @@ private static long writeSegment(String path, long baseSeq, int[] payloadLens) { * Turns {@code [keepBytes, SEGMENT_BYTES)} of the file into an unbacked * sparse hole: truncate down to {@code keepBytes} (frees the tail blocks), * then back up to {@code SEGMENT_BYTES} (re-extends the logical size without - * allocating blocks). Recovery maps the full stat length, so the hole is - * inside the mapping -- reads of it fault on ZFS and zero-fill on ext4. + * allocating blocks). Recovery preads the full stat length, so the hole is + * inside the scanned range -- it reads back as zeros on every filesystem. */ private static void punchSparseTail(String path, long keepBytes) { int fd = Files.openRW(path); @@ -346,8 +308,8 @@ private static void punchSparseTail(String path, long keepBytes) { /** * Shrinks the file to {@code keepBytes}, freeing every block past it, and * leaves it there (no re-extend). Combined with a facade that reports a - * larger length, the freed region becomes a beyond-EOF part of the mapping - * that faults on read on any filesystem. + * larger length, the freed region is past real end-of-file, so recovery's + * preads of it come back short on any filesystem. */ private static void truncateTo(String path, long keepBytes) { int fd = Files.openRW(path); @@ -360,19 +322,28 @@ private static void truncateTo(String path, long keepBytes) { } /** - * A {@link FilesFacade} that reports an inflated stat length for one target - * path so {@code openExisting} maps that file past end-of-file (see - * {@link FilesFacade#length(String)}); every other call, including - * {@code length} for any other path, delegates to the production - * {@link FilesFacade#INSTANCE}. + * A {@link FilesFacade} exposing the two recovery seams for one target + * path; every other call, including calls for any other path, delegates to + * the production {@link FilesFacade#INSTANCE}. + * */ - private static final class MapPastEofFacade implements FilesFacade { + private static final class RecoverySeamFacade implements FilesFacade { + private final long failReadsFromOffset; private final long reportedLength; private final String targetPath; - MapPastEofFacade(String targetPath, long reportedLength) { + RecoverySeamFacade(String targetPath, long reportedLength, long failReadsFromOffset) { this.targetPath = targetPath; this.reportedLength = reportedLength; + this.failReadsFromOffset = failReadsFromOffset; } @Override @@ -442,7 +413,9 @@ public long length(long pathPtr) { @Override public long length(String path) { - return targetPath.equals(path) ? reportedLength : INSTANCE.length(path); + return reportedLength >= 0 && targetPath.equals(path) + ? reportedLength + : INSTANCE.length(path); } @Override @@ -477,6 +450,12 @@ public int openRW(long pathPtr) { @Override public long read(int fd, long addr, long len, long offset) { + if (failReadsFromOffset >= 0) { + if (offset >= failReadsFromOffset) { + return -1; + } + len = Math.min(len, failReadsFromOffset - offset); + } return INSTANCE.read(fd, addr, len, offset); } diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentTest.java index 177ee5f6..c6de5029 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentTest.java @@ -212,7 +212,7 @@ public void testFirstFrameCrcCorruptionFlagsTornTailAndPreservesFile() throws Ex // frames are followed by garbage. None cover frame[0] itself // being corrupt — yet a single bit-flip on the CRC of frame[0] // at rest (bit-rot, partial-page-write at crash) is the - // worst-case data-loss trigger: scanFrames bails at HEADER_SIZE + // worst-case data-loss trigger: the recovery scan bails at HEADER_SIZE // and frameCount drops to 0, even though valid frames still // sit on disk past the corrupt header. // @@ -255,7 +255,7 @@ public void testFirstFrameCrcCorruptionFlagsTornTailAndPreservesFile() throws Ex Files.exists(path)); try (MmapSegment seg = MmapSegment.openExisting(path)) { - assertEquals("scanFrames must bail at the corrupt frame[0]", + assertEquals("the recovery scan must bail at the corrupt frame[0]", 0L, seg.frameCount()); assertEquals("publishedOffset must rewind to the header end", MmapSegment.HEADER_SIZE, seg.publishedOffset()); diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/PrReviewRedTests.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/PrReviewRedTests.java index e551c6f8..7ae2218a 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/PrReviewRedTests.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/PrReviewRedTests.java @@ -79,8 +79,8 @@ public void tearDown() { /** * Finding C1 / C10 — first-frame CRC corruption silently deletes the segment. *

- * If frame[0] of a recovered .sfa fails CRC validation, scanFrames returns - * lastGood=HEADER_SIZE, countFrames returns 0, and SegmentRing.openExisting + * If frame[0] of a recovered .sfa fails CRC validation, the recovery scan returns + * lastGood=HEADER_SIZE with frameCount=0, and SegmentRing.openExisting * unlinks the file as an "empty hot-spare leftover" — destroying every frame * that physically followed the corrupt header. The torn-tail WARN inside * MmapSegment.openExisting is dropped on the floor. @@ -128,7 +128,7 @@ public void testC1_recoveryMustNotUnlinkSegmentWithCorruptFirstFrame() throws Ex // Run recovery. SegmentRing recovered = SegmentRing.openExisting(tmpDir, 64 * 1024); try { - // The bug: openExisting sees frameCount=0 (because scanFrames + // The bug: openExisting sees frameCount=0 (because the scan // bailed at the corrupt frame[0]) and treats the segment as // an "empty hot-spare leftover" — closing AND UNLINKING the // file. The user's frames 1, 2, 3 are gone forever; the only From 58bb7c805b2f341ba90a8c574c491bcbd264f5bb Mon Sep 17 00:00:00 2001 From: Marko Topolnik Date: Tue, 21 Jul 2026 09:43:35 +0200 Subject: [PATCH 2/5] Fix mapping leak on openExisting error path openExisting creates the RW mapping only after the recovery scan completes. The catch block used to close the fd but not the mapping, so a throw between a successful Files.mmap and the constructor return (realistically only an OOME allocating the MmapSegment) leaked the mapping. Track the address across the try block and munmap it in the catch before closing the fd, mirroring the cleanup pattern create() uses. Co-Authored-By: Claude Fable 5 --- .../client/cutlass/qwp/client/sf/cursor/MmapSegment.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java index 13f3a618..e3cdec11 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java @@ -296,6 +296,7 @@ public static MmapSegment openExisting(FilesFacade ff, String path) { if (fd < 0) { throw new MmapSegmentException("openRW failed for " + path); } + long addr = Files.FAILED_MMAP_ADDRESS; try { RecoveryScan scan = scanFile(ff, fd, path, fileSize); if (scan.tornTailBytes > 0) { @@ -306,13 +307,16 @@ public static MmapSegment openExisting(FilesFacade ff, String path) { + "Investigate disk health or unexpected writer crash.", path, scan.tornTailBytes, scan.lastGood, fileSize, scan.frameCount); } - long addr = Files.mmap(fd, fileSize, 0, Files.MAP_RW, MemoryTag.MMAP_DEFAULT); + addr = Files.mmap(fd, fileSize, 0, Files.MAP_RW, MemoryTag.MMAP_DEFAULT); if (addr == Files.FAILED_MMAP_ADDRESS) { throw new MmapSegmentException("mmap failed for " + path); } return new MmapSegment(path, fd, addr, fileSize, scan.baseSeq, scan.lastGood, scan.frameCount, false, scan.tornTailBytes); } catch (Throwable t) { + if (addr != Files.FAILED_MMAP_ADDRESS) { + Files.munmap(addr, fileSize, MemoryTag.MMAP_DEFAULT); + } ff.close(fd); throw t; } From ac8166da66a13f62c50dd3a6fe09193b43f170de Mon Sep 17 00:00:00 2001 From: Marko Topolnik Date: Tue, 21 Jul 2026 09:43:42 +0200 Subject: [PATCH 3/5] Align RecoveryScan and PreadWindow ordering RecoveryScan's constructor takes four same-typed longs, so its parameter order now matches the alphabetical field declaration order (baseSeq, frameCount, lastGood, tornTailBytes) to remove the positional-swap hazard; scanFile's call site follows. The nested classes also swap places so PreadWindow precedes RecoveryScan, keeping members alphabetically sorted per the repo convention. Co-Authored-By: Claude Fable 5 --- .../qwp/client/sf/cursor/MmapSegment.java | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java index e3cdec11..919023e0 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java @@ -646,27 +646,12 @@ private static RecoveryScan scanFile(FilesFacade ff, int fd, String path, long f } } } - return new RecoveryScan(baseSeq, pos, count, torn); + return new RecoveryScan(baseSeq, count, pos, torn); } finally { Unsafe.free(buf, bufSize, MemoryTag.NATIVE_DEFAULT); } } - /** Result of {@link #scanFile}: header fields plus scan outcomes. */ - private static final class RecoveryScan { - final long baseSeq; - final long frameCount; - final long lastGood; - final long tornTailBytes; - - RecoveryScan(long baseSeq, long lastGood, long frameCount, long tornTailBytes) { - this.baseSeq = baseSeq; - this.lastGood = lastGood; - this.frameCount = frameCount; - this.tornTailBytes = tornTailBytes; - } - } - /** * Sliding {@code pread} window over one file for the recovery scan. * {@link #require} repositions the window when the requested span is not @@ -721,4 +706,19 @@ boolean require(long pos, int n) { return n <= got; } } + + /** Result of {@link #scanFile}: header fields plus scan outcomes. */ + private static final class RecoveryScan { + final long baseSeq; + final long frameCount; + final long lastGood; + final long tornTailBytes; + + RecoveryScan(long baseSeq, long frameCount, long lastGood, long tornTailBytes) { + this.baseSeq = baseSeq; + this.frameCount = frameCount; + this.lastGood = lastGood; + this.tornTailBytes = tornTailBytes; + } + } } From 1cb4c656c174ae34d6b815f161b55786dad4593f Mon Sep 17 00:00:00 2001 From: Marko Topolnik Date: Tue, 21 Jul 2026 10:02:17 +0200 Subject: [PATCH 4/5] 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 --- .../qwp/client/sf/cursor/MmapSegment.java | 12 ++-- .../MmapSegmentRecoveryBoundaryTest.java | 58 +++++++++++++++++-- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java index 919023e0..d4ce10a8 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java @@ -67,7 +67,8 @@ public final class MmapSegment implements QuietCloseable { private static final Logger LOG = LoggerFactory.getLogger(MmapSegment.class); // Recovery reads the file through a pread window of this size (or the // whole file when smaller). Sized so a typical segment scans in a handful - // of preads; frames larger than the window CRC-fold across refills. + // of preads; the scan checksums a frame larger than the window in + // window-sized chunks. private static final long RECOVERY_BUF_BYTES = 1L << 20; private final String path; @@ -591,10 +592,11 @@ private static RecoveryScan scanFile(FilesFacade ff, int fd, String path, long f if (payloadLen < 0 || pos + FRAME_HEADER_SIZE + payloadLen > fileSize) { break; } - // CRC over the (payloadLen, payload) pair, folded window by - // window for frames larger than the buffer. Chaining - // Crc32c.update calls is bit-identical to one call over the - // contiguous range (tryAppend writes the CRC the same way). + // CRC over the (payloadLen, payload) pair, computed + // incrementally: each Crc32c.update call feeds the next + // window-sized chunk into the running value, which is + // bit-identical to one call over the contiguous range + // (tryAppend writes the CRC the same way). long crcPos = pos + 4; long remaining = 4L + payloadLen; int crc = Crc32c.INIT; diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java index 28b3d263..d76c40d1 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java @@ -60,6 +60,10 @@ * point under a JIT-compiled caller — the JDK 8 CI flake this suite guards * against reintroducing.) *

+ * The suite also guards the fully-readable side of the same machinery: a file + * larger than the scan's pread window, whose recovery slides the window + * forward and checks one frame's CRC in chunks spanning several window loads. + *

* These tests drive the production entry point ({@code openExisting}), not * private scan methods via reflection, so they exercise the real recovery * path end to end. @@ -114,10 +118,11 @@ public void testRecoveryKeepsFramesBeforeUnbackedTail() throws Exception { * The harder case: a frame whose 8-byte header sits on a backed page but * whose payload reaches into the unbacked hole (a torn write leaves a real * positive {@code payloadLen} with the payload spanning the boundary). The - * CRC therefore folds across the backed-to-hole edge: the backed bytes are - * real, the hole preads as zeros, the CRC mismatches, and recovery rejects - * that frame while keeping the one below it. The surviving non-zero header - * bytes at the bail-out position are flagged as a torn tail. + * CRC check therefore reads bytes on both sides of the backed-to-hole + * edge: the backed bytes are real, the hole preads as zeros, the CRC + * mismatches, and recovery rejects that frame while keeping the one + * below it. The surviving non-zero header bytes at the bail-out position + * are flagged as a torn tail. */ @Test public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception { @@ -260,6 +265,40 @@ public void testReadErrorDuringScanEndsRecoveryAtBoundary() throws Exception { }); } + /** + * The multi-window scan: a segment larger than the recovery read window + * (1 MiB), holding a frame whose CRC span -- the (payloadLen, payload) + * pair -- is itself larger than the window, followed by a small frame. + * Verifying the large frame forces the scan to compute its CRC + * incrementally -- reloading the window several times and feeding each + * chunk into the running value, which chained {@code Crc32c.update} calls + * make bit-identical to {@code tryAppend}'s one-pass CRC -- and the small + * frame is then verified out of a repositioned window. Every byte is + * readable, so recovery must be total: both frames kept, the cursor at + * the last appended byte, no torn tail. + */ + @Test + public void testScanRecoversFrameLargerThanReadWindow() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final String path = tmpDir + "/seg-multi-window.sfa"; + // 3 MiB of payload in a 4 MiB segment: checking the large frame's + // CRC reloads the window four times, and the scan as a whole + // takes multiple preads. (Sized against + // MmapSegment.RECOVERY_BUF_BYTES = 1 MiB; keep the payload larger + // than that constant or this test degrades to a single-window + // scan.) + final int largeLen = 3 * (1 << 20); + final long segmentBytes = 4L * (1 << 20); + long used = writeSegment(path, 17L, new int[]{largeLen, 64}, segmentBytes); + + try (MmapSegment seg = MmapSegment.openExisting(path)) { + assertEquals("both frames must be recovered", 2L, seg.frameCount()); + assertEquals("scan must recover up to the last appended byte", used, seg.publishedOffset()); + assertEquals("a fully recovered segment has no torn tail", 0L, seg.tornTailBytes()); + } + }); + } + /** * Creates a segment at {@code path} and appends one frame per entry in * {@code payloadLens} (each filled with non-zero bytes so recovery can tell @@ -267,6 +306,15 @@ public void testReadErrorDuringScanEndsRecoveryAtBoundary() throws Exception { * (the published offset after the last append). */ private static long writeSegment(String path, long baseSeq, int[] payloadLens) { + return writeSegment(path, baseSeq, payloadLens, SEGMENT_BYTES); + } + + /** + * Variant of {@link #writeSegment(String, long, int[])} with an explicit + * segment size, for tests whose file must exceed the recovery scan's + * 1 MiB pread window. + */ + private static long writeSegment(String path, long baseSeq, int[] payloadLens, long segmentBytes) { int maxLen = 0; for (int len : payloadLens) { maxLen = Math.max(maxLen, len); @@ -276,7 +324,7 @@ private static long writeSegment(String path, long baseSeq, int[] payloadLens) { for (int i = 0; i < maxLen; i++) { Unsafe.getUnsafe().putByte(buf + i, (byte) (i | 1)); // all non-zero } - try (MmapSegment seg = MmapSegment.create(path, baseSeq, SEGMENT_BYTES)) { + try (MmapSegment seg = MmapSegment.create(path, baseSeq, segmentBytes)) { for (int len : payloadLens) { assertTrue("append must fit", seg.tryAppend(buf, len) >= 0); } From a0806f5a9719be6dc804a91cc60cbac51f9ed27c Mon Sep 17 00:00:00 2001 From: Marko Topolnik Date: Tue, 21 Jul 2026 10:33:53 +0200 Subject: [PATCH 5/5] Cover read failure inside a frame's CRC span testReadErrorMidFrameCrcSpanEndsRecoveryAtFrameStart covers the one recovery path the suite left unexercised: a read failure partway through a large frame's CRC span. Every earlier unreadable-region test fails on a frame-header read, taking the scan loop's outer break; this test makes the failure land on the second CRC chunk of a 3 MiB frame, driving the labeled break inside scanFile's incremental CRC loop. Recovery must discard the partially checksummed frame whole, keep the intact frame below it, and skip the torn-tail probe. A mutation check confirms the test discriminates: dropping the unreadable flag on that branch makes the torn-tail probe run against the unprobeable region and misreport a ~4 MiB torn tail, failing exactly this test and no other. The multi-window test's javadoc claimed the small second frame is verified out of a repositioned window; the window the large frame's final CRC chunk loaded already covers it, and the javadoc now says so. Co-Authored-By: Claude Fable 5 --- .../MmapSegmentRecoveryBoundaryTest.java | 45 +++++++++++++++++-- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java index d76c40d1..2a8c706f 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryBoundaryTest.java @@ -273,9 +273,10 @@ public void testReadErrorDuringScanEndsRecoveryAtBoundary() throws Exception { * incrementally -- reloading the window several times and feeding each * chunk into the running value, which chained {@code Crc32c.update} calls * make bit-identical to {@code tryAppend}'s one-pass CRC -- and the small - * frame is then verified out of a repositioned window. Every byte is - * readable, so recovery must be total: both frames kept, the cursor at - * the last appended byte, no torn tail. + * frame is then verified out of the window the large frame's final CRC + * chunk loaded, a window positioned near the file's end rather than at + * offset 0. Every byte is readable, so recovery must be total: both + * frames kept, the cursor at the last appended byte, no torn tail. */ @Test public void testScanRecoversFrameLargerThanReadWindow() throws Exception { @@ -299,6 +300,44 @@ public void testScanRecoversFrameLargerThanReadWindow() throws Exception { }); } + /** + * A failing read partway through a large frame's CRC span: the frame's + * header preads fine and the first window-sized CRC chunk succeeds, but + * the next chunk hits the unreadable region. A partially checksummed + * frame is unverifiable, so recovery must discard it whole -- stop at + * its start, keep the intact frame below it, and report no torn tail + * (the region cannot be probed). This is the one path where the + * incremental multi-window CRC meets an unreadable region; the other + * unreadable-region tests all fail on a frame-header read instead. + */ + @Test + public void testReadErrorMidFrameCrcSpanEndsRecoveryAtFrameStart() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final String path = tmpDir + "/seg-read-error-mid-frame.sfa"; + // A small intact frame, then a 3 MiB frame whose CRC span takes + // several window loads (sized against + // MmapSegment.RECOVERY_BUF_BYTES = 1 MiB). + final int smallLen = 64; + final int largeLen = 3 * (1 << 20); + final long segmentBytes = 4L * (1 << 20); + writeSegment(path, 21L, new int[]{smallLen, largeLen}, segmentBytes); + final long largeFrameOffset = MmapSegment.HEADER_SIZE + MmapSegment.FRAME_HEADER_SIZE + smallLen; + // Fail reads from 2 MiB on: past the large frame's header and its + // entire first CRC chunk, but inside its payload -- the failure + // lands on the second chunk's read, in the CRC loop, not on a + // frame-header read. + FilesFacade ff = new RecoverySeamFacade(path, -1L, 2L * (1 << 20)); + try (MmapSegment seg = MmapSegment.openExisting(ff, path)) { + assertEquals("the intact frame below the unreadable region must be recovered", + 1L, seg.frameCount()); + assertEquals("recovery must stop at the start of the partially checksummed frame", + largeFrameOffset, seg.publishedOffset()); + assertEquals("an unreadable region cannot be probed for a torn write", + 0L, seg.tornTailBytes()); + } + }); + } + /** * Creates a segment at {@code path} and appends one frame per entry in * {@code payloadLens} (each filled with non-zero bytes so recovery can tell