darwin-ci: address review of the tart host scripts (rebased on main)

scripts/darwin-ci/README.md

@@ -41,7 +41,12 @@ The script is `build/ci-images/darwin-<arch>/bootstrap.sh`, written by
 The versions it installs are the ones every other CI machine gets.
 
 `bake` is safe on a live host: it builds a staging image and swaps it in only
-after the toolchain verifies. Re-run it when toolchain pins move.
+after the toolchain verifies, under the same lock the command hook clones
+with. Re-run it when toolchain pins move.
+
+Everything here runs on whatever bun `host.sh` pinned when the host was first
+provisioned, so it sticks to `Bun.spawn` with argv arrays and stays off
+`Bun.$`.
 
 ## Bringing up a host
 

scripts/darwin-ci/guest/job.sh

@@ -6,20 +6,19 @@ export PATH=/usr/local/bin:/opt/homebrew/bin:/opt/rust/bin:$PATH
 # does not source; without them the proxies in /opt/rust/bin look for a toolchain in ~/.rustup and find none.
 export RUSTUP_HOME=/opt/rust CARGO_HOME=/opt/rust
 export BUILDKITE_BUILD_CHECKOUT_PATH=$HOME/work
-set -a; source ~/job.env; set +a
-cd ~/work
+set -a; source ~/job.env || exit 1; set +a
+cd ~/work || exit 1
 
 echo "[guest] macOS $(sw_vers -productVersion) shard=${BUILDKITE_PARALLEL_JOB:-0}/${BUILDKITE_PARALLEL_JOB_COUNT:-1}"
-bun install >/dev/null 2>&1 || true
-(cd test && bun install >/dev/null 2>&1) || true
-
-shard=()
-[ -n "${BUILDKITE_PARALLEL_JOB:-}" ] && shard=(--shard="$BUILDKITE_PARALLEL_JOB" --max-shards="$BUILDKITE_PARALLEL_JOB_COUNT")
+# warms node_modules with the guest's bun; the runner repeats both installs with the bun under test and reports those
+for dir in . test; do
+  (cd "$dir" && bun install) >"$HOME/install.out" 2>&1 || { echo "[guest] bun install in $dir failed:"; cat "$HOME/install.out"; }
+done
 
 # the runner can leak servers that keep node alive after the run, so wait on it directly and stream its log
 log=$HOME/runner.out
 : >"$log"
-$BUILDKITE_COMMAND "${shard[@]}" >"$log" 2>&1 &
+bash -e -o pipefail -c "$BUILDKITE_COMMAND" >"$log" 2>&1 &
 runner=$!
 tail -f "$log" &
 tailer=$!

scripts/darwin-ci/hooks/command.ts

@@ -1,9 +1,8 @@
-import { $ } from "bun";
 import { unlinkSync, writeFileSync } from "node:fs";
 import { join } from "node:path";
 import { config } from "../lib/config";
 import { guest } from "../lib/guest";
-import { fail } from "../lib/shell";
+import { fail, runInherit, shellQuote } from "../lib/shell";
 import { tart } from "../lib/tart";
 
 const env = process.env;
