diff --git a/crates/fakecloud-e2e/tests/s3_persistence.rs b/crates/fakecloud-e2e/tests/s3_persistence.rs index fb6330302..9a9740d2d 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,54 @@ 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(), + "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(), - "re-creating a load-skipped bucket destroyed its objects" + "DeleteBucket destroyed the objects the refusal was protecting" ); // Repairing the one bad file brings the whole bucket back. @@ -1685,7 +1734,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 +1747,180 @@ async fn persistence_recreating_a_load_skipped_bucket_does_not_destroy_its_objec 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 + // 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; + let err = client + .create_bucket() + .bucket("gone") + .send() + .await + .expect_err("create must be refused while the data is there"); + assert!( + format!("{err:?}").contains("BucketAlreadyExists"), + "unexpected error: {err:?}" + ); + + // 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"); + + // ...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("gone") + .key("fresh.txt") + .body(ByteStream::from_static(b"fresh")) + .send() + .await + .unwrap(); + 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; + client + .create_bucket() + .bucket("gone") + .send() + .await + .expect("a name that now holds a healthy bucket must not still be refused"); +} + #[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..426d2c201 100644 --- a/crates/fakecloud-persistence/src/s3.rs +++ b/crates/fakecloud-persistence/src/s3.rs @@ -379,6 +379,49 @@ 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 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". + fn bucket_load_refused(&self, _bucket: &str) -> bool { + 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 `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 + /// 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, @@ -536,11 +579,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 +692,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 +912,21 @@ 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 \ + the bad file is repaired and the server restarted, or the bucket's \ + directory is removed -- no API call discards it" + ); + } } } Ok(state) @@ -899,13 +963,45 @@ 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 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() + .remove(&crate::key_escape::escape_key_segment(bucket)); + Ok(()) + } + 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( @@ -1310,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 40a4306aa..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. /// @@ -480,7 +472,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, @@ -497,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); } @@ -655,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 0fcb144e7..0ad38519f 100644 --- a/crates/fakecloud-s3/src/service/buckets.rs +++ b/crates/fakecloud-s3/src/service/buckets.rs @@ -330,6 +330,47 @@ 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. 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, 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 + .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_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, 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())], + )); + } // Check global uniqueness across all accounts before creating for (other_account_id, acct_state) in accts.iter() { if acct_state.buckets.contains_key(bucket) { @@ -457,33 +498,52 @@ 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. - 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. + // 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. // - // 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 { + // 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. + // + // 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_subresource(bucket, *kind) + .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)?; self.put_bucket_subresource_if_set( bucket, diff --git a/crates/fakecloud-server/src/main.rs b/crates/fakecloud-server/src/main.rs index bf12d272e..a97b64f45 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 8644b8211..56feceb21 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 -- 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