BrighterCommand / BrighterCommand/Brighter

S3 test buckets leak: S3LuggageUploadTests never deletes its bucket, and no sweep covers brightertestbucket-*

Open
#4,316 0 comments 0 reactions 1 assignee Claimed by @iancooper View on GitHub
.NET 1 - Up Next Bug Maintenance
Dominant language
C#
Stars
2.5k
Forks
296
Avg merge
1d 11h
Merged PRs (30d)
21

Description

## Summary

The S3 luggage-store tests create real buckets named `brightertestbucket-`. One of them never deletes its bucket, and `clean_failed_tests_aws_assets.sh` has no S3 handling at all — it sweeps SQS, SNS and EventBridge Scheduler only. Nothing, anywhere, reclaims these.

This is the same shape as the SNS/SQS leak fixed in #4298, on a resource type that fix did not cover.

## The definite leak

`tests/Paramore.Brighter.AWS.Tests/Transformers/When_uploading_luggage_to_S3.cs` (and its `AWS.V4.Tests` mirror):

```csharp
_bucketName = $"brightertestbucket-{Guid.NewGuid()}"; // ctor
...
await luggageStore.EnsureStoreExistsAsync(); // creates the bucket
...
await luggageStore.DeleteAsync(claim); // deletes the claim, not the bucket
```

The class has no `Dispose`/`DisposeAsync` and never calls `DeleteBucketAsync`. Every run of `aws-ci` leaves two buckets behind — one per project — pass or fail.

## The conditional leaks

These clean up on the happy path only:

- `When_wrapping_a_large_message.cs` and `When_unwrapping_a_large_message.cs` delete the bucket in `DisposeAsync`, so a throw in the constructor skips it.
- `When_validating_a_luggage_store_exists.cs` deletes inline as the last statement of the test, so any earlier failure skips it.

(`When_creating_luggagestore_missing_parameters.cs` calls `EnsureStoreExistsAsync` on a guid-named bucket, but expects a `ConfigurationException` that should be thrown before S3 is reached. I have not confirmed whether a bucket is ever created there — worth checking rather than assuming either way.)

## Suggested fix

Two parts, matching what #4298 did for topics and queues:

1. Give the fixtures a teardown that deletes the bucket by *name* — including `S3LuggageUploadTests`, which has none — so a test that fails while standing infrastructure up still cleans up.
2. Add an S3 pass to `clean_failed_tests_aws_assets.sh` matching `^brightertestbucket-` as a backstop for the runs that die before they can. Note `ListBuckets` returns `CreationDate`, so unlike SNS topics these can be age-guarded directly with `CLEANUP_MIN_AGE_SECONDS` — no first-seen stamp needed. Buckets must be emptied before they can be deleted.

The existing `aws-cleanup.yml` credentials would need `s3:ListAllMyBuckets`, `s3:ListBucket`, `s3:DeleteObject` and `s3:DeleteBucket`.

## Context

Raised in review of #4298 (https://github.com/BrighterCommand/Brighter/pull/4298#issuecomment-5573515915) and left out of that PR as out of scope.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01DEAvZsgdBRMKYJFix4KGj7

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.