Compare fault counts in the icheck -P gate, not the capped fault list

The pruneAndClear gate excluded marked-version faults by comparing
_faults.size() with the marked-fault count, but both stop moving once
the fault list reaches MAX_FAULTS: on a volume with more than
MAX_FAULTS marked-version records a genuine fault found afterwards
(the garbage chain is checked after the trees) is dropped from the
list and the arithmetic still converges, so -P cleared the
TransactionIndex despite real corruption. Count every fault found -
recorded in the list or not - and gate on the counts instead.

Author: vharseko

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

@@ -80,6 +80,7 @@ public class IntegrityCheck extends Task {
     private boolean _csv;
 
     private final ArrayList<Fault> _faults = new ArrayList<Fault>();
+    private int _faultCount;
     private int _markedVersionFaultCount;
     private final ArrayList<CleanupIndexHole> _holes = new ArrayList<CleanupIndexHole>();
 
@@ -201,10 +202,9 @@ protected void visitDataRecord(final Key key, final int foundAt, final int tail,
             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++;
-                }
+                _markedVersionFaultCount++;
+                addFault("MVV has " + plural(marked, "version") + " with a leftover mark bit", _currentPage, 0,
+                        foundAt);
             }
             if (_versionVisitor._count > 0) {
                 _counters._mvvCount++;
@@ -314,9 +314,13 @@ protected void runTask() {
                  * 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.
+                 * faults and incomplete pruning count as failures here. The
+                 * comparison uses fault counts, not the _faults list: the
+                 * list is capped at MAX_FAULTS, so a genuine fault found
+                 * after marked-version faults fill the cap would be missing
+                 * from the list but must still block the clear.
                  */
-                if (_faults.size() == _markedVersionFaultCount && _counters._mvvPageCount == _counters._prunedPageCount
+                if (_faultCount == _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);
@@ -356,17 +360,17 @@ private String plural(final int n, final String m) {
         }
     }
 
-    private boolean addFault(final String description, final long page, final int level, final int position) {
+    private void 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);
-        final boolean recorded = _faults.size() < MAX_FAULTS;
-        if (recorded)
+        _faultCount++;
+        if (_faults.size() < MAX_FAULTS)
             _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) {
         final Fault fault = new Fault(resourceName(), this, description, page, 3, level, position);
+        _faultCount++;
         if (_faults.size() < MAX_FAULTS)
             _faults.add(fault);
         postMessage(fault.toString(), LOG_VERBOSE);

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

@@ -101,6 +101,12 @@ public void testBrokenMVVs() throws Exception {
         final IntegrityCheck icheck = icheck();
         icheck.checkTree(ex.getTree());
         assertTrue(icheck.getFaults().length > 0);
+        /*
+         * A structurally corrupt MVV must not inflate the marked-version
+         * counter: the leftover-mark scan runs only after visitAllVersions
+         * has validated the record.
+         */
+        assertEquals(0, icheck.getMarkedVersionCount());
     }
 
     /**
@@ -185,6 +191,34 @@ public void testPruneAndClearWithLeftoverMarkBits() throws Exception {
         assertFalse(output.toString(), output.toString().contains("PruneAndClear failed"));
     }
 
+    /**
+     * The -P gate must compare fault counts, not the MAX_FAULTS-capped fault
+     * list: with more than MAX_FAULTS marked-version faults the list holds
+     * only them, and a genuine fault found later (here a corrupt garbage
+     * chain, checked after the trees) is not recorded in the list - it must
+     * still block clearing the TransactionIndex.
+     */
+    @Test
+    public void testPruneAndClearBlockedByFaultBeyondFaultCap() throws Exception {
+        final Exchange ex = _persistit.getExchange(_volumeName, "mvv", true);
+        disableBackgroundCleanup();
+        transactionalStore(ex);
+        _persistit.getTransactionIndex().updateActiveTransactionCache();
+
+        assertTrue(corrupt5(ex, 0, IntegrityCheck.MAX_FAULTS + 1)[0] > IntegrityCheck.MAX_FAULTS);
+        corrupt4(ex);
+
+        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(output.toString(), output.toString().contains("PruneAndClear failed"));
+        assertFalse(output.toString(), output.toString().contains("aborted transactions were cleared by pruning"));
+    }
+
     @Test
     public void testIndexFixHoles() throws Exception {
         final Exchange ex = _persistit.getExchange(_volumeName, "mvv", true);
@@ -376,6 +410,7 @@ private interface MVVCorruption {
 
     /**
      * Applies a corruption to the raw bytes of up to ten stored MVV values
+     * following key 500
      *
      * @param ex
      * @param corruption
@@ -384,12 +419,29 @@ private interface MVVCorruption {
      * @throws PersistitException
      */
     private int[] corruptMVVs(final Exchange ex, final MVVCorruption corruption) throws PersistitException {
+        return corruptMVVs(ex, 500, 10, corruption);
+    }
+
+    /**
+     * Applies a corruption to the raw bytes of up to <code>limit</code>
+     * stored MVV values following <code>startKey</code>
+     *
+     * @param ex
+     * @param startKey
+     * @param limit
+     * @param corruption
+     * @return the number of values corrupted and the summed corruption
+     *         contributions, as a two-element array
+     * @throws PersistitException
+     */
+    private int[] corruptMVVs(final Exchange ex, final int startKey, final int limit, final MVVCorruption corruption)
+            throws PersistitException {
         ex.ignoreMVCCFetch(true);
         try {
-            ex.clear().to(key(500));
+            ex.clear().to(key(startKey));
             int corrupted = 0;
             int total = 0;
-            while (corrupted < 10 && ex.next()) {
+            while (corrupted < limit && ex.next()) {
                 final byte[] bytes = ex.getValue().getEncodedBytes();
                 final int length = ex.getValue().getEncodedSize();
                 if (MVV.isArrayMVV(bytes, 0, length)) {
@@ -460,7 +512,15 @@ private void corrupt4(final Exchange ex) throws PersistitException {
      * @throws PersistitException
      */
     private int[] corrupt5(final Exchange ex) throws PersistitException {
-        return corruptMVVs(ex, (bytes, length) -> {
+        return corrupt5(ex, 500, 10);
+    }
+
+    /**
+     * Same as {@link #corrupt5(Exchange)} for up to <code>limit</code> values
+     * following <code>startKey</code>
+     */
+    private int[] corrupt5(final Exchange ex, final int startKey, final int limit) throws PersistitException {
+        return corruptMVVs(ex, startKey, limit, (bytes, length) -> {
             int marked = 0;
             int from = 1;
             while (from + MVV.LENGTH_PER_VERSION <= length) {