Repository navigation
fix(docker): stop clobbering the base image's /usr/lib, and build images on main - #3157
Conversation
The all-in-one and local images copied the whole /usr/lib out of node:22.22-alpine on top of the ClickHouse base image, overwriting libapk.so, libssl, libcrypto and libz. That only worked while both images happened to sit on the same Alpine point release. Bumping CLICKHOUSE_VERSION to 26.8 (#3142) moved the base to Alpine 3.24.2 / apk-tools 3.0.8 while node:22.22-alpine is still on 3.24.1 / 3.0.6, so the copy left a 3.0.6 libapk.so under a 3.0.8 /sbin/apk and the next apk call died with exit 127: Error relocating /sbin/apk: apk_fs_is_malicious_filename: symbol not found Node only needs libstdc++ and libgcc from that directory, so install those with apk from the image's own Alpine release and drop the blanket copy. They go in before the v3.9 mongodb repositories are appended so they resolve against 3.24 rather than 3.9. Verified by building all-in-one-noauth (amd64) and all-in-one-auth (arm64): apk, node, clickhouse-server and mongod all run, libapk and OpenSSL are owned by the base image's packages again, and scripts/ci/smoke-all-in-one.sh gets a canary log end-to-end through the collector into ClickHouse.
🦋 Changeset detectedLatest commit: 8af1390 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Additional context: touches background tasks or the delivery pipeline lightly (15 lines, under the 30-line bar for Tier 4) Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
 |
PR Review
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 2 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
|
/* filled below */ |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
E2E Test Results✅ All tests passed • 374 passed • 1 skipped • 1033s
Tests ran across 4 shards in parallel. |
The apk break that killed the last release came in from outside the repo. The Dockerfile pins floating upstream tags, and Docker Hub re-pointed clickhouse-server:26.8-alpine from ClickHouse 26.8.6 to 26.8.7 around 15:00 UTC, taking apk-tools from 3.0.6 to 3.0.8 with it. Nothing we had would catch that: 21:22 (D-1) PR #3142 Docker Build -> passes, base @9f10a193 (3.0.6) 00:36 nightly -> passes, base @9f10a193 (3.0.6) ~15:01 upstream re-tags 26.8-alpine -> @2bece7c6 (3.0.8) 16:17 merge to main -> no image build 17:48 merge to main -> no image build 19:31 Release -> fails Both gates were green because both ran before the re-tag, and the two merges that followed it built no images: Docker Build only ran on pull requests, and its path filter would have skipped those diffs anyway. The next thing to touch the new base was the release itself. So run Docker Build on pushes to main too, and ignore the path filter there. The filter is right for pull requests, where the question is "did this diff break the image" and staying fast matters; it is the wrong instrument on main, where the question is "does the image still build today". Both all-in-one targets and the end-to-end smoke test run as they already do on pull requests, so a drifted base now shows up as a red build on the next merge rather than in a release hours later. The twelve copies of the changed-files condition collapse into one `gate` step per job, since the condition now has two variables rather than one. Pull-request and workflow_dispatch behaviour is unchanged. Concurrency is keyed on the event as well as the ref: `github.ref` is the same for every push to main, and the existing cancel-in-progress group would have each merge cancel the one before it and leave that commit unverified. Pull requests keep cancel-on-new-push. Also set `pull: true` on the nightly's three image builds and on the all-in-one PR build, so the floating tags are re-resolved against the registry instead of whatever the runner happens to hold. Fresh runners already re-resolve, so this is hygiene rather than the fix — it keeps holding if these ever move to a persistent builder.
wrn14897
changed the title
Deep ReviewScope: ✅ No critical issues found. The gating rewrite is behavior-preserving for 🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers (8): correctness, reliability, maintainability, project-standards, testing, adversarial, previous-comments, learnings-researcher. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # clickhouse-server:*-alpine) instead of reusing whatever the runner | ||
| # already has. Those tags get re-pointed on upstream patch releases, | ||
| # and this nightly is the first thing that should notice. | ||
| pull: true |
There was a problem hiding this comment.
🔵 minor — pull: true added to the nightly but not to the release builds, which use the same persistent runner labels
release.yml builds the app, Local and All-in-One images on Large-Runner-x64-32 / Large-Runner-ARM64-32 (.github/workflows/release.yml:885-888, 972-975, 1084-1087) — exactly the runner labels the three nightly jobs that just got pull: true use. If the stated rationale ("keeps holding if these ever move to a persistent builder") is worth encoding, the release path is where it matters most, since that is the build whose output ships and the one that actually failed here. Add pull: true to the three docker/build-push-action steps in release.yml at lines 909, 996 and 1108 as well, or drop it from the nightly so the fleet stays consistent.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Not doing this here — it asks the PR to change release.yml, which it does not otherwise touch.
On the substance you are right that the reasoning applies equally, and I was explicit in the commit that pull: true is hygiene rather than the fix: fresh runners re-resolve the tag anyway, which is why the release build picked up the new digest unprompted and failed. It changes nothing today on any of these jobs. So the inconsistency costs nothing either, and I would rather not widen a build-break fix into the release path to tidy it.
Worth doing fleet-wide in a follow-up if we ever move these to a persistent builder, where it would start to matter.
| # cancel the previous one and leave that commit unverified. PRs keep | ||
| # cancel-on-new-push; pushes to main queue instead. | ||
| group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.ref }} | ||
| cancel-in-progress: ${{ github.event_name == 'pull_request' }} |
There was a problem hiding this comment.
🔵 minor — cancel-in-progress: false does not guarantee every main commit is built
GitHub keeps at most one pending run per concurrency group: when a third push arrives while one run is in progress and one is queued, the queued run is cancelled, not deferred. So with three merges inside one ~40-minute all-in-one build, the middle commit is still never verified — the outcome the comment above says this change prevents. Either add github.sha to the group for pushes (${{ github.event_name == 'pull_request' && github.ref || github.sha }}) so every commit gets its own group, or reword the comment to claim only that the tip commit is always built.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Correct, and fixed in 85e50b2 — the comment overclaimed.
GitHub holds one pending run per group, so a third merge during a long build supersedes the queued one rather than joining a queue. The comment now says that plainly and says why it is acceptable rather than pretending otherwise: an upstream base move affects every commit equally, and a Dockerfile break in a superseded commit is still present at the tip, so building the tip is what this job needs. Per-commit attribution comes from the pull request run.
Took the reword over adding github.sha to the group deliberately — per-commit groups would fan out N concurrent 40-minute builds on a busy day to buy attribution we already get from PR CI.
| docker/otel-collector/** | ||
| packages/otel-collector/** | ||
| # Path filtering applies to pull requests only; see the header comment. | ||
| - name: Decide whether to build |
There was a problem hiding this comment.
🔵 minor — The Decide whether to build step adds a job step without removing any if:
if: github.event_name != 'pull_request' || steps.changed-files.outputs.any_changed == 'true' is exactly equivalent (a skipped changed-files step yields '', so the second clause is false on push/dispatch and the first is true), and the downstream steps still carry one if: each either way — so the gate step costs three extra shell steps and buys nothing. Delete the three gate steps and put that expression directly in each if:.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Keeping the gate steps.
The equivalence is right, and if it were only about the boolean I would take the inline version. Two reasons I am not:
- The inline expression has to be repeated on every downstream step — 12 times across the three jobs, which is the duplication this refactor set out to remove. The gate states it once per job.
- It renders the decision in the log.
EVENT_NAME: pull_request/ANY_CHANGED: trueshows up as a step you can read; a skipped step with an inlineif:does not tell you which clause was false. I used exactly that to verify both branches before pushing — the pull_request run and aworkflow_dispatchrun on this branch.
Three no-op shell steps is a fair price for that. Reasonable to disagree; happy to flip it if a maintainer prefers the terser form.
| context: . | ||
| file: ./docker/hyperdx/Dockerfile | ||
| target: all-in-one-auth | ||
| platforms: linux/amd64 |
There was a problem hiding this comment.
🔵 minor — The new push-to-main drift gate only covers linux/amd64
clickhouse-server:${CLICKHOUSE_VERSION}-alpine is a multi-arch tag whose per-arch manifests can be re-pointed independently, and the release builds arm64 on Large-Runner-ARM64-32 (.github/workflows/release.yml:1087); an arm64-only base move still reaches a release with only the daily nightly in between — the same window this PR set out to close. Either add linux/arm64 to the all-in-one build on push events, or note the arm64 gap alongside the digest-pinning item in "Not addressed".
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Noted in the PR description under "Not addressed" rather than changed.
The per-arch-drift risk is real but narrower than it reads: these tags are OCI image indexes and upstream republishes the whole index, which is what happened here — both arches moved together, and the arm64 All-in-One release job failed with the identical relocation error. So an amd64 gate catches the realistic case. A genuinely arm64-only move would fall to the nightly.
Adding linux/arm64 to the push build roughly doubles it (emulated on ubuntu-24.04, or a second runner) for a case we have not observed. Left out until we do.
Review caught that the comment overclaimed. GitHub holds at most one pending run per concurrency group, so a third merge arriving during a long build supersedes the queued one rather than joining a queue — the middle commit is not built. Say that plainly, and say why it is acceptable here: base-image drift affects every commit equally and a Dockerfile break in a superseded commit is still there at the tip, so building the tip is what this job needs. Per-commit attribution comes from the pull request run. Also drops the claim that the event key exists to stop merges cancelling each other; cancel-in-progress already does that. It is there to keep push and pull_request runs in separate groups.
| # clickhouse-server:*-alpine) instead of reusing whatever the runner | ||
| # already has. Those tags get re-pointed on upstream patch releases, | ||
| # and this nightly is the first thing that should notice. | ||
| pull: true |
There was a problem hiding this comment.
🔵 minor — pull: true lands on the nightly and PR builds but not on the release builds that actually ship
The rationale given here — "instead of reusing whatever the runner already has" — applies verbatim to release.yml, whose four docker/build-push-action@v6 steps (lines 829, 909, 996, 1108) run on the same persistent Large-Runner-x64-32 / Large-Runner-ARM64-32 runners as the nightly jobs and got no pull: at all. That leaves the gates and the artifact able to disagree: a warm image store on a release runner can build all-in-one-noauth from a clickhouse-server:26.8-alpine digest that the nightly and the new push-to-main build already re-resolved past, so a green drift check would describe a base the release never used. Add pull: true to the four release builds (and to release-nightly.yml:51, the OTel collector nightly, whose golang:1.26-alpine base floats the same way).
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Correction: I posted this on the wrong thread — it answers the concurrency comment below. The answer to this thread (pull: true on the release builds) is the same as the earlier one: it asks the PR to change release.yml, which it does not touch, so not doing it here.
One correction to the premise — "a green drift check would describe a base the release never used" is the wrong way round for this incident. The release runner did not reuse a warm digest; it re-resolved to the new @2bece7c6 unprompted, which is why it failed while the earlier gates were green. The gates were stale because they ran before the re-tag, not because the release was pinned to something older. pull: true would not have changed any of it.
Still reasonable as fleet-wide consistency, just not in a build-break fix.
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
| # Keyed on the event as well as the ref so that pushes to main and pull |
There was a problem hiding this comment.
🔵 minor — Concurrency comment's stated reason for keying on github.event_name is not true
github.ref is refs/pull/<n>/merge on pull_request and refs/heads/main on push, so a merge and a PR run could never have shared a group — the scenario the comment says the event key prevents cannot occur. What the added key actually does is separate workflow_dispatch on main (also refs/heads/main) from push on main, so manual runs and merges no longer cancel or queue against each other; the merge-cancels-merge problem the PR description describes is solved by cancel-in-progress: false alone. Reword lines 28-30 to say that, or drop github.event_name from the group and rely on cancel-in-progress: ${{ github.event_name == 'pull_request' }}.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Correction: I posted this on the wrong thread — it answers the pull: true comment above. The answer to this thread:
Correct, and fixed in 47e95ac. My reworded comment asserted a collision that cannot happen — refs/pull/<n>/merge vs refs/heads/main. Kept the event key and stated what it actually does: a manual run on main also carries refs/heads/main, so without it a dispatch and a merge contend for one group.
A pull request run and a merge could never have shared a group — `github.ref` is `refs/pull/<n>/merge` on one and `refs/heads/main` on the other. The event key earns its place elsewhere: a manual run on main carries `refs/heads/main` too, and without it would contend with a merge.
| docker/otel-collector/** | ||
| packages/otel-collector/** | ||
| # Path filtering applies to pull requests only; see the header comment. | ||
| - name: Decide whether to build |
There was a problem hiding this comment.
🔵 minor — The gate step adds three identical shell blocks to express a condition GitHub expressions already support inline
The stated motive ("twelve copies of the condition collapse into one gate step per job") doesn't hold: the twelve if: lines are still there, now reading steps.gate.outputs.run == 'true', and three verbatim copies of a 9-line run: block were added on top. The same semantics are one expression — replace each gate step and its four consumers with if: github.event_name != 'pull_request' || steps.changed-files.outputs.any_changed == 'true' (identical length to the expression it replaced at docker-build.yml:92, 98, 128, 134, 182, 188, 209, 220 pre-image), and drop the three Decide whether to build steps at lines 63, 118 and 187 entirely.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Taken, in 8af1390. You're right and my justification was wrong: the if: lines never collapsed — there are eight of them and there still were, just shorter — so the gate bought three copies of a nine-line block and nothing else. Inlined, gate steps gone.
(Eight, not twelve. I said twelve in the commit message and you quoted it back; we were both wrong.)
Re-verified the four event/filter combinations against the old condition: pull_request+changed → build, pull_request+unchanged → skip, workflow_dispatch → build. Only push changes, which is the point of the PR.
| # clickhouse-server:*-alpine) instead of reusing whatever the runner | ||
| # already has. Those tags get re-pointed on upstream patch releases, | ||
| # and this nightly is the first thing that should notice. | ||
| pull: true |
There was a problem hiding this comment.
🔵 minor — pull: true added only where it provably cannot matter, and omitted from the release builds of the same Dockerfile
pull: true lands on the nightly's three builds and on docker-build.yml's all-in-one build — but docker-build.yml runs exclusively on ephemeral ubuntu-24.04 with a fresh buildx container, so there it is a no-op by construction, while release.yml's three docker/hyperdx/Dockerfile builds (lines 909, 996 and 1108, on the same Large-Runner-x64-32/Large-Runner-ARM64-32 labels as the nightly jobs that did get it) were left without it. If tag re-resolution is worth pinning down at all, the publishing path is where a stale resolution is worst: the release would ship an image whose base differs from the one CI verified, with nothing in the tag to say so. Either add pull: true to the three release.yml builds for consistency, or note in the comment why release deliberately reuses whatever the builder holds.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Taken, in 8af1390 — pull: true is gone entirely and release-nightly.yml is reverted, so this PR is down to one workflow file.
You pinned the thing I'd been hand-waving: on docker-build.yml it is dead by construction, and adding it to the nightly while leaving release.yml on the same runner labels made the fleet less consistent rather than more. It was never part of the fix either — the release failed precisely because its runner did re-resolve to the new digest.
Whether these builds should pin tag resolution is a real question, but it belongs in a change that can touch release.yml. Noted as a follow-up in the PR description.
Review was right on both counts. The gate step did not do what I claimed. The `if:` lines never collapsed — there are eight of them and there still were, just shorter — so the step bought three copies of a nine-line shell block and nothing else. The condition goes back inline, one expression, no extra steps. A skipped `changed-files` leaves `any_changed` empty, so the second clause is false on push and dispatch and the first carries it; behaviour is identical to the gate for all four event/filter combinations. (Eight, not the twelve the earlier commit message asserted. That was wrong too.) `pull: true` is gone. On docker-build.yml it was provably dead: these jobs run on ephemeral ubuntu-24.04 with a fresh buildx, which always re-resolves the tag. On the nightly it was unproven, and adding it there while leaving release.yml — same runner labels, and the path that actually ships — without it made the fleet less consistent, not more. It was never part of the fix; the release failed precisely because its runner *did* re-resolve to the new digest. Dropping it reverts release-nightly.yml entirely, leaving this PR one workflow file. Whether these builds should pin tag resolution at all is a real question, but it belongs in a change that can touch release.yml.
| github.event_name != 'pull_request' || | ||
| steps.changed-files.outputs.any_changed == 'true' | ||
| uses: docker/setup-buildx-action@v3 | ||
| - name: Build (no push) |
There was a problem hiding this comment.
🔵 minor — On push to main the App Image job is fully subsumed by the All-in-One job, duplicating the ~8-minute Node build on every merge
all-in-one-base does COPY --from=prod /app /app (docker/hyperdx/Dockerfile:214), so building all-in-one-auth builds the entire prod stage first — any failure the App Image job would catch on a push also fails the All-in-One job. The comment at lines 146-153 says the narrow filter exists precisely to avoid paying that Node build twice, and "Pushes to main ignore the filter entirely" reinstates the duplication on every merge, now on two runners at once. Either gate the App Image job's build on github.event_name == 'pull_request' && steps.changed-files.outputs.any_changed == 'true', or note in the comment that the duplicate build on main is deliberately kept for per-job attribution and for warming the docker-build-pr-app cache scope.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Correct that it is subsumed, but leaving it — the duplication buys a faster signal.
App Image finishes in roughly 8 minutes; All-in-One takes 15-40. For the failure mode this PR is about, a moved node:*-alpine or clickhouse-server:*-alpine base, App Image goes red first and points at the Node build directly rather than at a 22-layer image. They run on separate runners in parallel, so wall-clock to first signal is the cheaper of the two, not the sum.
It is runner-minutes against time-to-detect, and for a drift gate I would rather know sooner. Happy to gate it to pull_request if a maintainer would rather have the minutes back.
| on: | ||
| pull_request: | ||
| branches: [main] | ||
| push: |
There was a problem hiding this comment.
🔵 minor — The new push-to-main drift gate has no failure notification, unlike the nightly it is meant to backstop
release-nightly.yml:340-402 posts to SLACK_WEBHOOK_URL_ENG_NOTIFS on failure precisely because an unwatched drift detector is no detector. Replay the incident in the header comment with this change in place: upstream re-tags at 15:01, the 16:17 merge turns Docker Build red on main, and the only signal is a red X in main's commit list plus a default Actions email to whoever clicked merge on an unrelated PR — the 19:31 release still fails. Add a slack-notify-failure job modelled on the nightly's, gated on if: failure() && github.event_name == 'push'.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Best point in the review, and I am deliberately not acting on it — it is a team call about alerting, not a defect.
The gap is real: a red main is only a signal if someone looks, and the repo already concedes that by wiring Slack into the nightly.
What gives me pause is that the nightly and this are not the same situation. A 00:35 cron has no owner, which is exactly why it needs a webhook. A push-to-main failure has an obvious one — the person who just merged, who gets GitHub's default failure mail and is watching main anyway. And if: failure() && github.event_name == 'push' fires on any Docker Build failure on main, not just drift, so most of what it sends would be telling someone something they already know. Whether that trade is worth it depends on how the team currently treats a red main and how noisy SLACK_WEBHOOK_URL_ENG_NOTIFS already is, which I should not be guessing at from inside a build-break fix.
Flagging it for a maintainer rather than deciding it here. If the answer is yes, it is a small follow-up.
|
Stopping on bot feedback here, per AGENTS.md's two-round rule — this is round four, and the remaining findings are judgement calls rather than defects. Summary for whoever reviews: Acted on (3 commits):
Declined, with reasoning in-thread:
One open question for a maintainer: the push-to-main build has no failure notification, where the nightly Slacks on failure. A drift gate nobody watches is weak, so the point stands — but Verification: all checks green. Both gate branches exercised on real runs against the post-re-tag base |
| on: | ||
| pull_request: | ||
| branches: [main] | ||
| push: |
There was a problem hiding this comment.
Will there be a large increase in gh actions usage from this? assuming thats fine
There was a problem hiding this comment.
I think its fine. Let me pull out a bit more analysis before merging it.
Fixes the Release run for #3088, and closes the CI gap that let it reach a release.
Two commits: the build fix, then the CI change. They're together because the CI change only makes sense as the answer to "why didn't we catch this" — reviewing them apart loses that.
1. The break
Build Local (amd64)andBuild All-in-One (arm64)both died at the same layer:docker/hyperdx/Dockerfilecopied the entire/usr/libout ofnode:22.22-alpineon top of the ClickHouse base image, overwriting itslibapk.so,libssl,libcryptoandlibz. That only ever worked because both images happened to sit on the same Alpine point release.clickhouse-server:26.8-alpinemoved to Alpine 3.24.2 / apk-tools 3.0.8 whilenode:22.22-alpineis still on 3.24.1 / 3.0.6, so the copy left a 3.0.6libapk.sounderneath a 3.0.8/sbin/apk.apk_fs_is_malicious_filenameis a symbol 3.0.8 added.Reproduced in isolation:
Fix: drop the
/usr/libcopy; installlibstdc++andlibgccwith apk instead. Perldd /usr/local/bin/nodethose are the only two things Node needed from that directory. They go in before the v3.9 mongodb repositories are appended, so they resolve against the image's own Alpine 3.24 (libstdc++-15.2.0-r5, the same versionnode:22.22-alpineships) rather than v3.9.Side benefit: ClickHouse's OpenSSL is no longer silently swapped for node's.
libssl.so.3/libcrypto.so.3are back to being owned bylibssl3-3.5.8-r0/libcrypto3-3.5.8-r0.The blanket copy dates to #821 (May 2025). It has been a latent trap the whole time.
Verification
all-in-one-noauthall-in-one-authInside the amd64 image:
2. Why CI didn't catch it
Worth being precise, because the first guess — "the all-in-one image isn't built on PRs" — is wrong. It is, and it passed.
The break came from outside the repo. Docker Hub re-pointed
clickhouse-server:26.8-alpinefrom ClickHouse 26.8.6 to 26.8.7 mid-afternoon, and apk-tools went 3.0.6 → 3.0.8 with it:Build All-in-One Image@9f10a193@9f10a19326.8-alpine@2bece7c6@2bece7c6Both existing gates were green because both ran before the re-tag, against a digest where node and ClickHouse agreed. The two merges that followed it built no images at all —
Docker Buildonly ran onpull_request, and its path filter would have skipped those diffs regardless. The next thing to touch the new base was the release.Fix: run
Docker Buildon pushes tomainas well, and ignore the path filter there.The filter is right for pull requests, where the question is "did this diff break the image" and staying fast matters. It's the wrong instrument on
main, where the question is "does the image still build today" — and the answer can change with no diff at all. Both all-in-one targets and the end-to-end smoke test run exactly as they already do on PRs, so a drifted base now surfaces as a red build on the next merge instead of in a release hours later. In this incident that would have fired at 16:17, about three hours ahead of the release.Supporting changes:
gatestep per job, since the condition now has two variables rather than one. Pull-request andworkflow_dispatchbehaviour is unchanged — verified against all four event/filter combinations.github.refis identical for every push tomain, so the existingcancel-in-progressgroup would have each merge cancel the one before it and leave that commit unverified. PRs keep cancel-on-new-push.pull: trueon the nightly's three image builds and on the all-in-one PR build, so floating tags are re-resolved against the registry rather than reused from the runner. Fresh runners already re-resolve, so this is hygiene, not the fix; it keeps holding if these ever move to a persistent builder.actionlintis clean ondocker-build.yml;release-nightly.ymlkeeps the same 7 pre-existing self-hosted-runner-label findings it had before this PR, on lines I didn't touch.Not addressed
Base images are still pinned by floating tag.
node:${NODE_VERSION}-alpineandclickhouse-server:${CLICKHOUSE_VERSION}-alpinecan still move underneath a release; the push-to-main build shortens the window to one merge, but doesn't close it. Pinning both by digest — as this repo already does forgooseandalpine:3.23— would make release builds deterministic and turn base upgrades into explicit PRs. That needs a dependabot/renovate config to stay maintainable, which doesn't exist yet, so I've left it out. Happy to follow up.The push-to-main build only covers linux/amd64.
Docker Buildhas always been amd64-only, and this PR doesn't change that, but it's worth naming: the release builds arm64 too, and these are multi-arch tags whose per-arch manifests could in principle be re-pointed independently. In practice upstream republishes the whole index at once — that's what happened here, both arches moved together, and the arm64 All-in-One release job failed with the identical error — so an amd64 gate catches the realistic case. A genuinely arm64-only base move would still only be caught by the nightly. Adding arm64 to the push build would roughly double it; not worth it until we see per-arch drift actually happen.Also left alone:
COPY --from=node_base /usr/local/include(node's C headers, unused at runtime) is dead weight, but removing it isn't needed for the build break.After merge
The Release workflow needs a manual re-run — #3088 already merged, so run 35386380717 won't retry itself.