BrighterCommand / BrighterCommand/Brighter
AWS cleanup sweep reports deletion failures as warnings only, and the workflow still goes green
- Dominant language
- C#
- Stars
- 2.5k
- Forks
- 296
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 21
Description
## Summary
`clean_failed_tests_aws_assets.sh` treats every deletion failure as a soft error: the parallel subshells `echo "WARNING: failed to delete ..."` and nothing aggregates them. The script exits 0 regardless, so the **AWS Test Resource Cleanup** workflow goes green whether the sweep deleted everything it matched or nothing at all.
A sweep in which every `DeleteTopic` was throttled looks identical, in the workflow's status, to a clean one. Finding out otherwise means reading up to 30 minutes of log by hand — or noticing weeks later that the account has hit a quota again, which is how #4298 started.
## Why this is reachable, not theoretical
SNS throttles `DeleteTopic` at roughly 30 TPS. The sweep runs `xargs -P 16` with no retry and no backoff, so a large backlog will hit that limit rather than approach it. The AWS CLI's own retries absorb some of it, but nothing in the script distinguishes "retried and succeeded" from "gave up".
## Suggested fix
Count the failures rather than only printing them — a temp file the subshells append to, or checking the `xargs` exit status — then:
- echo a summary line (`Failed to delete N of M matched topics`), and
- exit non-zero above a threshold, so a systematically failing sweep shows up as a failed workflow run rather than a green one.
A threshold rather than "any failure" because a resource deleted by a test's own teardown between the listing and the delete is normal and not worth failing on.
## Context
Raised in review of #4298 (https://github.com/BrighterCommand/Brighter/pull/4298#issuecomment-5573515915, item 8) and deliberately left out of that PR to keep it scoped to the leak itself.
Related: that PR added a first failure signal of this kind for one specific case — a topic whose `BrighterSweepFirstSeen` stamp cannot be written is reported on stderr, because losing `sns:TagResource` would silently defer every topic forever. The general case is still unreported.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01DEAvZsgdBRMKYJFix4KGj7
Contributor guide
Assessment
This issue has not been assessed yet.