feat(s3): add Content-Encoding header support for S3 uploads#26149
feat(s3): add Content-Encoding header support for S3 uploads#26149Jarred-Sumner merged 2 commits intomainfrom
Conversation
|
Updated 5:06 PM PT - Jan 15th, 2026
❌ @cirospaciari, your commit f47bb7e has 8 failures in
🧪 To try this PR locally: bunx bun-pr 26149That installs a local version of the PR into your bun-26149 --bun |
WalkthroughPropagates a new contentEncoding option through S3 upload flows: adds Headers.getContentEncoding, extends S3 APIs/structs to accept content_encoding, threads it through credential signing/canonical request generation and multipart/upload paths, updates header buffer sizing, and adds tests for presigned uploads respecting Content-Encoding. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/s3/credentials.zig (1)
372-383: Fix header buffer sizing to avoid OOB writes.
Withcontent_encodingadded, max optional headers rises to 7 (acl, session token, storage class, content disposition, content encoding, content md5, request payer), making total headers 11. The fixed-size array is length 10, so a fully populated header set can write past the end.🐛 Proposed fix
@@ - _headers: [10]picohttp.Header = .{ + _headers: [11]picohttp.Header = .{ .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, + .{ .name = "", .value = "" }, }, @@ - ._headers = [_]picohttp.Header{ + ._headers = [_]picohttp.Header{ .{ .name = "x-amz-content-sha256", .value = aws_content_hash }, .{ .name = "x-amz-date", .value = amz_date }, .{ .name = "Host", .value = host }, .{ .name = "Authorization", .value = authorization[0..] }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, .{ .name = "", .value = "" }, + .{ .name = "", .value = "" }, },Also applies to: 878-889
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between dfa704c and a74de3b9de1bfa91d5e6c05cd6e8d6a65767aaf7.
📒 Files selected for processing (8)
src/bun.js/webcore/Blob.zigsrc/bun.js/webcore/fetch.zigsrc/http/Headers.zigsrc/s3/client.zigsrc/s3/credentials.zigsrc/s3/multipart.zigsrc/s3/simple_request.zigtest/js/bun/s3/s3.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct {#foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/s3/multipart.zigsrc/s3/simple_request.zigsrc/bun.js/webcore/fetch.zigsrc/http/Headers.zigsrc/s3/client.zigsrc/s3/credentials.zigsrc/bun.js/webcore/Blob.zig
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/bun/s3/s3.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.ts: Use Bun's Jest-compatible test runner with proper test fixtures and imports fromharness
Always useport: 0when binding to ports in tests - never hardcode ports or use custom random port functions
UsenormalizeBunSnapshotto normalize snapshot output in tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output as these will never fail in CI
UsetempDirfromharnessto create temporary directories in tests - do not usetmpdirSyncorfs.mkdtempSync
In spawned process tests, use expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) for more useful error messages
Do not usesetTimeoutin tests - await the condition to be met instead, as you are testing the CONDITION not TIME PASSING
Files:
test/js/bun/s3/s3.test.ts
🧠 Learnings (9)
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/s3/multipart.zigsrc/s3/simple_request.zigsrc/bun.js/webcore/fetch.zigsrc/http/Headers.zigsrc/s3/client.zigsrc/s3/credentials.zigsrc/bun.js/webcore/Blob.zig
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
src/bun.js/webcore/fetch.zigtest/js/bun/s3/s3.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/bun/s3/s3.test.ts
📚 Learning: 2025-09-03T05:09:24.272Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22335
File: src/http.zig:1376-1393
Timestamp: 2025-09-03T05:09:24.272Z
Learning: In bun's HTTP module, `bun.http.default_allocator` is the correct allocator to use for internal HTTP buffers like MutableString initialization for response_message_buffer and compressed_body. This allocator is initialized in HTTPThread.zig as `bun.http.default_arena.allocator()` and is used consistently across the HTTP module, separate from any local `default_allocator` variables.
Applied to files:
src/s3/credentials.zig
📚 Learning: 2025-09-07T14:00:36.526Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22258
File: src/bun.js/test/jest.zig:63-86
Timestamp: 2025-09-07T14:00:36.526Z
Learning: In Bun's Zig codebase, the established pattern for HashMap hash functions is to truncate hash values to u32, even when the underlying hash computation produces u64. This is seen throughout the codebase, such as in src/css/rules/rules.zig where u64 hashes are stored but truncated to u32 in HashMap contexts. When implementing HashContext for HashMapUnmanaged, the hash function should return u32 to match this pattern.
Applied to files:
src/s3/credentials.zig
🧬 Code graph analysis (1)
test/js/bun/s3/s3.test.ts (1)
test/js/bun/s3/s3-stream-leak-fixture.js (1)
s3file(9-9)
🔇 Additional comments (10)
src/http/Headers.zig (1)
82-84: Nice, minimal accessor for Content-Encoding.
Matches existing header getters and keeps lookup centralized.src/bun.js/webcore/fetch.zig (1)
1303-1306: Content-Encoding propagation for S3 streams looks good.
This cleanly forwards the header when present.src/s3/simple_request.zig (2)
347-355: Good API surface addition.
Optional field aligns with existing content_* options.
371-377: Signing now honors Content-Encoding as intended.
Nice, direct propagation into signRequest.test/js/bun/s3/s3.test.ts (1)
448-486: Solid coverage for content-encoding across write and writer paths.
Usingdecompress: falsekeeps assertions accurate for uncompressed payloads.src/bun.js/webcore/Blob.zig (2)
971-972: Content-Encoding now flows through all S3 upload/write paths.
Good propagation across direct upload, uploadStream, and pipe paths.Also applies to: 1124-1125, 1165-1166, 1197-1198, 1405-1406, 1468-1469, 2447-2448
2684-2701: S3 writer supportscontentEncodingalongsidecontentDisposition.
The option parsing and slice lifetime handling look consistent.Also applies to: 2714-2715
src/s3/multipart.zig (1)
118-121: Multipart upload now propagates Content-Encoding correctly.
Field lifecycle and request wiring are aligned with existing header behavior.Also applies to: 288-292, 319-320, 612-613, 691-692
src/s3/client.zig (1)
235-257: S3 client APIs now plumbcontentEncodingthrough upload helpers.
The wiring into MultiPartUpload is consistent and complete.Also applies to: 263-271, 309-317, 447-458, 490-498
src/s3/credentials.zig (1)
232-245: Content-Encoding is consistently threaded through signing and header construction.
The option parsing, canonical request updates, and signed header generation are cohesive.Also applies to: 365-367, 413-415, 439-446, 556-559, 667-674, 916-921, 939-969, 975-1008, 1014-1055, 1057-1093
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Add support for setting the `Content-Encoding` header in S3 `.write()` and
`.writer()` calls, following the same pattern as `Content-Disposition`.
This allows users to specify the encoding of uploaded content:
```typescript
await s3file.write(data, { contentEncoding: "gzip" });
const writer = s3file.writer({ contentEncoding: "br" });
```
Implementation changes:
- Extended SignedHeaders.Key from 6 bits to 7 bits (64→128 combinations)
- Added content_encoding to S3CredentialsWithOptions, SignOptions, SignResult
- Updated CanonicalRequest format strings for AWS SigV4 signing
- Added getContentEncoding() to Headers for fetch-based uploads
- Expanded _headers array from 9 to 10 elements
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
cirospaciari
force-pushed
the
ciro/s3-content-encoding
branch
from
a74de3b to
9995f62
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bun.js/webcore/Blob.zig (1)
2684-2704: DeinitS3CredentialsWithOptionsto avoid leaking option slices.
getCredentialsWithOptionsallocates ZigString slices (now includingcontentEncoding), but the result is never deinitialized here. That leaks per call when options include any credential/option strings. Add a defer after creation since you pass duplicated credentials intoS3.writableStream.🛠️ Proposed fix
- const credentialsWithOptions = try s3.getCredentialsWithOptions(options, globalThis); + const credentialsWithOptions = try s3.getCredentialsWithOptions(options, globalThis); + defer credentialsWithOptions.deinit(); return try S3.writableStream( credentialsWithOptions.credentials.dupe(), path, globalThis, credentialsWithOptions.options, this.contentTypeOrMimeType(), if (content_disposition_str) |cd| cd.slice() else null, if (content_encoding_str) |ce| ce.slice() else null, proxy_url, credentialsWithOptions.storage_class, credentialsWithOptions.request_payer, );src/s3/credentials.zig (1)
387-414: Fix buffer overflow inmixWithHeader()call sites.
_headers_lencan reach 11 (from 4 initial headers plus 7 optional additions), but all callers pass[10]picohttp.Headerbuffers. WhenmixWithHeader()copies 11 existing headers and appends one more, it writes beyond the buffer bounds.Update all three call sites to use
[12]picohttp.Headerbuffers:
src/s3/simple_request.zig:388src/s3/client.zig:568src/bun.js/webcore/fetch.zig:1347
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between a74de3b9de1bfa91d5e6c05cd6e8d6a65767aaf7 and 9995f62.
📒 Files selected for processing (8)
src/bun.js/webcore/Blob.zigsrc/bun.js/webcore/fetch.zigsrc/http/Headers.zigsrc/s3/client.zigsrc/s3/credentials.zigsrc/s3/multipart.zigsrc/s3/simple_request.zigtest/js/bun/s3/s3.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct {#foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/webcore/fetch.zigsrc/s3/simple_request.zigsrc/bun.js/webcore/Blob.zigsrc/s3/client.zigsrc/http/Headers.zigsrc/s3/credentials.zigsrc/s3/multipart.zig
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/bun/s3/s3.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.ts: Use Bun's Jest-compatible test runner with proper test fixtures and imports fromharness
Always useport: 0when binding to ports in tests - never hardcode ports or use custom random port functions
UsenormalizeBunSnapshotto normalize snapshot output in tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output as these will never fail in CI
UsetempDirfromharnessto create temporary directories in tests - do not usetmpdirSyncorfs.mkdtempSync
In spawned process tests, use expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) for more useful error messages
Do not usesetTimeoutin tests - await the condition to be met instead, as you are testing the CONDITION not TIME PASSING
Files:
test/js/bun/s3/s3.test.ts
🧠 Learnings (6)
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/fetch.zigsrc/s3/simple_request.zigsrc/bun.js/webcore/Blob.zigsrc/s3/client.zigsrc/http/Headers.zigsrc/s3/credentials.zigsrc/s3/multipart.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/bun/s3/s3.test.ts
📚 Learning: 2025-09-03T05:09:24.272Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22335
File: src/http.zig:1376-1393
Timestamp: 2025-09-03T05:09:24.272Z
Learning: In bun's HTTP module, `bun.http.default_allocator` is the correct allocator to use for internal HTTP buffers like MutableString initialization for response_message_buffer and compressed_body. This allocator is initialized in HTTPThread.zig as `bun.http.default_arena.allocator()` and is used consistently across the HTTP module, separate from any local `default_allocator` variables.
Applied to files:
src/s3/credentials.zig
📚 Learning: 2025-09-07T14:00:36.526Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22258
File: src/bun.js/test/jest.zig:63-86
Timestamp: 2025-09-07T14:00:36.526Z
Learning: In Bun's Zig codebase, the established pattern for HashMap hash functions is to truncate hash values to u32, even when the underlying hash computation produces u64. This is seen throughout the codebase, such as in src/css/rules/rules.zig where u64 hashes are stored but truncated to u32 in HashMap contexts. When implementing HashContext for HashMapUnmanaged, the hash function should return u32 to match this pattern.
Applied to files:
src/s3/credentials.zig
🧬 Code graph analysis (1)
test/js/bun/s3/s3.test.ts (1)
test/js/bun/s3/s3-stream-leak-fixture.js (1)
s3file(9-9)
🔇 Additional comments (8)
src/bun.js/webcore/fetch.zig (1)
1295-1306: Content-Encoding is correctly plumbed into S3 stream uploads.
Matches the existing content-type/disposition pattern and keeps the S3 streaming path consistent.src/http/Headers.zig (1)
79-86: Header accessor addition is consistent and minimal.
The newgetContentEncoding()mirrors existing helpers and keeps the API surface tidy.src/s3/simple_request.zig (1)
347-355: Content-Encoding is now part of the signed request options.
Propagatingcontent_encodingintosignRequestaligns with the new header support.Also applies to: 371-377
src/s3/multipart.zig (1)
118-121: Multipart flow correctly carries and cleans up Content-Encoding.
Propagation mirrors existing header fields and deinit cleanup is consistent.Also applies to: 288-292, 312-323, 604-616, 684-695
test/js/bun/s3/s3.test.ts (1)
448-486: Solid coverage for Content-Encoding on write and writer flows.
Usingdecompress: falsekeeps header assertions reliable.src/bun.js/webcore/Blob.zig (1)
965-983: Content‑Encoding propagation across S3 write paths looks consistent.The new header is threaded through empty writes, stream uploads, and readable‑stream paths cleanly.
Also applies to: 1114-1129, 1159-1176, 1187-1202, 1395-1410, 1458-1473, 2437-2452
src/s3/client.zig (1)
235-261: S3 client API wiring forcontent_encodingis consistent.Signatures, request options, and multipart initialization all align with the new header.
Also applies to: 263-346, 448-527
src/s3/credentials.zig (1)
247-260: Content‑Encoding is fully integrated into signing and header emission.The new option flows through SignOptions, SignedHeaders, CanonicalRequest, and response headers as expected.
Also applies to: 381-399, 429-432, 455-466, 577-581, 689-697, 873-889, 967-972, 990-1024, 1030-1062, 1068-1148
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add MAX_HEADERS constant to SignResult to prevent buffer overflow when mixWithHeader() copies headers to caller-provided buffers - Fix memory leak in Blob.zig by adding defer deinit for credentialsWithOptions - Update all header buffer declarations to use MAX_HEADERS + 1 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/s3/credentials.zig`:
- Around line 247-261: Rename the private field _contentEncodingSlice to
`#contentEncodingSlice` everywhere: update the field declaration in the
credentials struct and replace all references (e.g.,
new_credentials._contentEncodingSlice, new_credentials._contentEncodingSlice.?
and any assignments) with the new private identifier
new_credentials.#contentEncodingSlice; also update any related uses that set
new_credentials.content_encoding =
new_credentials._contentEncodingSlice.?.slice() to use the renamed field. Apply
the same changes at the other occurrences you noted (around the other blocks at
lines referenced in the comment) so the declaration and all reads/writes use
`#contentEncodingSlice` consistently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/bun.js/webcore/Blob.zigsrc/bun.js/webcore/fetch.zigsrc/s3/client.zigsrc/s3/credentials.zigsrc/s3/simple_request.zig
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct {#foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/bun.js/webcore/fetch.zigsrc/s3/simple_request.zigsrc/bun.js/webcore/Blob.zigsrc/s3/client.zigsrc/s3/credentials.zig
🧠 Learnings (4)
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/webcore/fetch.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/fetch.zigsrc/s3/simple_request.zigsrc/bun.js/webcore/Blob.zigsrc/s3/client.zigsrc/s3/credentials.zig
📚 Learning: 2025-09-03T05:09:24.272Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22335
File: src/http.zig:1376-1393
Timestamp: 2025-09-03T05:09:24.272Z
Learning: In bun's HTTP module, `bun.http.default_allocator` is the correct allocator to use for internal HTTP buffers like MutableString initialization for response_message_buffer and compressed_body. This allocator is initialized in HTTPThread.zig as `bun.http.default_arena.allocator()` and is used consistently across the HTTP module, separate from any local `default_allocator` variables.
Applied to files:
src/s3/credentials.zig
📚 Learning: 2025-09-07T14:00:36.526Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22258
File: src/bun.js/test/jest.zig:63-86
Timestamp: 2025-09-07T14:00:36.526Z
Learning: In Bun's Zig codebase, the established pattern for HashMap hash functions is to truncate hash values to u32, even when the underlying hash computation produces u64. This is seen throughout the codebase, such as in src/css/rules/rules.zig where u64 hashes are stored but truncated to u32 in HashMap contexts. When implementing HashContext for HashMapUnmanaged, the hash function should return u32 to match this pattern.
Applied to files:
src/s3/credentials.zig
🔇 Additional comments (15)
src/bun.js/webcore/fetch.zig (2)
1293-1310: Content-Encoding is now threaded into S3 uploadStream.This ensures stream-based S3 uploads carry the header when present.
1346-1361: Confirm Content-Encoding for non-stream S3 fetch signing.This block still only mixes
range/Content-Type. IfHeaders.getContentEncoding()should affect non-stream S3 uploads (Line 1346+), it may need to be merged here or passed into signing. Please confirm the intended behavior.src/s3/simple_request.zig (2)
347-380: content_encoding is correctly threaded into simple request signing.This keeps signed headers consistent with the new option.
387-401: Header buffer now scales with MAX_HEADERS.Good guard against overflow as signed header count grows.
src/bun.js/webcore/Blob.zig (5)
929-983: Empty-source S3 uploads now include Content-Encoding.Consistent with the new option propagation.
1099-1203: Content-Encoding is wired through all S3 upload branches.Good coverage for bytes, file, and stream paths.
1381-1473: Locked Response/Request stream uploads now carry Content-Encoding.Nice to see parity with other S3 upload flows.
2421-2453: pipeReadableStreamToBlob passes Content-Encoding correctly.Keeps S3 stream writes aligned with the new option.
2651-2720: getWriter validates and forwards contentEncoding.The type check plus optional propagation looks solid.
src/s3/client.zig (4)
235-261: upload now includes Content-Encoding in the signed request.Keeps simple uploads aligned with the new option.
263-346: writableStream now threads Content-Encoding into multipart setup.Correctly duplicates the value for async use.
447-527: uploadStream propagates Content-Encoding to multipart upload.Consistent with writableStream and simple uploads.
554-575: downloadStream header buffer sizing updated.This aligns with the expanded signed header set.
src/s3/credentials.zig (2)
381-416: No action required; buffer sizing andcontent_encodinghandling are correct.All call sites in
simple_request.zig,client.zig, andfetch.zigproperly allocateMAX_HEADERS + 1bytes for the header buffer, correctly accounting for the additional header appended bymixWithHeader. Thecontent_encodingfield is also properly allocated viabun.default_allocator.dupe()and freed in thedeinit()function usingbun.freeSensitive().
456-465: Content-Encoding threading through signing is correct and spec-compliant.The implementation properly includes content-encoding in SignedHeaders and CanonicalRequest when present, normalizes empty values to null, and emits it in SignResult headers with correct lowercasing. This aligns with AWS SigV4 specification, which recommends including optional headers like Content-Encoding in signed headers to prevent tampering.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…#26149) ## Summary Add support for setting the `Content-Encoding` header in S3 `.write()` and `.writer()` calls, following the same pattern as `Content-Disposition`. This allows users to specify the encoding of uploaded content: ```typescript // With .write() await s3file.write("compressed data", { contentEncoding: "gzip" }); // With .writer() const writer = s3file.writer({ contentEncoding: "gzip" }); writer.write("compressed data"); await writer.end(); // With bucket.write() await bucket.write("key", data, { contentEncoding: "br" }); ``` ## Implementation - Extended `SignedHeaders.Key` from 6 bits to 7 bits (64→128 combinations) to accommodate the new header - Added `content_encoding` to `S3CredentialsWithOptions`, `SignOptions`, and `SignResult` structs - Updated `CanonicalRequest` format strings to include `content-encoding` in AWS SigV4 signing - Added `getContentEncoding()` method to `Headers` for fetch-based S3 uploads - Expanded `_headers` array from 9 to 10 elements - Pass `content_encoding` through all S3 upload paths (upload, uploadStream, writableStream) ## Test plan - Added tests for "should be able to set content-encoding" - Added tests for "should be able to set content-encoding in writer" - Tests verify the Content-Encoding header is properly set on uploaded objects via presigned URL fetch - All 4 new tests pass with `bun bd test` and fail with `USE_SYSTEM_BUN=1` (confirming the feature is new) ## Changelog > Describe your changes in 1-2 sentences. These will be featured on [bun.sh/blog](https://bun.sh/blog) and Bun's release notes. Added `contentEncoding` option to S3 `.write()` and `.writer()` methods, allowing users to set the `Content-Encoding` header when uploading objects. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
#27329) ## Summary - Adds the missing `contentEncoding` property to the `S3Options` TypeScript type definition in `packages/bun-types/s3.d.ts` - The runtime support for `contentEncoding` was added in PR #26149 but the type definitions were not updated, causing TypeScript errors and missing IDE autocompletion Closes #27328 ## Test plan - [x] Verify `contentEncoding` appears in `S3Options` interface alongside `contentDisposition` - [x] Existing runtime tests in `test/js/bun/s3/s3.test.ts` already cover the runtime behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Bot <claude-bot@bun.sh> Co-authored-by: Claude <noreply@anthropic.com>
3 participants
Summary
Add support for setting the
Content-Encodingheader in S3.write()and.writer()calls, following the same pattern asContent-Disposition.This allows users to specify the encoding of uploaded content:
Implementation
SignedHeaders.Keyfrom 6 bits to 7 bits (64→128 combinations) to accommodate the new headercontent_encodingtoS3CredentialsWithOptions,SignOptions, andSignResultstructsCanonicalRequestformat strings to includecontent-encodingin AWS SigV4 signinggetContentEncoding()method toHeadersfor fetch-based S3 uploads_headersarray from 9 to 10 elementscontent_encodingthrough all S3 upload paths (upload, uploadStream, writableStream)Test plan
bun bd testand fail withUSE_SYSTEM_BUN=1(confirming the feature is new)Changelog
Added
contentEncodingoption to S3.write()and.writer()methods, allowing users to set theContent-Encodingheader when uploading objects.🤖 Generated with Claude Code