Skip to content

Make the GCS simple-upload threshold configurable - #2729

Open
benelser wants to merge 1 commit into
TraceMachina:mainfrom
benelser:pr/02-simple-upload-threshold
Open

Make the GCS simple-upload threshold configurable#2729
benelser wants to merge 1 commit into
TraceMachina:mainfrom
benelser:pr/02-simple-upload-threshold

Conversation

@benelser

@benelser benelser commented Sep 4, 2026

Copy link
Copy Markdown

What and why

GcsStore::update only takes the single-request simple-upload path
when the caller passes UploadSizeInfo::ExactSize below 5MB. Every
upload that arrives as UploadSizeInfo::MaxSize goes through a
resumable session: start_resumable_write (POST), one upload_chunk
(PUT) and an object_exists (GET) verification, each acquiring the
shared connection permit in turn. CompressionStore can only ever pass
MaxSize, because the compressed length is not known until the stream
has been consumed, so behind a compression store a 300-byte action
result costs three GCS requests and three trips through the permit
queue.

This adds simple_upload_threshold to ExperimentalGcsSpec, a sibling
of resumable_chunk_size. When set, a MaxSize(max) upload with max
strictly below the threshold is buffered through EOF (at most max
bytes) and written with one write_object through the existing
retrier. A stream that keeps producing bytes past its declared bound is
rejected with InvalidArgument before any request is issued. The
threshold is capped at MIN_MULTIPART_SIZE (5 MiB), so a bounded
unknown-size upload is never treated more aggressively than an
exact-size one.

The threshold applies to the declared bound, not the final object size.
CompressionStore computes its bound in whole blocks, so with the
default 64 KiB block even a tiny input declares more than 64 KiB; the
threshold has to be chosen against that bound.

Unset is the default and keeps the current behavior exactly: MaxSize
always resumable, ExactSize below 5MB simple as before, zero-digest
fast path untouched.

The GCS how-to page documents the option under "Options worth setting"
with an opt-in example. gen:config-reference was run and produced no
diff: the generated reference renders ExperimentalCloudObjectSpec as
a stub, so provider fields only surface through the schema. The field
is allowlisted in lint-snippets.mjs next to sas_url, which exists
for the same reason.

Measured on a NativeLink v1.6.3 deployment (GCS slow tier behind a
compression store, Redis fast tier) under a warm-cache lookup storm:
the resumable round trips for small writes helped back up the per-pod
permit FIFO past the CAS server's 30s per-blob deadline. Together with
two other changes, enabling this took GetActionResult p99 from 30.0s
to under 1.2s and cut GCS request volume by roughly 6x.

How was this verified?

Fifteen tests added to nativelink-store/tests/gcs_store_test.rs. The
direct tests drive update() through a producer/consumer harness that
runs both halves concurrently and returns the store's error ahead of
the producer's, so a reader closed early cannot mask the cause. The
composition tests wrap the mock-backed GcsStore in a real
CompressionStore with the default 64 KiB block, so the declared bound
is the one compression actually computes:

  • default config, MaxSize(300): zero write_object, one
    start_resumable_write, one upload_chunk, one object_exists
  • threshold set, MaxSize(300): exactly one write_object, zero
    resumable calls, content round-trips through get_part
  • threshold set, stream shorter than the bound: one write_object of
    the actual length
  • threshold set, stream longer than the bound in 1-byte chunks:
    InvalidArgument and zero requests recorded by the mock
  • threshold set, overflow delivered as one chunk larger than the
    bound: InvalidArgument and zero requests
  • bound equal to the threshold: resumable (strict less-than)
  • threshold set, MaxSize(6MiB): resumable with several chunks
  • threshold configured above 5MB, MaxSize(6MiB): still resumable
    (cap)
  • compression over GCS, threshold 64 KiB: a 300-byte input stays
    resumable, because its declared bound exceeds one block
  • compression over GCS, threshold 5 MiB: exactly one write_object
    and the content round-trips through decompression
  • transient Unavailable on the first write_object: the retry
    replays identical bytes, only the replay reaches the backend
  • sender dropped without EOF: the store's Internal error surfaces
    and no request is issued
  • empty stream with MaxSize(300): one empty write_object, has
    reports size 0
  • empty stream with MaxSize(0) and a non-zero-digest key: one empty
    write_object, no resumable session
  • one byte on a MaxSize(0) stream: InvalidArgument, zero requests

With the gcs_store.rs change reverted and the tests kept, seven tests
fail (the five threshold-on routing tests, the compression single-write
test and the transient-failure test) and the other eight pass. The
build-schema binary of nativelink-config (feature dev-schema)
emits the new field as an optional uint64. lint:anchors,
lint:snippets and lint:links pass on the docs change.

Risk

Default unset means no behavior change for existing configurations.
When enabled, the only new failure mode is a stream that violates its
declared MaxSize bound, which now fails with InvalidArgument before
contacting GCS instead of being accepted by the resumable path; no
in-tree caller produces such a stream.

Memory is not hard-capped at 5 MiB per upload. The accepted payload is
capped below 5 MiB, but content.to_vec() makes a second full copy
before write_object acquires its connection permit, and the reader
can retain the original chunks in its retry buffer. Uploads waiting for
a permit therefore hold memory too, and the connection count does not
bound the number of buffered uploads. This copy already exists on the
exact-size simple path; what this change does is route compressed
MaxSize traffic onto it. Measure RSS with concurrent uploads waiting
for permits before enabling it broadly or making it the default.
ExactSize uploads and the zero-digest path are untouched.


This change is Reviewable

@vercel

vercel Bot commented Sep 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
nativelink Ready Ready Preview Sep 6, 2026 12:27am UTC
nativelink-aidm Ready Ready Preview Sep 6, 2026 12:27am UTC

Request Review

@CLAassistant

CLAassistant commented Sep 4, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The bounded single-request path is a useful optimization. Before merging, please complete the user-facing documentation and add coverage for the compression composition and failure paths below. This review is based on source inspection and the existing CI results; I have not run new benchmarks.

For expansion, I would keep this PR focused on GCS and follow with shared bounded-buffering logic for S3 (which also covers the R2 and OCI adapters), ONTAP S3, and Azure. Preserve backend-specific checksums, retries, and upload limits. MongoDB already buffers before an upsert, so it does not have the same multipart request-count opportunity.

Comment thread nativelink-config/src/stores.rs Outdated
Comment on lines +1267 to +1276
/// Uploads whose exact size is unknown but whose declared upper bound is
/// below this threshold are buffered in memory and written with one
/// single-request upload instead of a resumable session (one `POST` to
/// open the session, one `PUT` per chunk and one `GET` to verify).
/// Values above 5MB are clamped to 5MB, the same bound that uploads of
/// known exact size already use for the single-request path.
/// When unset, uploads of unknown exact size always use a resumable
/// session, regardless of their declared upper bound.
///
/// Default: unset (unknown-size uploads always use a resumable session)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please make the configuration semantics explicit in this source-of-truth comment, then regenerate the config reference with gen:config-reference rather than editing generated pages. Suggested replacement:

Suggested change
/// Uploads whose exact size is unknown but whose declared upper bound is
/// below this threshold are buffered in memory and written with one
/// single-request upload instead of a resumable session (one `POST` to
/// open the session, one `PUT` per chunk and one `GET` to verify).
/// Values above 5MB are clamped to 5MB, the same bound that uploads of
/// known exact size already use for the single-request path.
/// When unset, uploads of unknown exact size always use a resumable
/// session, regardless of their declared upper bound.
///
/// Default: unset (unknown-size uploads always use a resumable session)
/// Uploads with an unknown exact size use a single-request upload when
/// their declared upper bound is strictly below this threshold. The store
/// buffers the stream through EOF and uploads its actual length. A stream
/// exceeding its declared bound is rejected before an upload request.
///
/// The effective threshold is capped at 5 MiB (5,242,880 bytes). A bound
/// equal to the threshold still uses a resumable upload. Exact-size
/// uploads retain their existing behavior regardless of this setting.
///
/// This threshold applies to the declared bound, not the final object
/// size. Compression can produce a bound larger than the stored object.
/// Buffering occurs before acquiring a connection permit; this setting
/// does not bound aggregate upload memory or account for request copies.
///
/// Default: unset. Unset or zero leaves unknown-size uploads resumable.

Also update web/apps/docs/content/docs/how-to/stores/gcs.mdx, under Options worth setting. Its current “everything else uses a resumable upload” statement becomes incomplete once this option is enabled. Add a table row and a short opt-in example inside the existing GCS provider config, such as simple_upload_threshold: 5242880 (5 MiB), then explain:

