Adversarial review fixes, and the published image becomes the tested one

[MorningLightMountain713]

Eleven changes from an adversarial review of the FluxOS side (RunOnFlux/flux#1778), plus the release machinery that has to be right before any of them ship.

Each fix was proven red before green, and the container suite was run against a locally built image throughout.

What was wrong

A stop never reached an upload. signal.Notify was installed inside runChild, so an operation expressed as a command handled a stop and an upload — which has no child at all — handled nothing. flux-op runs as PID 1 in the executor's container, and the kernel delivers no signal to PID 1 that the process has not asked for: the TERM was discarded, the Go runtime gave up and exited 2, and nothing unwound. A partial upload was left at a name the reserved-name rules hide from the app owner, on a fixed-size volume, until the next boot sweep.

Closing stdin does not help either — the runtime does not poll standard input, so a read already blocked on it stays blocked and the container is SIGKILLed at the end of its grace period. The transfer is now waited on rather than waited for.

Publishing a directory over its own parent took the whole parent with it. Displacing the destination carries the staging path away inside it, so the second rename finds nothing. photos/2024 moved onto photos displaced all of photos — including files the caller never named — and then reported failure. Refused now, in both directions; the mirror case cannot be renamed at all.

Two writes trusted a path the application can also write to. inspect measured whatever it was pointed at, so a staging path that was a symlink measured as the few bytes of its own name and passed any ceiling. The marker was written with os.WriteFile, which follows a link. Neither is reachable while the caller names operations with a fresh UUID — which is the point: the safety rested entirely on the caller having chosen an unguessable name, an invariant this program neither states nor can check.

--id and --root became paths without being looked at. A separator in the identifier puts the artefacts in a subdirectory the boot sweep never reads; traversal puts them outside the volume. markerContents made its path relative by trimming a prefix, and trimming an absent prefix is a no-op — so a destination outside the root was written down as an absolute path, the one shape that function exists to make unrepresentable.

A FIFO is not a link, so nothing asked about it. --no-links looked for symlinks and for regular files with a second name. tar carries and recreates FIFOs, and this flag is used for exactly one operation — extracting an archive — so an archive was all it took. Whatever opens one without O_NONBLOCK waits for a writer that never comes. The flag is renamed to --ordinary-only because a flag called --no-links that refuses FIFOs tells the next reader something untrue.

The ceiling measured what files say rather than what they occupy. A file occupies whole blocks, so twenty thousand one-byte files are 20 KB by that measure and 82,436,096 bytes on a real ext4 volume — measured, not estimated. The ceiling is the volume's free space, counted in blocks, so the comparison was between two different kinds of number. The check runs after the command, so the sequence was: the extraction fills the disk, the result measures 20 KB, the ceiling is satisfied, and it publishes.

Tests that were not testing anything

The container suite had been failing since the identity work landed and nothing caught it, because the run that reported the Go suite green was go test ./cmd/.... These need -tags docker and a built image, so they were never in it.

cp supports -T grepped --help for -T. busybox lists it, implements it, and passes the functional test beside it — so the one case whose whole purpose was to fail if coreutils were dropped passed without it. Demonstrated rather than argued: an image built without coreutils passes both old assertions and fails the new one.

The cancelled-upload case did not exist. Cancellation was tested for the command path, and requireNoArtefacts — the assertion that catches the leak — was already written. Its twin now exists.

Two tests used a bug to reach the state they wanted. The interrupted-publish fixtures relied on the parent case failing, which is how the missing refusal survived. They now reach it by making staging's directory unwritable, and skip as root rather than passing without testing anything.

Release machinery

The image that was published was not the image that was tested. The workflow built twice — once in the job that ran the container tests, and again in the job that pushed. The Dockerfile pins a minor Alpine tag and installs unpinned packages, so two builds minutes apart are not required to agree.

Each architecture is now built once, pushed by digest with no tag, then pulled back and tested through the registry. The manifest list is assembled from the digests that passed. A digest with no tag on it is not published in any useful sense, so a failed run leaves no tag naming bytes that nothing ran.

The run summary now prints the whole pin. FluxOS records a tag and a per-architecture image id, and an id is the digest of that image's own config — which cannot be derived from the manifest list digest, so the old summary could not fill in the file it existed to fill in. Verified against the published v1.0.0: the same commands reproduce the two ids currently pinned.

Actions are pinned to commits, since every job holds packages: write. A concurrency group stops two pushes to main racing for the same tag.

Also here

README.md gains what a hostile archive cannot do, with a column for which layer actually stops each thing — traversal, absolute member names and writing through a symlink are all refused by GNU tar rather than by us, and the setuid bit is left alone because FluxOS mounts the volume nosuid. Backed by container tests rather than asserted, because that is how most of the above shipped.

scripts/test.sh runs both halves in one command, and fails loudly if docker is missing rather than reporting a pass for half a suite.

Verification

Unit suite with -race, container suite on a locally built image, gofmt over cmd and test, vet with and without the docker tag, and clean builds for linux/amd64, linux/arm64 and darwin — all green via ./scripts/test.sh.

The workflow itself is the one thing that cannot be verified locally. Its YAML parses, every run: block passes bash -n, and the summary logic is proven against the real v1.0.0 — but job wiring and push-by-digest only prove themselves on a real run. The publish path is skipped on pull requests by design, so this PR exercises the build and test path only.