fix(inner): drop the prompt future from _pending on every ACP-family turn exit (acp/goose/qwen) - #4370
Conversation
`AcpExecutor.run_turn` registers its `session/prompt` request future in `self._pending[req_id]`, but the stdout reader removes it only when it matches a RESPONSE. On the timeout path, and when the reader resolves the future with an exception (EOF / reader error) rather than a match, nothing removes `req_id` -- so `self._pending` accumulates one stale entry per silent or failed turn on a long-lived ACP session (a slow memory leak; a late response could also resolve a stale future). `_rpc` already guards its own timeout this way; `run_turn` did not. Wrap the prompt loop in try/finally that pops `req_id`, so every exit path (timeout, EOF/reader-error, normal completion, or an abandoned turn) leaves `_pending` clean. The happy path is unchanged: the reader still pops on the matching response, and the finally pop is then a no-op. Regression test drives a turn whose response never arrives and asserts `_pending` is empty afterward (fails without the finally). Signed-off-by: abhay-codes07 <abhaysingh0293@gmail.com>
|
@abhay-codes07 Thanks for the PR! It doesn't reference an issue yet. We require an issue for every PR, so the work can be prioritized before it's reviewed. Add one to the description:
No issue exists for this yet? Open one first, then reference it. That's how we track what's worth doing, and it's usually quicker than it sounds. Note a reference has to point at an issue: naming another PR doesn't count. The only exceptions are changes with no user-visible behaviour: pure Refactor / chore, Docs, or Test / CI work. If that's genuinely what this is, check that box under Type of change. Anything that fixes a bug, adds a feature, or changes the UI needs an issue, even when it also touches docs or tests. See CONTRIBUTING.md for the full policy. No action is taken beyond this comment. |
|
@abhay-codes07 This PR is a Bug fix, Feature, or UI / frontend change but the Demo section is missing or only contains a placeholder. These change types require a screenshot or screen recording so reviewers can see the new behaviour without checking out the branch. Please update the Demo section with:
Use |
…cutors GooseExecutor and QwenExecutor are the vendor-specific ACP executors the generic AcpExecutor was factored from, and they carry the identical run_turn leak: the session/prompt future is registered in self._pending[req_id] but only popped by the stdout reader on a matched response, so the timeout / EOF paths leak one stale future per silent or failed turn on a long-lived session. Wrapped each prompt loop in the same try/finally that drops req_id on every exit, and added the matching timeout regression test to both suites (each fails without the finally). Signed-off-by: abhay-codes07 <abhaysingh0293@gmail.com>
github-actions
Bot
added
size/XL
abhay-codes07
changed the title
|
Extended the fix in a second commit to |
Summary
Self-discovered resource leak in the generic ACP executor.
AcpExecutor.run_turnregisters itssession/promptrequest future inself._pending[req_id](so the stdout reader can resolve it), but_read_stdoutremoves the entry only when it matches a response (fut = self._pending.pop(msg_id)). Two exit paths never get that match:ExecutorError, and returns.req_idis left in_pending._read_stdoutresolves the future withset_exception(...)(broadcast over_pending.values()) without popping, sorun_turn'sfut.result()raises and it returns withreq_idstill present.On a long-lived ACP session (one process, many turns) each silent/failed turn leaves one stale, already-resolved future in
_pending— a slow memory leak, and a late response for an old id could resolve a stale future._rpcalready guards its own timeout (self._pending.pop(req_id, None));run_turndid not.Fix: wrap the prompt loop in
try/finallythat popsreq_idon every exit (timeout, EOF/reader-error, normal completion, or an abandoned generator). The happy path is unchanged — the reader still pops on the matching response and thefinallyis then a no-op.Test Plan
Added
test_run_turn_timeout_does_not_leak_pending_futuretotests/inner/test_acp_executor.py: drives a turn whose response never arrives (stubbed session, no reader) and asserts_pending == {}afterward.Verified it fails without the
finally(the future lingers) and passes with it. Full file: 34 passed (the 2test_end_to_end_*fake-agent e2e tests fail identically on cleanmainin this non-POSIX dev env — unrelated).ruff check+ruff format --checkclean.Demo
N/A. Internal resource-cleanup fix, no user-visible surface.
Type of change
Test coverage
Changelog
Fixed a slow memory leak in the ACP executor: a silent or failed turn no longer leaves its request future behind in the pending-request map.