fix(stream): bound a single streaming call and ignore keep-alive frames (#83657) - #84066
fix(stream): bound a single streaming call and ignore keep-alive frames (#83657)#84066JoaoMarcos44 wants to merge 1 commit into
Conversation
…es (NousResearch#83657) A nousresearch call ran 1239s inside one turn and pushed a cron run past its window, while the provider's edge was also serving a cluster of 502 origin_bad_gateway responses. The 502s are upstream and already handled by retry + fallback. The unbounded call is ours: two client-side guards were missing. 1. The stale-stream detector timestamped EVERY stream event, including content-free ones. Its own comment promises to kill "connections kept alive by SSE pings but no actual data" — which is precisely what it could not do, because an Anthropic `ping` or an empty OpenAI-style delta refreshed its patience. `stream_chunk_is_progress` now decides: content, reasoning, tool calls, a role opener, a finish reason, usage or any non-empty vendor field in `model_extra` count as progress; pings and empty deltas do not. Unknown chunk shapes count as progress, so an unfamiliar provider is never starved of patience. 2. Nothing bounded the total duration of one streaming call. Every guard we had — the stale detector, the agent activity tracker, the gateway's HERMES_AGENT_TIMEOUT, the cron leash — measures silence and resets on provider traffic, so a provider dripping real tokens outlives all of them. `resolve_stream_hard_timeout` adds the one bound activity cannot push back, mirroring the non-streaming stale budget, the codex hard ceiling (NousResearch#64507) and auxiliary_client's total_timeout. The ceiling defaults to the already-documented HERMES_API_TIMEOUT (1800s) and is floored at the stale-stream timeout so it can never preempt the detector. An explicit `providers.<id>.max_call_seconds` (or `.models.<model>.max_call_seconds`) or HERMES_STREAM_MAX_CALL_SECONDS is honored verbatim; 0 disables it. Local endpoints stay exempt unless configured explicitly, so long local generations keep working. On expiry the request is marked cancelled before the transport is aborted (NousResearch#6600 contract), the socket abort stays on the stranger-thread path that never releases FDs from the poll thread (NousResearch#29507 / NousResearch#67142 / NousResearch#70773), and the TimeoutError is handed to the existing retry / provider-fallback chain rather than reconnecting in place — reconnecting would hand the same wedged provider a brand-new budget. hermes_cli/timeouts.py grew `get_provider_max_call_timeout` on top of a `_lookup_provider_timeout` helper extracted from the two existing lookups; their behavior and module paths are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reproduced against head Minimal counterexample: The cause is that the new generic Minimal fix: let only the max-call lookup preserve zero (for example an The existing 30 targeted tests pass; this case is not covered. The PR also now conflicts with current |
|
I prepared the minimal follow-up directly on this PR's source HEAD (
It only preserves Fresh validation on the exact composed branch: 32 targeted tests pass; Ruff, Windows-footgun check, and |
|
Current-main handoff is now pushed without opening a duplicate upstream PR: https://github.com/enzo-adami/hermes-agent/tree/agent/pr84066-main-refresh Base is exact main 165c889; commit 83f281c preserves joaomarcos as primary author and includes the narrow max_call_seconds=0 correction from JoaoMarcos44#15. Net scope remains the six watchdog/config/docs files. Fresh validation: 32/32 dedicated first-byte/idle/max-call/keepalive tests, Ruff, Windows-footgun checker, and diff-check all pass. The branch is a force-update/cherry-pick handoff for #84066, not a replacement PR. |
imma fix this now, tysm |
Fixes #83657.
What the issue really contains
Two independent failures were reported together:
502 origin_bad_gatewaycluster frominference-api.nousresearch.comclassify_api_errormarks it retryable and the fallback chain swaps providers. Nothing to fix client-side.chat.completionscall ran 1239s and pushed a cron run past its windowRoot cause
1. Keep-alive frames counted as progress
The stale-stream detector's own comment promises it kills "connections kept alive by SSE pings but no actual data". It could not: the poll loop stamped
last_chunk_timeon every event, so an Anthropicpingor an empty OpenAI-style delta refreshed its patience. A provider heartbeating into the void was indistinguishable from one generating tokens.stream_chunk_is_progressnow decides what counts:reasoning_content, tool-call or function-call deltas, a role opener, afinish_reason,usage, or any non-empty vendor field inmodel_extrapingevents, deltas whose every field is empty, chunks with neither choices nor usage2. Nothing bounded a single streaming call
Every guard Hermes had measures silence and resets on provider traffic:
HERMES_STREAM_STALE_TIMEOUT)_touch_activity)HERMES_AGENT_TIMEOUT)HERMES_CRON_TIMEOUT)A provider dripping real tokens resets all four and can hold one call open indefinitely. That is the exact shape of the 1239s call — and the streaming path was the only one without a ceiling: the non-streaming path has its 90s stale budget, the codex path has a hard ceiling (#64507), and
auxiliary_clienthastotal_timeout.resolve_stream_hard_timeoutadds the one bound activity cannot push back.%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%% graph TD A[🔌 Provider Stream Event] --> B{⚡ stream_chunk_is_progress?} B -->|Content / Reasoning / Tool Call / Finish / Usage| C[🟢 Refresh Idle Window] B -->|Anthropic ping / Empty Delta| D[🚫 Ignored - Window Keeps Aging] C --> E{⏱️ Idle > Stale Timeout?} D --> E E -->|Yes| F[🔪 Stale Kill - Reconnect + Breaker Bump] E -->|No| G{🧭 Total Elapsed > Wall-Clock Ceiling?} G -->|No| A G -->|Yes| H[🛑 Mark Request Cancelled #6600] H --> I[🔌 Abort Transport - Stranger-Thread Safe #29507/#67142/#70773] I --> J[🚀 TimeoutError to Retry / Provider Fallback]Infographic :
Design decisions
HERMES_API_TIMEOUT(1800s), floored at the stale timeout. The ceiling must never preempt the stale detector, which owns reconnect + diagnostics. The floor applies only to the derived default — an explicitmax_call_seconds/HERMES_STREAM_MAX_CALL_SECONDSis honored verbatim, including small values.is_local_endpoint), so long local generations on Ollama / llama.cpp are not newly killed.0disables it._request_cancelledis set before the abort (fix(agent,gateway): voice interrupts + cascading interrupt hang #6600) so the worker reads its own torn transport as a cancellation, and the abort uses the stranger-thread path that never releases FDs from the poll thread (Interrupted OpenAI/httpx request thread survives across turns and writes TLS record bytes to unrelated file descriptors on delayed close #29507 / Anthropic stale-stream watchdog can still corrupt SQLite via TLS FD reuse #67142 / Custom OpenAI-compatible provider still corrupts SQLite via TLS FD reuse in v0.18.2 — streaming stale watchdog closes shared client pool from non-owner thread #70773).hermes_cli/timeouts.pygainedget_provider_max_call_timeouton top of a_lookup_provider_timeouthelper extracted from the two existing lookups — same behavior, same module paths, so existingpatch("hermes_cli.timeouts.get_provider_request_timeout")call sites are unaffected.Regression surface checked
HERMES_LOCAL_STREAM_STALE_TIMEOUTbehavior untouched)._accept_stream_chunkshares the same predicate, so the two paths cannot drift.Test plan
New:
tests/agent/test_stream_progress_and_ceiling_83657.py(26 unit tests) andtests/run_agent/test_stream_drip_watchdog_83657.py(4 behavioral tests driving the real poll loop with self-bounding fake streams, so a regression fails instead of hanging)._consecutive_stale_streamsTimeoutErrorand a torn-down transport0disables, local exempt, explicit local value honored, config-lookup failure degrades to the defaultpytest tests/agent/test_stream_progress_and_ceiling_83657.py tests/run_agent/test_stream_drip_watchdog_83657.py tests/run_agent/test_streaming.py tests/run_agent/test_stream_stale_breaker_reset.py tests/hermes_cli/test_timeouts.py tests/agent/test_stream_read_timeout_floor.py tests/agent/test_local_stream_timeout.py→ 114 passedtest_reasoning_stale_timeout_floor,test_non_stream_stale_timeout,test_relay_llm,test_moa_trace_streamed_capture,test_aux_progress_streaming,test_anthropic_adapter,test_nous_portal_anthropic_wire) → 174 passed, 2 skippedDocs
website/docs/reference/environment-variables.md— newHERMES_STREAM_MAX_CALL_SECONDSrow; theHERMES_STREAM_STALE_TIMEOUTrow now states that keep-alive frames do not reset it.website/docs/user-guide/configuration.md— new "Streaming call ceiling" row in the API Timeouts table plus a section explaining why an activity-based guard cannot catch this failure shape.