Uh oh!
There was an error while loading. Please reload this page.
fix(scripts): stop early-exit pipe consumers from aborting or misbranching - #680
Conversation
…ching The remaining client sites from the fleet audit of the `producer | head` / `| grep -q` / `| awk '...exit'` hazard under `set -euo pipefail`. The consumer closes the pipe, the producer takes SIGPIPE, pipefail makes the pipeline 141, and errexit either kills the script before its own diagnostics run or -- inside an `if` -- reads 141 as "no match" and takes the wrong branch. Same transform everywhere: capture the producer, slice the captured value with a here-string, so the producer always runs to completion. - resolve-ingestor-digest.sh:152 -- awk exits at ~line 3, so nearly the whole producer lands after the close; aborted before the "could not resolve a digest" diagnostic - lib/provision.sh:101 -- aborted _report_create_failure before warn/hint printed why provisioning failed, which is the whole function - lib/preflight.sh:107 -- `mount` on a container-heavy host exceeds 64KB; 141 aborted the installer inside preflight with no message - lib/summary.sh:69,73,84,89 -- inside `if`, so a misclassified CLIENT_STATE: "starting" instead of the bad-creds / image-pull-CA / crash remedy - migration-tools/migrate-tenant.sh:242 -- migration died mid-verification - lib/setup-linux.sh:93-94 -- dropped the KMODS_REBOOT_REQUIRED hint - release-helm-chart.yaml:356,358 -- lost the diagnostic dump Short-circuits and exit statuses are preserved: setup-linux still runs neither find unless $missing is set and keeps the block status 0; provision's empty capture still takes the else-branch; migrate-tenant prints nothing rather than a blank line when no PVC block matches. Two regression tests in summary.bats, following the pattern cluster.bats:282 and check-facts.bats:159 already use for this class. Note the first version of those tests was vacuous -- with the match appended AFTER the 64KB of noise, grep must read the whole stream and never closes early, so they passed against the unfixed code. Moving the match to the front makes them fail against the old summary.sh and pass against the new one. For `grep -q` the trigger is match position, not just producer size; `head -N`/`awk exit` close early regardless. Refs tracebloc/backend#1778 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 12, 2026
Full |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 36a8df5. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…libs The bootstrap verifies every sub-script it fetches against this manifest before running the privileged steps, so editing preflight/provision/ setup-linux/summary without regenerating it makes the installer refuse its own scripts. Produced by scripts/gen-manifest.sh; the four new hashes match exactly what CI computed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…o pipefail
I converted two pipelines in release-helm-chart.yaml on the assumption the
step ran under pipefail. It does not: the step has no `shell:` key, the
workflow and job have no `defaults.run.shell`, and the body sets no
`pipefail`, so Actions runs it as `bash -e {0}` -- errexit on, pipefail OFF.
Without pipefail the pipeline's status is the CONSUMER's, so a SIGPIPE'd
producer cannot change the outcome. There was no bug to fix there, and a
churn diff on a release workflow is not free.
The six shell-script sites in this PR are unaffected by this revert and stay:
resolve-ingestor-digest.sh and migrate-tenant.sh set `set -euo pipefail`
themselves, and preflight/provision/setup-linux/summary are sourced by
install-k8s.sh and install.sh, both of which do.
Refs tracebloc/backend#1778
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>LukasWodka
commented
Aug 12, 2026
Scope correction — dropped the |
| file | pipefail comes from |
|---|---|
scripts/resolve-ingestor-digest.sh | own set -euo pipefail |
docs/migration-tools/migrate-tenant.sh | own set -euo pipefail |
scripts/lib/preflight.sh | sourced by install-k8s.sh (L61) and install.sh — both set -euo pipefail |
scripts/lib/provision.sh | sourced by install-k8s.sh (L82) and install.sh |
scripts/lib/setup-linux.sh | sourced by install-k8s.sh (L66) and install.sh |
scripts/lib/summary.sh | sourced by install.sh and check-facts.sh |
Also in this push: scripts/manifest.sha256 regenerated (1d5b131)
The Static-analysis failure was mine — the bootstrap verifies every sub-script it fetches against that manifest before the privileged steps, so editing four libs without regenerating it makes the installer refuse its own scripts. Regenerated with scripts/gen-manifest.sh; the four hashes match exactly what CI computed.
On the PATH persist — alpine:3 failure — not from this PR
✖ zsh install.sh (SHELL=zsh) exited 1, caused by curl failing TLS verification while fetching SHA256SUMS ("failed to download SHA256SUMS — release may be malformed"). bash and fish both passed in the same job. develop's own Installer-tests run fails the same way (on fedora:latest, ✖ bash), so it is environmental rather than a regression here.
I had first suspected my here-strings broke busybox ash on Alpine — that was wrong: every file changed is #!/usr/bin/env bash, and the job explicitly bootstraps bash (apk add bash; exec bash scripts/tests/path-persist.sh).
Bugbot, correctly: I added `errmatches` in _report_create_failure without extending the `local` declaration on the line above, so it leaked into the installer's global scope. provision.sh is sourced by install-k8s.sh and install.sh, so a leaked name can collide with any other lib's variable. Swept the rest of the branch for the same mistake: every variable this PR introduces in a sourced lib (mount_out, mod_this_kernel, mod_any_kernel, errline) is already declared local. The two new names in the standalone scripts (imagetools_out in resolve-ingestor-digest.sh, manifest_out/policy_ctx in migrate-tenant.sh) are at top level in scripts nothing sources, so they are correct as-is. manifest.sha256 regenerated for the new provision.sh hash. 175 bats tests across summary/preflight/install-bootstrap pass; shellcheck clean on provision.sh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saadqbal
left a comment
There was a problem hiding this comment.
Clean, careful PR 👍 Same capture-then-here-string transform everywhere, behavior preserved. Verified: the errmatches local-leak was the only real trap and the head commit already scopes it; every new var in a sourced lib (mount_out, mod_this_kernel/any_kernel, errline/errmatches) is local, standalone-script vars are top-level as intended. Reran the mutation check — both new summary.bats tests fail against the old pipe matcher and pass against the fix, so they genuinely exercise the SIGPIPE path. summary 28/0, preflight 124/0, setup-linux 133/0; shellcheck clean apart from the pre-existing SC2034s; manifest hashes match the four changed libs.
Uh oh!
There was an error while loading. Please reload this page.
…b runs first #680 swept this hazard across the fleet but did not reach setup-macos.sh, whose _macos_user_is_admin is the FIRST command step b executes. printf '%s\n' $groups | grep -qx admin `grep -q` stops at its first match and `admin` sits near the FRONT of a macOS group list, so printf is often still writing when the pipe closes: SIGPIPE, pipefail, 141. The caller reads that as "not an administrator" and hard-fails a perfectly fine machine with the managed-Mac remedy. Reproducible, not theoretical — with a long group list the old form returns 141 on every run and the new one returns 0: old=141 new=0 (x5) Match POSITION is the trigger, not producer size, so a directory-bound or MDM-managed Mac with a long group list hits it and a short one does not. Same transform as #680 (capture, then match with a here-string), plus two more sites in the same file and one in common.sh where a SIGPIPE'd producer inside an `if` would MISBRANCH rather than abort: - setup-macos.sh: hw.optional.arm64 -> would call an Apple Silicon Mac amd64 and fetch the Intel Docker Desktop DMG - setup-macos.sh: the Docker.app arch probes -> `case`, which also drops the `A && B` set -e subtlety - common.sh: the load-time ARCH override -> would pick the wrong download for every pinned tool on Apple Silicon Mutation-real regression test in setup-macos-lifecycle.bats, driven through a real script so pipefail is genuinely in force (943 bats, green). Refs tracebloc/backend#1778 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_cluster_exists is the function client#682 names, and guarding the daemon-down case did not finish the job: all three of its probes piped k3d into a consumer that stops at the FIRST matching line — and our own cluster is usually that line. k3d takes SIGPIPE, pipefail makes the pipeline 141, and inside these `if`s that reads as "no such cluster". The gate then calls a machine with a live, running cluster FRESH and offers a first-time install: the same user-visible bug as a down daemon, reached a completely different way. Capture-then-match (#680's transform) on all three probes, which also spares two extra k3d invocations. Same fix in two more spots in this file: - _handle_existing_cluster's non-jq server count — awk `exit` closes the pipe on our row, so this could abort the installer mid-reconcile with no message - the proxy-env check — grep -Eq stops at the first match, so a present variable could be reported MISSING and produce a spurious warning Two tests in cluster.bats. Note the "found" one is mutation-real only against the WHOLE pre-fix function: reverting probe 2 alone still passes, because probe 3's grep fallback finds the cluster anyway — the vacuity trap #680 called out. The test comment says so. 946 bats, green. Refs tracebloc/backend#1778 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…time a fresh machine (client#681, client#682) (#683) * fix(installer): say where a step died, and stop calling a stopped runtime a fresh machine Two failures that were reported together, both bash-only gaps the PowerShell installer had already closed. 1. A step could fail with zero diagnostics (client#681). Under `set -euo pipefail` a command failing outside an if/&&/|| context killed the run with no output at all — and the install log, which is the whole session tee'd, recorded nothing either, so "check the install log" led to a log that said nothing. There was no ERR trap anywhere in scripts/. Adds an ERR trap (armed with `set -E`, without which it would only fire at top level and miss every failure inside install_macos/install_linux) that records file:line, the unexpanded command, and the exit status. The closer names the site on screen and logs the command; step b logs a breadcrumb per stage so the log narrows the failure even if the trap is bypassed. Ctrl-C and SIGTERM now read as "interrupted", not as an installer failure — they were indistinguishable from a real one, on screen and in the log. Counterparts: Show-FatalError / Show-Interrupted (#577), Err's detail lines (#423). 2. A stopped container runtime was classified as a fresh machine (client#682). `_cluster_exists` is a boolean whose three probes all swallow stderr and return 1, so a down daemon looked exactly like an empty machine: a laptop that only needed Docker started was told "setting up for the first time". Classifies an installed-but-unreachable runtime as degraded/runtime-down before the cluster probe, and says so. Deliberately narrow: no docker binary is still fresh, and "permission denied" is a different remedy that keeps its own path. The run CONTINUES — install_docker_desktop already starts Docker Desktop and create_cluster reconciles the existing cluster, and taking that away would trade one bad outcome for another. The bug was the claim, not the flow. Counterpart: the tri-state Get-ClusterRunStateFromList (#557). Covered by 15 new bats tests (940 total, green), including the pipefail/SIGPIPE death class that previously produced no output whatsoever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(macos): close the early-exit pipe hazard in the admin check step b runs first #680 swept this hazard across the fleet but did not reach setup-macos.sh, whose _macos_user_is_admin is the FIRST command step b executes. printf '%s\n' $groups | grep -qx admin `grep -q` stops at its first match and `admin` sits near the FRONT of a macOS group list, so printf is often still writing when the pipe closes: SIGPIPE, pipefail, 141. The caller reads that as "not an administrator" and hard-fails a perfectly fine machine with the managed-Mac remedy. Reproducible, not theoretical — with a long group list the old form returns 141 on every run and the new one returns 0: old=141 new=0 (x5) Match POSITION is the trigger, not producer size, so a directory-bound or MDM-managed Mac with a long group list hits it and a short one does not. Same transform as #680 (capture, then match with a here-string), plus two more sites in the same file and one in common.sh where a SIGPIPE'd producer inside an `if` would MISBRANCH rather than abort: - setup-macos.sh: hw.optional.arm64 -> would call an Apple Silicon Mac amd64 and fetch the Intel Docker Desktop DMG - setup-macos.sh: the Docker.app arch probes -> `case`, which also drops the `A && B` set -e subtlety - common.sh: the load-time ARCH override -> would pick the wrong download for every pinned tool on Apple Silicon Mutation-real regression test in setup-macos-lifecycle.bats, driven through a real script so pipefail is genuinely in force (943 bats, green). Refs tracebloc/backend#1778 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(assess): permission-denied must beat the connection match, not lose to it Bugbot, correctly. The real Linux docker-group error contains BOTH the permission wording and a `dial unix …` clause: permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Get "http://…/info": dial unix /var/run/docker.sock: connect: permission denied so matching the connection phrases first classified a docker-group problem as a down daemon and answered it with "start Docker" — the exact confusion _assess_runtime_down exists to prevent. Checks permission-denied FIRST and returns not-down. A negative match before the positive one is the only ordering that survives an error string containing both. The test was vacuous for the same reason: its fixture was a shortened message with no `dial unix`, so it passed against the broken code. Both fixtures are now the real full messages (the `permission denied` and `Got permission denied` variants), and are mutation-real — dropping the guard fails them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cluster): close the second route to the client#682 misclassification _cluster_exists is the function client#682 names, and guarding the daemon-down case did not finish the job: all three of its probes piped k3d into a consumer that stops at the FIRST matching line — and our own cluster is usually that line. k3d takes SIGPIPE, pipefail makes the pipeline 141, and inside these `if`s that reads as "no such cluster". The gate then calls a machine with a live, running cluster FRESH and offers a first-time install: the same user-visible bug as a down daemon, reached a completely different way. Capture-then-match (#680's transform) on all three probes, which also spares two extra k3d invocations. Same fix in two more spots in this file: - _handle_existing_cluster's non-jq server count — awk `exit` closes the pipe on our row, so this could abort the installer mid-reconcile with no message - the proxy-env check — grep -Eq stops at the first match, so a present variable could be reported MISSING and produce a spurious warning Two tests in cluster.bats. Note the "found" one is mutation-real only against the WHOLE pre-fix function: reverting probe 2 alone still passes, because probe 3's grep fallback finds the cluster anyway — the vacuity trap #680 called out. The test comment says so. 946 bats, green. Refs tracebloc/backend#1778 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(cluster): capture each k3d listing lazily, inside the probe that reads it Asad on #683: `_list` was captured at the top of _cluster_exists but is only read by probe 2. On the common re-run — jq present, our cluster found by the JSON probe — that shell-out ran and was thrown away, so the comment claiming the capture "spares two extra k3d calls" was backwards for exactly the path that matters: it ADDED one. Each capture now sits inside the probe that reads it, so a probe that never runs never shells out. The k3d call count is identical to the pre-fix code, and the common path is back to one call. Comment corrected to say that rather than the opposite. No behaviour change; 946 bats green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…686) (#688) * fix(scripts): retire the remaining early-exit pipe consumers (client#686) The sites #680 and #683 did not reach. Same transform: capture the producer, match the captured value, so the producer always runs to completion. `case` where the needle is a fixed substring — it also drops the `A && B` set -e subtlety two of these carried. Fixed — misbranch (inside `if`/`&&`, so pipefail's 141 reads as "no match"): - detect-gpu.sh:28,34 -- `lspci | grep -qi` -> GPU_VENDOR left "none" on a GPU host, i.e. a CPU-mode cluster. lspci is the one producer here that is routinely large enough to lose the race on its own (a dense server enumerates well past a stdio buffer), and one capture now serves both probes plus the AMD label. - install-client-helm.sh:449 -- repo believed absent -> re-runs `helm repo add`, which is unguarded on the next line and fails when the name exists with a different URL, escalating the misbranch into an aborted install. - install-client-helm.sh:594 -- loses --reset-then-reuse-values, so a reconcile silently stops picking up new chart defaults. `helm upgrade --help` is several KB in chunks and the flag sorts early. - install-client-helm.sh:835 -- sticky 8.4 lost -> resolves 5.7 against an 8.4 datadir, which MySQL 5.7 will not open. - setup-linux.sh:281,348 -- docker-group membership misread; 348 is nested now so the two mode guards still short-circuit ahead of `id`, which the old `&&` also did. - setup-linux.sh:898 -- nvidia runtime not detected -> CPU-only cluster on a Tier-0 GPU host that already has the toolkit. - setup-linux.sh:1151 -- the capture was already there (Asad #458); this drops the leftover `printf | grep -q` re-pipe of it. Fixed — abort: - preflight.sh:100 -- `findmnt | head -1` in an ASSIGNMENT, so 141 aborts the installer inside preflight with no message. The sibling mount pipeline two lines down was fixed in #680; this one was missed. Note errexit only propagates out of a command substitution on bash >= 4.4, so this bites on Linux (where findmnt exists at all) and not on the macOS system bash. Hardening, not live bugs — the shape is retired but the abort cannot happen today, and the commit says so rather than implying a field fix: - install.sh:538 -- the cosign checksum slice. Its only caller is `if ! ensure_cosign`, and a condition context suppresses errexit for the whole function, so the 141 is swallowed and `want` is already correct. Retired anyway: a function in the signature-verification path should not depend on how its caller happens to be written. - common.sh:262 -- argument position, where a 141 never trips errexit. Deliberately NOT changed, with the reason, so the next sweep does not re-open them: - diagnose.sh:61,96 -- `run_diagnose` runs `set +e` as its first statement, so no site in that function can abort. The support bundle was never at risk. - gpu-plugins.sh:112 -- the `|| echo ""` already guards it, and `head -5` has emitted its lines before the SIGPIPE propagates, so RAW keeps the correct value (verified: the pre-fix pipeline returns 141 but RAW is intact). - detect-gpu.sh:22,23,36 -- argument position inside `success`/`log`. - preflight.sh:727, common.sh:393, and the `awk`-without-`exit` sites -- a builtin printf under the buffer, an existing `|| true`, or a consumer that reads to EOF. 14 tests across 4 files, every one checked against the pre-fix code. Two things make them non-vacuous and both were got wrong first: the match must LEAD (a trailing match makes grep read the whole stream), and the filler must come from an EXTERNAL command — a producer built from bash builtins, or a mock ending in `return 0`, masks the SIGPIPE and the test passes unfixed. The preflight test additionally calls the function BARE, because the production command- substitution shape cannot abort on the bash 3.2 the suite runs on locally. setup-linux.bats' `id -nG` shape assertion is updated: it pinned the old `| grep -qw docker` text. It still pins what it was written to pin — that both probes key off $_grant_user and never bare $USER. Refs tracebloc/backend#1778 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(supply-chain): regenerate manifest.sha256 for the five changed libs The bootstrap verifies every sub-script it fetches against this manifest before running the privileged steps, so editing common/detect-gpu/install-client-helm/ preflight/setup-linux without regenerating it makes the installer refuse its own scripts. Produced by scripts/gen-manifest.sh. install.sh itself is the bootstrap and is not listed in its own manifest, so the cosign change there needs no hash. Refs tracebloc/backend#1778 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

