ci: run a composition's ci-cleanup workflow after every mzcompose job

[ggevay]

Motivation

Cancelling or timing out a Cluster spec sheet job leaves its Cloud region running. Buildkite ends the job with SIGTERM, which kills the mzcompose process without running the composition's finally block, so mz region disable --hard never happens. The region survives until the next run on the same account disables it at its start: for the production steps that is the next weekly run, for a staging shard it is when the same E2E account comes up again in the 7-of-20 rotation, which can take weeks.

On 2026-09-03 two cancelled builds (37 and 38) left nine staging regions behind (slack). One of them, cancelled at 20,000 materialized views, kept the staging eu-west-1 CockroachDB cluster at 75-85% CPU for over two days and stalled the v26.41.0-rc.3 staging rollout. Build 17 did the same on a smaller scale on 2026-07-08.

Description

Best reviewed commit by commit; each commit is one concern.

• Plugin mechanism. The mzcompose Buildkite plugin runs a composition's ci-cleanup workflow, if it defines one, from its exit trap: after the main workflow has exited for any reason, before the Docker teardown, with the main workflow's arguments. Before it runs, the hook kills the containers of the main run's compose project: SIGTERM ends the mzcompose process, but a docker compose run container it started keeps going, and an in-flight mz region enable could otherwise re-create the region after the cleanup deleted it. The cleanup run writes no JUnit report, since it runs under the same job id and would overwrite the main run's report, which for a normally failing job is the only source of the failure annotation. A cleanup failure is written to run.log, where a new ci-annotate-errors pattern turns it into an error annotation even for a cancelled job, and fails an otherwise green job. Documented in the plugin README.
• cluster-spec-sheet defines ci-cleanup. For a Cloud target started with --cleanup it destroys the region, otherwise it does nothing. It is idempotent: after a successful run the region is already gone, which mz region disable reports as success. The target construction shared with default moves into make_target.
• --target is now required instead of defaulting to cloud-production. ci-cleanup runs unattended and reads the main workflow's arguments with parse_known_args, so a mistyped target used to fall through to production with cleanup enabled. All CI steps pass the target explicitly.
• Cleanup is verified. CloudTarget.cleanup checks with mz region list --format json that the region is reported disabled (which also covers a deletion still in progress) and fails otherwise, and disable_region no longer swallows errors. A surviving region therefore shows up as a failed ci-cleanup in the error annotation, however the job ended. Because the end-of-run teardown can now fail, it runs after the result uploads and the analysis, so a teardown failure fails the run without discarding its data.
• README. Every command passes --target, and the staging credentials paragraph describes the E2E pool accounts the composition actually uses.
• Staging step label. Cluster spec sheet: envd Scalability + Cluster Object Count Limits (against Staging), following the "what runs (against where)" pattern of the two production steps. The Cluster spec sheet prefix, which the plugin's services.log check matches on, stays. Buildkite Test Analytics starts a new suite history under the new name.
• Review fixes. Seven more commits, one per review point; what they change is listed under build 44 in Verification below.

Not covered: a job that dies from outside (agent loss, host termination). The start-of-run disable on the next use of the account remains the backstop for that. A SIGTERM handler in Python was considered and dropped: it would race the plugin's container kill and mzcompose down, which end the container the disable runs in.

Verification

• 2026-09-05, spec-sheet builds 40 to 42, against the nine real leftover regions: dry runs where the trap declined to destroy, then the nine disables, then idempotent reruns of the trap.
• 2026-09-06, spec-sheet build 43, the staging step of this branch, cancelled once six shards had their regions up (the seventh never got an agent). All six SIGTERMed shards ran ci-cleanup from the trap, each disable took about 30 seconds and was reported disabled, and staging eu-west-1 was back to its four permanent environments within two minutes. That build correctly has no error annotation; checking how a failure would have surfaced is what found the missing ci-annotate-errors pattern. A review pass after that build found the JUnit overwrite (reproduced locally, and visible in build 43's uploaded reports) and the orphaned-container race.
• 2026-09-06, spec-sheet build 44, with all fixes: cancelled eleven seconds into one shard's mz region enable during its QPS sweep, i.e. with a live enable container. All six shards cleaned up (the seventh was still waiting for an agent), the JSON check reported the region disabled, no shard uploaded a JUnit report, run.log carries the cleanup output, there is no error annotation, and staging was back to its four permanent environments 80 seconds after the cancel and still ten minutes later. Changes since that build: an unbuffered sed so the cleanup output streams to the job log, and the review fixes (the container kill scoped to the job's composition, --kill-after on the cleanup's timeout, the QPS sweep's disable inside its retry loop, sturdier mz region list parsing, the staging image version computed on use, the results uploaded before the teardown).
• 2026-09-06, spec-sheet build 45, with the review fixes: cancelled 23 seconds into the one shard that had an agent, during its first mz region enable. The trap's cleanup disabled and verified the region in 18 seconds, no error annotation, no JUnit artifact, and staging was back to its four permanent environments 92 seconds after the cancel and still ten minutes later.
• Locally: with --target=docker or without --cleanup the workflow is a no-op, default --help still parses, and shellcheck, pyright and bin/fmt are clean.

Out of scope

cloud-canary and mz-e2e disable their Cloud region from a try/finally too, so a cancelled job leaves it up the same way. They stay as they are for now: the risk is much lower, since the next Nightly's start-of-run disable cleans up within a day rather than a week, their environments are far cheaper, and their runs last minutes rather than hours, so the window for a bad cancel is short. Historically it has not bitten: in the last two months of metrics, no cancel left either region up for more than 20 minutes, and cloud-canary has not been cancelled or timed out at all since December 2025. They can adopt ci-cleanup in a follow-up.

🤖 Generated with Claude Code

[antiguru]

Reviewed the branch locally. The design is sound and the verification is unusually thorough. Two things I'd want resolved, plus some smaller points.

1. Dropping the UIError swallow in disable_region is riskier than the commit message argues

test/cluster-spec-sheet/mzcompose.py:3389

The removed comment said mz region disable "Can return: status 404 Not Found". The new comment asserts the opposite ("treats an already-disabled region as success"). Builds 40-42 verify the already-enabled-then-disabled case, which is not the same as the never-enabled case a 404 would come from. That case is reachable in two places now:

• mzcompose.py:3971, the start-of-run target.cleanup(), which runs before anything is enabled. On a pool account whose region record does not exist yet, a 404 there fails the job outright.
• workflow_ci_cleanup, for a shard that dies before initialize() (image pull, missing app-password env var). That would hard-fail ci-cleanup and produce an error annotation on a job that had nothing to clean up.

The identical except UIError: # Can return: status 404 Not Found still sits in test/cloud-canary/mzcompose.py:328 and test/mz-e2e/mzcompose.py:252, so the tree now contradicts itself. What is the evidence the 404 cannot happen? If it is an old mz version, worth saying so in the comment and removing the stale swallow in the other two files as a follow-up.

2. The disable in the sweep's retry loop is now outside the try

test/cluster-spec-sheet/mzcompose.py:3507-3508

for attempt in range(1, attempts + 1):
    disable_region(target.composition, hard=True)   # now raises
    try:
        enable_region(target, envd_cpus=envd_cpus)

The loop's own comment at 3513 says "a sweep does a dozen of these calls and the staging Cloud API returns the occasional 502". That retry now covers only enable; a 502 on disable aborts a multi-hour sweep where it previously did not. Moving disable_region inside the try keeps the disable-before-retry invariant, since the loop body starts with it, and restores the protection the comment promises.

Strong suggestions

The container kill is broader than its comment claims. ci/plugins/mzcompose/hooks/command:161: the comment says "the main workflow's containers", but --filter label=com.docker.compose.project matches every compose container on the host. Scoping it to label=com.docker.compose.project=$BUILDKITE_PLUGIN_MZCOMPOSE_COMPOSITION (project name defaults to the composition name, composition.py:138) matches the stated intent and drops the dependency on one-job-per-host.

timeout 15m has no --kill-after. ci/plugins/mzcompose/hooks/command:169. run_mzcompose uses --signal=TERM --kill-after=30s deliberately. Here a cleanup that ignores SIGTERM escapes the 15-minute bound the comment above it promises.

Nits

