fix(aws): stop generated gateway tests leaking SNS topics and SQS queues

[iancooper]

Description

CI could no longer create AWS topics or queues. The account had accumulated 26,381 orphaned resources across two regions, including exactly 1,000 FIFO SNS topics in eu-west-1 — the AWS default quota for FIFO topics per account per region. That quota is what CI was hitting, and it is why most of the leftovers were .fifo.

The leak. The generated MessageGateway tests never deleted the infrastructure they created. All eight AWS providers' CleanUp/CleanUpAsync purged and disposed the channel and producer, then stopped — no DeleteTopicAsync/DeleteQueueAsync. They also built a ChannelFactory inline and discarded it, so they held no handle to delete with. Every generated test leaked its topic and queue on every run, pass or fail. The hand-written fixtures and the scheduler tests already did this correctly and were not the problem.

Why clean_failed_tests_aws_assets.sh never caught up with it:

1. The tag query filters on Environment=Test, but the gateway only stamps Source=Brighter (AWSMessagingGateway.cs:386), so it returned nothing.
2. The naming fallback listed the hand-written fixtures' prefixes, which predate the test generator (Add MessageGateway test generator #3996). 0 of 26,381 leaked names matched it.
3. list-queues was called without --page-size. SQS returns at most 1000 queues and no NextToken, so 20,900 queues looked like 1,000.
4. One AWS CLI call per resource could not clear a real backlog inside the cleanup workflow's timeout.

And test_clean_failed_tests_aws_assets.sh only ever created resources tagged Environment=Test, so it passed while the script found none of the actual leaks.

The fix. Providers now track every topic and queue name they hand out — including dead letter queues — and delete them from teardown via a new AwsTestResourceReaper. Reaping names rather than created objects also covers a test that failed while standing its infrastructure up, when there is no producer or channel left to dispose, and catches resources created as a side effect of a name (a DLQ, or a topic auto-created by a producer).

The teardown abstraction itself is not new — CleanUpAsync on IAmAMessageGatewayProactorProvider is exactly that hook. AWS simply was not honouring it. GCP and Kafka already delete their infrastructure through the same hook.

Related Issues

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the Contributing Guide
• I have checked the documentation for relevant guidance
• I have added/updated XML documentation for any public API changes
• I have added/updated tests as appropriate
• My changes follow the existing code style and conventions

Additional Notes

Verification

• 27 generated AWS tests run against the live account → 0 resources left behind (previously ~27 leaked).
• A test was deliberately forced to fail after a full message round trip → still 0 left behind, which is the "cleans up even on a failed run" requirement tested rather than assumed.
• test_clean_failed_tests_aws_assets.sh: 24/24 pass, including 4 new cases covering untagged, convention-named, FIFO resources. The existing negative test still proves untagged resources that do not match the convention survive.
• The 26,381 orphaned resources have been purged; all seven regions checked now report 0 topics and 0 queues.

The review-response commits have been verified as far as they can be locally: both projects build, and the offline reaper tests pass. The new live-account tests and the updated sweep test have not been run against AWS yet — CI is their first run.

This PR carries a workflow-scoped change

.github/workflows/aws-cleanup.yml is in the diff: the cleanup workflow's timeout-minutes goes from 5 to 30, the diagnostic resource counts are fixed, and the sweep's age guard is wired to the trigger. Pushing this branch therefore needs a token with the workflow scope.

Review response

Addressing #4298 (comment):

• The reap now runs from a finally. It was the last statement of each provider's CleanUp, after the channel purge and producer dispose — both of which reach AWS and can fail. PurgeQueue is throttled to one call per queue a minute, so a throw there was reachable under CI load, and it skipped the reap entirely.
• ReapAsync can no longer throw past teardown. Creating the SNS and SQS clients sat outside the guarded region, so a failure there replaced the test's own result — the opposite of what the method's summary promised. Each sweep guards its own client, and the tracked names are cleared in a finally so a half-completed run cannot leave names it will never retry.
• The scheduled sweep no longer deletes queues a live run may be using. Queues younger than CLEANUP_MIN_AGE_SECONDS (default an hour) are left for the next pass. The guard applies to the six-hourly run only; the run triggered by CI completing has nothing in flight and still sweeps everything, so no leak is deferred. SNS reports no creation time, so topics remain exposed on the scheduled path.
• Topic ARNs are composed from RegionEndpoint.PartitionName and Amazon.Arn, the way ValidateTopicByArnConvention builds them, instead of reading the partition out of the caller identity's ARN by position. A failed STS lookup is now remembered rather than retried per topic.
• The SQS providers no longer track a channel name that is never created. CreateSubscription replaces it with the publication's queue, which GetOrCreateRoutingKey already tracks — so tracking it after the override would cost the same wasted round trip rather than removing it.
• Diagnostic counts fixed. grep -c prints 0 and exits 1, so the || echo "0" fired too and the queue count came out as Total queues: 0 0; || true is what was wanted. The topic counts had the same per-page length(@) undercount the queue count was already fixed for.
• Test 8 polls to a deadline instead of reading once, and takes its hex id from uuidgen rather than openssl.

Two items were not taken as written:

• FindTopicAsync keeps no cancellation token — the SDK offers no overload that takes one.
• Tracking the SQS channel name inside CreateSubscription would not have removed the round trip, for the reason above; the name is left untracked instead.

New tests. MessageGatewayProviderCleanUpTests stands infrastructure up through a provider and then looks for it in AWS after CleanUpAsync, so the reap is verified against the account rather than the fixture's bookkeeping; a second case tears down through a channel that throttles on purge and asserts both that the failure surfaces and that the topic and queue went anyway. Deletion is eventually consistent, so the assertions poll. AwsTestResourceReaperTeardownFailureTests covers the swallow-and-drain contract offline, with no credentials needed.

Footgun worth knowing

The script and workflow operate on the ambient AWS region. The workflow pins eu-west-1, but local test runs leak into each developer's own default region — that is where a separate us-east-1 pile came from. Documented in the script header rather than widening the workflow's scope.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_012BpmKpyGj1G6VZ2EibRZoq

[claude]

Review — fix(aws): stop generated gateway tests leaking SNS topics and SQS queues

Thanks for the write-up; the diagnosis is unusually clear and the root cause is convincingly established. Reaping names rather than created objects is the right call — it survives a fixture that dies mid-setup and it picks up the DLQs and auto-created topics, which is exactly the class of leak that got us to 26,381 resources. The parallel sweep passes ARNs to sh -c as a positional (_ {}) rather than interpolating them into the script, which is the safe way to do it, and the --page-size and tag-vs-name findings are real bugs worth fixing on their own.

A few things I would want addressed before merge, then some smaller notes.

---

1. Reap() is not in a finally — teardown can still leak (highest-value fix)

tests/Paramore.Brighter.AWS.Tests/MessagingGateway/SnsFifoMessageGatewayProvider.cs:82-115, and the same shape in the other seven providers:

public async Task CleanUpAsync(IAmAMessageProducerAsync? producer, IAmAChannelAsync? channel, IEnumerable<Message> messages)
{
    if (channel != null)
    {
        await channel.PurgeAsync();     // throws => reap never runs
        channel.Dispose();
    }

    if (producer != null)
    {
        await producer.DisposeAsync();  // same
    }

    await _reaper.ReapAsync();
}

The stated requirement is "cleans up even on a failed run", and the manual verification covers a test that fails in the test body. But a throw in PurgeAsync/Dispose/DisposeAsync skips the reap entirely and leaks the topic and queue — precisely the failure being fixed. PurgeQueue is throttled to one call per queue per 60s, and SQS/SNS throttling under CI load is not hypothetical, so this is a reachable path rather than a theoretical one.

Suggested shape:

try
{
    if (channel != null) { await channel.PurgeAsync(); channel.Dispose(); }
    if (producer != null) await producer.DisposeAsync();
}
finally
{
    await _reaper.ReapAsync();
}

Same for the sync CleanUp. Eight files, mechanical, and it closes the last hole.

2. ReapAsync can itself throw and mask a test result

tests/Paramore.Brighter.AWS.Tests/Helpers/AwsTestResourceReaper.cs:65-90 — the XML doc says failures are swallowed and "must not replace that failure with its own", but only the per-resource work is inside try/catch. new AWSClientFactory(_connection).CreateSnsClient() (line 71) and CreateSqsClient() (line 82) sit outside it, as does the loop scaffolding. A throw there propagates out of DisposeAsync and xUnit reports it as a test failure, contradicting the documented contract. Wrapping the whole method body (or each if block) would make the code match the doc.

Related: _topics.Clear() / _queues.Clear() only run if the loop completes, so a mid-loop escape leaves the reaper half-drained. Moving the clears into a finally makes ReapAsync genuinely idempotent.

3. The broadened name sweep can now delete resources belonging to an in-flight CI run

.github/workflows/aws-cleanup.yml:7-9 runs the sweep every six hours as well as on CI completion. Previously the naming fallback matched only the hand-written fixture prefixes; it now matches (sqs|sns)-(std|fifo)(-ch)?-[0-9a-f]{32}, i.e. the majority of the suite. If the six-hourly cron lands while an AWS test job is running, the sweep deletes live topics and queues out from under it and produces a confusing flaky failure.

The hazard technically pre-existed for the older prefixes, but this change makes it likely rather than unlikely. Worth considering an age guard — e.g. skip queues whose CreatedTimestamp (from get-queue-attributes) is inside the last ~60 minutes — or narrowing the cron path. Not a blocker, but it would be a shame to trade a resource leak for CI flake.

---

Smaller points

ResolveTopicArnAsync re-derives what production code already knows (AwsTestResourceReaper.cs:159-165). _connection.Region.PartitionName gives the partition directly — see ValidateTopicByArnConvention.GetArnFromTopic (src/Paramore.Brighter.MessagingGateway.AWSSQS/ValidateTopicByArnConvention.cs:78-96), which composes the same string from _region.PartitionName plus Amazon.Arn. Using it removes the identity.Arn.Split(:)[1] positional parse — the comment above it documents an IAM-user ARN shape, and while the partition happens to sit at index 1 for assumed-role ARNs too, it is a fragile thing to depend on — and leaves STS supplying only the account id. Building the ARN with new Arn { Partition = ..., Service = "sns", ... } would also keep the two ARN constructions in step.

STS failure is not cached — same method, line 155: on failure _topicArnPrefix stays null, so every subsequent topic retries STS, fails again, and falls back to FindTopicAsync. That is the quadratic scan the remark warns about, plus a failing STS call per topic. A "do not bother" flag fixes it. The fallback FindTopicAsync(topicName) on line 169 also drops the cancellationToken.

The SQS providers track a channel name that is never created. SqsStandardMessageGatewayProvider.cs:52 overwrites channelName with routingKey ("for SQS point-to-point, the channel (queue) must match the publication queue"), so the sqs-std-ch-<guid> name from GetOrCreateChannelName is tracked but no such queue ever exists. Harmless — QueueDoesNotExistException is caught — but it costs a GetQueueUrl round trip plus a thrown exception on every teardown of both SQS suites. Tracking inside CreateSubscription, after the override, avoids it.

The diagnostic count bug is only half fixed (.github/workflows/aws-cleanup.yml:48). grep -c . prints 0 and exits 1 when there are no matches, so the || echo "0" fires as well and QUEUE_COUNT ends up as two lines — Total queues: 0 0. || true is what is wanted (still needed, since Actions runs the block under -e). Relatedly, TOPIC_COUNT and FIFO_COUNT on lines 39 and 44 still use length(@) under auto-pagination — worth checking whether those emit one number per page rather than a total, since that is the same class of undercount that hid the backlog in the first place.

The PR description is out of date on the workflow commit. The body says the timeout-minutes / queue-count commit is not in this PR, but .github/workflows/aws-cleanup.yml is in the diff. Good — both changes are worth having — but the description should say so, since it means the PR carries a workflow-scoped change.

---

Test coverage

The shell-side coverage is genuinely improved. Test 8 exercising untagged convention-named resources, with Test 5 as the negative counterpart, is the right pair, and generating a fresh GUID per run so the pattern is tested rather than a literal name is a nice touch.

Two gaps:

• The C# change has no test. CLAUDE.md is emphatic about /test-first for behavioural change, and the reaper — the actual fix — is verified only by the manual run described in the PR body. The AWS test projects already run against a live account in CI, so a small test in Paramore.Brighter.AWS.Tests that stands infrastructure up through a provider, calls CleanUpAsync, then asserts GetQueueUrlAsync throws QueueDoesNotExistException and GetTopicAttributesAsync throws NotFoundException would lock this in. Without it, the next edit to a provider CleanUp can silently reintroduce the leak, and it will be another 26k resources before anyone notices. A second test covering the throws-during-teardown path would also pin down point 1.
• Test 8 has no retry for eventual consistency. SNS GetTopicAttributes against a just-deleted topic can still succeed briefly, and the NotFound|not found|Not Found assertion will flake when it does. A short poll-with-timeout around those four assertions makes it deterministic. (openssl rand -hex 16 also adds a dependency the rest of the script does not have; uuidgen piped through tr is available wherever the AWS CLI is, if you would rather not rely on it.)

