blobstore: Add the conformance test for large file upload with 16mb by LihaoLiuXs · Pull Request #465 · salesforce/multicloudj

LihaoLiuXs · 2026-06-02T16:45:08Z

Summary

< Provide a brief description of the changes in this PR >

Some conventions to follow

add the module name as a prefix
- for example: add a prefix: docstore: for document store module, blobstore for Blob Store module
for a test only PR, add test:
for a perf improvement only PR, add perf:
for a refactoring only PR, add "refactor:"

codecov-commenter · 2026-06-02T16:54:59Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.58%. Comparing base (ba67974) to head (5c0a9ee).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #465      +/-   ##
============================================
- Coverage     81.67%   81.58%   -0.10%     
  Complexity      652      652              
============================================
  Files           204      204              
  Lines         13477    13523      +46     
  Branches       1774     1791      +17     
============================================
+ Hits          11008    11033      +25     
- Misses         1686     1700      +14     
- Partials        783      790       +7

Flag	Coverage Δ
unittests	`81.58% <ø> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sandeepvinayak · 2026-06-02T18:35:34Z

We shouldn't commit large files. It should use regex pattern similar to other files.

Removed the large 16mb file from commit.

… binaries Per Sandeep's review: switch from random-looking deterministic content (byte = i % 251) to a single repeating byte ('A'), matching the convention used by testMultipartUpload_multipleParts. This makes the recorded WireMock fixtures compress trivially in git pack: - Replace bodyFileName + 16 MiB binary in __files/ with inline body strings in the mapping JSON. Two binary files removed. - AWS GET-1: inline 16 MiB of 'A's as response.body, drop the x-amz-transfer-encoding=append-md5 header (no MD5 trailer in fixture), recompute ETag. - GCP GET-2: inline 16 MiB of 'A's as response.body. - GCP PUT-4 and PUT-5 retain WireMock-recorded bodyPatterns (regex prefix) and scenario-state ordering for the resumable-upload chunk → finalize sequence. - AWS PUT-2: bodyPatterns omitted because WireMock records binary AWS upload bodies as base64 and cannot reverse-encode them in replay matching. This is consistent with how AWS test fixtures handle large binary uploads. On-disk fixture footprint is ~32 MB (mostly the inlined 16 MiB 'A' bodies), but git-pack compressed size is ~36 KB total. Both AWS and GCP replay verified passing.

Per Sandeep's review — avoid committing large files to git. Instead of inlining the full 16MB 'A'-filled response body in the WireMock mapping JSON, use a compact [GENERATED:A:16777216] marker that is expanded at serve-time by a new GeneratedResponseBodyTransformer. This reduces the AWS/GCP GET mapping files from 16MB to under 2KB each while preserving full byte-for-byte validation in replay mode.

sandeepvinayak · 2026-06-04T07:21:44Z

+   * {@code [GENERATED:A:16777216]}) will be expanded into a body filled with the specified
+   * character repeated {@code size} times.
+   */
+  public static class GeneratedResponseBodyTransformer extends ResponseDefinitionTransformer {


little confused on why we need new transformer ? our MPU tests already have >5MB blobs and regex are already getting used through transformer

The existing MPU regex in bodyPatterns handles request body matching (validates the upload payload in the PUT). This transformer handles response body generation on the GET/download side — for 16 MiB, storing the literal response body in the mapping JSON would blow up the repo, so the [GENERATED:A:16777216] marker keeps the mapping tiny and expands at serve-time. Different concern from the upload-path regex.

why do we care but response body > 16mb, this is the upload task

iamabhilaksh

A couple of things before this lands — none are blocking the test logic itself.

Scope creep in .gitignore: the .agents/ and .gus/ lines look unrelated to the 16MiB conformance test. Could you drop those into a separate PR (or just from this one)? Easier to revert the test-only change later if needed.

Re: Sandeep's transformer question — [Thinking out loud] I think the existing MPU regex in the WireMock setup matches the upload request path, while the new Authorization-stripping transformer is rewriting the GET response body on download. Different concerns, so I don't think they conflict — but worth a one-liner reply from you to confirm I'm reading it right.

SriramGuduri

Clean, well-documented conformance test that guards against the GCP large byte[] upload truncation regression. The GeneratedResponseBodyTransformer is a clever pattern to keep WireMock fixtures small in git while still exercising the full 16 MiB payload path. Approve — no blocking issues.

SriramGuduri · 2026-06-04T09:31:42Z

+    public ResponseDefinition transform(
+        Request request,
+        ResponseDefinition responseDefinition,
+        FileSource files,


nitpick: Integer.parseInt will throw NumberFormatException if the size value overflows int (>2^31-1). For a test utility this is fine since fixture content is developer-authored, but a descriptive error message might save debugging time vs. a raw NFE.

SriramGuduri · 2026-06-04T09:31:42Z

+        Request request,
+        ResponseDefinition responseDefinition,
+        FileSource files,
+        Parameters parameters) {


suggestion: Allocating a char[] of 16 MiB (32 MB on heap since char is 2 bytes) + converting to String (another copy) means ~64 MB of transient heap per response expansion. Consider using byte[] and withBody(byte[]) if WireMock supports it — halves the allocation. Not blocking since tests typically have generous heap, but worth noting for future larger payloads.

SriramGuduri · 2026-06-04T09:31:42Z

+          content.length,
+          downloaded.length,
+          testName + ": Content-Length did not match (truncation regression)");
+      Assertions.assertArrayEquals(


nitpick: try (outputStream) around a ByteArrayOutputStream is unnecessary since its close() is a no-op and toByteArray() is called after the block anyway. Removing the try-with-resources would simplify the code without changing behavior.

SriramGuduri · 2026-06-04T09:31:42Z

 .claude/*
 !.claude/skills/
+.agents/
+.gus/


nitpick: Adding .agents/ and .gus/ is unrelated to the blobstore test change. Consider splitting into a separate commit for cleaner history, or mention in PR description.

LihaoLiuXs · 2026-06-04T17:32:15Z

@sandeepvinayak Per your review feedback — the 16MB inline response bodies have been replaced with compact [GENERATED:A:16777216] markers (~900 bytes each) that are expanded at serve-time by a new GeneratedResponseBodyTransformer in TestsUtil.

Changes in this revision:

AWS get-1.json: 16MB → 902 bytes
GCP get-2.json: 16MB → 1,677 bytes
Added GeneratedResponseBodyTransformer that expands [GENERATED:<char>:<size>] markers during WireMock replay
No large files committed to git

The pattern follows the same spirit as the existing TruncateRequestBodyTransformer (regex prefix matching for request bodies) but for the response side.

- Use byte[] instead of char[]/String in GeneratedResponseBodyTransformer to halve heap allocation (~32MB vs ~64MB for 16MiB payloads) - Add descriptive error for size overflow in GENERATED body marker - Remove unnecessary try-with-resources around ByteArrayOutputStream - Remove unrelated .agents/ and .gus/ entries from .gitignore

sandeepvinayak · 2026-06-04T20:21:07Z

+  /**
+   * Regression test for the GCP large byte[] upload truncation bug. Uploads a 16 MiB byte array via
+   * {@link BucketClient#upload(UploadRequest, byte[])} with only the fields that callers like
+   * CoreStash use (key + contentLength + base64 CRC32C checksum), then downloads and verifies the
+   * full payload round-trips byte-for-byte. Before the fix, the GCP byte[] path used
+   * WriteChannel#write(ByteBuffer) which violates the resumable-upload chunking contract above
+   * ~6 MiB and silently truncated the object.
+   *
+   * <p>Scoped to AWS and GCP — the original bug was provider-specific to GCP's resumable-upload
+   * implementation, and AWS coverage guards the byte[] path on S3 against a similar regression.
+   * Alibaba is excluded because recording 16 MiB fixtures for every provider triples test-resource
+   * size with no additional bug coverage.
+   *
+   * <p>Fixture convention: payload is filled with a single repeating byte ({@code 'A'}) so
+   * that the recorded WireMock {@code bodyPatterns} compress trivially in git pack (matching
+   * the {@code testMultipartUpload_multipleParts} convention). Response body mappings use the
+   * {@code [GENERATED:A:16777216]} marker expanded at serve-time by
+   * {@code TestsUtil.GeneratedResponseBodyTransformer} to avoid committing large files. This
+   * keeps recorded fixtures small in the git object store while preserving full body-match
+   * validation in replay mode.
+   */


what's this paragraph, it was local AI not meant to retain ? :D Let's remove

iamabhilaksh · 2026-06-15T06:38:07Z

+      if (!matcher.matches()) {
+        return responseDefinition;
+      }
+      byte fillByte = (byte) matcher.group(1).charAt(0);


[Thinking out loud] The regex at L249 matches (.) — any single character — but the cast (byte) matcher.group(1).charAt(0) here silently truncates anything outside ASCII to its low 8 bits. For example [GENERATED:é:1024] would compile fine, regex-match fine, then quietly fill the body with (byte) 0xE9. Our only real caller is A, so this is purely theoretical today, but I'm wondering if we should either tighten the regex to [ -~] (printable ASCII) or note in the Javadoc that the marker is ASCII-only — keeps a future contributor from getting a confusing fixture mismatch. Happy either way. What's your take?

iamabhilaksh

Single non-blocking observation inline on the new [GENERATED:c:N] body transformer — the regex accepts any character but the byte cast is ASCII-only, so worth either tightening the regex or noting the ASCII contract in the Javadoc. Otherwise the post-review changes look clean.

Add the conformance test for large file upload with 16mb

62eb900

salesforce-cla Bot added the cla:signed label Jun 2, 2026

Merge branch 'main' into fix/gcp-large-upload-truncation2

0213903

sandeepvinayak reviewed Jun 2, 2026

View reviewed changes

sandeepvinayak requested changes Jun 2, 2026

View reviewed changes

LihaoLiuXs and others added 3 commits June 2, 2026 15:27

Merge branch 'main' into fix/gcp-large-upload-truncation2

ca0e51e

sandeepvinayak reviewed Jun 4, 2026

View reviewed changes

iamabhilaksh reviewed Jun 4, 2026

View reviewed changes

SriramGuduri approved these changes Jun 4, 2026

View reviewed changes

LihaoLiuXs requested a review from sandeepvinayak June 4, 2026 17:32

sandeepvinayak reviewed Jun 4, 2026

View reviewed changes

sandeepvinayak requested changes Jun 4, 2026

View reviewed changes

iamabhilaksh mentioned this pull request Jun 8, 2026

Fix/gcp large upload truncation #464

Open

iamabhilaksh reviewed Jun 15, 2026

View reviewed changes

Uh oh!

Conversation

LihaoLiuXs commented Jun 2, 2026

Summary

Some conventions to follow

Uh oh!

codecov-commenter commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamabhilaksh left a comment

Choose a reason for hiding this comment

Uh oh!

SriramGuduri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LihaoLiuXs commented Jun 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamabhilaksh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jun 2, 2026 •

edited

Loading