From 147be052f1c7182e4fdc6fcec82586c8d319e6b7 Mon Sep 17 00:00:00 2001 From: bboonnee Date: Thu, 6 Aug 2026 00:01:27 +0800 Subject: [PATCH] deploy_rollback: stop leaking the revived process on every rollback_path_b failure --- supervisor/deploy_rollback.py | 15 +++++++++++++++ supervisor/test_deploy_rollback.py | 28 +++++++++++++++++++++------- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/supervisor/deploy_rollback.py b/supervisor/deploy_rollback.py index 2ba4e52..21e646f 100644 --- a/supervisor/deploy_rollback.py +++ b/supervisor/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 diff --git a/supervisor/test_deploy_rollback.py b/supervisor/test_deploy_rollback.py index 48e6563..551f8e3 100644 --- a/supervisor/test_deploy_rollback.py +++ b/supervisor/test_deploy_rollback.py @@ -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)) -- libgit2 0.21.4