From 9fd769ab79a2e9173dc23145fae3310b3206729d Mon Sep 17 00:00:00 2001 From: codex Date: Sun, 26 Jul 2026 22:43:02 +0000 Subject: [PATCH] fix(firecracker): abort cleanup on scan errors --- bot_bottle/backend/firecracker/cleanup.py | 22 +++++++++++++---- tests/unit/test_firecracker_cleanup.py | 30 ++++++++++++++++++++++- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/bot_bottle/backend/firecracker/cleanup.py b/bot_bottle/backend/firecracker/cleanup.py index bce6c9d6..ac3e855f 100644 --- a/bot_bottle/backend/firecracker/cleanup.py +++ b/bot_bottle/backend/firecracker/cleanup.py @@ -29,6 +29,7 @@ import subprocess from pathlib import Path from ...log import info +from .. import EnumerationError from . import util from .bottle_cleanup_plan import FirecrackerBottleCleanupPlan @@ -62,12 +63,23 @@ def _scan_processes(run_root: Path) -> tuple[set[str], list[int]]: * ``orphan_pids`` — firecracker pids whose run dir no longer exists (a lingering VMM to kill). """ - result = subprocess.run( - ["pgrep", "-a", "firecracker"], - capture_output=True, text=True, check=False, - ) - if result.returncode != 0: + try: + result = subprocess.run( + ["pgrep", "-a", "firecracker"], + capture_output=True, text=True, check=False, + ) + except OSError as exc: + raise EnumerationError( + f"could not enumerate Firecracker processes: {exc}" + ) from exc + if result.returncode == 1: + # pgrep's documented "no processes matched" result. return set(), [] + if result.returncode != 0: + detail = (result.stderr or "").strip() or f"exit {result.returncode}" + raise EnumerationError( + f"could not enumerate Firecracker processes: {detail}" + ) live: set[str] = set() orphan_pids: list[int] = [] for line in result.stdout.splitlines(): diff --git a/tests/unit/test_firecracker_cleanup.py b/tests/unit/test_firecracker_cleanup.py index a66660d4..c6cd55c9 100644 --- a/tests/unit/test_firecracker_cleanup.py +++ b/tests/unit/test_firecracker_cleanup.py @@ -15,6 +15,7 @@ import unittest from pathlib import Path from unittest.mock import patch +from bot_bottle.backend import EnumerationError from bot_bottle.backend.firecracker import cleanup as fc_cleanup from bot_bottle.backend.firecracker.bottle_cleanup_plan import ( FirecrackerBottleCleanupPlan, @@ -58,10 +59,37 @@ class TestProcessScan(unittest.TestCase): self.assertEqual({str(run_root / "live-a")}, live) self.assertEqual([222], orphan_pids) - def test_scan_empty_when_pgrep_fails(self): + def test_scan_empty_when_pgrep_finds_no_processes(self): with patch.object(fc_cleanup.subprocess, "run", return_value=_proc(returncode=1)): self.assertEqual((set(), []), fc_cleanup._scan_processes(Path("/x"))) + def test_scan_raises_when_pgrep_errors(self): + proc = subprocess.CompletedProcess( + [], 2, stdout="", stderr="invalid process expression", + ) + with ( + patch.object(fc_cleanup.subprocess, "run", return_value=proc), + self.assertRaisesRegex(EnumerationError, "invalid process expression"), + ): + fc_cleanup._scan_processes(Path("/x")) + + def test_prepare_cleanup_does_not_plan_deletions_when_scan_errors(self): + with tempfile.TemporaryDirectory() as tmp: + run_root = Path(tmp) + live = run_root / "live-a" + live.mkdir() + with ( + patch.object(fc_cleanup, "_run_root", return_value=run_root), + patch.object( + fc_cleanup.subprocess, + "run", + return_value=_proc(returncode=2), + ), + self.assertRaises(EnumerationError), + ): + fc_cleanup.prepare_cleanup() + self.assertTrue(live.is_dir()) + def test_live_run_dirs_returns_paths_in_stable_order(self): with patch.object(fc_cleanup, "_run_root", return_value=Path("/run")), \ patch.object(fc_cleanup, "_scan_processes",