Repository navigation
voice-agents-python: the .NET runtime holds frames during a resume, fails lost tool calls, reports a drained connection's leftovers - #5
Conversation
…ool calls, reports a drained connection's leftovers
|
Superseded by the render on voice-agents/render-44694f3, which carries this change and the lane's later ones. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Besides the inline findings, I checked the new log line in this diff for secrets (REVIEW.md item 5). It is the handshake/idle timeout warning in agent.py, and it logs only the awaited frame name and the timeout. It does not log a resume token or an API key.
Extended reasoning...
The change touches the connection reader, reconnect backoff, handler scheduling and session resume/loss handling in agent.py and session.py, plus the generated frames.py. The only added log line prints a frame name and a timeout value, so no credentials appear in it. The hunt stopped at its bug cap, not because it ran dry, and confirmed findings are posted inline, so this is not an approval.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| async def _send(self, frame: frames.Frame) -> None: | ||
| """Sends a frame for the session on its connection, keeping it if it counts. A frame that could not be | ||
| sent still counts: Sent's `received` on the next resume says whether to send it again.""" | ||
| sent still counts: Sent's `received` on the next resume says whether to send it again. A frame that | ||
| breaks the protocol is refused before it counts. While a resume is unanswered the frame is kept and not | ||
| sent: the replay after session.resumed sends it, once, on the connection the session is on.""" | ||
| if self.ended: | ||
| raise SessionEnded(self.id) | ||
| frames.encode(frame) | ||
| self._keep(frame) | ||
| if self._resuming: | ||
| return | ||
| await self._connection.send(frame) |
There was a problem hiding this comment.
🔴 Python SDK customers will see a changelog line about "the .NET runtime" that describes a feature this change does not ship. The only commit, be2632b, has the subject "fix(sdk): the .NET runtime holds frames during a resume, fails lost tool calls, reports a drained connection's leftovers". release-please copies it verbatim into CHANGELOG.md. No code here reports a drained connection's leftovers: _drain and _closed are unchanged. Fix: reword the subject, and the matching PR title, to describe this Python change. For example: "fix: hold frames while a resume is unanswered, fail tool calls of a lost session, time out a missing connection.ready". [also at: sentdm_voice_agents/agent.py:335 - Python customers get a changelog line about "the .NET runtime" and about reporting a drained connection's leftovers, which this change does not do.]
Why this was flagged
The only commit in the range, be2632b, has the subject "fix(sdk): the .NET runtime holds frames during a resume, fails lost tool calls, reports a drained connection's leftovers". CLAUDE.md says the subject "becomes the changelog line customers read", and REVIEW.md item 7 asks for a fix: subject a customer can read. This repository is the Python SDK, so after the release-please release Python users would read a line about another language's runtime. The line also names "reports a drained connection's leftovers", but Agent._drain (agent.py:283) and Agent._closed (agent.py:355) are untouched by this diff, so the changelog promises behaviour that does not ship. The changes that do ship (frame holding in Session._send at session.py:135-146, _abandon, HANDSHAKE_TIMEOUT_S before connection.ready, handlers moved off the reader) are partly missing from the line. Nothing in CI catches a misworded conventional-commit subject.
Verification: Trigger: the PR is merged and release-please cuts the next release. In release-please-config.json, "fix" maps to the visible "Bug Fixes" section and is not hidden, so this subject goes into CHANGELOG.md as written. This repository is the Python SDK, so the line describes another language's runtime. The diff never touches _drain (agent.py:283) or _closed (agent.py:355).
| session = connection.sessions.get(frame.session_id or "") | ||
| if session is None or not session._resuming: | ||
| return | ||
| if frame.frame_type != "session.resume" and frame.error.code not in ("resume_failed", "connection_full"): |
There was a problem hiding this comment.
🔴 If Sent refuses a resume with no frame_type and a code other than resume_failed or connection_full, the session hangs instead of being reported lost. The new filter at agent.py:335 skips errors unless frame_type == "session.resume" or the code is one of those two. frame_type is optional ("when it had one"). Fix: while a session is _resuming on this connection, treat every session-scoped error as answering the resume, with an unknown code taking the fallback (lost). Nothing else is sent for the session during a resume now.
Why this was flagged
The condition is Sent sending an error for a pending session.resume without frame_type, or with a code this SDK does not list, such as a future code. ErrorFrame.frame_type is str | None at frames.py:104. Agent._error at agent.py:335 then returns early and leaves session._resuming True. The connection stays open, so _closed never reports the session lost. send_audio silently drops audio (session.py:98), _send holds frames without sending them (session.py:144), and tool-call futures never resolve. On the base branch, any error for a resuming session made it lost and fired on_session_lost. Session._send no longer sends during a resume, so the session.resume is the only frame an error on that connection can be answering. REVIEW.md item 6 asks for unknown error codes to be treated as the fallback.
Verification: The new guard at agent.py:335 is if frame.frame_type != "session.resume" and frame.error.code not in ("resume_failed", "connection_full"): return. It returns before session._resuming = False (line 337). On the base, every session-scoped error that arrived while _resuming was set either ended the resume (lost) or moved it (connection_full).
| elif isinstance(frame, frames.Ping): | ||
| await connection.send(frames.Pong(ts=frame.ts)) | ||
| # A connection that lived through a heartbeat interval is a good one: the next loss starts the | ||
| # backoff over. A connection that dies before its first ping keeps it growing. | ||
| if connection.ready: | ||
| self._attempt = 0 |
There was a problem hiding this comment.
🔴 When a second connection keeps failing, its reconnect backoff never grows past a few seconds. A healthy connection's pings keep resetting the shared counter. At agent.py:250 a ping on any ready connection sets the agent-wide _attempt to 0, even when a different connection is the one failing. Fix: track the attempt count per reconnect chain, for example on _Connection passed through _open. Reset it only when that same connection gets its first ping after ready, and have _closed take the delay from the closing connection's count. On the base branch the counter reset only on connection.ready, so a sibling's failures still grew to MAX_BACKOFF_S.
Why this was flagged
An agent can hold two connections at once. One case is a drain: _drain at agent.py:283-287 opens B while the draining A stays up. The other is an overflow: _error at agent.py:339-341 opens B for a connection_full session. If B's handshake keeps failing (a 503 from Sent at capacity, or during a deploy), each failure goes _serve finally → _closed(B) → _backoff() (agent.py:375-381), which increments the shared self._attempt. Meanwhile A, still ready, receives a ping every heartbeat_s (5000-15000 ms in the fixtures). _handle (agent.py:249-250) then sets self._attempt = 0. B therefore retries at 0, 1, 2, 4 s and then starts again from 0, never reaching MAX_BACKOFF_S = 30.0, for as long as A lives. On the base branch only _ready reset the counter, and A's ready came once, so B's retries backed off to 30 s. The new comment says the reset applies to "a connection that lived through a heartbeat interval", but the counter is not per connection.
Verification: _attempt is one counter on the Agent (agent.py:95). The new lines at agent.py:247-250 reset it on a ping from ANY ready connection, not only from the connection whose chain is failing. On the base, _attempt = 0 ran only in _ready. The new test (tests/test_agent.py:52-59) uses only one connection and does not cover this.
| if self.ended: | ||
| raise SessionEnded(self.id) |
There was a problem hiding this comment.
🟡 (optional) Audio agents, including the bundled conformance agent, now log an ERROR traceback ("a handler failed") for each audio chunk still queued when a call ends or is lost. The reader sets end, or lost via _abandon, as soon as session.end or the loss arrives. The session task then keeps delivering already-queued audio to on_audio, and send_audio now raises SessionEnded. Fix: stop handing queued audio to handlers once the session has ended, by skipping bytes items in _run_session when session.ended. Audio is uncounted, so nothing is lost. Also catch SessionEnded in the conformance agent's on_audio.
Why this was flagged
In agent.py:302-307, _session_frame puts session.end on the inbox and then calls session._finish, which sets end synchronously. Audio bytes queued earlier by _audio (agent.py:316) are still waiting in _inbox. _lost (agent.py:350-353) does the same thing through _abandon, which sets lost. _run_session (agent.py:393-394) then runs on_audio for each queued chunk. ConformanceAgent.on_audio (conformance/main.py:62-63) calls session.send_audio with no try/except, so the new if self.ended: raise SessionEnded check (session.py:94-95) raises. _safely then emits log.exception("a handler failed") once per chunk. On the base branch, send_audio after the end just wrote bytes to the still-open socket or silently swallowed ConnectionClosed, so nothing was logged. Customer agents that use the same echo/STT pattern in on_audio get the same tracebacks, and the slower their on_audio is, the more chunks are queued.
Verification: _run_session still hands each bytes item to self._safely(self.on_audio(...)) and never checks session.ended. send_audio now starts with if self.ended: raise SessionEnded(self.id) (session.py:94-95). On the base this did not happen: send_audio had no ended check, and _Connection.send_bytes (agent.py:74-80) swallows ConnectionClosed, so leftover chunks were sent or dropped without a sound.
| session._resuming = False | ||
| connection.sessions.pop(session.id, None) | ||
| if frame.error.code == "connection_full": |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Pre-existing, partly fixed here: after a connection_full refusal, the agent's audio and frames still go to the full connection. The audio uses a stream number that may be another caller's. _error clears _resuming and detaches the session, but session._connection stays the full connection until the reopened one is ready. send_audio and _send check only _resuming, so they send there. Fix: keep the session marked as detached (for example _resuming or a new flag) from the refusal until session.resumed, and in _closed for movable sessions waiting out the backoff, so every send path holds or drops.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Sent answers session.resume with error code connection_full. Agent._error (agent.py:337-341) sets session._resuming = False, pops the session from connection.sessions, and calls _open([session], 0.0). session._connection still points at the full connection. Until the new connection's _ready calls _resume, the session's handler can call Session.send_audio. That passes the _resuming guard at session.py:98 and sends audio_frame(self.stream, ...) on the full connection. That connection carries max_sessions other calls, so the old stream number can belong to another caller. The diff's own comment names this hazard. Text frames from _send (session.py:144) also go to a connection that does not carry the session. The base branch sent during every resume. This diff closes only the _resuming window and leaves this one. Movable sessions in _closed have the same gap during the backoff delay.
Verification: In agent.py:337-342, _error sets session._resuming = False and reopens, but never changes session._connection. So send_audio passes if self._resuming: at session.py:98 and sends on the full connection. The base send_audio had no _resuming guard at all, so the base fails the same way on this path. That makes this pre-existing, not a regression.
Rendered by the Voice Agents lane in sent-dm-sdk-pipeline from sent-dm-voice-protocol at
v0.2.0, after https://github.com/sentdm/sent-dm-sdk-pipeline/commit/92693698ce229cf190024d6a19e6d22b1af85128.What a reviewer checks: this repository's CI, the conformance job included, is green on this PR; the generated frames module and the runtime changed as the protocol or the template did; nothing release-please owns moved. Merging makes release-please open the release.