From 2c20c1a5d63e31d42b92ceb1347d41bf0e4a1638 Mon Sep 17 00:00:00 2001 From: speriaswamy-amd Date: Fri, 14 Aug 2026 15:16:07 -0400 Subject: [PATCH 1/3] fix(runner): tear down resources after a partially-successful setup BaseRunner.execute() only set _setup_complete after setup() returned True, so the finally block's teardown() was skipped whenever setup() failed. A multi-node setup that launched containers on nodes 0..N-1 before failing on node N therefore leaked every container it had already created. Set _setup_complete as soon as setup() has been attempted, so teardown() runs for partial setups too. teardown() implementations already tolerate being called with partially-populated state. Adds cvs/runners/unittests/ (the package had no unit tests) with four cases pinning the lifecycle: teardown after success, after setup failure, after run() raises, and not before setup is attempted. Co-Authored-By: Claude --- .gitignore | 1 + cvs/runners/_base_runner.py | 7 +- cvs/runners/unittests/__init__.py | 0 cvs/runners/unittests/test_base_runner.py | 79 +++++++++++++++++++++++ 4 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 cvs/runners/unittests/__init__.py create mode 100644 cvs/runners/unittests/test_base_runner.py diff --git a/.gitignore b/.gitignore index 273ca0eb2..dec2cc930 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,7 @@ __pycache__/ *.egg-info/ # Virtual environments +.venv/ .test_venv/ .cvs_venv/ .ruff_venv/ diff --git a/cvs/runners/_base_runner.py b/cvs/runners/_base_runner.py index 714c2bed0..9a4e6b27b 100644 --- a/cvs/runners/_base_runner.py +++ b/cvs/runners/_base_runner.py @@ -184,11 +184,14 @@ def execute(self, **kwargs) -> RunResult: try: # Setup phase log.info(f"Setting up {self.__class__.__name__}...") - if not self.setup(): + setup_ok = self.setup() + # Set even on partial/failed setup so the finally block below still + # tears down resources a partially-successful setup already created. + self._setup_complete = True + if not setup_ok: return RunResult( status=RunStatus.FAILED, start_time=start_time, end_time=time.time(), error_message="Setup failed" ) - self._setup_complete = True # Run phase log.info(f"Running {self.__class__.__name__}...") diff --git a/cvs/runners/unittests/__init__.py b/cvs/runners/unittests/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/cvs/runners/unittests/test_base_runner.py b/cvs/runners/unittests/test_base_runner.py new file mode 100644 index 000000000..d6054348b --- /dev/null +++ b/cvs/runners/unittests/test_base_runner.py @@ -0,0 +1,79 @@ +""" +Unit tests for BaseRunner.execute()'s setup -> run -> teardown lifecycle. + +Copyright 2025 Advanced Micro Devices, Inc. +All rights reserved. +""" + +import unittest + +from cvs.runners._base_runner import BaseRunner, RunConfig, RunResult, RunStatus + + +class _FakeRunner(BaseRunner): + """Minimal concrete BaseRunner for exercising execute().""" + + def __init__(self, config, setup_return=True, run_return=None, run_raises=None): + super().__init__(config) + self._setup_return = setup_return + self._run_return = run_return + self._run_raises = run_raises + self.teardown_calls = 0 + + def setup(self) -> bool: + return self._setup_return + + def run(self, **kwargs) -> RunResult: + if self._run_raises is not None: + raise self._run_raises + return self._run_return + + def teardown(self) -> bool: + self.teardown_calls += 1 + return True + + +def _config() -> RunConfig: + return RunConfig(nodes=["10.0.0.1"], username="testuser") + + +class TestExecuteTeardownLifecycle(unittest.TestCase): + def test_teardown_runs_after_successful_setup_and_run(self): + run_result = RunResult(status=RunStatus.COMPLETED, start_time=0, end_time=1) + runner = _FakeRunner(_config(), setup_return=True, run_return=run_result) + + result = runner.execute() + + self.assertEqual(result.status, RunStatus.COMPLETED) + self.assertEqual(runner.teardown_calls, 1) + + def test_teardown_runs_when_setup_fails(self): + runner = _FakeRunner(_config(), setup_return=False) + + result = runner.execute() + + self.assertEqual(result.status, RunStatus.FAILED) + self.assertEqual(result.error_message, "Setup failed") + self.assertEqual(runner.teardown_calls, 1) + + def test_teardown_runs_when_run_raises(self): + runner = _FakeRunner(_config(), setup_return=True, run_raises=RuntimeError("boom")) + + result = runner.execute() + + self.assertEqual(result.status, RunStatus.FAILED) + self.assertIn("boom", result.error_message) + self.assertEqual(runner.teardown_calls, 1) + + def test_teardown_not_run_before_setup_attempted(self): + runner = _FakeRunner( + _config(), setup_return=True, run_return=RunResult(status=RunStatus.COMPLETED, start_time=0, end_time=1) + ) + + self.assertFalse(runner._setup_complete) + runner.execute() + self.assertTrue(runner._setup_complete) + + +if __name__ == "__main__": + unittest.main() From 6381ed2af9a51b8fbab1dc446b233ff87c0276d1 Mon Sep 17 00:00:00 2001 From: speriaswamy-amd Date: Tue, 8 Sep 2026 11:18:04 -0400 Subject: [PATCH 2/3] fix(ci): match branch names containing slashes in pull_request trigger branches: ['*'] does not cross '/' in GitHub Actions glob matching, so pull_request events targeting a namespaced branch (e.g. surya/foo) never triggered this workflow. Use '**' which matches across path segments. --- .github/workflows/ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9baaa9c19..298c38681 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,7 +3,8 @@ name: CI on: pull_request: types: [opened, synchronize, reopened] - branches: ['*'] + branches: ['**'] + workflow_dispatch: {} jobs: test: From 6c5fae52a70cba03812452c308811d88c83c6ca8 Mon Sep 17 00:00:00 2001 From: speriaswamy-amd Date: Thu, 10 Sep 2026 15:35:55 -0400 Subject: [PATCH 3/3] fix(runner): keep teardown reachable when setup() raises _setup_complete was only set after setup() returned, so an exception raised inside setup() (after it had already created resources) skipped the finally block's teardown call entirely, reintroducing the leak this lifecycle was meant to close. Set the flag in a finally around the setup() call so it always fires. --- cvs/runners/_base_runner.py | 11 +++++++---- cvs/runners/unittests/test_base_runner.py | 14 +++++++++++++- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/cvs/runners/_base_runner.py b/cvs/runners/_base_runner.py index 9a4e6b27b..80a9a2e1f 100644 --- a/cvs/runners/_base_runner.py +++ b/cvs/runners/_base_runner.py @@ -184,10 +184,13 @@ def execute(self, **kwargs) -> RunResult: try: # Setup phase log.info(f"Setting up {self.__class__.__name__}...") - setup_ok = self.setup() - # Set even on partial/failed setup so the finally block below still - # tears down resources a partially-successful setup already created. - self._setup_complete = True + try: + setup_ok = self.setup() + finally: + # Set even if setup() raises or partially succeeds, so the + # finally block below still tears down resources a + # partially-successful setup already created. + self._setup_complete = True if not setup_ok: return RunResult( status=RunStatus.FAILED, start_time=start_time, end_time=time.time(), error_message="Setup failed" diff --git a/cvs/runners/unittests/test_base_runner.py b/cvs/runners/unittests/test_base_runner.py index d6054348b..890e0d05a 100644 --- a/cvs/runners/unittests/test_base_runner.py +++ b/cvs/runners/unittests/test_base_runner.py @@ -13,14 +13,17 @@ class _FakeRunner(BaseRunner): """Minimal concrete BaseRunner for exercising execute().""" - def __init__(self, config, setup_return=True, run_return=None, run_raises=None): + def __init__(self, config, setup_return=True, setup_raises=None, run_return=None, run_raises=None): super().__init__(config) self._setup_return = setup_return + self._setup_raises = setup_raises self._run_return = run_return self._run_raises = run_raises self.teardown_calls = 0 def setup(self) -> bool: + if self._setup_raises is not None: + raise self._setup_raises return self._setup_return def run(self, **kwargs) -> RunResult: @@ -56,6 +59,15 @@ def test_teardown_runs_when_setup_fails(self): self.assertEqual(result.error_message, "Setup failed") self.assertEqual(runner.teardown_calls, 1) + def test_teardown_runs_when_setup_raises(self): + runner = _FakeRunner(_config(), setup_raises=RuntimeError("setup exploded")) + + result = runner.execute() + + self.assertEqual(result.status, RunStatus.FAILED) + self.assertIn("setup exploded", result.error_message) + self.assertEqual(runner.teardown_calls, 1) + def test_teardown_runs_when_run_raises(self): runner = _FakeRunner(_config(), setup_return=True, run_raises=RuntimeError("boom"))