Skip to content

fix(ci): stop list_groups sweeping on an unverified parse - #158

Merged
naxty merged 1 commit into
mainfrom
naxty/harden-list-groups
Sep 3, 2026
Merged

fix(ci): stop list_groups sweeping on an unverified parse#158
naxty merged 1 commit into
mainfrom
naxty/harden-list-groups

Conversation

@naxty

@naxty naxty commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

pre-cleanup no longer dies — but it now does nothing for 50 minutes, and reports
success-shaped output while doing it. This makes that impossible.

What is happening on main right now

From the run at 08:56Z:

clean-environment.sh: command failed at line 218 (exit 1): GROUPS=$(list_groups)
Pass 1 - found:
  1001
  deleting 1001
    already gone
Waiting up to 25m for deletion to complete...
  204 group(s) still deleting, waiting...      ← ×50
  ...then Pass 2 repeats the whole thing

list_groups returned the single token 1001 instead of 204 group names. The
script asked ARM to delete a resource group called 1001, got
ResourceGroupNotFound, counted that as "already gone", then sat in wait_gone for
two full 25-minute budgets while every real group survived untouched.

The Azure activity log confirms it independently: four resourcegroups/delete
events, all against rg=1001, all Failed.

Consequence: pre-cleanup can never pass while groups survive, so
conformance-tests never runs. #152 is hard-blocked on this.

Two defects made that possible

1. Every failure in the parse pipeline was swallowed.

| jq -r ... 2>/dev/null \
| grep -E "^${TEST_PREFIX}[A-Za-z0-9._-]*$" || true

jq's stderr went to /dev/null and the grep carried || true, so a broken parse
was indistinguishable from an empty subscription — and "no test groups" is a normal
exit 0. That is exactly how a sweep silently does nothing while 204 groups keep
billing.

Now: jq's stderr is not discarded, and the function proves its parse rather than
assuming it. It asks how many groups az group list reported (jq length) and
compares that against the number of names actually extracted. A mismatch prints the
first 200 bytes of the body plus the extracted names, and is fatal.

2. grep was in the trust path.

No regex anchored on ^${TEST_PREFIX} can return 1001, yet 1001 reached
az group delete. Whatever the mechanism, grep is the link that provably
misbehaved, so it is gone: the prefix test is now bash's own
[[ "${name}" == "${TEST_PREFIX}"* ]] — no external binary, nothing on PATH that
can shadow it, exact. The destructive guard is preserved: only names the function
re-verified itself are ever printed, which is what kept 1001 merely useless rather
than catastrophic.

The flag is a file, not a variable

Callers invoke this as GROUPS=$(list_groups) — a subshell — so a variable set
inside the function is discarded on return. The first cut of this patch made exactly
that mistake and exited 0 on a detected parse failure. It is a flag file now,
cleaned up on EXIT.

Verification

Against stub az/jq, both cases asserted:

Case Result
Healthy — 4 groups, 2 prefixed, az group list exits 1 with valid JSON proof does not fire; debug: az reported 4 group(s), parsed 4, matched prefix 2; only the 2 prefixed groups reach az group deleteproduction-rg and a decoy group literally named 1001 are both spared
Mangledjq length says 3, name extraction yields 1001 detected, body and names reported, exit 1: "could not parse the resource-group list; nothing was swept. Treat this as a failed sweep, not a clean subscription."

What this does not claim

It does not explain why CI's pipeline produced 1001. The pattern is odd — the
same function returns correct results when called later in the same run (wait_gone
counted 204 correctly) — and I could not reproduce it faithfully, because this
workstation's shell rewrites command output including values inside pipelines, so an
untraced local run of this script cannot be trusted at all. The new debug line and
the un-swallowed jq stderr exist to name the mechanism on the next real CI run.

What it does guarantee: the failure can no longer be silent, and can no longer burn
50 minutes deleting a group that does not exist.

This must land on main to be exercised — a PR touching only scripts/ resolves
to an empty conformance scope, which skips pre-cleanup entirely. That is also why
this PR's own CI will not prove it.

`pre-cleanup` no longer dies, but it now does nothing for 50 minutes. From the
run on main at 08:56Z:

    clean-environment.sh: command failed at line 218 (exit 1): GROUPS=$(list_groups)
    Pass 1 - found:
      1001
      deleting 1001
        already gone
    Waiting up to 25m for deletion to complete...
      204 group(s) still deleting, waiting...        <- x50
      ...then Pass 2 does it again

`list_groups` returned the single token `1001` instead of 204 group names. The
script duly asked ARM to delete a resource group called `1001`, got
`ResourceGroupNotFound`, counted that as "already gone", and then sat in
`wait_gone` for two full 25-minute budgets while every real group survived. The
Azure activity log confirms it: four `resourcegroups/delete` events, all against
`rg=1001`, all Failed.

Two things made that possible, and this commit fixes both.

## 1. Every failure in the parse pipeline was swallowed

    | jq -r ... 2>/dev/null \
    | grep -E "^${TEST_PREFIX}[A-Za-z0-9._-]*$" || true

`jq`'s stderr went to /dev/null and the `grep` carried `|| true`, so a broken
parse was indistinguishable from an empty subscription. jq's stderr is no longer
discarded, and the function now *proves* its parse instead of assuming it: it
asks `az group list` how many groups it returned (`jq length`) and compares that
to the number of names actually extracted. A mismatch is reported with the first
200 bytes of the body and the extracted names, and is fatal.

That distinction is the important one. "Extracted no test groups" and "there are
no test groups" look identical from the outside, and the second is a normal exit
0 - which is precisely how a sweep silently does nothing while 204 groups keep
billing.

## 2. grep was in the trust path

No regex anchored on `^${TEST_PREFIX}` can return `1001`, yet `1001` reached
`az group delete`. Whatever the mechanism, `grep` is the link that provably
misbehaved, so it is gone from this path: the prefix test is now bash's own
`[[ "${name}" == "${TEST_PREFIX}"* ]]`, which needs no external binary, cannot be
shadowed by anything on PATH, and is exact. This also keeps the destructive guard
- only names the function has re-verified itself are ever printed - which is the
property that stopped `1001` being catastrophic rather than merely useless.

## The flag is a file, not a variable

Every caller invokes this through `GROUPS=$(list_groups)`, i.e. in a subshell, so
a variable set inside the function is discarded on return. The first version of
this patch made exactly that mistake and exited 0 on a detected parse failure.
It is a flag file now, cleaned up on EXIT.

## Verification

Against stub `az`/`jq`, both cases asserted:

- **Healthy parse** (4 groups, 2 prefixed, `az group list` exiting 1 with valid
  JSON): parse proof does not fire, `debug: az reported 4 group(s), parsed 4,
  matched prefix 2`, and only the two prefixed groups are passed to
  `az group delete` - `production-rg` and a decoy group literally named `1001`
  are both spared.
- **Mangled parse** (`jq length` says 3, name extraction yields the single token
  `1001`): detected, reported with the body and the extracted names, and **exit
  1** - "could not parse the resource-group list; nothing was swept. Treat this
  as a failed sweep, not a clean subscription."

## What this does NOT claim

It does not explain *why* CI's jq/grep pipeline produced `1001`. The pattern is
odd - the same function returns correct results when called later in the same run
(`wait_gone` counted 204 correctly) - and I could not reproduce it faithfully:
this workstation's shell rewrites command output, including values inside
pipelines, so an untraced local run of this script cannot be trusted at all. The
new debug line and the loud jq stderr are there to name the mechanism on the next
real CI run.

What it does guarantee is that the failure can no longer be silent, and can no
longer burn 50 minutes deleting a group that does not exist.

Note this must land on `main` to be exercised: a PR touching only `scripts/`
resolves to an empty conformance scope, which skips `pre-cleanup` entirely.
@naxty
naxty merged commit 98fb0f0 into main Sep 3, 2026
9 checks passed
naxty added a commit that referenced this pull request Sep 3, 2026
* fix(ci): pass the group list through a file, not a command substitution

