Bound executor-controlled HTTP response buffering#31781
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 939bff96b6
ℹ️ 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".
| }) | ||
| .await?; | ||
| } | ||
| tokio::time::sleep(Duration::from_millis(100)).await; |
There was a problem hiding this comment.
This test relies on the notification handler draining and applying all 17 one-MiB body-delta notifications during this fixed 100 ms pause before the http/request response is sent. On a slow CI run, the response can be delivered first, the test starts calling recv(), permits are released as deltas arrive, and the shared byte budget may never be exhausted, causing the test to time out instead of observing the intended error. Use explicit synchronization that proves the queue has filled before returning the response rather than depending on timing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I tend to agree with codex re: sleeps for ordering in tests, I've seen them cause so many test flakes caused by noisy neighbors in CI. Is there a cleaner way to synchronize the test?
| /// What this tests: frame-count backpressure cannot hide an unbounded amount | ||
| /// of executor-controlled body bytes across the orchestrator's stream queues. | ||
| #[tokio::test] | ||
| async fn http_response_body_stream_enforces_queued_byte_budget() -> Result<()> { |
There was a problem hiding this comment.
Could we also have a test that checks that two or more streams all contend for the same 16mb budget? IIUC this test would still pass if the permits were per-connection.
| done: true, | ||
| error: Some(message.clone()), | ||
| }, | ||
| /*byte_permit*/ None, |
There was a problem hiding this comment.
nit: maybe leave a comment explaining? IIUC this is a failure path so we want to bypass budgets to deliver the notification but would help me and future readers to have the intent
| }) | ||
| .await?; | ||
| } | ||
| tokio::time::sleep(Duration::from_millis(100)).await; |
There was a problem hiding this comment.
I tend to agree with codex re: sleeps for ordering in tests, I've seen them cause so many test flakes caused by noisy neighbors in CI. Is there a cleaner way to synchronize the test?
| } else { | ||
| u32::try_from(queued_bytes).ok().and_then(|queued_bytes| { | ||
| Arc::clone(&self.http_body_stream_byte_budget) | ||
| .try_acquire_many_owned(queued_bytes) |
There was a problem hiding this comment.
IIUC exec-server basically has to trust that app-server will more or less work, is there a strong reason to fail these streams at capacity rather than doing self.http_body_stream_byte_budget.acquire(queued_bytes).await?
2 participants
Why
The remote exec-server is not a trusted process. Streamed HTTP responses were bounded by frame count, but each frame could carry almost the full JSON-RPC message limit. A peer could therefore make app-server retain a very large amount of response data before the existing 256-frame backpressure fired.
What changed
http/request/bodyDeltapayload.Scope
This bounds executor-controlled bytes retained by the HTTP response stream queues. It does not yet add a byte budget to the generic inbound JSON-RPC transport queue; that remains a separate hardening follow-up.