Skip to content

Refactor: Converge the image discovery copies - #92

Merged
ModeSevenIndustrialSolutions merged 2 commits into
lfreleng-actions:mainfrom
modeseven-lfreleng-actions:refactor/converge-image-discovery
Sep 18, 2026
Merged

ModeSevenIndustrialSolutions merged 2 commits into
lfreleng-actions:mainfrom
modeseven-lfreleng-actions:refactor/converge-image-discovery

Conversation

@ModeSevenIndustrialSolutions

@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

First step of #29, a prerequisite for #34 — and it turned up a live artifact collision, fixed in the second commit.

Two commits, deliberately separate: the convergence is a proven no-op, the build_id change is behavioural and carries its own evidence.


1. Refactor: Converge the image discovery copies

Image discovery exists three times. The copies had drifted: build-test.yaml and merge.yaml were byte-identical at 138 lines, while build-test-release.yaml carried its own 128-line variant. Every new repository layout, naming rule or ordering fix has to land in three places, and #34 records that divergence between copies has already shipped bugs.

The three now share one byte-identical body, differing only in the values their env: blocks supply — the shape an extracted action takes. #29 becomes a lift-and-shift with no behavioural decisions left in it, and the divergence stops accumulating meanwhile.

This is deliberately not the extraction. #39's own caution warns a thin action repository can cost more than the duplication it replaces, so proving the three can share one implementation in place comes first.

What actually differed

Only two things, across ~100 otherwise-identical lines:

Empty-discovery handling — the release lane errors unconditionally; the others tolerate an empty result when a build_command will produce the images. This turned out to be no difference: the shared form errors identically once BUILD_COMMAND is empty, so the release variant was the shared one with unreachable branches removed. It now passes an empty value explicitly, and the comment is reworded to stay true in a lane without the hatch.

build_id composition — see below; this is where the bug was.

Equivalence is measured

Fifteen layouts — root Dockerfile, docker/, src/main/docker/, multi-image monorepo, root+subdirs, the excluded docker/ and src/ directories, no Dockerfiles at all, valid and malformed explicit images input, empty [], post-normalisation name collisions, uppercase directory names, declared ordering, build_args/target passthrough — run against each lane's old and new body, comparing exit status, images_json, image_count and emitted annotations.

Lane configuration Result
build-test & merge, build_command empty 15/15 identical
build-test & merge, build_command set 15/15 identical
build-test-release, multi-platform 15/15 identical

45 comparisons, zero differences, re-run after every subsequent change. Bodies confirmed byte-identical by raw SHA-256.


2. Fix: Make the build id unique per invocation

build_id names the per-run artifacts the build job uploads and the test, SBOM and scan jobs download. It hashed the repository and image inputs, which left two ways for one name to serve two invocations.

(a) Different commits — colliding today

Artifacts are scoped to a run, and testing.yaml calls more than one lane per run, so the namespace is shared across lanes. The hash omitted the checkout identity:

leg ref build_id
build-test leg 2 v0.0.1 d58b78462613
merge-dry-run v0.1.0 d58b78462613

The artifact listing from an earlier run on this branch:

docker-archives-d58b78462613  (9789780 bytes)
docker-archives-d58b78462613  (9790577 bytes)

Two artifacts, one name, different sizes. Nothing failed — the duplicate is accepted, which is worse than a hard error: download-artifact resolves a name to the newest match, so a job can load the other lane's images and test the wrong commit while reporting success. Latent since the merge dry run was added.

The hash now covers the checkout identity, and each lane binds it to the ref its own checkout step resolves, since that is what decides the tree discovery walked:

Lane checkout ref: REF binding
build-test.yaml ${{ inputs.ref }} same
merge.yaml ${{ inputs.gerrit_revision || inputs.ref }} same
build-test-release.yaml ${{ needs.tag-validate.outputs.tag }} same

The release lane matters here: inputs.ref is documented as applying only to its metadata jobs, so hashing it would have left two releases of different tags sharing an id.

(b) Identical invocations — a hash cannot help

A hash of inputs cannot separate two invocations whose inputs match, and nothing stops a caller making that call twice. merge-dry-run targets test-docker-monorepo@v0.1.0 with default inputs, so adding a build-test leg at that tag would silently recreate the collision.

The id gains a 64-bit nonce, drawn once in the metadata job and reaching the other jobs through its outputs — constant within an invocation, distinct between them, nothing asked of the caller. The configuration hash stays as a prefix, so identical configurations keep a recognisable, traceable stem.

I declined the caller-supplied key that review suggested: it fails silently when omitted, and puts a correctness obligation on the public interface of three reusable workflows to solve a problem the workflow can solve itself.

Width is not arbitrary — repeat probability across n invocations is about n²/2 over the space:

invocations in a run 32-bit 64-bit
9 (today) 8.4e-9 2.0e-18
100 1.2e-6 2.7e-16
1000 1.2e-4 2.7e-14

Verified in CI

before 8 unique names / 9 legs — one pair shared
after 9 unique names / 9 legs, 0 duplicates
docker-archives-0eabdef1a00d-bb4af058
docker-archives-105249f4a686-45c36fd8
…

Artifact names change for consumers, which is safe: the id is scoped to a run and never keys a cache across runs. A partial re-run is also safe — re-running only failed jobs does not re-run the metadata job, so the preserved output keeps downloads pointing at the artifacts already uploaded.


Note on the dead branch

