fix(stream-events): enforce timeout_secs after a silent backend - #1030
Conversation
timeout_secs was only checked on incoming SSE chunks and EOS. Cap the upstream read timeout and pair examples with cluster read_timeout_ms so an idle connection after the first event is torn down. Closes praxis-proxy#938 Signed-off-by: mkoushni <mkoushni@redhat.com>
praxis-bot
left a comment
There was a problem hiding this comment.
Review: fix(stream-events): enforce timeout_secs after a silent backend
Well-structured change. The approach of capping the upstream read_timeout at timeout_secs and then recognizing transport-level idle timeouts as stream timeouts is sound. The validate_stream_end refactoring into StreamEndKind is cleaner than the original.
One correctness concern below.
| Severity | Count |
|---|---|
| Medium | 1 |
…-idle-timeout Signed-off-by: mkoushni <mkoushni@redhat.com> # Conflicts: # apis/src/openai/responses/stream_events/mod.rs
An idle or deadline abort after a terminal SSE event must not set skip_persist; the parser already observed a completed lifecycle. Signed-off-by: mkoushni <mkoushni@redhat.com>
…helper The example now ships cluster read_timeout_ms, so the vLLM harness must not inject a second copy. Split the chunked backend writer to satisfy clippy's line limit. Signed-off-by: mkoushni <mkoushni@redhat.com>
leseb
left a comment
There was a problem hiding this comment.
blocked by #1013 since this uses logical_stream and 1031 removes it
in the meantime:
P2 — Lines 330–333 ignore the actual filter-induced timeout classification. Praxis 0.5.4 reports a winning peer read_timeout as StreamTerminationCause::Io, not IdleTimeout; logical error bytes are then discarded because IRR still considers the termination unhandled.
P2 — Lines 311–316 implement a per-read timeout, not the documented duration “from first chunk to completion.” It can expire before the first event, while a stream that stalls near the deadline can survive almost twice the configured duration.
Only publish stream_error_code and skip_persist when the parser is still Open, so a slow HTTP close after response.completed does not suppress persistence. mark_stream_termination_handled stays unconditional. Signed-off-by: mkoushni <mkoushni@redhat.com>
Keep IRR-only logical stream composition from main and retain the idle timeout cap plus skip-after-terminal-event behavior. Signed-off-by: mkoushni <mkoushni@redhat.com>
The idle-backend example test injected timeout_secs at pre-IRR indent and failed YAML parse. Treat peer read_timeout as Io so IRR does not discard the stream, and isolate OpenResponses streaming-response from parallel CPU load. Signed-off-by: mkoushni <mkoushni@redhat.com>
Bring in streaming previous_response_id restore and bounded MCP tools/list deserialization on top of the idle-timeout cap. Signed-off-by: mkoushni <mkoushni@redhat.com>
Bring in MCP server URL redaction in debug logs. Signed-off-by: mkoushni <mkoushni@redhat.com>
late_upstream_failure_does_not_replace_committed_sse forbade any "error" substring. After Io terminations are marked handled, the logical stream appends an SSE error terminator instead of replacing the 200 event-stream response. Signed-off-by: mkoushni <mkoushni@redhat.com>
leseb
left a comment
There was a problem hiding this comment.
P1: timeout_secs is never applied in the documented IRR ordering because both cap attempts occur before load balancing selects ctx.upstream. The integration test passes only because it separately sets read_timeout_ms.
P2: matching every StreamTerminationCause::Io mislabels ordinary transport failures as timeouts. Coverage fails deterministically: 622 passed, 1 failed.
P2: read_timeout remains per-read, not an absolute deadline from the first SSE chunk; it may fire before that chunk or permit nearly 2T.
IRR runs the buffered request-body phase before load_balancer, so capping ctx.upstream earlier was a no-op. Place the documented filter after load_balancer, cap the remaining budget on the selected peer, and treat Io as a timeout only after the first SSE chunk exhausts timeout_secs. Signed-off-by: mkoushni <mkoushni@redhat.com>
|
P1 — cap after load balancing. openai_responses_proxy buffers the request body, so IRR runs that phase before load_balancer sets ctx.upstream. Both earlier cap attempts were no-ops; the idle test only passed because it also set read_timeout_ms: 1000. The documented stream-events.yaml (and irr-terminal-streaming.yaml) now place openai_stream_events after load_balancer, and on_request caps the selected peer. The idle test no longer tightens cluster read_timeout_ms. Agentic configs keep the filter before openai_agentic_loop so the loop still sees raw SSE on the reverse response path. Idle there still pairs with cluster read_timeout_ms. P2 — Io mislabel. Praxis reports a winning peer read_timeout as Io, but so do truncated chunks. Io is a stream timeout only after a first SSE chunk and timeout_secs has elapsed. late_upstream_failure_does_not_replace_committed_sse now asserts the body does not contain exceeded timeout. P2 — remaining budget. The parser deadline still starts at the first SSE chunk. After that chunk the peer cap is the leftover time, not another full period. Praxis snapshots read_timeout at dispatch, so later remaining-budget caps apply only while ctx.upstream is still visible. |
Removing the unused request-body hook made the generated Responses pipeline README stale, which failed lint. Signed-off-by: mkoushni <mkoushni@redhat.com>
cargo audit fails on RUSTSEC-2026-0285; 0.23.45 rejects TLS 1.3 handshake messages sent at the wrong encryption level. Signed-off-by: mkoushni <mkoushni@redhat.com>
|
@mkoushni final After each chunk, the filter tries to shorten ctx.upstream.read_timeout, but IRR response-body contexts have upstream: None; Praxis already copied the original timeout into the live body. A chunk near deadline therefore restarts the full per-read timer, allowing nearly 2T instead of an absolute T. |
…-idle-timeout Signed-off-by: mkoushni <mkoushni@redhat.com> # Conflicts: # examples/configs/openai/responses/stream-events.yaml # tests/integration/tests/suite/examples/openai_stream_events.rs
Keep the idle-timeout branch current with main before landing the live-body recap and first-chunk deadline fixes. Signed-off-by: mkoushni <mkoushni@redhat.com>
Do not cap upstream read_timeout at arm time; timeout_secs is an absolute deadline from the first SSE chunk only. Each chunk publishes leftover budget through HttpFilterContext::cap_stream_read_timeout so Praxis can copy it onto the live SubResponseBody instead of restarting the dispatch snapshot (requires praxis-proxy/praxis#1172). Signed-off-by: mkoushni <mkoushni@redhat.com>
Git-pin praxis-proxy/praxis#1172 so cap_stream_read_timeout is available on CI; crates.io 0.5.5 predates that merge. Recap leftover budget on every chunk after the first SSE event (including zero at the deadline), align check_timeout with io_exceeded_stream_deadline, and fix the idle integration test comment syntax. Signed-off-by: mkoushni <mkoushni@redhat.com>
Keep agentic-loop read_timeout_ms single-declaration while adopting main's translate_to_chat backend_endpoint wiring from praxis-proxy#1129. Signed-off-by: mkoushni <mkoushni@redhat.com>
Regenerate Cargo.lock after merging main (praxis-proxy#1008 token_rate_limit, praxis-proxy#1193 deps). Signed-off-by: mkoushni <mkoushni@redhat.com>
Use the fully qualified praxis_filter path so config.rs doc builds with -D warnings. Signed-off-by: mkoushni <mkoushni@redhat.com>
Sync generated docs with the fully qualified cap_stream_read_timeout link. Signed-off-by: mkoushni <mkoushni@redhat.com>
Relative read-timeout recaps restart when downstream backpressure delays the next upstream poll, so timeout_secs could overrun. Publish an absolute cutoff through cap_stream_deadline instead. Pin mkoushni/praxis at 21be732 until the Praxis change lands upstream. Signed-off-by: mkoushni <mkoushni@redhat.com>
dependency-check rejects git sources not listed in allow-git. The cap_stream_deadline fork pin needs the same temporary exemption as the earlier praxis-proxy/praxis git deps. Signed-off-by: mkoushni <mkoushni@redhat.com>
leseb
left a comment
There was a problem hiding this comment.
blocked on 0.5.6 core release
Use praxis-proxy/praxis for the temporary cap_stream_deadline git pin and restore protocol/tls/praxis to crates.io 0.5.5 like main. Drop the fork allow-list entry from deny.toml. Signed-off-by: mkoushni <mkoushni@redhat.com>
Mixing git-pinned core/filter with crates.io protocol/tls pulled two praxis_core copies into the graph and broke server health-check wiring. Pin the full praxis stack to the same rev until praxis-proxy#1179 ships on crates.io. Signed-off-by: mkoushni <mkoushni@redhat.com>
Signed-off-by: mkoushni <mkoushni@redhat.com>
Signed-off-by: mkoushni <mkoushni@redhat.com>
Signed-off-by: mkoushni <mkoushni@redhat.com>
Signed-off-by: mkoushni <mkoushni@redhat.com>
Signed-off-by: mkoushni <mkoushni@redhat.com>
Signed-off-by: mkoushni <mkoushni@redhat.com>
Signed-off-by: mkoushni <mkoushni@redhat.com>
Head branch was pushed to by a user without write access
Summary
openai_stream_events.timeout_secswas only checked when an SSE chunk or end-of-stream arrived. After the first event, a silent backend never invokes those callbacks, so the documented wall-clock budget did not fire.read_timeoutattimeout_secs(without relaxing a tighter cluster timeout) fromon_requestandon_request_body.responses.stream_error_*metadata as a parser timeout.read_timeout_msso a silent connection is torn down without waiting for another body callback.A filter-only timer cannot wake Pingora without body traffic. Cluster
read_timeout_msremains the reliable idle deadline when load balancing has not yet setctx.upstream.Related issue
Closes #938
Validation
make lintThe new integration test sends one
response.in_progressevent, stalls 10s, and asserts the proxy cuts the stream in under 4s viaread_timeout_ms: 1000plustimeout_secs: 1.Checklist
Signed-off-bytrailer.