---

Net: the design is right and the analysis behind it is excellent. Point 1 is the one I would insist on — eight small edits, and it is the difference between "leaks are fixed" and "leaks are fixed unless teardown itself has a bad day". Points 2 and 3 are close behind; the rest is polish.

[claude]

Review: fix(aws): stop generated gateway tests leaking SNS topics and SQS queues

The root-cause work here is excellent — the four independent reasons the cleanup script never caught up are each verified rather than assumed, and the "track names, not objects" decision is right for exactly the reason given (a test that dies during setup has no producer or channel to dispose, but has already created the topic). Reusing CleanUpAsync rather than inventing a new teardown hook is also the right instinct.

A few things I would want addressed before merge, then some smaller notes.

---

1. CleanUp/CleanUpAsync can still skip the reap — the leak survives on the failure path (all 8 provider files)

This is the one I would fix first, because it undoes the fix in precisely the scenario the PR is written for.

public async Task CleanUpAsync(...)
{
    if (channel != null)
    {
        await channel.PurgeAsync();     // <-- throws => reap never runs
        channel.Dispose();
    }

    if (producer != null)
    {
        await producer.DisposeAsync();  // <-- same
    }

    await _reaper.ReapAsync();
}

SqsMessageConsumer.PurgeAsync logs and rethrows every exception (src/Paramore.Brighter.MessagingGateway.AWSSQS/SqsMessageConsumer.cs:155-159), and it calls EnsureChannelUrl first, so a missing queue surfaces as QueueDoesNotExistException. Concrete failure: a test whose channel creation partly succeeded, or that hits a throttle/timeout on PurgeQueue (plausible across 27 live tests), throws out of PurgeAsync inside DisposeAsync, ReapAsync is never reached, and the topic and queue leak again. The reaper itself is careful to swallow everything so teardown cannot mask a test failure; the callers should be equally defensive:

try
{
    if (channel != null) { await channel.PurgeAsync(); channel.Dispose(); }
    if (producer != null) { await producer.DisposeAsync(); }
}
finally
{
    await _reaper.ReapAsync();
}

Same shape in the four sync CleanUp methods. Worth applying in the test generator template too, if these providers are templated, so it does not regress on the next regeneration.

2. The name sweep now fails silently — the same blind spot that hid 26,381 resources

The listing calls changed from 2>&1 || echo "" to 2>/dev/null || echo "":

ALL_TOPICS=$(aws sns list-topics --query 'Topics[*].TopicArn' --output text 2>/dev/null || echo "")
ALL_QUEUES=$(aws sqs list-queues --page-size 1000 --query 'QueueUrls[*]' --output text 2>/dev/null || echo "")

If either call fails — expired credentials, missing sns:ListTopics, throttling, wrong region — the variable is empty, the script prints Found 0 untagged test topic(s) and Cleanup complete. and exits 0. A green cleanup job that swept nothing is indistinguishable from a green cleanup job that had nothing to sweep, which is how this backlog went unnoticed for so long.

The script already gets this right for the Tagging API 40 lines earlier (clean_failed_tests_aws_assets.sh:57-63): capture the exit code, print the response, exit 1. Please hold the name sweep to the same standard as the tag sweep.

3. The broadened pattern can now delete resources of in-flight tests

