Skip to content

feat(service): Implement MultipartUploadBackend on InMemory and LocalFs#456

Open
lcian wants to merge 2 commits intolcian/test/gcs-multipartfrom
lcian/feat/multipart-upload-other-backends
Open

feat(service): Implement MultipartUploadBackend on InMemory and LocalFs#456
lcian wants to merge 2 commits intolcian/test/gcs-multipartfrom
lcian/feat/multipart-upload-other-backends

Conversation

@lcian
Copy link
Copy Markdown
Member

@lcian lcian commented May 4, 2026

⚠️ Stacked on #454

Implements MultipartUploadBackend on InMemoryBackend and LocalFsBackend, used for tests.
TieredStorage will come in the next PR.

InMemoryBackend now stores some auxiliary data structures dedicated to multipart uploads, while LocalFs uses a __multipart__/ directory where to store metadata and parts for each ongoing upload.

Close FS-349

@lcian lcian changed the title feat(multipart): implement MultipartUploadBackend for all backends feat(service): Implement MultipartUploadBackend on InMemory and LocalFs May 4, 2026
@lcian lcian changed the base branch from main to lcian/test/gcs-multipart May 4, 2026 09:43
@lcian lcian marked this pull request as ready for review May 4, 2026 09:55
@lcian lcian requested a review from a team as a code owner May 4, 2026 09:55
Comment thread objectstore-service/src/backend/in_memory.rs Outdated
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 611d2c3. Configure here.

Comment thread objectstore-service/src/backend/in_memory.rs Outdated
…lidation

InMemoryBackend::complete_multipart was calling .remove() on the
multipart store before validating parts. If validation failed, the
upload was permanently destroyed and the client could not retry.

Changed to .get() for validation, only removing after successful
assembly. Added retry-after-failure test coverage for both InMemory
and LocalFs backends.
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 5, 2026

Comment thread objectstore-service/src/backend/local_fs.rs
Comment thread objectstore-service/src/backend/in_memory.rs
}

#[async_trait::async_trait]
impl MultipartUploadBackend for LocalFsBackend {
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.

i think this impl will be pretty susceptible to 5XXs. operations don't acquire any sort of lock on the upload dir so a concurrent abort/complete request can remove the upload dir in the middle of other concurrent handlers. if/when we want this backend to be production-worthy we probably want to make sure races result in an appropriate 4XX

Copy link
Copy Markdown
Member Author

@lcian lcian May 7, 2026

Choose a reason for hiding this comment

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

Hmm yes I see what you mean. We need some sort of lock files. Filed as a follow-up we need to eventually do for self-hosted support: https://linear.app/getsentry/issue/PF-81/avoid-races-in-fs-backend

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