Skip to content

test(e2e): docker build e2e suite — real-world matrix, scenarios D1–D5 + D9–D10 - #495

Open
AprilNEA wants to merge 22 commits into
masterfrom
test/docker-build-e2e
Open

test(e2e): docker build e2e suite — real-world matrix, scenarios D1–D5 + D9–D10#495
AprilNEA wants to merge 22 commits into
masterfrom
test/docker-build-e2e

Conversation

@AprilNEA

Copy link
Copy Markdown
Member

Adds the docker_build e2e suite planned in internal-docs/plans/docker-build-e2e-matrix.md (included): a test matrix derived from a 25-file corpus of flagship open-source Dockerfiles (moby, buildkit, grafana, airflow, mastodon, authentik, immich, next.js, turborepo, official images…), so the suite covers what production Dockerfiles actually use, weighted by prevalence.

Scenarios (one booted VZ daemon, failures aggregate, quiet-log check)

ID Scenario Covers
D1 large_context 512 MiB incompressible + 100k-file tree through the context upload; .dockerignore; in-build payload-sha + whole-tree-sha byte-exactness
D2 stage_graph 12-stage diamond: FROM scratch script-carrier, 4 parallel branches, COPY --from/--link, heredoc, FROM scratch AS export, external # syntax=docker/dockerfile:1 frontend (pre-loaded); export-artifact byte-exact + full docker export residue sweep
D3 cache_semantics RUN --mount=type=cache,sharing=locked; cold/warm/leaf-change/base-change; per-execution stamps prove exactly which steps re-ran; cache-mount token survives across builds; warm ≤ max(cold/10, 3 s)
D4 session_secret_ssh --secret (readable in RUN → gone next layer → absent from every byte of docker save, gzip-aware sweep) + --ssh (throwaway host agent with a loaded key) — first e2e coverage of the /session upgrade path
D5 bind_mounts RUN --mount=type=bind lockfile shape: derived hash lands, mount leaves no trace
D9 output_streaming ~340 KiB of ordered step output, paced under BuildKit's 200 KiB/s log rate clip, arrives un-clipped; -q reduces to one image ID
D10 exporters --output type=local / type=tar — artifacts streaming host-ward through the session, byte-exact

Also wires docker_build into the xtask prebuild recipe (shared arm with egress_throughput) and adds the missing AGENTS.md table rows.

Bugs this suite caught before merging

  • ABX-494 (P0, first-ever run): guest FEX spun forever on BuildKit's amd64 arch-probe (exec'd in an empty chroot, no /proc), permanently wedging the BuildKit Control API — every buildx docker build hung silently while everything else looked healthy. Fixed in boot-assets#44, shipped as bundle v0.6.10 (chore(assets): bump boot assets to 0.6.10 (FEX procfs fix, ABX-494) #494).
  • Latent harness bug (first green run): run_with_timeout never drained pipes mid-run, so any command with >~64 KiB of output (e.g. --progress=plain builds) deadlocked on the full pipe and read as a bogus timeout. Fixed here with background drain threads + output-tail-on-timeout forensics + two no-VM regression tests — this protects every e2e suite.

Two BuildKit realities are encoded in the assertions (documented in the plan): image IDs are not comparable across builds (created re-stamped even on full cache hits), and step logs are rate-clipped at 200 KiB/s.