With simple_upload_threshold set, an upload whose exact size is unknown can also use one request when its declared maximum is strictly below the effective threshold. Compression supplies a conservative maximum, so choose the threshold using that bound rather than the final compressed size. With the default 64 KiB compression blocks, even a tiny object has a declared maximum greater than 64 KiB. Uploads buffer before waiting for a connection, so monitor memory as well as request counts when enabling this option.

Link the generated field reference for the remaining details, retain the page's explicit anchors/source links, and run the docs snippet and anchor checks. The new field and backend guide are required follow-ups under AGENTS.md.


/// Drives `update()` with `upload_size`, sending `data` in `chunk_size` pieces
/// so the store cannot infer the total length from the first chunk.
async fn run_update_in_chunks(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a test of the motivating CompressionStore -> GcsStore composition. These direct MaxSize(300) tests exercise the routing well, but compression's UploadState::new calculates its bound using whole blocks: with the default 64 KiB block, a tiny input's bound is already greater than 64 KiB. A regression test should use the real bound, assert that a threshold below it stays resumable, and that a threshold above it produces one write and round-trips through decompression.

Please also cover the newly enabled path with a transient write failure followed by successful replay of identical bytes, sender termination without EOF (no upload request), and overflow delivered in one chunk larger than the bound. Empty MaxSize streams, including MaxSize(0) with a non-zero-digest key, would round out the boundary cases. Use a producer/consumer harness that preserves the store error when the producer sees the reader close early.

_ => None,
};
if let Some(bound) = simple_upload_bound {
let content = reader.consume(Some(usize::try_from(bound)?)).await?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please qualify the PR description's claim that memory per in-flight upload is bounded by 5 MB. The accepted payload is capped below 5 MiB, but content.to_vec() allocates another full copy before GcsClient::write_object acquires its connection permit; the reader can also retain the original chunks in its retry buffer. Consequently, queued uploads consume memory too, and the connection count is not a bound on the number of buffered uploads.

The existing exact-size path already has this copy, so this is not a newly introduced allocation bug. However, this PR brings compressed MaxSize traffic onto that path. Please document the distinction and avoid claiming a hard 5 MiB memory ceiling. Before making this the default or extending it broadly, measure RSS with concurrent uploads waiting for permits; a replayable Bytes body and a separate buffering budget are possible follow-ups.

## What and why

`GcsStore::update` only takes the single-request simple-upload path
when the caller passes `UploadSizeInfo::ExactSize` below 5MB. Every
upload that arrives as `UploadSizeInfo::MaxSize` goes through a
resumable session: `start_resumable_write` (POST), one `upload_chunk`
(PUT) and an `object_exists` (GET) verification, each acquiring the
shared connection permit in turn. `CompressionStore` can only ever pass
`MaxSize`, because the compressed length is not known until the stream
has been consumed, so behind a compression store a 300-byte action
result costs three GCS requests and three trips through the permit
queue.

This adds `simple_upload_threshold` to `ExperimentalGcsSpec`, a sibling
of `resumable_chunk_size`. When set, a `MaxSize(max)` upload with `max`
strictly below the threshold is buffered through EOF (at most `max`
bytes) and written with one `write_object` through the existing
retrier. A stream that keeps producing bytes past its declared bound is
rejected with `InvalidArgument` before any request is issued. The
threshold is capped at `MIN_MULTIPART_SIZE` (5 MiB), so a bounded
unknown-size upload is never treated more aggressively than an
exact-size one.

The threshold applies to the declared bound, not the final object size.
`CompressionStore` computes its bound in whole blocks, so with the
default 64 KiB block even a tiny input declares more than 64 KiB; the
threshold has to be chosen against that bound.

Unset is the default and keeps the current behavior exactly: `MaxSize`
always resumable, `ExactSize` below 5MB simple as before, zero-digest
fast path untouched.

The GCS how-to page documents the option under "Options worth setting"
with an opt-in example. `gen:config-reference` was run and produced no
diff: the generated reference renders `ExperimentalCloudObjectSpec` as
a stub, so provider fields only surface through the schema. The field
is allowlisted in `lint-snippets.mjs` next to `sas_url`, which exists
for the same reason.

Measured on a NativeLink v1.6.3 deployment (GCS slow tier behind a
compression store, Redis fast tier) under a warm-cache lookup storm:
the resumable round trips for small writes helped back up the per-pod
permit FIFO past the CAS server's 30s per-blob deadline. Together with
two other changes, enabling this took GetActionResult p99 from 30.0s
to under 1.2s and cut GCS request volume by roughly 6x.

## How was this verified?

Fifteen tests added to `nativelink-store/tests/gcs_store_test.rs`. The
direct tests drive `update()` through a producer/consumer harness that
runs both halves concurrently and returns the store's error ahead of
the producer's, so a reader closed early cannot mask the cause. The
composition tests wrap the mock-backed `GcsStore` in a real
`CompressionStore` with the default 64 KiB block, so the declared bound
is the one compression actually computes:

- default config, `MaxSize(300)`: zero `write_object`, one
  `start_resumable_write`, one `upload_chunk`, one `object_exists`
- threshold set, `MaxSize(300)`: exactly one `write_object`, zero
  resumable calls, content round-trips through `get_part`
- threshold set, stream shorter than the bound: one `write_object` of
  the actual length
- threshold set, stream longer than the bound in 1-byte chunks:
  `InvalidArgument` and zero requests recorded by the mock
- threshold set, overflow delivered as one chunk larger than the
  bound: `InvalidArgument` and zero requests
- bound equal to the threshold: resumable (strict less-than)
- threshold set, `MaxSize(6MiB)`: resumable with several chunks
- threshold configured above 5MB, `MaxSize(6MiB)`: still resumable
  (cap)
- compression over GCS, threshold 64 KiB: a 300-byte input stays
  resumable, because its declared bound exceeds one block
- compression over GCS, threshold 5 MiB: exactly one `write_object`
  and the content round-trips through decompression
- transient `Unavailable` on the first `write_object`: the retry
  replays identical bytes, only the replay reaches the backend
- sender dropped without EOF: the store's `Internal` error surfaces
  and no request is issued
- empty stream with `MaxSize(300)`: one empty `write_object`, `has`
  reports size 0
- empty stream with `MaxSize(0)` and a non-zero-digest key: one empty
  `write_object`, no resumable session
- one byte on a `MaxSize(0)` stream: `InvalidArgument`, zero requests

With the `gcs_store.rs` change reverted and the tests kept, seven tests
fail (the five threshold-on routing tests, the compression single-write
test and the transient-failure test) and the other eight pass. The
`build-schema` binary of `nativelink-config` (feature `dev-schema`)
emits the new field as an optional `uint64`. `lint:anchors`,
`lint:snippets` and `lint:links` pass on the docs change.

## Risk

Default unset means no behavior change for existing configurations.
When enabled, the only new failure mode is a stream that violates its
declared `MaxSize` bound, which now fails with `InvalidArgument` before
contacting GCS instead of being accepted by the resumable path; no
in-tree caller produces such a stream.

Memory is not hard-capped at 5 MiB per upload. The accepted payload is
capped below 5 MiB, but `content.to_vec()` makes a second full copy
before `write_object` acquires its connection permit, and the reader
can retain the original chunks in its retry buffer. Uploads waiting for
a permit therefore hold memory too, and the connection count does not
bound the number of buffered uploads. This copy already exists on the
exact-size simple path; what this change does is route compressed
`MaxSize` traffic onto it. Measure RSS with concurrent uploads waiting
for permits before enabling it broadly or making it the default.
`ExactSize` uploads and the zero-digest path are untouched.
@benelser
benelser force-pushed the pr/02-simple-upload-threshold branch from 777d206 to cace162 Compare September 6, 2026 00:25
@benelser

benelser commented Sep 6, 2026

Copy link
Copy Markdown
Author

Pushed a revision against all three comments.

Doc comment: your text verbatim. gen:config-reference produced no diff, since the generated reference renders ExperimentalCloudObjectSpec as a stub and provider fields only surface through the schema. The GCS how-to has the table row, opt-in example and your paragraph; lint:anchors, lint:snippets and lint:links pass.

Tests: CompressionStore -> GcsStore with the real declared bound (64 KiB threshold stays resumable, 5 MiB writes once and round-trips), transient write failure with identical replay, sender dropped without EOF, single-chunk overflow, empty MaxSize streams including MaxSize(0). Harness returns the store error first.

Memory: the Risk section now describes the second copy and permit-queue buffering, no ceiling claimed.

Agreed on GCS-only here; S3, ONTAP and Azure as a follow-up.

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.

3 participants