@tus/s3-store: gracefully handle zero byte uploads#824
Conversation
🦋 Changeset detectedLatest commit: fc1b7df The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
itslenny
had a problem deploying
to
external-testing
GitHub Actions
Error
WalkthroughUploads a single empty S3 multipart part when no parts exist during multipart completion, accepts Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/s3-store/src/test/index.ts (1)
283-323: Test accurately reproduces the StreamLimiter zero-byte scenario.Good coverage — pipelining
Readable.from(Buffer.alloc(0))throughStreamLimiterbeforestore.writeis the exact flow that previously failed, and theContentLength === 0check on the resulting S3 object is a solid end-to-end assertion. Overlaps meaningfully with the existingshould successfully upload a zero byte filetest only in intent; the StreamLimiter path is what triggers the original bug, so keeping both is justified.Minor nit (optional): the
metadata: { contentType, cacheControl }on theUploadisn't asserted against the stored object (e.g.,headResult.ContentType/CacheControl). If the intent was to also verify those are preserved through the zero-byte completion path, consider adding assertions; otherwise the metadata block can be dropped to keep the test minimal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/s3-store/src/test/index.ts` around lines 283 - 323, The test currently includes metadata on the Upload but doesn't assert it's preserved; either remove the metadata block from the Upload creation to keep the test minimal, or add assertions after s3Client.getObject to verify headResult.ContentType and headResult.CacheControl match upload.metadata (check variables: Upload instance named upload, headResult from s3Client.getObject in the test 'should upload an empty part when completing zero byte multipart upload'); update the assertions accordingly and keep the existing ContentLength check.packages/s3-store/src/index.ts (1)
464-500: Zero-byte multipart handling looks correct.Uploading an empty part with
PartNumber: 1beforecompleteMultipartUploadis a valid workaround — S3 allows the last (and here only) part to be 0 bytes, and the SDK acceptsBuffer.alloc(0)as a Body. The call site inwriteguards this path by only invokingfinishMultipartUploadwhennewOffset === metadata.file.size, so an accidental empty completion on a non-zero upload isn't possible.One optional alternative worth considering: rather than uploading an empty part and completing the multipart, you could
abortMultipartUploadandputObjectwith an empty body + the originalContentType/CacheControl/metadata. That avoids the (harmless but slightly odd) artifact of a "multipart" object that was never really multipart, and keeps the completion path conceptually for actual data. Not a blocker — current approach is simpler and works.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/s3-store/src/index.ts` around lines 464 - 500, The current zero-byte multipart workaround in finishMultipartUpload uploads an empty part and completes the multipart which is valid; no mandatory change required, but if you prefer to avoid creating a "multipart" artifact implement the optional alternative: when parts.length === 0 call this.client.abortMultipartUpload({Bucket: this.bucket, Key: metadata.file.id, UploadId: metadata['upload-id']}) and then call this.client.putObject({Bucket: this.bucket, Key: metadata.file.id, Body: Buffer.alloc(0), ContentType: metadata.file.contentType, CacheControl: metadata.file.cacheControl, Metadata: metadata.file.metadata}) and return the putObject result Location instead of uploading an empty part; modify finishMultipartUpload accordingly and keep the existing completeMultipartUpload path for non-empty parts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/s3-store/src/index.ts`:
- Around line 464-500: The current zero-byte multipart workaround in
finishMultipartUpload uploads an empty part and completes the multipart which is
valid; no mandatory change required, but if you prefer to avoid creating a
"multipart" artifact implement the optional alternative: when parts.length === 0
call this.client.abortMultipartUpload({Bucket: this.bucket, Key:
metadata.file.id, UploadId: metadata['upload-id']}) and then call
this.client.putObject({Bucket: this.bucket, Key: metadata.file.id, Body:
Buffer.alloc(0), ContentType: metadata.file.contentType, CacheControl:
metadata.file.cacheControl, Metadata: metadata.file.metadata}) and return the
putObject result Location instead of uploading an empty part; modify
finishMultipartUpload accordingly and keep the existing completeMultipartUpload
path for non-empty parts.
In `@packages/s3-store/src/test/index.ts`:
- Around line 283-323: The test currently includes metadata on the Upload but
doesn't assert it's preserved; either remove the metadata block from the Upload
creation to keep the test minimal, or add assertions after s3Client.getObject to
verify headResult.ContentType and headResult.CacheControl match upload.metadata
(check variables: Upload instance named upload, headResult from
s3Client.getObject in the test 'should upload an empty part when completing zero
byte multipart upload'); update the assertions accordingly and keep the existing
ContentLength check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7aaa644a-66a9-4d4f-aea9-64985727878a
📒 Files selected for processing (2)
packages/s3-store/src/index.tspackages/s3-store/src/test/index.ts
itslenny
force-pushed
the
fix/handle-s3-upload-with-zero-bytes
branch
from
a4ecb04 to
cc8c7d1
Compare
itslenny
temporarily deployed
to
external-testing
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/s3-store/src/index.ts (1)
464-479: Fix targets the symptom rather than the root cause.Unconditionally uploading an empty part at finalize time works, but it adds an extra S3 round-trip to every completed upload whose final
retrievePartsresult happens to be empty, not only truly zero-byte uploads. More importantly, the underlying issue is thatuploadPartsnever emits achunkFinishedevent for an empty input stream (becauseStreamSplitter/StreamLimiterdon't produce a zero-byte chunk), sobytesUploadedreturns 0 and no part is ever uploaded duringwrite. Consider gating this on the known zero-byte case to avoid the extra request on non-empty uploads, e.g.:Proposed narrower guard
- if (parts.length === 0) { + if (parts.length === 0 && metadata.file.size === 0) { const uploadResult = await this.client.uploadPart({Alternatively, fix it upstream in
uploadPartsso an empty input stream still produces one (final) zero-byte part viauploadPart, keepingfinishMultipartUploadagnostic to this edge case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/s3-store/src/index.ts` around lines 464 - 479, The current finishMultipartUpload unconditionally uploads a zero-byte part when parts.length === 0, causing an extra S3 round-trip for uploads that merely had no parts returned by retrieveParts; instead, only add the synthetic empty part when the upload is known to be zero-bytes (e.g., metadata or metadata.file contains a size/length field equal to 0) or fix the root cause in uploadParts so an empty input stream produces one final zero-byte chunk: either (a) change finishMultipartUpload to gate the uploadPart call on a known-zero size in metadata (check metadata.file.size or metadata.size) and only then push the empty part, or (b) modify uploadParts / StreamSplitter / StreamLimiter so that an empty source still emits a chunkFinished/uploadPart for a zero-byte final part, ensuring bytesUploaded and write behavior are correct and finishMultipartUpload stays agnostic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/s3-store/src/index.ts`:
- Around line 464-479: The current finishMultipartUpload unconditionally uploads
a zero-byte part when parts.length === 0, causing an extra S3 round-trip for
uploads that merely had no parts returned by retrieveParts; instead, only add
the synthetic empty part when the upload is known to be zero-bytes (e.g.,
metadata or metadata.file contains a size/length field equal to 0) or fix the
root cause in uploadParts so an empty input stream produces one final zero-byte
chunk: either (a) change finishMultipartUpload to gate the uploadPart call on a
known-zero size in metadata (check metadata.file.size or metadata.size) and only
then push the empty part, or (b) modify uploadParts / StreamSplitter /
StreamLimiter so that an empty source still emits a chunkFinished/uploadPart for
a zero-byte final part, ensuring bytesUploaded and write behavior are correct
and finishMultipartUpload stays agnostic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9597e9c9-3a39-429f-830b-686554acb36f
📒 Files selected for processing (3)
.changeset/public-beds-sing.mdpackages/s3-store/src/index.tspackages/s3-store/src/test/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/s3-store/src/test/index.ts
| // Handle zero-byte uploads - S3 requires at least one part to complete a multipart upload | ||
| // S3 allows the last part to be 0 bytes, so we upload a single empty part | ||
| if (parts.length === 0) { | ||
| const uploadResult = await this.client.uploadPart({ |
There was a problem hiding this comment.
Can't you reuse the uploadPart method?
There was a problem hiding this comment.
Yeah, the only issue is that when I try to pass Readable.from(Buffer.alloc(0)) I get the error TypeError [ERR_HTTP_INVALID_HEADER_VALUE]: Invalid value "undefined" for header "x-amz-decoded-content-length"
I updated uploadPart to allow passing a Buffer directly and that works correctly.
itslenny
temporarily deployed
to
external-testing
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/s3-store/src/index.ts (1)
232-246: Consider narrowing theBodytype rather than wideningreadStream.Widening the parameter to
fs.ReadStream | Readable | Bufferworks, but the namereadStreamis now slightly misleading for theBuffercase, andfs.ReadStreamis already a subtype ofReadableso the union is a bit redundant. Optional cleanup:Proposed rename/simplification
protected async uploadPart( metadata: MetadataValue, - readStream: fs.ReadStream | Readable | Buffer, + body: Readable | Buffer, partNumber: number ): Promise<string> { const data = await this.client.uploadPart({ Bucket: this.bucket, Key: metadata.file.id, UploadId: metadata['upload-id'], PartNumber: partNumber, - Body: readStream, + Body: body, })Purely a readability nit — feel free to ignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/s3-store/src/index.ts` around lines 232 - 246, The parameter type and name in uploadPart should be simplified for clarity: replace the readStream: fs.ReadStream | Readable | Buffer union with a single sensible type (e.g., Readable | Buffer or just Readable if you always stream) and rename the parameter from readStream to body (or stream) to avoid misleading callers when passing a Buffer; update the uploadPart signature and its callers to accept the new type and name, keeping metadata, partNumber, and the client.uploadPart Body usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/s3-store/src/index.ts`:
- Around line 232-246: The parameter type and name in uploadPart should be
simplified for clarity: replace the readStream: fs.ReadStream | Readable |
Buffer union with a single sensible type (e.g., Readable | Buffer or just
Readable if you always stream) and rename the parameter from readStream to body
(or stream) to avoid misleading callers when passing a Buffer; update the
uploadPart signature and its callers to accept the new type and name, keeping
metadata, partNumber, and the client.uploadPart Body usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6e5e53e4-42ad-4798-b68e-87a2168377f8
📒 Files selected for processing (1)
packages/s3-store/src/index.ts
Murderlon
left a comment
Murderlon
left a comment
There was a problem hiding this comment.
Thanks for the PR, one more thing
| } | ||
| }) | ||
|
|
||
| it('should upload an empty part when completing zero byte multipart upload', async function () { |
There was a problem hiding this comment.
Since the existing test didn't work as expected, can you just add StreamLimiter to that test and remove this new one?
There was a problem hiding this comment.
Done. I also ran the test with / without my fix to ensure it still fails as expected.
3 participants
This PR resolves an issue with zero byte uploads on S3 or Minio.
Error message
MalformedXML: The XML you provided was not well-formed or did not validate against our published schemaInvalidRequest: You must specify at least one partReproduction
Any upload with zero bytes will reproduce this issue. This is a simple repro using
tus-js-clientAdditional context
Including the
StreamLimitertransform as is done here in BaseHandler causes zero byte chunks to not be emitted. This means thatchunkFinishedis never called and therefore no part is ever created. This results in the error as described above because you cannot complete a multipart upload that has no parts.The test suite already has a test for zero byte uploads, but it doesn't use
StreamLimiterso it succeeds. I've added a mostly identical test that includesStreamLimiterwhich consistently reproduces the issue.As a fix I simply check if there are zero parts and add an empty part before trying to finalize the upload.
This PR also adds the AWS_ENDPOINT env to tests to allow testing locally against Minio