Repository navigation
fix(voice): preserve tool outputs across concurrent realtime context updates - #7905
Conversation
Signed-off-by: Alpinist <199138677+Epsilon006@users.noreply.github.com>
|
Heads up for whoever reviews: #7904 is the same fix, opened about a minute before this one. Same lock, same three snapshot sites, so one of the two should just be closed — no strong feelings about which. One thing from comparing them, in case it is useful. This PR also locks the two pushes that hand over a context the activity already owns ( async def update_chat_ctx(self, chat_ctx):
async with self._update_chat_ctx_lock:
...
events = self._create_update_chat_ctx_events(chat_ctx)A push that overlaps another one therefore does not diff until the earlier push's ack has landed and the mirror has caught up. The staleness that bites is only in a snapshot of the mirror taken outside that lock — which is the three sites we both fixed. For a locally built context there is nothing to go stale against, so I could not construct a case where locking those two changes the outcome. If you have one, I would like to see it — it would mean the invariant is wider than I think. |
|
Correcting myself: a bot review on #7904 produced the case I said I could not construct, and it is real. The text turn adds its user message to the agent context only after its push returns ( Where I think my conclusion still holds: serializing those two sites does not fix it. The replacement's context object is captured before it blocks on the lock, and no ordering of the pushes refreshes it — after the lock it still diffs a copy with no user message against a mirror that has one. Worth checking me on that. What does fix it is ordering: both tool-output paths already insert locally before pushing ( |
|
Thanks for following up. I agree that locking cannot refresh a replacement context that was already captured. I checked both PR heads and found a different ordering where the additional locks do help: replacement or cleanup first, then an append while its ACK is pending. Two reproducible cases:
Both reproduced through a full AgentSession, using the real OpenAI engine’s diff, provider lock, and ACK handlers with scripted transport. The cleanup test uses real tool execution and stubbed skipped playout. Explicit context replacement can still delete items as intended. I couldn’t reproduce the original text-first/overlapping-replacement deletion in my tested schedule: the completing text task updates the local context object shared with the replacement before the provider copies it. That doesn’t rule out another schedule, but I wouldn’t use that example as evidence for these locks. So I think these are separate questions. Your follow-up concerns when text becomes visible locally; the additional locks here prevent later appends from snapshotting an outdated mirror during replacement or cleanup. The differential suite passed 22 cases across 20 runs, and the realtime regression group passed 197 tests. |
|
You're right, and this is better than mine. Thanks for doing the work to show it. The ordering I missed is the reverse of the one I reasoned about. I only considered append-then-replace: the replacement captures its context, blocks on the lock behind the append, and still diffs a stale object afterwards — which the lock genuinely cannot fix. Your case is replace-then-append: the replacement goes first and holds the provider lock, and the later append snapshots a mirror that does not yet contain the replacement's item. There the activity lock is exactly what is needed, because it stops the snapshot being taken at all until the earlier push has been acked. Same for cleanup versus a late tool output recreating a deleted item. So locking all five sites is the correct invariant and three was not enough. I'm closing #7904 in favour of this one — no point making a reviewer read two versions of the same change, and yours has the coverage and the repros. Thanks also for being straight about not reproducing my original example. That makes sense now: the completing text task upserts into the same local object the replacement holds, so the message is there by the time the provider copies it. The case that does bite is when the text turn is still waiting — which is #7906, and as you say, a separate question. One thing from mine you may or may not want: I had folded the snapshot and the push into a single |
|
Thanks for digging into this with me! Your questions helped me look more closely at the ordering, and I’m glad we got it figured out. The helper idea makes sense too. I’ll keep this PR focused for now, but happy to revisit that if the maintainers prefer. |
Fixes #7880.
A text turn can copy the realtime mirror while a tool-output push is still awaiting acknowledgement. When its stale snapshot is eventually pushed, the full-context diff deletes the newly acknowledged function_call_output. The local Agent context still contains it.
Serialize AgentActivity's remote-context snapshots and pushes under one private lock, held through acknowledgement. Apply it to text turns, normal and interrupted tool-output pushes, explicit context replacements, and interrupted-message cleanup. Explicit full-context replacement keeps its deletion behavior. No provider/public API or version changes.
Validation
ceb3d598because the remote output is deleted; both pass with this fix. A third test confirms that explicit context replacement can still delete tool outputs.python -m pytest tests/test_realtime*.py tests/test_openai_realtime_chat_ctx.py --unit -q(PowerShell expands the file list explicitly).ruff check .andruff format --check .pass with the locked Ruff 0.15.22.livekit.agents.voice.agent_activitypasses with locked mypy 2.3.0 and--platform linux --follow-imports=silent. This is not the all-plugin type-check gate.git diff --checkpasses; commit is DCO signed off.The complete unit/audio-EOT collection was attempted but stopped on 37 import errors from optional provider/test dependencies absent in this focused environment. No live OpenAI calls or websocket-server reproduction is claimed. I asked the reporter to share their scripted-server test for cross-checking; hosted CI has passed the unit-tests, Ruff, Python 3.10/3.13 type checks, and Ubuntu/macOS/Windows blockguard checks. The reporter's additional scripted-server validation remains pending.