s3: evaluate all conditional write headers under the object lock (#12211)

* filer_pb,filer: per-mutation preconditions on ObjectTransaction

A transaction's request-level condition evaluates one entry before any
mutation runs, so a commit guarding two paths — the object it publishes
and the upload directory it assembles — could not be expressed. Give each
ObjectMutation an optional condition (evaluated against condition_key,
else the mutation's target, under the same lock just before the mutation
applies) and report which mutation failed through failed_mutation on the
response, so callers can tell a mutation guard's rejection apart from the
request-level precondition's.

* s3: evaluate multipart completion conditions under the object lock

CompleteMultipartUpload checked conditional headers gateway-side before
committing, and the routed commit transaction guarded only the upload
directory — nothing made If-None-Match: * atomic with the publish, so
concurrent conditional completions of the same absent key could all
report success.

The request's reducible write condition (If-None-Match/If-Match, star or
single strong ETag) now rides the commit as the transaction's request
condition on the owner filer: on the object path for unversioned buckets,
falling back to the .versions directory for suspended and enabled
versioning. The upload-exists guard moves onto the PUT mutation, keeping
its check-before-publish ordering. Conditions the reducer cannot express
stay on the distributed write lock, where check and commit already
serialize.

* filer,s3: resolve the latest version first for versioned conditional writes

* filer: keep directory entries holding content out of the absent-slot rule

* s3: keep a gateway-side upload-exists check for filers without mutation conditions

* filer: honor null-version-is-latest resolving prefer-alt conditions

* drop scratch multipart-race repro programs

* s3: note the write lock does not cover routed writes on non-reducible conditions

* filer: mirror the gateway's remote-etag precedence in condition evaluation

storedEntryETag only knew the extended attribute and the chunk fallback,
so a synced remote entry compared chunk-derived ETags while the gateway
compares the remote ETag. Follow the same precedence: extended attribute,
remote ETag while the entry still holds the remote bytes, Md5, then the
chunk hash — and no ETag for an entry holding none.

* s3: reduce all conditional write headers into owner-lock clauses

Time-based headers, combined conditions, ETag lists, and weak ETags stayed
on the distributed write lock because buildWriteCondition could not express
them, leaving their gateway check racing owner-routed writes. Reduce every
parseable header form to WriteCondition clauses — multiple clauses compose
AND, ETag candidates carry the raw list for the filer's canonicalization,
and RFC 7232 precedence keeps If-Unmodified-Since hidden by If-Match and
If-Modified-Since hidden by If-None-Match — so the owner filer evaluates
them under the same object-path lock as the commit. Only unparseable
headers still fall back, where the lock path reports the same parse error.
This commit is contained in:
Chris Lu authored and GitHub committed 2026-10-29 11:49:48 -07:00
1 parent 2737315e9d
commit 2dbf9c9108
7 files changed
+162 -56

No files matched your search

+73 -18
View File
@@ -72,7 +72,7 @@ func TestBuildWriteCondition(t *testing.T) {
t.Fatal("want ok")
}
c := oneClause(t, cond)
if c.Kind != filer_pb.WriteCondition_IF_ETAG_MATCH || len(c.Etags) != 1 || c.Etags[0] != "abc123" {
if c.Kind != filer_pb.WriteCondition_IF_ETAG_MATCH || len(c.Etags) != 1 || c.Etags[0] != `"abc123"` {
t.Fatalf("clause = %+v", c)
}
})
@@ -82,33 +82,84 @@ func TestBuildWriteCondition(t *testing.T) {
t.Fatal("want ok")
}
c := oneClause(t, cond)
if c.Kind != filer_pb.WriteCondition_IF_ETAG_NOT_MATCH || len(c.Etags) != 1 || c.Etags[0] != "abc123" {
if c.Kind != filer_pb.WriteCondition_IF_ETAG_NOT_MATCH || len(c.Etags) != 1 || c.Etags[0] != `"abc123"` {
t.Fatalf("clause = %+v", c)
}
})
t.Run("weak etag falls back", func(t *testing.T) {
if _, ok := buildWriteCondition(reqWith(map[string]string{s3_constants.IfMatch: `W/"abc"`})); ok {
t.Fatal("weak etag must not take the fast path")
t.Run("weak etag reduces verbatim", func(t *testing.T) {
cond, ok := buildWriteCondition(reqWith(map[string]string{s3_constants.IfMatch: `W/"abc"`}))
if !ok {
t.Fatal("want ok")
}
c := oneClause(t, cond)
if c.Kind != filer_pb.WriteCondition_IF_ETAG_MATCH || len(c.Etags) != 1 || c.Etags[0] != `W/"abc"` || c.AllowWeak {
t.Fatalf("clause = %+v", c)
}
})
t.Run("etag list falls back", func(t *testing.T) {
if _, ok := buildWriteCondition(reqWith(map[string]string{s3_constants.IfMatch: `"a","b"`})); ok {
t.Fatal("etag list must not take the fast path")
t.Run("etag list reduces", func(t *testing.T) {
cond, ok := buildWriteCondition(reqWith(map[string]string{s3_constants.IfMatch: `"a", "b"`}))
if !ok {
t.Fatal("want ok")
}
c := oneClause(t, cond)
if c.Kind != filer_pb.WriteCondition_IF_ETAG_MATCH || len(c.Etags) != 2 || c.Etags[0] != `"a"` || c.Etags[1] != `"b"` {
t.Fatalf("clause = %+v", c)
}
})
t.Run("both match and none-match falls back", func(t *testing.T) {
if _, ok := buildWriteCondition(reqWith(map[string]string{
t.Run("both match and none-match reduce to clauses", func(t *testing.T) {
cond, ok := buildWriteCondition(reqWith(map[string]string{
s3_constants.IfMatch: "*",
s3_constants.IfNoneMatch: "*",
})); ok {
t.Fatal("ambiguous combination must not take the fast path")
}))
if !ok {
t.Fatal("want ok")
}
if len(cond.Clauses) != 2 || cond.Clauses[0].Kind != filer_pb.WriteCondition_IF_EXISTS || cond.Clauses[1].Kind != filer_pb.WriteCondition_IF_NOT_EXISTS {
t.Fatalf("clauses = %+v", cond.Clauses)
}
})
t.Run("time-based falls back", func(t *testing.T) {
if _, ok := buildWriteCondition(reqWith(map[string]string{
t.Run("time-based reduces to a unix bound", func(t *testing.T) {
cond, ok := buildWriteCondition(reqWith(map[string]string{
"If-Unmodified-Since": "Wed, 21 Oct 2015 07:28:00 GMT",
}))
if !ok {
t.Fatal("want ok")
}
c := oneClause(t, cond)
want := time.Date(2015, 10, 21, 7, 28, 0, 0, time.UTC).Unix()
if c.Kind != filer_pb.WriteCondition_IF_UNMODIFIED_SINCE || c.UnixTime != want {
t.Fatalf("clause = %+v, want unix %d", c, want)
}
})
t.Run("If-Match hides If-Unmodified-Since", func(t *testing.T) {
cond, ok := buildWriteCondition(reqWith(map[string]string{
s3_constants.IfMatch: `"abc"`,
"If-Unmodified-Since": "Wed, 21 Oct 2015 07:28:00 GMT",
}))
if !ok {
t.Fatal("want ok")
}
if c := oneClause(t, cond); c.Kind != filer_pb.WriteCondition_IF_ETAG_MATCH {
t.Fatalf("clause = %+v", c)
}
})
t.Run("If-None-Match hides If-Modified-Since", func(t *testing.T) {
cond, ok := buildWriteCondition(reqWith(map[string]string{
s3_constants.IfNoneMatch: `"abc"`,
"If-Modified-Since": "Wed, 21 Oct 2015 07:28:00 GMT",
}))
if !ok {
t.Fatal("want ok")
}
if c := oneClause(t, cond); c.Kind != filer_pb.WriteCondition_IF_ETAG_NOT_MATCH {
t.Fatalf("clause = %+v", c)
}
})
t.Run("unparseable date falls back", func(t *testing.T) {
if _, ok := buildWriteCondition(reqWith(map[string]string{
"If-Modified-Since": "not a date",
})); ok {
t.Fatal("time condition must not take the fast path")
t.Fatal("unparseable headers must not reduce")
}
})
}
@@ -637,8 +688,12 @@ func TestRouteWriteCondition(t *testing.T) {
if _, ok := routeWriteCondition(reqWith(map[string]string{s3_constants.IfMatch: `"e"`}), true); ok {
t.Fatal("conditional unique write must not route")
}
// A non-reducible condition bails regardless.
if _, ok := routeWriteCondition(reqWith(map[string]string{s3_constants.IfMatch: `W/"e"`}), false); ok {
t.Fatal("weak etag must not route")
// A weak ETag still routes — the clause carries it verbatim.
if _, ok := routeWriteCondition(reqWith(map[string]string{s3_constants.IfMatch: `W/"e"`}), false); !ok {
t.Fatal("weak etag should route")
}
// An unparseable condition bails regardless.
if _, ok := routeWriteCondition(reqWith(map[string]string{"If-Modified-Since": "not a date"}), false); ok {
t.Fatal("unparseable headers must not route")
}
}