fix(voice): terminate wait=False test drain on end-of-turn, re-enable in CI#691
fix(voice): terminate wait=False test drain on end-of-turn, re-enable in CI#691minh2416294 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Changesagent(wait=False) CI hang fix
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/tests/voice/test_agent_wait_false.py`:
- Line 32: Add explicit type annotations to comply with the repository's typing
requirements. For the class attribute `self._spoke` on line 32, add a type
annotation indicating it is a boolean type. For the async function
`test_agent_wait_false_returns_before_turn_finishes` on line 67, add a return
type annotation to specify what the function returns (typically None for async
test functions). Use Python type hint syntax to annotate both symbols
appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5379e078-2cd1-4c7b-a4e7-fd568ec90731
📒 Files selected for processing (1)
python/tests/voice/test_agent_wait_false.py
| super().__init__() | ||
| self.recv_delay = recv_delay | ||
| self.recv_returned_at: float | None = None | ||
| self._spoke = False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check for missing annotations in the reviewed file.
python - <<'PY'
import ast
from pathlib import Path
p = Path("python/tests/voice/test_agent_wait_false.py")
tree = ast.parse(p.read_text(), filename=str(p))
missing_returns = []
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
if node.returns is None:
missing_returns.append((node.name, node.lineno))
print("Functions missing return annotations:")
for name, line in missing_returns:
print(f" - {name} (line {line})")
print("\nPotential unannotated self-attributes in __init__:")
for cls in [n for n in tree.body if isinstance(n, ast.ClassDef)]:
for fn in cls.body:
if isinstance(fn, ast.FunctionDef) and fn.name == "__init__":
for stmt in fn.body:
if isinstance(stmt, ast.Assign):
for target in stmt.targets:
if (
isinstance(target, ast.Attribute)
and isinstance(target.value, ast.Name)
and target.value.id == "self"
):
print(f" - {cls.name}.{target.attr} (line {stmt.lineno})")
PYRepository: langwatch/scenario
Length of output: 548
Add explicit type annotations for newly touched symbols.
The class attribute self._spoke (line 32) and async function test_agent_wait_false_returns_before_turn_finishes (line 67) lack type annotations, violating the repository's strict typing requirements.
Suggested patch
class _SlowAdapter(VoiceAgentAdapter):
@@
- self._spoke = False
+ self._spoke: bool = False
@@
-async def test_agent_wait_false_returns_before_turn_finishes():
+async def test_agent_wait_false_returns_before_turn_finishes() -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/tests/voice/test_agent_wait_false.py` at line 32, Add explicit type
annotations to comply with the repository's typing requirements. For the class
attribute `self._spoke` on line 32, add a type annotation indicating it is a
boolean type. For the async function
`test_agent_wait_false_returns_before_turn_finishes` on line 67, add a return
type annotation to specify what the function returns (typically None for async
test functions). Use Python type hint syntax to annotate both symbols
appropriately.
Source: Coding guidelines
… in CI _SlowAdapter.recv_audio returned audio on every call, so the base _drain_agent_response loop never tail-silenced and ran to response_max_duration (~90s wall-clock) — tripping CI's 60s timeout while squeaking under it locally. The fixture now emits one chunk then an end-of-turn empty chunk, the contract every real adapter honors, so the turn terminates in ~1s. Removes the skipif(CI) workaround and adds a timeout(30) regression guard. Relates to langwatch#648.
minh2416294
force-pushed
the
flake/diagnose
branch
from
90bd4ff to
5cadd05
Compare
|
Thanks! Applied I skipped the |
langwatch-agent
added
the
hound-checked
|
🐕 PR Hound review brief — assigned reviewer @drewdrewthis Review mode: Fast Skim Inspection targets (max 3, each names a file or behavior):
Author questions (optional, max 3):
|
|
Thanks for the brief — went back through the three points against the code. 1. Is Still meaningful. The tail-silence change only affects when the drain loop exits, not when the first recv stamps. 2. Does Yes. The empty chunk only returns on the second 3. Does empty-chunk-on-every-subsequent-call faithfully model the end-of-turn contract, or could it mask an #648 regression? It's faithful, and it can't mask #648. The base while accumulated < self.response_max_duration:
try:
nxt = await self.recv_audio(timeout=self.response_tail_silence)
except asyncio.TimeoutError:
break
if not nxt.data:
break # <- exits here on the first data=b""
...So whether the mock returns empty once or on every later call is immaterial to the drain — it exits on the first On #648: that's the product-side path — a real adapter whose recv never yields tail silence, where the drain has no wall-clock guard beyond One note on the CodeRabbit docstring-coverage warning (16.67% < 80%): I left it as-is. The flagged symbols are test functions and the |
|
@drewdrewthis thank you brother |
|
This one is approved by @drewdrewthis, CI is green, and it shows as mergeable. It just needs a merge. @rogeriochaves would you be able to take a look? Happy to rebase if that helps. Thank you. |
CI regression diagnosisRoot cause: main-moved breakage, not a problem with this PR's code. The 3 failing checks are caused by an infra gap in What happened
The 3 failing checks
Fix needed in
|
langwatch-agent
left a comment
langwatch-agent
left a comment
There was a problem hiding this comment.
No code/security findings from static external-PR review. The fixture change matches the documented end-of-turn contract and narrows the test-only CI skip. I did not execute fork code. Residual blockers: current CI shows failing Validate PR Title and test (3.12) / python-complete, so those need to be fixed or explained before merge.
4 participants
Why
The
agent(wait=False)voice tests intests/voice/test_agent_wait_false.pywere skipped in CI via@pytest.mark.skipif(CI=="true")with the note "hangs in python-ci, can't reproduce locally." The hang isn't infinite — it's a ~90s turn. The test's_SlowAdapter.recv_audioreturns a non-empty audio chunk on every call, so the base_drain_agent_responseloop (scenario/voice/adapter.py) never tail-silences and runs untilaccumulated_audio >= response_max_duration(30s of audio ≈ 90s of wall-clock at this cadence). That trips CI's 60s timeout reliably under load while squeaking under it on faster local machines — a textbook timing flake. This restores the lost CI coverage of thewait=Falseconcurrency primitive.Relates to #648 (same audio-gated-drain class; this PR fixes the test fixture side, not the product-side adapter terminal path).
What changed
_SlowAdapternow emits one real chunk, then a terminal emptyAudioChunk(data=b"")— the same end-of-turn signal real adapters send (OpenAI Realtime, Gemini Live, Composable) so the base drain loop exits on tail silence instead of running toresponse_max_duration. Chose fixing the unrealistic mock over adding a wall-clock cap to the base loop, because fix(voice): surface tool-only realtime turns (no audio chunk) #647 establishes that the drain terminates on an audio-terminal event, deliberately keepingresponse_timeoutas the only time-guard.skipif(CI)workaround (and the now-unusedimport os). With the fixture fixed, the tests run in CI again — closing the coverage gap the skip created.@pytest.mark.timeout(30)totest_agent_wait_false_returns_before_turn_finishesso any regression of the ~90s drain fails fast and loud instead of wedging the runner.Test plan
scenario.run()with thewait=Falsescript ran ~90s (measured:RETURNED after 90.3s success=True) and exceeded the 60s pytest-timeout.CI=trueset (the condition that previously skipped it)._stampfires beforeslow.recv_returned_at(thewait=Falsestep returns control before the slow recv completes). The siblingtest_double_wait_false_in_flight_raisesrace is preserved (the fixture change only affects calls after the first chunk).How I can prove I was successful
No playable artifact — see Test plan. This is a test-fixture/control-flow change; the demonstration is the suite going from skipped-in-CI (~90s drain on the parent commit when the skip is removed) to
3 passed in ~1.2son this branch withCI=true.