Feature: conditional upload API by alsenz · Pull Request #178 · thanos-io/objstore

alsenz · 2025-04-21T13:53:11Z

Resolves #129

Adds support for conditional object uploads.

Extends ObjectAttributes with optional ObjectVersion (if supported), and Upload(...) with three new ObjectUploadOption, depending on provider support.

IfMatch: the object is written only if the existing object matches the provided version
IfNotMatch: the object is written only if the existing object does not match the provided version
IfNotExists: the object is written only if no object already exists for the provided key

Supports two forms of ObjectVersion:

Generational versions (i.e. increasing integers)
Etags

Not all providers support conditional write. The following providers are supported:

Filesystem: uses extended filesystem attributes. Only supported if the host system supports extended attributes.
InMem
GCS
Azure
S3: IfNotMatch is not yet supported (as it is not supported by AWS).

Clients can check conditional API by calling SupportedObjectUploadOptions on the Bucket interface.

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

API changes

Added 3 new ObjectUploadOptions: IfMatch, IfNotMatch and IfNotExists
1.1 Made ObjectUploadOption typed, repeating the existing IterOption pattern.
1.2 Added three new parameter modifiers- WithIfMatch, WithIfNotExists and WithIfNotMatch, and WithContentType to support the previous un-typed ObjectUploadOption
Added optional ObjectVersion field to ObjectAttributes
Added acceptance tests for IfMatch, IfNotMatch, IfNotExists and for Attributes API
Added Bucket provider implementations for:
1.1 filesystem: this now uses Extended Attributes, if supported by the host system
1.2 inmem
1.3 GCS
1.4 Azure
1.5 S3, excepting IfNoneMatch which is not fully supported (by AWS) yet
1.6 Wrappers

Backwards compatibility

API changes are additive and should be backwards compatible.

The Upload API changes use a trailing variadic parameter so should be backwards compatible
The ObjectAttributes add new field so should be backwards compatible
The SupportedObjectUploadOptions interface method potentially breaks existing Bucket implementations. Library users who implement their own providers will need to implement this method. Full or stub implementations are provided for all provider implementations in the libary.

Dependencies

Adds dependency on github.com/pkg/xattr, which is BSD-2-clause licensed.

Verification

Added acceptance tests for

IfMatch
IfNotMatch
IfExists
Versions on Attributes

Did not run integration tests on other object store providers, as I do not have access to test environments, but these should be unchanged.

…, inmem providers Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

alsenz · 2025-05-01T07:38:54Z

@bwplotka - would you be the appropriate maintainer to submit this for a first review?

Greatly appreciated.

yuchen-db · 2025-10-17T19:52:47Z

+	}
+}
+
+func tryOpenFile(name string, ifNotExists bool) (exists bool, err error) {


does it leak file descriptor?

Agreed - fixed.

yuchen-db · 2025-10-17T19:55:06Z

+}
+
+// ValidateUploadOptions ensures that only supported options are passed as options.
+func ValidateUploadOptions(supportedOptions []ObjectUploadOptionType, opts ...ObjectUploadOption) error {


maybe we should also validate IfNotExists and IfMatch/IfNotMatch aren't used together

Sounds sensible - done.

yuchen-db · 2025-10-17T19:57:57Z

 	b.mtx.Lock()
 	defer b.mtx.Unlock()
+
+	params := ApplyObjectUploadOptions(opts...)


shall we validate the options here?

Whoops - done!

anarcher · 2025-12-12T05:50:28Z

What's the progress on this PR? Any chance it gets merged?

Signed-off-by: Alsenz <7210552+alsenz@users.noreply.github.com>

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

alsenz · 2025-12-22T22:38:00Z

Thanks @yuchen-db for your review. I agree with all your points and have pushed some changes accordingly. This is an old PR but hopefully we can get it across the line.

alsenz · 2025-12-22T22:39:37Z

@anarcher this is an older PR that didn't get much attention, hopefully the maintainers will be able to find some time to review and accept. I have brought the feature branch back in line with main and responded to comments, so it should be ready to go.

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

RichardZhangRZ · 2026-04-01T16:40:12Z

Would like to see this PR merged soon :)

