Skip to content

feat(resumable): Define and expose upload granularity - #652

Open
lcian wants to merge 14 commits into
feat/tiered-resumable-uploadsfrom
lcian/feat/resumable-upload-granularity
Open

lcian wants to merge 14 commits into
feat/tiered-resumable-uploadsfrom
lcian/feat/resumable-upload-granularity

Conversation

@lcian

@lcian lcian commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

⚠️ Stacked on #642

This introduces the concept of upload granularity for resumable uploads.
The only backend that has a granularity is effectively GCS with its documented limit of 256 KiB. Tiered inherits from long-term. The other backends get the default value of 0.

The Rust client learns the granularity from session creation responses, and rejects put_chunk/put_stream/put_read calls upfront when length < granularity and the chunk is not the last one, avoiding a round-trip to the server.

Note that a resumable upload handle reconstructed on the client from a persisted token keeps its granularity unknown and leaves chunk-size validation to the server which will return a generic 400.
I aim to address this in a follow-up where we will introduce an error code on our errors and parse it back on the client to give a more useful error.

Refs FS-514

Report upload granularity at session creation and in progress responses. Reject undersized non-final chunks in GCS and tiered storage, and let the Rust client preflight known chunk lengths.
@lcian lcian changed the title feat(resumable): Expose upload granularity feat(resumable): Define and expose upoad granularity Sep 25, 2026
@lcian lcian changed the title feat(resumable): Define and expose upoad granularity feat(resumable): Define and expose upload granularity Sep 25, 2026
@linear-code

This comment has been minimized.

@codecov

codecov Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.48276% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.28%. Comparing base (35d2d8e) to head (5a78d7b).

Files with missing lines Patch % Lines
clients/rust/src/resumable.rs 67.74% 10 Missing ⚠️
objectstore-service/src/error.rs 0.00% 5 Missing ⚠️
objectstore-service/src/backend/testing.rs 50.00% 3 Missing ⚠️
Additional details and impacted files
@@                        Coverage Diff                        @@
##           feat/tiered-resumable-uploads     #652      +/-   ##
=================================================================
- Coverage                          91.32%   91.28%   -0.04%     
=================================================================
  Files                                116      116              
  Lines                              23625    23725     +100     
=================================================================
+ Hits                               21575    21657      +82     
- Misses                              2050     2068      +18     
Components Coverage Δ
Rust Backend 94.82% <90.58%> (-0.03%) ⬇️
Rust Client 81.68% <67.74%> (-0.28%) ⬇️
Python Client 93.75% <ø> (ø)

☔ View full report in Codecov by Harness.
📢 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.

@lcian

This comment has been minimized.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 5b08a22. Configure here.

Keep creation-time granularity on new client handles and leave reconstructed handles without it. Remove progress-response granularity metadata and inline the client chunk-length check.
Comment thread objectstore-server/tests/resumable.rs
Comment thread objectstore-server/tests/resumable.rs
Comment thread objectstore-server/tests/resumable.rs Outdated
Comment thread objectstore-server/tests/resumable.rs
@lcian
lcian marked this pull request as ready for review September 25, 2026 10:16
@lcian
lcian requested a review from a team as a code owner September 25, 2026 10:16
Comment thread clients/rust/tests/e2e.rs Outdated
Comment thread objectstore-types/src/resumable.rs
Comment thread clients/rust/src/error.rs
#[error("resumable upload session is not available")]
ResumableUploadUnavailable,
/// A non-final chunk is shorter than the upload granularity.
#[cfg(feature = "resumable-upload-api")]

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.

I'm noticing that our error type isn't marked as non_exhaustive, yet we add or remove variants with feature flags, which seems disruptive. By adding a feature flag, existing code may stop to compile.

When this stabilizes, I think we should keep those un-flagged and revisit our idea of hiding the function behind a feature flag permanently. It seems that it's better to just have the code in always, and instead:

  • either use an extension trait on session to make opt-in to the low-level methods explicit
  • or simply keep calling them out as low-level in the docs and that's it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. The ultimate cause of this though is that this enum is not marked non_exhaustive so whenever we introduce an error kind it will be a breaking change.
Should we consider marking this as non_exhaustive or would you rather do so when we know all the APIs are relatively stable?

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.

2 participants