fix(TC-1354): size S3 block deletes with HeadObject

.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: |

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<Option<u64>, 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<either::Either<file_system::TempFileSystemStage, memory
 impl ImmutableDeleteStore for S3BlockStore {
     type Error = S3StoreError;
     async fn remove(&self, space: &SpaceId, id: &Hash) -> Result<Option<()>, 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(()))
     }
 }
 

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<String> {
+    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<u8> {
+    (0..n).map(|i| (i * 13 % 251) as u8).collect()
+}
+
+async fn put(store: &S3BlockStore, space: &SpaceId, data: &[u8]) -> Result<Hash, S3StoreError> {
+    let mut staged = HashBuffer::new(Vec::new());
+    staged.write_all(data).await.unwrap();
+    ImmutableWriteStore::<MemoryStaging>::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(())
+}