GiedriusS · 2026-04-08T14:34:21Z

I'll try to take a look no later than 2 months~ from now

GiedriusS · 2026-04-14T09:11:01Z

 		return ctx.Err()
 	}

 	file := filepath.Join(b.rootDir, name)


Why can't we create an extended attribute on the same file?

This was written a while ago, so apologies if I'm a little hazy, but I'm fairly use we're using the swap-and-move idiom to prevent race conditions when the file is created but the xattrs aren't written, which would could cause versioning assumptions to go wrong- this relies on atomic move of course but that's already ensured by SupportedObjectUploadOptions - we don't do this code path if we don't have xattrs and atomic moves.

To double check, I ran this through claude (which I try to avoid relying on but is a good double-check), it said:
▎ The xattr is set on the swap file before the atomic os.Rename. Since os.Rename on Unix is atomic and xattrs are stored as inode metadata, they travel with the file on rename. Setting the xattr on file after the
rename would introduce a race: another goroutine could call checkConditions in that window and see the file without its version data.

GiedriusS · 2026-04-14T09:12:07Z

 	return false
 }

+// IsConditionNotMetErr returns true if the response status code was Precondition Failed or Not Modified.


The same here: I suggest not adding into the comments what is already in the code.

Agreed - will fix in next commit

GiedriusS · 2026-04-14T09:13:31Z

