Repository navigation
fix: close the connection when OPA refuses an upload - #421
stuartwdouglas wants to merge 1 commit into
Conversation
When OPA denied a request with a body, net/http was meant to notice the large unread body and reply with Connection: close. otelhttp replaces r.Body, which hides the unread length, so net/http instead read up to 256KiB of the refused body before writing the 403. Setting Connection: close up front makes it reply immediately and close the connection, so clients and proxies don't reuse a socket that still has the unsent upload queued on it. Amp-Thread-ID: https://ampcode.com/threads/T-01a19db1-1772-7511-b6d4-83c4f42d74e8 Co-authored-by: Amp <amp@ampcode.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: fea2edfc73
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // The unread body is still on the wire, so the connection can't be reused. Saying so | ||
| // up front also stops net/http reading up to 256KiB of the body before replying, which | ||
| // it does because otelhttp replaces r.Body and hides the unread length from it. |
There was a problem hiding this comment.
This three-line comment annotates a small six-line denial branch, despite the repository rule allowing comments only for relatively large blocks of at least 20 lines. The condition and header assignment are already concise, so remove the comment rather than introducing a repository-convention violation.
AGENTS.md reference: AGENTS.md:L19-L22
Useful? React with 👍 / 👎.
When OPA refuses a request that has a body, cachew now sets
Connection: closeand sends the 403 straight away.net/http normally spots a large unread request body and replies with
Connection: closeon its own. otelhttp replacesr.Body, which hides how much is left unread, so net/http instead read up to 256 KiB of the refused upload before it wrote the 403. A client that sends slowly, or stops sending once it has a response, waited for that read. Setting the header up front makes net/http skip the read, reply at once, and close the socket, so nothing reuses a connection that still has the rest of the upload queued on it.This is the cachew side of squareup/blox#5031, where Roxy reused a connection after a refused cache save and the next request failed with a 502.
Checked against the full
newServermiddleware stack with a 64 MiB upload where only 1000 bytes were sent. Before the change, no response arrived within 5 s. After it, the 403 withConnection: closearrived in under 1 ms and the server closed the connection. The newTestMiddlewareDeniedUploadClosesConnectionfails without the fix.If Roxy reaches cachew through an Envoy mesh, check that the header gets through. Envoy treats
Connectionas hop-by-hop.