Repository navigation
voice-agents-python: the Python runtime holds frames during a resume, fails lost tool calls, counts unknown session frames - #6
Conversation
… tool calls, counts unknown session frames
sent-dm-sdk-pipeline
Bot
changed the title
0d691de to
85a58e9
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| for session in pending: | ||
| await self._resume(connection, session) | ||
| await self._safely(self.on_connected(frame)) | ||
| self._spawn(self.on_connected(frame)) |
There was a problem hiding this comment.
🟡 (optional) An agent that sets up state in on_connected can now have on_session_start run before that setup finishes. On the base branch, setup always finished first. _ready now spawns on_connected on a background task. The reader then dispatches the session.start that Sent sends right after connection.ready. That session's task starts while on_connected is still awaiting. This follows from the stated goal of keeping handlers off the reader. Fix: keep the reader unblocked but state the new ordering in the on_connected docstring and README handler table. Alternatively, make session tasks started on that connection await the spawned on_connected task before on_session_start.
Why this was flagged
On the base branch, _ready awaited self._safely(self.on_connected(frame)) on the reader. No later frame on that connection was read until on_connected returned, so no session.start could create a session task before then. Now agent.py:273 calls self._spawn(self.on_connected(frame)), and _read (agent.py:240) goes straight to the next frame. If that frame is session.start, _start (agent.py:289-293) creates the session task immediately. Its on_session_start runs as soon as on_connected yields at any await, such as a config fetch or opening a client. A handler that reads state on_connected was still preparing now sees missing or stale state, for example an AttributeError logged by _safely and a session with no greeting. Nothing orders the two tasks: _spawn returns no handle the session path waits on. The on_connected docstring ("A connection to Sent is ready for sessions.") and README still suggest it runs before sessions on that connection.
Verification: On the base, _ready ended with await self._safely(self.on_connected(frame)). Now agent.py:273 calls self._spawn(self.on_connected(frame)), and _read goes straight on to the next socket.recv(). If that frame is session.start, _start (agent.py:289-293) creates _run_session at once. on_session_start then sees whatever on_connected has not set up yet.
| 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.
🟡 nit (optional): release-please will publish a changelog line that does not describe this change. The commit subject is "fix(sdk): a hand render can name its subject and its language". The diff actually changes the handshake timeout, the reconnect backoff, resume buffering, audio during a resume, and lost-session behaviour. Fix: reword the fix: subject so a customer can read what changed (e.g. resume and connection-loss handling), and split unrelated changes per "One logical change per PR".
Why this was flagged
CLAUDE.md lines 27-28 say the Conventional Commit subject becomes the changelog line customers read. REVIEW.md item 7 asks for a fix: subject a customer can read. Commit 0d691de's subject names a hand render and a subject/language, but no file in the diff touches rendering, subject or language. The diff touches agent.py (timeout, backoff at agent.py:245-250, background handlers), session.py (resume gating, lost flag) and frames.py. Once merged, release-please writes the misleading line into CHANGELOG.md and the release notes.
Verification: nit: the misleading changelog line appears once the PR merges and release-please cuts the release. The PR range has one commit, 0d691de fix(sdk): a hand render can name its subject and its language. Nothing in the diff deals with rendering, a subject or a language. The PR title has no prefix, so release-please takes only this subject, against CLAUDE.md:27-28.
| if frame.frame_type != "session.resume" and frame.error.code not in ("resume_failed", "connection_full"): | ||
| return | ||
| 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: after a connection_full refusal, an audio session's PCM can still reach another caller on the full connection. _error clears _resuming but leaves session._connection on the full connection. So send_audio sends with the old connection's stream number, which may belong to another call there. Fix: keep the session gated until _resume reattaches it on the new connection, for both send_audio and _send (keep _resuming set, or add a detached flag both check). The new session.py:98 guard only covers an unanswered resume.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
_resume (agent.py:275-281) sets session._connection = connection and _resuming = True, then sends session.resume. Sent answers with error code connection_full because that connection already carries max_sessions. _error (agent.py:335-340) sets session._resuming = False, pops the session and calls _open([session], 0.0), but leaves session._connection on the full, still-open connection. Until the new connection's ready, the session task's send_audio passes the new _resuming check at session.py:98. It then calls send_bytes(audio_frame(self.stream, ...)) on the full connection with the stream number assigned by the old connection. The diff's own comment at session.py:99-100 notes that number may be another caller's there. Base sent this audio for the whole resume window. The diff closes the unanswered-resume part, but this window still behaves exactly as base.
Verification: pre-existing (security-relevant). Trigger: Sent refuses a session.resume with connection_full. _error (agent.py:337-342) runs session._resuming = False. It never detaches session._connection. An audio frame carries only that stream number, so Sent routes the PCM to whichever session on the full connection owns it, which can be another caller. The base send_audio has no gate at all.
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/44694f3a0577f5a9f88164609f62ea379a9fdf06.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.