snapshot of https://github.com/seaweedfs/seaweedfs.git (master, 2026-09-16; blobs > 131072B omitted)

design-bucket-config-serialization.md

@@ -0,0 +1,167 @@
+# Design: Serializing Bucket Configuration Mutations
+
+Issue #9651 — concurrent `PutBucketVersioning` + `PutBucketEncryption` (as Terraform
+issues them in parallel) intermittently lose the encryption write.
+
+## Root cause
+
+The bucket's entire config lives in one filer entry, `/buckets/<name>`. Every
+config API does a read-modify-write of that single entry, and the writes are not
+serialized:
+
+- `updateBucketConfig(bucket, fn)` (`s3api_bucket_config.go:468`) — sources from a
+  possibly-stale cached `BucketConfig`, mutates `Entry.Extended`, writes the
+  **whole** entry. Used by: versioning, object-lock config, lifecycle, ACL/owner.
+- `UpdateBucketMetadata` → `setBucketMetadata` (`:1042`) — reads a fresh entry,
+  mutates `Entry.Content`, writes the **whole** entry. Used by: encryption, CORS,
+  tagging, ownership, policy, notification.
+
+Two ingredients produce the lost update:
+
+1. **No serialization** of the read→modify→write (the cache mutexes only guard the
+   in-memory map, not the RMW).
+2. **Whole-entry rewrite from an independent snapshot** — `updateBucketConfig`
+   rebuilds from a stale cached `BucketConfig` whose `Content` predates the
+   concurrent encryption write, so writing the whole entry reverts `Content`.
+
+Sequential calls always pass (each sees the previous write), so it only surfaces
+under concurrency — and CI's slower IO widens the window (the "2 of ~12 runs").
+
+## Goals
+
+- No lost updates across concurrent bucket-config changes — for **all** config
+  fields, not just versioning/encryption.
+- Correct for a single S3 gateway (the reported case) and for multiple gateways.
+- Reuse the filer primitives just merged (per-path lock, `WriteCondition`,
+  `ObjectTransaction`); do not reintroduce a distributed lock.
+- Minimal blast radius: the fix lands at the two chokepoint helpers.
+
+## Non-goals
+
+- Changing the one-entry-per-bucket storage model.
+- Multi-filer-concurrent bucket writes (addressed only as an optional phase 3).
+
+## The two ingredients map to two complementary fixes
+
+### Fix A — serialize + read fresh (closes the window for whole-entry writers)
+
+Both `updateBucketConfig` and `UpdateBucketMetadata` must run their RMW under one
+per-bucket critical section, and **re-read the entry fresh from the filer inside
+it** — not rebuild from the cached `BucketConfig`. The lock alone is insufficient:
+without the fresh read, two serialized writers still each apply a stale snapshot.
+
+### Fix B — field-level updates (removes the collision entirely)
+
+The two writers touch disjoint fields (`Extended[versioning]` vs `Content`). If
+each path updated only its own field instead of rewriting the whole entry, neither
+could clobber the other regardless of ordering. This is the structural fix and
+makes serialization a defense-in-depth concern rather than a correctness
+requirement for cross-field cases.
+
+## Where to serialize (layering)
+
+The bucket entry is a single filer entry, so unlike object writes there is no
+sharding — the question is purely the scope of the lock:
+
+| Layer | Serializes across | Cost | Notes |
+|---|---|---|---|
+| 1. Gateway-local per-bucket lock | one gateway process | tiny | fixes the reported (single-gateway/CI) case |
+| 2. Filer per-path lock via conditional write | all gateways on one filer | small | reuses #9640 `CreateEntry`+`WriteCondition` |
+| 3. Route-by-key to bucket-key owner filer | all gateways and filers | medium | same mechanism as the object DLM-removal |
+
+## Recommended plan (phased)
+
+### Phase 1 — minimal fix for #9651 (gateway-local lock + fresh read)
+
+Add a bounded per-bucket lock table to `S3ApiServer`, reusing the same
+`util.LockTable` the filer uses for its per-path lock:
+
+```go
+// in S3ApiServer
+bucketConfigLocks *util.LockTable[string] // serialize bucket-entry RMW
+
+func (s3a *S3ApiServer) withBucketConfigLock(bucket string, fn func() s3err.ErrorCode) s3err.ErrorCode {
+    lk := s3a.bucketConfigLocks.AcquireLock("bucketConfig", bucket, util.ExclusiveLock)
+    defer s3a.bucketConfigLocks.ReleaseLock(bucket, lk)
+    return fn()
+}
+```
+
+Wrap the RMW in **both** chokepoints, and inside the lock read the entry fresh:
+
+- `updateBucketConfig`: acquire the lock; re-read `/buckets/<name>` from the filer
+  (not the cache); rebuild `BucketConfig` from that fresh entry; apply `fn`; write;
+  invalidate cache; release.
+- `UpdateBucketMetadata`/`setBucketMetadata`: same lock key; it already reads fresh,
+  so it just needs to share the critical section.
+
+Both must use the **same** lock keyed on `bucket`, so versioning and encryption
+contend on one mutex. This closes the reported window. Limitation: only one
+gateway; two gateways behind a load balancer still race.
+
+Test: parallel `PutBucketVersioning` + `PutBucketEncryption`, assert both persist
+(the exact Terraform scenario), plus an N-way parallel variant over distinct
+fields.
+
+### Phase 2 — robust across gateways (field-level + CAS via merged primitives)
+
+Move the writers off whole-entry rewrites:
+
+- **Extended-based config** (versioning, object-lock, ownership, tagging-in-Extended)
+  → `ObjectTransaction` `PATCH_EXTENDED` on `/buckets/<name>`. The owner filer reads
+  the entry fresh under its per-path lock and merges only the named keys, so the
+  gateway never sends a whole-entry snapshot — this dissolves *both* ingredients for
+  these fields.
+- **`Content`-based config** (encryption, CORS, tags blob) — **chosen and
+  implemented (b3): extend `PATCH_EXTENDED` with `set_content`.** Under the same
+  per-path lock the filer reads the entry fresh, merges extended attributes, and
+  replaces `Content`, preserving the rest. So a content write becomes a field-level
+  patch too — `setBucketMetadata` patches `Content`, `updateBucketConfig` patches
+  extended keys, and the two serialize on the lock instead of racing whole-entry
+  rewrites. This is cleaner than the alternatives below: no client-side retry, no
+  storage migration, and it reuses `ObjectTransaction`'s existing atomic lock.
+  - (b1, rejected) Conditional `CreateEntry` overwrite with `IF_ETAG_MATCH` + retry
+    (#9640): correct but needs client-side retry, and the bucket directory entry has
+    no reliable ETag to compare on.
+  - (b2, future) Migrate each per-feature config out of the single `Content` blob
+    into its own `Extended` key. Then even *intra-blob* writes (tags vs encryption)
+    stop racing. Larger migration; tracked separately.
+
+Once all paths are field-level patches, the phase-1 gateway lock is unnecessary —
+the filer enforces atomicity. (This is the path taken: phase 1 was skipped.)
+
+### Phase 3 — multi-filer (only if needed)
+
+If multiple filers can write `/buckets/<name>` concurrently, a filer-local per-path
+lock no longer suffices. Route bucket-config writes to
+`PrimaryForKey("/buckets/<name>")` (the lock-ring view) and serialize on that one
+owner filer — the same route-by-key design used to take object writes off the DLM.
+Overkill for rare config writes; include only if multi-filer bucket writes are real.
+
+## Correctness summary
+
+- Phase 1: all RMW for a bucket serialize within a gateway; the fresh read means the
+  second writer observes the first's change. Closes #9651 for single-gateway.
+- Phase 2: `PATCH_EXTENDED` is atomic field-level merge at the filer (no snapshot);
+  CAS turns a concurrent `Content` write into a retry, enforced under the filer's
+  per-path lock — correct for any number of gateways sharing a filer.
+- Phase 3: one owner filer serializes all writers — correct across filers too.
+
+## Scope checklist (every path that RMWs the bucket entry)
+
+All of these funnel through the two chokepoints, so fixing the chokepoints covers
+them — but the fix must not leave any of them on an unserialized path:
+
+- via `updateBucketConfig`: versioning, object-lock config, lifecycle, ACL/owner.
+- via `UpdateBucketMetadata`/`setBucketMetadata`: encryption, CORS, tagging,
+  ownership controls, bucket policy, notification.
+- bucket create/delete (`CreateEntry`/`DeleteEntry` of `/buckets/<name>`) already
+  go through the filer's per-path lock on `CreateEntry`; ensure they take the same
+  bucket lock if they also patch config.
+
+## Cache rule (must document in code)
+
+Under the lock, **read the entry from the filer, never rebuild from the cached
+`BucketConfig`**. The cache is for reads; it must be invalidated on every write and
+never be the source for an RMW. This is the single most important detail — the lock
+without the fresh read does not fix the bug.