Fourth attempt, and the first aimed at the right place. Every previous fix
(#151, #153, #155, #158) targeted `list_groups`. The function was never wrong.

A CLEAN_DEBUG=1 trace from CI shows it working perfectly - and then the value
being destroyed on the way out:

    ++ echo '  debug: az reported 205 group(s), parsed 205, matched prefix 204'
    ++ return 0
    + GROUPS='formae-plugin-sdk-test-vnet-rg-13770d81
    formae-plugin-sdk-test-pip-rg-9042acb1
    ...'
    ++ echo 'clean-environment.sh: command failed at line 267 (exit 1): GROUPS=$(list_groups)'
    + [[ -z 1001 ]]
    + issue_deletes 1001
    ++ az group delete --name 1001 --yes --no-wait
    + err='ERROR: (ResourceGroupNotFound) Resource group '1001' could not be found.'

205 reported, 205 parsed, 204 matched, `NetworkWatcherRG` correctly excluded,
`return 0`, and the correct 204 names visibly assigned. One traced line later
`GROUPS` is the string `1001` and the substitution has reported exit 1.

So the sweep asked ARM to delete a resource group named `1001`, was told it does
not exist, counted that as "already gone", and then sat in `wait_gone` for two
full 25-minute budgets while every one of the 204 real groups survived. That is
the ~50 minutes of apparent work with nothing to show for it, and it is why
`pre-cleanup` can never pass: it correctly reports survivors and exits non-zero,
which skips `conformance-tests` and blocks every PR in the repo.

It also explains the one thing that never fit: `wait_gone` always counted 204
correctly. It re-lists and counts rather than capturing, so it never went through
the broken path.

## The fix

The group list now travels through files. `list_groups` still writes names to
stdout; callers redirect rather than capture:

    list_groups > "${GROUPS_FILE}"

and `remove_locks` / `issue_deletes` read that file line by line instead of
taking 204 positional arguments. Nothing depends on capturing ~8KB of multi-line
output through `$(...)` any more. Short substitutions are untouched -
`err=$(mktemp)` traces correctly in the same run, so `mktemp` stays as it is.

This also removes the unquoted `issue_deletes ${GROUPS}` word-splitting, which
was relying on IFS to rebuild the list it had just flattened.

## Verification

Against stub `az`/`jq`, three cases, asserting on what actually reached
`az group delete` rather than on printed output:

- **Healthy** (4 groups, 2 prefixed, plus a decoy group literally named `1001`):
  delete targets are exactly `formae-plugin-sdk-test-aaa` and
  `formae-plugin-sdk-test-bbb`. `production-rg` and the `1001` decoy are both
  spared. Previous versions produced `20` here.
- **Mangled parse** (`jq length` says 3, name extraction yields `1001`):
  detected, exit 1, and **nothing is deleted**.
- **Empty subscription**: exits 0, reports clean, deletes nothing.

The first case is the one that matters: it is the first time any version of this
script has been observed passing real group names to `az group delete` on this
machine. The same workstation shim that rewrites values inside pipelines was
mangling them to `20` locally exactly as CI mangled them to `1001`; routing
through a file defeats both.

* chore(ci): summarise the sweep by resource kind instead of naming every group

A full sweep touches ~200 resource groups, and the script named each one twice -
once on discovery, once on delete - so the log was 400+ lines of near-identical
names and the only thing a reader wants was buried: WHAT got cleaned up.

The fixture kind is already encoded in the group name, so
`formae-plugin-sdk-test-cdn-profile-rg-6d137340` reduces to `cdn-profile` by
dropping the prefix, the trailing run id and a trailing `-rg`. Counting those
turns the whole sweep into a few lines:

    Pass 1 - deleting 17 group(s):
      sa x5
      acr x4
      vnet x3
      grafana-mpe x2
      cdn-profile x2
      bastion x1

Survivors still get named, because that is the failure path and someone has to
go and delete them by hand - but capped at 20 with a "... and N more", since a
wholesale failure would otherwise reprint all ~200. Individual names remain in
the trace under CLEAN_DEBUG=1 whenever a specific group needs chasing.

No associative arrays: `sort | uniq -c` keeps this working under the bash 3.2
used for local testing as well as the runner's bash 5.

Verified against four stub scenarios, asserting on what actually reached
`az group delete` rather than on printed output:

    17 groups / 6 kinds   exit 1  53 lines  34 deletes  all real names
    4 groups / 2 matching exit 1  26 lines   4 deletes  all real names
    mangled parse         exit 1  19 lines   0 deletes
    empty subscription    exit 0   9 lines   0 deletes
@naxty
naxty deleted the naxty/harden-list-groups branch September 4, 2026 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant