fix(orchestrator): interrupt-proof teardown so a timeout can't drop task.json

[bai-uipath]

The bug

The task-timeout watchdog cancels the running asyncio task, delivering a CancelledError at the next await. When that lands inside _run_post_run_commands (post-run commands do awaited I/O), the cancellation used to abort run()'s finally block wholesale:

• _finalize_result() skipped → the task's task.json is never written, so it silently drops out of the run and out of the rollup denominator
• _cleanup() skipped → its tempdir leaks

Fix

run()'s teardown is now interrupt-proof: an exception (including CancelledError) raised during the post-run phase is held, the full teardown completes (cleanup → log-tail → finalize), then the original exception is re-raised so callers observe the same outcome as before. The watchdog cancels exactly once, so the awaits after the catch run normally.

Scope

Deliberately narrow — just the teardown correctness fix. An earlier revision of this branch also pruned capture-ignored entries (.venv/node_modules/credential stores) on the preserve_to/DIRECT_WRITE preservation paths; that has been dropped and deferred to its own hygiene PR, as it's defense-in-depth for a workspace layout that isn't currently reachable.

🤖 Generated with Claude Code

[github-actions]

Claude finished @bai-uipath's task in 1m 18s —— View job

---

Code Review In Progress

Todo List:

• Read .github/code_review.md for review guidelines
• Read CLAUDE.md for project conventions
• Review the full diff (git diff origin/main...HEAD)
• Read complete changed files for context
• Perform cross-file consistency checks
• Analyze what's missing or could be improved
• Post comprehensive review feedback

Starting review...

[uipreliga]

Review: coder_eval — pr:29

Scope: pr:29 · branch bai/prune-preserved-workspaces · d31f46f · 2026-07-17T22:53Z · workflow variant

Change class: complex — reworks run()'s teardown control flow to catch a task-timeout watchdog interrupt (CancelledError) or exception during post-run, complete cleanup + finalize, then re-raise; touches asyncio cancellation and error-handling semantics on the orchestrator hot path

coder_eval is in excellent shape (9.6/10, clean on type safety, security, error handling, and API surface), and the sole real risk is a single confirmed correctness gap in the timeout-teardown fix: the watchdog interrupt guard covers only post-run commands, so default empty-post_run tasks can still drop task.json on cancellation — silently changing metric denominators for identical agent output — with the remaining items being complexity/cohesion and test-coverage debt around that same code.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	9 / 10	0	1	0	0	Orchestrator.run cyclomatic complexity crosses the >20 hot-module threshold (CC 21, grade D)
2. Type Safety	10 / 10	0	0	0	0	—
3. Test Health	8.9 / 10	0	1	0	1	Teardown tests only cover post-run interrupt; the cleanup-phase / empty-post_run interrupt path the fix leaves broken is untested
4. Security	10 / 10	0	0	0	0	—
5. Architecture & Design	9.9 / 10	0	0	0	1	Interrupt-capture teardown inlined into run()'s finally instead of a cohesive _teardown() helper
6. Error Handling & Resilience	10 / 10	0	0	0	0	—
7. API Surface & Maintainability	10 / 10	0	0	0	0	—
8. Evaluation Harness Quality	9 / 10	0	1	0	0	Interrupt guard covers only post-run, not _cleanup()/_finalize_result(); default empty-post_run tasks still drop task.json on watchdog cancel

Overall Score: 9.6 / 10 · Weakest Axis: Test Health at 8.9 / 10
Totals: 🔴 0 · 🟠 3 · 🟡 0 · 🔵 2 across 8 axes.

Blockers

1. [Axis 1] Orchestrator.run cyclomatic complexity crosses the >20 hot-module threshold (CC 21, grade D) (src/coder_eval/orchestrator.py:399) — The added nested try/except in run()'s finally block pushed Orchestrator.run from CC 19 to CC 21 (verified: radon cc reports M 399:4 Orchestrator.run - D (21) on the PR head), matching the Axis-1 High anchor 'cyclomatic complexity > 20 in a hot module (orchestrator)'. Extract the whole finally-block teardown sequence (_refresh_runtime_tool_versions -> _run_post_run_commands -> _cleanup -> log-tail capture -> _finalize_result -> re-raise) into one async def _teardown(self, start_time, log_tail) -> None helper and call it from the finally. This drops run() back below the threshold and, if the helper wraps ALL teardown steps, also closes the gap in finding #2.
2. [Axis 3] Teardown tests only cover post-run interrupt; the cleanup-phase / empty-post_run interrupt path the fix leaves broken is untested (tests/test_teardown_interrupt.py:62) — Both tests inject the watchdog interrupt only at the post-run raise site and mock cleanup away — line 61 patch.object(orch, "_run_post_run_commands", AsyncMock(side_effect=asyncio.CancelledError())) + line 62 patch.object(orch, "_cleanup", cleanup) (mirrored at lines 82-83). But in orchestrator.py the new try/except guards ONLY _refresh_runtime_tool_versions() + await self._run_post_run_commands() (pr-29 orchestrator.py lines 570-575); await self._cleanup() at line 582 is OUTSIDE the guard, and _cleanup performs multiple real await points (await self.agent.stop(), await asyncio.to_thread(self.sandbox.capture_to/preserve_to/grant_read_access) at orchestrator.py:2091,2107,2111,2124). The task-timeout watchdog fires on a wall-clock timer independent of execution position: if post-run finishes first (or there are no post-run commands), the single task.cancel() lands during _cleanup() instead, so _finalize_result(start_time) (orchestrator.py:596) is skipped and task.json is dropped — the EXACT failure mode the PR claims to fix. The fix comment's assumption 'the watchdog cancels exactly once, so the teardown awaits below run normally' only holds when the cancel lands in post-run, not when it lands in the longer, I/O-heavy cleanup region. Add a test that raises asyncio.CancelledError from _cleanup (leaving _run_post_run_commands succeeding) and asserts _finalize_result still ran (orch.result.completed_at is not None); it will fail against the current code, exposing that the guard must be extended to wrap _cleanup (and the log-tail/finalize block) too, not just post-run.
3. [Axis 8] Interrupt guard covers only post-run, not _cleanup()/_finalize_result(); default empty-post_run tasks still drop task.json on watchdog cancel (src/coder_eval/orchestrator.py:582) — The new try/except wraps only self._refresh_runtime_tool_versions() + await self._run_post_run_commands() (lines 573-574). await self._cleanup() (line 582) and self._finalize_result(start_time) (line 596) remain OUTSIDE the guard. Trigger: post_run defaults to an empty list (models/tasks.py:395 default_factory=list), so for the common task _run_post_run_commands -> _run_command_list hits if not commands ...: return (orchestrator.py:1975) and never suspends. The watchdog delivers cancellation via loop.call_soon_threadsafe(task.cancel) (agents/watchdog.py:100-101); a cancel requested at the eval-loop boundary but not yet delivered therefore lands on the FIRST real suspension inside _cleanup() — await self.agent.stop() or await asyncio.to_thread(self.sandbox.capture_to, ...). CancelledError is a BaseException, so _cleanup()'s except Exception (orchestrator.py:2091) does not catch it; it escapes the finally block, skipping the error_log_tail capture (line 588-595) AND _finalize_result() (line 596), which is the sole writer of task.json (atomic write at ~line 715). The row then silently vanishes from the run, changing metric denominators for identical agent output — exactly the Axis-8 scoring-correctness class. The PR's own commit message ('interrupt-proof teardown so a timeout can't drop task.json') and code comment overclaim: the fix closes the post-run path but leaves the DEFAULT-config path (empty post_run) unguarded. Not a regression (baseline dropped task.json on any finally-block interrupt), hence High rather than Critical, but the fix is incomplete on the exact path it targets. Fix: bring _cleanup() and _finalize_result() under the same interrupt-accumulation pattern — e.g. wrap await self._cleanup() in its own try/except that folds a caught (Exception, asyncio.CancelledError) into teardown_interrupt, so _finalize_result(start_time) always runs before the final re-raise.

Non-blocking, but please consider before merge

None.

Nits

1. [Axis 3] Cancellation test asserts only the exception type, not that the SAME instance is re-raised unchanged (tests/test_teardown_interrupt.py:64) — The module docstring states the contract is to 're-raise the original exception unchanged', and orchestrator.py:597-598 does raise teardown_interrupt on the captured instance. The cancellation test asserts only the class via pytest.raises(asyncio.CancelledError) (line 64) and never binds the injected instance, so a regression that swallowed the original and raised a fresh CancelledError() would pass. Bind the injected exception to a variable and assert excinfo.value is <that instance> (the plain-exception test at line 79 already pins the message via match="post-run blew up", so only the cancellation case is weak on identity).
2. [Axis 5] Interrupt-capture teardown inlined into run()'s finally instead of a cohesive _teardown() helper (src/coder_eval/orchestrator.py:559) — The fix is correct, but it deepens an already-~200-line hot method (run() spans lines 399-600) rather than extracting the teardown into a helper. The finally block (lines 559-598) now performs a full seven-step teardown sequence inline: interrupt-guarded post-run (teardown_interrupt: BaseException | None = None / try: self._refresh_runtime_tool_versions(); await self._run_post_run_commands() except (Exception, asyncio.CancelledError) as e: teardown_interrupt = e), then await self._cleanup(), the log-tail allowlist block, self._finalize_result(start_time), and finally if teardown_interrupt is not None: raise teardown_interrupt. This nested try/except-plus-deferred-reraise is what drove Orchestrator.run CC from 19 to 21 (C->D per the routed radon signal). Extracting this coherent unit into an async def _teardown(self, start_time) -> BaseException | None helper (returning the captured interrupt for the caller to re-raise) would restore run()'s finally to a couple of lines, keep the interrupt-proofing logic testable in isolation, and pull the method back under the CC-20 line for the orchestrator hot module. This is a cohesion nit only — the current code is correct and the divergence is not painful today (per the Axis-5 Low anchor: 'method that could be split for cohesion but isn't painful today').

What's Missing

Tests:

• 🟠 No test exercises the cleanup-phase interrupt the fix leaves broken: both new tests inject the failure at _run_post_run_commands (lines 61/82) and mock _cleanup away (62/83), so the branch where a watchdog cancel lands inside _cleanup() — the common default empty-post_run case — and skips _finalize_result is never run. Add a test that raises asyncio.CancelledError from _cleanup with post-run succeeding and asserts _finalize_result still ran (orch.result.completed_at is not None); it will fail against current code. (trigger: tests/test_teardown_interrupt.py) (restates: Axis 3: Teardown tests only cover post-run interrupt; cleanup-phase path untested)

Parallel paths:

• 🟡 The parallel DockerRunner path guarantees the task.json contract with _write_synthetic_task_json (writes a synthetic ERROR record when the container dies before finalizing), but the in-process Orchestrator.run() finally being hardened here has no equivalent fallback — so on the still-unguarded _cleanup()-interrupt path the in-process row produces no task.json on disk at all. The PR hardened only in-process teardown and did not bring it to parity with the driver it mirrors. (trigger: src/coder_eval/orchestrator.py) _(restates: Axis 8: Interrupt guard covers only post-run, not _cleanup()/finalize_result())

Downstream consumers:

• 🟡 Disk-based re-aggregation consumers assume a task.json exists for every attempted row: recover_task_results / aggregate_run_dir (rglob over task.json) and the --resume already-complete check both skip a row with no task.json. On the incomplete-guard path batch.py still folds the escaped CancelledError into an in-memory ERROR TaskResult (post-gather BaseException handler), so the live run.json disagrees with what a later disk re-aggregation sees — the row silently drops from the recomputed denominators (and --resume would re-run it). (trigger: src/coder_eval/orchestrator.py) _(restates: Axis 8: Interrupt guard covers only post-run, not _cleanup()/finalize_result())

Daily/nightly:

• 🟠 The change edits the production run path (the finally that writes task.json) and is motivated by a task-timeout scenario, but the PR never states the nightly blast radius: with the default empty post_run (the common nightly task) the guard does not cover _cleanup(), so an in-process nightly still drops task.json on a task_timeout watchdog cancel, and the cross-repo consumer (coder-eval-uipath / eval-runner reading per-row task.json) loses that row. Docker-driver nightlies are shielded by the host-side synthetic fallback; in-process nightlies are not — call this out explicitly. (trigger: src/coder_eval/orchestrator.py) _(restates: Axis 8: Interrupt guard covers only post-run, not _cleanup()/finalize_result())

Harness & Lint Improvements

Static checks (lint / type):

• [ruff] Enable ruff's mccabe complexity gate on the hot modules to make CC>20 fail in make check/CI before review. In pyproject add C901 to [tool.ruff.lint] select and set [tool.ruff.lint.mccabe] max-complexity = 20. Because several existing orchestrator methods already exceed 20 (_simulation_dialog_loop CC40, _finalize_result CC26, _check_run_limits CC24, _evaluation_loop CC23), grandfather them with an explicit # noqa: C901 (or per-file-ignore) at each site — the noqa list itself makes the pre-existing debt visible and, crucially, any NEW crossing (like Orchestrator.run going 19→21) fails the build because a fresh function has no grandfathering. If per-function noqa grandfathering is undesirable, encode the same intent as a custom CE025 rule modeled on CE022: an AST/radon-based per-function CC ratchet over orchestrator.py/checker.py/sandbox.py with a baseline allowlist of the current over-threshold functions, firing when any other function crosses CC 20 or a listed one worsens. Prevents: Finding #1 (Orchestrator.run CC 21 > 20 in a hot module) and the complexity-driver half of Finding #4 (the inlined interrupt-capture teardown that pushed run() from CC 19 to 21) — both would have failed the gate at the moment run() crossed 20, forcing the _teardown() extraction before merge.

Harness improvements (not statically reachable):

• Extend tests/test_teardown_interrupt.py from a single post-run injection site into a parametrized test that injects asyncio.CancelledError (and a plain Exception) at EVERY teardown seam in run()'s finally — _run_post_run_commands, _cleanup (with post-run succeeding), the log-tail block, and _finalize_result — and, without mocking _cleanup/_finalize_result away, asserts for each seam that orch.result.completed_at is not None and that task.json was actually written. Against current code the _cleanup case fails (completed_at stays None), which is exactly what exposes that the interrupt guard must wrap _cleanup+log-tail+_finalize_result, not just post-run. Why not static: The defect only manifests at runtime: it requires an already-delivered watchdog cancel landing on the first suspending await inside _cleanup (reachable precisely because the DEFAULT empty post_run never suspends) and then asserting a persisted filesystem artifact (task.json) still exists. Which teardown steps must always run under interrupt is a semantic property of the finally block, not a grep-/AST-detectable shape. Prevents: Finding #2 (cleanup-phase / empty-post_run interrupt path is untested) and Finding #5 (guard covers only post-run, so a watchdog cancel during _cleanup skips _finalize_result and silently drops task.json, changing metric denominators for identical agent output).
• Strengthen the cancellation case in tests/test_teardown_interrupt.py to pin re-raise IDENTITY, not just the exception class: bind the injected CancelledError instance, wrap the call in pytest.raises(asyncio.CancelledError) as excinfo, and assert excinfo.value is <injected instance>. The plain-exception test already pins its message via match=; only the cancellation path is identity-weak. Why not static: The contract ('re-raise the ORIGINAL exception unchanged' per the module docstring) is semantic — a regression that swallows the original and raises a fresh CancelledError() is type-identical, so no type or AST check distinguishes it. A lint rule flagging every pytest.raises(X) used without value-binding/match= would be far too noisy (most type-only assertions are legitimate) to be worth enforcing. Prevents: Finding #3 (cancellation test asserts only the exception type, so a swallow-and-re-raise-fresh regression of the teardown re-raise contract would pass unnoticed).

Top 5 Priority Actions

1. Extend the interrupt guard so it also wraps await self._cleanup() (src/coder_eval/orchestrator.py:582) and self._finalize_result(start_time) (line 596) — fold a caught (Exception, asyncio.CancelledError) into teardown_interrupt before the final re-raise — so a watchdog cancel landing in cleanup (the common empty-post_run path) still writes task.json instead of dropping the row and skewing metrics.
2. Add a regression test that raises asyncio.CancelledError from _cleanup while post-run succeeds and asserts _finalize_result still ran (orch.result.completed_at is not None) at tests/test_teardown_interrupt.py:62 — it fails against current code and pins the cleanup-phase path the two existing tests mock away.
3. Extract the seven-step finally teardown sequence into one async def _teardown(self, start_time) helper (src/coder_eval/orchestrator.py:399/559), which drops Orchestrator.run back below the CC-20 hot-module threshold (currently D/21) and resolves the Axis-5 cohesion nit in the same change while making the interrupt logic unit-testable in isolation.
4. Strengthen the cancellation test to bind the injected exception and assert excinfo.value is <that instance> (tests/test_teardown_interrupt.py:64) so a regression that swallows the original and raises a fresh CancelledError() is caught, matching the module's 're-raise unchanged' contract.
5. Follow up on the higher-CC sibling hot methods flagged in the same module — _simulation_dialog_loop E(40), _finalize_result D(26), _check_run_limits D(24), _evaluation_loop D(23) in src/coder_eval/orchestrator.py — as a maintainability backlog since they already exceed the threshold that run() only marginally crossed.

---

Stats: 0 🔴 · 3 🟠 · 0 🟡 · 2 🔵 across 8 axes reviewed.

[mohsen-uipath]

LGTM — reviewed the diff and verified the load-bearing assumption.

What it does: Previously a task-timeout CancelledError thrown during post-run flew straight out of the finally, so _cleanup() and _finalize_result() were skipped entirely (leaked tempdir + lost task.json, dropping the task from the rollup denominator). Now the interrupt is caught and held, teardown runs to completion, and the same exception is re-raised — cleanup/finalize always execute, and callers observe the same outcome.

Verified assumption: The fix relies on the watchdog cancelling exactly once. Confirmed in agents/watchdog.py::_fire — single-shot threading.Timer guarded by _fired under a lock, so task.cancel is scheduled at most once; the awaits after the catch then run normally. It also hard-kills the subprocess before cancelling, so the now-guaranteed _cleanup() never waits on a live process.

Tests: Both interrupt kinds (cancellation + plain exception) covered — cleanup awaited, result finalized.

Two non-blocking notes: (1) no task.uncancel(), which is fine because the fix re-raises rather than swallows the cancellation (it's honored, not hijacked); (2) teardown now always runs with no deadline of its own — a hanging _cleanup() couldn't be rescued by the already-spent watchdog, but that's low risk since the subprocess is already dead before cleanup starts.

Good call narrowing scope to the teardown fix and deferring the .venv/symlink hygiene to coder_eval_uipath #57.