deploy_rollback: stop leaking the revived process on every rollback_path_b failure

_launch_candidate() spawns a real `python -m src.backend_server` process (`revived`) to re-verify the previous version during a post-cutover rollback. On the success path it's correctly left running -- it IS the new Live. But on every failure path (never bound live_port, failed re-verification, git revert conflict) nothing ever terminated it -- found via 31 accumulated orphaned processes on this machine, one per failed rollback_path_b call across many real test runs this session. This is also the likely cause of RollbackPathBFailureTest's own reported non-determinism (three independent real Validator sessions saw it fail, never reproduced manually): a leaked process from an earlier failed run can still be squatting on the port a later run's _find_free_port() picks, producing a false "alive" health-check reading. Fix: call deploy_cutover._stop_process(revived.pid) in both failure branches before raising, mirroring the pattern cutover() already uses for its own stop_timeout-bounded process teardown. Also fixes RollbackPathBHappyPathTest's own matching leak (it re-appended bad_live.pid a second time instead of tracking the actual revived process, a copy-paste bug, not real cleanup) by capturing the real pid via a thin _launch_candidate wrapper. 31 pre-existing orphaned processes killed by hand; verified 5 repeated test runs after the fix leak zero new ones. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

deploy_rollback: stop leaking the revived process on every rollback_path_b failure
_launch_candidate() spawns a real `python -m src.backend_server` process (`revived`) to re-verify the previous version during a post-cutover rollback. On the success path it's correctly left running -- it IS the new Live. But on every failure path (never bound live_port, failed re-verification, git revert conflict) nothing ever terminated it -- found via 31 accumulated orphaned processes on this machine, one per failed rollback_path_b call across many real test runs this session. This is also the likely cause of RollbackPathBFailureTest's own reported non-determinism (three independent real Validator sessions saw it fail, never reproduced manually): a leaked process from an earlier failed run can still be squatting on the port a later run's _find_free_port() picks, producing a false "alive" health-check reading. Fix: call deploy_cutover._stop_process(revived.pid) in both failure branches before raising, mirroring the pattern cutover() already uses for its own stop_timeout-bounded process teardown. Also fixes RollbackPathBHappyPathTest's own matching leak (it re-appended bad_live.pid a second time instead of tracking the actual revived process, a copy-paste bug, not real cleanup) by capturing the real pid via a thin _launch_candidate wrapper. 31 pre-existing orphaned processes killed by hand; verified 5 repeated test runs after the fix leak zero new ones. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bboonnee
1 parent 701f6137
Showing 2 changed files with 36 additions and 7 deletions
supervisor/deploy_rollback.py
supervisor/test_deploy_rollback.py
@@ -141,6 +141,7 @@ def rollback_path_b(
     conn.execute("UPDATE deployment SET status = 'rollback_in_progress' WHERE deployment_id = ?", (deployment_id,))
     conn.commit()
+    revived = None
     try:
         deploy_cutover._stop_process(live_pid, timeout=stop_timeout)
@@ -167,12 +168,26 @@ def rollback_path_b(
         _git_revert(repo_root, git_commit_hash)
     except subprocess.CalledProcessError as e:
+        # 2026-08-05 real bug found: `revived` was never stopped on any
+        # failure path -- every failed rollback_path_b call leaked a real
+        # `python -m src.backend_server` process forever (found via 31
+        # accumulated orphans on this machine, and very likely the cause of
+        # RollbackPathBFailureTest's own reported non-determinism: a stale
+        # leaked process from an earlier failed run can still be squatting
+        # on the port a later run's `_find_free_port()` picks, producing a
+        # false "alive" health-check reading). On the SUCCESS path `revived`
+        # is deliberately left running -- it IS the new Live -- so this
+        # cleanup only belongs in the failure branches.
+        if revived is not None:
+            deploy_cutover._stop_process(revived.pid, timeout=stop_timeout)
         _abort_revert_if_in_progress(repo_root)
         _block(conn, deployment_id, "rollback_failed (path B): git revert failed: {}".format(e.stderr))
         raise RollbackFailedError(
             "rollback path B failed for deployment {}: git revert error: {}".format(deployment_id, e.stderr)
         ) from e
     except RollbackFailedError as e:
+        if revived is not None:
+            deploy_cutover._stop_process(revived.pid, timeout=stop_timeout)
         _block(conn, deployment_id, "rollback_failed (path B): {}".format(e))
         raise
@@ -15,6 +15,7 @@ import sys
 import tempfile
 import time
 import unittest
+from unittest.mock import patch
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
@@ -222,13 +223,26 @@ class RollbackPathBHappyPathTest(DeployRollbackTestBase):
         )
         rollback_port = deploy_stage._find_free_port()
-        result = deploy_rollback.rollback_path_b(
-            self.conn, "dep-b1", bad_live.pid, self.prev_dir, self.prod_db_path, self.commit_v2,
-            repo_root=self.repo_dir, live_port=rollback_port,
-        )
-        self._cleanup_pids.append(  # find the revived process's pid via port not tracked -- best-effort cleanup
-            bad_live.pid
-        )
+        # 2026-08-05: capture the real revived process's pid for cleanup --
+        # rollback_path_b()'s own return value doesn't expose it (by design,
+        # it's meant to become the new Live, not a tracked handle), and this
+        # test used to just re-append bad_live.pid a second time here
+        # (a copy-paste mistake, not real cleanup), leaking the revived
+        # process on every happy-path test run.
+        real_launch = deploy_stage._launch_candidate
+        revived_holder = {}
+
+        def _capturing_launch(*args, **kwargs):
+            proc = real_launch(*args, **kwargs)
+            revived_holder["proc"] = proc
+            return proc
+
+        with patch.object(deploy_stage, "_launch_candidate", side_effect=_capturing_launch):
+            result = deploy_rollback.rollback_path_b(
+                self.conn, "dep-b1", bad_live.pid, self.prev_dir, self.prod_db_path, self.commit_v2,
+                repo_root=self.repo_dir, live_port=rollback_port,
+            )
+        self._cleanup_pids.append(revived_holder["proc"].pid)
         self.assertEqual(result["status"], "rolled_back")
         self.assertFalse(deploy_supervisor.is_alive(bad_live.pid))