From 98f979390da476e9197a6276615fd2c671fd9e9c Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Sun, 27 Sep 2026 19:18:32 -0300 Subject: [PATCH 1/5] fix(s3): stop a re-created bucket from adopting a previous one's objects A bucket absent from memory can still have a directory in the data path: after `/_fakecloud/reset` (which clears memory and deliberately leaves the store alone), or after a create or delete that stopped partway. CreateBucket cleared only the sidecars, so the stale `objects/` tree survived: the caller saw an empty bucket, and the previous incarnation's objects came back on the next load. CreateBucket now clears the whole stored directory for the name, before writing this bucket's own state (the clear removes `meta.toml`, so doing it afterwards would delete the bucket the create just wrote). That clear would otherwise turn re-creating a name into the thing that destroys recoverable data: the loader skips a bucket whose objects it cannot read, so such a bucket is absent from memory while its files sit intact on disk. The store now records which buckets `load` refused, and CreateBucket refuses those names with BucketAlreadyExists and a message naming the repair. DeleteBucket is the in-band escape -- it is the explicitly destructive verb -- so a refused name is never permanently stuck. - `S3Store::bucket_state_exists` and `bucket_load_refused`, recorded at load rather than probed per call, so "the loader could not read this" is never confused with "this is not in memory" - the refusal is cleared only once `remove_dir_all` actually succeeded - e2e: the re-created bucket is empty before AND after a restart; a load-refused name is declined without destroying its objects, and repairing the one bad file brings the whole bucket back; DeleteBucket frees a refused name - docs: Gotcha covering the clear, the refusal and the escape --- crates/fakecloud-e2e/tests/s3_persistence.rs | 133 ++++++++++++++++--- crates/fakecloud-persistence/src/s3.rs | 75 +++++++++-- crates/fakecloud-s3/src/service/buckets.rs | 101 ++++++++++---- website/content/docs/services/s3.md | 1 + 4 files changed, 253 insertions(+), 57 deletions(-) diff --git a/crates/fakecloud-e2e/tests/s3_persistence.rs b/crates/fakecloud-e2e/tests/s3_persistence.rs index fb6330302..03e340857 100644 --- a/crates/fakecloud-e2e/tests/s3_persistence.rs +++ b/crates/fakecloud-e2e/tests/s3_persistence.rs @@ -1604,12 +1604,37 @@ async fn persistence_create_after_reset_reuses_the_name() { .await .expect("re-creating a bucket after a reset must succeed"); - // The bucket's stored CONFIGURATION does not carry over: the create clears - // every sidecar for the name, so the tag set from before the reset is gone - // rather than being restored on the next load. + // The re-created bucket is empty, and stays empty across a restart. Leaving + // the old directory in place meant the caller saw an empty bucket now and + // the previous incarnation's objects came back on the next load. + let list = client + .list_objects_v2() + .bucket("reset-reuse") + .send() + .await + .unwrap(); + assert!( + list.contents().is_empty(), + "re-created bucket is not empty: {:?}", + list.contents() + ); + let mut server = server; server.restart().await; let client = server.s3_client().await; + + let list = client + .list_objects_v2() + .bucket("reset-reuse") + .send() + .await + .unwrap(); + assert!( + list.contents().is_empty(), + "the previous bucket's objects came back after a restart: {:?}", + list.contents() + ); + // Nor does its stored configuration carry over. let tags = client .get_bucket_tagging() .bucket("reset-reuse") @@ -1622,26 +1647,26 @@ async fn persistence_create_after_reset_reuses_the_name() { } #[tokio::test] -async fn persistence_recreating_a_load_skipped_bucket_does_not_destroy_its_objects() { - // The loader skips a bucket it cannot fully read (a corrupt object meta, a - // missing part body) and logs a warning -- the data is still on disk and - // recoverable by fixing the one bad file. Such a bucket is absent from - // ListBuckets, so its name looks free: re-creating it must not be what - // destroys the objects, which is why the create clears only the sidecars. +async fn persistence_recreating_a_load_refused_bucket_is_declined_not_destructive() { + // The loader skips a bucket it cannot fully read and logs a warning: the data + // is on disk and recoverable by fixing the one bad file. Such a bucket is + // absent from ListBuckets, so its name looks free -- and a create now clears + // the directory, so it has to be refused rather than becoming the thing that + // destroys the data. let tmp = tempfile::tempdir().unwrap(); let mut server = TestServer::start_persistent(tmp.path()).await; let client = server.s3_client().await; client .create_bucket() - .bucket("skipped") + .bucket("refused") .send() .await .unwrap(); for key in ["keep.txt", "corrupt.txt"] { client .put_object() - .bucket("skipped") + .bucket("refused") .key(key) .body(ByteStream::from_static(b"precious")) .send() @@ -1653,30 +1678,34 @@ async fn persistence_recreating_a_load_skipped_bucket_does_not_destroy_its_objec .path() .join("s3") .join("buckets") - .join("skipped") + .join("refused") .join("objects"); - let corrupt_meta = objects_dir.join("corrupt.txt").join("null.toml"); - assert!(corrupt_meta.exists(), "expected {corrupt_meta:?} to exist"); - std::fs::write(&corrupt_meta, "not valid toml = = =").unwrap(); + let corrupt = objects_dir.join("corrupt.txt").join("null.toml"); + assert!(corrupt.exists(), "expected {corrupt:?}"); + std::fs::write(&corrupt, "not valid toml = = =").unwrap(); server.restart().await; let client = server.s3_client().await; let list = client.list_buckets().send().await.unwrap(); assert!( - !list.buckets().iter().any(|b| b.name() == Some("skipped")), + !list.buckets().iter().any(|b| b.name() == Some("refused")), "expected the unreadable bucket to be skipped on load" ); - client + let err = client .create_bucket() - .bucket("skipped") + .bucket("refused") .send() .await - .unwrap(); + .expect_err("the name must be refused while the data is still there"); + assert!( + format!("{err:?}").contains("BucketAlreadyExists"), + "unexpected error: {err:?}" + ); assert!( objects_dir.join("keep.txt").join("null.bin").exists(), - "re-creating a load-skipped bucket destroyed its objects" + "the refused create destroyed the objects it was protecting" ); // Repairing the one bad file brings the whole bucket back. @@ -1685,7 +1714,7 @@ async fn persistence_recreating_a_load_skipped_bucket_does_not_destroy_its_objec let client = server.s3_client().await; let body = client .get_object() - .bucket("skipped") + .bucket("refused") .key("keep.txt") .send() .await @@ -1698,6 +1727,68 @@ async fn persistence_recreating_a_load_skipped_bucket_does_not_destroy_its_objec assert_eq!(&body[..], b"precious"); } +#[tokio::test] +async fn persistence_delete_frees_a_load_refused_name() { + // Without an in-band escape a refused name is both missing (HeadBucket 404, + // absent from ListBuckets) and un-creatable, leaving no way out but editing + // the data path by hand. DeleteBucket is the explicitly destructive verb, so + // it is the one that may discard the unreadable directory. + let tmp = tempfile::tempdir().unwrap(); + let mut server = TestServer::start_persistent(tmp.path()).await; + let client = server.s3_client().await; + + client + .create_bucket() + .bucket("escape") + .send() + .await + .unwrap(); + client + .put_object() + .bucket("escape") + .key("k.txt") + .body(ByteStream::from_static(b"v")) + .send() + .await + .unwrap(); + std::fs::write( + tmp.path() + .join("s3") + .join("buckets") + .join("escape") + .join("objects") + .join("k.txt") + .join("null.toml"), + "not valid toml = = =", + ) + .unwrap(); + + server.restart().await; + let client = server.s3_client().await; + + assert!( + client + .create_bucket() + .bucket("escape") + .send() + .await + .is_err(), + "create must be refused first" + ); + client + .delete_bucket() + .bucket("escape") + .send() + .await + .expect("delete must clear a load-refused bucket"); + client + .create_bucket() + .bucket("escape") + .send() + .await + .expect("the name must be usable after the delete"); +} + #[tokio::test] async fn persistence_put_object_acl_survives_restart() { // PutObjectAcl writes the object's meta sidecar, and that snapshot copies diff --git a/crates/fakecloud-persistence/src/s3.rs b/crates/fakecloud-persistence/src/s3.rs index 01a0d3869..719c0eec6 100644 --- a/crates/fakecloud-persistence/src/s3.rs +++ b/crates/fakecloud-persistence/src/s3.rs @@ -379,6 +379,26 @@ pub trait S3Store: Send + Sync { fn delete_bucket_subresource(&self, bucket: &str, kind: BucketSubresource) -> StoreResult<()>; fn delete_bucket(&self, bucket: &str) -> StoreResult<()>; + /// Whether the store holds any persisted state for `bucket`. + /// + /// A bucket absent from memory can still have files on disk: after + /// `/_fakecloud/reset` (which clears memory and leaves the store alone), or + /// from a create or delete that stopped partway. Memory-only stores hold + /// nothing, hence the default. + fn bucket_state_exists(&self, _bucket: &str) -> bool { + false + } + + /// Whether the last [`S3Store::load`] REFUSED this bucket -- a corrupt + /// object meta, a missing part body: data still on disk and recoverable by + /// repairing the one bad file. + /// + /// Recorded at load, not probed per call, so a caller cannot confuse "the + /// loader could not read this" with "this is simply not in memory". + fn bucket_load_refused(&self, _bucket: &str) -> bool { + false + } + fn put_object( &self, bucket: &str, @@ -536,11 +556,18 @@ impl S3Store for MemoryS3Store { pub struct DiskS3Store { root: PathBuf, cache: std::sync::Arc, + /// Escaped directory names `load` could not read, so a caller can tell + /// recoverable data apart from state the operator already discarded. + load_refused: parking_lot::RwLock>, } impl DiskS3Store { pub fn new(root: PathBuf, cache: std::sync::Arc) -> Self { - Self { root, cache } + Self { + root, + cache, + load_refused: parking_lot::RwLock::new(std::collections::HashSet::new()), + } } fn buckets_dir(&self) -> PathBuf { @@ -642,6 +669,10 @@ fn io_other(msg: impl Into) -> StoreError { impl S3Store for DiskS3Store { fn load(&self) -> StoreResult { + // This load decides which buckets are refused, so start clean: carrying + // entries over would keep a bucket whose bad file was repaired + // un-creatable. + self.load_refused.write().clear(); let mut state = S3State::default(); let buckets_dir = self.buckets_dir(); if !buckets_dir.exists() { @@ -858,11 +889,20 @@ impl S3Store for DiskS3Store { state.buckets.insert(snap.meta.name.clone(), snap); } Ok(None) => {} - Err(e) => tracing::warn!( - bucket = %bdir.display(), - error = %e, - "skipping unreadable S3 bucket during load" - ), + Err(e) => { + // Remembered so a later CreateBucket knows this directory + // holds recoverable data rather than state the operator + // discarded. + if let Some(dir_name) = bdir.file_name().and_then(|n| n.to_str()) { + self.load_refused.write().insert(dir_name.to_string()); + } + tracing::warn!( + bucket = %bdir.display(), + error = %e, + "skipping unreadable S3 bucket during load; its name is refused until it \ + is repaired and the server restarted, or the bucket is deleted" + ); + } } } Ok(state) @@ -899,13 +939,32 @@ impl S3Store for DiskS3Store { } } + fn bucket_state_exists(&self, bucket: &str) -> bool { + self.bucket_dir(bucket).exists() + } + + fn bucket_load_refused(&self, bucket: &str) -> bool { + self.load_refused + .read() + .contains(&crate::key_escape::escape_key_segment(bucket)) + } + fn delete_bucket(&self, bucket: &str) -> StoreResult<()> { let dir = self.bucket_dir(bucket); - match std::fs::remove_dir_all(&dir) { + let outcome = match std::fs::remove_dir_all(&dir) { Ok(_) => Ok(()), Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), - Err(e) => Err(e.into()), + Err(e) => Err(StoreError::from(e)), + }; + // Only once the tree is really gone: a removal that stops partway + // returns Err and the caller reports 500, so dropping the refusal here + // would leave the next CreateBucket free to discard the remains. + if outcome.is_ok() { + self.load_refused + .write() + .remove(&crate::key_escape::escape_key_segment(bucket)); } + outcome } fn put_object( diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index 8b3101431..72eaa314c 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -329,6 +329,34 @@ impl S3Service { let acl = acl_header.unwrap_or("private"); let mut accts = self.state.write(); + // A bucket the loader could not read is absent from memory, so its name + // looks free -- but its objects are on disk and recoverable by repairing + // the one bad file, and this create is about to clear the directory. + // Refuse instead, and say how to get the name back. DeleteBucket is the + // in-band escape (it is the explicitly destructive verb), so the name is + // never permanently stuck. + if self.store.bucket_load_refused(bucket) + && !accts + .iter() + .any(|(_, acct)| acct.buckets.contains_key(bucket)) + { + tracing::warn!( + target: "fakecloud::s3", + bucket = %bucket, + "CreateBucket refused: the store holds data for this bucket that could not be \ + read at load", + ); + return Err(AwsServiceError::aws_error( + StatusCode::CONFLICT, + "BucketAlreadyExists", + format!( + "The requested bucket name is not available: {bucket} holds persisted data \ + that could not be read at load (an unreadable object, or a delete that \ + stopped partway). Repair its directory in the data path and restart, or \ + DeleteBucket to discard it." + ), + )); + } // Check global uniqueness across all accounts before creating for (other_account_id, acct_state) in accts.iter() { if acct_state.buckets.contains_key(bucket) { @@ -456,33 +484,26 @@ impl S3Service { })?), None => None, }; - // The meta goes first, and nothing is destroyed until it lands: a - // create that fails here has changed nothing on disk, where clearing - // the old sidecars first would have thrown away the configuration of - // whatever bucket this name belonged to for a create that never - // happened. + // Clear whatever the store still holds for this name BEFORE writing this + // bucket's own state -- the clear removes `meta.toml`, so doing it + // afterwards would delete the bucket this create just wrote. + // + // A bucket absent from memory can still have a directory on disk: after + // `/_fakecloud/reset` (which clears memory and deliberately leaves the + // store alone), or from a create or delete that stopped partway. Leaving + // it meant the new bucket inherited the previous one's `objects/` on the + // next load, so the caller saw an empty bucket now and the old objects + // came back after a restart. + // + // What this can destroy is state the operator already discarded: a name + // whose data the loader REFUSED is turned away earlier, before anything + // is written, so merely-unreadable data is never what a create clears. + self.store + .delete_bucket(bucket) + .map_err(super::persistence_error)?; self.store .put_bucket_meta(bucket, &meta) .map_err(super::persistence_error)?; - // Clear every stored subresource for this name before writing this - // bucket's own, now that the create is committed. A create or delete that stopped partway -- or a - // `/_fakecloud/reset`, which clears memory and leaves the store alone -- - // can leave sidecars behind, and a later create would otherwise be - // restored carrying the old bucket's `policy.toml`, `acl.toml` and the - // rest. Each delete tolerates a missing file, which is the normal case. - // - // Scoped to the sidecars on purpose. `objects/` is NOT touched: the - // loader skips a bucket whose objects it cannot read, so that bucket is - // absent from memory while its data sits intact on disk, and clearing - // the directory here would make re-creating the name the thing that - // destroys it. Whether a create should adopt or discard a stale object - // tree is a separate question from this one, and this is not the change - // that answers it. - for kind in fakecloud_persistence::ALL_SUBRESOURCES { - self.store - .delete_bucket_subresource(bucket, *kind) - .map_err(super::persistence_error)?; - } self.put_bucket_subresource_if_set( bucket, @@ -524,10 +545,34 @@ impl S3Service { ) -> Result { let mut accts = self.state.write(); let state = accts.get_or_create(account_id); - let b = state - .buckets - .get(bucket) - .ok_or_else(|| no_such_bucket(bucket))?; + let Some(b) = state.buckets.get(bucket) else { + // A bucket the loader refused is absent from memory, so every read + // reports it missing while CreateBucket refuses its name. Delete is + // the explicitly destructive verb, so it is the way out: discard the + // unreadable directory and free the name. No emptiness check is + // possible here -- the objects are exactly what could not be read -- + // and no owner check either, since the metadata carrying ownership is + // what failed, so any caller may clear it. + if self.store.bucket_load_refused(bucket) { + tracing::warn!( + target: "fakecloud::s3", + bucket = %bucket, + account_id = %account_id, + "DeleteBucket discarding data that could not be read at load; ownership could \ + not be verified, so this is reachable by any account", + ); + self.store + .delete_bucket(bucket) + .map_err(super::persistence_error)?; + return Ok(AwsResponse { + status: StatusCode::NO_CONTENT, + content_type: "application/xml".to_string(), + body: Bytes::new().into(), + headers: HeaderMap::new(), + }); + } + return Err(no_such_bucket(bucket)); + }; // Bucket must be empty to delete (no objects and no versions) let has_real_objects = b.objects.values().any(|o| !o.is_delete_marker); let has_versions = b.object_versions.values().any(|v| !v.is_empty()); diff --git a/website/content/docs/services/s3.md b/website/content/docs/services/s3.md index efa446855..09e2e7459 100644 --- a/website/content/docs/services/s3.md +++ b/website/content/docs/services/s3.md @@ -47,6 +47,7 @@ REST. Path-based routing (`/bucket/key`), HTTP method + query string for actions - In persistent mode, object bodies stream to disk with a bounded LRU cache (`--s3-cache-size`, default 256 MiB). Objects larger than `cache-size / 2` bypass the cache. - The `/_fakecloud/s3/notifications` introspection buffer is intentionally not persisted across restarts. +- In persistent mode, `CreateBucket` clears whatever the store still holds under that name, so a re-created bucket is genuinely empty rather than inheriting the previous incarnation's objects on the next load. That matters after `/_fakecloud/reset`, which clears in-memory state and deliberately leaves the store alone. The exception is a bucket the loader could not read (a corrupt object sidecar, a missing multipart part body): it is skipped at load with a warning, so it is absent from `ListBuckets` while its data sits intact on disk, and `CreateBucket` refuses the name with `BucketAlreadyExists` rather than becoming the thing that destroys it. Repair the file and restart to get the bucket back, or `DeleteBucket` to discard it and free the name -- that delete skips the usual empty-bucket check and any ownership check, since the objects and the metadata carrying ownership are exactly what could not be read. - SigV4 signatures are parsed for request routing but never validated. ## Source From c0ee85802ebadd718614d2afdb13efc4ea9b3632 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Sun, 27 Sep 2026 19:57:08 -0300 Subject: [PATCH 2/5] fix(s3): bound the load-refused name refusal to data that is still there Review of the commit before this one, three edges it got wrong: - The refusal is recorded at load and never re-probed, so an operator who followed the error text by deleting the bucket's directory -- without restarting -- found the name refused for the rest of the process lifetime, told to repair something that no longer existed. It now requires `bucket_state_exists` as well, which is also the caller that method was missing. - The whole-directory clear is a recursive remove running under the global S3 write lock. It is now skipped when there is no directory, so the ordinary create pays a `stat` rather than a walk, and only the create-after-reset that needs it does the work. - The DeleteBucket escape skipped the empty-bucket check, and with it the only thing that makes an Object Lock bucket undeletable. A reflexive `aws s3 rb`, or a retry of the tool that just got the 409, could destroy retained data that no bucket on real S3 lets go. The lock config is its own file and is usually readable when an object's is not, so the escape refuses while it is there and leaves that case to the operator. - `S3Store::bucket_subresource_exists`, readable without the bucket being loadable, which is what makes it usable on a refused bucket - e2e: removing the directory frees the name with no restart; a refused bucket under Object Lock is not discarded by DeleteBucket and its objects survive, and repairing the bad file brings it back still locked - docs: both edges, plus the asymmetry a caller sees (reads report the name missing while the create refuses it) --- crates/fakecloud-e2e/tests/s3_persistence.rs | 117 +++++++++++++++++++ crates/fakecloud-persistence/src/s3.rs | 16 +++ crates/fakecloud-s3/src/service/buckets.rs | 42 ++++++- website/content/docs/services/s3.md | 2 +- 4 files changed, 173 insertions(+), 4 deletions(-) diff --git a/crates/fakecloud-e2e/tests/s3_persistence.rs b/crates/fakecloud-e2e/tests/s3_persistence.rs index 03e340857..8cd218221 100644 --- a/crates/fakecloud-e2e/tests/s3_persistence.rs +++ b/crates/fakecloud-e2e/tests/s3_persistence.rs @@ -1727,6 +1727,123 @@ async fn persistence_recreating_a_load_refused_bucket_is_declined_not_destructiv assert_eq!(&body[..], b"precious"); } +#[tokio::test] +async fn persistence_removing_a_refused_directory_frees_the_name_without_a_restart() { + // The refusal is recorded at load and never re-probed, so it has to be + // paired with "is the data still there". An operator who follows the error + // text by deleting the directory outright would otherwise find the name + // refused for the rest of the process lifetime, told to repair something + // that no longer exists. + let tmp = tempfile::tempdir().unwrap(); + let mut server = TestServer::start_persistent(tmp.path()).await; + let client = server.s3_client().await; + + client.create_bucket().bucket("gone").send().await.unwrap(); + client + .put_object() + .bucket("gone") + .key("k.txt") + .body(ByteStream::from_static(b"v")) + .send() + .await + .unwrap(); + let bucket_dir = tmp.path().join("s3").join("buckets").join("gone"); + std::fs::write( + bucket_dir.join("objects").join("k.txt").join("null.toml"), + "not valid toml = = =", + ) + .unwrap(); + + server.restart().await; + let client = server.s3_client().await; + assert!( + client.create_bucket().bucket("gone").send().await.is_err(), + "create must be refused while the data is there" + ); + + // Same running server, no restart. + std::fs::remove_dir_all(&bucket_dir).unwrap(); + client + .create_bucket() + .bucket("gone") + .send() + .await + .expect("the name is free once the data is gone, restart or not"); +} + +#[tokio::test] +async fn persistence_delete_will_not_discard_a_refused_bucket_under_object_lock() { + // The normal delete path enforces Object Lock only indirectly: a retained + // object keeps the bucket non-empty, so the delete is refused. The escape + // hatch cannot run that check -- the objects are what could not be read -- + // so it must refuse outright rather than letting a reflexive `rb` destroy + // retained data. + let tmp = tempfile::tempdir().unwrap(); + let mut server = TestServer::start_persistent(tmp.path()).await; + let client = server.s3_client().await; + + client + .create_bucket() + .bucket("locked") + .object_lock_enabled_for_bucket(true) + .send() + .await + .unwrap(); + client + .put_object() + .bucket("locked") + .key("k.txt") + .body(ByteStream::from_static(b"retained")) + .send() + .await + .unwrap(); + let bucket_dir = tmp.path().join("s3").join("buckets").join("locked"); + assert!( + bucket_dir.join("object_lock.toml").exists(), + "expected the lock config to be persisted" + ); + std::fs::write( + bucket_dir.join("objects").join("k.txt").join("null.toml"), + "not valid toml = = =", + ) + .unwrap(); + + server.restart().await; + let client = server.s3_client().await; + + let err = client + .delete_bucket() + .bucket("locked") + .send() + .await + .expect_err("a refused bucket under Object Lock must not be discarded by DeleteBucket"); + assert!( + format!("{err:?}").contains("BucketNotEmpty"), + "unexpected error: {err:?}" + ); + assert!( + bucket_dir.join("objects").join("k.txt").exists(), + "the refused delete destroyed the objects it was protecting" + ); + + // Repairing the bad file is the way out, and the bucket comes back locked. + std::fs::remove_dir_all(bucket_dir.join("objects").join("k.txt")).unwrap(); + server.restart().await; + let client = server.s3_client().await; + let lock = client + .get_object_lock_configuration() + .bucket("locked") + .send() + .await + .expect("the repaired bucket should load with its lock configuration"); + assert_eq!( + lock.object_lock_configuration() + .and_then(|c| c.object_lock_enabled()) + .map(|e| e.as_str()), + Some("Enabled") + ); +} + #[tokio::test] async fn persistence_delete_frees_a_load_refused_name() { // Without an in-band escape a refused name is both missing (HeadBucket 404, diff --git a/crates/fakecloud-persistence/src/s3.rs b/crates/fakecloud-persistence/src/s3.rs index 719c0eec6..8cfbcfb58 100644 --- a/crates/fakecloud-persistence/src/s3.rs +++ b/crates/fakecloud-persistence/src/s3.rs @@ -389,6 +389,16 @@ pub trait S3Store: Send + Sync { false } + /// Whether the store holds this one subresource for `bucket`. + /// + /// Readable without the bucket itself being loadable, which is what makes it + /// usable on a load-refused bucket: the file the loader choked on is rarely + /// the one being asked about. Memory-only stores hold nothing, hence the + /// default. + fn bucket_subresource_exists(&self, _bucket: &str, _kind: BucketSubresource) -> bool { + false + } + /// Whether the last [`S3Store::load`] REFUSED this bucket -- a corrupt /// object meta, a missing part body: data still on disk and recoverable by /// repairing the one bad file. @@ -943,6 +953,12 @@ impl S3Store for DiskS3Store { self.bucket_dir(bucket).exists() } + fn bucket_subresource_exists(&self, bucket: &str, kind: BucketSubresource) -> bool { + self.bucket_dir(bucket) + .join(Self::subresource_filename(kind)) + .exists() + } + fn bucket_load_refused(&self, bucket: &str) -> bool { self.load_refused .read() diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index 72eaa314c..53edbb757 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -335,7 +335,13 @@ impl S3Service { // Refuse instead, and say how to get the name back. DeleteBucket is the // in-band escape (it is the explicitly destructive verb), so the name is // never permanently stuck. + // `bucket_state_exists` as well as the refusal: the refusal is recorded + // at load and never re-probed, so an operator who followed the advice + // below by deleting the directory outright -- without restarting -- would + // otherwise find the name refused for the rest of the process lifetime, + // with an error telling them to repair something that is gone. if self.store.bucket_load_refused(bucket) + && self.store.bucket_state_exists(bucket) && !accts .iter() .any(|(_, acct)| acct.buckets.contains_key(bucket)) @@ -498,9 +504,18 @@ impl S3Service { // What this can destroy is state the operator already discarded: a name // whose data the loader REFUSED is turned away earlier, before anything // is written, so merely-unreadable data is never what a create clears. - self.store - .delete_bucket(bucket) - .map_err(super::persistence_error)?; + // + // Gated on there being a directory at all, which is the case for every + // ordinary create. The clear is a recursive remove and this runs under + // the global S3 write lock, so an unconditional call would put a + // stat-and-walk of a possibly huge tree in front of every other S3 + // request on the one create-after-reset that needs it -- and a bare + // `stat` in front of all the rest. + if self.store.bucket_state_exists(bucket) { + self.store + .delete_bucket(bucket) + .map_err(super::persistence_error)?; + } self.store .put_bucket_meta(bucket, &meta) .map_err(super::persistence_error)?; @@ -554,6 +569,27 @@ impl S3Service { // and no owner check either, since the metadata carrying ownership is // what failed, so any caller may clear it. if self.store.bucket_load_refused(bucket) { + // Object Lock is the one thing the normal path enforces that + // this branch cannot: a compliance-retained object is + // undeletable because the bucket is never empty, and here the + // objects are exactly what could not be read. A reflexive + // `aws s3 rb` -- or a retry of the tool that just got the 409 -- + // would otherwise destroy retained data, which no bucket on real + // S3 permits. The lock config is its own file and is usually + // readable even when an object's is not, so refuse while it is + // there and leave that case to the operator's own hands. + if self.store.bucket_subresource_exists( + bucket, + fakecloud_persistence::BucketSubresource::ObjectLock, + ) { + return Err(AwsServiceError::aws_error( + StatusCode::CONFLICT, + "BucketNotEmpty", + format!( + "{bucket} could not be read at load and has an Object Lock configuration, so its contents cannot be shown to be free of retention. Repair its directory in the data path and restart, then delete it through the normal path." + ), + )); + } tracing::warn!( target: "fakecloud::s3", bucket = %bucket, diff --git a/website/content/docs/services/s3.md b/website/content/docs/services/s3.md index 09e2e7459..9b76632d9 100644 --- a/website/content/docs/services/s3.md +++ b/website/content/docs/services/s3.md @@ -47,7 +47,7 @@ REST. Path-based routing (`/bucket/key`), HTTP method + query string for actions - In persistent mode, object bodies stream to disk with a bounded LRU cache (`--s3-cache-size`, default 256 MiB). Objects larger than `cache-size / 2` bypass the cache. - The `/_fakecloud/s3/notifications` introspection buffer is intentionally not persisted across restarts. -- In persistent mode, `CreateBucket` clears whatever the store still holds under that name, so a re-created bucket is genuinely empty rather than inheriting the previous incarnation's objects on the next load. That matters after `/_fakecloud/reset`, which clears in-memory state and deliberately leaves the store alone. The exception is a bucket the loader could not read (a corrupt object sidecar, a missing multipart part body): it is skipped at load with a warning, so it is absent from `ListBuckets` while its data sits intact on disk, and `CreateBucket` refuses the name with `BucketAlreadyExists` rather than becoming the thing that destroys it. Repair the file and restart to get the bucket back, or `DeleteBucket` to discard it and free the name -- that delete skips the usual empty-bucket check and any ownership check, since the objects and the metadata carrying ownership are exactly what could not be read. +- In persistent mode, `CreateBucket` clears whatever the store still holds under that name, so a re-created bucket is genuinely empty rather than inheriting the previous incarnation's objects on the next load. That matters after `/_fakecloud/reset`, which clears in-memory state and deliberately leaves the store alone. The exception is a bucket the loader could not read (a corrupt object sidecar, a missing multipart part body): it is skipped at load with a warning, so it is absent from `ListBuckets` while its data sits intact on disk, and `CreateBucket` refuses the name with `BucketAlreadyExists` rather than becoming the thing that destroys it. Repair the file and restart to get the bucket back, or `DeleteBucket` to discard it and free the name -- that delete skips the usual empty-bucket check and any ownership check, since the objects and the metadata carrying ownership are exactly what could not be read. Two edges worth knowing: a refused bucket that carries an Object Lock configuration is **not** deletable that way (its contents cannot be shown to be free of retention, so the repair is the only way out), and every read still reports the name missing -- `HeadBucket` 404s and `ListBuckets` omits it -- while the create refuses it, so a head-then-create script fails on the create rather than silently overwriting the data. Removing the directory yourself frees the name immediately, with no restart needed. - SigV4 signatures are parsed for request routing but never validated. ## Source From eb4e90cbe4c9517e8111c3b257e99ec51e35c2df Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Sun, 27 Sep 2026 22:19:06 -0300 Subject: [PATCH 3/5] fix(s3): drop the DeleteBucket escape and clear a refusal when the name is reused Review of the two commits before this one found the pair combining into data loss, and the containment I had added for it aiming at the wrong file. Cutting the escape rather than patching it again. The refusal was recorded at load and never invalidated. Requiring the data to still be there let a create through once the directory was removed -- and that create no longer called `delete_bucket`, which had been the only thing dropping the refusal. So a healthy, live bucket ended up owning a name still in the refused set, and then: - any second account's DeleteBucket missed the per-account memory probe, found the stale refusal, and recursively deleted the live bucket's directory, with neither the emptiness check nor the ownership check the normal path runs; - a later `/_fakecloud/reset` refused the create for a bucket that read fine. A successful create now clears the refusal, which is the root cause. And the escape is gone: DeleteBucket could check neither emptiness (the objects are exactly what could not be read) nor ownership (the metadata carrying it is what failed), so it was a verb that discards retained data for any caller. The Object Lock guard added to contain that probed only the bucket-level `object_lock.toml`, so it missed per-object retention while wrongly blocking an empty lock-enabled bucket. Removing the directory frees the name with no restart, so the escape was never load-bearing -- an operator whose store the loader cannot read is already in the data path. - the refusal message carries `BucketName`, as AWS does with that code, and has its line continuations back (they had been mangled into runs of spaces, which no test caught because the assertion only checked the error code) - `bucket_subresource_exists` goes with its only caller - the two e2e tests written for the escape go with it; the surviving one now pins the exact 409 code, that the create leaves no refusal behind, and that a delete of a name absent from memory is a plain NoSuchBucket --- crates/fakecloud-e2e/tests/s3_persistence.rs | 156 +++++-------------- crates/fakecloud-persistence/src/s3.rs | 33 ++-- crates/fakecloud-s3/src/service/buckets.rs | 87 ++++------- website/content/docs/services/s3.md | 2 +- 4 files changed, 84 insertions(+), 194 deletions(-) diff --git a/crates/fakecloud-e2e/tests/s3_persistence.rs b/crates/fakecloud-e2e/tests/s3_persistence.rs index 8cd218221..4391d80e7 100644 --- a/crates/fakecloud-e2e/tests/s3_persistence.rs +++ b/crates/fakecloud-e2e/tests/s3_persistence.rs @@ -1756,9 +1756,15 @@ async fn persistence_removing_a_refused_directory_frees_the_name_without_a_resta server.restart().await; let client = server.s3_client().await; + let err = client + .create_bucket() + .bucket("gone") + .send() + .await + .expect_err("create must be refused while the data is there"); assert!( - client.create_bucket().bucket("gone").send().await.is_err(), - "create must be refused while the data is there" + format!("{err:?}").contains("BucketAlreadyExists"), + "unexpected error: {err:?}" ); // Same running server, no restart. @@ -1769,141 +1775,55 @@ async fn persistence_removing_a_refused_directory_frees_the_name_without_a_resta .send() .await .expect("the name is free once the data is gone, restart or not"); -} - -#[tokio::test] -async fn persistence_delete_will_not_discard_a_refused_bucket_under_object_lock() { - // The normal delete path enforces Object Lock only indirectly: a retained - // object keeps the bucket non-empty, so the delete is refused. The escape - // hatch cannot run that check -- the objects are what could not be read -- - // so it must refuse outright rather than letting a reflexive `rb` destroy - // retained data. - let tmp = tempfile::tempdir().unwrap(); - let mut server = TestServer::start_persistent(tmp.path()).await; - let client = server.s3_client().await; - client - .create_bucket() - .bucket("locked") - .object_lock_enabled_for_bucket(true) - .send() - .await - .unwrap(); + // ...and that create has to leave no refusal behind. The refusal is recorded + // at load and consulted long afterwards, so a name whose unreadable data is + // gone must stop being refused -- otherwise the next reset refuses a create + // for a bucket that reads perfectly. client .put_object() - .bucket("locked") - .key("k.txt") - .body(ByteStream::from_static(b"retained")) + .bucket("gone") + .key("fresh.txt") + .body(ByteStream::from_static(b"fresh")) .send() .await .unwrap(); - let bucket_dir = tmp.path().join("s3").join("buckets").join("locked"); - assert!( - bucket_dir.join("object_lock.toml").exists(), - "expected the lock config to be persisted" - ); - std::fs::write( - bucket_dir.join("objects").join("k.txt").join("null.toml"), - "not valid toml = = =", - ) - .unwrap(); - - server.restart().await; - let client = server.s3_client().await; - - let err = client - .delete_bucket() - .bucket("locked") - .send() - .await - .expect_err("a refused bucket under Object Lock must not be discarded by DeleteBucket"); - assert!( - format!("{err:?}").contains("BucketNotEmpty"), - "unexpected error: {err:?}" - ); - assert!( - bucket_dir.join("objects").join("k.txt").exists(), - "the refused delete destroyed the objects it was protecting" - ); - - // Repairing the bad file is the way out, and the bucket comes back locked. - std::fs::remove_dir_all(bucket_dir.join("objects").join("k.txt")).unwrap(); - server.restart().await; - let client = server.s3_client().await; - let lock = client - .get_object_lock_configuration() - .bucket("locked") + let status = reqwest::Client::new() + .post(format!("{}/_fakecloud/reset/s3", server.endpoint())) .send() .await - .expect("the repaired bucket should load with its lock configuration"); - assert_eq!( - lock.object_lock_configuration() - .and_then(|c| c.object_lock_enabled()) - .map(|e| e.as_str()), - Some("Enabled") - ); -} - -#[tokio::test] -async fn persistence_delete_frees_a_load_refused_name() { - // Without an in-band escape a refused name is both missing (HeadBucket 404, - // absent from ListBuckets) and un-creatable, leaving no way out but editing - // the data path by hand. DeleteBucket is the explicitly destructive verb, so - // it is the one that may discard the unreadable directory. - let tmp = tempfile::tempdir().unwrap(); - let mut server = TestServer::start_persistent(tmp.path()).await; + .expect("reset should respond") + .status(); + assert!(status.is_success(), "reset failed: {status}"); let client = server.s3_client().await; - client .create_bucket() - .bucket("escape") + .bucket("gone") .send() .await - .unwrap(); - client - .put_object() - .bucket("escape") - .key("k.txt") - .body(ByteStream::from_static(b"v")) + .expect("a name that now holds a healthy bucket must not still be refused"); + + // A delete of a name absent from memory is a plain 404, never a recursive + // discard of whatever the store holds under it: with a stale refusal in the + // set, that path destroyed a live bucket's objects. + let status = reqwest::Client::new() + .post(format!("{}/_fakecloud/reset/s3", server.endpoint())) .send() .await - .unwrap(); - std::fs::write( - tmp.path() - .join("s3") - .join("buckets") - .join("escape") - .join("objects") - .join("k.txt") - .join("null.toml"), - "not valid toml = = =", - ) - .unwrap(); - - server.restart().await; + .expect("reset should respond") + .status(); + assert!(status.is_success(), "reset failed: {status}"); let client = server.s3_client().await; - - assert!( - client - .create_bucket() - .bucket("escape") - .send() - .await - .is_err(), - "create must be refused first" - ); - client + let err = client .delete_bucket() - .bucket("escape") - .send() - .await - .expect("delete must clear a load-refused bucket"); - client - .create_bucket() - .bucket("escape") + .bucket("gone") .send() .await - .expect("the name must be usable after the delete"); + .expect_err("a bucket absent from memory does not exist as far as delete is concerned"); + assert!( + format!("{err:?}").contains("NoSuchBucket"), + "unexpected error: {err:?}" + ); } #[tokio::test] diff --git a/crates/fakecloud-persistence/src/s3.rs b/crates/fakecloud-persistence/src/s3.rs index 8cfbcfb58..c2c6ba888 100644 --- a/crates/fakecloud-persistence/src/s3.rs +++ b/crates/fakecloud-persistence/src/s3.rs @@ -389,16 +389,6 @@ pub trait S3Store: Send + Sync { false } - /// Whether the store holds this one subresource for `bucket`. - /// - /// Readable without the bucket itself being loadable, which is what makes it - /// usable on a load-refused bucket: the file the loader choked on is rarely - /// the one being asked about. Memory-only stores hold nothing, hence the - /// default. - fn bucket_subresource_exists(&self, _bucket: &str, _kind: BucketSubresource) -> bool { - false - } - /// Whether the last [`S3Store::load`] REFUSED this bucket -- a corrupt /// object meta, a missing part body: data still on disk and recoverable by /// repairing the one bad file. @@ -409,6 +399,16 @@ pub trait S3Store: Send + Sync { false } + /// Forget that [`S3Store::load`] refused `bucket`, because the name now + /// belongs to a bucket that loaded. + /// + /// The refusal is recorded at load and consulted long afterwards, so it has + /// to be dropped when it stops being true, or a name whose unreadable data + /// is gone stays refused for the life of the process. + fn clear_bucket_load_refusal(&self, _bucket: &str) -> StoreResult<()> { + Ok(()) + } + fn put_object( &self, bucket: &str, @@ -953,18 +953,19 @@ impl S3Store for DiskS3Store { self.bucket_dir(bucket).exists() } - fn bucket_subresource_exists(&self, bucket: &str, kind: BucketSubresource) -> bool { - self.bucket_dir(bucket) - .join(Self::subresource_filename(kind)) - .exists() - } - fn bucket_load_refused(&self, bucket: &str) -> bool { self.load_refused .read() .contains(&crate::key_escape::escape_key_segment(bucket)) } + fn clear_bucket_load_refusal(&self, bucket: &str) -> StoreResult<()> { + self.load_refused + .write() + .remove(&crate::key_escape::escape_key_segment(bucket)); + Ok(()) + } + fn delete_bucket(&self, bucket: &str) -> StoreResult<()> { let dir = self.bucket_dir(bucket); let outcome = match std::fs::remove_dir_all(&dir) { diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index 53edbb757..fb54c8856 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -332,14 +332,19 @@ impl S3Service { // A bucket the loader could not read is absent from memory, so its name // looks free -- but its objects are on disk and recoverable by repairing // the one bad file, and this create is about to clear the directory. - // Refuse instead, and say how to get the name back. DeleteBucket is the - // in-band escape (it is the explicitly destructive verb), so the name is - // never permanently stuck. + // Refuse instead, and say how to get the name back. Both ways out are in + // the data path, where an operator with an unreadable store already is: + // repair the one bad file and restart, or remove the directory, which + // frees the name immediately (the check below is on the data still being + // there, not on the refusal alone). There is deliberately no API verb + // that discards it -- DeleteBucket cannot check emptiness here, since the + // objects are exactly what could not be read, nor ownership, since the + // metadata carrying it is what failed. // `bucket_state_exists` as well as the refusal: the refusal is recorded - // at load and never re-probed, so an operator who followed the advice - // below by deleting the directory outright -- without restarting -- would - // otherwise find the name refused for the rest of the process lifetime, - // with an error telling them to repair something that is gone. + // at load, so an operator who took the second way out below -- removing + // the directory, without restarting -- would otherwise find the name + // refused for the rest of the process lifetime, with an error telling + // them to repair something that is gone. if self.store.bucket_load_refused(bucket) && self.store.bucket_state_exists(bucket) && !accts @@ -352,15 +357,16 @@ impl S3Service { "CreateBucket refused: the store holds data for this bucket that could not be \ read at load", ); - return Err(AwsServiceError::aws_error( + return Err(AwsServiceError::aws_error_with_fields( StatusCode::CONFLICT, "BucketAlreadyExists", format!( "The requested bucket name is not available: {bucket} holds persisted data \ that could not be read at load (an unreadable object, or a delete that \ - stopped partway). Repair its directory in the data path and restart, or \ - DeleteBucket to discard it." + stopped partway). Repair its directory in the data path and restart to get \ + the bucket back, or remove the directory to free the name." ), + vec![("BucketName".to_string(), bucket.to_string())], )); } // Check global uniqueness across all accounts before creating @@ -537,6 +543,14 @@ impl S3Service { b.ownership_controls.as_deref(), )?; state.buckets.insert(bucket.to_string(), b); + // This name now belongs to a bucket that loaded, so whatever the last + // load could not read under it is gone (either cleared above, or removed + // out of band, which is what let the create through at all). Leaving the + // refusal behind would refuse the name again after the next + // `/_fakecloud/reset`, for data that is no longer there. + self.store + .clear_bucket_load_refusal(bucket) + .map_err(super::persistence_error)?; let mut headers = HeaderMap::new(); headers.insert("location", format!("/{bucket}").parse().unwrap()); @@ -560,55 +574,10 @@ impl S3Service { ) -> Result { let mut accts = self.state.write(); let state = accts.get_or_create(account_id); - let Some(b) = state.buckets.get(bucket) else { - // A bucket the loader refused is absent from memory, so every read - // reports it missing while CreateBucket refuses its name. Delete is - // the explicitly destructive verb, so it is the way out: discard the - // unreadable directory and free the name. No emptiness check is - // possible here -- the objects are exactly what could not be read -- - // and no owner check either, since the metadata carrying ownership is - // what failed, so any caller may clear it. - if self.store.bucket_load_refused(bucket) { - // Object Lock is the one thing the normal path enforces that - // this branch cannot: a compliance-retained object is - // undeletable because the bucket is never empty, and here the - // objects are exactly what could not be read. A reflexive - // `aws s3 rb` -- or a retry of the tool that just got the 409 -- - // would otherwise destroy retained data, which no bucket on real - // S3 permits. The lock config is its own file and is usually - // readable even when an object's is not, so refuse while it is - // there and leave that case to the operator's own hands. - if self.store.bucket_subresource_exists( - bucket, - fakecloud_persistence::BucketSubresource::ObjectLock, - ) { - return Err(AwsServiceError::aws_error( - StatusCode::CONFLICT, - "BucketNotEmpty", - format!( - "{bucket} could not be read at load and has an Object Lock configuration, so its contents cannot be shown to be free of retention. Repair its directory in the data path and restart, then delete it through the normal path." - ), - )); - } - tracing::warn!( - target: "fakecloud::s3", - bucket = %bucket, - account_id = %account_id, - "DeleteBucket discarding data that could not be read at load; ownership could \ - not be verified, so this is reachable by any account", - ); - self.store - .delete_bucket(bucket) - .map_err(super::persistence_error)?; - return Ok(AwsResponse { - status: StatusCode::NO_CONTENT, - content_type: "application/xml".to_string(), - body: Bytes::new().into(), - headers: HeaderMap::new(), - }); - } - return Err(no_such_bucket(bucket)); - }; + let b = state + .buckets + .get(bucket) + .ok_or_else(|| no_such_bucket(bucket))?; // Bucket must be empty to delete (no objects and no versions) let has_real_objects = b.objects.values().any(|o| !o.is_delete_marker); let has_versions = b.object_versions.values().any(|v| !v.is_empty()); diff --git a/website/content/docs/services/s3.md b/website/content/docs/services/s3.md index 9b76632d9..bb23748b7 100644 --- a/website/content/docs/services/s3.md +++ b/website/content/docs/services/s3.md @@ -47,7 +47,7 @@ REST. Path-based routing (`/bucket/key`), HTTP method + query string for actions - In persistent mode, object bodies stream to disk with a bounded LRU cache (`--s3-cache-size`, default 256 MiB). Objects larger than `cache-size / 2` bypass the cache. - The `/_fakecloud/s3/notifications` introspection buffer is intentionally not persisted across restarts. -- In persistent mode, `CreateBucket` clears whatever the store still holds under that name, so a re-created bucket is genuinely empty rather than inheriting the previous incarnation's objects on the next load. That matters after `/_fakecloud/reset`, which clears in-memory state and deliberately leaves the store alone. The exception is a bucket the loader could not read (a corrupt object sidecar, a missing multipart part body): it is skipped at load with a warning, so it is absent from `ListBuckets` while its data sits intact on disk, and `CreateBucket` refuses the name with `BucketAlreadyExists` rather than becoming the thing that destroys it. Repair the file and restart to get the bucket back, or `DeleteBucket` to discard it and free the name -- that delete skips the usual empty-bucket check and any ownership check, since the objects and the metadata carrying ownership are exactly what could not be read. Two edges worth knowing: a refused bucket that carries an Object Lock configuration is **not** deletable that way (its contents cannot be shown to be free of retention, so the repair is the only way out), and every read still reports the name missing -- `HeadBucket` 404s and `ListBuckets` omits it -- while the create refuses it, so a head-then-create script fails on the create rather than silently overwriting the data. Removing the directory yourself frees the name immediately, with no restart needed. +- In persistent mode, `CreateBucket` clears whatever the store still holds under that name, so a re-created bucket is genuinely empty rather than inheriting the previous incarnation's objects on the next load. That matters after `/_fakecloud/reset`, which clears in-memory state and deliberately leaves the store alone. The exception is a bucket the loader could not read (a corrupt object sidecar, a missing multipart part body): it is skipped at load with a warning, so it is absent from `ListBuckets` while its data sits intact on disk, and `CreateBucket` refuses the name with `BucketAlreadyExists` rather than becoming the thing that destroys it. Both ways out are in the data path, where an operator with an unreadable store already is: repair the one bad file and restart to get the bucket back, or remove the directory to free the name -- that frees it immediately, with no restart needed, and a later `CreateBucket` for it succeeds. There is deliberately no API call that discards it: `DeleteBucket` could check neither emptiness (the objects are exactly what could not be read) nor ownership (the metadata carrying it is what failed), so it would be a verb that destroys retained data for any caller. It answers `NoSuchBucket` like any other name it cannot see. Every read agrees with that -- `HeadBucket` 404s and `ListBuckets` omits the name -- while the create refuses it, so a head-then-create script fails on the create rather than silently overwriting the data. - SigV4 signatures are parsed for request routing but never validated. ## Source From 3fab240bc64ff805a006667bd5c490ad3f2e4977 Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Mon, 28 Sep 2026 05:19:10 -0300 Subject: [PATCH 4/5] fix(s3): refuse a name whose BUCKET sidecar could not be read, not just its objects Two layers can skip a bucket at load, and only one of them was recording a refusal. The store skips a bucket whose OBJECTS it cannot read. The layer above parses the bucket's own sidecars, and an unparseable `tags.toml`, `acl.toml` or `inventory.toml` skips it there -- with the store reporting no trouble at all, because the objects read fine. That layer already hands each skipped bucket to a `refused` callback, and its doc comment says the bucket is then "reported exactly like a store-level refusal". `main.rs` passed `|_, _| {}`. So the name looked free, and this branch's whole-directory clear deleted objects that were never unreadable -- where before the branch only the sidecars were swept, leaving `objects/` to be recovered by repairing the one bad file. Wiring the callback to a new `S3Store::mark_bucket_load_refused` is what the hook was written for. Also from the same review: - The refusal clear moves ahead of the writes. A create that failed partway -- after `put_bucket_meta`, on a subresource -- left the refusal standing over a directory that create had just made, and every later create for the name was told to repair a directory holding nothing but that failed attempt's `meta.toml`. It is only reached once the refusal is known not to apply, so dropping it there cannot discard a live one. Gated on a read first, to keep a write lock off the ordinary create path. - The trailing assertion added last commit could not fail: nothing had left a stale refusal by that point, so re-pasting the deleted DeleteBucket escape verbatim kept it green. The property now sits on the genuinely refused bucket, asserting both the `NoSuchBucket` and that its objects survive -- verified by re-adding the escape, which fails it. - Two doc comments the earlier cut falsified: the hydrate hook still advertised "clearable by DeleteBucket", and the docs implied only object-level corruption is protected. (The previous commit message credited the mangled-literal fix to the wrong string: it was the `BucketNotEmpty` Object Lock message, which that commit deleted, not the `BucketAlreadyExists` one.) --- crates/fakecloud-e2e/tests/s3_persistence.rs | 139 ++++++++++++++++--- crates/fakecloud-persistence/src/s3.rs | 14 ++ crates/fakecloud-s3/src/persistence.rs | 9 +- crates/fakecloud-s3/src/service/buckets.rs | 25 ++-- crates/fakecloud-server/src/main.rs | 12 +- website/content/docs/services/s3.md | 2 +- 6 files changed, 168 insertions(+), 33 deletions(-) diff --git a/crates/fakecloud-e2e/tests/s3_persistence.rs b/crates/fakecloud-e2e/tests/s3_persistence.rs index 4391d80e7..9a9740d2d 100644 --- a/crates/fakecloud-e2e/tests/s3_persistence.rs +++ b/crates/fakecloud-e2e/tests/s3_persistence.rs @@ -1708,6 +1708,26 @@ async fn persistence_recreating_a_load_refused_bucket_is_declined_not_destructiv "the refused create destroyed the objects it was protecting" ); + // Nor does DeleteBucket discard it. There is deliberately no API verb that + // does: delete could check neither emptiness (the objects are exactly what + // could not be read) nor ownership (the metadata carrying it is what + // failed), so a refused name answers NoSuchBucket like any other name the + // server cannot see, and the data stays for the operator to repair. + let err = client + .delete_bucket() + .bucket("refused") + .send() + .await + .expect_err("a refused bucket must not be discardable through the API"); + assert!( + format!("{err:?}").contains("NoSuchBucket"), + "unexpected error: {err:?}" + ); + assert!( + objects_dir.join("keep.txt").join("null.bin").exists(), + "DeleteBucket destroyed the objects the refusal was protecting" + ); + // Repairing the one bad file brings the whole bucket back. std::fs::remove_dir_all(objects_dir.join("corrupt.txt")).unwrap(); server.restart().await; @@ -1727,6 +1747,103 @@ async fn persistence_recreating_a_load_refused_bucket_is_declined_not_destructiv assert_eq!(&body[..], b"precious"); } +#[tokio::test] +async fn persistence_a_bucket_skipped_for_a_corrupt_sidecar_is_refused_not_cleared() { + // Two layers can skip a bucket at load. The store skips one whose OBJECTS it + // cannot read. A layer above parses the bucket's own sidecars, and a bad + // `tags.toml` skips the bucket there -- with the store reporting no problem + // at all, since the objects read fine. Only that layer's report keeps the + // name from looking free, and a create clears the whole directory, so + // without it the create deletes objects that were never unreadable. + let tmp = tempfile::tempdir().unwrap(); + let mut server = TestServer::start_persistent(tmp.path()).await; + let client = server.s3_client().await; + + client + .create_bucket() + .bucket("sidecar") + .send() + .await + .unwrap(); + client + .put_object() + .bucket("sidecar") + .key("keep.txt") + .body(ByteStream::from_static(b"precious")) + .send() + .await + .unwrap(); + client + .put_bucket_tagging() + .bucket("sidecar") + .tagging( + aws_sdk_s3::types::Tagging::builder() + .tag_set( + aws_sdk_s3::types::Tag::builder() + .key("team") + .value("a") + .build() + .unwrap(), + ) + .build() + .unwrap(), + ) + .send() + .await + .unwrap(); + + let bucket_dir = tmp.path().join("s3").join("buckets").join("sidecar"); + let tags = bucket_dir.join("tags.toml"); + assert!(tags.exists(), "expected {tags:?}"); + std::fs::write(&tags, "not valid toml = = =").unwrap(); + + server.restart().await; + let client = server.s3_client().await; + + let list = client.list_buckets().send().await.unwrap(); + assert!( + !list.buckets().iter().any(|b| b.name() == Some("sidecar")), + "expected the bucket to be skipped on load" + ); + + let err = client + .create_bucket() + .bucket("sidecar") + .send() + .await + .expect_err("the name must be refused while the objects are still there"); + assert!( + format!("{err:?}").contains("BucketAlreadyExists"), + "unexpected error: {err:?}" + ); + assert!( + bucket_dir + .join("objects") + .join("keep.txt") + .join("null.bin") + .exists(), + "the create destroyed objects the loader had no trouble reading" + ); + + // Repairing the one bad file brings the bucket back, objects included. + std::fs::remove_file(&tags).unwrap(); + server.restart().await; + let client = server.s3_client().await; + let body = client + .get_object() + .bucket("sidecar") + .key("keep.txt") + .send() + .await + .expect("the repaired bucket should load") + .body + .collect() + .await + .unwrap() + .into_bytes(); + assert_eq!(&body[..], b"precious"); +} + #[tokio::test] async fn persistence_removing_a_refused_directory_frees_the_name_without_a_restart() { // The refusal is recorded at load and never re-probed, so it has to be @@ -1802,28 +1919,6 @@ async fn persistence_removing_a_refused_directory_frees_the_name_without_a_resta .send() .await .expect("a name that now holds a healthy bucket must not still be refused"); - - // A delete of a name absent from memory is a plain 404, never a recursive - // discard of whatever the store holds under it: with a stale refusal in the - // set, that path destroyed a live bucket's objects. - let status = reqwest::Client::new() - .post(format!("{}/_fakecloud/reset/s3", server.endpoint())) - .send() - .await - .expect("reset should respond") - .status(); - assert!(status.is_success(), "reset failed: {status}"); - let client = server.s3_client().await; - let err = client - .delete_bucket() - .bucket("gone") - .send() - .await - .expect_err("a bucket absent from memory does not exist as far as delete is concerned"); - assert!( - format!("{err:?}").contains("NoSuchBucket"), - "unexpected error: {err:?}" - ); } #[tokio::test] diff --git a/crates/fakecloud-persistence/src/s3.rs b/crates/fakecloud-persistence/src/s3.rs index c2c6ba888..704fba06d 100644 --- a/crates/fakecloud-persistence/src/s3.rs +++ b/crates/fakecloud-persistence/src/s3.rs @@ -399,6 +399,14 @@ pub trait S3Store: Send + Sync { false } + /// Record that `bucket` could not be loaded, for a reason found ABOVE the + /// store: its objects read fine, but a sidecar this layer does not parse + /// (`tags.toml`, `acl.toml`, `inventory.toml`) did not. The caller hydrating + /// a snapshot reports each such bucket here so it is refused exactly like a + /// store-level refusal -- otherwise its name looks free and the next + /// `CreateBucket` clears the directory, objects included. + fn mark_bucket_load_refused(&self, _bucket: &str) {} + /// Forget that [`S3Store::load`] refused `bucket`, because the name now /// belongs to a bucket that loaded. /// @@ -959,6 +967,12 @@ impl S3Store for DiskS3Store { .contains(&crate::key_escape::escape_key_segment(bucket)) } + fn mark_bucket_load_refused(&self, bucket: &str) { + self.load_refused + .write() + .insert(crate::key_escape::escape_key_segment(bucket)); + } + fn clear_bucket_load_refusal(&self, bucket: &str) -> StoreResult<()> { self.load_refused .write() diff --git a/crates/fakecloud-s3/src/persistence.rs b/crates/fakecloud-s3/src/persistence.rs index 40a4306aa..502cdfbf6 100644 --- a/crates/fakecloud-s3/src/persistence.rs +++ b/crates/fakecloud-s3/src/persistence.rs @@ -480,7 +480,14 @@ pub fn hydrate_s3_state( /// malformed `acl.toml` or `tags.toml` used to abort startup -- every other /// bucket inaccessible because of one file. Such a bucket is skipped and handed /// to `refused` so it is reported exactly like a store-level refusal: absent -/// from memory, its name refused by CreateBucket, and clearable by DeleteBucket. +/// from memory, and its name refused by CreateBucket rather than treated as free +/// -- a create clears the whole stored directory, so a name that is merely +/// unreadable must not look available. Repair the file and restart, or remove +/// the directory, to get the name back. +/// +/// Wire this to the store's `mark_bucket_load_refused`, not to a no-op: with the +/// hook dropped on the floor, a bucket skipped here is indistinguishable from a +/// name nobody has used, and the next create deletes its objects. pub fn hydrate_s3_state_reporting( snapshot: S3StateSnapshot, account_id: &str, diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index fb54c8856..dca17469b 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -522,6 +522,23 @@ impl S3Service { .delete_bucket(bucket) .map_err(super::persistence_error)?; } + // This name now belongs to a bucket that loads, so whatever the last load + // could not read under it is gone (either cleared just above, or removed + // out of band, which is what let the create past the refusal at all). + // Leaving the refusal behind would refuse the name again after the next + // `/_fakecloud/reset`, for data that is no longer there. + // + // Before the writes below, not after: a create that fails partway would + // otherwise leave the refusal standing over a directory it had just + // created, and every later create for the name would be told to repair a + // directory holding nothing but that failed attempt's `meta.toml`. Only + // reached once the refusal is known not to apply, so dropping it here + // cannot discard a live one. + if self.store.bucket_load_refused(bucket) { + self.store + .clear_bucket_load_refusal(bucket) + .map_err(super::persistence_error)?; + } self.store .put_bucket_meta(bucket, &meta) .map_err(super::persistence_error)?; @@ -543,14 +560,6 @@ impl S3Service { b.ownership_controls.as_deref(), )?; state.buckets.insert(bucket.to_string(), b); - // This name now belongs to a bucket that loaded, so whatever the last - // load could not read under it is gone (either cleared above, or removed - // out of band, which is what let the create through at all). Leaving the - // refusal behind would refuse the name again after the next - // `/_fakecloud/reset`, for data that is no longer there. - self.store - .clear_bucket_load_refusal(bucket) - .map_err(super::persistence_error)?; let mut headers = HeaderMap::new(); headers.insert("location", format!("/{bucket}").parse().unwrap()); diff --git a/crates/fakecloud-server/src/main.rs b/crates/fakecloud-server/src/main.rs index d299e6549..c3af3e262 100644 --- a/crates/fakecloud-server/src/main.rs +++ b/crates/fakecloud-server/src/main.rs @@ -2539,10 +2539,20 @@ async fn main() { let bucket_count = snapshot.buckets.len(); let object_count: usize = snapshot.buckets.values().map(|b| b.objects.len()).sum(); - let hydrated = match fakecloud_s3::persistence::hydrate_s3_state( + // Report each bucket the sidecar layer cannot read into the + // store's refusal set. Its objects loaded fine, so nothing + // below this layer knows the bucket is unusable -- and a name + // absent from both memory and that set is one CreateBucket + // clears the directory of, objects included. + let mut refused = |bucket: &str, _err: &str| { + :: + mark_bucket_load_refused(&disk, bucket); + }; + let hydrated = match fakecloud_s3::persistence::hydrate_s3_state_reporting( snapshot, &cli.account_id, &cli.region, + &mut refused, ) { Ok(h) => h, Err(err) => fatal_exit(format_args!( diff --git a/website/content/docs/services/s3.md b/website/content/docs/services/s3.md index bb23748b7..dc8c402e3 100644 --- a/website/content/docs/services/s3.md +++ b/website/content/docs/services/s3.md @@ -47,7 +47,7 @@ REST. Path-based routing (`/bucket/key`), HTTP method + query string for actions - In persistent mode, object bodies stream to disk with a bounded LRU cache (`--s3-cache-size`, default 256 MiB). Objects larger than `cache-size / 2` bypass the cache. - The `/_fakecloud/s3/notifications` introspection buffer is intentionally not persisted across restarts. -- In persistent mode, `CreateBucket` clears whatever the store still holds under that name, so a re-created bucket is genuinely empty rather than inheriting the previous incarnation's objects on the next load. That matters after `/_fakecloud/reset`, which clears in-memory state and deliberately leaves the store alone. The exception is a bucket the loader could not read (a corrupt object sidecar, a missing multipart part body): it is skipped at load with a warning, so it is absent from `ListBuckets` while its data sits intact on disk, and `CreateBucket` refuses the name with `BucketAlreadyExists` rather than becoming the thing that destroys it. Both ways out are in the data path, where an operator with an unreadable store already is: repair the one bad file and restart to get the bucket back, or remove the directory to free the name -- that frees it immediately, with no restart needed, and a later `CreateBucket` for it succeeds. There is deliberately no API call that discards it: `DeleteBucket` could check neither emptiness (the objects are exactly what could not be read) nor ownership (the metadata carrying it is what failed), so it would be a verb that destroys retained data for any caller. It answers `NoSuchBucket` like any other name it cannot see. Every read agrees with that -- `HeadBucket` 404s and `ListBuckets` omits the name -- while the create refuses it, so a head-then-create script fails on the create rather than silently overwriting the data. +- In persistent mode, `CreateBucket` clears whatever the store still holds under that name, so a re-created bucket is genuinely empty rather than inheriting the previous incarnation's objects on the next load. That matters after `/_fakecloud/reset`, which clears in-memory state and deliberately leaves the store alone. The exception is a bucket the loader could not read -- either its objects (a corrupt object sidecar, a missing multipart part body) or its own stored configuration (an unparseable `tags.toml`, `acl.toml` or `inventory.toml`): it is skipped at load with a warning, so it is absent from `ListBuckets` while its data sits intact on disk, and `CreateBucket` refuses the name with `BucketAlreadyExists` rather than becoming the thing that destroys it. Both ways out are in the data path, where an operator with an unreadable store already is: repair the one bad file and restart to get the bucket back, or remove the directory to free the name -- that frees it immediately, with no restart needed, and a later `CreateBucket` for it succeeds. There is deliberately no API call that discards it: `DeleteBucket` could check neither emptiness (the objects are exactly what could not be read) nor ownership (the metadata carrying it is what failed), so it would be a verb that destroys retained data for any caller. It answers `NoSuchBucket` like any other name it cannot see. Every read agrees with that -- `HeadBucket` 404s and `ListBuckets` omits the name -- while the create refuses it, so a head-then-create script fails on the create rather than silently overwriting the data. - SigV4 signatures are parsed for request routing but never validated. ## Source From dc3907ff9c3d73dc0398485d44b3f768edc5355c Mon Sep 17 00:00:00 2001 From: Lucas Vieira Date: Tue, 29 Sep 2026 08:18:22 -0300 Subject: [PATCH 5/5] fix(s3): say which file was unreadable, and delete the no-op hydrate wrapper Review of the commit before this one. No correctness bug, but the text an operator reads was left describing the narrower behavior: - The `CreateBucket` refusal named only "an unreadable object, or a delete that stopped partway" -- while this branch is precisely what widened refusals to a bucket's OWN files. Someone hitting it for a corrupt `tags.toml` was sent hunting through `objects/`. It now names both kinds and points at the log for which file it was. - `bucket_load_refused`'s doc still said the set holds what `load` refused, eight lines above the method that inserts entries `load` never saw. - The loader's warning still offered "or the bucket is deleted" -- the in-band escape removed two commits ago. It now says to remove the directory, and that no API call discards it. - The hydrate-layer warning did not mention that the name is now refused, unlike the store-layer warning for the same outcome. `hydrate_s3_state` is deleted. Its own sibling's doc says "wire this to the store's `mark_bucket_load_refused`, not to a no-op: with the hook dropped on the floor, a bucket skipped here is indistinguishable from a name nobody has used, and the next create deletes its objects" -- and that wrapper WAS the no-op, exported from a published crate as the convenient default. After this branch its only caller was a unit test in the same file, which now passes `&mut |_, _| {}` explicitly, so the choice to discard refusals is visible at the call site instead of hidden behind a shorter name. This is the hole that shipped in #2558 and only became destructive when the whole-directory clear landed; removing the shape stops it recurring. Plus the unit test the wiring never had. Its correctness rests on `escape_key_segment(bucket_name)` matching the on-disk directory name, because `load` inserts the directory name while the sidecar layer inserts an escaped bucket name -- and both e2e tests use names that escape to themselves, so a key-space mismatch would pass them. `a_refusal_is_found_under_the_escaped_directory_name` uses `odd:name`, which does not, and is verified failing: drop the escaping from `mark_bucket_load_refused` and the mark is no longer found. --- crates/fakecloud-persistence/src/s3.rs | 58 +++++++++++++++++++--- crates/fakecloud-s3/src/persistence.rs | 19 ++++--- crates/fakecloud-s3/src/service/buckets.rs | 7 +-- 3 files changed, 64 insertions(+), 20 deletions(-) diff --git a/crates/fakecloud-persistence/src/s3.rs b/crates/fakecloud-persistence/src/s3.rs index 704fba06d..426d2c201 100644 --- a/crates/fakecloud-persistence/src/s3.rs +++ b/crates/fakecloud-persistence/src/s3.rs @@ -389,9 +389,14 @@ pub trait S3Store: Send + Sync { false } - /// Whether the last [`S3Store::load`] REFUSED this bucket -- a corrupt - /// object meta, a missing part body: data still on disk and recoverable by - /// repairing the one bad file. + /// Whether this bucket was REFUSED at load: data still on disk and + /// recoverable by repairing the one bad file. + /// + /// Covers both layers that can refuse one. [`S3Store::load`] records the + /// buckets whose OBJECTS it could not read (a corrupt object meta, a missing + /// part body); the layer that parses a bucket's own files reports the rest + /// through [`S3Store::mark_bucket_load_refused`], since the store reads those + /// as opaque text and sees nothing wrong. /// /// Recorded at load, not probed per call, so a caller cannot confuse "the /// loader could not read this" with "this is simply not in memory". @@ -407,8 +412,8 @@ pub trait S3Store: Send + Sync { /// `CreateBucket` clears the directory, objects included. fn mark_bucket_load_refused(&self, _bucket: &str) {} - /// Forget that [`S3Store::load`] refused `bucket`, because the name now - /// belongs to a bucket that loaded. + /// Forget that `bucket` was refused at load, because the name now belongs to + /// a bucket that loaded. /// /// The refusal is recorded at load and consulted long afterwards, so it has /// to be dropped when it stops being true, or a name whose unreadable data @@ -917,8 +922,9 @@ impl S3Store for DiskS3Store { tracing::warn!( bucket = %bdir.display(), error = %e, - "skipping unreadable S3 bucket during load; its name is refused until it \ - is repaired and the server restarted, or the bucket is deleted" + "skipping unreadable S3 bucket during load; its name is refused until \ + the bad file is repaired and the server restarted, or the bucket's \ + directory is removed -- no API call discards it" ); } } @@ -1400,6 +1406,44 @@ mod disk_tests { } } + /// The refusal set is written by two layers that name a bucket differently: + /// `load` inserts the on-disk directory name, while the sidecar layer above + /// the store hands `mark_bucket_load_refused` a bucket NAME. Both readers + /// escape. A name that escapes to itself -- which every name in the e2e + /// suites happens to be -- cannot tell the two key spaces apart, so pin it + /// with one that does not. + #[test] + fn a_refusal_is_found_under_the_escaped_directory_name() { + let tmp = TempDir::new().unwrap(); + let store = new_store(&tmp); + + let name = "odd:name"; + let escaped = crate::key_escape::escape_key_segment(name); + assert_ne!(escaped, name, "pick a name that actually escapes"); + + assert!(!store.bucket_load_refused(name)); + store.mark_bucket_load_refused(name); + assert!( + store.bucket_load_refused(name), + "marked by name, read back by name" + ); + + // The directory the store creates for it is the escaped one, so `load`'s + // raw-directory-name insert lands on the same key this reader uses. + let meta = BucketMeta { + name: name.to_string(), + ..Default::default() + }; + store.put_bucket_meta(name, &meta).unwrap(); + assert!( + tmp.path().join("buckets").join(&escaped).is_dir(), + "expected the bucket directory under the escaped name" + ); + + store.clear_bucket_load_refusal(name).unwrap(); + assert!(!store.bucket_load_refused(name)); + } + #[test] fn put_bucket_meta_roundtrip() { let tmp = TempDir::new().unwrap(); diff --git a/crates/fakecloud-s3/src/persistence.rs b/crates/fakecloud-s3/src/persistence.rs index 502cdfbf6..a2199451d 100644 --- a/crates/fakecloud-s3/src/persistence.rs +++ b/crates/fakecloud-s3/src/persistence.rs @@ -463,14 +463,6 @@ pub fn s3_bucket_from_snapshot( Ok(b) } -pub fn hydrate_s3_state( - snapshot: S3StateSnapshot, - account_id: &str, - region: &str, -) -> Result { - hydrate_s3_state_reporting(snapshot, account_id, region, &mut |_, _| {}) -} - /// Hydrate a loaded snapshot, reporting each bucket that cannot be used instead /// of failing the whole load. /// @@ -504,7 +496,9 @@ pub fn hydrate_s3_state_reporting( tracing::warn!( bucket = %name, error = %e, - "skipping S3 bucket whose stored configuration could not be read", + "skipping S3 bucket whose stored configuration could not be read; its name is \ + refused until the bad file is repaired and the server restarted, or the \ + bucket's directory is removed", ); refused(&name, &e); } @@ -662,7 +656,12 @@ permission = "READ" ..Default::default() }, ); - let state = hydrate_s3_state(snapshot, "123", "us-east-1").unwrap(); + // The no-op reporter is spelled out here on purpose: production must wire + // this to the store's `mark_bucket_load_refused`, and a convenience + // wrapper that defaulted to discarding refusals is what let the + // sidecar-refusal hole ship in the first place. + let state = + hydrate_s3_state_reporting(snapshot, "123", "us-east-1", &mut |_, _| {}).unwrap(); assert!(state.buckets.contains_key("b")); } } diff --git a/crates/fakecloud-s3/src/service/buckets.rs b/crates/fakecloud-s3/src/service/buckets.rs index dca17469b..458481eaa 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -362,9 +362,10 @@ impl S3Service { "BucketAlreadyExists", format!( "The requested bucket name is not available: {bucket} holds persisted data \ - that could not be read at load (an unreadable object, or a delete that \ - stopped partway). Repair its directory in the data path and restart to get \ - the bucket back, or remove the directory to free the name." + that could not be read at load -- an unreadable object, one of the bucket's \ + own files (tags.toml, acl.toml, inventory.toml), or a delete that stopped \ + partway. The server logged which file it was. Repair it in the data path and \ + restart to get the bucket back, or remove the directory to free the name." ), vec![("BucketName".to_string(), bucket.to_string())], ));