The remaining
clientsites from the fleet-wide audit of this hazard class (tracebloc/backend#1778). Follows #678, which fixed the one live instance indocker/k3s-cuda/build.sh; these are the latent ones in the same repo. All mechanical, all the same transform: capture the producer, then slice the captured value with a here-string, so the producer always runs to completion.The shape
producer | head -N/| grep -q/| awk '…exit'underset -euo pipefail. The consumer closes the pipe early, the producer dies of SIGPIPE,pipefailmakes the pipeline status 141, and errexit either aborts the script before its own diagnostics run, or — worse, inside anif— reads 141 as "no match" and silently takes the wrong branch.Sites
scripts/resolve-ingestor-digest.sh:152awk '/^Digest:/ … exit}'scripts/lib/provision.sh:101head -4_report_create_failurebeforewarn/hintprint why provisioning failed — the function's entire purposescripts/lib/preflight.sh:107awk … exitmounton a container-heavy host genuinely exceeds 64 KB → 141 aborts the installer inside preflight with no messagescripts/lib/summary.sh:69,73,84,89grep -qif→ misclassifiedCLIENT_STATE: the user gets "starting" instead of the bad-creds / image-pull-CA / crash remedydocs/migration-tools/migrate-tenant.sh:242head -25scripts/lib/setup-linux.sh:93-94grep -q .if→ drops theKMODS_REBOOT_REQUIREDhint.github/workflows/release-helm-chart.yaml:356,358grep -q,head -25A correction to the audit's severity model
Writing the regression tests turned up something that changes how these should be read, and I got it wrong on the first attempt.
For
grep -qsites the hazard depends on where the match is, not just on producer size. If the matching line is near the end, grep must read the whole stream to find it, never closes early, and no SIGPIPE occurs. Only a match before the producer finishes triggers it.My first two regression tests appended the matching line after 64 KB of noise — and passed against the unfixed code. They were vacuous. Moving the match to the front made them fail against the old
summary.shand pass against the new one. Worth stating plainly because it means "producer > 64 KB" alone is not the trigger for thegrep -qsites;head -Nandawk … exitsites close early unconditionally and are the strictly worse shape.Behaviour-preserving details worth checking in review
provision.sh:head -4 <<<"$errmatches"on an empty capture yields an empty substitution (trailing newlines stripped), so[[ -n "$errline" ]]still takes the else-branch exactly as before.setup-linux.sh: the short-circuit is preserved — neitherfindruns unless$missingis non-empty — and the nestedifkeeps the block's exit status 0 when the condition is false, so it can't trip errexit as a trailing[[ … ]] && …would.migrate-tenant.sh: guarded so nothing is printed when no PVC block matches, rather than the blank line a bare here-string would emit.summary.sh: one shared comment above_diagnose_not_readyinstead of four copies of the same note.resolve-ingestor-digest.sh:156(docker manifest inspect --verbose | grep -m1 | sed) is left as-is:grep -m1closes early on the fleet's largest producer, but|| trueoutside the substitution already swallows the status and the value is captured correctly.seddrains, so there's no second early-exit.Test plan
Two new regression tests in
summary.bats, following the house pattern already established bycluster.bats:282,check-facts.bats:159-183andgpu-amd.bats:71-93for this exact bug class.Plus the full
scripts/tests/*.batssuite (result appended in a comment below).Mutation-checked: against the pre-change
summary.sh, both new tests fail (not ok 4,not ok 5); with the fix, 28/28 pass.Also verified:
bash -non all six changed shell files,yaml.safe_loadon the workflow, andshellcheck --severity=warning— the only findings are 3 + 2 pre-existing SC2034s on untouched lines (identical counts ondevelop; CI gates libs at--severity=errorfor exactly that reason).Note
Medium Risk
Touches installer, provisioning, preflight, and migration shell paths that run under
set -euo pipefail; behavior is intended to be preserved, but incorrect capture/slicing could still hide diagnostics or misclassify client readiness.Overview
Closes the remaining backend#1778 sites where an early-closing pipe consumer (
head,grep -q,awk … exit) made producers die with SIGPIPE underpipefail, either aborting the script or silently taking the wrongifbranch.Applies the same mechanical transform everywhere: capture the producer, then slice via a here-string so the producer always finishes. Highest-impact sites are
resolve-ingestor-digest.sh(digest resolve aborted before its error path),_report_create_failureinprovision.sh(failed before printing why create failed), and_diagnose_not_readyinsummary.sh(could downgrade crash/bad-creds/image-pull to “starting”).Also hardens
preflight.sh(mountfstype probe),setup-linux.sh(kernel-module reboot hint), andmigrate-tenant.sh(manifest keep-annotation check). Adds twosummary.batsregression tests that put the match before >64KB of noise so the SIGPIPE case is actually exercised.Reviewed by Cursor Bugbot for commit 3dd60d0. Bugbot is set up for automated code reviews on this repo. Configure here.