fix(acp): abort the turn immediately when close() lands mid-prompt

[onatozmenn]

HUMAN:

I addressed this because closing the agent during an active prompt could trigger unnecessary retries and eventually fail with an unrelated AttributeError. I verified the fix with the original reproducer and new regression tests, confirming that the turn now aborts immediately with the expected closed-agent error.

---

AGENT:

Why

Fixes #4329.

ACPAgent.close() runs on whatever thread calls it and drops _executor / _process, while a Conversation.run() on another thread can be inside step() between its own None check and the call. Two things go wrong when that happens:

1. Killing the subprocess first surfaces as Connection closed, which the prompt loop treats as a retriable connection error. So the turn sleeps a full retry delay (5s by default on the sync path, up to 5+15+30s on the async path) before it can fail, even though a closed agent can never succeed. close() from another thread is the only way to abort a step() that is blocked for the whole turn (pause() / interrupt() only take effect between steps), so that delay defeats the entire point of calling it.
2. The retry then dereferences self._executor, which is now None, and the turn dies with AttributeError: 'NoneType' object has no attribute 'run_async' instead of anything a caller can act on.

One correction to the issue report, which was filed against 1.38.0: on current main the exception is not swallowed and run() does exit, since step() re-raises after _emit_turn_error (added in #4126), so ConversationRunError propagates. What the reporter measured as "run() never returned within 5s" is the retry delay landing exactly on their 5s bound. The race itself and the AttributeError are real, and the abort latency is the substantive part.

Summary

• ACPAgent._require_executor() reads _executor once so a concurrent close() cannot null it out between the check and the call, and raises ACPAgentClosedError (a RuntimeError subclass, so existing RuntimeError handling still applies) when teardown has already started. Used by the sync prompt loop and by the astep() portal read.
• Both retry loops bail out as soon as _closed is set instead of sleeping through a retry delay. _closed is assigned before teardown begins, so it also covers the window where _executor is still assigned but its portal is going away.
• astep() re-reads self._require_executor().portal on every attempt instead of capturing it once before the loop, mirroring what the sync path already does with the executor. Without that, a teardown landing between two attempts reaches a stale portal and anyio raises RuntimeError("This portal is not running"), which is not in _RETRIABLE_CONNECTION_ERRORS and slips past the _closed guard as a generic ACPPromptError.
• New ACPAgentClosed code on the emitted ConversationErrorEvent, so a client can tell "someone closed this agent" apart from a generic prompt failure.

Issue Number

Fixes #4329

How to Test

End-to-end, using the reporter's own reproducer (stdlib-only ACP stub whose session/prompt sleeps forever; close() is called from the main thread 1.5s into a blocked turn). I widened their future.result(timeout=5) to 20s so the outcome is measured rather than clipped by the bound, and printed how long the abort actually took.

uv run python repro_close_step_race.py 3

Before, on main @ 8ce9300 (3/3):

[13:55:55] WARNING  ACP prompt failed with retriable error (attempt 1/4), retrying in 5s: Connection closed
[13:56:00] ERROR    ACP prompt failed (AttributeError): 'NoneType' object has no attribute 'run_async'
attempt 1: ConversationRunError propagated after 5.1s: ... 'NoneType' object has no attribute 'run_async'
attempt 2: ConversationRunError propagated after 5.0s: ... 'NoneType' object has no attribute 'run_async'
attempt 3: ConversationRunError propagated after 5.0s: ... 'NoneType' object has no attribute 'run_async'

After, with this branch (3/3), no retry warning and no AttributeError:

attempt 1: ConversationRunError propagated after 0.0s: ... ACP agent was closed while a prompt was in flight
attempt 2: ConversationRunError propagated after 0.0s: ... ACP agent was closed while a prompt was in flight
attempt 3: ConversationRunError propagated after 0.0s: ... ACP agent was closed while a prompt was in flight

Unit coverage, five new tests in TestACPPromptRetry:

uv run pytest tests/sdk/agent/test_acp_agent.py -k PromptRetry

• test_close_during_prompt_aborts_without_retry_delay, one attempt, time.sleep never called, ACPAgentClosedError, error event code ACPAgentClosed
• test_close_during_astep_aborts_without_retry_delay, same for the async loop
• test_step_after_teardown_reports_closed_agent / test_astep_after_teardown_reports_closed_agent, a torn-down executor is reported as a closed agent, not a None dereference
• test_astep_reports_closed_agent_when_teardown_lands_between_retries, the portal drops the executor after the first attempt and returns a retriable failure, so the loop reaches a second attempt against dead state. With the per-attempt read: one attempt, ACPAgentClosedError, code ACPAgentClosed. With the read hoisted back out of the loop: four attempts and a ConnectionError.

I checked the tests actually pin the behaviour by neutralising the guards: all of them fail (the async ones visibly sleeping 5+15+30s), and the seven pre-existing retry tests still pass.

tests/sdk/agent 445 passed, tests/sdk/conversation 764 passed / 1 skipped. Five tests were excluded as pre-existing Windows failures unrelated to this change, verified failing identically on unmodified main: the four TestACPFileSecretMaterialisation / TestACPDataDirIsolation cases (POSIX 0o600 expectations) and test_conversation_large_event_handling (disk-speed timeout on this host).

ruff format, ruff check, pycodestyle --max-line-length=88, and scripts/check_import_rules.py are clean on both touched files.

Video/Screenshots

The before/after console output above is the evidence; this path has no UI surface.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

• Behaviour change: a connection error raised while _closed is set now fails as ACPAgentClosedError rather than being retried. That only affects agents that are already being torn down, where every retry was doomed anyway.
• Scoped to the teardown race. The issue's alternative suggestion, a lock shared between close() and the whole prompt path, would make close() block for the duration of the turn it is trying to abort, which is the opposite of what the reporter needs.
• ACPAgentClosed is deliberately not added to classify_error's code table, so it classifies as unknown / non-retryable. ACPStartupTimeout and UsagePolicyRefusal come out of the same module and are not in that table either, so adding only this one would make it the odd one out. Happy to do all three in a follow-up if that is wanted.
• Adjacent observation, left alone deliberately: LocalConversation.run() breaks on PAUSED / STUCK / FINISHED but not on ERROR. It does not bite here because ACPAgent.step() re-raises, but an agent that sets ERROR without raising would be stepped again. Happy to open a separate issue if that is not intentional.

_{Investigated and implemented with AI assistance.}

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: b88d5344bf69f55d3d74863fcbdb00b63cd35331
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/99bed795-bc7e-4a85-8011-aa23cb8c8c71

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Verdict: Approve (low risk)

The change correctly closes the teardown race described in #4329. I read the full step()/astep() retry loops, the _shutdown_runtime ordering, the new _require_executor() helper, and the error classifier, and ran the new and pre-existing TestACPPromptRetry suite (11/11 pass) plus the full tests/sdk/agent/test_acp_agent.py (436/436 pass).

Correctness

• close() sets _closed = True under _file_credential_lock before _shutdown_runtime() kills the subprocess/drops _executor. Because the ConnectionError that surfaces to the prompt loop is a consequence of process.terminate(), _closed is already True when the retriable error is caught. Both retry loops read _closed and raise ACPAgentClosedError immediately, so the doomed retry delay (and the subsequent None deref) are gone. This ordering is the load-bearing part and it holds.
• _require_executor() reads _executor once into a local, closing the check-then-call TOCTOU for the initial run_async/portal access. The post-call teardown window is then caught by the _closed guard inside the _RETRIABLE_CONNECTION_ERRORS handler. Layered correctly.
• ACPAgentClosedError(RuntimeError) keeps existing RuntimeError handling working, and _classify_acp_turn_error surfaces a distinct ACPAgentClosed code. Good for clients that want to branch on "closed by someone" vs. a generic prompt failure.
• _require_executor() returns Any, which matches the existing _executor: Any PrivateAttr typing; no new # type: ignore introduced.

Tests

Four new tests pin the two guards (sync + async) and the post-teardown None-executor case. The author states they neutralised both guards and confirmed all four fail (the async one visibly sleeping 5+15+30s), which is the right way to prove the tests exercise the new code rather than passing for incidental reasons. The sync test also asserts time.sleep is never called, which directly pins the "no retry delay" behaviour.

Non-blocking observations (not requesting changes)

1. Captured-portal asymmetry on the async path. astep reads portal = self._require_executor().portal once before the retry loop, while step re-reads the executor (self._require_executor().run_async(...)) on every attempt. If a torn-down portal's start_task_soon raised a retriable error, the async loop would still abort via the _closed check; but anyio raises a plain RuntimeError("This portal is not running") there, which is not in _RETRIABLE_CONNECTION_ERRORS, so it would bypass the _closed guard and be classified as ACPPromptError rather than ACPAgentClosed. I verified this empirically with a synthetic portal. In the real close() flow this window is unreachable — _closed is set before the teardown that produces the first ConnectionError, so the first retriable error already hits the _closed check and the loop never reaches a second attempt against a stale portal. So this is defensive, not a live bug. If you ever want symmetry, re-reading self._require_executor().portal per attempt (or adding RuntimeError from a closed portal to the closed-agent path) would make the async path match the sync one, but it is not required for correctness today.

2. Async test doesn't assert asyncio.sleep was not called. test_close_during_astep_aborts_without_retry_delay asserts call_count == 1, which transitively proves no sleep ran (sleep is inside the post-_closed-check branch that raises). The sync counterpart is more explicit (patch + mock_sleep.assert_not_called()). Minor inconsistency only; not worth a change.

3. ACPAgentClosedError is module-level but not exported from openhands.sdk. That's fine for now (callers catch RuntimeError/ConversationRunError), but if it is intended to be part of the public catch surface, consider documenting it. Not a blocker.

The fix is appropriately scoped to the teardown race and deliberately avoids the shared-lock alternative (which would make close() block for the duration of the turn it is meant to abort) — that trade-off is the right call.

[onatozmenn]

Thanks for the read, and for checking the _closed-before-teardown ordering rather than taking it on trust. That ordering is the load-bearing part and I would rather it were pinned by something other than my own reasoning.

Took observation 1 in b296caa. You are right that it is unreachable through close() today, but the asymmetry was not deliberate on my part, and relying on "the first retriable error always arrives after _closed is set" makes correctness depend on the ordering inside _shutdown_runtime staying the way it is. astep now re-reads self._require_executor().portal per attempt, exactly as step re-reads the executor, so a teardown landing between two attempts raises ACPAgentClosedError instead of anyio's non-retriable RuntimeError("This portal is not running") slipping past the _closed guard as an ACPPromptError.

Added test_astep_reports_closed_agent_when_teardown_lands_between_retries for it. The portal drops the executor after the first attempt and returns a retriable failure, so the loop reaches a second attempt against dead state. With the fix: one attempt, ACPAgentClosedError, error code ACPAgentClosed. With the read hoisted back out of the loop: four attempts and a ConnectionError. Retry delays are patched to zero rather than mocking asyncio.sleep.

On 2, I left it alone for the reason you gave, plus a practical one: await asyncio.sleep here would mean patching the shared asyncio module for the duration of an asyncio.run, which reaches further than the code under test. call_count == 1 proves the same thing without that.

On 3, ACPAgentClosedError is intentionally not in openhands.sdk.__all__. Exporting it puts it under check_sdk_api_breakage.py, and I would rather not commit to that surface on a bug fix. It subclasses RuntimeError and the typed ACPAgentClosed code is on the ConversationErrorEvent, so both the catch path and the branch-on-reason path work without it. Happy to export it if a maintainer wants it public.

_{Investigated and implemented with AI assistance.}

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Verdict: Approve (low risk)

The follow-up commit (b296caa) cleanly addresses the prior review's non-blocking observation about the async/sync portal-read asymmetry: astep now re-reads self._require_executor().portal per attempt inside the retry loop, mirroring step's per-attempt self._require_executor().run_async(...). I re-read the full step()/astep() retry loops, _shutdown_runtime ordering, _require_executor, _emit_turn_error, _classify_acp_turn_error, and classify_error, and ran the new + pre-existing TestACPPromptRetry suite (12/12 pass) and the full tests/sdk/agent/test_acp_agent.py (437/437 pass). ruff check/ruff format --check are clean on both touched files.

Correctness

• The per-attempt re-read in astep closes the only remaining gap. A teardown that lands after a retriable failure but before the next attempt now hits _require_executor() (which sees _executor is None and raises ACPAgentClosedError) instead of reaching a stale portal whose start_task_soon would raise anyio's RuntimeError("This portal is not running") — a plain RuntimeError that is not in _RETRIABLE_CONNECTION_ERRORS and would have bypassed the _closed guard. The new regression test test_astep_reports_closed_agent_when_teardown_lands_between_retries pins exactly this.
• _require_executor() reads _executor once into a local and also checks _closed, so the check-then-call TOCTOU on the initial access is closed, and the post-call teardown window is caught by the _closed guard inside the _RETRIABLE_CONNECTION_ERRORS handler. ACPAgentClosedError(RuntimeError) is not a member of _RETRIABLE_CONNECTION_ERRORS, so when _require_executor() raises it (between retries), it propagates straight to the outer except Exception → _emit_turn_error → _classify_acp_turn_error → "ACPAgentClosed". Correct, no accidental retry.
• close() sets _closed = True under _file_credential_lock before _shutdown_runtime() kills the process/drops _executor, so the ConnectionError that surfaces to the prompt loop is a consequence of teardown and _closed is already True when it is caught. Both loops short-circuit immediately. The ordering is the load-bearing part and it holds.
• Deliberately avoiding the shared-lock alternative (which would make close() block for the duration of the turn it is meant to abort) is the right trade-off; Python's GIL makes the unlocked _closed/_executor reads acceptable here.

Tests

Five new tests pin both guards (sync + async), the post-teardown None-executor case, and the new between-retries re-read case. The author states they neutralised the guards and confirmed all fail (the async one visibly sleeping 5+15+30s), which proves the tests exercise the new code. The sync test additionally asserts time.sleep is never called, directly pinning the "no retry delay" behaviour.

Non-blocking observations (not requesting changes)

1. ACPAgentClosed is not mapped in classify_error, so its ErrorClassification is UNKNOWN (non-retryable). I verified this empirically: classify_error("ACPAgentClosed", "ACP agent was closed while a prompt was in flight") → UNKNOWN, retryable=False. For a closed agent, non-retryable is the correct semantic — you must not retry a torn-down agent — so this is fine and arguably desirable. The note is only that consumers branching on classification.kind rather than code get no structured signal beyond UNKNOWN; if that matters for UX, ACPAgentClosed could be added to the code-based table. Not a blocker.

2. The async abort test does not assert asyncio.sleep was not called. test_close_during_astep_aborts_without_retry_delay asserts call_count == 1, which transitively proves no sleep ran (the sleep sits inside the post-_closed-check branch that raises). The sync counterpart is more explicit (patch + assert_not_called()). Minor inconsistency only.

The fix is appropriately scoped to the teardown race and is sound.