Skip to content

fix(streaming): gate stale-timer reset on content chunks (ignore keepalive pings) - #34541

Open
sh940701 wants to merge 1 commit into
NousResearch:mainfrom
sh940701:pr/streaming-stale-timer-gate
Open

fix(streaming): gate stale-timer reset on content chunks (ignore keepalive pings)#34541
sh940701 wants to merge 1 commit into
NousResearch:mainfrom
sh940701:pr/streaming-stale-timer-gate

Conversation

@sh940701

Copy link
Copy Markdown

What does this PR do?

Fixes a 10-minute stream stall on the ChatGPT codex backend (chatgpt.com/backend-api/codex).

The chat_completions streaming loop in agent/chat_completion_helpers.py resets last_chunk_time on every chunk. That backend keeps a stalled stream alive with SSE keepalive pings while delivering zero content tokens. The unconditional reset defeats both this detector AND the httpx read timeout — a hung request stays alive until the backend gives up (~10 min) instead of being killed and retried at HERMES_STREAM_STALE_TIMEOUT (the immediate retry succeeds in seconds).

A trivial "pong" request was measured at 615 s before the fix and ~4 s with the immediate retry.

The fix gates the last_chunk_time reset on chunks that carry real progress (content / reasoning / reasoning_content / tool_calls / function_call deltas). Keepalive pings still drive the per-attempt diagnostic counters but no longer mask the stall.

Related Issue

Fixes #

(No related issue — opening this PR to start that conversation. Happy to file one separately if preferred.)

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/chat_completion_helpers.py (around the chat_completions stream loop, current upstream line ~1761) — replace the unconditional last_chunk_time["t"] = time.time() with a content-progress predicate that mirrors the OpenAI chunk shape: reset only when choices[0].delta has at least one of content, reasoning_content, reasoning, tool_calls, function_call. Diagnostic counters (_diag["first_chunk_at"], _diag["chunks"], _diag["bytes"]) still see every chunk.
  • tests/agent/test_stream_stale_ping_gate.py — new file. 8 unit tests:
    • keepalive ping (no choices) → does NOT reset
    • role-only / empty delta → does NOT reset
    • content / reasoning_content / reasoning / tool_calls / function_call → DOES reset
    • 200-ping stall simulation → stale_elapsed correctly grows past 90 s threshold

How to Test

  1. pytest tests/agent/test_stream_stale_ping_gate.py -q — 8 tests pass.
  2. pytest tests/ -q — passes (no regressions).
  3. Manual: under a real ChatGPT codex backend session, trigger a long-running turn (e.g. heavy tool use). Before this fix, HERMES_STREAM_STALE_TIMEOUT=90 would not fire if the backend was sending pings; after, the timer fires on the actual content stall and the retry loop reconnects.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(streaming):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and the suite passes
  • I've added tests for my changes (tests/agent/test_stream_stale_ping_gate.py — 8 tests)
  • I've tested on my platform: macOS 15.5 (Darwin 25.5.0), Python 3.11

Documentation & Housekeeping

  • Documentation — N/A (in-code comment explains the invariant)
  • cli-config.yaml.example — N/A (no new config keys; HERMES_STREAM_STALE_TIMEOUT already exists)
  • CONTRIBUTING.md / AGENTS.md — N/A
  • Cross-platform — pure Python, no platform-specific calls
  • Tool descriptions/schemas — N/A

Notes for reviewer

  • The bug surfaced in a downstream fork against the older run_agent.py chunk loop (commit 53ba42ad4). The fix is re-ported here onto the current chat_completion_helpers.py surface; the test file is placed next to the new location (not the legacy tests/run_agent/ path).
  • Three similar unconditional resets exist nearby (chat_completion_helpers.py:1725, :1974, :1997) on what look like Anthropic / different-provider paths. They're left untouched in this PR because the ping-stall pattern was only verified on chatgpt.com/backend-api/codex. Happy to widen the predicate if the maintainer wants symmetry; I'd suggest doing that as a follow-up so this PR stays scoped.
  • The predicate is duplicated between source and test (with a comment to keep them in lock-step). If you'd prefer a shared helper module, I can refactor — just call it out.

…alive pings)

The chat_completions streaming loop in `agent/chat_completion_helpers.py`
resets `last_chunk_time` on every chunk, but the ChatGPT codex backend
(`chatgpt.com/backend-api/codex`) keeps a stalled stream alive with SSE
keepalive pings while delivering zero content tokens. The unconditional
reset defeats both this detector AND the httpx read timeout — a hung
request stays alive until the backend gives up (~10 min) instead of
killing+reconnecting at `HERMES_STREAM_STALE_TIMEOUT` (the immediate retry
succeeds in seconds). A trivial "pong" request was measured at 615s before
the fix.

Gate the reset on chunks that carry real progress (content / reasoning /
tool-call / function-call deltas). Keepalive pings still drive the
diagnostic counters but no longer mask the stall.

Adds a regression test (`tests/agent/test_stream_stale_ping_gate.py`)
that mirrors the exact predicate shape so a future refactor must
preserve the invariant.

Originally developed against an older `run_agent.py` loop on a local
branch; re-ported here to the current upstream helper location.
@sh940701
sh940701 force-pushed the pr/streaming-stale-timer-gate branch from f0b6c19 to f29db6a Compare May 29, 2026 10:07
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels May 29, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying a real mismatch in the stale-stream path. Current main still resets last_chunk_time for every streamed chunk at agent/chat_completion_helpers.py:2318, while the polling loop says it must detect SSE-ping-only streams at agent/chat_completion_helpers.py:3053-3073.

Problems

  • tests/agent/test_stream_stale_ping_gate.py:31 reimplements the proposed predicate locally rather than exercising interruptible_streaming_api_call(). That test can pass even if the production timer assignment at agent/chat_completion_helpers.py:2318 is reverted or diverges.

Suggested changes

  • Put the predicate in a small private helper in agent/chat_completion_helpers.py and test that helper directly, or use a fake stream through the real streaming path and verify empty-choice chunks let the actual stale detector close the client.

This is an automated hermes-sweeper review.


import time


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the production predicate instead of testing it. Please extract the gate into a private helper in agent/chat_completion_helpers.py and import it here, or drive the real streaming loop; otherwise a regression in the source assignment can leave this suite green.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit area/streaming Streaming responses: gateway delivery, provider wire labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants