Preserve symbol dict on a transient stat fault

PersistedSymbolDict.open() computed the routing length as
ff.length(path) and sent anything below HEADER_SIZE to openFresh(),
which O_TRUNCs the file. A stat that fails transiently (an EIO on a
flaky disk) returns -1 for a fully populated dictionary, so that
routing could truncate the only copy of load-bearing recovery state --
the exact destruction the class's "never recreate over an existing
file" contract forbids, and which the openExisting read path already
guards against but the routing check did not.

A stat error reports -1, distinguishable from a genuine sub-header stub
in [0, HEADER_SIZE), so open() now degrades to null when the file
exists but its length reads negative: it falls back to full
self-sufficient frames and leaves every byte on disk for a later
attempt, once the transient clears.

Also move appendRawEntries' per-entry (addr, len, count) consistency
walk behind an assert. It re-reads every entry's length prefix on the
common flush path, yet its sole caller derives count and len from one
beginMessage, so it cannot fire today. The assert keeps the structural
guard in the client's -ea test suite while eliding it from production,
where client apps run without -ea.

PersistedSymbolDictTest now covers a transient length fault leaving the
file intact and recovering once it clears, and an inconsistent
raw-entries triple being rejected under -ea.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: glasstiger

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

@@ -215,7 +215,22 @@ public static PersistedSymbolDict open(String slotDir) {
      */
     public static PersistedSymbolDict open(FilesFacade ff, String slotDir) {
         String filePath = slotDir + "/" + FILE_NAME;
-        long existing = ff.exists(filePath) ? ff.length(filePath) : -1L;
+        boolean exists = ff.exists(filePath);
+        long existing = exists ? ff.length(filePath) : -1L;
+        if (exists && existing < 0) {
+            // The file is present but its length could not be stat'd (a transient EIO
+            // on a flaky disk). Do NOT fall through to openFresh below, which O_TRUNCs:
+            // truncating the only copy of load-bearing state on a TRANSIENT fault is the
+            // exact destruction the class-level "Never recreate over an existing file"
+            // note forbids -- and unlike the openExisting read path, this routing check
+            // otherwise has no guard. A genuine sub-header stub reports a length in
+            // [0, HEADER_SIZE); only a stat error reports < 0, so the two are
+            // distinguishable. Degrade to full self-sufficient frames and leave every
+            // byte on disk for a later attempt, once the transient clears.
+            LOG.warn("symbol dict {} exists but its length could not be read; "
+                    + "falling back to full-dictionary frames (file left intact)", filePath);
+            return null;
+        }
         if (existing >= HEADER_SIZE) {
             // A dictionary at or past Integer.MAX_VALUE cannot be read back:
             // openExisting reads it into ONE int-sized native buffer, and the (int)
@@ -310,36 +325,12 @@ public synchronized void appendRawEntries(long addr, int len, int count) {
         // with the entries it holds, shifting the dense id->symbol map on recovery.
         // The sole caller derives count and len from one beginMessage, so this cannot
         // fire today -- but the file this guards is the one the "resend required"
-        // contract rests on, so it stays.
-        long src = addr;
-        long srcLimit = addr + len;
-        for (int i = 0; i < count; i++) {
-            long symLen = 0;
-            int shift = 0;
-            while (src < srcLimit) {
-                byte b = Unsafe.getUnsafe().getByte(src++);
-                symLen |= (long) (b & 0x7F) << shift;
-                if ((b & 0x80) == 0) {
-                    break;
-                }
-                shift += 7;
-                if (shift > 35) {
-                    // A canonical entry-length varint is <= 5 bytes; a longer
-                    // continuation run is corrupt. The bound check below rejects it.
-                    break;
-                }
-            }
-            src += symLen; // src was just past the len varint
-            if (src > srcLimit) {
-                throw new IllegalStateException("malformed raw symbol-dict entries to " + FILE_NAME
-                        + " [entry=" + i + ", count=" + count + ']');
-            }
-        }
-        if (src != srcLimit) {
-            throw new IllegalStateException("raw symbol-dict entries under-filled the buffer to "
-                    + FILE_NAME + " [count=" + count + ", len=" + len
-                    + ", consumed=" + (int) (src - addr) + ']');
-        }
+        // contract rests on, so keep the structural guard. Gated behind assert: it
+        // re-walks every entry's length prefix on the common flush path, and the client
+        // library runs without -ea in production (embedded in user apps), so this holds
+        // the guard in the client's own -ea test suite without the per-flush cost in
+        // production.
+        assert validateRawEntries(addr, len, count);
         int hdrLen = NativeBufferWriter.varintSize(count) + NativeBufferWriter.varintSize(len);
         ensureScratch((long) hdrLen + len + CRC_SIZE);
         long p = NativeBufferWriter.writeVarint(scratchAddr, count);
@@ -693,6 +684,48 @@ private static PersistedSymbolDict openFresh(FilesFacade ff, String filePath) {
         return new PersistedSymbolDict(ff, fd, HEADER_SIZE, 0, 0L, 0);
     }
 
+    /**
+     * Verifies that {@code count} wire entries ({@code [len varint][utf8]}) occupy
+     * exactly {@code len} bytes from {@code addr}. Returns {@code true} when the triple
+     * is consistent; throws {@link IllegalStateException} (naming the offending entry /
+     * count / consumed bytes) otherwise. Called only from an {@code assert} in
+     * {@link #appendRawEntries}: it guards an internal invariant the sole caller cannot
+     * violate today, so it runs under the test suite's {@code -ea} but is elided from
+     * the per-flush production path (client apps run without {@code -ea}).
+     */
+    private static boolean validateRawEntries(long addr, int len, int count) {
+        long src = addr;
+        long srcLimit = addr + len;
+        for (int i = 0; i < count; i++) {
+            long symLen = 0;
+            int shift = 0;
+            while (src < srcLimit) {
+                byte b = Unsafe.getUnsafe().getByte(src++);
+                symLen |= (long) (b & 0x7F) << shift;
+                if ((b & 0x80) == 0) {
+                    break;
+                }
+                shift += 7;
+                if (shift > 35) {
+                    // A canonical entry-length varint is <= 5 bytes; a longer
+                    // continuation run is corrupt. The bound check below rejects it.
+                    break;
+                }
+            }
+            src += symLen; // src was just past the len varint
+            if (src > srcLimit) {
+                throw new IllegalStateException("malformed raw symbol-dict entries to " + FILE_NAME
+                        + " [entry=" + i + ", count=" + count + ']');
+            }
+        }
+        if (src != srcLimit) {
+            throw new IllegalStateException("raw symbol-dict entries under-filled the buffer to "
+                    + FILE_NAME + " [count=" + count + ", len=" + len
+                    + ", consumed=" + (int) (src - addr) + ']');
+        }
+        return true;
+    }
+
     /**
      * Grows the append scratch buffer to hold at least {@code required} bytes.
      * <p>

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

@@ -31,6 +31,7 @@
 import io.questdb.client.test.tools.DelegatingFilesFacade;
 import io.questdb.client.test.tools.TestUtils;
 import org.junit.Assert;
+import org.junit.Assume;
 import org.junit.Test;
 
 import java.nio.charset.StandardCharsets;
@@ -152,6 +153,56 @@ public void testAppendRawEntriesMatchesAppendSymbols() throws Exception {
         });
     }
 
+    @Test
+    public void testAppendRawEntriesRejectsInconsistentTripleUnderAssertions() throws Exception {
+        // appendRawEntries validates the (addr,len,count) triple before writing: an
+        // inconsistent triple would record a chunk whose stored entryCount disagreed
+        // with its entries and shift the dense id->symbol map on recovery, so it must
+        // fail loudly rather than corrupt the file. That check is an assert -- gated on
+        // -ea to keep the per-entry walk off the per-flush production path (client apps
+        // run without -ea) -- so observe it under the test suite's -ea, and skip if
+        // assertions happen to be disabled.
+        boolean assertionsEnabled = false;
+        assert assertionsEnabled = true;
+        Assume.assumeTrue("the guard is assert-gated; only observable under -ea", assertionsEnabled);
+        assertMemoryLeak(() -> {
+            Path src = Files.createTempDirectory("qwp-symdict-src");
+            Path dst = Files.createTempDirectory("qwp-symdict-dst");
+            try {
+                GlobalSymbolDictionary dict = new GlobalSymbolDictionary();
+                dict.getOrAddSymbol("AAPL"); // id 0
+                dict.getOrAddSymbol("MSFT"); // id 1
+                dict.getOrAddSymbol("GOOG"); // id 2
+                PersistedSymbolDict encoded = PersistedSymbolDict.open(src.toString());
+                encoded.appendSymbols(dict, 0, 2);
+                encoded.close();
+
+                // Grab the on-disk entry region (3 entries) verbatim, then feed it back
+                // with a count of 1: validateRawEntries stops one entry in with
+                // src < srcLimit and throws "under-filled", before any bytes are written.
+                PersistedSymbolDict reopened = PersistedSymbolDict.open(src.toString());
+                try {
+                    PersistedSymbolDict raw = PersistedSymbolDict.open(dst.toString());
+                    try {
+                        raw.appendRawEntries(reopened.loadedEntriesAddr(), reopened.loadedEntriesLen(), 1);
+                        Assert.fail("an inconsistent (len,count) triple must be rejected");
+                    } catch (IllegalStateException expected) {
+                        Assert.assertTrue("message names the under-filled buffer: " + expected.getMessage(),
+                                expected.getMessage().contains("under-filled"));
+                        Assert.assertEquals("a rejected triple must write nothing", 0, raw.size());
+                    } finally {
+                        raw.close();
+                    }
+                } finally {
+                    reopened.close();
+                }
+            } finally {
+                rmDir(src);
+                rmDir(dst);
+            }
+        });
+    }
+
     @Test
     public void testAppendRawEntriesShortWriteThrowsWithoutAdvancingIsIdempotentOnRetry() throws Exception {
         // The producer's FAST path persists a frame's already-encoded delta bytes
@@ -774,6 +825,60 @@ public void testTornTrailingEntrySelfHeals() throws Exception {
         });
     }
 
+    @Test
+    public void testTransientLengthFaultDegradesWithoutDestroyingTheFile() throws Exception {
+        // open() routes on ff.length(): a value < HEADER_SIZE falls through to the
+        // truncating openFresh(). A genuine sub-header stub reports a length in
+        // [0, HEADER_SIZE), but a TRANSIENT stat failure (an EIO on a flaky disk)
+        // reports the -1 error sentinel for a fully populated file -- and routing that
+        // to openFresh would O_TRUNC the only copy of load-bearing state, the exact
+        // destruction the "Never recreate over an existing file" contract forbids. A
+        // negative length is distinguishable from a stub, so open() must degrade to
+        // null (fall back to full self-sufficient frames) and leave every byte on disk
+        // for a later attempt, once the transient clears.
+        assertMemoryLeak(() -> {
+            Path dir = Files.createTempDirectory("qwp-symdict");
+            try {
+                PersistedSymbolDict d = PersistedSymbolDict.open(dir.toString());
+                d.appendSymbol("one");
+                d.appendSymbol("two");
+                d.close();
+
+                Path f = dir.resolve(".symbol-dict");
+                byte[] before = Files.readAllBytes(f);
+                Assert.assertTrue("a populated dict must exceed the header", before.length > HEADER_SIZE);
+
+                // Reopen through a facade whose length() reports the -1 stat-error
+                // sentinel for the (present) file -- the branch under test.
+                PersistedSymbolDict reopened = PersistedSymbolDict.open(new StatFailsLengthFacade(), dir.toString());
+                if (reopened != null) {
+                    // Pre-fix: open() fell through to openFresh() and handed back a fresh
+                    // empty dict over the now-truncated file. Close its fd so only the
+                    // assertion below reports the failure, not a leaked descriptor.
+                    reopened.close();
+                }
+                Assert.assertNull("a populated dict whose length cannot be stat'd must degrade to null, not truncate",
+                        reopened);
+                Assert.assertArrayEquals("a transient length-stat fault must NOT destroy the dictionary",
+                        before, Files.readAllBytes(f));
+
+                // Once the filesystem recovers, the SAME file reopens its intact content.
+                PersistedSymbolDict re = PersistedSymbolDict.open(dir.toString());
+                Assert.assertNotNull(re);
+                try {
+                    Assert.assertEquals("the intact content survives the transient", 2, re.size());
+                    ObjList<String> s = re.readLoadedSymbols();
+                    Assert.assertEquals("one", s.getQuick(0));
+                    Assert.assertEquals("two", s.getQuick(1));
+                } finally {
+                    re.close();
+                }
+            } finally {
+                rmDir(dir);
+            }
+        });
+    }
+
     @Test
     public void testTruncateFailureDegradesWithoutDestroyingTheFile() throws Exception {
         // A host crash can leave a torn/stale tail past the last complete chunk.
@@ -894,4 +999,17 @@ public long write(int fd, long addr, long len, long offset) {
             return INSTANCE.write(fd, addr, len, offset);
         }
     }
+
+    /**
+     * Reports a length of -1 -- the stat-error sentinel -- for the dictionary file,
+     * reproducing a transient stat failure (an EIO on a flaky disk) where the file is
+     * present but its size cannot be read. open() must treat this as "present but
+     * unreadable" and degrade to null, NOT route it to the truncating fresh-open path.
+     */
+    private static final class StatFailsLengthFacade extends DelegatingFilesFacade {
+        @Override
+        public long length(String path) {
+            return -1L;
+        }
+    }
 }