+	if uploadOpts.IfNotExists {
+		putOpts.SetMatchETagExcept("*")
+	} else if uploadOpts.Condition != nil {
+		// If-None-Match with header values other than "*" is not supported by AWS yet.


Should we error out on this condition? How the user would know about this? The user could pass something and it would do something different than what is expected so I would expect an error here.

Absolutely - yes, good catch! Will fix in next commit.

GiedriusS · 2026-04-24T10:34:55Z

@alsenz ping

alsenz · 2026-04-28T15:26:33Z

@GiedriusS just seen this thanks for the review (been a while sorry).

I will resolve these tonight after work

Co-authored-by: Giedrius Statkevičius <giedriuswork@gmail.com> Signed-off-by: Alsenz <7210552+alsenz@users.noreply.github.com>

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

alsenz · 2026-04-29T20:56:11Z

@GiedriusS -- PR comments addressed - haven't resolved the comments yet until you take a look. my best.

GiedriusS

Thank you, a few more suggestions

GiedriusS · 2026-04-30T10:23:32Z

 	// Upload should be idempotent.
-	Upload(ctx context.Context, name string, r io.Reader, opts ...ObjectUploadOption) error
+	Upload(ctx context.Context, name string, r io.Reader, options ...ObjectUploadOption) error
+


Some random whitespace issues here.

Will resolve.

GiedriusS · 2026-04-30T10:26:14Z

+
+// ObjectUploadOption configures UploadObjectParams.
+type ObjectUploadOption struct {
+	Type  ObjectUploadOptionType


I don't like that these two are disjointed now and users can misuse this ie create some ObjectUploadOption that doesn't do what Type says it should do. Should we make Type and Apply hidden i.e. lowercase so that users wouldn't be able to do that?

No fair enough, although I think there may be other places where symbols are overexposed maybe. Either way, easy change to make and very happy to do.

GiedriusS · 2026-04-30T10:35:50Z

+}
+
+func tryOpenFile(name string, ifNotExists bool) (exists bool, err error) {
+	// First try to open the file with exclusive create, then truncate if permitted


Where truncation happens here? 🤔

No idea. I think it's a comment typo rather than code typo, re-readin gthe whole logic.

I'll just remove the comment fo rnow

GiedriusS · 2026-04-30T10:36:25Z

+		if _, err := io.Copy(writer, r); err != nil {
+			return errors.Wrapf(err, "copy to %s", swap)
+		}
+		// Write the checksum into an xattr


I suggest removing self-explanatory comments like this.

Will remove

GiedriusS · 2026-04-30T10:37:07Z

+		return err
+	}
+
+	if xattr.XATTR_SUPPORTED {


If this is false then where are we writing to swf?

This is a bug- thanks - think the fix is a few lines, will fix.

GiedriusS · 2026-04-30T10:38:03Z

+
+// ValidateUploadOptions ensures that only supported options are passed as options, and that options used simultaneously are valid.
+func ValidateUploadOptions(supportedOptions []ObjectUploadOptionType, opts ...ObjectUploadOption) error {
+


Typically we don't put a empty line just after the definition.

GiedriusS · 2026-04-30T10:38:13Z

 // Upload writes the file specified in src to into the memory.
-func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader, _ ...objstore.ObjectUploadOption) (err error) {
+func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader, opts ...objstore.ObjectUploadOption) (err error) {
+


:thumbs-up:

alsenz · 2026-05-04T16:13:42Z

Thanks for the second pass @GiedriusS - will apply changes tonight and push.

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

Fix some nits from the review. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

GiedriusS

Fixed a few nits from my review. Hope that's OK. Merging this. Thank you for your contribution! I will try to use it in the Thanos parquet converter and there might be a few more things that will need to be changed but overall I think it's good!

**What this PR does** Fixes the flakiness of `TestBlocksCleaner_RaceCondition_CleanerUpdatesBucketIndexWhileAnotherCleanerDeletesBlocks`, which sometimes fails with `upload already in progress for user-1/bucket-index.json.gz, retry, creating swap file failed: ...` ``` --- FAIL: TestBlocksCleaner_RaceCondition_CleanerUpdatesBucketIndexWhileAnotherCleanerDeletesBlocks (0.17s) --- FAIL: TestBlocksCleaner_RaceCondition_CleanerUpdatesBucketIndexWhileAnotherCleanerDeletesBlocks/the_2nd_cleaner_fetches_new_deletion_marks_after_the_1st_cleaner_has_started_deleting_blocks_for_the_new_deletion_marks (0.10s) blocks_cleaner_test.go:1730: Error Trace: /__w/mimir/mimir/pkg/compactor/blocks_cleaner_test.go:1730 /usr/local/go/src/runtime/asm_amd64.s:1771 Error: Received unexpected error: upload already in progress for user-1/bucket-index.json.gz, retry, creating swap file failed: open /tmp/TestBlocksCleaner_RaceCondition_CleanerUpdatesBucketIndexWhileAn1005483577/001/user-1/bucket-index.json.gz.swap: file exists upload bucket index github.com/grafana/mimir/pkg/storage/tsdb/bucketindex.WriteIndex /__w/mimir/mimir/pkg/storage/tsdb/bucketindex/storage.go:99 github.com/grafana/mimir/pkg/compactor.(*BlocksCleaner).cleanUser /__w/mimir/mimir/pkg/compactor/blocks_cleaner.go:517 github.com/grafana/mimir/pkg/compactor.TestBlocksCleaner_RaceCondition_CleanerUpdatesBucketIndexWhileAnotherCleanerDeletesBlocks.func3.2 /__w/mimir/mimir/pkg/compactor/blocks_cleaner_test.go:1730 runtime.goexit /usr/local/go/src/runtime/asm_amd64.s:1771 Test: TestBlocksCleaner_RaceCondition_CleanerUpdatesBucketIndexWhileAnotherCleanerDeletesBlocks/ ``` **Why it fails randomly** The thanos-io/objstore update in #15939 brought thanos-io/objstore#178, which adds support for conditional writes. The filesystem bucket implementation for it achieves this with temporary file locks (the `<name>.swap` file) that are held during an Upload, and can fail to be acquired. The catch is this mutual exclusion was introduced for all uploads, no matter if conditional or not. This makes it so concurrent Upload calls now fail. The test in question is exercising two cleaners concurrently working on the same tenant, and they sometimes happen to call Upload on the same object at the same time. **The fix** I'd expect the filesystem bucket to mimic S3 and friends by allowing concurrent writes with [last-writer-wins semantics](https://docs.aws.amazon.com/AmazonS3/latest/userguide/Welcome.html#Concurrent-applications). I looked into fixing this myself upstream, but it was not trivial, and every solution I thought of had its caveats. So I opted to just fix our test by ensuring `Upload` calls are serialized.

alsenz changed the title ~~Feature: conditions API~~ Feature: conditional upload API Apr 21, 2025

alsenz added 4 commits April 21, 2025 14:56

implemented conditional upload API for GCS, Azure, AWS and filesystem…

186f3f3

…, inmem providers Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

support non-xattr filesystems; add CHANGELOG

fe792ef

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

fix linter errors

e63bf4b

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

fixed changelog

1bb876b

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

alsenz force-pushed the feat_conditions_API_129 branch from c21b930 to 1bb876b Compare April 21, 2025 13:57

yuchen-db reviewed Oct 17, 2025

View reviewed changes

yuchen-db approved these changes Oct 17, 2025

View reviewed changes

alsenz added 2 commits December 22, 2025 21:43

Merge branch 'main' into feat_conditions_API_129

5d10115

Signed-off-by: Alsenz <7210552+alsenz@users.noreply.github.com>

fix: fixed metric regression tests merge

b395c1c

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

alsenz force-pushed the feat_conditions_API_129 branch from a5d77ae to b395c1c Compare December 22, 2025 22:15

fix: reviewer comments

e5c31be

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

alsenz added 2 commits December 22, 2025 22:40

fix: golint.

328eae5

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

fix: golint (again)

9ee1894

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

alsenz force-pushed the feat_conditions_API_129 branch from cbdcc69 to 9ee1894 Compare December 22, 2025 22:44

retrigger checks -- docs plugin 504 error

dd65867

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

GiedriusS reviewed Apr 14, 2026

View reviewed changes

alsenz and others added 2 commits April 28, 2026 16:39

Update providers/azure/azure.go

d369478

Co-authored-by: Giedrius Statkevičius <giedriuswork@gmail.com> Signed-off-by: Alsenz <7210552+alsenz@users.noreply.github.com>

fix: PR review comments

b0814c7

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

alsenz requested a review from GiedriusS April 29, 2026 20:55

GiedriusS reviewed Apr 30, 2026

View reviewed changes

fix: addressing PR comments

ee34e75

Signed-off-by: Tom Plowman <7210552+alsenz@users.noreply.github.com>

GiedriusS reviewed May 5, 2026

View reviewed changes

Comment thread tracing/opentracing/opentracing.go Outdated

Comment thread providers/filesystem/filesystem.go Outdated

Comment thread providers/filesystem/filesystem.go Outdated

Comment thread providers/filesystem/filesystem.go Outdated

*: fix various small nits

39eca79

Fix some nits from the review. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

GiedriusS approved these changes Jun 15, 2026

View reviewed changes

GiedriusS merged commit fb6fd3a into thanos-io:main Jun 15, 2026
7 of 8 checks passed

npazosmendez mentioned this pull request Jul 23, 2026

Fix TestBlocksCleaner_RaceCondition... flakiness grafana/mimir#16217

Merged


		// ValidateUploadOptions ensures that only supported options are passed as options, and that options used simultaneously are valid.
		func ValidateUploadOptions(supportedOptions []ObjectUploadOptionType, opts ...ObjectUploadOption) error {

Uh oh!

Conversation

alsenz commented Apr 21, 2025

Adds support for conditional object uploads.

Changes

API changes

Backwards compatibility

Dependencies

Verification

Uh oh!

alsenz commented May 1, 2025

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!

Choose a reason for hiding this comment

Uh oh!

anarcher commented Dec 12, 2025

Uh oh!

alsenz commented Dec 22, 2025

Uh oh!

alsenz commented Dec 22, 2025

Uh oh!

RichardZhangRZ commented Apr 1, 2026

Uh oh!

GiedriusS commented Apr 8, 2026

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

GiedriusS commented Apr 24, 2026

Uh oh!

alsenz commented Apr 28, 2026

Uh oh!

alsenz commented Apr 29, 2026

Uh oh!

GiedriusS 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!

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!

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!