Repository navigation
fix: don't truncate downloads when the cache write fails - #420
Conversation
A cache write failure aborted the shared copy feeding the client, so clients got 200 OK with a truncated body. The S3 backend triggered this whenever upstream response headers exceeded S3's 2 KB user metadata limit. - handler: keep streaming to the client when the cache writer fails or can't be created, and discard the cache entry. - s3: when headers don't fit in user metadata, store only the headers needed to serve the object (content headers, ETag, Last-Modified, X-Cachew-*), and fail Create up front if even those don't fit. - handler: log the cache key with fetch and serve errors. Amp-Thread-ID: https://ampcode.com/threads/T-01a19b0c-ec15-75f2-b427-a1f18a35f91c 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: 019e757601
ℹ️ 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".
| // cacheTee forwards writes to a cache writer until it fails, then discards | ||
| // them, so a cache failure can't abort the copy that feeds the client. |
There was a problem hiding this comment.
This private type and its Write method form a small, self-explanatory helper, so the newly added comment violates the repository rule that comments are reserved for non-obvious blocks of at least 20 lines; remove this comment.
AGENTS.md reference: AGENTS.md:L19-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed in 7558c3e, along with a similar inline comment in s3UserMetadata.
Amp-Thread-ID: https://ampcode.com/threads/T-01a19b0c-ec15-75f2-b427-a1f18a35f91c Co-authored-by: Amp <amp@ampcode.com>
jrobotham-square
left a comment
jrobotham-square
left a comment
There was a problem hiding this comment.
Only concern from me is that it's not ideal to behave differently when the size is too big... perhaps we should always filter to the 'essential headers'?
That said, the bug feels pretty edgy so this is probably fine. (and switching to 'essential headers' might be more likely to cause other issues)
A cache write failure aborted the copy feeding the client, so clients received
200 OKwith a truncated body (e.g. Gradle wrapper zips failing withzip END header not found). The S3 backend hit this whenever upstream response headers exceeded S3's 2 KB user metadata limit (Your metadata headers exceed the maximum allowed metadata size), and since the write always failed, the object was never cached.Createfailures no longer interrupt the client stream; the response is streamed in full and the cache entry is discarded.ETag,Last-Modified,X-Cachew-*).Createfails up front if even those don't fit.cache_keyso failures can be tied to an upstream URL.