fix(agent): bound context compression summary stalls (salvage #49905) - #56295
Closed
kshitijk4poor wants to merge 2 commits into
Closed
fix(agent): bound context compression summary stalls (salvage #49905)#56295kshitijk4poor wants to merge 2 commits into
kshitijk4poor wants to merge 2 commits into
Conversation
…bort Rebase follow-up on the compression hard-timeout salvage. The new hard timeout raises a builtin TimeoutError, but _generate_summary's handler set _is_streaming_closed = _is_connection_error(e) — and _is_connection_error classifies ANY exception whose type name contains 'Timeout' as a network close. That set _last_summary_network_failure=True, so compress() took the network-failure ABORT branch (return messages unchanged) instead of the deterministic-fallback path — the opposite of the PR's intent (a timeout should fall back to a best-effort summary, not abort compression). Gate it: _is_streaming_closed = _is_connection_error(e) and not _is_timeout. Timeouts (our hard-timeout, or 408/429/504) are already handled by the _is_timeout classification above; only a genuine mid-stream close should trip the network-failure abort. The contributor's own test_compress_hard_timeout_uses_existing_fallback_path guards this (mutation-verified: reverting the gate fails it).
kshitijk4poor
enabled auto-merge (rebase)
July 1, 2026 10:30
Collaborator
Author
|
Withdrawing this salvage for now — closing per maintainer direction to focus the current batch elsewhere. The underlying #49905 (bound context compression summary stalls) remains open and can be re-salvaged later. No changes merged. |
auto-merge was automatically disabled
July 1, 2026 10:33
Pull request was closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Salvage of #49905 by @LeonSGP43 (rebased onto current
main+ a rebase-follow-up fixing a production gap the rebase exposed). Bounds context-compression summary generation with a hard timeout so a wedged summary call can't block the gateway loop indefinitely (#49768).The bug
call_llmforwardsauxiliary.compression.timeoutto the provider client as a socket/read timeout only — it does not bound the caller's wait if the network stack wedges or the SDK ignores it. The synchronous summary call incontext_compressor.pywas wrapped only inaux_interrupt_protection()(which suppresses interrupts — the opposite of a timeout), so compression summary generation could block the gateway loop forever. Confirmed still present onmain.The fix
@LeonSGP43's fix: run the summary
call_llmin aThreadPoolExecutor(max_workers=1)worker and stop waiting afterauxiliary.compression.timeout(future.result(timeout=...),shutdown(wait=False, cancel_futures=True)). On timeout it raises, caught by the existing handler and routed into the deterministic fallback summary path — the gateway stays responsive.Rebase + production-gap fix (this salvage)
The PR was ~592 commits behind; the compressor drifted (two cosmetic conflicts). Resolved by composing main's
aux_interrupt_protectionwrapper + defensive message coercion with the PR's timeout call (documented the thread-local interaction honestly: the protection guards caller-thread unpacking, the hard timeout is the stronger guarantee for the wedged-socket case — complementary, not nested).Fixing the rebase surfaced a real production gap: the hard timeout raises a builtin
TimeoutError, but_generate_summary's handler set_is_streaming_closed = _is_connection_error(e)— and_is_connection_errorclassifies any exception whose type name contains "Timeout" as a network close. That set_last_summary_network_failure=True, makingcompress()take the network-failure ABORT branch (return messages unchanged) instead of the deterministic fallback — the opposite of the PR's intent. Gated it:_is_streaming_closed = _is_connection_error(e) and not _is_timeout. Mutation-verified against the contributor's owntest_compress_hard_timeout_uses_existing_fallback_path(reverting the gate fails it). Genuine streaming-closes don't carry "timeout" wording, so they're unaffected.Review
Ran hermes-agent-dev + hermes-pr-review Phase 2c — 0 Critical. The executor pattern is correct, the abandoned-thread trade-off is bounded/documented,
auxiliary.compression.timeoutconfirmed inDEFAULT_CONFIG, and the false-suppression edge for the_is_timeoutgate was independently checked (genuine mid-stream closes are correctly preserved).Tests
Supersedes #49905. Full credit to @LeonSGP43.