From e8a176abaf7075e2ac17fbeba6b82a59604dcd0e Mon Sep 17 00:00:00 2001 From: Sam Gbafa Date: Sat, 17 Oct 2026 11:21:37 +0000 Subject: [PATCH] fix(TC-1354): size S3 block deletes with HeadObject S3BlockStore::remove called GetObjectAttributes without an attribute list, which S3 and MinIO reject, so every S3 block delete errored. Use HeadObject for the size and existence check, as #336 does. A missing object is still Ok(None), and the quota drops by the stored length. Adds a MinIO-backed test run in the at-rest harness CI job. --- .github/workflows/rust.yml | 5 + tinycloud-node-server/src/storage/s3.rs | 81 ++++++++-------- tinycloud-node-server/tests/s3_block_store.rs | 97 +++++++++++++++++++ 3 files changed, 145 insertions(+), 38 deletions(-) create mode 100644 tinycloud-node-server/tests/s3_block_store.rs diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 74a82725..a25f3805 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -315,6 +315,11 @@ jobs: - name: TC-887 RED checks (must be RED until S3/S4) run: bash scripts/at-rest-harness/harness.sh test red --expect red "$TC887_STATE" + - name: S3 block store (against the harness MinIO) + run: | + (set -a; . "$TC887_RUN_DIR/harness.env"; set +a + cargo test --locked -p tinycloud-node --test s3_block_store -- --ignored) + - name: Harness logs if: failure() run: | diff --git a/tinycloud-node-server/src/storage/s3.rs b/tinycloud-node-server/src/storage/s3.rs index 8c88c07d..8f9abe27 100644 --- a/tinycloud-node-server/src/storage/s3.rs +++ b/tinycloud-node-server/src/storage/s3.rs @@ -1,8 +1,5 @@ use aws_sdk_s3::{ - error::{ - GetObjectAttributesError, GetObjectAttributesErrorKind, GetObjectError, GetObjectErrorKind, - HeadObjectError, HeadObjectErrorKind, - }, + error::{GetObjectError, GetObjectErrorKind, HeadObjectError, HeadObjectErrorKind}, types::{ByteStream, SdkError}, Client, // Config, Error as S3Error, @@ -119,6 +116,42 @@ impl S3BlockStore { async fn decrement_size(&self, space: &SpaceId, size: u64) { self.sizes.decrement_size(space, size).await; } + + /// The stored size of an object, or `None` if absent. HeadObject, not + /// GetObjectAttributes: that call needs an explicit attribute list, and + /// S3 (and MinIO) reject it without one. + async fn object_size(&self, key: &str) -> Result, S3StoreError> { + match self + .client + .head_object() + .bucket(&self.bucket) + .key(key) + .send() + .await + { + Ok(o) => Ok(Some(o.content_length().try_into()?)), + Err(SdkError::ServiceError { + err: + HeadObjectError { + kind: HeadObjectErrorKind::NotFound(_), + .. + }, + .. + }) => Ok(None), + Err(e) => Err(S3Error::from(e).into()), + } + } + + async fn delete(&self, key: &str) -> Result<(), S3StoreError> { + self.client + .delete_object() + .bucket(&self.bucket) + .key(key) + .send() + .await + .map_err(S3Error::from)?; + Ok(()) + } } pub fn convert(e: ByteStreamError) -> IoError { @@ -435,41 +468,13 @@ impl ImmutableWriteStore Result, Self::Error> { - let size: u64 = match self - .client - .get_object_attributes() - .bucket(&self.bucket) - .key(self.key(space, id)) - .send() - .await - { - Ok(o) if !o.delete_marker() => o.object_size().try_into()?, - Ok(_) => return Ok(None), - Err(SdkError::ServiceError { - err: - GetObjectAttributesError { - kind: GetObjectAttributesErrorKind::NoSuchKey(_), - .. - }, - .. - }) => return Ok(None), - Err(e) => return Err(S3Error::from(e).into()), + let key = self.key(space, id); + let Some(size) = self.object_size(&key).await? else { + return Ok(None); }; - match self - .client - .delete_object() - .bucket(&self.bucket) - .key(self.key(space, id)) - .send() - .await - { - Ok(_) => { - self.decrement_size(space, size).await; - Ok(Some(())) - } - // TODO does this distinguish between object missing and object present? - Err(e) => Err(S3Error::from(e).into()), - } + self.delete(&key).await?; + self.decrement_size(space, size).await; + Ok(Some(())) } } diff --git a/tinycloud-node-server/tests/s3_block_store.rs b/tinycloud-node-server/tests/s3_block_store.rs new file mode 100644 index 00000000..7860b194 --- /dev/null +++ b/tinycloud-node-server/tests/s3_block_store.rs @@ -0,0 +1,97 @@ +//! TC-1354: the S3 block store against a real S3 API (the native MinIO of +//! `scripts/at-rest-harness`). Ignored by default; run with the harness +//! environment loaded: +//! +//! ```sh +//! (set -a; . "$RUN_DIR/harness.env"; set +a; +//! cargo test -p tinycloud-node --test s3_block_store -- --ignored) +//! ``` + +use anyhow::{ensure, Context, Result}; +use futures::io::AsyncWriteExt; +use tinycloud::storage::s3::{S3BlockConfig, S3BlockStore, S3StoreError}; +use tinycloud_auth::resource::SpaceId; +use tinycloud_core::{ + hash::Hash, + storage::{ + memory::MemoryStaging, HashBuffer, ImmutableDeleteStore, ImmutableReadStore, + ImmutableWriteStore, StorageConfig, StorageSetup, StoreSize, + }, +}; + +fn env(name: &str) -> Result { + std::env::var(name).with_context(|| format!("{name} (load the harness env)")) +} + +async fn store() -> Result<(S3BlockStore, SpaceId)> { + // The S3 client reads credentials from the AWS_* environment. + std::env::set_var("AWS_ACCESS_KEY_ID", env("TC887_S3_ACCESS_KEY_ID")?); + std::env::set_var("AWS_SECRET_ACCESS_KEY", env("TC887_S3_SECRET_ACCESS_KEY")?); + std::env::set_var("AWS_REGION", env("TC887_S3_REGION")?); + std::env::set_var("AWS_EC2_METADATA_DISABLED", "true"); + let config: S3BlockConfig = serde_json::from_value(serde_json::json!({ + "bucket": env("TC887_S3_BUCKET")?, + "endpoint": env("TC887_S3_ENDPOINT")?, + }))?; + let store = config.open().await?; + let space: SpaceId = format!( + "tinycloud:key:s3remove:{}", + hex::encode(rand::random::<[u8; 6]>()) + ) + .parse()?; + store.create(&space).await?; + Ok((store, space)) +} + +fn value(n: usize) -> Vec { + (0..n).map(|i| (i * 13 % 251) as u8).collect() +} + +async fn put(store: &S3BlockStore, space: &SpaceId, data: &[u8]) -> Result { + let mut staged = HashBuffer::new(Vec::new()); + staged.write_all(data).await.unwrap(); + ImmutableWriteStore::::persist(store, space, staged).await +} + +#[tokio::test] +#[ignore = "needs the at-rest harness MinIO (TC887_S3_* env)"] +async fn remove_deletes_the_block_and_releases_its_size() -> Result<()> { + let (store, space) = store().await?; + let removed = put(&store, &space, &value(5000)).await?; + let kept = put(&store, &space, &value(17)).await?; + ensure!( + store.total_size(&space).await? == Some(5017), + "size after puts" + ); + + ensure!( + store.remove(&space, &removed).await? == Some(()), + "remove of a stored block" + ); + ensure!( + !store.contains(&space, &removed).await?, + "object still exists" + ); + ensure!(store.contains(&space, &kept).await?, "other block removed"); + ensure!( + store.total_size(&space).await? == Some(17), + "size drops by the removed block's length" + ); + + // Removing again, or removing a block never stored, is a no-op. + ensure!( + store.remove(&space, &removed).await?.is_none(), + "second remove" + ); + let mut unstored = HashBuffer::new(Vec::new()); + unstored.write_all(b"never stored").await?; + ensure!( + store.remove(&space, &unstored.hash()).await?.is_none(), + "absent block" + ); + ensure!( + store.total_size(&space).await? == Some(17), + "no-op removes leave the size" + ); + Ok(()) +}