feat(filter): resume IRR iterations within streaming responses - #988
Conversation
Keep the downstream response open while the iterative request router runs successive model and tool steps. Stream each step incrementally, then use completion transitions to resume the next step without committing a new response. Add bounded iteration state, logical stream limits, deadline handling, header-safe failover, and typed termination handling. Propagate runtime resources and listener limits into nested step pipelines, and preserve correct lifecycle ordering across normal completion, errors, and client disconnects. Cover the behavior with unit and end-to-end tests for streaming order, multi-step resumption, suppression, limits, failover, and cleanup. Fixes praxis-proxy#984 Signed-off-by: Sébastien Han <seb@redhat.com>
|
PR too large: 2175 lines added (limit: 750, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. Add |
|
Validated on praxis-proxy/ai#756 |
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review: feat(core): add streaming SubRequestClient response transport
Adds IrrStreamingSession for multi-step streaming within a committed downstream response, refactors step execution into a reusable IrrStepRunner, and introduces typed abnormal stream termination (StreamTermination) with filter-driven completion hooks.
Overall Assessment
This is a large, well-structured PR with thorough test coverage (85+ streaming tests). The streaming session design is sound: header-safe failovers run before body exposure, completion-dependent transitions run after clean EOF, and the bounded pending-chunk emission model keeps memory under control. The nested pipeline propagation via visit_nested_pipelines is a necessary architectural fix.
Three findings, all Medium severity.
| Severity | File | Finding |
|---|---|---|
| Medium | runner.rs |
Missing per-step tracing span (observability regression) |
| Medium | mod.rs, streaming.rs |
into_completion() error paths drop parent extensions |
| Medium | request_filter/mod.rs |
next_chunk() cancellation safety undocumented in tokio::select! |
Fixes a testing flake when the metrics wouldn't respond quickly enough. Signed-off-by: Shane Utt <shaneutt@linux.com>
Restore the per-step tracing span around nested filter and transport execution so logs retain their step and iteration context. Return completion failures with recoverable parent extensions and restore them across buffered and committed-streaming error paths. Add regression coverage for both lifecycle shapes. Document the RAII cleanup contract when downstream disconnect races an in-flight streaming step. Signed-off-by: Sébastien Han <seb@redhat.com>
The merge with upstream/main incorrectly kept the old multi-line wait_for_metric call that upstream replaced with thread::sleep, causing a nightly rustfmt check failure in CI. Signed-off-by: Sébastien Han <seb@redhat.com>
shaneutt
left a comment
There was a problem hiding this comment.
Did an initial pass: putting a hold for me to do a thorough review.
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review
All three findings from the initial review are addressed by the fix commit (75ba179):
- Per-step tracing span: restored via
.instrument(step_span)inopen_step(). into_completion()extension recovery:StepCompletionErrorcarries parent extensions; four regression tests confirm restoration across buffered, streaming, and guard paths.- Cancellation safety: documented with a
// Cancellation contract:comment at thetokio::select!site.
One new finding from the refactored code.
The streaming transport path logged failures with warn! but the buffered path silently discarded both build_peer and execute errors. Add matching warn! calls so production debugging has parity across both transport modes. Signed-off-by: Sébastien Han <seb@redhat.com>
Upstream renamed `has_body_dependent_transitions` to `streaming_transition_order_is_valid` with new semantics (interleaved transition phases) and shortened the StreamBuffer error message. Update test names, YAML configs, and assert strings accordingly. Signed-off-by: Sébastien Han <seb@redhat.com>
…-984 Resolve three conflicts from upstream's perf/refactor batch: - filter/src/pipeline/build.rs: adopt upstream's body_filter_indices() and request/response_body_filter_indices fields while keeping our id_generator/time_source construction path. - filter/.../iterative_request_router/config.rs: keep both the max_stream_response_bytes validation (ours) and the max_response_bytes and step_timeout_ms validations (upstream); they are additive. - filter/.../iterative_request_router/mod.rs: keep our runner.open_step() refactor and drop upstream's superseded inline step-execution block; the extracted logic lives in runner.rs. Signed-off-by: Sébastien Han <seb@redhat.com>
What does this PR do?
Extends
iterative_request_routerso a single committed downstream response can span multiple IRR steps.The first streamed step can complete, evaluate its existing
on_resulttransitions, start another model or tool step, and continue writing through the same client response without recommitting headers.Key additions:
IrrStepRunnershared by buffered and streaming executionIrrStreamingSessionthat owns state and resumes steps after streamed completionThe existing
on_resultconfiguration continues to drive transitions. No provider-specific SSE parsing, tool policy, or new transition YAML is added to Praxis.This builds on the streaming transport and terminal-response foundation merged through #951, #952, and #953. Those changes are dependencies; they do not implement the resumed IRR session loop added here.
Which issue(s) does this relate to?
Fixes #984
Parent epic: #877
Checklist
git commit -s)make lintpasses locallymake docpasses locallyDoes this introduce a breaking change?
No. Existing buffered and terminal-only streaming behavior remains supported. The new streaming-session capabilities and limits are additive.