• verify_region_disabled (3399): entry["region"] / entry["status"] raise KeyError, not UIError, if the schema shifts, while everything else in the function is careful to convert. splitlines()[-1] also breaks silently if mz ever pretty-prints; picking the first line starting with [ would be sturdier. mz-e2e does a plain json.loads(output.stdout), so a word on why this one cannot would help.
• make_target for cloud-staging calls staging_version(), which does os.environ['BUILDKITE_COMMIT'] (mzcompose.py:109). ci-cleanup does not need the version, so a local run ci-cleanup --target=cloud-staging --cleanup dies with a KeyError instead of disabling the region.
• CloudTarget.region duplicates the Mz service's region. They come from the same constant in make_target so they cannot diverge today, but it is a second source of truth for the same fact.
• Pre-existing but now more exposed: the main run writes run.log through a process substitution (hooks/command:464) while the trap appends with tee -a. On a cancel those two writers can overlap. Same pattern as the existing test timed out printf, so not new here.

Checked and fine

• set -o pipefail is still in effect inside cleanup() (set +e only clears errexit), so if ! ... | sed | tee does catch a cleanup failure.
• All three pipeline steps pass --target; nothing else in the repo invokes this composition.
• The Cluster spec sheet prefix survives the rename, so the services.log-empty exemption (hooks/command:441) still matches, and nothing keys off the old label.
• parse_known_args handles the main workflow's extra options and positional scenario names; ${run_args[@]:1} drops the workflow name correctly, and CI_EXTRA_ARGS is still set when ci-cleanup runs, so it gets the same extras the main run got.
• JUnit suppression does not hide failures: raise UIError("at least one test case failed") still exits non-zero, so the run.log path fires.
• No downstream exit-code analysis, so the killed containers' 137s do not create false annotations.

---

Generated by Claude Code

[def-]

QA LLM Review

1. MEDIUM -- A failing region teardown now discards the whole run's results

test/cluster-spec-sheet/mzcompose.py:4019

CloudTarget.cleanup() runs inside the finally that precedes the result upload, and this diff turned it from something that could not fail into something that can: disable_region no longer swallows UIError, and verify_region_disabled raises one of its own. A teardown hiccup at the end of a multi-hour sweep now propagates out of the finally and skips the test-analytics upload, the CSV artifact upload, and the analysis, so the run produces no data at all.

Details

workflow_default closes the streams and calls target.cleanup() in the finally at 4015-4019; stream.spec.upload(...) (4027), buildkite.upload_artifact(...) (4033) and stream.spec.analyze(...) (4037) all sit after the try/finally. CloudTarget.cleanup (4155-4156) is now disable_region(hard=True) plus verify_region_disabled(...), and composition.run raises CommandFailureCausedUIError, a UIError subclass — exactly what the deleted except UIError: pass used to absorb.

Two concrete ways this fires. mz region disable --hard polls for the region to be fully gone for up to 600 s inside a 720 s retry budget (src/cloud-api/src/client/region.rs:206, src/mz/src/command/region.rs:131); a region that just held tens of thousands of objects is the case most likely to exceed it. And mz region list omits any cloud provider whose lookup errored (src/mz/src/command/region.rs:186), so a transient API error on the target region makes verify_region_disabled raise "missing from mz region list output".

The new ci-cleanup trap already disables the region after the workflow exits however it exited, so the in-workflow teardown is no longer the last line of defence — its failure has no reason to cost the run's data. Move the upload/artifact/analyze steps ahead of the teardown, or wrap target.cleanup() in its own try inside the finally and re-raise after the uploads have run.

[ggevay]

Thanks for the careful pass. The fixes are separate commits on top, one per point.

1. The 404 case. mz region disable has reported a missing region as success since 2a40992 (July 2026): disable() in src/mz/src/command/region.rs maps a 404 from the delete to "Region already disabled" instead of retrying. The composition runs the mz image built from the tree (mzbuild: "mz"), so that is the CLI in play. The never-enabled case is exercised in CI: every shard's start-of-run target.cleanup() in builds 43 and 44 ran against an account whose region had been hard-deleted hours earlier (the cloud API answers a DELETE for a region that does not exist with 404), with the swallow already gone. In build 44, shard 4 that disable returned after about three seconds, and the mz region list right after it reported both regions disabled. The swallow in cloud-canary and mz-e2e predates the CLI change (e91d55a, December 2024) and is stale. I'd remove it in a follow-up rather than widen this PR. The comment in disable_region now points at the CLI's mapping.

2. Sweep retry. Fixed: the disable is inside the try, so a retry starts over with it, and the warning names the step generically. The loop's comment no longer claims the CLI does not retry the enable: both mz region enable and mz region disable retry their API call for up to 12 minutes, and the 502s that reach this loop are the ones that survive that.

Container kill scope. Fixed: label=com.docker.compose.project=$BUILDKITE_PLUGIN_MZCOMPOSE_COMPOSITION. Checked locally that the filter matches a docker compose run one-off of that project and not one of another project.

--kill-after. Fixed: timeout --signal=TERM --kill-after=30s 15m, as in run_mzcompose.

Nits.

• verify_region_disabled parses from the first line that starts with [ to the end of the output, so a pretty-printed array works, and turns KeyError/TypeError/ValueError into the same UIError with the raw output. The comment says why a plain json.loads, as in mz-e2e, would break on an Error: line.
• The version is now passed only inside enable_region's staging branch, so CloudTarget no longer carries it and ci-cleanup --target=cloud-staging never reads BUILDKITE_COMMIT. Checked with stubs that cleanup() runs without it and that a staging enable still pins the version.
• CloudTarget.region vs Mz: left as is. Mz keeps the region only inside its generated config file, so deriving one from the other means either exposing it from Mz or having CloudTarget build the Mz service. Happy to do the latter if you prefer.
• run.log writers: agreed, pre-existing, same as the timed-out marker. Follow-up idea: let the main run's sed ignore TERM and append, so its late flush cannot clobber the trap's markers, and truncate run.log at job start.

Since the trap changed, we reran the cancel test: build 45, cancelled 23 seconds into a shard's first mz region enable (with the version now resolved on use), region disabled and verified by the trap's cleanup in 18 seconds, no error annotation, no JUnit artifact, staging back to its four permanent environments within 92 seconds and still at ten minutes. A seventh commit addresses the QA review's finding below (results uploaded before the teardown). The fix commits were rebuilt once after a simplification pass, so their hashes changed.

[ggevay]

Valid, thanks. The end-of-run teardown did sit ahead of the uploads, and this PR made it able to fail. Fixed in 827327a: the uploads, the artifact upload and the analysis now run inside the outer try, and the teardown is in its finally, so a teardown failure still fails the run but no longer discards its data.

On the two triggers: the largest region we have torn down, the leftover with 20,000 materialized views, took 167 seconds to hard-delete (build 41), against the CLI's 600-second poll, so the timeout is unlikely. The transient lookup error making mz region list omit the region is real, and the reordering covers it.

[antiguru]

All addressed, thanks. Spot-checked the substance rather than taking the answers on trust:

• The 404 mapping is where you say it is (src/mz/src/command/region.rs:145-155, NOT_FOUND to Ok(())), and the composition runs the mz image built from this tree, so dropping the swallow is safe. While I was there I also confirmed the two things verify_region_disabled leans on: list prints Error: {:?} for a failed provider lookup before emitting the table, and the field is region with enabled/disabled. Agreed on leaving the stale swallow in cloud-canary and mz-e2e to a follow-up.
• Sweep retry: disable is inside the try now, and the corrected comment (both subcommands already retry, so what reaches this loop survived that) is more accurate than what it replaced.
• Container kill: the filter can't silently no-op, since mzcompose.py:222 sets name=args.find or Path.cwd().name and composition.py:138 falls back to that for project_name, and the plugin never passes --project-name.
• Moving staging_version() into enable_region's staging branch is better than what I asked for: it retires CloudTarget.version and the assert (version is not None) == is_staging with it, so placement enforces the invariant instead of an assert.
• The reordering for the QA finding reads correctly: inner try/finally closes the streams, uploads and analysis in the outer try, teardown in its finally. An upload failure now also gets a teardown, which it didn't before.
• CloudTarget.region vs Mz: fine as is, not worth the churn.

One thing the refactor introduced, non-blocking: staging_version() now runs per enable rather than once per run, and it shells out to cargo metadata --no-deps (mz_version.py:107). The time is irrelevant, but it puts a subprocess.CalledProcessError inside cloud_recreate_region_with_envd_cpus's retry, which catches only UIError, so a transient cargo hiccup would escape the retry it now sits inside. A functools.cache on staging_version closes that and the repetition. The version can't change within a run.

---

Generated by Claude Code

[bosconi]

Looks good. Some minor items here. Thank you for getting to the bottom of this!

[bosconi]

The section is missing the three things the next adopter cannot infer from the hook. test/mz-e2e and test/cloud-canary, which the description names as follow-ups, would hit all three on their first attempt:

1. The workflow receives the main workflow's full args: list plus CI_EXTRA_ARGS, so it must use parse_known_args. With parse_args it exits 2 on every run, the marker fires, and every green job of that composition turns red.
2. check-mzcompose-files.sh fails a test/ composition with more than one def workflow_ unless default loops over c.workflows, and both of those compositions have exactly one workflow today and are not on its exclusion list. The remedy the lint prints would make default run ci-cleanup mid-run. Worth fixing the lint to skip workflow_ci_cleanup in this PR (see the review body), and saying so here either way.
3. "kills" is docker kill, so SIGKILL, and it runs on green jobs too. Under CI_COVERAGE_ENABLED the hook deliberately uses a graceful mzcompose down so the .profraw files get written at exit; the kill would lose them silently. The 15m budget is also only in a hook comment.

Suggested change

	A composition that creates resources outside of Docker, such as a Cloud
	region, can define a workflow named `ci-cleanup`. The command hook runs it
	after the main workflow has exited, however it exited, and passes it the same
	`args`. Cancelling or timing out a job ends the main workflow with SIGTERM,
	which does not run Python `finally` blocks, so a composition must not rely on
	its own cleanup path for those cases. Before the workflow runs, the hook kills
	the main workflow's containers, so nothing left over from the main run can
	race the cleanup; the Docker teardown proper happens afterwards. The workflow
	must be idempotent: it also runs after a successful run that already cleaned
	up. It writes no JUnit report, so the main workflow's report survives. Its
	failure is recorded in the error annotation and fails an otherwise green job.
	A composition that creates resources outside of Docker, such as a Cloud
	region, can define a workflow named `ci-cleanup`. The command hook runs it
	after the main workflow has exited, however it exited, and passes it the main
	workflow's full argument list (the step's `args`, plus any `CI_EXTRA_ARGS`),
	so the workflow must parse with `parse_known_args` and must be able to find
	its target from those arguments alone. Cancelling or timing out a job ends
	the main workflow with SIGTERM, which does not run Python `finally` blocks,
	so a composition must not rely on its own cleanup path for those cases.
	Before the workflow runs, the hook stops the main run's compose-managed
	containers with `docker kill` (SIGKILL), so that a command still in flight
	from the main run, such as an `mz region enable` that outlived the cancelled
	process, cannot undo the cleanup once it has finished; the Docker teardown
	proper happens afterwards. The kill is skipped
	under `CI_COVERAGE_ENABLED`, since a killed process writes no `.profraw`. The
	workflow must be idempotent: it also runs after a successful run that already
	cleaned up. It has 15 minutes, so that a hung cleanup cannot eat the cancel
	grace period before the artifacts are uploaded. It writes no JUnit report, so
	the main workflow's report survives. Its failure is recorded in the error
	annotation and fails an otherwise green job. A composition whose `default`
	loops over `c.workflows` must skip `ci-cleanup` in that loop.

[bosconi]

nit: no trailing newline, and the cleanup's tee -a run.log below now appends straight after this, so on a timed-out job the marker line becomes <label>: test timed out--- <first cleanup line>. ERROR_RE still matches (.* $), so the annotation fires, but its text carries whatever the cleanup printed first.

Suggested change

	printf "\n%s" "$BUILDKITE_LABEL: test timed out" >> run.log
	printf "\n%s\n" "$BUILDKITE_LABEL: test timed out" >> run.log

[bosconi]

Problem. docker kill is SIGKILL, and this runs on every job of a composition that defines ci-cleanup, successful ones included. Under CI_COVERAGE_ENABLED the hook's teardown is deliberately graceful (mzcompose down --volumes, line 289) because the LLVM runtime writes .profraw only at normal process exit. A SIGKILL before that point discards the job's coverage data, and nothing reports it: the job stays green, there is just no .profraw to collect.

Scope. cluster-spec-sheet does not run under coverage, so nothing breaks today. This is for the next composition that adopts ci-cleanup.

Fix. Skip the kill when CI_COVERAGE_ENABLED is set; the graceful down then handles the containers, as it does for every other coverage job. The race the kill guards against (an mz region enable outliving the cancelled process) is a Cloud-target concern, and coverage jobs do not run against Cloud, so nothing is lost.

Suggested change

	# `mzcompose down` removes them.
	docker ps --quiet --filter "label=com.docker.compose.project=$BUILDKITE_PLUGIN_MZCOMPOSE_COMPOSITION" | xargs --no-run-if-empty docker kill > /dev/null || true
	# `mzcompose down` removes them. Skipped under coverage: SIGKILL loses the
	# .profraw files written at exit, and the coverage teardown is graceful for
	# the same reason.
	if [ -z "${CI_COVERAGE_ENABLED:-}" ]; then
	docker ps --quiet --filter "label=com.docker.compose.project=$BUILDKITE_PLUGIN_MZCOMPOSE_COMPOSITION" | xargs --no-run-if-empty docker kill > /dev/null || true
	fi

[bosconi]

Design point rather than a defect: the reason for the suppression is a property of how the plugin invokes the workflow (same BUILDKITE_JOB_ID, hence the same junit_mzcompose_<job id>.xml), but the policy lives here, keyed on a workflow name the CLI otherwise knows nothing about. Two consequences: a developer who runs bin/mzcompose run ci-cleanup by hand, or a composition whose default calls c.workflow("ci-cleanup"), silently gets no report; and a composition that names its cleanup workflow anything else gets no suppression. An env var or --no-junit-report set by the hook on this one invocation would keep the decision where the knowledge is. Fine to defer if you would rather not widen the CLI surface now.

[bosconi]

We can summarize the full behavior here.

Suggested change

	help="Destroy the region at the end of the workflow.",
	help="Destroy the region when the workflow ends, and from CI's ci-cleanup after a cancel or timeout.",

[bosconi]

Branching on the string first keeps the no-op path free of environment requirements.

Suggested change

	target, mz = make_target(composition, args.target)
	if not isinstance(target, CloudTarget):
	print(f"Nothing to clean up for --target={args.target}")
	return
	if args.target == "docker":
	print("Nothing to clean up for --target=docker")
	return
	target, mz = make_target(composition, args.target)
	assert isinstance(target, CloudTarget)

[bosconi]

A manual staging run also needs BUILDKITE_COMMIT, which staging_version() reads unconditionally.

Suggested change

To run the cloud canary test manually, you can specify either `--target=cloud-production` (which is hardcoded to aws/us-east-1) or `--target=cloud-staging` (which is hardcoded to aws/eu-west-1). For production, you need to set the environment variables `NIGHTLY_MZ_USERNAME` and `MZ_CLI_APP_PASSWORD`. For staging, the run uses one account from the E2E database pool: set `E2E_STAGING_TEST_FRONTEGG_DATABASE_APP_PASSWORD_<n>` for the pool index `<n>` and select it with `CI_CONCURRENCY_POOL_SLOT=<n>` (outside CI, index 0 is used when the slot is unset); the username is derived from the index.

To run the cloud canary test manually, you can specify either `--target=cloud-production` (which is hardcoded to aws/us-east-1) or `--target=cloud-staging` (which is hardcoded to aws/eu-west-1). For production, you need to set the environment variables `NIGHTLY_MZ_USERNAME` and `MZ_CLI_APP_PASSWORD`. For staging, the run uses one account from the E2E database pool: set `E2E_STAGING_TEST_FRONTEGG_DATABASE_APP_PASSWORD_<n>` for the pool index `<n>` and select it with `CI_CONCURRENCY_POOL_SLOT=<n>` (outside CI, index 0 is used when the slot is unset); the username is derived from the index. Staging runs also need `BUILDKITE_COMMIT`, which selects the image version to enable.