feat(filter): add a bounded, incremental SSE codec - #1060
Conversation
|
Commit message format: the following commits do not follow conventional commits:
Expected format: |
|
PR too large: 1788 lines added (limit: 500, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. |
|
@shaneutt any chance we could squeeze this in 0.5.4? If not, no biggie. |
Add a provider-neutral, bounded, incremental Server-Sent Events (SSE) codec to praxis-filter, exposed as `praxis_filter::sse`. - `SseDecoder` frames records from streamed body chunks under configurable byte/field limits (`SseLimits`), tolerating cross-chunk splits, CR/CRLF/LF terminators, and a leading UTF-8 BOM, and poisons itself on a limit violation. - `SseRecord`/`SseField` model fields in wire order with typed accessors (`data`/`event`/`id`/`retry`/`is_event`); a validated `SseRecordBuilder` rejects framing-breaking values. - `encode`/`encode_into` serialize records to canonical wire bytes. Pingora keeps ownership of transport streaming, backpressure, and H1/H2 framing; provider JSON, `[DONE]`, and lifecycle concerns stay in the consumer. Leaf library with no consumers yet. Signed-off-by: Sébastien Han <seb@redhat.com>
8f5b20b to
8824952
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Well-implemented bounded SSE codec with thorough test coverage. The decoder correctly handles all SSE spec edge cases (BOM stripping, CR/CRLF/LF terminators, cross-chunk splits, limit poisoning). The encoder round-trips cleanly with the decoder. Two convention issues found.
Overall: Solid library addition. Minor convention fixes needed.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 2 |
Test helper fns and constants must come after all #[test] functions in a #[cfg(test)] module, and separator comments must use the full-width canonical form. - decoder.rs: move BOM_BYTES, push_ok, decode_whole, decode_split, and assert_all_splits_match below the last test; replace the short-form "// ----- Test Utilities -----" separator with the full-width form. - record.rs: move assert_invalid_unknown_name below the last test under a full-width "// Test Utilities" separator. Signed-off-by: Sébastien Han <seb@redhat.com>
|
|
||
| /// Feed one body chunk; returns the records it completed and an optional | ||
| /// error. | ||
| pub fn push(&mut self, chunk: &[u8]) -> SseBatch { |
There was a problem hiding this comment.
The signature here is something I would wanna get settled early since it's the expensive thing to change once filters depend on it 🤔
Pingora gives body filters a Bytes, and by taking &[u8] we're committing to copying every field value twice: once into line_buf, then again with Bytes::copy_from_slice when the line is classified, plus a fresh Vec<SseField> allocation per record because fields gets mem::taken.
For an LLM token stream that's roughly three allocations per token. If push took Bytes instead, any field that lands entirely inside one chunk (which is nearly all of them in practice) could just be a slice_ref into the chunk, and we'd only fall back to copying when a line actually straddles a chunk boundary.
It would be OK in my book to have this be a follow-up for later and keep the internals as they are for now I just don't want the public signature to rule that out for us to optimize later.
| match self.parse(chunk, &mut records) { | ||
| Ok(()) => SseBatch { records, error: None }, | ||
| Err(err) => { | ||
| self.state = DecoderState::Poisoned(err); |
There was a problem hiding this comment.
Something we might optimize: When we poison line_buf and fields are left as-is so a decoder that just hit a limit is holding onto up to max_line_bytes + max_record_bytes until someone drops it. That's fine if the consumer drops it immediately, but if a filter keeps the decoder in a per-connection ctx (which is the natural place for it) that memory sits for the life of the connection.
Since a poisoned decoder can't produce anything further anyway, I think I would just clear both buffers as part of the transition? 🤔
| /// in-progress block accumulated any field, returns it as a single trailing | ||
| /// record. Idempotent: a second call returns an empty batch. While poisoned, | ||
| /// re-reports the limit error. | ||
| pub fn finish(&mut self) -> SseBatch { |
There was a problem hiding this comment.
I think this one might be worth changing before anything consumes it:
If the upstream connection drops in the middle of a data: line, the finish method currently takes whatever's buffered, treats it as a terminated line, and hands it back as a normal record.
From the consumer's side that's indistinguishable from a real one, so a half-written JSON payload would get parsed (or forwarded to the client) as though it were complete?
The event-stream spec actually says the opposite: "an incomplete event at EOF is discarded, not dispatched". That feels right, WTDY? If I'm not misunderstanding, we need a mechanism here so the caller doesn't lose framing without knowing.
I'd rather finish say "there was a partial record at EOF" explicitly, either as an SseDecodeError::Truncated alongside the completed records, or as a separate trailing field on SseBatch, and let the caller decide whether that's an error or something to salvage.
| self.state = DecoderState::Active; | ||
| self.bom_len = 0; | ||
| self.bom_resolved = false; | ||
| } |
There was a problem hiding this comment.
If you wanted to ensure that this reset doesn't let new fields on the struct slip past in the future, you can use a destructuring instead of the type fields directly:
let Self { line_buf, fields, record_bytes, prev_cr, state, bom_len, bom_resolved, limits = _ } = self;
line_buf.clear();Notably not using the range operator (..) in the destructuring so any future field added that doesn't get a reset implementation gets a compile-time error.
All this said, I don't think we actually want a reset method here: you would normally implement a reset method for something that's on the hot path with allocations so you can reduce memory thrash, however, that isn't the case here:
with_limits is already allocation-free (Vec::new() doesn't allocate), so constructing a fresh decoder costs the same as resetting one. The only capacity reset could preserve is line_buf (fields is mem::taken on every dispatched record, so its allocation never survives anyhow) and a decoder lives for a whole stream, so construction happens per stream, not per chunk. A couple of line_buf reallocs per streamed response isn't measurable given this context.
Semantically this also is a bit perplexing: the main thing reset does is "unpoison" or "unfinish", but once a limit violation has lost framing, resuming the same stream just yields garbage? The only legitimate use is starting a new stream, and a fresh with_limits(self.limits) does that without offering an escape hatch. With no consumers yet, I might drop reset entirely? If you want to keep the name, make it *self = Self::with_limits(self.limits), which also sidesteps the field-drift problem above, but IDK if that's really worth it over just having the caller make their own?
| } | ||
|
|
||
| /// Consume a byte slice, appending completed records to `records`. | ||
| fn feed(&mut self, bytes: &[u8], records: &mut Vec<SseRecord>) -> Result<(), SseDecodeError> { |
There was a problem hiding this comment.
This is the loop that runs for every byte of every streamed response, and right now it's a Vec::push plus a limit check per byte: so scanning ahead for the next "\n/\r" with iter().position and then extend_from_slice-ing the whole run with a single length check per run would be a lot cheaper and slightly simpler to read.
|
|
||
| pub use decoder::{SseBatch, SseDecodeError, SseDecoder, SseLimits}; | ||
| pub use encoder::{encode, encode_into}; | ||
| pub use record::{SseBuildError, SseField, SseRecord, SseRecordBuilder}; |
There was a problem hiding this comment.
I appreciate the generous rustdocs in the mod.rs 🙇
Address review feedback on the SSE decoder limits: - Extract the SseLimits default magic numbers into named DEFAULT_MAX_* constants, matching the convention used elsewhere in the codebase. - Lower the default max_line_bytes from 10 MiB to 1 MiB; callers that need more can raise it via SseLimits. - Expand the max_record_bytes doc to explain that the in-progress line is bounded separately by max_line_bytes, so peak per-record memory is max_record_bytes + max_line_bytes. - Add the standard section separators for each top-level type (DecoderState, SseLimits, SseBatch, SseDecodeError, SseDecoder). Signed-off-by: Sébastien Han <seb@redhat.com>
|
Pulled out of 0.5.4, I don't want to block the release. |
What does this PR do?
Adds a provider-neutral, bounded, incremental Server-Sent Events (SSE) codec to
praxis-filter, exposed aspraxis_filter::sse.SseDecoderframes records from streamed body chunks under configurable byte/field limits, tolerating cross-chunk splits, CR/CRLF/LF terminators, and a leading UTF-8 BOM, and it poisons itself on a limit violation so a caller cannot silently lose framing. A validatedSseRecordBuilderplusencode/encode_intoserialize locally generatedSseRecords to canonical wire bytes. It is a leaf codec: Pingora keeps ownership of transport streaming, backpressure, and H1/H2 framing, while provider JSON,[DONE], and lifecycle concerns stay in the consumer. There are no consumers yet, so this adds the library only.Which issue(s) does this relate to?
Fixes #986
Checklist
git commit -s)make lint && make testpasses locally —make lintandmake buildpass, and the filter crate's 2114 unit tests + 65 doctests pass. The full-workspacemake testis green except a pre-existing, unrelated flaky testtls::watcher::tests::watcher_reloads_on_file_change, which fails only under full-suite parallel load (fixed 2000 ms filesystem-watch window) and passes 262/262 in isolation. This PR touches onlyfilter/src/sse/*and one line offilter/src/lib.rs, so it has no causal path to the TLS watcher.Does this introduce a breaking change?
No. It adds a new, additive module (
praxis_filter::sse) with no changes to existing code and no current consumers.