Report leftover MVV mark bits in IntegrityCheck

A prune interrupted before the issue #286 fix could leave 0x8000 mark
bits behind in on-disk version length fields. Since PR #288 the read
paths strip the mark bit silently, so such versions read normally and
icheck reported affected volumes as clean.

Add MVV.countMarkedVersions and scan every data record during icheck:
marked versions are counted in a new MarkedVersions counter (report,
CSV and getMarkedVersionCount) and reported as a fault so affected
volumes can be identified. The scan runs only after visitAllVersions
has validated the version structure, so a structurally corrupt MVV
cannot produce a bogus marked-version fault.

icheck -p remains the remediation: pruning rewrites the marked
versions (issue #292 makes prune clear stale marks unconditionally).
With icheck -P the marked-version faults are reported but do not
block clearing the TransactionIndex, since the prune pass has already
rewritten the marks. The icheck -c CSV example in Management.rst is
updated for the new MarkedVersions column.

Fixes #290

Author: vharseko

persistit/core/src/main/java/com/persistit/IntegrityCheck.java

@@ -60,6 +60,7 @@ public class IntegrityCheck extends Task {
 
     private Volume _currentVolume;
     private Tree _currentTree;
+    private long _currentPage;
     private LongBitSet _usedPageBits = new LongBitSet();
     private long _totalPages = 0;
     private long _pagesVisited = 0;
@@ -79,6 +80,7 @@ public class IntegrityCheck extends Task {
     private boolean _csv;
 
     private final ArrayList<Fault> _faults = new ArrayList<Fault>();
+    private int _markedVersionFaultCount;
     private final ArrayList<CleanupIndexHole> _holes = new ArrayList<CleanupIndexHole>();
 
     // Used in checking long values
@@ -98,6 +100,7 @@ private static class Counters {
         private long _mvvCount = 0;
         private long _mvvOverhead = 0;
         private long _mvvAntiValues = 0;
+        private long _markedVersionCount = 0;
         private long _pruningErrorCount = 0;
         private long _prunedPageCount = 0;
         private long _garbagePageCount = 0;
@@ -118,6 +121,7 @@ private static class Counters {
             _mvvCount = counters._mvvCount;
             _mvvOverhead = counters._mvvOverhead;
             _mvvAntiValues = counters._mvvAntiValues;
+            _markedVersionCount = counters._markedVersionCount;
             _pruningErrorCount = counters._pruningErrorCount;
             _prunedPageCount = counters._prunedPageCount;
             _garbagePageCount = counters._garbagePageCount;
@@ -135,6 +139,7 @@ void difference(final Counters counters) {
             _mvvCount = counters._mvvCount - _mvvCount;
             _mvvOverhead = counters._mvvOverhead - _mvvOverhead;
             _mvvAntiValues = counters._mvvAntiValues - _mvvAntiValues;
+            _markedVersionCount = counters._markedVersionCount - _markedVersionCount;
             _pruningErrorCount = counters._pruningErrorCount - _pruningErrorCount;
             _prunedPageCount = counters._prunedPageCount - _prunedPageCount;
             _garbagePageCount = counters._garbagePageCount - _garbagePageCount;
@@ -144,20 +149,21 @@ void difference(final Counters counters) {
         public String toString() {
             return String.format("Index pages/bytes: %,d / %,d Data pages/bytes: %,d / %,d"
                     + " LongRec pages/bytes: %,d / %,d  MVV pages/records/bytes/antivalues: "
-                    + "%,d / %,d / %,d / %,d  Holes %,d Pages pruned %,d", _indexPageCount, _indexBytesInUse,
-                    _dataPageCount, _dataBytesInUse, _longRecordPageCount, _longRecordBytesInUse, _mvvPageCount,
-                    _mvvCount, _mvvOverhead, _mvvAntiValues, _indexHoleCount, _prunedPageCount);
+                    + "%,d / %,d / %,d / %,d  Holes %,d Pages pruned %,d Marked versions %,d", _indexPageCount,
+                    _indexBytesInUse, _dataPageCount, _dataBytesInUse, _longRecordPageCount, _longRecordBytesInUse,
+                    _mvvPageCount, _mvvCount, _mvvOverhead, _mvvAntiValues, _indexHoleCount, _prunedPageCount,
+                    _markedVersionCount);
         }
 
         private String toCSV() {
-            return String.format("%d,%d,%d,%d,%d,%d,%d,%d,%d,%d,%d,%d", _indexPageCount, _indexBytesInUse,
+            return String.format("%d,%d,%d,%d,%d,%d,%d,%d,%d,%d,%d,%d,%d", _indexPageCount, _indexBytesInUse,
                     _dataPageCount, _dataBytesInUse, _longRecordPageCount, _longRecordBytesInUse, _mvvPageCount,
-                    _mvvCount, _mvvOverhead, _mvvAntiValues, _indexHoleCount, _prunedPageCount);
+                    _mvvCount, _mvvOverhead, _mvvAntiValues, _indexHoleCount, _prunedPageCount, _markedVersionCount);
         }
 
         private final static String CSV_HEADERS = "IndexPages,IndexBytes,"
                 + "DataPages,DataBytes,LongRecordPages,LongRecordBytes,MvvPages,"
-                + "MvvRecords,MvvOverhead,MvvAntiValues,IndexHoles,PrunedPages";
+                + "MvvRecords,MvvOverhead,MvvAntiValues,IndexHoles,PrunedPages,MarkedVersions";
 
     }
 
@@ -186,6 +192,20 @@ public void sawVersion(final long version, final int offset, final int valueLeng
         protected void visitDataRecord(final Key key, final int foundAt, final int tail, final int klength,
                 final int offset, final int length, final byte[] bytes) throws PersistitException {
             MVV.visitAllVersions(_versionVisitor, bytes, offset, length);
+            /*
+             * Scanned only after visitAllVersions has validated the version
+             * structure. Leftover prune marks read normally (the length
+             * accessors strip the mark bit), so only this dedicated scan can
+             * detect and report them - see issue #290.
+             */
+            final int marked = MVV.countMarkedVersions(bytes, offset, length);
+            if (marked > 0) {
+                _counters._markedVersionCount += marked;
+                if (addFault("MVV has " + plural(marked, "version") + " with a leftover mark bit", _currentPage, 0,
+                        foundAt)) {
+                    _markedVersionFaultCount++;
+                }
+            }
             if (_versionVisitor._count > 0) {
                 _counters._mvvCount++;
                 final int voffset = _versionVisitor._lastOffset;
@@ -289,7 +309,14 @@ protected void runTask() {
                 postMessage("Total " + toString(), LOG_NORMAL);
             }
             if (_pruneAndClear) {
-                if (_faults.isEmpty() && _counters._mvvPageCount == _counters._prunedPageCount
+                /*
+                 * Leftover mark-bit faults (issue #290) identify volumes
+                 * affected by a pre-#286 interrupted prune but do not block
+                 * clearing the TransactionIndex: the prune pass above has
+                 * already rewritten those marks (issue #292). Only other
+                 * faults and incomplete pruning count as failures here.
+                 */
+                if (_faults.size() == _markedVersionFaultCount && _counters._mvvPageCount == _counters._prunedPageCount
                         && _counters._pruningErrorCount == 0) {
                     final int count = _persistit.getTransactionIndex().resetMVVCounts(startTimestamp);
                     postMessage(String.format("%,d aborted transactions were cleared by pruning", count), LOG_NORMAL);
@@ -329,11 +356,13 @@ private String plural(final int n, final String m) {
         }
     }
 
-    private void addFault(final String description, final long page, final int level, final int position) {
+    private boolean addFault(final String description, final long page, final int level, final int position) {
         final Fault fault = new Fault(resourceName(), this, description, page, _treeDepth, level, position);
-        if (_faults.size() < MAX_FAULTS)
+        final boolean recorded = _faults.size() < MAX_FAULTS;
+        if (recorded)
             _faults.add(fault);
         postMessage(fault.toString(), LOG_VERBOSE);
+        return recorded;
     }
 
     private void addGarbageFault(final String description, final long page, final int level, final int position) {
@@ -544,6 +573,16 @@ public long getMvvAntiValues() {
         return _counters._mvvAntiValues;
     }
 
+    /**
+     * @return Count of MVV versions whose length field still carries a
+     *         leftover prune mark bit, left behind by a prune interrupted
+     *         before the issue #286 fix. Such versions read normally but
+     *         indicate the volume was corrupted; pruning rewrites them.
+     */
+    public long getMarkedVersionCount() {
+        return _counters._markedVersionCount;
+    }
+
     /**
      * @return Count of pages for which an expected index pointer is missing
      */
@@ -1144,6 +1183,7 @@ private Buffer walkRight(final int level, final long toPage, final Key key, fina
     }
 
     private boolean verifyPage(final Buffer buffer, final long page, final int level, final Key key, final Tree tree) {
+        _currentPage = page;
         if (buffer.getPageAddress() != page) {
             addFault("Buffer contains wrong page " + buffer.getPageAddress(), page, level, 0);
             return false;

persistit/core/src/main/java/com/persistit/MVV.java

@@ -643,6 +643,37 @@ static boolean verify(final byte[] bytes, final int offset, final int length) {
         return true;
     }
 
+    /**
+     * Count versions whose length field still carries the mark bit. Marks are
+     * transient state private to {@link #prune} and must never be observable
+     * outside it; a non-zero count on a stored MVV means the value was
+     * corrupted by a prune interrupted before the issue #286 fix. The length
+     * accessors strip the mark bit silently, so such versions read normally
+     * and only this scan can detect them (issue #290).
+     *
+     * @param bytes
+     *            the byte array
+     * @param offset
+     *            the index of the first byte of the MVV within the byte array
+     * @param length
+     *            the count of bytes in the MVV
+     * @return the number of marked versions, 0 for a non-MVV value
+     */
+    static int countMarkedVersions(final byte[] bytes, final int offset, final int length) {
+        if (!isArrayMVV(bytes, offset, length)) {
+            return 0;
+        }
+        int marked = 0;
+        int from = offset + 1;
+        while (from + LENGTH_PER_VERSION <= offset + length) {
+            if (isMarked(bytes, from)) {
+                marked++;
+            }
+            from += getLength(bytes, from) + LENGTH_PER_VERSION;
+        }
+        return marked;
+    }
+
     static boolean verify(final TransactionIndex ti, final byte[] bytes, final int offset, final int length) {
         if (!isArrayMVV(bytes, offset, length)) {
             /*

persistit/core/src/test/java/com/persistit/IntegrityCheckTest.java

@@ -23,8 +23,12 @@
 import org.junit.Test;
 
 import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.SortedMap;
+import java.util.TreeMap;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
@@ -99,6 +103,88 @@ public void testBrokenMVVs() throws Exception {
         assertTrue(icheck.getFaults().length > 0);
     }
 
+    /**
+     * Issue #290: a leftover prune mark bit (pre-#288 interrupted-prune
+     * corruption) reads normally through all fetch paths, so IntegrityCheck
+     * must detect it explicitly and report it as a fault.
+     */
+    @Test
+    public void testLeftoverMarkBitsReported() throws Exception {
+        final Exchange ex = _persistit.getExchange(_volumeName, "mvv", true);
+        disableBackgroundCleanup();
+        transactionalStore(ex);
+
+        IntegrityCheck icheck = icheck();
+        icheck.checkTree(ex.getTree());
+        assertEquals(0, icheck.getFaults().length);
+        assertEquals(0, icheck.getMarkedVersionCount());
+
+        final int[] corrupted = corrupt5(ex);
+        final int corruptedValues = corrupted[0];
+        final int markedVersions = corrupted[1];
+        assertTrue(corruptedValues > 0);
+        assertTrue("some values must carry more than one mark", markedVersions > corruptedValues);
+
+        icheck = icheck();
+        icheck.checkTree(ex.getTree());
+        assertEquals(corruptedValues, icheck.getFaults().length);
+        assertEquals(markedVersions, icheck.getMarkedVersionCount());
+    }
+
+    /**
+     * Issue #290: pruning rewrites marked versions, so icheck with pruning
+     * enabled is the remediation path - a subsequent icheck must be clean.
+     */
+    @Test
+    public void testPruneClearsLeftoverMarkBits() throws Exception {
+        final Exchange ex = _persistit.getExchange(_volumeName, "mvv", true);
+        disableBackgroundCleanup();
+        transactionalStore(ex);
+        _persistit.getTransactionIndex().updateActiveTransactionCache();
+
+        final SortedMap<String, String> expectedContents = treeContents(ex);
+        assertTrue(corrupt5(ex)[0] > 0);
+
+        IntegrityCheck icheck = icheck();
+        icheck.setPruneEnabled(true);
+        icheck.checkTree(ex.getTree());
+        assertTrue(icheck.getMarkedVersionCount() > 0);
+        assertTrue(icheck.getPrunedPagesCount() > 0);
+
+        assertEquals("remediation must not change the visible contents", expectedContents, treeContents(ex));
+
+        icheck = icheck();
+        icheck.checkTree(ex.getTree());
+        assertEquals(0, icheck.getMarkedVersionCount());
+        assertEquals(0, icheck.getFaults().length);
+    }
+
+    /**
+     * Issues #290/#292: icheck -P reports leftover mark bits as faults but
+     * must still clear the TransactionIndex - the prune pass has already
+     * rewritten the marks, so they are not a pruning failure.
+     */
+    @Test
+    public void testPruneAndClearWithLeftoverMarkBits() throws Exception {
+        final Exchange ex = _persistit.getExchange(_volumeName, "mvv", true);
+        disableBackgroundCleanup();
+        transactionalStore(ex);
+        _persistit.getTransactionIndex().updateActiveTransactionCache();
+
+        assertTrue(corrupt5(ex)[0] > 0);
+
+        final IntegrityCheck icheck = (IntegrityCheck) CLI.parseTask(_persistit, "icheck trees=* -u -P");
+        final StringWriter output = new StringWriter();
+        icheck.setMessageWriter(new PrintWriter(output));
+        icheck.setup(1, "icheck", "cli", 0, 5);
+        icheck.run();
+
+        assertTrue(icheck.getMarkedVersionCount() > 0);
+        assertTrue(icheck.hasFaults());
+        assertTrue(output.toString(), output.toString().contains("aborted transactions were cleared by pruning"));
+        assertFalse(output.toString(), output.toString().contains("PruneAndClear failed"));
+    }
+
     @Test
     public void testIndexFixHoles() throws Exception {
         final Exchange ex = _persistit.getExchange(_volumeName, "mvv", true);
@@ -283,26 +369,52 @@ private void corrupt1(final Exchange ex) throws PersistitException {
         buffer.release();
     }
 
+    private interface MVVCorruption {
+        /** Corrupts one stored MVV value; returns its contribution to the total */
+        int corrupt(byte[] bytes, int length);
+    }
+
     /**
-     * Corrupts some MVV values on a page by damaging a version length field
-     * 
+     * Applies a corruption to the raw bytes of up to ten stored MVV values
+     *
      * @param ex
+     * @param corruption
+     * @return the number of values corrupted and the summed corruption
+     *         contributions, as a two-element array
      * @throws PersistitException
      */
-    private void corrupt2(final Exchange ex) throws PersistitException {
+    private int[] corruptMVVs(final Exchange ex, final MVVCorruption corruption) throws PersistitException {
         ex.ignoreMVCCFetch(true);
-        ex.clear().to(key(500));
-        int corrupted = 0;
-        while (corrupted < 10 && ex.next()) {
-            final byte[] bytes = ex.getValue().getEncodedBytes();
-            final int length = ex.getValue().getEncodedSize();
-            if (MVV.isArrayMVV(bytes, 0, length)) {
-                bytes[9]++;
-                ex.store();
-                corrupted++;
+        try {
+            ex.clear().to(key(500));
+            int corrupted = 0;
+            int total = 0;
+            while (corrupted < 10 && ex.next()) {
+                final byte[] bytes = ex.getValue().getEncodedBytes();
+                final int length = ex.getValue().getEncodedSize();
+                if (MVV.isArrayMVV(bytes, 0, length)) {
+                    total += corruption.corrupt(bytes, length);
+                    ex.store();
+                    corrupted++;
+                }
             }
+            return new int[] { corrupted, total };
+        } finally {
+            ex.ignoreMVCCFetch(false);
         }
-        ex.ignoreMVCCFetch(false);
+    }
+
+    /**
+     * Corrupts some MVV values on a page by damaging a version length field
+     *
+     * @param ex
+     * @throws PersistitException
+     */
+    private void corrupt2(final Exchange ex) throws PersistitException {
+        corruptMVVs(ex, (bytes, length) -> {
+            bytes[9]++;
+            return 1;
+        });
     }
 
     /**
@@ -338,6 +450,45 @@ private void corrupt4(final Exchange ex) throws PersistitException {
         buffer.release();
     }
 
+    /**
+     * Simulates the pre-#288 interrupted-prune corruption (issue #286) by
+     * setting the leftover mark bit on every version of some MVV values
+     *
+     * @param ex
+     * @return the number of values corrupted and the total number of versions
+     *         marked, as a two-element array
+     * @throws PersistitException
+     */
+    private int[] corrupt5(final Exchange ex) throws PersistitException {
+        return corruptMVVs(ex, (bytes, length) -> {
+            int marked = 0;
+            int from = 1;
+            while (from + MVV.LENGTH_PER_VERSION <= length) {
+                MVV.mark(bytes, from);
+                marked++;
+                from += MVV.getLength(bytes, from) + MVV.LENGTH_PER_VERSION;
+            }
+            return marked;
+        });
+    }
+
+    /**
+     * Returns the visible key/value contents of the tree as seen by a
+     * non-transactional traversal
+     *
+     * @param ex
+     * @return the visible contents, keyed by decoded key string
+     * @throws PersistitException
+     */
+    private SortedMap<String, String> treeContents(final Exchange ex) throws PersistitException {
+        final SortedMap<String, String> contents = new TreeMap<String, String>();
+        ex.clear().append(Key.BEFORE);
+        while (ex.next()) {
+            contents.put(ex.getKey().decodeString(), ex.getValue().getString());
+        }
+        return contents;
+    }
+
     private IntegrityCheck icheck() {
         final IntegrityCheck icheck = new IntegrityCheck(_persistit);
         icheck.setMessageLogVerbosity(Task.LOG_VERBOSE);