Repository navigation
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eaada605e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Four tests in TestSendEventAndConfig seed audio and interrupt on the next
statement, so they assume the clock advanced in between. time.monotonic()
on Windows is GetTickCount64() with a 15.625ms resolution, so both reads
land in the same tick, elapsed_ms is exactly 0.0, and the truncate path
under `if elapsed_ms > 0` never runs:
test_normal_interrupt_targets_response_and_interrupts_audio
test_interrupt_respects_auto_cancellation_when_not_forced
test_interrupt_force_cancel_overrides_auto_cancellation
test_interrupt_honors_falsy_present_session_auto_cancellation
They pass in CI, so whether they run depends on the ambient system timer
resolution rather than on the code under test.
Backdate initial_received_time, which is the idiom the same file already
uses in test_speech_started_skips_truncate_when_audio_complete. No
production code changes: the tests become deterministic on any host, and
the separate question of whether the `> 0` guard should treat a
sub-tick elapsed time as "no audio played" is left to openai#4456.
2eaada6 to
be4e125
Compare
seratch
left a comment
There was a problem hiding this comment.
Thanks for taking the time to investigate this and for clearly isolating the Windows-specific test behavior. I agree that these tests should not depend on the host's ambient clock resolution.
After tracing the runtime path, I believe the test adjustment and the underlying runtime behavior should be addressed together in this PR. The elapsed_ms == 0 case from #4456 is a valid state that can occur when an interrupt arrives within the same clock tick. Backdating the audio start by five seconds avoids that case, but it also places each 100 ms audio fixture beyond its buffered duration. As a result, the ongoing-response and clamping path can make the tests pass without exercising active playback.
Could you please revise the PR to treat elapsed_ms == 0 as a valid truncation position in _interrupt_audio_playback, add a deterministic regression test with equal monotonic readings that asserts both audio_interrupted and conversation.item.truncate with audio_end_ms=0, and remove the four five-second backdates? Please preserve the existing behavior for negative elapsed values, missing audio, completed audio, and truncation clamping.
Thank you again for the careful report and reproduction. With those focused changes, this should cover both the Windows test failure and the user-visible runtime issue.
Per review on openai#4457: address the runtime behaviour rather than working around it in the tests. _interrupt_audio_playback gated truncation on `elapsed_ms > 0`, so an interrupt arriving inside one clock tick was treated as "nothing to truncate". That is a reachable state, not a contrived one: time.monotonic() advances in ~15.6ms steps on Windows, so an interrupt in the same tick as the audio it interrupts produces exactly 0. The item then kept audio the user never heard, which is the condition truncation exists to prevent. The guard becomes `>= 0`. The branch's own log line already read "Didn't interrupt bc elapsed ms is < 0", so the message described the intended condition while the code did something narrower; it is now accurate. truncated_ms already clamped through max(int(elapsed_ms), 0), so a zero elapsed time needed no other handling. Behaviour preserved deliberately: a negative elapsed time still skips, a missing item id or elapsed value still skips, and the completed-audio and max_audio_ms clamping paths are untouched. Tests: the four five-second backdates are removed. They avoided the zero case rather than covering it, and — as the review noted — pushed each 100ms fixture past its buffered duration, so the ongoing-response and clamping path could satisfy them without active playback ever running. Two deterministic tests replace them, pinning both monotonic readings to the same value: one asserts audio_interrupted and a conversation.item.truncate with audio_end_ms=0, the other asserts a backwards clock still truncates nothing.
|
Thanks — you were right, and the reason is worse than "the backdate is unnecessary". Revised in 9e9c2bb. Your point about the fixture is the one that matters: each fixture buffers 4,800 bytes of PCM16, so a five-second backdate put playback past its 100ms duration. Combined with ChangesRuntime — One detail that supports the reading: the Preserved, as asked: negative elapsed still skips; a missing item id or elapsed value still skips; the completed-audio and Tests — the four backdates are gone. Two deterministic tests replace them, both pinning the recording and reading clocks to the same value:
VerificationThe four originally-failing tests now pass with no backdate at all, because the runtime fix makes them work for the right reason: Full suite against baseline, diffing the One thing to flag so it is not mistaken for mine: the after-run shows #4456 can close with this, since it now covers the runtime half rather than only the test half. |
On Windows,
time.monotonic()can return the same value for consecutive reads. When an interrupt arrived within the same clock tick as the first audio delta,_interrupt_audio_playbackcalculatedelapsed_ms == 0and skipped bothaudio_interruptedandconversation.item.truncate.This change treats zero elapsed time as a valid truncation position. The model now emits
audio_interruptedand sendsconversation.item.truncatewithaudio_end_ms=0, keeping the server-side conversation aligned with the audio the user actually heard.The existing behavior is preserved for negative elapsed values, missing playback state, completed audio, and truncation clamping.
Tests
audio_interruptedandconversation.item.truncatewithaudio_end_ms=0.TestSendEventAndConfigsuite and the complete Realtime test suite.Fixes #4456