Validation

  • cargo test -p arcbox-e2e --lib — 8 passed (incl. the two new harness regression tests).
  • Full suite on boot 0.6.10 (VZ): all 7 scenarios + quiet-log green, 181.8 s wall (large-context build 33.3 s, cache cold→warm 14.6 s→1.75 s). network_workload W14 green on the same bundle.
  • Requires bundle ≥ 0.6.10 (merged in chore(assets): bump boot assets to 0.6.10 (FEX procfs fix, ABX-494) #494): on older bundles the suite is red by design (ABX-494).

Refs: ABX-494, boot-assets#44, #494.

Phase 3 follow-ups (not in this PR): D6 --platform linux/amd64 FEX both-ways assertions, D7 concurrent builds, D8 mid-build cancellation, Tier X pinned real-project builds.

AprilNEA added 12 commits July 22, 2026 17:20
…ate clip

Pre-validation against a fixed-FEX guest caught both: RUN lines persist
into image-config history, so inlining the secret value planted the very
leak the save-sweep hunts; and BuildKit clips step logs at 200KiB/s, so a
bulk emitter is clipped by design — the paced emitter keeps the no-clip
assertion honest. Also un-wedge the sweep script's set -e/grep interaction.
BuildKit re-stamps created per build, so identical-image-ID is not a
valid full-cache-hit assertion — the per-execution stamps are; and a
relative --secret src resolves against the CLI CWD, not the context.
…ail on timeout

The old loop only read the pipes after exit, so any command with more
than ~64 KiB of output (a --progress=plain build, a chatty pull) blocked
on the full pipe and read as a bogus timeout — first exposed by the D9
streaming scenario, which emits ~350 KiB of plain progress.
@linear-code

linear-code Bot commented Jul 22, 2026

Copy link
Copy Markdown

ABX-494

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — one rough edge in the timeout path worth a look.

Reviewed changes — a test-only PR adding the docker_build e2e suite (scenarios D1–D5, D9–D10) plus a harness pipe-draining fix that protects every e2e suite.

  • Add docker_build suitetests/e2e/tests/docker_build.rs runs 7 build scenarios through one booted VZ daemon, aggregating failures and asserting a quiet proxy-error log; integrity is checked byte-exact inside the build (context sha, cache stamps, secret-absence sweep, exporter round-trips).
  • Drain command pipes in run_with_timeouttests/e2e/src/docker.rs now drains stdout/stderr on background threads (an undrained pipe filled at ~64 KiB and turned any chatty command into a bogus timeout) and carries an output tail on genuine timeouts; two no-VM regression tests added.
  • Plan docinternal-docs/plans/docker-build-e2e-matrix.md records the 25-Dockerfile corpus, feature census, and derived matrix.
  • xtask prebuild wiringdocker_build shares the egress_throughput recipe (cargo build --release -p arcbox-daemon); xtask/AGENTS.md table rows updated to match. Verified against scenario.rs's self-build — the lockstep invariant holds.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread tests/e2e/src/docker.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c80c639303

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread xtask/src/commands/e2e.rs Outdated
Comment thread tests/e2e/src/docker.rs Outdated
Comment thread tests/e2e/tests/docker_build.rs Outdated
@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

Adds comprehensive Docker build end-to-end coverage and hardens command timeout handling.

  • Adds Docker build scenarios D1–D10 covering context transfer, stage graphs, caching, BuildKit sessions, mounts, cross-platform execution, concurrency, cancellation, output streaming, and exporters.
  • Adds opt-in external project build scenarios.
  • Updates run_with_timeout to drain output concurrently and terminate descendant process groups on both timeout and normal child exit.
  • Wires the suite into xtask prebuild automation and documents the test matrix.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the timeout path terminates the child process group before joining drain threads, and the completed-child path now performs the same cleanup before collecting output.

Important Files Changed

Filename Overview
tests/e2e/src/docker.rs Adds concurrent pipe draining, process-group cleanup on both exit paths, timeout output diagnostics, and regression tests for descendant-held pipes.
tests/e2e/tests/docker_build.rs Adds the shared-daemon Docker build matrix covering scenarios D1–D10 with aggregated failures and quiet-log validation.
tests/e2e/tests/docker_build_external.rs Adds environment-gated external Docker build scenarios using pinned real-world projects.
xtask/src/commands/e2e.rs Integrates the Docker build suite into the e2e prebuild workflow.
internal-docs/plans/docker-build-e2e-matrix.md Documents the real-world Dockerfile corpus, derived scenario matrix, baseline measurements, and test methodology.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[xtask e2e docker_build] --> B[Boot shared VZ daemon]
    B --> C[Preload base and frontend images]
    C --> D[Run D1-D10 scenarios]
    D --> E[Docker and BuildKit requests]
    E --> F[run_with_timeout]
    F --> G[Drain stdout and stderr concurrently]
    F --> H{Direct child exits before deadline?}
    H -->|Yes| I[Kill remaining process group]
    H -->|No| J[Kill process group at timeout]
    I --> K[Join drain threads and return output]
    J --> L[Join drain threads and return timeout diagnostics]
    D --> M[Aggregate scenario failures]
    M --> N[Check daemon logs for proxy errors]
Loading

Reviews (10): Last reviewed commit: "fix(xtask): guard every backend-pinned e..." | Re-trigger Greptile

Comment thread tests/e2e/src/docker.rs Outdated
AprilNEA added 3 commits July 22, 2026 17:40
Upstream pins no packageManager, so corepack pulls latest pnpm, whose
supply-chain default refuses sharp's postinstall (ERR_PNPM_IGNORED_BUILDS)
on any engine. Apply pnpm's documented escape hatch; bail loudly if a pin
bump ever makes the accommodation stale.
@AprilNEA

Copy link
Copy Markdown
Member Author

Phase 3 + Tier X pushed (4 commits):

Tier D is now the complete D1–D10 matrix — 10/10 scenarios green in one run (403 s wall, VZ, boot 0.6.10):

  • D6 cross_platform: native build bakes linux/arm64:arm64 from BUILDPLATFORM/TARGETARCH; --platform linux/amd64 build asserts uname -m = x86_64 in-build (the exact surface ABX-494 wedged) and the image records amd64. The fail-closed unprovisioned-FEX direction still awaits an e2e knob (FEX ships in every bundle ≥ 0.6.6).
  • D7 concurrent_builds: 4 distinct + 2 same-context builds in flight at once; unique markers prove no cross-talk (28 s).
  • D8 cancellation: client SIGKILLed mid-RUN and mid-256 MiB-upload; guest reaps the cancelled RUN's process (polled via --pid=host), follow-up build succeeds, suite quiet-log stays clean (17 s).

Tier X (docker_build_external.rs, ARCBOX_E2E_EXTERNAL=1, pinned upstream commits) — 3/3 green with real network:

  • X1 postgres 17/bookworm (apt + wget + gpg keyserver) — 85 s, smoke postgres --version.
  • X2 next.js examples/with-docker (corepack + pnpm frozen install + next build) — one documented accommodation: upstream pins no packageManager, so corepack pulls latest pnpm whose supply-chain default refuses sharp's postinstall (ERR_PNPM_IGNORED_BUILDS) on any engine; the scenario applies pnpm's documented escape hatch and bails loudly if a pin bump makes it stale.
  • X3 caddy 2.11/builder (apk + checksum-verified xcaddy fetch) — smoke xcaddy version.

Both suites' full runs: Tier D test result: ok … 403.06s, Tier X test result: ok (X2 rerun after the accommodation: 128 s).

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No new issues in this delta — the D6–D8 and Tier-X additions are clean.

Reviewed changes — the incremental delta since the last pullfrog review: three new scenarios added to the docker_build suite plus a new manual external-build suite.

  • Add D6 cross_platform — a native --platform=$BUILDPLATFORM / TARGETARCH expansion build (asserts linux/arm64:arm64) and a --platform linux/amd64 build whose RUN executes through the binfmt translator (uname -m in-build + image-recorded amd64); pulls the amd64 base with the same 3-attempt retry discipline as ensure_image.
  • Add D7 concurrent_builds — four distinct contexts plus two racing on one shared context, all in flight via thread::scope (the #[expect(clippy::needless_collect)] forces every spawn before the first join); unique per-image markers prove no cross-talk.
  • Add D8 cancellation — SIGKILLs the client mid-RUN and mid-256 MiB-context-upload, polls the guest via --pid=host to confirm the cancelled RUN is reaped, and asserts a follow-up build succeeds; the new spawn_build helper mirrors the existing DOCKER_HOST/null-stdio pattern.
  • Add Tier X external suitetests/e2e/tests/docker_build_external.rs, gated behind ARCBOX_E2E_EXTERNAL=1, sparse-fetches pinned upstream Dockerfiles (postgres, next.js, caddy) at fixed SHAs and builds them through the datapath; the next.js scenario patches pnpm install --frozen-lockfile with --dangerously-allow-all-builds and bails loudly if the line moves under a pin bump.
  • Wire docker_build_external into xtask prebuild — shared cargo build --release -p arcbox-daemon arm with egress_throughput/docker_build; xtask/AGENTS.md table row added to match, and the plan doc updated to record D1–D10 + Tier X status.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

AprilNEA added 2 commits July 23, 2026 05:49
…backend flag

run_with_timeout killed only the direct child, so a descendant that
inherited the pipes kept the write end open and the drain-thread joins
blocked past the deadline — 30s for the shell repro, unbounded when the
survivor is a wedged docker-buildx, which is the shape docker build takes.
Spawn into a fresh process group and killpg it. The regression test now
bounds elapsed time and uses `sleep 30 & wait`: with a plain `sleep 30`
the shell execs it, leaving no grandchild, and the bug hides (verified
A/B — 30.06s before, 1.05s after).

D4's ssh probe asserted `test -S $SSH_AUTH_SOCK`, which only stats the
socket inode BuildKit created and passes even when no agent traffic
crosses. Query the agent instead and require the throwaway key's own
fingerprint back.

xtask e2e now rejects --backend hv/both for targets built on
scenario::run_vz_scenario*, which hardcode vz: the run was VZ regardless
while the label, metrics, and archive said HV, silently corrupting any
oracle comparison read from the artifacts.
@AprilNEA
AprilNEA force-pushed the test/docker-build-e2e branch from 5068efc to 51200f4 Compare July 31, 2026 22:12
pullfrog[bot]
pullfrog Bot previously approved these changes Jul 31, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found — the timeout-path finding is genuinely retired, and the two other hardening changes are sound.

Reviewed changes — the incremental delta since the prior pullfrog review: one commit (51200f40) that fixes the run_with_timeout timeout path, turns D4's ssh check into a real round-trip, and makes the xtask backend flag fail loudly on VZ-pinned targets.

  • Killed the whole process group on timeoutrun_with_timeout now spawns with process_group(0) and the timeout branch killpgs the group before joining the drain threads, so a surviving docker-buildx grandchild can no longer hold the inherited pipe write ends open past the deadline. pgid == child.id() holds by construction, the child is still unreaped when the signal lands, and the killpg failure path falls back to child.kill().
  • Made the timeout regression test able to failtimeout_returns_at_deadline_despite_surviving_descendant uses sleep 30 & wait (a plain sleep 30 is exec'd by the shell, leaving no grandchild and hiding the bug) and asserts elapsed < 10s alongside the original tail assertion, so a regression reads as ~30s rather than a silent pass. It sits in the crate's --lib tests, the cheapest rung of the validation ladder.
  • Hardened D4's ssh assertion into a round-tripThrowawaySshAgent records the loaded key's SHA256: fingerprint via ssh-keygen -lf, and the in-build step runs ssh-add -l and greps for that exact token; test -S only stats the socket inode BuildKit created, which passes even when no agent traffic survives the crossing. The fingerprint's base64 alphabet carries no BRE metacharacters, the set -e pipeline still fails closed through grep -q, and the new apk/network dependency is called out in-code.
  • Made --backend fail loudly on VZ-pinned targetsVZ_PINNED_TESTS in xtask/src/commands/e2e.rs bails when a non-vz backend is requested for a target whose harness hardcodes ARCBOX_VM_BACKEND=vz, rather than archiving a VZ run under an HV label and corrupting a later HV↔VZ comparison. The list matches exactly the six test targets that call run_vz_scenario*.
  • Recorded both invariants in the agent docsxtask/AGENTS.md gains the pinned-target section plus a fourth extending-checklist item, and the plan doc's D4 row now documents why the socket-stat check was insufficient.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Comment thread tests/e2e/src/docker.rs

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — the merge itself is clean, but it silently opened a gap in the guard this PR just added.

Reviewed changes — the incremental delta since the prior pullfrog review (51200f40, approved) is a single merge commit pulling origin/master into the branch. Reviewed as a merge-integration pass.

  • Merged origin/master (68c3afdd) into the branchgit diff 51200f40..43c7d0c1 restricted to the PR's six paths is empty, so no conflict resolution touched the docker_build suite, the run_with_timeout harness fix, or the xtask wiring; that content is byte-identical to the approved tree.
  • Confirmed the SKIP_BUILD lockstep still holdsscenario.rs still self-builds exactly cargo build --release -p arcbox-daemon and still hardcodes ARCBOX_VM_BACKEND=vz + RunMetrics::new(name, Some("vz")), so the shared egress_throughput | docker_build | docker_build_external prebuild arm still matches character-for-character.
  • Confirmed no helper-signature driftrun_vz_scenario_with_log, RunMetrics, DaemonHandle, ensure_image, docker_output, docker_ignore are unchanged by master, and master's new run_with_timeout callers (bench_virtiofs, network_iperf, the nfs_* probes, the rewritten idle_balloon) depend on none of the old blocking-pipe behavior.
  • Picked up boot bundle 0.6.13 and a Docker toolchain bumpassets.lock moved past the suite's declared >= 0.6.10 floor via 098771c0, alongside a44178b0 (host docker/buildx) and bde8fb01 (guest runtime exec'd from a read-only block image).
  • Inherited two new VZ-pinned e2e targets — master added network_iperf and bench_virtiofs, both routed through scenario::run_vz_scenario_with_log; see the inline note.

ℹ️ The suite's recorded green run predates the bundle and toolchain this merge brings in

The plan doc pins the full-green result to boot 0.6.10 / dockerd 29.6.1 / BuildKit v0.31.1, and the PR body's validation section says the same. The merge moves the tree to boot 0.6.13 and a newer host docker/buildx, and changes how the guest runtime is exec'd — which are exactly the moving parts the suite's encoded BuildKit realities are calibrated against (the 200 KiB/s log rate clip D9 paces under, image-ID non-comparability, RUN --mount=type=cache semantics). This is a re-validation ask, not evidence of breakage.

Technical details
# `docker_build` suite validated on boot 0.6.10; branch now merges boot 0.6.13 + newer buildx

## Affected sites
- `internal-docs/plans/docker-build-e2e-matrix.md:122` — baseline header records `boot 0.6.10, dockerd 29.6.1 / BuildKit v0.31.1`.
- `internal-docs/plans/docker-build-e2e-matrix.md:204` — "fully green against boot bundle >= 0.6.10".
- `assets.lock:17``version = "0.6.13"` at the merged head.
- Merged master commits that move the relevant surface: `098771c0` (boot assets -> 0.6.13),
  `a44178b0` (Docker toolchain bump), `bde8fb01` (container runtime exec'd from a read-only
  block image).

## Required outcome
- A recorded green run of `cargo xtask e2e --test docker_build --backend vz` on the merged
  tree, so the D3 cache-semantics and D9 rate-clip assertions are known-calibrated against the
  BuildKit version that actually ships now.
- If anything shifted, update the baseline header and the encoded-realities notes in the plan
  doc in the same change rather than loosening an assertion.

## Open questions for the human
- Was the suite re-run after the merge, or is the 0.6.10 result still the latest evidence?

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread xtask/src/commands/e2e.rs Outdated
…VZ pin

The success branch joined the drain threads without touching the process
group, so a command that exits promptly while leaving a descendant on the
inherited pipes blocked the join with no deadline left to enforce —
`sh -c 'echo done; sleep 30 & exit 0'` took 30.07s. Both exit paths now
kill the group before joining (verified A/B: 30.07s -> 1.04s).

kill_process_group takes the pgid captured at spawn rather than a live
Child, since the success path has already reaped the leader. That is
sound: POSIX forbids reusing a pid while it is still the group id of an
existing group, so the signal reaches that group or nothing (ESRCH).

The VZ pin is now read from the target's own source instead of a
hardcoded list. The list was already stale at this branch's merged head —
network_iperf and bench_virtiofs both use run_vz_scenario_with_log and
were missing — which is the failure mode a list has: it goes stale
exactly when a new target needs the guard.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — the derived guard closes the gap it was aimed at, but not the whole class.

Reviewed changes — the incremental delta since the prior pullfrog review (43c7d0c1) is a single commit, 580fb4c2, which responds to both inline notes from that pass.

  • run_with_timeout now kills the process group on the success path too (tests/e2e/src/docker.rs) — kill_process_group changed from &mut Child to a plain pgid: i32 captured before any reap, and the try_wait branch signals the group before joining the drain threads. Verified the ordering is sound: bytes already buffered in the pipe survive the writer's death, so no output is lost, and POSIX forbids recycling a pid while it is still a live group's id, so a post-reap killpg reaches that group or nothing (ESRCH). Surveyed every run_with_timeout caller (docker_output, docker_build_external.rs:117 git, network_iperf.rs:423 iperf3, bench_virtiofs.rs:158) — none expects a descendant to outlive the call, so the unconditional group kill is safe.
  • The new regression test is real coverage, not a tautologysuccess_returns_promptly_despite_surviving_descendant fails at ~30s without the fix, because non-interactive sh has no job control and leaves the & job in the shell's own process group. The sleep 30 & exit 0 shape (vs. the timeout test's & wait) is the right distinction.
  • VZ_PINNED_TESTS replaced by is_vz_pinned, derived from the target's source (xtask/src/commands/e2e.rs) — this retires the prior thread: all eight run_vz_scenario targets are single top-level files under tests/e2e/tests/ with no [[test]] path overrides in tests/e2e/Cargo.toml, so the derived predicate is a strict superset of the deleted six-entry list and picks up network_iperf/bench_virtiofs for free. No stale references to the constant remain anywhere in the tree.
  • xtask/AGENTS.md updated in the same change — the lockstep paragraph and checklist item 4 now describe the derived guard rather than the list. See the inline note: the new prose claims more completeness than the predicate delivers.

ℹ️ Not re-raised: the plan doc's green run still predates the merged toolchain

The prior pass asked for a recorded green run of docker_build on the merged tree (boot 0.6.13 + newer host docker/buildx) since the baseline header pins 0.6.10 / BuildKit v0.31.1. That is unchanged by this commit and out of scope for an incremental pass — flagging only so it isn't assumed retired along with the thread above.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread xtask/src/commands/e2e.rs Outdated
…ones

The previous derivation only recognised scenario::run_vz_scenario, so five
targets that hardcode ARCBOX_VM_BACKEND in their own source stayed
unguarded — idle_balloon, machine, stats_watch, nfs_restart_probe pin vz,
and virtio_debug and hv_reboot pin hv.

The hv-pinned pair is why broadening cannot be a simple 'is it pinned to
vz' check: --backend vz on virtio_debug mislabels exactly as badly as
--backend hv on a vz-pinned target. pinned_backend now reports WHICH
backend a target pins, and the runner errors only when the request
conflicts with it.

Derived from the sources, not a list: scan the target for a line carrying
both ARCBOX_VM_BACKEND and a "vz"/"hv" literal, then the arcbox_e2e
modules it imports — one level of indirection, where run_vz_scenario and
the sandbox harness keep theirs. Lines that merely read the variable carry
no literal, so boot_assets and backend_matrix stay correctly unpinned.

Verified against all 22 targets in the tree; unit tests pin the three pin
shapes actually used, the read-the-env non-pin, and the import scan.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — the derived guard is now correct in both directions, but making it bidirectional turned the default invocation into a hard error for the two hv-pinned targets.

Reviewed changes — the incremental delta since the prior pullfrog review (580fb4c2) is a single commit, 92b1cde9, touching only xtask/src/commands/e2e.rs and xtask/AGENTS.md. It responds to the one inline note from that pass.

  • is_vz_pinnedpinned_backend, reporting which backend a target pins — this retires the prior thread. I ran the predicate's logic against every target under tests/e2e/tests/ and the classification in the doc comment matches the tree exactly: vz pins found in machine.rs:86, idle_balloon.rs:86, stats_watch.rs:82, nfs_restart_probe.rs:82 (the .into() shape), hv pins in virtio_debug.rs:42 and hv_reboot.rs:68, and the one level of indirection resolves for the run_vz_scenario targets (src/scenario.rs:82) and sandbox (src/sandbox.rs:126).
  • No false positives on the two targets that genuinely honor --backend — this was the failure mode that ruled out the naive "match ARCBOX_VM_BACKEND" broadening, so I checked it directly rather than trusting the shape. In boot_assets.rs neither the read (:38, env::var("ARCBOX_VM_BACKEND")) nor the forward (:527, backend.as_str().to_owned()) puts a "vz"/"hv" literal on the line, so literal_backend_pin correctly declines both; backend_matrix reaches the same code. The "carries a literal" rule is doing real discriminating work, not coincidentally passing.
  • cargo test -p xtask is green (7 passed, 0 failed), including the three new commands::e2e::tests::*. The let-chain in pinned_backend is fine at the workspace's edition 2024 / rust-version = "1.96".
  • Blast radius is developer-ergonomics only — nothing under .github/ invokes cargo xtask e2e, so a wrong bail cannot break CI.

ℹ️ Nitpicks

  • hv_vmm is structurally HV-only (it drives the HV backend directly, no daemon) but contains no ARCBOX_VM_BACKEND line at all, so --backend vz --test hv_vmm — which is what the bare command does today — still archives an HV run under a vz label. The new AGENTS.md wording is honest about this (the guard only reaches targets that "pin the way every existing one does"), so this is noted as the one live instance rather than a defect in the derivation.
  • Carrying forward, unchanged by this commit and not re-raised: the plan doc's recorded full-green run is still at boot 0.6.10 / BuildKit v0.31.1, while the merged head is on boot 0.6.13 with newer host docker/buildx.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread xtask/src/commands/e2e.rs
E2eBackend::Hv => &[Some("hv")],
E2eBackend::Both => &[Some("vz"), Some("hv")],
};
if let Some(pinned) = pinned_backend(&root, &args.test)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--backend carries default_value = "vz" (xtask/src/main.rs:32), so by the time this guard runs, a user who typed nothing is indistinguishable from one who typed --backend vz. Now that the guard is bidirectional, that turns a defaulted value into a hard error:

cargo xtask e2e --test virtio_debug    # bailed: pins hv
cargo xtask e2e --test hv_reboot       # bailed: pins hv

Both ran before this commit (the old is_vz_pinned guard only fired on --backend hv/both), and the run they produced was correctly labelled — the target pins hv, the archive says hv. The error text tells the user to "re-run with --backend hv", which is right, but it fires on the invocation the docs recommend.

xtask/AGENTS.md is untouched in exactly the two places that now mislead: :104 still names --test virtio_debug as a rung of the HV validation ladder, and :109-110 still documents Defaults: --test boot_assets --backend vz --repeat 1 with no note that the default is now fatal for an hv-pinned target. virt/AGENTS.md, app/AGENTS.md and tests/e2e/AGENTS.md all point at virtio_debug as a first-move diagnostic too, though they invoke it via cargo test rather than xtask.

Two ways out, both small:

  • Make the flag backend: Option<E2eBackend> and map None to &[None], skipping the guard — an unspecified backend sets no env, so the daemon's own default applies and nothing is mislabelled. The machinery is already there: backend_label = backend.unwrap_or("default") at :150 handles the label, and BootAssetsConfig already models backend as Option. This is the only option that leaves cargo xtask e2e --test virtio_debug working.
  • Or keep the flag as-is and narrow the AGENTS.md ladder wording, so the next person reading :104 knows an hv-pinned target needs an explicit --backend hv.