@@ -26,14 +25,16 @@ const hostOnlyEnv = new Set([
 
 if (!command.includes("runner.node.")) {
   console.log("--- running on host (not a test step)");
-  process.exit((await $`bash -c ${command}`.nothrow()).exitCode);
+  // same flags buildkite-agent's own command runner uses, so a failing middle line still fails the step
+  process.exit(await runInherit(["bash", "-e", "-o", "pipefail", "-c", command], { cwd: checkout }));
 }
 
 let cleaned = false;
 async function cleanup(): Promise<void> {
   if (cleaned) return;
   cleaned = true;
-  await tart.destroy(vm);
+  // pre-exit destroys it again and reaps whatever this misses, so a failure here must not override the test status
+  await tart.destroy(vm).catch((error: unknown) => console.error(`failed to destroy ${vm}: ${error}`));
 }
 for (const signal of ["SIGINT", "SIGTERM", "SIGHUP"] as const) {
   process.on(signal, () => cleanup().finally(() => process.exit(130)));
@@ -61,13 +62,11 @@ async function runInGuest(): Promise<number> {
   console.log(`+++ :test_tube: ${command}`);
   const socket = "/tmp/bk-job-api.sock";
   const forward = ["-o", "StreamLocalBindUnlink=yes", "-R", `${socket}:${env.BUILDKITE_AGENT_JOB_API_SOCKET}`];
-  const status = await g.run(
-    `BUILDKITE_AGENT_JOB_API_SOCKET=${socket} BUILDKITE_AGENT_JOB_API_TOKEN=${env.BUILDKITE_AGENT_JOB_API_TOKEN ?? ""} /bin/bash ~/job.sh`,
-    forward,
-  );
-
-  console.log("--- :outbox_tray: collect reports");
-  await g.collectReports("work", checkout);
+  const jobEnv = [
+    `BUILDKITE_AGENT_JOB_API_SOCKET=${socket}`,
+    `BUILDKITE_AGENT_JOB_API_TOKEN=${shellQuote(env.BUILDKITE_AGENT_JOB_API_TOKEN ?? "")}`,
+  ];
+  const status = await g.run(`${jobEnv.join(" ")} /bin/bash ~/job.sh`, forward);
   return status;
 }
 
@@ -87,9 +86,12 @@ async function retryBoot(): Promise<string | undefined> {
 async function pushEnv(g: ReturnType<typeof guest>): Promise<void> {
   const lines = Object.entries(env)
     .filter(([key]) => /^(BUILDKITE|BUN_|EXPECTED_PLATFORM_)|^(CI|ASAN_OPTIONS)$/.test(key) && !hostOnlyEnv.has(key))
-    .map(([key, value]) => `${key}=${$.escape(String(value ?? ""))}`);
+    .map(([key, value]) => `${key}=${shellQuote(value ?? "")}`);
   const file = `/tmp/${vm}.env`;
-  writeFileSync(file, lines.join("\n") + "\n");
-  await g.push(file, "job.env");
-  unlinkSync(file);
+  writeFileSync(file, lines.join("\n") + "\n", { mode: 0o600 });
+  try {
+    await g.push(file, "job.env");
+  } finally {
+    unlinkSync(file);
+  }
 }

scripts/darwin-ci/hooks/pre-exit.ts

@@ -1,19 +1,31 @@
-import { $ } from "bun";
 import { readdirSync, statSync } from "node:fs";
 import { homedir } from "node:os";
 import { join } from "node:path";
 import { succeeds } from "../lib/shell";
 import { tart } from "../lib/tart";
 
-await tart.destroy(`bk-${process.env.BUILDKITE_JOB_ID ?? "none"}`);
+// best effort: a guest that will not delete is logged and left for the next job or the nightly cleanup, and never
+// changes this job's status (a non-zero pre-exit would fail a job whose tests passed, and every later job on the host)
+async function reap(name: string, action: () => Promise<void>): Promise<void> {
+  try {
+    await action();
+  } catch (error) {
+    console.error(`pre-exit: could not remove ${name}: ${error}`);
+  }
+}
+
+const own = `bk-${process.env.BUILDKITE_JOB_ID ?? "none"}`;
+await reap(own, () => tart.destroy(own));
 
 // a bk-* guest with no `tart run` behind it belongs to a job that died without its cleanup running
 const vms = join(homedir(), ".tart", "vms");
 const tenMinutesAgo = Date.now() - 10 * 60_000;
 for (const name of readdirSync(vms).filter((name: string) => name.startsWith("bk-"))) {
   const stat = statSync(join(vms, name), { throwIfNoEntry: false });
   if (!stat || stat.mtimeMs > tenMinutesAgo) continue;
-  if (await succeeds($`pgrep -f ${`tart run ${name}`}`)) continue;
-  await tart.remove(name);
-  console.log(`pre-exit: reaped orphan guest ${name}`);
+  if (await succeeds(["pgrep", "-f", `tart run ${name}`])) continue;
+  await reap(name, async () => {
+    await tart.remove(name);
+    console.log(`pre-exit: reaped orphan guest ${name}`);
+  });
 }

scripts/darwin-ci/host.sh

@@ -4,20 +4,36 @@ set -euo pipefail
 here=$(cd "$(dirname "$0")" && pwd)
 sudo -n true 2>/dev/null || { echo "needs passwordless sudo for $(whoami)"; exit 1; }
 
-if [ "$(uname -m)" = arm64 ]; then prefix=/opt/homebrew; target=aarch64; else prefix=/usr/local; target=x64; fi
+# https://github.com/Homebrew/install/commits/master
+brew_installer_commit=a34ae4ee9151cbce4c3b33bca7043a972b7ae9a5
+brew_installer_sha256=12479a24be3f5307eecac7cde670fad7118640f031229e964f544b1367b52a41
+# https://github.com/oven-sh/bun/releases/download/bun-v$bun_version/SHASUMS256.txt
+bun_version=1.4.2
+bun_sha256_aarch64=90987a3a16d7db556d886ac3d551e7b6d3edf0a1cf43acaed622e8676be1d12f
+bun_sha256_x64=80520d7e17526308c9185d261679ac6d27798d3803a0e9f7ff9121ab8affb012
+
+if [ "$(uname -m)" = arm64 ]; then prefix=/opt/homebrew; target=aarch64; bun_sha256=$bun_sha256_aarch64; else prefix=/usr/local; target=x64; bun_sha256=$bun_sha256_x64; fi
+
+tmp=$(mktemp -d)
+trap 'rm -rf "$tmp"' EXIT
+
+fetch() { # <url> <file> <sha256>
+  curl -fsSL -o "$2" "$1"
+  echo "$3  $2" | shasum -a 256 -c - >/dev/null || { echo "checksum mismatch for $1"; exit 1; }
+}
 
 if [ ! -x "$prefix/bin/brew" ]; then
-  NONINTERACTIVE=1 /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"
+  fetch "https://raw.githubusercontent.com/Homebrew/install/$brew_installer_commit/install.sh" "$tmp/brew-install.sh" "$brew_installer_sha256"
+  NONINTERACTIVE=1 /bin/bash "$tmp/brew-install.sh"
 fi
 eval "$("$prefix/bin/brew" shellenv)"
 
 if [ ! -x /usr/local/bin/bun ]; then
-  tmp=$(mktemp -d)
-  curl -fsSL -o "$tmp/bun.zip" "https://github.com/oven-sh/bun/releases/latest/download/bun-darwin-$target.zip"
+  fetch "https://github.com/oven-sh/bun/releases/download/bun-v$bun_version/bun-darwin-$target.zip" "$tmp/bun.zip" "$bun_sha256"
   unzip -q "$tmp/bun.zip" -d "$tmp"
   sudo mkdir -p /usr/local/bin
   sudo install -m 755 "$tmp/bun-darwin-$target/bun" /usr/local/bin/bun
-  rm -rf "$tmp"
 fi
 
+rm -rf "$tmp"; trap - EXIT
 exec /usr/local/bin/bun "$here/main.ts" provision "$@"

scripts/darwin-ci/lib/agent.ts

@@ -1,9 +1,21 @@
-import { $ } from "bun";
 import { join } from "node:path";
 import { ciUserHome, ciUserId } from "./ci-user";
 import { config, releaseTier } from "./config";
+import { bootstrapCheckout } from "./host";
 import { plist } from "./launchd";
-import { consoleUser, fail, output, poll, sleep, succeeds, sudoRead, sudoWrite } from "./shell";
+import {
+  consoleUser,
+  fail,
+  poll,
+  probe,
+  run,
+  runInheritOrThrow,
+  sleep,
+  spawn,
+  succeeds,
+  sudoRead,
+  sudoWrite,
+} from "./shell";
 
 const path = "/usr/local/bin:/opt/homebrew/bin:/usr/bin:/bin:/usr/sbin:/sbin";
 const agentLabel = "com.buildkite.buildkite-agent";
@@ -17,13 +29,14 @@ async function agentToken(): Promise<string> {
   );
 }
 
-type TartAgentOptions = { release: number; spawn: number };
+export type TartAgentOptions = { release: number; spawn: number };
 
-export async function installTartAgent({ release, spawn }: TartAgentOptions): Promise<void> {
+export async function installTartAgent({ release, spawn: workers }: TartAgentOptions): Promise<void> {
   const user = config.ciUser;
   if ((await consoleUser()) !== user)
     fail(`${user} is not logged in at the console; run setup-user, reboot, then retry`);
-  const baked = await succeeds($`sudo -u ${user} -H ${config.tart.bin} get ${config.tart.image}`);
+  const asCiUser = ["sudo", "-u", user, "-H"];
+  const baked = await succeeds([...asCiUser, config.tart.bin, "get", config.tart.image]);
   if (!baked) fail(`no ${config.tart.image} image for ${user}; run bake as ${user} first`);
 
   const home = await ciUserHome();
@@ -34,7 +47,7 @@ export async function installTartAgent({ release, spawn }: TartAgentOptions): Pr
   const hooks = join(config.installDir, "hooks");
   const owner = `${user}:staff`;
 
-  await $`sudo install -d -o ${user} -g staff ${builds} ${logs} ${join(home, ".buildkite-agent")}`;
+  await run(["sudo", "install", "-d", "-o", user, "-g", "staff", builds, logs, join(home, ".buildkite-agent")]);
 
   await sudoWrite(
     configPath,
@@ -44,7 +57,7 @@ export async function installTartAgent({ release, spawn }: TartAgentOptions): Pr
       `tags="queue=${config.queue},os=darwin,arch=aarch64,distro=macOS,release=${release},release-tier=${releaseTier(release)},tart=true"`,
       `hooks-path="${hooks}"`,
       `build-path="${builds}"`,
-      `spawn=${spawn}`,
+      `spawn=${workers}`,
       `cancel-grace-period=30`,
       "",
     ].join("\n"),
@@ -71,11 +84,12 @@ export async function installTartAgent({ release, spawn }: TartAgentOptions): Pr
     }),
   );
 
+  const tartAsCiUser = `${asCiUser.join(" ")} ${config.tart.bin}`;
   const nightly = [
     `PATH=${path}`,
     `launchctl bootout gui/${uid}/${agentLabel}`,
     "sleep 40",
-    `for vm in $(sudo -u ${user} tart list --source local --quiet | grep '^bk-'); do sudo -u ${user} tart delete $vm; done`,
+    `for vm in $(${tartAsCiUser} list --source local --quiet | grep '^bk-'); do ${tartAsCiUser} delete $vm; done`,
     `rm -rf ${builds}/* /tmp/bk-*.log`,
     "shutdown -r now",
   ].join("; ");
@@ -89,28 +103,36 @@ export async function installTartAgent({ release, spawn }: TartAgentOptions): Pr
     }),
   );
 
