[MOD-19365] Bound the default Redis shutdown wait

README.md

@@ -267,3 +267,18 @@ def test_example_3():
     env.assertEqual(con2.get('x'), '1')
 
 ```
+
+### Shutdown grace period
+
+With the default shutdown policy (`terminateRetries=None`), RLTest retries
+SIGTERM once per second for up to 30 seconds, then sends SIGKILL and waits up to
+5 seconds for process exit. Valgrind and sanitizer runs get a 300-second grace
+period to allow for slower saves and exit-time analysis. Explicit
+`terminateRetries`/`terminateRetrySecs` settings retain their existing behavior.
+A forced shutdown under the default policy fails the test even without
+`--check-exitcode`.
+
+On macOS, interactive debugger teardown also bounds the inferior-process wait:
+30 seconds after SIGTERM, followed by SIGKILL and a 5-second wait. An inferior
+left paused at a breakpoint can therefore be killed and reported as a failure;
+teardown no longer waits indefinitely for debugger input.

RLTest/Enterprise/EnterpriseClusterEnv.py

@@ -139,6 +139,11 @@ def broadcast(self, *cmd):
         for shard in self.shards:
             shard.broadcast(*cmd)
 
+    def hasShutdownFailure(self, reset=False):
+        # Visit every shard even if an earlier one failed, to consume all flags.
+        failures = [shard.hasShutdownFailure(reset=reset) for shard in self.shards]
+        return any(failures)
+
     def checkExitCode(self):
         for shard in self.shards:
             if not shard.checkExitCode():

RLTest/__main__.py

@@ -358,10 +358,18 @@ def __init__(self, runner):
         self.runner = runner
 
     def __enter__(self):
-        pass
+        self.runner._pendingResults = []
+        self.runner._teardownFailed = False
 
     def __exit__(self, type, value, traceback):
-        self.runner.takeEnvDown()
+        try:
+            self.runner.takeEnvDown()
+        finally:
+            pending = self.runner._pendingResults
+            self.runner._pendingResults = None
+        if type is None and not self.runner._teardownFailed:
+            for printer, name in pending:
+                printer(name)
 
 class TestTimeLimit(object):
     """
@@ -630,9 +638,11 @@ def takeEnvDown(self, fullShutDown=False):
                 except:
                     flush_ok = False
             self.currEnv.stop()
-            if self.require_clean_exit and self.currEnv and (not self.currEnv.checkExitCode() or not flush_ok):
+            if self.currEnv.hasShutdownFailure(reset=True) or (self.require_clean_exit and (not self.currEnv.checkExitCode() or not flush_ok)):
                 print(Colors.Bred('\tRedis did not exit cleanly'))
                 self.addFailure(self.currEnv.testName, ['redis process failure'])
+                self._teardownFailed = True
+                self.printFail(self.currEnv.testName)
                 if self.args.check_exitcode:
                     raise Exception('Process exited dirty')
             self.currEnv = None
@@ -746,14 +756,16 @@ def _runTest(self, test, numberOfAssertionFailed=0, prefix='', before=lambda x=N
 
         hasException = False
         setup_ok = False
+        skipped = False
         try:
             before_func()
             setup_ok = True
             fn()
             passed = True
         except unittest.SkipTest:
-            self.printSkip(testFullName)
-            return 0
+            # Still run teardown and consume shutdown failures for this test.
+            skipped = True
+            passed = True
         except TestAssertionFailure:
             if self.args.exit_on_failure:
                 self.takeEnvDown(fullShutDown=True)
@@ -796,7 +808,14 @@ def _runTest(self, test, numberOfAssertionFailed=0, prefix='', before=lambda x=N
                 self.handleFailure(testFullName=testFullName, prefix=msgPrefix,
                                    testname=test.name, env=self.currEnv)
                 passed = False
-        elif not hasException:
+            # Attribute a mid-test stop/restart to this test before env reuse
+            # changes testName. Consume only after reporting, never on restart.
+            if self.currEnv.hasShutdownFailure(reset=True):
+                self.addFailure(test.name, ['redis process failure'])
+                self.printFail(testFullName)
+                numFailed += 1
+                passed = False
+        elif not hasException and not skipped:
             self.addFailure(test.name, '<Environment destroyed>')
             passed = False
 
@@ -808,7 +827,10 @@ def _runTest(self, test, numberOfAssertionFailed=0, prefix='', before=lambda x=N
             input('press any button to move to the next test')
 
         if passed:
-            self.printPass(testFullName)
+            if skipped:
+                self.printSkip(testFullName)
+            else:
+                self.printPass(testFullName)
 
         if hasException:
             numFailed += 1 # exception should be counted as failure
@@ -827,6 +849,10 @@ def _closeGitHubActionsTestsGroup(self):
             self.github_actions_group_open = False
 
     def printSkip(self, name):
+        pending = getattr(self, '_pendingResults', None)
+        if pending is not None:
+            pending.append((self.printSkip, name))
+            return
         print('%s:\r\n\t%s' % (Colors.Cyan(name), Colors.Green('[SKIP]')))
 
     def printFail(self, name):
@@ -836,6 +862,10 @@ def printError(self, name):
         print('%s:\r\n\t%s' % (Colors.Cyan(name), Colors.Bred('[ERROR]')))
 
     def printPass(self, name):
+        pending = getattr(self, '_pendingResults', None)
+        if pending is not None:
+            pending.append((self.printPass, name))
+            return
         print('%s:\r\n\t%s' % (Colors.Cyan(name), Colors.Green('[PASS]')))
 
     def envScopeGuard(self):
@@ -879,21 +909,30 @@ def run_single_test(self, test, on_timeout_func):
                             obj = test.create_instance()
 
                     except unittest.SkipTest:
-                        self.printSkip(test.name)
+                        if self.currEnv and self.currEnv.hasShutdownFailure(reset=True):
+                            self.addFailure(test.name, ['redis process failure'])
+                            self.printFail(test.name)
+                        else:
+                            self.printSkip(test.name)
                         return 0
 
                     except Exception as e:
                         self.printException(e)
                         self.addFailure(test.name + " [__init__]")
+                        if self.currEnv and self.currEnv.hasShutdownFailure(reset=True):
+                            self.addFailure(test.name + " [__init__]", ['redis process failure'])
                         return 0
 
                     failures = 0
                     before = getattr(obj, 'setUp', lambda x=None: None)
                     after = getattr(obj, 'tearDown', lambda x=None: None)
                     for subtest in test.get_functions(obj):
                         timeout_handler.reset()
+                        # Only assertion counts belong in the assertion watermark.
+                        # Shutdowns and exceptions contribute to failures separately.
+                        assertions = self.currEnv.getNumberOfFailedAssertion() if self.currEnv else 0
                         failures += self._runTest(subtest, prefix='\t',
-                                                numberOfAssertionFailed=failures,
+                                                numberOfAssertionFailed=assertions,
                                                 before=before, after=after)
                         done += 1
 

RLTest/env.py

@@ -270,11 +270,17 @@ def __init__(self, testName=None, testDescription=None, module=None,
 
         self.startupGraceSecs = startupGraceSecs if startupGraceSecs is not None else Defaults.startup_grace_secs
 
+        # Carry unreported failures across both runner replacement and wrapper reuse.
+        # Leave the old flag intact in case constructing the new environment fails.
+        previous = Env.RTestInstance.currEnv if Env.RTestInstance else None
+        self._previousShutdownFailure = previous.hasShutdownFailure() if previous else False
+
         if not freshEnv and Env.RTestInstance and Env.RTestInstance.currEnv and self.compareEnvs(Env.RTestInstance.currEnv):
             self.envRunner = Env.RTestInstance.currEnv.envRunner
         else:
             if Env.RTestInstance and Env.RTestInstance.currEnv:
                 Env.RTestInstance.currEnv.stop()
+                self._previousShutdownFailure = previous.hasShutdownFailure()
             self.envRunner = self.getEnvByName()
 
         try:
@@ -603,6 +609,15 @@ def debugPrint(self, msg, force=False):
         if Defaults.debug_print or force:
             print('\t' + Colors.Bold('debug:\t') + Colors.Gray(msg))
 
+    def hasShutdownFailure(self, reset=False):
+        # External environments do not own Redis processes.
+        check = getattr(self.envRunner, 'hasShutdownFailure', None)
+        current = check(reset=reset) if check is not None else False
+        previous = getattr(self, '_previousShutdownFailure', False)
+        if reset:
+            self._previousShutdownFailure = False
+        return current or previous
+
     def checkExitCode(self):
         return self.envRunner.checkExitCode()
 

RLTest/redis_cluster.py

@@ -267,6 +267,11 @@ def broadcast(self, *cmd):
         for shard in self.shards:
             shard.broadcast(*cmd)
 
+    def hasShutdownFailure(self, reset=False):
+        # Visit every shard even if an earlier one failed, to consume all flags.
+        failures = [shard.hasShutdownFailure(reset=reset) for shard in self.shards]
+        return any(failures)
+
     def checkExitCode(self):
         for shard in self.shards:
             if not shard.checkExitCode():

RLTest/redis_std.py

@@ -16,6 +16,10 @@
 MASTER = 'master'
 SLAVE = 'slave'
 
+_TERMINATE_TIMEOUT = 30
+_INSTRUMENTED_TERMINATE_TIMEOUT = 300
+_KILL_TIMEOUT = 5
+
 
 class StandardEnv(object):
     def __init__(self, redisBinaryPath, port=6379, modulePath=None, moduleArgs=None, outputFilesFormat=None,
@@ -51,6 +55,7 @@ def __init__(self, redisBinaryPath, port=6379, modulePath=None, moduleArgs=None,
         self.masterProcess = None
         self.masterStdout = None
         self.masterStderr = None
+        self.shutdownFailed = False
         self.masterExitCode = None
         self.slaveProcess = None
         self.slaveStdout = None
@@ -463,27 +468,54 @@ def _stopProcess(self, role):
                     self.verbose_analyse_server_log(role)
             return
         try:
-            if platform.system() == 'Darwin':
-                # On macOS, with lldb, killing lldb process does not terminate inferior processes
-                p0 = psutil.Process(pid=process.pid)
-                pchi = p0.children(recursive=True)
-                for p in pchi:
+            if platform.system() == 'Darwin' and self.has_interactive_debugger:
+                # lldb does not forward termination to its inferiors. Bound the
+                # whole child group wait, rather than waiting per child forever.
+                children = psutil.Process(process.pid).children(recursive=True)
+                for child in children:
                     try:
-                        p.terminate()
-                        p.wait()
-                    except:
+                        child.terminate()
+                    except psutil.NoSuchProcess:
                         pass
+                _, alive = psutil.wait_procs(children, timeout=_TERMINATE_TIMEOUT)
+                if alive:
+                    self.shutdownFailed = True
+                    for child in alive:
+                        try:
+                            child.kill()
+                        except psutil.NoSuchProcess:
+                            pass
+                    _, alive = psutil.wait_procs(alive, timeout=_KILL_TIMEOUT)
+                    if alive:
+                        print(Colors.Bred('[TERMINATING] debugger children survived SIGKILL'))
 
             if self.terminateRetries is None:
-                # ask once, then wait for process to exit
-                process.terminate()
-                termination_start_time = time.time()
-                while process.poll() is None:  # None returns if the processes is not finished yet, retry until redis exits
-                    time.sleep(0.1)
-                    if time.time() - termination_start_time > 30:
-                        # if process is still running after 30 seconds, try reading its output
-                        process_out, process_err = process.communicate()
-                        print(Colors.Bred(f'\t[TERMINATING] out ({process_out}), error ({process_err})'))
+                # Wait on the process, not pipe EOF: Redis fork children can
+                # inherit stdout/stderr and outlive their parent.
+                grace = (_INSTRUMENTED_TERMINATE_TIMEOUT
+                         if self.sanitizer or (self.debugger and not self.has_interactive_debugger)
+                         else _TERMINATE_TIMEOUT)
+                deadline = time.monotonic() + grace
+                while process.poll() is None:
+                    process.terminate()
+                    remaining = deadline - time.monotonic()
+                    if remaining <= 0:
+                        break
+                    try:
+                        process.wait(timeout=min(1, remaining))
+                    except subprocess.TimeoutExpired:
+                        # Redis may refuse shutdown during initial AOF rewrite;
+                        # retry SIGTERM so it can exit cleanly after the rewrite.
+                        continue
+                if process.poll() is None:
+                    self.shutdownFailed = True
+                    print(Colors.Bred('[TERMINATING] {0} server id {1} did not exit on SIGTERM; sending SIGKILL'.format(role, serverId)))
+                    process.kill()
+                    try:
+                        process.wait(timeout=_KILL_TIMEOUT)
+                    except subprocess.TimeoutExpired:
+                        print(Colors.Bred('[TERMINATING] {0} server id {1} did not exit after SIGKILL'.format(role, serverId)))
+                    self._print_shutdown_log(role)
             else:
                 # keep asking every few seconds until process has exited, otherwise kill
                 if self.terminateRetrySecs is None:
@@ -508,6 +540,24 @@ def _stopProcess(self, role):
                 'OSError caught while waiting for {0} process to end: {1}'.format(role, e.__str__())))
             pass
 
+    def hasShutdownFailure(self, reset=False):
+        failed = self.shutdownFailed
+        if reset:
+            self.shutdownFailed = False
+        return failed
+
+    def _print_shutdown_log(self, role):
+        try:
+            path = os.path.join(self.dbDirPath or '', self._getFileName(role, '.log'))
+            with open(path, 'rb') as log:
+                log.seek(0, os.SEEK_END)
+                log.seek(max(0, log.tell() - 8192))
+                print(Colors.Bred('[TERMINATING] last server log bytes ({0}):\n{1}'.format(
+                    path, log.read(8192).decode('utf-8', errors='replace'))))
+        except Exception as error:
+            # Diagnostics must never interrupt teardown, including invalid paths.
+            print(Colors.Bred('[TERMINATING] could not read server log: {0}'.format(error)))
+
     def verbose_analyse_server_log(self, role):
         path = "{0}".format(self._getFileName(role, '.log'))
         if self.dbDirPath is not None: