fix(ecstore): never cache fabricated bucket metadata as authoritative (#5307)

BucketMetadataSys::get_config lazily fabricated a default BucketMetadata (object-lock off) for any bucket whose .metadata.bin was ConfigNotFound and cached it in the map that the map-only, fail-closed metadata_sys::get() serves. The object-lock batch-delete gate (object_lock_delete_check_required, backlog#929 / #4297) treats that map as authoritative, so a metadata miss became a cached "no lock" answer: a versioning peek could poison the cache and let delete_objects skip the per-object retention/legal-hold stat. The same fabrication raced make_bucket (lost update overwriting freshly persisted lock-enabled metadata) and let the 15-minute refresh loop replace good cached metadata on a transient quorum dip. Production changes: - get_config caches only metadata actually read from disk; misses are recorded in a bounded negative cache (30s TTL, 10k entries, invalidated by set()) so repeated lookups for metadata-less names cost no extra namespace-lock + erasure-set fanout (reachable pre-auth via CORS preflight and per-key in DeleteObjects). - concurrent_load never lets a fabricated default REPLACE an existing map entry; startup insert-if-vacant behavior for legacy buckets is preserved. - delete_objects and new_ns_lock resolve dist-erasure, versioning, and the object-lock gate from the set's own instance context (backlog#1052) instead of the ambient facade, so a second in-process instance (or, in tests, another test's transient DistErasure window) cannot reroute locking onto an empty dist locker list or answer with the wrong instance's bucket state. Test-isolation changes (the bug that surfaced all of the above: the delete_objects lock-gating test failed deterministically when sharing a process with the lifecycle env tests): - The MinIO-migration test builds on an isolated InstanceContext instead of registering soon-deleted disks in the shared bootstrap registry. - The cached lifecycle env re-registers its disks on every use, surviving other serial tests' reset_local_disk_test_state. - Hermetic SetDisks helpers gain isolated-context variants pinned to plain erasure; tier-free non-serial test modules use them, guard-based SetupTypeGuard tests stay on the bootstrap context. - Three deterministic pin tests (nextest-safe) cover the caching contract, the delete gate resolution source, and the ns-lock resolution source. Verification: - cargo test -p rustfs-ecstore --lib -- --exact <4-test combo from the report> (previously failing, now green) - cargo test -p rustfs-ecstore --lib: 3169 passed / 0 failed across repeated runs; cargo fmt --check and cargo clippy --lib --tests clean - Adversarial validation (high-risk tier, all seven roles) run per AGENTS.md; all findings fixed or rebutted with evidence
2026-08-25 13:36:50 +00:00 · 2026-07-27 00:53:48 +08:00
parent b2a376c2d2
commit 63e57378d6
12 changed files with 485 additions and 39 deletions
@@ -9344,8 +9344,27 @@ mod tests {

    static STALE_MULTIPART_TEST_ENV: OnceLock<(Vec<PathBuf>, Arc<ECStore>)> = OnceLock::new();

+    /// Re-register the cached environment's disks into its (shared bootstrap)
+    /// context registry. Other `#[serial]` tests reset or reshape that
+    /// registry (`reset_local_disk_test_state`, their own `init_local_disks`),
+    /// and the peer-sys bucket operations of this env resolve local disks
+    /// through it at call time — without this repair, a lifecycle test that
+    /// runs after such a test fails bucket creation on write quorum.
+    async fn reregister_env_local_disks(ecstore: &Arc<ECStore>) {
+        use crate::disk::DiskAPI as _;
+
+        let map = ecstore.ctx.local_disk_map();
+        let mut guard = map.write().await;
+        for disks in ecstore.disk_map.values() {
+            for disk in disks.iter().flatten() {
+                guard.insert(disk.endpoint().to_string(), Some(disk.clone()));
+            }
+        }
+    }
+
    async fn setup_test_env() -> (Vec<PathBuf>, Arc<ECStore>) {
        if let Some((paths, ecstore)) = STALE_MULTIPART_TEST_ENV.get() {
+            reregister_env_local_disks(ecstore).await;
            return (paths.clone(), ecstore.clone());
        }

@@ -649,7 +649,7 @@ impl BucketMetadata {
        Ok(())
    }

-    fn default_timestamps(&mut self) {
+    pub(crate) fn default_timestamps(&mut self) {
        if self.policy_config_updated_at == OffsetDateTime::UNIX_EPOCH {
            self.policy_config_updated_at = self.created
        }
@@ -1093,16 +1093,25 @@ pub async fn load_bucket_metadata(api: Arc<ECStore>, bucket: &str) -> Result<Buc
 }

 pub async fn load_bucket_metadata_parse(api: Arc<ECStore>, bucket: &str, parse: bool) -> Result<BucketMetadata> {
-    let mut bm = match read_bucket_metadata(api.clone(), bucket).await {
-        Ok(res) => res,
+    Ok(load_bucket_metadata_parse_with_presence(api, bucket, parse).await?.0)
+}
+
+/// The returned `bool` reports whether the metadata was actually read from
+/// persisted storage; `false` means no metadata exists for this bucket on this
+/// store and the returned value is a fabricated in-memory default.
+pub(crate) async fn load_bucket_metadata_parse_with_presence(
+    api: Arc<ECStore>,
+    bucket: &str,
+    parse: bool,
+) -> Result<(BucketMetadata, bool)> {
+    let (mut bm, persisted) = match read_bucket_metadata(api.clone(), bucket).await {
+        Ok(res) => (res, true),
        Err(err) => {
            if err != Error::ConfigNotFound {
                return Err(err);
            }

-            // info!("bucketmeta {} not found with err {:?}, start to init ", bucket, &err);
-
-            BucketMetadata::new(bucket)
+            (BucketMetadata::new(bucket), false)
        }
    };

@@ -1112,7 +1121,7 @@ pub async fn load_bucket_metadata_parse(api: Arc<ECStore>, bucket: &str, parse:
        bm.parse_all_configs()?;
    }

-    Ok(bm)
+    Ok((bm, persisted))
 }

 async fn read_bucket_metadata(api: Arc<ECStore>, bucket: &str) -> Result<BucketMetadata> {
@@ -16,7 +16,7 @@ use super::metadata::{BUCKET_TARGETS_FILE, BucketMetadata, load_bucket_metadata}
 use super::quota::BucketQuota;
 use super::target::BucketTargets;
 use crate::bucket::bucket_target_sys::BucketTargetSys;
-use crate::bucket::metadata::load_bucket_metadata_parse;
+use crate::bucket::metadata::{load_bucket_metadata_parse, load_bucket_metadata_parse_with_presence};
 use crate::bucket::utils::is_meta_bucketname;
 use crate::disk::RUSTFS_META_BUCKET;
 use crate::error::{Error, Result, is_err_bucket_not_found};
@@ -190,7 +190,7 @@ pub async fn get(bucket: &str) -> Result<Arc<BucketMetadata>> {
 // instance cell is not initialized yet (early startup) they fall back to the
 // ambient default — the single-instance legacy behavior.

-fn bucket_metadata_sys_of(ctx: &crate::runtime::instance::InstanceContext) -> Result<Arc<RwLock<BucketMetadataSys>>> {
+pub(crate) fn bucket_metadata_sys_of(ctx: &crate::runtime::instance::InstanceContext) -> Result<Arc<RwLock<BucketMetadataSys>>> {
    if let Some(sys) = ctx.bucket_metadata_sys() {
        return Ok(sys);
    }
@@ -422,9 +422,23 @@ pub async fn list_bucket_targets(bucket: &str) -> Result<BucketTargets> {
    bucket_meta_sys.get_bucket_targets_config(bucket).await
 }

+/// Bound and lifetime of the negative cache for buckets with no persisted
+/// metadata. Entries are invalidated the moment real metadata is cached, so
+/// the TTL only bounds staleness for out-of-band creations whose reload
+/// notification was lost; the capacity bounds memory under bogus-name floods.
+const ABSENT_BUCKET_METADATA_TTL: Duration = Duration::from_secs(30);
+const ABSENT_BUCKET_METADATA_MAX_ENTRIES: u64 = 10_000;
+
 #[derive(Debug)]
 pub struct BucketMetadataSys {
    metadata_map: RwLock<HashMap<String, Arc<BucketMetadata>>>,
+    /// Buckets recently observed to have no persisted metadata. Serving the
+    /// fabricated default from here (instead of re-reading disk) keeps the
+    /// per-request cost of repeated lookups for such names bounded — without
+    /// this, every request naming a nonexistent bucket pays a namespace-lock
+    /// acquisition plus a full erasure-set metadata fanout (reachable
+    /// pre-auth via CORS preflight, and per-key in DeleteObjects).
+    absent_metadata: moka::future::Cache<String, ()>,
    api: Arc<ECStore>,
    initialized: RwLock<bool>,
 }
@@ -433,6 +447,10 @@ impl BucketMetadataSys {
    pub fn new(api: Arc<ECStore>) -> Self {
        Self {
            metadata_map: RwLock::new(HashMap::new()),
+            absent_metadata: moka::future::Cache::builder()
+                .max_capacity(ABSENT_BUCKET_METADATA_MAX_ENTRIES)
+                .time_to_live(ABSENT_BUCKET_METADATA_TTL)
+                .build(),
            api,
            initialized: RwLock::new(false),
        }
@@ -487,7 +505,7 @@ impl BucketMetadataSys {
                        },
                    )
                    .await;
-                load_bucket_metadata(self.api.clone(), bucket.as_str()).await
+                load_bucket_metadata_parse_with_presence(self.api.clone(), bucket.as_str(), true).await
            });
        }

@@ -495,9 +513,24 @@ impl BucketMetadataSys {

        for (idx, res) in results.into_iter().enumerate() {
            match res {
-                Ok(res) => {
+                Ok((bm, persisted)) => {
                    if let Some(bucket) = buckets.get(idx) {
-                        self.set(bucket.clone(), Arc::new(res)).await;
+                        if persisted {
+                            self.set(bucket.clone(), Arc::new(bm)).await;
+                        } else {
+                            // A fabricated default (no persisted metadata
+                            // readable right now) must never REPLACE an
+                            // existing entry: the periodic refresh would
+                            // otherwise downgrade a lock-enabled bucket to an
+                            // authoritative "no lock" default on a transient
+                            // ConfigNotFound, disabling the object-lock
+                            // delete gate and wiping its target/durability
+                            // sync state. Insert-if-vacant keeps the startup
+                            // behavior for legacy buckets without a metadata
+                            // file, atomically under the map write lock.
+                            let mut map = self.metadata_map.write().await;
+                            map.entry(bucket.clone()).or_insert_with(|| Arc::new(bm));
+                        }
                    }
                }
                Err(e) => {
@@ -528,6 +561,8 @@ impl BucketMetadataSys {
            let mut map = self.metadata_map.write().await;
            map.insert(bucket.clone(), bm.clone());
            drop(map);
+            // Real metadata supersedes any recorded absence immediately.
+            self.absent_metadata.invalidate(&bucket).await;
            sync_bucket_target_sys(&bucket, &bm).await;
            sync_bucket_durability(&bucket, &bm);
        }
@@ -630,13 +665,22 @@ impl BucketMetadataSys {
    pub async fn get_config(&self, bucket: &str) -> Result<(Arc<BucketMetadata>, bool)> {
        let has_bm = {
            let map = self.metadata_map.read().await;
-            map.get(&bucket.to_string()).cloned()
+            map.get(bucket).cloned()
        };

        if let Some(bm) = has_bm {
            Ok((bm, false))
        } else {
-            let bm = match load_bucket_metadata(self.api.clone(), bucket).await {
+            // A recent lookup already established there is no persisted
+            // metadata: serve the fabricated default without another
+            // namespace-lock + erasure-set fanout.
+            if self.absent_metadata.get(bucket).await.is_some() {
+                let mut bm = BucketMetadata::new(bucket);
+                bm.default_timestamps();
+                return Ok((Arc::new(bm), true));
+            }
+
+            let (bm, persisted) = match load_bucket_metadata_parse_with_presence(self.api.clone(), bucket, true).await {
                Ok(res) => res,
                Err(err) => {
                    return if *self.initialized.read().await {
@@ -647,13 +691,27 @@ impl BucketMetadataSys {
                }
            };

-            let mut map = self.metadata_map.write().await;
-
            let bm = Arc::new(bm);
-            map.insert(bucket.to_string(), bm.clone());
-            drop(map);
-            sync_bucket_target_sys(bucket, &bm).await;
-            sync_bucket_durability(bucket, &bm);
+
+            // This lazy path caches only metadata that actually exists on
+            // this store. A fabricated default must not enter the map:
+            // `get()` is map-only and fail-closed — the object-lock delete
+            // gate (`object_lock_delete_check_required`) skips its per-object
+            // protection stat exactly when the map serves metadata saying the
+            // bucket has no Object Lock, so caching a fabricated default here
+            // would turn a metadata miss into an authoritative "no lock"
+            // answer. (Startup `concurrent_load` still caches fabricated
+            // defaults for buckets listed on disk — legacy buckets without a
+            // metadata file — but never lets one replace an existing entry.)
+            if persisted {
+                let mut map = self.metadata_map.write().await;
+                map.insert(bucket.to_string(), bm.clone());
+                drop(map);
+                sync_bucket_target_sys(bucket, &bm).await;
+                sync_bucket_durability(bucket, &bm);
+            } else {
+                self.absent_metadata.insert(bucket.to_string(), ()).await;
+            }

            Ok((bm, true))
        }
@@ -873,13 +931,126 @@ impl BucketMetadataSys {
    }
 }

+/// Test-only fixture shared with sibling modules (e.g. the quota checker
+/// tests): a 4-disk `ECStore` on an isolated instance context, so tests
+/// exercising the metadata system never touch ambient process state.
+#[cfg(test)]
+pub(crate) mod test_support {
+    use super::*;
+    use crate::disk::endpoint::Endpoint;
+    use crate::layout::endpoints::{EndpointServerPools, Endpoints, PoolEndpoints};
+    use crate::runtime::instance::InstanceContext;
+    use crate::store::init_local_disks_with_instance_ctx;
+
+    pub(crate) async fn isolated_store_over_temp_disks() -> (Vec<tempfile::TempDir>, Arc<ECStore>) {
+        let mut dirs = Vec::with_capacity(4);
+        let mut endpoints = Vec::with_capacity(4);
+        for disk_idx in 0..4 {
+            let dir = tempfile::tempdir().expect("tempdir should be created");
+            let mut endpoint =
+                Endpoint::try_from(dir.path().to_str().expect("tempdir path should be utf8")).expect("endpoint should parse");
+            endpoint.set_pool_index(0);
+            endpoint.set_set_index(0);
+            endpoint.set_disk_index(disk_idx);
+            dirs.push(dir);
+            endpoints.push(endpoint);
+        }
+        let endpoint_pools = EndpointServerPools(vec![PoolEndpoints {
+            legacy: false,
+            set_count: 1,
+            drives_per_set: 4,
+            endpoints: Endpoints::from(endpoints),
+            cmd_line: "metadata-sys-cache-test".to_string(),
+            platform: "test".to_string(),
+        }]);
+        let instance_ctx = Arc::new(InstanceContext::new());
+        init_local_disks_with_instance_ctx(&instance_ctx, endpoint_pools.clone())
+            .await
+            .expect("local disks should initialize");
+        let ecstore = ECStore::new_with_instance_ctx(
+            "127.0.0.1:0".parse().expect("test address"),
+            endpoint_pools,
+            CancellationToken::new(),
+            instance_ctx,
+        )
+        .await
+        .expect("ECStore should initialize");
+        (dirs, ecstore)
+    }
+}
+
 #[cfg(test)]
 mod tests {
+    use super::test_support::isolated_store_over_temp_disks;
    use super::*;
    use crate::bucket::target::{BucketTarget, BucketTargetType, Credentials};
    use serial_test::serial;
    use tokio::time::timeout;

+    /// Pins the fail-closed caching contract of the lazy `get_config` path
+    /// and the refresh no-replace rule: fabricated defaults are returned but
+    /// never served by the map-only `get()`, persisted metadata is cached on
+    /// lazy load (superseding a recorded absence), and a refresh-load miss
+    /// never replaces an existing entry.
+    #[tokio::test]
+    async fn get_config_never_caches_fabricated_defaults_as_authoritative() {
+        let (_dirs, ecstore) = isolated_store_over_temp_disks().await;
+        let sys = BucketMetadataSys::new(ecstore);
+
+        // (a) Miss: the fabricated default is returned but not cached.
+        let (bm, _) = sys
+            .get_config("absent-bucket")
+            .await
+            .expect("fabricated default should be returned");
+        assert!(bm.object_lock_config_xml.is_empty());
+        assert!(
+            sys.get("absent-bucket").await.is_err(),
+            "a fabricated default must never be served by the map-only get()"
+        );
+
+        // The repeat lookup is served from the negative cache, same answer.
+        let (bm, _) = sys
+            .get_config("absent-bucket")
+            .await
+            .expect("negative-cached default should be returned");
+        assert!(bm.object_lock_config_xml.is_empty());
+        assert!(sys.get("absent-bucket").await.is_err());
+
+        // (b) Persisting real metadata supersedes the recorded absence, and a
+        // lazy reload after a map wipe re-caches it.
+        let mut persisted = BucketMetadata::new("absent-bucket");
+        persisted.policy_config_json = b"persisted-marker".to_vec();
+        sys.persist_and_set(persisted).await.expect("metadata should persist");
+        sys.metadata_map.write().await.clear();
+        let _ = sys
+            .get_config("absent-bucket")
+            .await
+            .expect("persisted metadata should lazily reload");
+        let cached = sys
+            .get("absent-bucket")
+            .await
+            .expect("lazily loaded persisted metadata must be cached");
+        assert_eq!(cached.policy_config_json, b"persisted-marker".to_vec());
+
+        // (c) A refresh-load miss (no persisted metadata readable) must not
+        // replace an existing entry.
+        let mut kept = BucketMetadata::new("kept-bucket");
+        kept.policy_config_json = b"kept-marker".to_vec();
+        sys.set("kept-bucket".to_string(), Arc::new(kept)).await;
+        let mut failed = HashSet::new();
+        let refresh_targets = vec!["kept-bucket".to_string()];
+        sys.concurrent_load(&refresh_targets, &mut failed).await;
+        let kept = sys
+            .get("kept-bucket")
+            .await
+            .expect("existing entry must survive a refresh miss");
+        assert_eq!(
+            kept.policy_config_json,
+            b"kept-marker".to_vec(),
+            "a fabricated refresh default must not replace real metadata"
+        );
+    }
+
    fn target(bucket: &str, id: &str) -> BucketTarget {
        BucketTarget {
            source_bucket: bucket.to_string(),
@@ -538,10 +538,12 @@ mod tests {
        use crate::disk::{BUCKET_META_PREFIX, MIGRATING_META_BUCKET, RUSTFS_META_BUCKET};
        use crate::layout::endpoints::{EndpointServerPools, Endpoints, PoolEndpoints};
        use crate::object_api::{ObjectOptions, PutObjReader};
+        use crate::runtime::instance::InstanceContext;
        use crate::storage_api_contracts::bucket::{BucketOperations, BucketOptions, MakeBucketOptions};
        use crate::storage_api_contracts::object::{ObjectIO, ObjectOperations};
-        use crate::store::{ECStore, init_local_disks};
+        use crate::store::{ECStore, init_local_disks_with_instance_ctx};
        use rustfs_utils::path::SLASH_SEPARATOR;
+        use std::sync::Arc;
        use tokio::fs;
        use tokio_util::sync::CancellationToken;
        use uuid::Uuid;
@@ -570,10 +572,20 @@ mod tests {
            cmd_line: "minio-migrate-test".to_string(),
            platform: format!("OS: {} | Arch: {}", std::env::consts::OS, std::env::consts::ARCH),
        }]);
-        init_local_disks(endpoint_pools.clone()).await.unwrap();
-        let ecstore = ECStore::new("127.0.0.1:0".parse().unwrap(), endpoint_pools, CancellationToken::new())
+        // Isolated instance context: this test deletes its disks at the end,
+        // and dead entries in the shared registry break other cached envs.
+        let instance_ctx = Arc::new(InstanceContext::new());
+        init_local_disks_with_instance_ctx(&instance_ctx, endpoint_pools.clone())
            .await
            .unwrap();
+        let ecstore = ECStore::new_with_instance_ctx(
+            "127.0.0.1:0".parse().unwrap(),
+            endpoint_pools,
+            CancellationToken::new(),
+            instance_ctx,
+        )
+        .await
+        .unwrap();
        let existing: Vec<String> = ecstore
            .list_bucket(&BucketOptions {
                no_metadata: true,
@@ -118,11 +118,17 @@ impl QuotaChecker {
    }

    pub async fn get_quota_config(&self, bucket: &str) -> Result<BucketQuota, QuotaError> {
-        let meta = self
+        // `get_config`, not the map-only `get()`: a bucket with no persisted
+        // metadata must resolve to the fabricated default (no quota
+        // configured) so the admission check passes and the request reaches
+        // the NoSuchBucket answer — a map-only miss would fail every such
+        // PUT closed with 503 before the 404 could be produced. Real read
+        // faults still surface as errors and keep the fail-closed behavior.
+        let (meta, _) = self
            .metadata_sys
            .read()
            .await
-            .get(bucket)
+            .get_config(bucket)
            .await
            .map_err(QuotaError::StorageError)?;

@@ -178,6 +184,26 @@ impl QuotaChecker {
 #[cfg(test)]
 mod tests {
    use super::*;
+    use crate::bucket::metadata_sys::test_support::isolated_store_over_temp_disks;
+
+    /// Regression (PR #5307 / s3-tests `test_100_continue_error_retry`): a
+    /// bucket with no persisted metadata has no quota, so the admission check
+    /// must pass and let the request reach its NoSuchBucket answer. With the
+    /// map-only `get()` this failed closed as a retryable 503 on every PUT to
+    /// a nonexistent bucket.
+    #[tokio::test]
+    async fn quota_check_allows_bucket_without_persisted_metadata() {
+        let (_dirs, ecstore) = isolated_store_over_temp_disks().await;
+        let sys = Arc::new(RwLock::new(BucketMetadataSys::new(ecstore)));
+        let checker = QuotaChecker::new(sys);
+
+        let result = checker
+            .check_quota("no-such-bucket", QuotaOperation::PutObject, 1024)
+            .await
+            .expect("a bucket with no persisted metadata has no quota and must not fail the check");
+        assert!(result.allowed);
+        assert_eq!(result.quota_limit, None);
+    }

    #[tokio::test]
    async fn test_quota_check_no_limit() {
@@ -85,4 +85,21 @@ impl BucketVersioningSys {

        Ok(cfg)
    }
+
+    /// Instance-scoped variant of [`Self::get`] (backlog#1052): resolves the
+    /// caller's own instance context so a second in-process store never
+    /// answers with the first instance's versioning state; falls back to the
+    /// ambient system when the instance cell is not initialized.
+    pub(crate) async fn get_in(ctx: &crate::runtime::instance::InstanceContext, bucket: &str) -> Result<VersioningConfiguration> {
+        if bucket == RUSTFS_META_BUCKET || bucket.starts_with(RUSTFS_META_BUCKET) {
+            return Ok(VersioningConfiguration::default());
+        }
+
+        let bucket_meta_sys_lock = crate::bucket::metadata_sys::bucket_metadata_sys_of(ctx)?;
+        let bucket_meta_sys = bucket_meta_sys_lock.read().await;
+
+        let (cfg, _) = bucket_meta_sys.get_versioning_config(bucket).await?;
+
+        Ok(cfg)
+    }
 }