Repository navigation
test(providers): fail a hung stream test in one bound, not two - #659
Conversation
The two truncated-stream loopback tests waited on the server thread for a second `TRUNCATED_STREAM_BOUND` even after the client had already timed out, so a client that never connected took ~20 s to fail. Wait on the server only when the client returned; the client timeout is reported either way. In the property tests, the three `*_events` helpers and `events_for_whole_body` now share one `collect_events`, and `FirstCallInterrupted` is `InterruptingReader` with an unbounded period (the reader now interrupts its first read too, which the periodic property also exercises). Also say at the zero-chunk trailer read why its result is ignored: every event has been delivered by then, so failing would discard a complete answer. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed this with two independent passes (a blind fan-out correctness review plus a separate Standards/Spec pass). Both came back clean.
Traced the served_request_within_bound skip-on-timeout refactor and confirmed it's behavior-preserving on the client-returned path and correctly skips only the redundant second wait on the timeout path, matching the claimed speedup. The InterruptingReader/FirstCallInterrupted merge was independently verified (including an actual simulation) to reproduce the deleted type's behavior exactly, with no dangling references left anywhere. This is also a clean, complete closure of all four notes left on the predecessor PR #527, not unrelated scope bundling.
Only minor non-blocking notes: a slightly loosened panic-message distinction in a test helper, and a small duplicated 3-line shape between the two edited test functions - neither worth holding this up for.
One thing worth a look, not from this diff: "E2E tests (Strix Halo, WSL2)" is currently failing on an unexpected regression in comfyui-source-download-shows-live-progress (a download-progress timeout), which isn't on the known-flaky list and isn't the usual comfyui status-test flake. This PR only touches local-provider SSE parsing, nothing related to ComfyUI installs, so it looks unrelated - but flagging it in case it's a new flake worth tracking rather than assuming it's unrelated.
Approving - nothing in the diff itself blocks merge.
|
Thanks. Both minor notes (the looser panic-message distinction in the helper, and the three-line shape repeated in the two edited tests) will go into a follow-up PR. The WSL2 |
|
The promised follow-up is #685. |
2 participants
Follows up #527 (jussielo-amd's notes).
collect_events, andFirstCallInterruptedis folded intoInterruptingReader. Removing theInterruptedretry still failsclose_delimited_reader_retries_one_interrupted_readand the property.[DONE]included, has already been delivered, so failing would throw away a complete answer.summary:suffix stays out. fix(providers): end a local stream cut off before its final chunk #527 removed it on purpose (in85058139) to clear CodeQL cleartext-logging alerts.Verified locally:
rocm --bin rocm, with the known timing flakes passing on re-run, ande2e-cucumberlib, plusfeature_namingx86_64-pc-windows-gnuThis is test-only, so no scenario.