-  await $`sudo launchctl bootout system/buildkite-agent`.quiet().nothrow();
-  await $`sudo launchctl bootout system/${cleanupLabel}`.quiet().nothrow();
+  await spawn(["sudo", "launchctl", "bootout", "system/buildkite-agent"]);
+  await spawn(["sudo", "launchctl", "bootout", `system/${cleanupLabel}`]);
   await unload(`gui/${uid}/${agentLabel}`);
-  await $`sudo launchctl bootstrap gui/${uid} /Library/LaunchAgents/${agentLabel}.plist`;
-  await $`sudo launchctl bootstrap system /Library/LaunchDaemons/${cleanupLabel}.plist`;
+  await run(["sudo", "launchctl", "bootstrap", `gui/${uid}`, `/Library/LaunchAgents/${agentLabel}.plist`]);
+  await run(["sudo", "launchctl", "bootstrap", "system", `/Library/LaunchDaemons/${cleanupLabel}.plist`]);
 
   await sleep(4000);
-  console.log(await output($`sudo tail -4 ${join(logs, "buildkite-agent.log")}`));
+  console.log(
+    (await probe(["sudo", "tail", "-4", join(logs, "buildkite-agent.log")])) ?? "(agent log not written yet)",
+  );
 }
 
 export async function installBareAgent(): Promise<void> {
-  const checkout = join(process.env.HOME!, "bun-bootstrap");
   const token = await agentToken();
-  await $`sudo env BUILDKITE_AGENT_TOKEN=${token} PATH=${path} node scripts/agent.ts install`.cwd(checkout);
-  await sleep(4000);
-  console.log(
-    await output($`tail -4 ${join(process.env.HOME!, "Library", "Logs", "buildkite-agent", "buildkite-agent.log")}`),
+  await runInheritOrThrow(
+    ["sudo", "env", `PATH=${path}`, `BUILDKITE_AGENT_TOKEN=${token}`, "node", "scripts/agent.ts", "install"],
+    {
+      cwd: bootstrapCheckout,
+    },
   );
+  await sleep(4000);
+  const log = join(process.env.HOME!, "Library", "Logs", "buildkite-agent", "buildkite-agent.log");
+  console.log((await probe(["tail", "-4", log])) ?? "(agent log not written yet)");
 }
 
 // bootout returns before a busy agent has finished its cancel grace period, and bootstrap fails with EIO until it has
 async function unload(target: string): Promise<void> {
-  await $`sudo launchctl bootout ${target}`.quiet().nothrow();
-  await poll(30, 2000, async () => ((await succeeds($`sudo launchctl print ${target}`)) ? undefined : true));
+  await spawn(["sudo", "launchctl", "bootout", target]);
+  const unloaded = await poll(30, 2000, async () =>
+    (await succeeds(["sudo", "launchctl", "print", target])) ? undefined : true,
+  );
+  if (!unloaded) fail(`${target} is still loaded after 60s; launchctl bootstrap would fail with EIO`);
 }

scripts/darwin-ci/lib/bake.ts

@@ -2,14 +2,15 @@ import { join } from "node:path";
 import { config, toolchain } from "./config";
 import { ensureHostKey, guest } from "./guest";
 import { generateBootstrap } from "./host";
-import { fail, log } from "./shell";
+import { fail, log, shellQuote, succeeds } from "./shell";
 import { tart } from "./tart";
 
 type BakeOptions = { base: string; ref: string };
 
 export async function bake({ base, ref }: BakeOptions): Promise<void> {
   const { image, cpu, memoryMb } = config.tart;
   const staging = `${image}-new`;
+  if (!(await succeeds(["git", "check-ref-format", "--allow-onelevel", ref]))) fail(`invalid ref: ${ref}`);
   const publicKey = await ensureHostKey();
 
   log(`pull ${base}`);
@@ -28,7 +29,7 @@ export async function bake({ base, ref }: BakeOptions): Promise<void> {
   (await tart.waitForAgent(staging)) ?? fail("tart guest agent not responding");
   await tart.exec(
     staging,
-    `mkdir -p ~/.ssh && chmod 700 ~/.ssh && echo '${publicKey}' > ~/.ssh/authorized_keys && chmod 600 ~/.ssh/authorized_keys`,
+    `mkdir -p ~/.ssh && chmod 700 ~/.ssh && echo ${shellQuote(publicKey)} > ~/.ssh/authorized_keys && chmod 600 ~/.ssh/authorized_keys`,
   );
 
   const g = guest(ip);
@@ -38,17 +39,34 @@ export async function bake({ base, ref }: BakeOptions): Promise<void> {
   await g.push(await generateBootstrap(ref), "/tmp/bootstrap.sh");
 
   log("install the toolchain in the guest");
+  const bakeArgs = [config.buildkiteAgent.version, toolchain.join(" ")].map(shellQuote);
   const status = await g.run(
-    `/bin/bash -l /tmp/bake.sh ${config.buildkiteAgent.version} '${toolchain.join(" ")}' 2>&1 | tee /tmp/bake.log; grep -qx BAKE_OK /tmp/bake.log`,
+    `/bin/bash -l /tmp/bake.sh ${bakeArgs.join(" ")} 2>&1 | tee /tmp/bake.log; grep -qx BAKE_OK /tmp/bake.log`,
   );
   if (status !== 0) fail(`bake failed in guest; ${image} untouched, ${staging} left running at ${ip} for inspection`);
 
-  await g.run("sync; sudo shutdown -h now").catch(() => {});
+  await g.run("sync; sudo shutdown -h now");
   log("waiting for guest to power off");
   await Promise.race([vm.exited, Bun.sleep(180_000)]);
   await tart.stop(staging);
 
-  await tart.remove(image);
-  await tart.rename(staging, image);
+  await promote(staging, image);
   log(`${image} ready`);
 }
+
+// swap under the image lock so no job clones mid-rename; the previous image is kept until the new one is in place
+async function promote(staging: string, image: string): Promise<void> {
+  const previous = `${image}-previous`;
+  await tart.withImageLock(async () => {
+    await tart.destroy(previous);
+    const hadPrevious = await tart.exists(image);
+    if (hadPrevious) await tart.rename(image, previous);
+    try {
+      await tart.rename(staging, image);
+    } catch (error) {
+      if (hadPrevious) await tart.rename(previous, image);
+      throw error;
+    }
+    await tart.destroy(previous);
+  });
+}