Either is fine; leaving both as they are is the part worth avoiding, since the bail is the first thing a developer hits following the repo's own debugging instructions.

Comment thread xtask/src/commands/e2e.rs
}

#[cfg(test)]
mod tests {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These three assert against string literals written here, so they pin the shapes the author transcribed rather than the shapes in the tree — and literal_backend_pin scans line by line, which makes that gap load-bearing. If a future rustfmt run or a refactor splits a pin across lines, e.g.

env: vec![(
    "ARCBOX_VM_BACKEND".to_owned(),
    "vz".to_owned(),
)],

no line carries both the variable and the literal, pinned_backend returns None, the guard silently stops applying to that target — and all three tests stay green. That is the same fail-open-on-drift failure the switch away from VZ_PINNED_TESTS was meant to close, just relocated from a hardcoded list to a hardcoded line shape. It would also quietly falsify the new AGENTS.md claim that "the unit tests in that file pin the shapes."

One assertion against the real tree closes it, and run() already has the root helper:

#[test]
fn pins_in_the_tree_are_still_detected() {
    let root = repo::root_from_xtask_manifest(env!("CARGO_MANIFEST_DIR")).unwrap();
    assert_eq!(pinned_backend(&root, "idle_balloon").unwrap().as_deref(), Some("vz"));
    assert_eq!(pinned_backend(&root, "docker_build").unwrap().as_deref(), Some("vz"));
    assert_eq!(pinned_backend(&root, "virtio_debug").unwrap().as_deref(), Some("hv"));
    assert_eq!(pinned_backend(&root, "boot_assets").unwrap(), None);
}

Four targets covering all three groups: a direct vz pin, the indirect pin through scenario.rs, an hv pin, and the env-honouring non-pin. It fails the moment a real pin stops being recognized, which the literal-only tests cannot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant