Skip to content

fix(recorder-core): slice progressive multipart uploads into uniform parts for Cloudflare R2 (#2275) - #2288

Open
massmarketconsumer-arch wants to merge 1 commit into
CapSoftware:mainfrom
massmarketconsumer-arch:fix/cloudflare-r2-uniform-multipart-parts
Open

fix(recorder-core): slice progressive multipart uploads into uniform parts for Cloudflare R2 (#2275)#2288
massmarketconsumer-arch wants to merge 1 commit into
CapSoftware:mainfrom
massmarketconsumer-arch:fix/cloudflare-r2-uniform-multipart-parts

Conversation

@massmarketconsumer-arch

@massmarketconsumer-arch massmarketconsumer-arch commented Sep 12, 2026

Copy link
Copy Markdown

Resolves #2275

Problem

When using Cloudflare R2 as the S3 storage backend, camera-only Instant Mode recordings fail during CompleteMultipartUpload with InvalidPart: All non-trailing parts must have the same length.
In packages/recorder-core/src/instant-mp4-uploader.ts, flushBuffer() previously dumped all buffered chunks into a single variable-sized Blob whenever this.bufferedBytes >= MIN_PART_SIZE_BYTES (5 MiB). Unlike AWS S3 and MinIO, Cloudflare R2 strictly enforces that all non-trailing parts in a multipart upload must be identical in size.

Solution

  • Refactored flushBuffer() to leverage takeBufferedPart(partSize) in a loop, ensuring each non-final part upload is sliced to exactly MIN_PART_SIZE_BYTES (5 MiB), matching the uniform chunking strategy already used for Google Drive resumable uploads.
  • Updated handleChunk to track combined in-flight and buffered bytes against MAX_PENDING_UPLOAD_BYTES to protect memory limits.
  • Added unit test in packages/recorder-core/__tests__/instant-recording-uploader.test.ts verifying that streamed buffers larger than 5 MiB are cleanly sliced into uniform 5 MiB non-trailing parts and a remainder trailing part.

Verification

  • bun x vitest run packages/recorder-core/__tests__/instant-recording-uploader.test.ts (18/18 passed)
  • bun x biome check packages/recorder-core/src/instant-mp4-uploader.ts packages/recorder-core/__tests__/instant-recording-uploader.test.ts (pass)

RetriggerConfidence Score: 4/5

The upload implementation appears behaviorally safe, but the explicit repository comment rule must be satisfied before merging.

Findings

  1. P2 Narrative comments violate guidance
  2. P2 Repeated slicing remains untested
Fix with agent prompt
### Issue 1
packages/recorder-core/__tests__/instant-recording-uploader.test.ts:1026
The comments above the chunk creation and final assertions only restate what the test code already expresses. Repository guidance prohibits comments that merely narrate code, so both comments must be removed before merging.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 2
packages/recorder-core/__tests__/instant-recording-uploader.test.ts:1027
This test uploads only 7 MiB, so the non-forced loop emits one full 5 MiB part before finalization handles the remainder. It does not exercise a second full-part iteration, allowing regressions in repeated slicing to pass unnoticed. Use a payload above 10 MiB and assert two uniform full parts plus the trailing remainder.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Uses the existing buffered-part slicing helper repeatedly for object-store uploads.
  • Preserves a smaller trailing part during finalization.
  • Extends the XMLHttpRequest mock to record uploaded Blob sizes.
  • The implementation is internally consistent, but the new test does not cover repeated full-part slicing and its narrative comments violate repository guidance.

Reviews (1) · Last reviewed commit: "fix(recorder-core): slice streamed chunk..."

sendProgressUpdate: vi.fn().mockResolvedValue(undefined),
});

// Push a chunk larger than MIN_PART_SIZE_BYTES

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Narrative comments violate guidance

The comments above the chunk creation and final assertions only restate what the test code already expresses. Repository guidance prohibits comments that merely narrate code, so both comments must be removed before merging.

Context Used: AGENTS.md (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/recorder-core/__tests__/instant-recording-uploader.test.ts
Line: 1026

Comment:
**Narrative comments violate guidance**

The comments above the chunk creation and final assertions only restate what the test code already expresses. Repository guidance prohibits comments that merely narrate code, so both comments must be removed before merging.

**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

});

// Push a chunk larger than MIN_PART_SIZE_BYTES
const chunk = makeBlob(TOTAL_SIZE, "video/webm;codecs=vp9,opus");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Repeated slicing remains untested

This test uploads only 7 MiB, so the non-forced loop emits one full 5 MiB part before finalization handles the remainder. It does not exercise a second full-part iteration, allowing regressions in repeated slicing to pass unnoticed. Use a payload above 10 MiB and assert two uniform full parts plus the trailing remainder.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/recorder-core/__tests__/instant-recording-uploader.test.ts
Line: 1027

Comment:
**Repeated slicing remains untested**

This test uploads only 7 MiB, so the non-forced loop emits one full 5 MiB part before finalization handles the remainder. It does not exercise a second full-part iteration, allowing regressions in repeated slicing to pass unnoticed. Use a payload above 10 MiB and assert two uniform full parts plus the trailing remainder.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Self-hosted + Cloudflare R2: camera-only Instant recordings fail with "InvalidPart: All non-trailing parts must have the same length"

1 participant