From 1361c0b5359ac9898d554a674a23ddb382398b41 Mon Sep 17 00:00:00 2001 From: Bai Li Date: Fri, 17 Jul 2026 11:10:05 -0700 Subject: [PATCH 1/3] fix(sandbox): prune capture-ignored entries on every preservation path; interrupt-proof teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps surfaced by a lost nightly (a preserved workspace's dangling .venv/bin/python symlinks broke the run-dir artifact publish): 1. Only capture_to filtered _WORKSPACE_CAPTURE_IGNORE. preserve_to (MOVE_ON_WRITE) moved the raw workspace and DIRECT_WRITE — the docker driver's default — never copies, so agent-created .venv/node_modules (sandbox-only symlinks) and credential-store names persisted verbatim in run_dir/artifacts. All three paths now prune the same ignore set. 2. A task-timeout CancelledError delivered while _run_post_run_commands awaits aborted run()'s finally block wholesale: _cleanup() was skipped (tempdir leaked, workspace never pruned/granted) and _finalize_result() was skipped (task.json lost). Teardown now catches the interrupt, completes cleanup + finalize, then re-raises it. Co-Authored-By: Claude Fable 5 --- src/coder_eval/orchestrator.py | 43 ++++- src/coder_eval/sandbox.py | 70 ++++++++ tests/test_preserved_workspace_prune.py | 208 ++++++++++++++++++++++++ 3 files changed, 313 insertions(+), 8 deletions(-) create mode 100644 tests/test_preserved_workspace_prune.py diff --git a/src/coder_eval/orchestrator.py b/src/coder_eval/orchestrator.py index 0c629c95..c0416e81 100644 --- a/src/coder_eval/orchestrator.py +++ b/src/coder_eval/orchestrator.py @@ -557,10 +557,30 @@ def _kill_agent_subprocess_sync() -> None: logger.error(f"Evaluation failed: {e}", exc_info=True) finally: - # BEFORE post-run/cleanup: needs the live sandbox to resolve - # the agent-aligned `uip`, and post-task tool state on disk. - self._refresh_runtime_tool_versions() - await self._run_post_run_commands() + # Teardown must be interrupt-proof: the task-timeout watchdog can + # fire while post-run commands are awaiting and deliver its + # CancelledError right here in the finally block, which used to + # abort it wholesale — skipping _cleanup() (tempdir leaked; a + # DIRECT_WRITE workspace kept agent-created .venv/node_modules + # whose dangling symlinks later broke artifact publishing) AND + # _finalize_result() (task.json lost). Catch the interrupt, + # finish the full teardown, then re-raise it at the end so + # callers observe the same exception as before. The watchdog + # cancels exactly once, so the teardown awaits below run + # normally after the CancelledError is caught. + teardown_interrupt: BaseException | None = None + try: + # BEFORE post-run/cleanup: needs the live sandbox to resolve + # the agent-aligned `uip`, and post-task tool state on disk. + self._refresh_runtime_tool_versions() + await self._run_post_run_commands() + except (Exception, asyncio.CancelledError) as e: + teardown_interrupt = e + logger.warning( + "Teardown interrupted during post-run (%s: %s); completing cleanup before re-raising", + type(e).__name__, + e, + ) await self._cleanup() # Capture the sanitised log tail AFTER teardown so any errors # logged during post-run / cleanup also land in the report, @@ -576,6 +596,8 @@ def _kill_agent_subprocess_sync() -> None: }: self.result.error_log_tail = log_tail.get_text() or None self._finalize_result(start_time) + if teardown_interrupt is not None: + raise teardown_interrupt return self.result @@ -2094,11 +2116,16 @@ async def _cleanup(self) -> None: logger.info(f"Sandbox preserved to: {preserved_path}") elif self.preservation_mode == PreservationMode.DIRECT_WRITE and self.result: # Sandbox already lives in run_dir/artifacts — nothing to move. - # Set sandbox_path first, then grant a+rX (a fallible chmod) so - # artifacts written by a root-owned docker container stay - # traversable across the host uid boundary (MOVE_ON_WRITE gets - # this via preserve_to; DIRECT_WRITE skips it, so apply it here). + # Set sandbox_path first, then prune capture-ignored entries + # (MOVE_ON_WRITE gets this inside preserve_to; DIRECT_WRITE + # never copies, so the raw workspace — agent-created + # .venv/node_modules with sandbox-only symlinks, credential + # stores — would otherwise persist in run_dir/artifacts and + # break artifact publishing), then grant a+rX (a fallible + # chmod) so artifacts written by a root-owned docker container + # stay traversable across the host uid boundary. self.result.sandbox_path = str(self.sandbox.sandbox_dir) + await asyncio.to_thread(self.sandbox.prune_preserved) await asyncio.to_thread(self.sandbox.grant_read_access) logger.info(f"Sandbox preserved (in-place): {self.sandbox.sandbox_dir}") elif self.preservation_mode == PreservationMode.NONE and self.result: diff --git a/src/coder_eval/sandbox.py b/src/coder_eval/sandbox.py index 07217061..c1762646 100644 --- a/src/coder_eval/sandbox.py +++ b/src/coder_eval/sandbox.py @@ -63,6 +63,55 @@ ) +def _prune_capture_ignored(root: Path) -> list[str]: + """Remove :data:`_WORKSPACE_CAPTURE_IGNORE` entries from a preserved tree. + + ``capture_to`` filters these at copy time (``shutil.ignore_patterns`` applies + at every directory level), but the other two preservation paths kept the raw + workspace: ``preserve_to`` is a ``shutil.move`` and DIRECT_WRITE never copies + at all. That leaks the same two classes the ignore tuple exists for — the + credential-store names, and agent-created ``.venv``/``node_modules`` bulk + whose ``bin/`` symlinks point at sandbox-only interpreter paths (dangling on + the host, they break artifact publishing: Azure DevOps' + PublishPipelineArtifact refuses ANY symlink). Walk the preserved tree with + the same matcher and drop every hit, so preserved workspaces look the same + regardless of which preservation path produced them. + + Per-entry deletion failures are warning-logged and skipped — pruning is + hygiene and must never fail a preservation that already succeeded. Returns + the pruned paths relative to ``root`` (for the caller's log line). + """ + if not root.is_dir(): + return [] + matcher = shutil.ignore_patterns(*_WORKSPACE_CAPTURE_IGNORE) + pruned: list[str] = [] + for dirpath, dirnames, filenames in os.walk(root, topdown=True): + base = Path(dirpath) + for name in sorted(matcher(dirpath, dirnames + filenames)): + target = base / name + try: + # is_dir() follows symlinks: unlink a symlinked dir rather than + # rmtree THROUGH it into content outside the preserved tree. + if target.is_dir() and not target.is_symlink(): + shutil.rmtree(target) + else: + target.unlink(missing_ok=True) + except OSError as exc: + logger.warning("Failed to prune %s from preserved workspace: %s", target, exc) + continue + pruned.append(str(target.relative_to(root))) + if name in dirnames: + dirnames.remove(name) # deleted — don't descend into it + if pruned: + logger.info( + "Pruned %d capture-ignored entrie(s) from preserved workspace %s: %s", + len(pruned), + root, + ", ".join(pruned), + ) + return pruned + + def _grant_read_traverse(root: Path) -> None: """Recursively apply ``chmod a+rX`` semantics under ``root``. @@ -1037,6 +1086,21 @@ def grant_read_access(self) -> None: if self.sandbox_dir is not None and self.sandbox_dir.exists(): _grant_read_traverse(self.sandbox_dir) + def prune_preserved(self) -> list[str]: + """Drop capture-ignored entries from the (preserved) sandbox tree, in place. + + For DIRECT_WRITE preservation the sandbox already lives in the artifacts + dir, so neither ``capture_to``'s copy-time filter nor ``preserve_to``'s + post-move prune ever runs — agent-created ``.venv``/``node_modules`` + (symlink-bearing bulk) and any credential-store names would persist in + ``run_dir/artifacts`` verbatim. Called by the orchestrator's DIRECT_WRITE + cleanup arm so all three preservation paths agree on what a preserved + workspace contains. Returns the pruned paths (relative), for callers/tests. + """ + if self.sandbox_dir is None or not self.sandbox_dir.exists(): + return [] + return _prune_capture_ignored(self.sandbox_dir) + def preserve_to(self, artifact_dir: Path) -> Path: """Preserve sandbox contents to an artifact directory. @@ -1070,6 +1134,12 @@ def preserve_to(self, artifact_dir: Path) -> Path: old_sandbox_dir = self.sandbox_dir shutil.move(str(old_sandbox_dir), str(preserve_path)) + # Match capture_to's filtering: the move carried the raw workspace, + # including capture-ignored entries (credential stores; .venv/node_modules + # bulk whose symlinks break artifact publishing). Prune them from the + # preserved copy — before the a+rX grant, so we don't chmod doomed files. + _prune_capture_ignored(preserve_path) + # mkdtemp creates the sandbox root at 0700. Under driver:docker the # container runs as root, so the preserved tree lands on the host # bind-mount owned by root with that 0700 top dir -- the host user diff --git a/tests/test_preserved_workspace_prune.py b/tests/test_preserved_workspace_prune.py new file mode 100644 index 00000000..2c8aab06 --- /dev/null +++ b/tests/test_preserved_workspace_prune.py @@ -0,0 +1,208 @@ +"""Preserved workspaces are pruned of capture-ignored entries on EVERY preservation path. + +``capture_to`` (docker WORKDIR alignment) filters ``_WORKSPACE_CAPTURE_IGNORE`` at +copy time, but ``preserve_to`` (MOVE_ON_WRITE) moved the raw workspace and +DIRECT_WRITE never copies at all — so agent-created ``.venv``/``node_modules`` +(whose ``bin/`` symlinks point at sandbox-only interpreter paths, dangling on the +host) and credential-store names persisted verbatim in ``run_dir/artifacts``. +One such dangling ``.venv/bin/python`` symlink is exactly what a symlink-refusing +artifact uploader chokes on. These tests pin the prune on all three paths, plus +the teardown-interrupt fix: a task-timeout CancelledError delivered during +post-run must not skip ``_cleanup()`` / ``_finalize_result()``. +""" + +import asyncio +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from coder_eval.models import ( + AgentKind, + FileExistsCriterion, + SandboxConfig, + TaskDefinition, + parse_agent_config, +) +from coder_eval.orchestrator import Orchestrator, PreservationMode +from coder_eval.sandbox import Sandbox, _prune_capture_ignored + + +def _seed_workspace(root: Path) -> None: + """Lay out a workspace with keepers and capture-ignored entries at several depths.""" + (root / "src").mkdir(parents=True) + (root / "src" / "app.py").write_text("print('hi')\n", encoding="utf-8") + (root / "artifact.json").write_text("{}\n", encoding="utf-8") + # Agent-created venv with a DANGLING interpreter symlink (the nightly-killer shape). + venv_bin = root / ".venv" / "bin" + venv_bin.mkdir(parents=True) + (venv_bin / "python").symlink_to("/nonexistent/uv/cpython-3.13/bin/python3.13") + # Nested bulk + a node_modules bin symlink. + nested = root / "src" / "pkg" / ".venv" + nested.mkdir(parents=True) + (nested / "pyvenv.cfg").write_text("home = /x\n", encoding="utf-8") + nm_bin = root / "node_modules" / ".bin" + nm_bin.mkdir(parents=True) + (nm_bin / "tool").symlink_to("../tool/cli.js") + # Credential-store name. + (root / ".claude").mkdir() + (root / ".claude" / ".credentials.json").write_text("{}\n", encoding="utf-8") + + +def test_prune_removes_ignored_entries_at_all_depths(tmp_path): + root = tmp_path / "ws" + root.mkdir() + _seed_workspace(root) + + pruned = _prune_capture_ignored(root) + + assert not (root / ".venv").exists() + assert not (root / "src" / "pkg" / ".venv").exists() + assert not (root / "node_modules").exists() + assert not (root / ".claude").exists() + # Keepers survive. + assert (root / "src" / "app.py").exists() + assert (root / "artifact.json").exists() + assert set(pruned) >= {".claude", ".venv", "node_modules", str(Path("src") / "pkg" / ".venv")} + + +def test_prune_unlinks_symlinked_dir_without_following(tmp_path): + """A symlinked ignored dir is unlinked, never rmtree'd through into its target.""" + outside = tmp_path / "outside" + outside.mkdir() + (outside / "keep.txt").write_text("keep\n", encoding="utf-8") + root = tmp_path / "ws" + root.mkdir() + (root / ".venv").symlink_to(outside, target_is_directory=True) + + pruned = _prune_capture_ignored(root) + + assert pruned == [".venv"] + assert not (root / ".venv").exists(follow_symlinks=False) + assert (outside / "keep.txt").exists() + + +def test_prune_missing_root_is_noop(tmp_path): + assert _prune_capture_ignored(tmp_path / "nope") == [] + + +def test_preserve_to_prunes_moved_workspace(tmp_path): + """MOVE_ON_WRITE: preserve_to's move is followed by the same filter capture_to applies.""" + sandbox = Sandbox(SandboxConfig(driver="tempdir", python=None), task_id="prune_move_test") + try: + sandbox_dir = sandbox.setup() + _seed_workspace(sandbox_dir) + + preserved = sandbox.preserve_to(tmp_path / "artifacts") + + assert (preserved / "src" / "app.py").exists() + assert not (preserved / ".venv").exists(follow_symlinks=False) + assert not (preserved / "node_modules").exists() + assert not (preserved / ".claude").exists() + finally: + sandbox.cleanup() + + +def test_sandbox_prune_preserved_in_place(tmp_path): + """DIRECT_WRITE's seam: prune_preserved() filters the sandbox tree where it stands.""" + sandbox = Sandbox(SandboxConfig(driver="tempdir", python=None), task_id="prune_inplace_test") + try: + sandbox_dir = sandbox.setup() + _seed_workspace(sandbox_dir) + + pruned = sandbox.prune_preserved() + + assert ".venv" in pruned + assert not (sandbox_dir / ".venv").exists(follow_symlinks=False) + assert (sandbox_dir / "artifact.json").exists() + finally: + sandbox.cleanup() + + +def test_sandbox_prune_preserved_without_setup_is_noop(): + sandbox = Sandbox(SandboxConfig(driver="tempdir", python=None), task_id="prune_unset_test") + assert sandbox.prune_preserved() == [] + + +# --- Orchestrator seams ------------------------------------------------------- + + +def _build_orchestrator(tmp_path: Path) -> Orchestrator: + task = TaskDefinition( + task_id="prune_teardown_task", + description="d", + initial_prompt="p", + agent=parse_agent_config(type=AgentKind.CLAUDE_CODE), + sandbox=SandboxConfig(driver="tempdir"), + success_criteria=[FileExistsCriterion(description="x", path="x.py")], + ) + run_dir = tmp_path / "run" / "prune_teardown_task" + return Orchestrator(task=task, run_dir=run_dir, variant_id="v1") + + +def _patch_finalize_persistence(): + """Skip the on-disk persistence side-effects of _finalize_result.""" + return patch("coder_eval.reports_html.write_task_html", return_value=None) + + +@pytest.mark.asyncio +async def test_direct_write_cleanup_prunes_before_grant(tmp_path): + orch = _build_orchestrator(tmp_path) + orch.preservation_mode = PreservationMode.DIRECT_WRITE + orch.result = MagicMock() + calls: list[str] = [] + mock_sandbox = MagicMock() + mock_sandbox.sandbox_dir = tmp_path / "sb" + mock_sandbox.prune_preserved = MagicMock(side_effect=lambda: calls.append("prune")) + mock_sandbox.grant_read_access = MagicMock(side_effect=lambda: calls.append("grant")) + orch.sandbox = mock_sandbox + + await orch._cleanup() + + assert calls == ["prune", "grant"] + mock_sandbox.cleanup.assert_called_once_with(preserve=False) + + +@pytest.mark.asyncio +async def test_post_run_cancellation_still_runs_cleanup_and_finalize(tmp_path): + """The task-timeout watchdog's CancelledError lands during post-run: teardown + must still complete (cleanup + finalize) and the cancellation still propagate.""" + orch = _build_orchestrator(tmp_path) + cleanup = AsyncMock() + + async def boom_setup() -> None: + raise RuntimeError("synthetic setup failure") + + with ( + _patch_finalize_persistence(), + patch.object(orch, "_setup", side_effect=boom_setup), + patch.object(orch, "_run_post_run_commands", AsyncMock(side_effect=asyncio.CancelledError())), + patch.object(orch, "_cleanup", cleanup), + pytest.raises(asyncio.CancelledError), + ): + await orch.run() + + cleanup.assert_awaited_once() + # _finalize_result ran: the result was completed and scored despite the interrupt. + assert orch.result is not None + assert orch.result.completed_at is not None + + +@pytest.mark.asyncio +async def test_post_run_plain_exception_still_runs_cleanup_and_reraises(tmp_path): + orch = _build_orchestrator(tmp_path) + cleanup = AsyncMock() + + with ( + _patch_finalize_persistence(), + patch.object(orch, "_setup", AsyncMock()), + patch.object(orch, "_evaluation_loop", AsyncMock(return_value=True)), + patch.object(orch, "_run_post_run_commands", AsyncMock(side_effect=OSError("post-run blew up"))), + patch.object(orch, "_cleanup", cleanup), + pytest.raises(OSError, match="post-run blew up"), + ): + await orch.run() + + cleanup.assert_awaited_once() + assert orch.result is not None + assert orch.result.completed_at is not None From d99886e61ec0085a9017111b1b05f10c5fbf9d3f Mon Sep 17 00:00:00 2001 From: Bai Li Date: Fri, 17 Jul 2026 15:29:36 -0700 Subject: [PATCH 2/3] refactor(orchestrator): trim to interrupt-proof teardown; drop preservation-prune Preservation-path pruning is covered downstream by coder_eval_uipath #57 and isn't reachable on current configs; keep this PR to the teardown fix. Co-Authored-By: Claude Fable 5 --- src/coder_eval/orchestrator.py | 13 +- src/coder_eval/sandbox.py | 70 -------- tests/test_preserved_workspace_prune.py | 208 ------------------------ tests/test_teardown_interrupt.py | 90 ++++++++++ 4 files changed, 94 insertions(+), 287 deletions(-) delete mode 100644 tests/test_preserved_workspace_prune.py create mode 100644 tests/test_teardown_interrupt.py diff --git a/src/coder_eval/orchestrator.py b/src/coder_eval/orchestrator.py index c0416e81..9fc55872 100644 --- a/src/coder_eval/orchestrator.py +++ b/src/coder_eval/orchestrator.py @@ -2116,16 +2116,11 @@ async def _cleanup(self) -> None: logger.info(f"Sandbox preserved to: {preserved_path}") elif self.preservation_mode == PreservationMode.DIRECT_WRITE and self.result: # Sandbox already lives in run_dir/artifacts — nothing to move. - # Set sandbox_path first, then prune capture-ignored entries - # (MOVE_ON_WRITE gets this inside preserve_to; DIRECT_WRITE - # never copies, so the raw workspace — agent-created - # .venv/node_modules with sandbox-only symlinks, credential - # stores — would otherwise persist in run_dir/artifacts and - # break artifact publishing), then grant a+rX (a fallible - # chmod) so artifacts written by a root-owned docker container - # stay traversable across the host uid boundary. + # Set sandbox_path first, then grant a+rX (a fallible chmod) so + # artifacts written by a root-owned docker container stay + # traversable across the host uid boundary (MOVE_ON_WRITE gets + # this via preserve_to; DIRECT_WRITE skips it, so apply it here). self.result.sandbox_path = str(self.sandbox.sandbox_dir) - await asyncio.to_thread(self.sandbox.prune_preserved) await asyncio.to_thread(self.sandbox.grant_read_access) logger.info(f"Sandbox preserved (in-place): {self.sandbox.sandbox_dir}") elif self.preservation_mode == PreservationMode.NONE and self.result: diff --git a/src/coder_eval/sandbox.py b/src/coder_eval/sandbox.py index c1762646..07217061 100644 --- a/src/coder_eval/sandbox.py +++ b/src/coder_eval/sandbox.py @@ -63,55 +63,6 @@ ) -def _prune_capture_ignored(root: Path) -> list[str]: - """Remove :data:`_WORKSPACE_CAPTURE_IGNORE` entries from a preserved tree. - - ``capture_to`` filters these at copy time (``shutil.ignore_patterns`` applies - at every directory level), but the other two preservation paths kept the raw - workspace: ``preserve_to`` is a ``shutil.move`` and DIRECT_WRITE never copies - at all. That leaks the same two classes the ignore tuple exists for — the - credential-store names, and agent-created ``.venv``/``node_modules`` bulk - whose ``bin/`` symlinks point at sandbox-only interpreter paths (dangling on - the host, they break artifact publishing: Azure DevOps' - PublishPipelineArtifact refuses ANY symlink). Walk the preserved tree with - the same matcher and drop every hit, so preserved workspaces look the same - regardless of which preservation path produced them. - - Per-entry deletion failures are warning-logged and skipped — pruning is - hygiene and must never fail a preservation that already succeeded. Returns - the pruned paths relative to ``root`` (for the caller's log line). - """ - if not root.is_dir(): - return [] - matcher = shutil.ignore_patterns(*_WORKSPACE_CAPTURE_IGNORE) - pruned: list[str] = [] - for dirpath, dirnames, filenames in os.walk(root, topdown=True): - base = Path(dirpath) - for name in sorted(matcher(dirpath, dirnames + filenames)): - target = base / name - try: - # is_dir() follows symlinks: unlink a symlinked dir rather than - # rmtree THROUGH it into content outside the preserved tree. - if target.is_dir() and not target.is_symlink(): - shutil.rmtree(target) - else: - target.unlink(missing_ok=True) - except OSError as exc: - logger.warning("Failed to prune %s from preserved workspace: %s", target, exc) - continue - pruned.append(str(target.relative_to(root))) - if name in dirnames: - dirnames.remove(name) # deleted — don't descend into it - if pruned: - logger.info( - "Pruned %d capture-ignored entrie(s) from preserved workspace %s: %s", - len(pruned), - root, - ", ".join(pruned), - ) - return pruned - - def _grant_read_traverse(root: Path) -> None: """Recursively apply ``chmod a+rX`` semantics under ``root``. @@ -1086,21 +1037,6 @@ def grant_read_access(self) -> None: if self.sandbox_dir is not None and self.sandbox_dir.exists(): _grant_read_traverse(self.sandbox_dir) - def prune_preserved(self) -> list[str]: - """Drop capture-ignored entries from the (preserved) sandbox tree, in place. - - For DIRECT_WRITE preservation the sandbox already lives in the artifacts - dir, so neither ``capture_to``'s copy-time filter nor ``preserve_to``'s - post-move prune ever runs — agent-created ``.venv``/``node_modules`` - (symlink-bearing bulk) and any credential-store names would persist in - ``run_dir/artifacts`` verbatim. Called by the orchestrator's DIRECT_WRITE - cleanup arm so all three preservation paths agree on what a preserved - workspace contains. Returns the pruned paths (relative), for callers/tests. - """ - if self.sandbox_dir is None or not self.sandbox_dir.exists(): - return [] - return _prune_capture_ignored(self.sandbox_dir) - def preserve_to(self, artifact_dir: Path) -> Path: """Preserve sandbox contents to an artifact directory. @@ -1134,12 +1070,6 @@ def preserve_to(self, artifact_dir: Path) -> Path: old_sandbox_dir = self.sandbox_dir shutil.move(str(old_sandbox_dir), str(preserve_path)) - # Match capture_to's filtering: the move carried the raw workspace, - # including capture-ignored entries (credential stores; .venv/node_modules - # bulk whose symlinks break artifact publishing). Prune them from the - # preserved copy — before the a+rX grant, so we don't chmod doomed files. - _prune_capture_ignored(preserve_path) - # mkdtemp creates the sandbox root at 0700. Under driver:docker the # container runs as root, so the preserved tree lands on the host # bind-mount owned by root with that 0700 top dir -- the host user diff --git a/tests/test_preserved_workspace_prune.py b/tests/test_preserved_workspace_prune.py deleted file mode 100644 index 2c8aab06..00000000 --- a/tests/test_preserved_workspace_prune.py +++ /dev/null @@ -1,208 +0,0 @@ -"""Preserved workspaces are pruned of capture-ignored entries on EVERY preservation path. - -``capture_to`` (docker WORKDIR alignment) filters ``_WORKSPACE_CAPTURE_IGNORE`` at -copy time, but ``preserve_to`` (MOVE_ON_WRITE) moved the raw workspace and -DIRECT_WRITE never copies at all — so agent-created ``.venv``/``node_modules`` -(whose ``bin/`` symlinks point at sandbox-only interpreter paths, dangling on the -host) and credential-store names persisted verbatim in ``run_dir/artifacts``. -One such dangling ``.venv/bin/python`` symlink is exactly what a symlink-refusing -artifact uploader chokes on. These tests pin the prune on all three paths, plus -the teardown-interrupt fix: a task-timeout CancelledError delivered during -post-run must not skip ``_cleanup()`` / ``_finalize_result()``. -""" - -import asyncio -from pathlib import Path -from unittest.mock import AsyncMock, MagicMock, patch - -import pytest - -from coder_eval.models import ( - AgentKind, - FileExistsCriterion, - SandboxConfig, - TaskDefinition, - parse_agent_config, -) -from coder_eval.orchestrator import Orchestrator, PreservationMode -from coder_eval.sandbox import Sandbox, _prune_capture_ignored - - -def _seed_workspace(root: Path) -> None: - """Lay out a workspace with keepers and capture-ignored entries at several depths.""" - (root / "src").mkdir(parents=True) - (root / "src" / "app.py").write_text("print('hi')\n", encoding="utf-8") - (root / "artifact.json").write_text("{}\n", encoding="utf-8") - # Agent-created venv with a DANGLING interpreter symlink (the nightly-killer shape). - venv_bin = root / ".venv" / "bin" - venv_bin.mkdir(parents=True) - (venv_bin / "python").symlink_to("/nonexistent/uv/cpython-3.13/bin/python3.13") - # Nested bulk + a node_modules bin symlink. - nested = root / "src" / "pkg" / ".venv" - nested.mkdir(parents=True) - (nested / "pyvenv.cfg").write_text("home = /x\n", encoding="utf-8") - nm_bin = root / "node_modules" / ".bin" - nm_bin.mkdir(parents=True) - (nm_bin / "tool").symlink_to("../tool/cli.js") - # Credential-store name. - (root / ".claude").mkdir() - (root / ".claude" / ".credentials.json").write_text("{}\n", encoding="utf-8") - - -def test_prune_removes_ignored_entries_at_all_depths(tmp_path): - root = tmp_path / "ws" - root.mkdir() - _seed_workspace(root) - - pruned = _prune_capture_ignored(root) - - assert not (root / ".venv").exists() - assert not (root / "src" / "pkg" / ".venv").exists() - assert not (root / "node_modules").exists() - assert not (root / ".claude").exists() - # Keepers survive. - assert (root / "src" / "app.py").exists() - assert (root / "artifact.json").exists() - assert set(pruned) >= {".claude", ".venv", "node_modules", str(Path("src") / "pkg" / ".venv")} - - -def test_prune_unlinks_symlinked_dir_without_following(tmp_path): - """A symlinked ignored dir is unlinked, never rmtree'd through into its target.""" - outside = tmp_path / "outside" - outside.mkdir() - (outside / "keep.txt").write_text("keep\n", encoding="utf-8") - root = tmp_path / "ws" - root.mkdir() - (root / ".venv").symlink_to(outside, target_is_directory=True) - - pruned = _prune_capture_ignored(root) - - assert pruned == [".venv"] - assert not (root / ".venv").exists(follow_symlinks=False) - assert (outside / "keep.txt").exists() - - -def test_prune_missing_root_is_noop(tmp_path): - assert _prune_capture_ignored(tmp_path / "nope") == [] - - -def test_preserve_to_prunes_moved_workspace(tmp_path): - """MOVE_ON_WRITE: preserve_to's move is followed by the same filter capture_to applies.""" - sandbox = Sandbox(SandboxConfig(driver="tempdir", python=None), task_id="prune_move_test") - try: - sandbox_dir = sandbox.setup() - _seed_workspace(sandbox_dir) - - preserved = sandbox.preserve_to(tmp_path / "artifacts") - - assert (preserved / "src" / "app.py").exists() - assert not (preserved / ".venv").exists(follow_symlinks=False) - assert not (preserved / "node_modules").exists() - assert not (preserved / ".claude").exists() - finally: - sandbox.cleanup() - - -def test_sandbox_prune_preserved_in_place(tmp_path): - """DIRECT_WRITE's seam: prune_preserved() filters the sandbox tree where it stands.""" - sandbox = Sandbox(SandboxConfig(driver="tempdir", python=None), task_id="prune_inplace_test") - try: - sandbox_dir = sandbox.setup() - _seed_workspace(sandbox_dir) - - pruned = sandbox.prune_preserved() - - assert ".venv" in pruned - assert not (sandbox_dir / ".venv").exists(follow_symlinks=False) - assert (sandbox_dir / "artifact.json").exists() - finally: - sandbox.cleanup() - - -def test_sandbox_prune_preserved_without_setup_is_noop(): - sandbox = Sandbox(SandboxConfig(driver="tempdir", python=None), task_id="prune_unset_test") - assert sandbox.prune_preserved() == [] - - -# --- Orchestrator seams ------------------------------------------------------- - - -def _build_orchestrator(tmp_path: Path) -> Orchestrator: - task = TaskDefinition( - task_id="prune_teardown_task", - description="d", - initial_prompt="p", - agent=parse_agent_config(type=AgentKind.CLAUDE_CODE), - sandbox=SandboxConfig(driver="tempdir"), - success_criteria=[FileExistsCriterion(description="x", path="x.py")], - ) - run_dir = tmp_path / "run" / "prune_teardown_task" - return Orchestrator(task=task, run_dir=run_dir, variant_id="v1") - - -def _patch_finalize_persistence(): - """Skip the on-disk persistence side-effects of _finalize_result.""" - return patch("coder_eval.reports_html.write_task_html", return_value=None) - - -@pytest.mark.asyncio -async def test_direct_write_cleanup_prunes_before_grant(tmp_path): - orch = _build_orchestrator(tmp_path) - orch.preservation_mode = PreservationMode.DIRECT_WRITE - orch.result = MagicMock() - calls: list[str] = [] - mock_sandbox = MagicMock() - mock_sandbox.sandbox_dir = tmp_path / "sb" - mock_sandbox.prune_preserved = MagicMock(side_effect=lambda: calls.append("prune")) - mock_sandbox.grant_read_access = MagicMock(side_effect=lambda: calls.append("grant")) - orch.sandbox = mock_sandbox - - await orch._cleanup() - - assert calls == ["prune", "grant"] - mock_sandbox.cleanup.assert_called_once_with(preserve=False) - - -@pytest.mark.asyncio -async def test_post_run_cancellation_still_runs_cleanup_and_finalize(tmp_path): - """The task-timeout watchdog's CancelledError lands during post-run: teardown - must still complete (cleanup + finalize) and the cancellation still propagate.""" - orch = _build_orchestrator(tmp_path) - cleanup = AsyncMock() - - async def boom_setup() -> None: - raise RuntimeError("synthetic setup failure") - - with ( - _patch_finalize_persistence(), - patch.object(orch, "_setup", side_effect=boom_setup), - patch.object(orch, "_run_post_run_commands", AsyncMock(side_effect=asyncio.CancelledError())), - patch.object(orch, "_cleanup", cleanup), - pytest.raises(asyncio.CancelledError), - ): - await orch.run() - - cleanup.assert_awaited_once() - # _finalize_result ran: the result was completed and scored despite the interrupt. - assert orch.result is not None - assert orch.result.completed_at is not None - - -@pytest.mark.asyncio -async def test_post_run_plain_exception_still_runs_cleanup_and_reraises(tmp_path): - orch = _build_orchestrator(tmp_path) - cleanup = AsyncMock() - - with ( - _patch_finalize_persistence(), - patch.object(orch, "_setup", AsyncMock()), - patch.object(orch, "_evaluation_loop", AsyncMock(return_value=True)), - patch.object(orch, "_run_post_run_commands", AsyncMock(side_effect=OSError("post-run blew up"))), - patch.object(orch, "_cleanup", cleanup), - pytest.raises(OSError, match="post-run blew up"), - ): - await orch.run() - - cleanup.assert_awaited_once() - assert orch.result is not None - assert orch.result.completed_at is not None diff --git a/tests/test_teardown_interrupt.py b/tests/test_teardown_interrupt.py new file mode 100644 index 00000000..4f28882e --- /dev/null +++ b/tests/test_teardown_interrupt.py @@ -0,0 +1,90 @@ +"""``run()``'s teardown must be interrupt-proof. + +The task-timeout watchdog cancels the asyncio task via +``loop.call_soon_threadsafe(task.cancel)``, delivering a ``CancelledError`` at +the next await. When that await is inside ``_run_post_run_commands`` (post-run +commands do awaited I/O), the cancellation used to land in ``run()``'s +``finally`` block and abort it wholesale — skipping ``_cleanup()`` (tempdir +leaked; workspace never preserved) AND ``_finalize_result()`` (task.json lost, +so the task silently vanished from the run). These tests pin that a post-run +interrupt — cancellation or a plain exception — still runs the full teardown +(cleanup + finalize) and then re-raises the original exception unchanged. +""" + +import asyncio +from pathlib import Path +from unittest.mock import AsyncMock, patch + +import pytest + +from coder_eval.models import ( + AgentKind, + FileExistsCriterion, + SandboxConfig, + TaskDefinition, + parse_agent_config, +) +from coder_eval.orchestrator import Orchestrator + + +def _build_orchestrator(tmp_path: Path) -> Orchestrator: + task = TaskDefinition( + task_id="teardown_interrupt_task", + description="d", + initial_prompt="p", + agent=parse_agent_config(type=AgentKind.CLAUDE_CODE), + sandbox=SandboxConfig(driver="tempdir"), + success_criteria=[FileExistsCriterion(description="x", path="x.py")], + ) + run_dir = tmp_path / "run" / "teardown_interrupt_task" + return Orchestrator(task=task, run_dir=run_dir, variant_id="v1") + + +def _patch_finalize_persistence(): + """Skip the on-disk persistence side-effects of _finalize_result.""" + return patch("coder_eval.reports_html.write_task_html", return_value=None) + + +@pytest.mark.asyncio +async def test_post_run_cancellation_still_runs_cleanup_and_finalize(tmp_path): + """The task-timeout watchdog's CancelledError lands during post-run: teardown + must still complete (cleanup + finalize) and the cancellation still propagate.""" + orch = _build_orchestrator(tmp_path) + cleanup = AsyncMock() + + async def boom_setup() -> None: + raise RuntimeError("synthetic setup failure") + + with ( + _patch_finalize_persistence(), + patch.object(orch, "_setup", side_effect=boom_setup), + patch.object(orch, "_run_post_run_commands", AsyncMock(side_effect=asyncio.CancelledError())), + patch.object(orch, "_cleanup", cleanup), + pytest.raises(asyncio.CancelledError), + ): + await orch.run() + + cleanup.assert_awaited_once() + # _finalize_result ran: the result was completed and scored despite the interrupt. + assert orch.result is not None + assert orch.result.completed_at is not None + + +@pytest.mark.asyncio +async def test_post_run_plain_exception_still_runs_cleanup_and_reraises(tmp_path): + orch = _build_orchestrator(tmp_path) + cleanup = AsyncMock() + + with ( + _patch_finalize_persistence(), + patch.object(orch, "_setup", AsyncMock()), + patch.object(orch, "_evaluation_loop", AsyncMock(return_value=True)), + patch.object(orch, "_run_post_run_commands", AsyncMock(side_effect=OSError("post-run blew up"))), + patch.object(orch, "_cleanup", cleanup), + pytest.raises(OSError, match="post-run blew up"), + ): + await orch.run() + + cleanup.assert_awaited_once() + assert orch.result is not None + assert orch.result.completed_at is not None From d31f46f50c600e773d432415327dd0af5dbf8d07 Mon Sep 17 00:00:00 2001 From: Bai Li Date: Fri, 17 Jul 2026 15:36:24 -0700 Subject: [PATCH 3/3] docs(orchestrator): correct teardown comment to scope of the fix The .venv/publishing framing conflated this fix with pipeline #57; _cleanup() does not remove the .venv. Comment now reflects what interrupt-proofing fixes: tempdir leak + lost task.json. Co-Authored-By: Claude Fable 5 --- src/coder_eval/orchestrator.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/coder_eval/orchestrator.py b/src/coder_eval/orchestrator.py index 9fc55872..b1581470 100644 --- a/src/coder_eval/orchestrator.py +++ b/src/coder_eval/orchestrator.py @@ -560,14 +560,12 @@ def _kill_agent_subprocess_sync() -> None: # Teardown must be interrupt-proof: the task-timeout watchdog can # fire while post-run commands are awaiting and deliver its # CancelledError right here in the finally block, which used to - # abort it wholesale — skipping _cleanup() (tempdir leaked; a - # DIRECT_WRITE workspace kept agent-created .venv/node_modules - # whose dangling symlinks later broke artifact publishing) AND - # _finalize_result() (task.json lost). Catch the interrupt, - # finish the full teardown, then re-raise it at the end so - # callers observe the same exception as before. The watchdog - # cancels exactly once, so the teardown awaits below run - # normally after the CancelledError is caught. + # abort it wholesale — skipping _cleanup() (tempdir leaked) AND + # _finalize_result() (task.json lost, so the task silently drops + # out of the run). Catch the interrupt, finish the full teardown, + # then re-raise it at the end so callers observe the same exception + # as before. The watchdog cancels exactly once, so the teardown + # awaits below run normally after the CancelledError is caught. teardown_interrupt: BaseException | None = None try: # BEFORE post-run/cleanup: needs the live sandbox to resolve