Fail the image build when the Harper install fails - #637
Conversation
The `RUN <<-EOF` heredocs run under `sh` without `set -e`, so only the last command's exit status reaches Docker. A failed `npm install --global` was masked by the `rm`/`mkdir`/`chown` cleanup that follows it, and the layer exited 0. v5.2.0-beta.4 shipped that way: `npm install --global` died with ETARGET (`@aws-sdk/core@^3.977.4` was published 87s after the build resolved it), all four standard build legs went green, and the pushed image contained no harper at all — only the `uWebSockets.js` tree a later COPY laid down. Containers on it crash-loop with `setpriv: failed to execute harper: No such file or directory` (exit 127). Add `set -e` to every heredoc RUN in the three Dockerfiles, and assert the install with `harper version` so an install that exits 0 without producing a working bin also fails the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed; no blockers found. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates Dockerfile, Dockerfile-gpu, and Dockerfile-openshift to include set -e in several RUN blocks, ensuring that failures during the build process are not masked. It also adds a harper version check immediately after the global npm installation of Harper Pro to verify that the binary resolves and runs successfully. I have no feedback to provide as there are no review comments.
GPU image builds have failed on every 5.2 beta since beta.2 with:
ERROR: failed to build: failed to solve:
base name (${CUDA_RUNTIME_IMAGE}) should not be blank
An ARG referenced by a FROM must be declared in the global scope, before any
stage. #496 added `ARG CUDA_RUNTIME_IMAGE` immediately above the `FROM
${CUDA_RUNTIME_IMAGE} AS run` line, but that position is after `FROM ... AS
build`, so the ARG belonged to the build stage and the later FROM expanded it
to an empty base name.
Moves the ARG (with its comment) above the first FROM, alongside
NODE_BUILD_VERSION and NODE_VERSION, which are already global for exactly this
reason. The default and the override contract are unchanged — a T4 host pinned
to an older driver branch still overrides with
`--build-arg CUDA_RUNTIME_IMAGE=nvidia/cuda:12.6.3-cudnn-runtime-ubuntu22.04`.
Added a note on the ordering constraint so it does not regress.
Only the GPU images were affected; standard, pointer-compression and OpenShift
builds do not use this ARG and have been publishing normally. 5.1.x is
unaffected — it has no CUDA_RUNTIME_IMAGE ARG at all.
The CUDA runtime base was ubuntu22.04 (glibc 2.35). This image bundles uWebSockets.js, copied from the build stage, and its prebuilt Linux binaries link against GLIBC_2.38 — so the addon could not load on that base. It is the same failure documented at the top of ./Dockerfile, which is why the standard image runs on Debian trixie rather than bookworm (glibc 2.36). Switches the default to nvidia/cuda:13.3.0-cudnn-runtime-ubuntu24.04 (glibc 2.39) and updates the CUDA 12.x override example to the matching ubuntu24.04 tag. All four tag combinations were confirmed present on Docker Hub. Documents the glibc floor next to the ARG: the point of this ARG is that operators override it for older driver branches, and an override back to an ubuntu22.04 base would silently break uWS again. Latent until now — the build has failed outright since #496, so this base was never actually produced. Credit: flagged by gemini-code-assist on #628.
Two problems the ubuntu24.04 base exposed.
1. Ubuntu 24.04 images ship a default `ubuntu` user at uid/gid 1000, so
`groupadd --gid 1000` / `useradd --uid 1000` failed:
groupadd: GID 1000 already exists
useradd: UID 1000 is not unique
harperdb has to own 1000 — existing deployments chown their data volumes to
it — so rename the incumbent rather than moving harperdb to another id. This
is what ./Dockerfile already does for the node image's `node` user. Handles
both shapes, since CUDA_RUNTIME_IMAGE is overridable and 22.04 bases ship no
uid-1000 user.
2. The setup block had no `set -e`, so both failures above were swallowed: the
block continued and exited 0 on the trailing `rm -rf`, producing an image
with no harperdb user. That surfaced much later and far from the cause, as
unable to find user harperdb: no matching entries in passwd file
on the pnpm step. Adding `set -e` makes the block fail at the real cause.
Verified the reclaim logic under sh with both base shapes (uid 1000 present and
absent) — the rename path and the create path each exit 0.
|
Reviewed Incremental re-review since — |
DavidCockerill
left a comment
DavidCockerill
left a comment
There was a problem hiding this comment.
Approving — and thanks for turning this round so fast off the #628 thread.
Checked completeness rather than the idea: all 7 heredoc RUN blocks across the three Dockerfiles now carry set -e, and the harper version assertion is positioned inside the install block — after the install, before cleanup — so a failure actually fails the build rather than deferring to container start.
One gap left in the thread, same bug class but not a heredoc so set -e can't reach it: the two wget … | bash - pnpm pipelines.
— Reviewed by DAIvid (Claude Opus 5)
| # exit status of the cleanup commands that follow it, and `harper version` asserts | ||
| # the installed bin actually resolves and runs: v5.2.0-beta.4 published an image | ||
| # with no harper in it off a green build when npm install hit a transient ETARGET. | ||
| RUN <<-EOF |
There was a problem hiding this comment.
Non-blocking, and about line 43 above rather than this hunk (same shape in Dockerfile-gpu:53): RUN wget -qO- https://get.pnpm.io/install.sh | ... bash - is the last uncovered instance of the bug this PR closes.
A pipeline's exit status is the RHS's, so a failed download hands bash - an empty stdin and it exits 0 — image ships with no pnpm, build green. set -e can't help here; the RUN already succeeded. There's no SHELL directive in any of the three files, so RUN is /bin/sh -c (dash, no pipefail), and nothing sets pipefail anywhere in the repo.
Narrower blast radius than the Harper case — pnpm only matters for components declaring devEngines.packageManager — but worth closing: download to a file and run it, or move it into a heredoc with an explicit bash SHELL and set -eo pipefail.
If wget fails, bash runs on empty input and exits 0, so the build continued without pnpm. Download to a file first so each step's failure fails the build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the remaining gap from the review: the two — KrAIs (Claude Fable 5) |
3 participants
What happened
v5.2.0-beta.4's published image contains no Harper. Containers on it crash-loop:The
@harperfast/harper-prodirectory in that image holds 22 files, all of them theuWebSockets.jstree that the laterCOPYatDockerfile:59lays down. A good image has ~34,000. Thenpm install --globallayer is 238 MB where beta.3's is 827 MB.Root cause
RUN <<-EOFheredocs execute undershwith noset -e, so only the last command's exit status reaches Docker. The install block ends in cleanup:From the beta.4 release build log (run 30602239501, job
build (24, linux/amd64)):The trigger was a registry race, not a code change: the build resolved
@aws-sdk/core@^3.977.4at 03:45:28Z; that version was published at 03:46:55Z — 87 seconds later. TheDockerfileis byte-identical between beta.3 and beta.4.All four standard
buildlegs went green and the manifest was pushed. (The run is marked failed, but only because of the unrelatedbuild-gpulegs — the standard image published normally.)The fix
Two layers, because the failure has two independent halves — the error was thrown away, and nothing downstream noticed the result was empty:
set -ein every heredocRUNacross all three Dockerfiles (6 blocks; only the pointer-compression block atDockerfile:71already had it). This alone turns beta.4's build red.harper versionas a post-install assertion, so an install that exits 0 without producing a working bin still fails the build.set -eonly catches a non-zero exit; this catches a silently incomplete one.The non-install blocks get
set -etoo — same masking applies there, e.g. a failedapt-get install zstdwould ship an image withoutzstd.Verification
Heredoc masking, reproduced minimally — failing command followed by succeeding cleanup:
set -eset -eThe assertion, run against the real published images:
harper versionin aRUN5.2.0-beta.3(good)5.2.0-beta.3, no false positive5.2.0-beta.4(broken)exit code: 127That's the same exit 127 the crash-looping containers hit — caught at build time instead of in production.
Notes for reviewers
5.1.24,5.1.25,5.1.26and5.2.0-beta.3— all have a workingharperbin. No customer release is affected.5.2.0-beta.4on Docker Hub is still broken and this PR does not fix it. It needs a rebuild —@aws-sdk/core@3.977.4resolves now, so a re-run should produce a good image. Worth deciding whether the tag gets re-pushed or pulled.harper versionruns as root inDockerfile-openshift(that stage isUSER 0). It's placed before thechown -R 1001:0, so anything it might touch is still swept up by the existing ownership fixup.mergejob inpublish-docker.yamlhas no post-build smoke check, whilemerge-pchas one (Verify pointer compression and rebuilt uWS, line 229). Adocker run ... harper versionthere would be an outer net catching a broken image from any cause, not just this layer. Happy to add it here or separately — say which.🤖 Generated with Claude Code (Opus 5)