Repository navigation
refactor: official golden only; custom images as-is; delete the curated bake - #427
Conversation
…mages
The curated stock-Ubuntu bake is deleted. Two golden sources remain: the
published packed official golden (ubuntu-latest/ubuntu-24.04 and the
unconfigured default; a download or verify failure is fatal), and a
configured image, baked as-is plus the GitHub-runner contract.
Deleted: ToolchainLayer, base_install_script and the apt/pin baseline,
the apt-index marker and refresh, environment goldens
(prepare_golden_for_env's fan-out, GoldenRegistry's packed fallback, the
env-golden retry loop), the local stock bake, the direct-create fallback,
the per-VM ownership walk and reconcile script, RUSTUP_HOME/CARGO_HOME and
the /usr/local/{cargo,go}/bin PATH entries.
Added: the lean golden contract (glibc check, runner account with _work and
passwordless sudo, one owner-only ownership walk at build time, writable
toolcache + /etc/environment, /etc/preloop-bake.json), official-only label
mapping, and `preloop build-golden --base-image` resolving to the configured
image.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (28)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 11 potential issues.
🐛 7 issues in files not directly in the diff
🐛 Default AgentENV pool cannot start
With AgentENV and no configured image, bake_golden_in_guest boots the official sentinel as an OCI image. AgentENV requires a real image reference, so the pool never starts.
🐛 Existing runner account retains the wrong UID
If a custom image already has runner at another UID, runner_account_script skips creation but changes its home to UID 1001. Job processes then use the mismatched account and can lose access to their home.
🐛 Official toolchains vanish from job PATH
On the official runner image, guest_runner_path removes the existing Cargo and Go binary directories. Steps invoking those preinstalled tools without setup actions now fail command lookup.
🐛 Non-Ubuntu Mac goldens fail at apt
On Apple Silicon, bake_golden_in_guest runs the Ubuntu Rosetta installer for every custom image. A glibc image without Ubuntu apt sources fails before the runner contract runs.
⚠️ Shutdown waits for golden preparation
When shutdown arrives during prepare_fork_base, RunnerPool::run cannot observe it until the full download or bake finishes. An interrupted golden preparation can delay engine shutdown for the entire transfer.
🟥 Runner username injects root shell commands
When PRELOOP_RUNNER_USER contains shell syntax, runner_account_script embeds it unquoted in the root-run bake script. The injected commands execute with golden-builder privileges.
🟨 Unchecked mirror payload becomes job image
If PRELOOP_GOLDEN_URL has no valid checksum sidecar, download_release_asset installs its payload anyway. The mirror's bytes become the golden without integrity verification.
| // fingerprint untouched would keep a golden whose runner demands | ||
| // the previous version, so every JS action step fails with | ||
| // `bundled nodeXX is missing` against a bundle that is itself | ||
| // perfectly valid at the new pin. | ||
| "node_externals": crate::node_externals::expected_runtimes() |
There was a problem hiding this comment.
🔴 Account changes reuse the wrong golden
Changing PRELOOP_RUNNER_USER or PRELOOP_RUNNER_UID leaves EnvironmentSpec's fingerprint unchanged. The pool reuses an artifact baked for the old account, so jobs run against mismatched home ownership.
Learn more
A configured golden's fingerprint is computed from the base and the default runner contract, hardcoded to runner and UID 1001. Actual baking uses apply_golden_contract, which takes the configured user and UID, while as_runner_user also uses those configured values. On a restart with the same image but a different account, ensure_golden_payload accepts the existing artifact and the new job runs on the old ownership.
Example: Bake with runner/1001, then restart with PRELOOP_RUNNER_UID=2000. The existing fingerprint is reused, while the job runs as UID 2000 against a home owned by 1001.
Recommended fix: Include the effective runner user and UID in the configured-image artifact fingerprint at every path computing it, including the golden-path CLI. Keep the runtime and bake using the same account values.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // fingerprint untouched would keep a golden whose runner demands | ||
| // the previous version, so every JS action step fails with | ||
| // `bundled nodeXX is missing` against a bundle that is itself |
There was a problem hiding this comment.
🔴 Mirror changes leave the old golden active
Changing PRELOOP_GOLDEN_URL does not change the official golden's fingerprint. ensure_golden_payload accepts the existing pack, so the new mirror is never fetched.
Learn more
The official artifact's path is derived from EnvironmentSpec::for_base, which hashes the default or overridden OCI reference. download_prebaked_golden_with_space selects PRELOOP_GOLDEN_URL instead of OCI when set, but that URL does not enter the fingerprint. ensure_golden_payload returns immediately for an existing path, so a change to the selected release mirror cannot invalidate its previous contents.
Example: Fetch the default OCI artifact, then restart with PRELOOP_GOLDEN_URL=https://mirror.example/new-pack. The original pack remains at the same path and the mirror receives no request.
Recommended fix: Include the effective source URL or immutable content identity in the official cache key, and use the same resolution in both the pool and golden-path.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /// The official golden is downloaded packed and is never baked locally. | ||
| /// | ||
| /// The deleted stock-Ubuntu bake used to stand in when the official golden | ||
| /// could not be fetched, which silently ran jobs on a different image than the | ||
| /// one `runs-on: ubuntu-latest` names. There is no substitute now: an | ||
| /// unreachable pack fails the pool at startup, and the error names the | ||
| /// official golden so an operator knows to point | ||
| /// `PRELOOP_GOLDEN_OCI_REF`/`PRELOOP_GOLDEN_URL` at a reachable one. | ||
| #[tokio::test] | ||
| async fn official_golden_download_failure_is_a_startup_error() { | ||
| let fixture = Fixture::new("official-fatal", false); | ||
| let mut config = fixture.config.clone(); | ||
| config.base_image = preloop_orchestrator::environment::OFFICIAL_GOLDEN.to_owned(); | ||
| let provider = Arc::new(RecordingVmProvider::with_machines(&[], vec![])); | ||
| let pool = RunnerPool::new(provider.clone(), config).unwrap(); | ||
|
|
||
| let error = pool | ||
| .run(CancellationToken::new()) | ||
| .await | ||
| .expect_err("an unreachable official golden must fail the pool, not bake a substitute"); | ||
| let message = error.to_string(); | ||
| assert!( | ||
| message.contains("official golden"), | ||
| "the error must name the official golden: {message}" | ||
| ); | ||
| assert!( | ||
| provider.snapshot().await.events.is_empty(), | ||
| "no substitute golden may be created for the official sentinel" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔍 Golden fixes lack before-and-after evidence
The new lifecycle tests cover changed golden behavior, but the PR provides no failing output from the base revision. The repository review guide requires that evidence for bug fixes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| A golden comes from exactly one of two sources. The **official packed golden** | ||
| is the published official GitHub runner image with `preloop-runner` baked in; | ||
| Preloop downloads it per architecture, digest-pinned, and verifies it before | ||
| use. A **configured image** (`PRELOOP_RUNNER_BASE_IMAGE`, or the `[golden] | ||
| base_image` that `preloop init` records) is baked as it is, plus the | ||
| GitHub-runner machinery described below. There is no third source and no local | ||
| fallback: a golden download that cannot complete fails the job that needed it. |
There was a problem hiding this comment.
🔍 Older golden instructions remain discoverable
The new two-source guide coexists with older current-sounding bake instructions elsewhere. Check the operational runbooks for recipes that now describe removed behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
# Conflicts: # .github/workflows/apt-indices-refresh.yml # .github/workflows/release-golden.yml # .github/workflows/release.yml # CHANGELOG.md
The previous run was failed by an engine restart (deploy of the migrated build), not by this tree.
# Conflicts: # crates/preloop-orchestrator/src/lib.rs # crates/preloop-orchestrator/tests/golden_fidelity.rs # crates/preloop-orchestrator/tests/runner_pool_lifecycle.rs
The runner-image dump ships with /var/lib/apt/lists wiped, so a step's
bare `sudo apt-get install <pkg>` fails with 'Unable to locate package'
unless it runs apt-get update first — unlike a GitHub-hosted runner.
Refresh the indices once at bake time (best-effort, 5-minute bound,
skipped on images without apt) and once when an unpacked official pack
has none, before it is frozen; forks inherit the result.
Also:
- startup_cleanup removes goldens keyed on retired environments
(neither the official golden nor the configured image resolves to
them, so no fingerprint rotation retires them) along with their
fingerprint records;
- the artifact sweep reclaims payload files still named for the
retired stock Ubuntu stems, which no fingerprint under the current
stem reaches;
- golden_contract_script("root", …) no longer emits `; ;`, which
`sh -n` rejected — the root contract never actually parsed;
- TestProvider::list reports the machines it created so the stale-
machine cleanup is testable in-process.
3 participants
One golden source survives: the published packed official golden (the official GitHub runner image with
preloop-runnerbaked in). Everything curated is deleted. A configured image is baked as-is plus the GitHub-runner contract.Decisions
runs-on: ubuntu-latest/ubuntu-24.04, and any pool with no image configured, resolve to the digest-pinned packed official golden (PRELOOP_GOLDEN_OCI_REFoverrides it per arch;PRELOOP_GOLDEN_URLstill selects a release-asset mirror). A missing or unverifiable golden FAILS the job: no local bake, no stock-Ubuntu fallback, no direct-create fallback.ubuntu-22.04maps nowhere — it keeps the configured image (the official golden when nothing is configured) instead of silently selecting a 24.04 base.ubuntu-slimresolves the same way. The AgentENV template mapping is out of scope here.runneraccount (uid 1001, home,_work, passwordless sudo) unless the image already has one; ownership of home/_workfixed once via an owner-only walk (find … ! -user 1001 -exec chown -h 1001:1001 {} +, which leaves a runner-owned file's group alone); a writable/opt/hostedtoolcachewithRUNNER_TOOL_CACHE/AGENT_TOOLSDIRECTORYin/etc/environment; and the/etc/preloop-bake.jsonbuild record. The one enforced requirement is a glibc dynamic loader — the bake fails with a clear message if it is missing. Nothing else is checked: a missingbash/git/dockerfails the step, exactly like GitHub-hosted runners. No toolchains, packages, PATH or env overrides.Deleted / kept, file-level
Total: 24 files, +1673 / −3649.
Deleted outright
.github/workflows/apt-indices-refresh.yml(−140)crates/preloop-orchestrator/tests/golden_fidelity.rs(−436)scripts/write-golden-provenance.pyapt-index wiring (−14)environment.rs−705/+157:ToolchainLayer,base_install_script, the apt/pin baseline, the per-runs-onenvironment goldens, the curated classification (898 → 350 lines).lib.rs−1654/+843 (10351 → 9540): toolchain-layer plumbing, environment-golden fan-out and retry loop, the per-VMchown -Rprelude and reconcile script,RUSTUP_HOME/CARGO_HOMEand/usr/local/{cargo,go}/binPATH overrides, the packed-artifact fallback to a local stock bake, the direct-create fallback.versions.toml203 → 47 lines (−166/+10): 90 keys → 5.Kept / renamed
GoldenRegistry→GoldenCache;prepare_golden_for_env→prepare_fork_base, now the single golden entry point: the file-pack backend unpacks the official packed artifact, or packs a golden baked from the configured custom image viabuild_golden_artifact; the non-file-pack (AgentENV) backend boots the image, appliesgolden_contract_scriptin-guest and freezes.PRELOOP_GOLDEN_URL(release-asset mirror),PRELOOP_GOLDEN_OCI_REF(new override),PRELOOP_RUNNER_BASE_IMAGE,[golden] base_image, the fork pool andmachine fork --freeze-sourcebehavior._work+ passwordless sudo, one owner-only ownership walk at build time, writable toolcache +/etc/environment,/etc/preloop-bake.json;preloop build-golden --base-imageresolving to the configured image.Migration notes (what operators must change)
PRELOOP_USE_PACKED_GOLDENis gone — a file-pack backend always uses a packed golden. Remove it from host configs.PRELOOP_GOLDEN_URL, if set, must be a mirror of the published golden release assets; the OCI reference (digest-pinned, per arch) is the default source.ubuntu-22.04no longer selects a 22.04 golden.versions.tomloverrides: the curated keys (ubuntu_24_04_base,ubuntu_22_04_base, toolchain pins, apt pins,apt_indices_max_age_days,github_runner_image_version) no longer exist. The 5 remaining keys arerunner_version,smolvm_min_version,smolvm_golden_version, node externals, and the runner-image docker/buildx pins.preloop build-goldenno longer has a default base: pass--base-image <ref>or configurePRELOOP_RUNNER_BASE_IMAGE/[golden] base_image. The official golden is published packed and cannot (and need not) be built locally.preloop init"official" now stores the choice as a complete answer (kind alone; no base image, nothing to re-ask).Interactions
Rebased on
main@ 5e9778d. Overlaps #426 (which callsprepare_golden_for_env/GoldenRegistryand keeps twois_packed_disabled()guards — this PR renames/deletes those) and #425 (hosted-runtime parity; its limits andguest_hosted_runtime_init_scriptcall in provisioning/as_runner_userare preserved). Whoever merges second rebases.Verification (macstudio, branch b3b4096,
CARGO_TARGET_DIR=$HOME/pr-work/target-curated)cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleanzizmor .github/workflows/— no findingscargo build --locked -p preloop-cli -p preloop-runner-clientandcargo zigbuild --locked -p preloop-runner --target aarch64-unknown-linux-gnu— both succeedcargo test --locked --workspace(scratch Postgres on 127.0.0.1:54931,PROPTEST_CASES=8): 837 passed, 2 failed, 3 ignored in thepreloop-runner-serverlib binary; every other test binary green. Both failures —snapshots::remote_checkout_cache_tests::lfs_private_without_credential_is_not_fetchedandevent_feed::tests::dirty_marks_are_coalesced_into_one_notification— pass in isolation on the same build, sit in files this PR does not touch, and are timing-sensitive (the box ran at load average 20–40 with ~15 concurrent VMs). CI's sharded nextest run is the arbiter.golden-pathand unpacked (31.7 GBlayers-cs), but the golden VM's first start failed atkrun_start_enter returned: -22 (EINVAL)with the host at load average 41; the pool retries. Not yet green.Follow-ups (not in this PR)
benchmarks/real-world/conformance-10repos.sh,conformance-new5repos.sh,benchmarks/substrates/{e2e,project}-bench.sh) still export the removedPRELOOP_USE_PACKED_GOLDENand pin the oldrunner-imagesbase; they need the same official-sentinel rewriteconformance-5repos.shgot.Summary by cubic
Collapses golden sources to exactly two: the published packed official golden — the default for
ubuntu-latest/ubuntu-24.04and any pool with no image configured — and the configured custom image, baked as-is plus the GitHub-runner contract. The curated stock-Ubuntu bake (toolchain layers, apt baseline, package pins) is deleted, and a failed official-golden download or verification now fails the job instead of falling back to a local stock bake or direct create. A deadlock in the golden checksum-probe download is fixed, and apt indices are refreshed at bake time and when an unpacked official pack has none, so baresudo apt-get installsteps work like on GitHub-hosted runners. Startup cleanup also retires goldens and artifact payloads keyed on the removed stock-Ubuntu environments.Migration
PRELOOP_USE_PACKED_GOLDEN; the file-pack backend always uses a packed golden.preloop build-goldenhas no default base: pass--base-image <ref>or setPRELOOP_RUNNER_BASE_IMAGE/[golden] base_image. The official golden is published packed and cannot be built locally.official-image.tomlinstead ofversions.toml.Written for commit 6f57554. Summary will update on new commits.