The `future.done()` guard added for dead workers (#117261 / #63892)
unconditionally logged `future.exception()` and returned `(False, None)`.
If the worker finished SUCCESSFULLY in the window between
`result(timeout=)` expiring and the `done()` check, that discarded a
completed compression and sent the caller down the stall/fallback path —
`future.cancel()` becomes a no-op on a settled future and the fallback
route costs a second LLM call. Siblings `_await_in_flight_commit` and
tool_executor's `_poll_sequential_future` already re-read the result on
a settled future; do the same here: `exception() is None` → return
`(True, result)`, otherwise take the stall path as before.
The compression-seam test grows a settled-successful case through the
same helper (a Future subclass whose first timed `result()` still raises
TimeoutError, modelling the race); it fails on the previous guard and
passes with this one.
Gate mutation testing showed the tool_executor.py half of the fix was
untested: removing the `if future.done()` guard in _poll_sequential_future
left every test green. The compression-side tests exercise
conversation_compression only; tool_executor is a separate seam with the
same 3.11+ TimeoutError-aliasing bug.
The new test submits a worker that dies with TimeoutError and polls it with
a 2 s deadline. On base the loop re-waits on the settled future until the
deadline and returns ("timeout", None) — the test fails with DID NOT RAISE
after 2 s. With the guard the worker's TimeoutError propagates within
0.5 s. Verified red by temporarily checking out origin/main's
agent/tool_executor.py, then green after restoring it.
pytest-timeout is not a project dependency (0 hits in pyproject.toml and
uv.lock), so `@pytest.mark.timeout(10)` was an unknown marker that did
nothing in CI while the docstring claimed it bounded the red-on-base hang.
The ceiling-less commit-wait loop really does never return on base; what
bounds that is the runner's per-file timeout (scripts/run_tests.sh,
HERMES_TEST_FILE_TIMEOUT). Say so instead of asserting a guard that
isn't there.
Keep test_compression_wait_returns_promptly_when_worker_dies (builtin
TimeoutError only — asyncio.TimeoutError IS the same class on 3.11+, so the
parametrize was a duplicate; it also asserts the _join_cancelled_worker
sibling guard) and test_in_flight_commit_surfaces_worker_timeout_instead_of_looping.
Drop the alias premise test, the live-worker/non-timeout regression checks
and the tool_executor poll test (same guard, same shape).
Imports are top-level (no __import__ tricks). idle/ceiling are 1s/2s so the
red-on-base run fails in ~1s on the elapsed assertion instead of burning 30s,
and the commit-wait test carries pytest.mark.timeout(10) — pytest-timeout is
already a project dev dependency (tests/agent/lsp/test_custom_servers.py uses
the marker; conftest tunes --timeout-method) — because on base that loop never
returns.
_join_cancelled_worker returned False from its `except
concurrent.futures.TimeoutError:` arm. On 3.11+ that class IS the builtin
TimeoutError, so the arm also fires when the cancelled worker itself DIED
raising a timeout-class error (the aux client raises bare TimeoutError on a
stalled summary stream). The caller, _release_cancelled_worker, treats False
as "still running": it logs 'did not exit within grace' and skips
fence.allow_cancelled_lock_release(), so the session compression lease of a
provably-dead worker was retained/orphaned.
Return future.done() instead: a settled future never becomes unsettled, so a
done future means the thread exited and the lease can be released. A live
worker that merely outlasted the grace still yields False.
Same guard family as the sibling loops fixed in the preceding pick (#117261).
On Python 3.11+ concurrent.futures.TimeoutError IS the builtin TimeoutError
(asyncio.TimeoutError and socket.timeout alias it too). Poll loops shaped like
try:
return future.result(timeout=slice)
except concurrent.futures.TimeoutError:
...keep waiting...
therefore cannot distinguish "the wait slice expired" (worker alive) from "the
worker raised TimeoutError" (worker dead). auxiliary_client raises a bare
TimeoutError when a summary stream stalls, so this is reachable in production.
When it happened the host re-waited on an already-settled future. result() then
returned instantly every iteration, spinning at ~2k iterations/sec and logging
"Context compression still streaming" about a dead worker, until the entire idle
budget elapsed. One session burned 535s and wrote ~90k duplicate log lines
(15.6MiB) before failing with context_compression_timeout, and every later turn
re-entered the same path.
Guard each loop with future.done(): a settled future never becomes unsettled.
- _await_worker_within_budget: take the stall path at once, so the configured
fallback chain is actually reached instead of after a 120s false stall.
- _await_in_flight_commit: re-raise the worker's exception. This loop had no
ceiling, so a dead worker spun forever.
- tool_executor._poll_sequential_future: same, and with deadline=None it also
span indefinitely.
Non-timeout worker exceptions still propagate unchanged, and a live worker still
polls exactly as before.
Regression tests pin all three. Against unpatched code the two wait tests fail
and the commit-wait test hangs until the 300s harness SIGKILL, reproducing the
infinite loop directly.
(cherry picked from commit 274457304ce0393407574fe4e43c4a450f20bac3)