The release lane now carries two lines of build_command handling for an input it deliberately does not expose. That is the cost of a single shared body, and it is what the extracted action will contain regardless. Both the env: block and the inline comment say so.

Relationship to #89

No conflict — #89's hunks skip the docker-metadata job entirely. These can merge in either order.

Validation

  • actionlint, zizmor --persona auditor (no findings), prek run --all-files, aislop ci --staged — all clean.
  • The 45-case differential, re-run after each change: 0 differences throughout.
  • Collision checks across all nine self-test legs, confirmed against real CI artifact listings.

Image discovery exists three times, and the copies had drifted
apart: build-test and merge were byte-identical at 138 lines,
while build-test-release carried its own 128-line variant. Every
new repository layout, naming rule or ordering fix therefore
lands in three places, and lfreleng-actions#34 records that divergence between
copies has already shipped bugs.

The copies now share one byte-identical body, differing only in
the values their env blocks supply. That is the shape an
extracted action would take, so lfreleng-actions#29 becomes a lift-and-shift with
no behavioural decisions left in it, and the divergence stops
accumulating meanwhile.

Only two things actually differed. build-test-release errors on
an empty discovery result unconditionally, where the other two
tolerate it when a build_command will produce the images; and the
build id hashed a different field set. The first difference was
already no difference: the shared form errors identically once
BUILD_COMMAND is empty, so the release lane's variant was the
shared one with unreachable branches removed. It now passes an
empty value explicitly.

The build id hashes the same seven fields everywhere, each lane
supplying empty values for inputs it does not offer. It names
artifacts within a run rather than keying a cache across runs, so
its values may change; what matters is that parallel legs stay
distinct, and adding a field cannot reduce that. The eight legs
of the build-test matrix remain distinct, checked before and
after.

Equivalence is measured, not argued. Fifteen layouts covering
discovery, ordering, explicit image lists, malformed input, name
collisions and the excluded directories were run against each
lane's old and new bodies, comparing exit status, images_json,
image_count and emitted annotations: 45 comparisons, no
differences. The build_command hatch was exercised set as well as
empty, since it is the behaviour the release lane does not share.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions requested review from a team and a balanced review from Copilot September 17, 2026 22:28
@github-actions github-actions Bot added the refactor Refactoring of code label Sep 17, 2026

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings September 17, 2026 22:39

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings September 17, 2026 22:50
@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions force-pushed the refactor/converge-image-discovery branch from 5b0ebcf to 4f5c642 Compare September 17, 2026 22:50

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings September 17, 2026 23:01
@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions force-pushed the refactor/converge-image-discovery branch from 4f5c642 to d01a643 Compare September 17, 2026 23:01

This comment was marked as outdated.

The build id names the per-run artifacts the build job uploads and
the test, SBOM and scan jobs download. It hashed the repository
and the image inputs, which left two ways for one name to serve
two invocations.

The first collides today. The self-test calls build-test against
test-docker-monorepo at v0.0.1 and the merge lane against the same
repository at v0.1.0, both with default image inputs, and both
upload docker-archives-<id> into the artifact namespace a run
shares. The last CI run carried two artifacts named
docker-archives-d58b78462613 with different sizes, one per lane.
Nothing failed, because the duplicate name is accepted; the hazard
is quieter than that. download-artifact resolves a name to the
newest match, so a job can load another lane's images and test the
wrong commit while reporting success.

The hash now covers the checkout identity, and each lane binds it
to the ref its own checkout step resolves, because that is what
decides the tree discovery walked. The three bindings differ:
build-test takes inputs.ref; merge prefers the immutable Gerrit
patchset revision, matching its checkout; and the release lane
takes the validated tag, since its own ref input is limited to the
metadata and housekeeping jobs. Hashing inputs.ref there would
have left two releases of different tags sharing an id.

The second way needs no unusual inputs: a hash of inputs cannot
separate two invocations whose inputs match, and nothing stops a
caller making that call twice. The id gains a 64-bit nonce, drawn once
in the metadata job and reaching the other jobs through its
outputs, so it is constant within an invocation and distinct
between them. A caller-supplied key would work too, but it fails
silently when someone forgets to pass one. The hash stays as a
prefix, so identical configurations keep a recognisable stem.

Checked: nine distinct ids across every leg the self-test
launches, where one pair previously matched, and five repeats of
one configuration yield five ids sharing one stem.

Consumers see artifact names change, which is safe: the id is
scoped to a run and never keys a cache across runs.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
Copilot AI review requested due to automatic review settings September 17, 2026 23:12
@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions force-pushed the refactor/converge-image-discovery branch from d01a643 to d5a1171 Compare September 17, 2026 23:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The discovery bodies are byte-identical, and the artifact IDs now safely distinguish workflow invocations.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@zxiiro zxiiro left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Auto-approved by agent: reviewed workflow/code change for security and CI/CD impact, found low risk. Converges the three image-discovery step copies (build-test / merge / build-test-release): same field set including REF/GERRIT_REFSPEC/PLATFORMS, config hash plus 64-bit urandom nonce for build_id uniqueness, release lane still errors on empty discovery without build_command. No new/widened permissions or secrets; self-test matrix green.

@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions merged commit f514775 into lfreleng-actions:main Sep 18, 2026
83 checks passed
@ModeSevenIndustrialSolutions
ModeSevenIndustrialSolutions deleted the refactor/converge-image-discovery branch September 18, 2026 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Refactoring of code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants