Skip to content

s3_store: size multipart parts at 64 MiB by default, not the 5 MiB floor - #58

Open
b7r6 wants to merge 1 commit into
mainfrom
pr/s3-multipart-64mib
Open

b7r6 wants to merge 1 commit into
mainfrom
pr/s3-multipart-64mib

Conversation

@b7r6

@b7r6 b7r6 commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

What and why

Lane A stability fix from the findings catalog (see nativelink-findings-2026-10-05.csv
on the meta/findings-catalog-2026-10-05 branch), rebased onto OSS main
@bf4b3aa855. This fork PR exists to validate the change on CI that is
representative of upstream PR lanes (public read-only cache, same endpoints)
before any upstream submission.

How was this verified?

The branch carries its own regression tests (fails-without where the fix
class allows it — see the branch's test commits). This PR's purpose is the
full representative CI run; green here rehearses the exact checks an
upstream PR would face.

Risk

Fork-internal validation PR; nothing lands anywhere by merging this. The
underlying fix's risk assessment happens at upstream-submission time.

AI assistance

Agent-assisted pipeline (Claude) for rebase and PR mechanics under my
direction; the fix itself carries verification in its tests.

@b7r6
b7r6 force-pushed the pr/s3-multipart-64mib branch 2 times, most recently from d2a56e1 to c5e9460 Compare October 7, 2026 20:40
The part-size math targeted MIN_MULTIPART_SIZE (5 MiB), so a large blob
was split into thousands of tiny parts — a 12 GiB blob became ~2300
concurrent 5 MiB PUTs. That churns HTTP connection pools (aws-smithy
"connection never set" storms); on a good link the retries win slowly,
on a lossy one they exhaust and the slow-store write fails, which
write-through then cascades into dropping the successful fast-store
write too.

Size parts at a configurable multipart_part_size (default 64 MiB),
growing only as needed to stay under the 10,000-part ceiling.
12 GiB -> ~192 parts. The same knob is wired into the ONTAP S3 store,
whose previous formula divided the object size by the 5 MiB floor — a
part count — and clamped that as a byte size, so every blob used 5 MiB
parts.

Larger parts mean more buffered bytes, so this also makes the buffering
bounds real, in both stores. The existing select loop drained the
bounded part channel into an unbounded FuturesUnordered, so the
documented per-upload bound was never enforced; the recv arm is now
gated on in-flight uploads, bounding one upload's buffer at roughly
(2 * multipart_max_concurrent_uploads + 1) * part_size. Across uploads,
a store-wide admission semaphore (max_concurrent_multipart_uploads,
default 4, whole-upload permits so it cannot deadlock the part loop)
bounds the aggregate near 5 GiB at the defaults.

The single-request threshold intentionally stays at 5 MiB: the
single-PUT path replays from a max_retry_buffer_per_request-sized
buffer (default 5 MiB), so raising the threshold would break retries
for mid-size uploads. Mid-size blobs pay two extra small requests.

The existing multipart mock tests pin multipart_part_size to the old
5 MiB so their part boundaries stay small (which also exercises the
knob); a new test locks the default to a single part for a blob
between 5 MiB and 64 MiB.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@b7r6
b7r6 force-pushed the pr/s3-multipart-64mib branch from c5e9460 to 002ffc3 Compare October 8, 2026 15:12
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.

1 participant