GENERATED_TEST_PATTERN matches every resource the generated AWS suites create, and the workflow runs on schedule: '0 */6 * * *' as well as on CI completion. A scheduled run (or a workflow_run fired by PR A's CI) that overlaps PR B's AWS test job will happily delete B's live sqs-std-<guid> queue mid-test. The old fallback had the same exposure through the hand-written prefixes, but its blast radius was a handful of names; this is now every generated resource in the account.

Options, roughly in order of cost:

• Skip young queues: aws sqs get-queue-attributes --attribute-names CreatedTimestamp, ignore anything under ~2h. (No equivalent for SNS topics, unfortunately — but queues are the bulk of the volume.)
• Add a concurrency: group shared with the CI workflow, or gate the sweep on no in-progress CI runs.
• At minimum, note the hazard in the script header next to the region footgun, so the next person debugging a mystery QueueDoesNotExist mid-test has a lead.

[claude]

(review continued)

4. ResolveTopicArnAsync reimplements existing production logic, less robustly

ValidateTopicByArnConvention.GetArnFromTopic (src/Paramore.Brighter.MessagingGateway.AWSSQS/ValidateTopicByArnConvention.cs:78-96) already composes an SNS ARN from the caller identity, using RegionEndpoint.PartitionName and Amazon.Arn rather than string-splitting the STS ARN:

var partition = identity.Arn.Split(':')[1];   // reaper
Partition = _region.PartitionName             // existing gateway code

_connection.Region.PartitionName gives you the partition without parsing, without depending on the shape of the caller's ARN (user vs. assumed-role vs. root), and without needing identity.Arn at all — only identity.Account. Building the ARN via new Arn { ... }.ToString() would also match the format the gateway itself uses.

Two smaller things in the same method:

• On the catch path _topicArnPrefix stays null, so every subsequent topic retries GetCallerIdentityAsync and then falls back to FindTopicAsync. That contradicts the "we resolve them once" remark in the XML docs and reintroduces the quadratic scan the method exists to avoid. Cache a sentinel (or a bool _identityUnavailable) so the STS attempt happens at most once.
• FindTopicAsync(topicName) does not get the cancellationToken that every other call in the class threads through.

5. The reaper tracks raw names; the gateway creates normalised ones

The gateway does not create the name it is handed — it creates name.ToValidSQSQueueName(isFifo) / ToValidSNSTopicName(isFifo), which truncates to 80/256, replaces . with _, and re-appends .fifo (src/Paramore.Brighter.MessagingGateway.AWSSQS/AWSNameExtensions.cs:56-77).

I worked through the current names and they all round-trip unchanged (sqs-fifo-ch-<32hex>-dlq.fifo is 53 chars, truncated to 48, .fifo re-appended, identical), so this is not currently a bug. But it is an invisible coupling: the moment a generated name grows past the limit or gains a . that is not the FIFO suffix, the reaper deletes a name that was never created, every failure is swallowed, and the leak returns with no signal. Passing tracked names through the same extension methods would make the drift impossible rather than merely unlikely.

6. Total silence on failure is risky for the thing whose whole job is to prevent an invisible leak

DeleteTopicAsync and DeleteQueueAsync both end in catch (Exception) { }. Not throwing from teardown is correct — but not reporting means an IAM change, a region change, or the name drift in #5 turns the reaper into a no-op that nobody notices until the next quota wall. A one-line Console.WriteLine warning costs nothing and shows up in the test log. (QueueDoesNotExistException should stay quiet — that one is expected.)

---

Smaller notes

• Reap() sync-over-async. ReapAsync().GetAwaiter().GetResult() is called from IDisposable.Dispose() in the Reactor tests, i.e. on xUnit's synchronisation context. It matches the existing GetMessageFromDeadLetterQueue at SqsStandardMessageGatewayProvider.cs:213, so it is consistent with local style — but this codebase has its own answer to this (BrighterAsyncContext.Run(() => ReapAsync()), as used by SqsMessageConsumer.Purge), which sidesteps the deadlock window entirely.
• Workflow diagnostic prints 0 0. grep -c . prints 0 and exits 1, so ... | grep -c . || echo "0" yields two lines and Total queues: 0 0 when the account is empty (.github/workflows/aws-cleanup.yml:48). Cosmetic, but it is a diagnostic line whose whole purpose is to be trusted. | wc -l, or dropping the ||, fixes it.
• AWS CLI retries. 16-way parallel delete-topic will brush against the SNS request rate on a large backlog. AWS_RETRY_MODE=adaptive / AWS_MAX_ATTEMPTS in the workflow env would cut the WARNING noise and reduce reliance on the next scheduled run to finish the job.
• Duplication. Two identical 175-line copies of AwsTestResourceReaper. That is the established convention for the V4 test project (it has no linked compiles anywhere), so I would leave it — just noting that any fix from this review needs applying twice, and Support for multiple Application Layer Protocols in Task Queues #1 needs applying eight times.

Test coverage

The shell-level coverage is genuinely improved: the 4 new cases finally exercise the case that actually leaks (untagged + convention-named + FIFO), and keeping Test 5 as the negative counterpart matters — without it the new pattern could quietly widen towards "delete everything" and still pass.

Two gaps:

1. Nothing automated asserts the C# fix. The "0 resources left behind" and "still 0 after a forced failure" results are the real evidence, but they live in the PR description, not the suite — so the next regeneration of the providers can silently drop the reap and CI stays green. A single AWS-tagged test that stands a fixture up, tears it down, and asserts GetQueueUrlAsync throws QueueDoesNotExistException would pin the behaviour. Per CLAUDE.md's TDD workflow this is the test that should have come first.
2. Nothing covers Support for multiple Application Layer Protocols in Task Queues #1. The most likely remaining leak path — teardown throwing before the reap — is cheap to cover: a fixture whose channel purge throws, asserting the resources are still gone.

Also worth confirming: test_clean_failed_tests_aws_assets.sh Test 8 asserts absence immediately after deletion. sns get-topic-attributes and sqs get-queue-url are eventually consistent after a delete, so this can flake. The pre-existing tagged-resource assertions have the same shape, so it is not a regression — but Test 8 creates FIFO topics, the quota-constrained resource, so a flake there is worth a short retry loop.

---

Nothing above changes the shape of the fix — the design is right and the diagnosis is unusually thorough. #1 and #2 are the two I would treat as blocking: the first because the leak survives the exact failure mode being fixed, the second because a silent no-op sweep is what let this reach 26,381 resources.

[claude]

Review

I read the diff against cd50a43, plus ci.yml, aws-cleanup.yml, the generated tests that call these providers, and the V4 mirrors (verified byte-identical modulo namespace/using — nice).

The core of this is right, and the PR body is the best root-cause writeup I've read on this repo. Reaping names rather than created objects is the correct call and the <remarks> explaining why — a test that dies while standing infrastructure up has no producer or channel left to dispose — is exactly the reasoning that stops someone "simplifying" it back later. Wrapping the reap in finally in all eight providers, and the PendingTopics/PendingQueues surface that makes the drain contract testable with no credentials, are both good. So are the --page-size 1000 and grep -c . || true fixes, with the comments that explain why they were wrong.

Findings below, most severe first.

---

1. CLEANUP_MIN_AGE_SECONDS: 0 on the workflow_run trigger can delete a concurrent CI run's queues

aws-cleanup.yml:29 disables the age guard for everything except schedule, on the stated grounds that "the run triggered by CI completing has nothing in flight to protect". That holds for the run that just finished, but not for the account:

• ci.yml:12-16 — CI fires on every push to master/release/9X and every PR to those branches.
• Neither ci.yml nor aws-cleanup.yml declares a concurrency: group (I grepped both), so in-progress runs are never cancelled.

So: PR A's CI finishes → aws-cleanup fires with the guard off → the name sweep matches sqs-std-<32hex> / sqs-fifo-<32hex> → it deletes queues that PR B's aws-ci job is actively receiving from. Before this PR that hazard didn't exist, because the naming fallback matched 0 of 26,381 generated names, as you note. Widening the pattern to cover generated names is what makes it live.

The reaper now handles the same-run case, so keeping the guard on for workflow_run defers nothing that would otherwise leak — the sweep is purely a backstop now. I'd set the guard unconditionally, or at least to a non-zero floor (900s) rather than 0. workflow_dispatch has the same exposure, and being manual makes it more likely to be run because a job looks stuck.

2. The tests that verify nothing leaks can themselves leak

MessageGatewayProviderCleanUpTests (When_cleaning_up_should_delete_the_topic_and_queue.cs) creates a real topic and queue in CreateInfrastructureAsync, and the only reap is the explicit CleanUpAsync in the Act step. There is no IAsyncLifetime/IAsyncDisposable on the class. If CreateProducerAsync, CreateChannelAsync, or the FindTopicAsync line (see #3) throws, the topic and queue survive with nothing to delete them — in the one test whose subject is leaked resources.

A DisposeAsync calling _provider.CleanUpAsync(null, null, []) fixes it, and is safe precisely because of the contract you documented: a second reap is a no-op, since ReapAsync clears the tracked names in finally.

3. (await snsClient.FindTopicAsync(...)).TopicArn is an unguarded null deref on an eventually-consistent read

When_cleaning_up_should_delete_the_topic_and_queue.cs:72. FindTopicAsync pages ListTopics, which is eventually consistent — a topic the producer created moments earlier may not be listed yet. FindTopicAsync returns null in that case, and the test fails with an NRE that says nothing about the behaviour under test (and leaks, per #2). Worth noting the fallout: _topicArn then stays string.Empty, so if you do add teardown, GetTopicAttributesAsync("") returns InvalidParameter rather than NotFound and AssertEventuallyThrowsAsync burns its full 90s before failing.

Composing the ARN the way AwsTestResourceReaper.ResolveTopicArnAsync already does would sidestep both the flake and the scan.

---

4. These two tests also run in aws-mock-ci (LocalStack), unfiltered, in an 8-minute job

ci.yml:470 / ci.yml:505-507: aws-mock-ci runs Paramore.Brighter.AWS.Tests with no --filter against AWS_SERVICE_URL=http://localhost:4566, timeout-minutes: 8. So both new live tests execute there, not just in aws-ci.

Four AssertEventuallyThrowsAsync calls at a 90s deadline is up to six minutes of polling if floci's delete semantics differ from SNS/SQS in any way — GetQueueUrl still resolving, or a different exception type than QueueDoesNotExistException/NotFoundException. That's a job-timeout failure on a path the PR body doesn't mention testing. Have you run these under aws-mock-ci? If not, either shorten the deadline (SQS documents 60s, and the reap has already returned by the time you poll) or gate them. Note [Trait("Fragile", "CI")] is the mechanism the other jobs use, but it would also skip them in aws-ci, which defeats the point — so a LocalStack-specific skip is probably what's wanted.

5. ReapAsync has no timeout

Every teardown calls it as ReapAsync() / Reap() with CancellationToken.None, and the reactor path blocks a test thread on GetAwaiter().GetResult(). A hung SNS or SQS call hangs teardown indefinitely, and the AWS jobs pass --blame but no --blame-hang-timeout. Defaulting to a CancellationTokenSource(TimeSpan.FromSeconds(30)) when the caller supplies no token would bound it, and is consistent with the method's stated promise not to make a failing test's situation worse.

6. Region: is the sweep even looking where CI leaks?

Adjacent to your own "footgun worth knowing" note, and pre-existing, but it decides whether this workflow does anything. aws-ci sets AWS_REGION: us-west-2 in job env (ci.yml:519) while its configure-aws-credentials step passes aws-region: eu-west-1 (ci.yml:539). Job-level env normally wins over what an action exports via $GITHUB_ENV, which would put the tests in us-west-2 — while aws-cleanup.yml:24 pins the sweep to eu-west-1. Given the PR found piles in two regions, worth confirming which region aws-ci actually resolves before trusting the sweep as a backstop. (The reaper is unaffected — it deletes through the test's own connection.)

7. Convention: one test method per file

.agent_instructions/testing.md:9 — "Name test files for the test method in the file." Both new files carry two [Fact]s:

• When_cleaning_up_should_delete_the_topic_and_queue.cs also holds When_teardown_throws_should_still_delete_the_topic_and_queue
• When_reaping_cannot_create_an_aws_client_should_not_throw.cs also holds When_reaping_cannot_create_an_aws_client_should_not_leave_resources_pending

Each wants its own file. The class names (MessageGatewayProviderCleanUpTests, AwsTestResourceReaperTeardownFailureTests) correctly follow the [Behavior]Tests rule.

---

Nits

• [Collection] / [Trait] consistency. MessageGatewayProviderCleanUpTests drives SnsStandardMessageGatewayProvider but carries no [Collection("SnsStandard")], so it runs in parallel with that collection; and Category=AWS is a category nothing else in the project uses (Sns/Sqs). Harmless under --filter "Fragile!=CI", just inconsistent.
• The teardown-throws test drops SDK clients. PurgeAsync throws before channel.Dispose() and producer.DisposeAsync(), so the real channel and producer are never disposed — only the AWS resources get reaped. Correct for what's asserted; a finally in the test would tidy it.
• One STS GetCallerIdentity per reaper, i.e. per test — roughly 54 across both projects. A static Lazy<Task<string?>> would make it one per run. Trivial either way; ignore if you prefer the instance self-contained.
• List<string> tracking is unsynchronised. Nothing calls the Track* methods concurrently today (When_multiple_threads_try_to_post... only parallelises SendAsync), but the class is public and generically named. Either a ConcurrentBag or an XML remark saying it isn't thread-safe would save the next caller finding out at 3am.
• TEST_PREFIXES is blunter than GENERATED_TEST_PATTERN. The generated pattern requires 32 hex digits, and the comment rightly calls that "the only thing standing between a real queue and deletion" — but the prefix list is start-anchored with no such requirement, so a queue named DLQ-Reader-orders would be swept. Pre-existing, and presumably a dedicated test account, but the asymmetry is worth a line in the header.
• hex_id()'s comment overstates uuidgen. It's uuid-runtime on Debian/Ubuntu and absent from plenty of minimal images. Fine where it actually runs; the comment claims more than that.

---

Nothing here touches production code, and #1 is the only finding I'd call blocking — it trades one source of CI failure for another. #2 and #3 are small changes to new test code.

[iancooper]

Thanks — this was a good catch list. All of it is addressed in f6d4d6e03..d2dbb077d, except two items I did not take as written and one gap I have left open deliberately. Point by point:

1. Reap() not in a finally. Fixed in all eight providers, sync and async. You were right that this is the one that matters: the throttle on PurgeQueue makes it a reachable path, not a theoretical one, and it skipped the reap entirely.

2. ReapAsync masking a test result. Fixed. Client creation now sits inside the guarded region — each sweep guards its own client, so a failure creating the SNS client no longer skips the queues as well — and _topics.Clear()/_queues.Clear() moved into a finally, which makes the single-attempt contract real. The XML doc now says what the code does.

3. The scheduled sweep vs. an in-flight run. Age guard added: queues younger than CLEANUP_MIN_AGE_SECONDS are left for the next pass, with the lookups running at the sweep's parallelism and only against names that already matched. A queue whose age cannot be read is treated as old, since the likeliest reason is that it has already gone.

Two things the suggestion needed that were not obvious from the diff:

• The guard has to be off for the workflow_run path. CI has finished by then, nothing is in flight, and leaving it on would defer every leak from that run by six hours. The workflow now sets it per trigger — 3600 on schedule, 0 otherwise.
• test_clean_failed_tests_aws_assets.sh creates its fixtures seconds before invoking the sweep, so the guard would have skipped all of them and failed Test 8. It exports CLEANUP_MIN_AGE_SECONDS=0, since what that script tests is which resources are matched, not when the sweep declines to act.

Smaller points. ResolveTopicArnAsync now builds ARNs with Amazon.Arn from RegionEndpoint.PartitionName, the way ValidateTopicByArnConvention does, so STS supplies only the account id and the positional parse is gone. A failed identity lookup is remembered rather than retried per topic. Diagnostic counts fixed — || true for the queue count (reproduced locally: the old form really did print Total queues: 0 0), and the topic counts had the same per-page length(@) undercount, so they count names now. Test 8 polls to a deadline and takes its hex id from uuidgen. The PR description now says the workflow-scoped change is in the diff.

Not taken as written:

• FindTopicAsync cancellation token — there is no overload that takes one in this SDK version; adding it fails the build with CS1501. Left as-is with a comment saying why.
• Tracking the SQS channel name inside CreateSubscription — after the override the effective name is the routing key's queue, which GetOrCreateRoutingKey already tracks. Tracking it there would cost the same GetQueueUrl and caught exception, just against an already-deleted queue instead of a never-created one. The name is simply left untracked, with a remark recording why.

Test coverage. Two new suites:

• MessageGatewayProviderCleanUpTests (live account) stands infrastructure up through a provider and then looks for it in AWS after CleanUpAsync — so the reap is verified against the account, not the fixture's own bookkeeping. The second case tears down through a channel that throws PurgeQueueInProgressException on purge, and asserts both that the failure still surfaces and that the topic and queue went anyway. Revert any of the eight finally blocks and it fails. Assertions poll to a deadline for exactly the eventual-consistency reason you raised about Test 8.
• AwsTestResourceReaperTeardownFailureTests covers the swallow-and-drain contract offline, through a connection whose client configuration fails, so point 2 is pinned on every build without credentials.

One gap left open. SNS reports no creation time, so topics cannot be age-guarded the way queues can and remain exposed to the scheduled sweep. Narrowing the scheduled path to the tag query for topics would close it, at the cost of making a leaked topic wait for the next CI completion. Recorded in the commit message and the PR description rather than fixed here.

CI is the first run for the live tests and the updated sweep test — they cannot run locally without credentials.