Add regression tests for paced upstream streaming - #650
Conversation
…akeups) The outbound streaming client's tests (http_stream_test.rs) serve their whole body in one shot with Content-Length and 'Connection: close', so every read finds bytes already buffered. The feature's real workload — a model endpoint trickling SSE/NDJSON chunks onto a kept-alive connection — exercises the complementary shape: chunks arriving while 'wait for next line' is parked, and end-of-body observable only from the chunked terminator (the server never closes the socket). Adds two multi-thread-runtime integration tests for that shape (testing.md §11.3 streaming/lifecycle): a single chunk arriving 200ms after the read parks, and ten 30ms-paced chunks delivered in order ending in a clean EOF. Both pass today — this documents and locks in the behavior the Rin chat demo (LogbieLLC/Rin) depends on for proxying the Anthropic API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141poBiWyVCasUHBttCuHq9
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3a8f52039
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// `wait for next line` must wake the read and be delivered. | ||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn test_next_line_wakes_for_chunk_arriving_while_parked() { | ||
| let url = spawn_paced_chunked_server(vec!["late-arrival"], 200).await; |
There was a problem hiding this comment.
The 200 ms delay starts after the server writes the response headers, not after the interpreter has entered wait for next line. On a loaded or temporarily suspended runner, the timer can expire and the chunk can be buffered before run_wfl reaches the read, allowing this test to pass even when parked-read wakeups are broken. Add deterministic synchronization with the pending-read state rather than relying on elapsed wall-clock time.
AGENTS.md reference: AGENTS.md:L145-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already resolved on the branch by commit e1a9b84: the parked-read test now synchronizes via a /go side-channel (the late chunk is withheld until the program has consumed the first line and signaled progress) and asserts the read genuinely parked (>=150ms of a 300ms delay), so a pre-buffered chunk fails loudly instead of passing vacuously. No further change needed.
| assert_eq!(get_number(&interpreter, "line_count"), expected); | ||
| assert_eq!(get_text(&interpreter, "last_line"), "data: ten"); |
There was a problem hiding this comment.
These assertions only verify that ten lines were returned and that the final line was data: ten; a regression that duplicates, drops, substitutes, or reorders any intermediate lines while preserving the count and final value would still pass. Since this test claims to prove that every paced chunk is delivered in order, collect and compare the full sequence or assert each position.
AGENTS.md reference: AGENTS.md:L145-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already resolved on the branch by commit e1a9b84: the paced-sequence test now collects the full ordered sequence into all_lines and asserts it equals data: one|...|data: ten|, so a dropped, duplicated, substituted, or reordered intermediate line fails the test.
There was a problem hiding this comment.
Pull request overview
Adds new Rust integration tests that regress outbound HTTP response streaming behavior when the upstream delivers data progressively over time on a kept-alive, chunked connection. This complements the existing http_stream_test.rs coverage that primarily exercises buffered/Connection: close style responses.
Changes:
- Introduces a paced, chunked TCP upstream test server helper that writes delayed chunks and keeps the connection open after the chunk terminator.
- Adds a regression test ensuring
wait for next linewakes correctly when data arrives after the read is already parked. - Adds a regression test ensuring multiple paced chunks are delivered in order and EOF is detected via chunked framing (not connection close), under bounded timeouts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback on the paced-stream tests: - The parked-read wakeup test now synchronizes with the upstream via a /go side channel instead of racing a wall-clock delay against interpreter startup: the late chunk is written only after the program has consumed the first line and acknowledged it. The program also timestamps the parked read, and the test asserts it genuinely waited (>=150ms of a 300ms delay) so a pre-buffered chunk fails loudly instead of passing vacuously. - The paced-sequence test now asserts the complete ordered line sequence, not just the count and final line, so dropped or reordered middle chunks are caught. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141poBiWyVCasUHBttCuHq9
| let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); | ||
| let addr = listener.local_addr().unwrap(); | ||
| tokio::spawn(async move { | ||
| let (mut socket, _) = listener.accept().await.unwrap(); | ||
| let mut tmp = [0u8; 4096]; | ||
| let _ = socket.read(&mut tmp).await; | ||
| socket.write_all(CHUNKED_HEAD).await.unwrap(); | ||
| socket.flush().await.unwrap(); |
Summary
Add comprehensive regression tests for outbound response streaming against a paced (time-delayed) upstream server. These tests validate the core streaming feature by exercising the scenario it was designed for: an upstream that emits data progressively over time on a kept-alive connection, rather than all at once.
Key Changes
New test file:
tests/http_stream_paced_test.rswith two integration teststest_next_line_wakes_for_chunk_arriving_while_parked(): Validates thatwait for next linewakes for a chunk that arrives strictly after the read is parked, and detects EOF via the chunked terminator (not connection close). Synchronization is deterministic rather than a bare wall-clock race: the upstream sends areadyline, withholds the late chunk until the program consumesreadyand hits a/goendpoint, and the test asserts the read genuinely parked (>= 150ms of a 300ms delay) so a pre-buffered chunk fails loudly instead of passing vacuously.test_next_line_delivers_every_paced_chunk_then_eof(): Validates that many paced chunks (~30ms cadence, simulating real SSE/NDJSON) are delivered in order with clean EOF. It asserts the complete ordered line sequence (not just the count and final line), so a dropped, duplicated, substituted, or reordered intermediate line is caught.Shared test helpers:
chunk_frame()builds oneTransfer-Encoding: chunkedframe for a line, and theCHUNKED_HEADconstant holds the chunked response head. Each test defines its own upstream inline because their shapes differ: the parked-read test needs a two-connection/gohandshake for deterministic synchronization, while the paced-sequence test uses a simple timed chunk loop. Both hold the connection open after the terminator (noConnection: close) so EOF is proven from the chunked framing alone.Implementation Details
tokio::time::timeout()to enforce bounded wall-clock completion and catch stalled readstesting.md§11.3): proves ordering, wakeups for parked reads, clean EOF via chunked framing, and bounded completionhttp_stream_test.rswhich tests the simpler case (whole body buffered,Connection: close)https://claude.ai/code/session_0141poBiWyVCasUHBttCuHq9
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.