Fix hang when buggy (unaltered) OK comes after STOP

Make sure that we don't proceed with an unmodified file - for this,
consistently validate the pass' results on both codepaths (in
process_done_futures() and wait_for_first_success()).

Besides fixing the hang that was possible when a pass misbehaved,
there are a couple of other side effects:

* more cases of ERRORs in a pass will be reported - previously only
  the ones on the first codepath were handled;
* max_improvement is enforced stricter now - previously the second
  codepath allowed a larger reduction to sneak in.

This fixes marxin#156.

cvise/tests/test_test_manager.py

@@ -2,6 +2,7 @@
 import os
 import pytest
 import shutil
+import time
 from unittest.mock import patch
 
 from cvise.passes.abstract import AbstractPass, PassResult  # noqa: E402
@@ -80,6 +81,18 @@ def transform(self, test_case, state, process_event_notifier):
         return (PassResult.OK, state)
 
 
+class SlowUnalteredThenStoppingPass(StubPass):
+    """Attempts to simulate the "STOP event observed before a buggy OK" scenario."""
+
+    DELAY_SECS = 1  # the larger the number, the higher the chance of catching bugs
+
+    def transform(self, test_case, state, process_event_notifier):
+        if state == 0:
+            time.sleep(self.DELAY_SECS)
+            return (PassResult.OK, state)
+        return (PassResult.STOP, state)
+
+
 def read_file(path):
     with open(path) as f:
         return f.read()
@@ -203,3 +216,12 @@ def test_halt_on_unaltered(input_file, manager):
     assert read_file(input_file) == INPUT_DATA
     # This number of "failed to modify the variant" reports were to be created.
     assert bug_dir_count() == testing.TestManager.MAX_CRASH_DIRS + 1
+
+
+def test_halt_on_unaltered_after_stop(input_file, manager):
+    """Check that we quit after the pass' stop, even if it interleaved with a misbehave."""
+    p = SlowUnalteredThenStoppingPass()
+    manager.run_pass(p)
+    assert read_file(input_file) == INPUT_DATA
+    # Whether the misbehave ("failed to modify the variant") is detected depends on timing.
+    assert bug_dir_count() <= 1

cvise/utils/testing.py

@@ -1,5 +1,6 @@
 from concurrent.futures import FIRST_COMPLETED, wait
 import difflib
+from enum import auto, Enum, unique
 import filecmp
 import logging
 import math
@@ -32,6 +33,14 @@
 MAX_PASS_INCREASEMENT_THRESHOLD = 3
 
 
+@unique
+class PassCheckingOutcome(Enum):
+    """Outcome of checking the result of an invocation of a pass."""
+    ACCEPT = auto()
+    IGNORE = auto()
+    STOP = auto()
+
+
 def rmfolder(name):
     assert 'cvise' in str(name)
     try:
@@ -406,33 +415,14 @@ def process_done_futures(self):
                         raise future.exception()
 
                 test_env = future.result()
-                if test_env.success:
-                    if self.max_improvement is not None and test_env.size_improvement > self.max_improvement:
-                        logging.debug(f'Too large improvement: {test_env.size_improvement} B')
-                    else:
-                        # Report bug if transform did not change the file
-                        if filecmp.cmp(self.current_test_case, test_env.test_case_path):
-                            if not self.silent_pass_bug:
-                                if not self.report_pass_bug(test_env, 'pass failed to modify the variant'):
-                                    quit_loop = True
-                        else:
-                            quit_loop = True
-                            new_futures.add(future)
-                else:
-                    self.pass_statistic.add_failure(self.current_pass)
-                    if test_env.result == PassResult.OK:
-                        assert test_env.exitcode
-                        if self.also_interesting is not None and test_env.exitcode == self.also_interesting:
-                            self.save_extra_dir(test_env.test_case_path)
-                    elif test_env.result == PassResult.STOP:
-                        quit_loop = True
-                    elif test_env.result == PassResult.ERROR:
-                        if not self.silent_pass_bug:
-                            self.report_pass_bug(test_env, 'pass error')
-                            quit_loop = True
-                    if not self.no_give_up and test_env.order > self.GIVEUP_CONSTANT:
-                        self.report_pass_bug(test_env, 'pass got stuck')
-                        quit_loop = True
+                outcome = self.check_pass_result(test_env)
+                if outcome == PassCheckingOutcome.ACCEPT:
+                    quit_loop = True
+                    new_futures.add(future)
+                elif outcome == PassCheckingOutcome.IGNORE:
+                    pass
+                elif outcome == PassCheckingOutcome.STOP:
+                    quit_loop = True
             else:
                 new_futures.add(future)
 
@@ -446,13 +436,45 @@ def wait_for_first_success(self):
         for future in self.futures:
             try:
                 test_env = future.result()
-                if test_env.success:
+                outcome = self.check_pass_result(test_env)
+                if outcome == PassCheckingOutcome.ACCEPT:
                     return test_env
             # starting with Python 3.11: concurrent.futures.TimeoutError == TimeoutError
             except (TimeoutError, concurrent.futures.TimeoutError):
                 pass
         return None
 
+    def check_pass_result(self, test_env):
+        if test_env.success:
+            if self.max_improvement is not None and test_env.size_improvement > self.max_improvement:
+                logging.debug(f'Too large improvement: {test_env.size_improvement} B')
+                return PassCheckingOutcome.IGNORE
+            # Report bug if transform did not change the file
+            if filecmp.cmp(self.current_test_case, test_env.test_case_path):
+                if not self.silent_pass_bug:
+                    if not self.report_pass_bug(test_env, 'pass failed to modify the variant'):
+                        return PassCheckingOutcome.STOP
+                return PassCheckingOutcome.IGNORE
+            return PassCheckingOutcome.ACCEPT
+
+        self.pass_statistic.add_failure(self.current_pass)
+        if test_env.result == PassResult.OK:
+            assert test_env.exitcode
+            if self.also_interesting is not None and test_env.exitcode == self.also_interesting:
+                self.save_extra_dir(test_env.test_case_path)
+        elif test_env.result == PassResult.STOP:
+            return PassCheckingOutcome.STOP
+        elif test_env.result == PassResult.ERROR:
+            if not self.silent_pass_bug:
+                self.report_pass_bug(test_env, 'pass error')
+                return PassCheckingOutcome.STOP
+        if not self.no_give_up and test_env.order > self.GIVEUP_CONSTANT:
+            if not self.giveup_reported:
+                self.report_pass_bug(test_env, 'pass got stuck')
+                self.giveup_reported = True
+            return PassCheckingOutcome.STOP
+        return PassCheckingOutcome.IGNORE
+
     @classmethod
     def terminate_all(cls, pool):
         pool.stop()
@@ -464,6 +486,7 @@ def run_parallel_tests(self):
         with pebble.ProcessPool(max_workers=self.parallel_tests) as pool:
             order = 1
             self.timeout_count = 0
+            self.giveup_reported = False
             while self.state is not None:
                 # do not create too many states
                 if len(self.futures) >= self.parallel_tests: