fix(orchestrator): run long executor subprocesses off the event loop

[siliangchen-amd]

Problem

The coordinator is a single-threaded asyncio event loop. Concurrent tasks share that thread and only yield at await. Several executor helpers called blocking subprocess.run (and urllib) on that thread: enablement setup replay (up to twelve installs, 30 minutes each), attempt-runtime provision (venv + pip, 1800s), specialist patch vetting (serial git apply --check), localization HTTP fetch, localization editable-refresh (10 minutes), and specialist git worktree add (60s).

While the loop is frozen, wall-clock timeouts keep ticking. asyncio.wait_for records an absolute deadline when the await starts; when the loop resumes, the timer has already fired.

This does not stall the same tick's own model call: integrate_patch is joined. The work that is concurrent on the loop is what breaks: another in-flight action's streaming LLM call (per-chunk idle timeout), the dispatcher's re-scan poll, shutdown cancel-grace, and the kernel-step heartbeat.

The dispatcher assumes every executor spends blocking time inside asyncio.to_thread so cooperative cancel can run. These paths violated that.

Fix

Keep the helpers synchronous. At the coroutine boundary, wrap each one in asyncio.to_thread and wait until it finishes. Same convention as the magpie grid path. Provision+probe (including the probe-fail downgrade) is one hop, not two.

Do not use the existing offload helper. That helper waits with a budget, then returns a default while the worker keeps running. On setup replay or provision that would mean pip is still mutating the environment while integrate continues to apply patches and bench.

Setup replay polls the action CancelScope between commands (the ContextVar is copied into the worker). A command already inside subprocess.run is not killed. Cancelling the await unwinds integrate and does not apply patches. That is the opposite of an offload timeout, where the caller continues. Provision's single 1800s pip is fire-and-forget the same way.

Helpers that must stay off the loop are marked Blocking (...); call via asyncio.to_thread.

Out of scope

The many short git helpers on the integrate path stay on the loop. Each call is bounded (two-minute default) and usually sub-second. Moving them would force a cascade of sync helpers to become coroutines.

Loop-lag detection, and writing the "upper bound shorter than the shortest timeout concurrent on the loop" criterion next to the dispatcher cancel invariant, remain a follow-up. A failed attempt venv rmtree on the loop (tens of seconds on shared storage) is exactly what that criterion would catch.

The localization editable-refresh except Exception swallow is pre-existing and untouched.

Tests

• Setup replay and provision each assert the sync helper's thread id is not the event-loop thread, by driving the real async stages.
• Patch vetting is driven through the in-process specialist run path (the production wrap around finalize).
• Setup replay stops between commands when the cancel scope is set.
• Existing allowlist and patch-vetting unit tests are unchanged: helper signatures did not change.

Limitations

Threads started by asyncio.to_thread cannot be cancelled. Setup replay cooperates between commands; mid-command pip and provision pip are fire-and-forget. A long install occupies one slot in the default executor pool (min(32, cpu+4)). The magpie path already does this; this change does not introduce a dedicated pool.

[ZhengGong-amd]

The diagnosis is right and asyncio.to_thread is the right mechanism — rejecting the budgeted offload helper (returns a default while the worker keeps mutating the environment) is the correct call. Three things must change before this lands, and one thing about the shape of the fix.

Blocking

1. adapter.provision is still on the loop thread — and it is the longest one. _stage_provision_attempt_runtime runs before _stage_apply in the same enablement attempt:

# integrate_patch.py:2023
result = adapter.provision(action, attempt_dir)
if result.ok and not adapter.probe(result, action):

provision creates a venv, pip-installs the vLLM ROCm wheel, and runs four verify/version probes, all through adapters.py:70 _default_run → subprocess.run(timeout=_PROVISION_TIMEOUT_SEC) with _PROVISION_TIMEOUT_SEC = 1800. That freezes the loop for the same order of magnitude this PR fixes for setup replay, on the same path, and it is not covered by the stated out-of-scope (short git helpers). Wrap provision and probe the same way you wrapped setup replay.

2. build_localization_diff does synchronous HTTP on the loop. integrate_patch.py:2126 fans out into three urllib.request.urlopen sites in agents/framework/sources/github.py, several requests per localization. This PR fixes the tail of the localization path (_finalize_localization_keep) and leaves the head.

3. The three helpers need the blocking annotation this repo already uses. They went from "call however" to "must be called via to_thread", and none of the docstrings say so. The convention exists:

# recover.py:600
"""Per-GPU free VRAM, for callers that need the probe without the action.

Blocking (shells out to ``rocm-smi``); call via ``asyncio.to_thread``.

One line each on _run_setup_commands, _finalize_localization_keep, and _patch_safety.vet_patches — the last is exported in __all__, so it is the one a future caller will get wrong. Without this line the next call site puts the work back on the loop and the fix is silently undone.

Make it smaller, and pick one shape

_finalize has no await in its body other than the one you added. It does not need to become a coroutine — wrap it whole at its two call sites:

return await asyncio.to_thread(
    self._finalize,
    ctx=ctx,
    ...
)

to_thread takes kwargs, so no partial. That is 5 net lines in runner.py, zero test changes (the three existing sync _finalize tests pass untouched — verified: 36 passed in that file, 630 passed for -k specialist), and it also moves the _write_specialist_done disk write off the loop. It is also the shape you already chose for _finalize_localization_keep at integrate_patch.py:3094. Right now one PR uses two conventions for the same problem; converge on the wrap. If you prefer the async signature instead, apply it to both and say why.

Please state the cancel semantics

Before this change, a frozen loop meant cancel could not be delivered mid-install. After it, cancelling the action task raises out of await asyncio.to_thread(...) immediately and _stage_apply unwinds while pip is still mutating the environment — the exact hazard you cite to reject the offload helper, reached from a different trigger. cancel_inflight_actions solves this for benchmark executors with a CancelScope the blocking side polls. Either poll a scope between setup commands, or record explicitly that setup replay's cancel is fire-and-forget. Right now neither is in the PR.

Why I am asking for the invariant to be enforced, not just re-applied

The analysis in this PR is above average and it still missed a 1800s pip install in the file it edits, a synchronous HTTP fan-out on the same path, and a 60s git worktree add (subprocess_.py:709, called from async def _prepare). That is not carelessness — it is the failure mode of fixing this per call site. The next helper added will miss the list again, and the symptoms (LLM streams timing out mid-stream, queued GPU tasks starting late, cancel grace expiring unwatched) never point at the cause.

Two consequences:

• The out-of-scope criterion needs to be written down, and "bounded" is not it. A 60s freeze is enough to expire a per-chunk stream idle timeout on a gateway that is still streaming. The criterion is upper bound shorter than the shortest timeout running concurrently on the loop. Put it next to dispatcher.py:220, which is where the invariant is already asserted and which this PR restores.
• There is no loop-lag detection anywhere in the repo (no slow_callback_duration, no set_debug, no lag sampler). One place to add it covers provision, urlopen, worktree add, and everything added later. It also replaces the per-call-site thread-identity tests: those two new tests assert an implementation detail (which thread ran), while lag detection asserts the property actually in question (is the loop still responsive). I would rather have one detector than one test per wrapper.

Separately, and not for this PR: _finalize_localization_keep swallows editable-refresh failure at integrate_patch.py:3174 with except Exception: log.debug(...), so KEEP records a manifest for a closure that was never refreshed. Pre-existing; worth its own PR.

What I need to see: wrap provision/probe and the localization fetch; add the three docstring lines; revert _finalize to sync and wrap it at the call sites; state the cancel semantics. The loop-lag detector and the written criterion can be a follow-up, but without them this file will need this same review again.

[siliangchen-amd]

Thanks — provision and the localization fetch were real misses on the same enablement path, and wrapping them the same way as setup replay is the right fix. A few corrections on the rest, then what landed.

Probe. The four verify/version subprocesses run inside provision (venv, pip, torch/vLLM checks). adapter.probe is a disk expected_files check. Both still go through one to_thread hop because they are sequential; probe is not a second 1800s install.

urlopen. This path uses pr_patches (up to 2×30s) and fetch_raw_file (30s per path). The third site is search/discovery and is not on the localization fetch.

Docstring convention. The Blocking … call via asyncio.to_thread line used to live on the recover GPU probe and was stripped by a later docstring compaction. Restored on the helpers this PR wraps, including exported patch vetting.

_finalize shape. The async signature was correct (vetting was already off the loop). It is now sync again and wrapped whole at the two call sites, matching localization keep, so the PR uses one convention.

Cancel is not offload. Offload returns a default on timeout and the caller continues (pip still mutating, integrate applies and benches). Cancelling await to_thread raises out of the await and integrate unwinds — it does not apply patches. What remains fire-and-forget is the in-flight subprocess.run (a thread cannot be cancelled). Setup replay now polls the cancel scope between commands so later installs are skipped; a command already inside subprocess.run is not killed. Provision's single 1800s pip is the same fire-and-forget; this PR does not add Popen+kill.

Worktree add (60s) is wrapped from prepare for the same reason: a 60s freeze is enough to expire a per-chunk stream idle timeout. Same one-line wrap, same file as finalize.

Not in this PR, as you already scoped: loop-lag detector and rewriting the dispatcher cancel docstring (that paragraph is the CancelScope invariant, not "how short a freeze may stay on the loop"). The editable-refresh except Exception swallow is pre-existing and untouched.

[ZhengGong-amd]

All four blocking items are addressed, and two of them better than I asked for. _provision_and_probe offloads provision+probe+the downgrade as one atomic step rather than two thread round-trips. The docstring annotation went onto every offloaded helper, not just the three I named, which turns the convention into a habit. _finalize is sync again with the wrap at both call sites, so the PR now has one shape. And setup replay actually polls CancelScope between commands with a deterministic test, instead of just documenting the semantics — the ContextVar scope is copied into the to_thread worker, so that chain is sound.

Approving. Three things left, none blocking:

1. Broken docstring at integrate_patch.py:1983. The old line was not removed:

        adapter (off the event loop; an in-flight pip install is not killed
        if the await is cancelled), and on success stores the resolved runtime on
        adapter, and on success stores the resolved runtime on

2. test_patch_vetting_runs_off_the_event_loop_thread is now self-proving. It calls await asyncio.to_thread(r._finalize, ...) in the test body and asserts the thread differs — that tests asyncio, not this code. Since _finalize went back to sync, it no longer covers the two real call sites (_run_via_backend, _run_via_subprocess). Drive one of those, or delete it. The other two tests go through _stage_apply and _stage_provision_attempt_runtime, so they are real.

3. The PR body is stale. It still says "three executor helpers", still lists "short git helpers" as the out-of-scope, and Limitations still reads "threads cannot be cancelled" with no mention of CancelScope. The new scope and the cancel semantics exist only in the commit message and the docstrings; the body is what reviewers and git log --merges readers see.

The two follow-ups from my earlier comment still stand: no loop-lag/slow-callback detection, and the criterion (upper bound shorter than the shortest timeout running concurrently on the loop) is not written next to dispatcher.py:220. A fresh example of why that matters: _gc_attempt_dir shutil.rmtrees a multi-GB failed venv on the loop thread (integrate_patch.py:2047, 2060, 2866). It is tens of seconds on shared storage, it is exactly what the written criterion would catch, and it is exactly what a hand-maintained list misses.

[siliangchen-amd]

All three nits are in. GC rmtree / lag detector / dispatcher criterion were left for follow-up.

Docstring. _stage_provision_attempt_runtime no longer repeats “and on success stores the resolved runtime on”. One sentence: off-loop, in-flight pip is not killed.

Vetting test. test_patch_vetting_runs_off_the_event_loop_thread now goes through SpecialistRunner.run + MockBackend (SPECIALIST_DONE), so the spy on vet_patches hits the production _run_via_backend wrap, not a to_thread in the test body.

Ship. Pushed and PR body replaced.