Repository navigation
darwin-ci: address review of the tart host scripts #37771
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,8 @@ | ||
| import { unlinkSync, writeFileSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
| import { config } from "../lib/config"; | ||
| import { guest } from "../lib/guest"; | ||
| 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.mjs")) { | ||
| console.log("--- running on host (not a test step)"); | ||
| // 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; | ||
| // 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,10 +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 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); | ||
|
|
||
| console.log("--- :outbox_tray: collect reports"); | ||
| await g.collectReports("work", checkout); | ||
|
|
@@ -87,9 +89,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}=${shellQuote(value ?? "")}`); | ||
| const file = `/tmp/${vm}.env`; | ||
| writeFileSync(file, lines.join("\n") + "\n", { mode: 0o600 }); | ||
| try { | ||
| await g.push(file, "job.env"); | ||
| } finally { | ||
| unlinkSync(file); | ||
| } | ||
|
Comment on lines
93
to
+99
Contributor
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: Bun's Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
sed -n '1,130p' scripts/darwin-ci/hooks/command.ts
printf '%s\n' '--- related temporary-file patterns ---'
rg -n --glob '*.ts' --glob '*.js' 'mkdtempSync|openSync\\([^\\n]*[\"'\"']wx|writeFileSync\\([^\\n]*flag|O_EXCL|O_NOFOLLOW|unlinkSync' scripts src test 2>/dev/null | head -200Repository: oven-sh/bun Length of output: 3789 🏁 Script executed: #!/bin/bash
set -eu
node - <<'JS'
const fs = require("node:fs");
const flags = fs.constants.O_CREAT | fs.constants.O_EXCL;
console.log(JSON.stringify({
wxIncludesCreate: (fs.constants.O_CREAT & flags) === fs.constants.O_CREAT,
wxIncludesExclusive: (fs.constants.O_EXCL & flags) === fs.constants.O_EXCL,
}));
let observed;
try {
try {
throw new Error("push failed");
} finally {
throw new Error("unlink failed");
}
} catch (error) {
observed = error.message;
}
console.log(JSON.stringify({finallyErrorReplacesPrimary: observed === "unlink failed"}));
JSRepository: oven-sh/bun Length of output: 239 Make the temporary environment file exclusive and cleanup-safe.
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,32 @@ | ||
| 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"; | ||
|
|
||
| let failed = false; | ||
| async function reap(name: string, action: () => Promise<void>): Promise<void> { | ||
| try { | ||
| await action(); | ||
| } catch (error) { | ||
| failed = true; | ||
| console.error(`pre-exit: could not remove ${name}: ${error}`); | ||
| } | ||
| } | ||
|
|
||
| const own = `bk-${process.env.BUILDKITE_JOB_ID ?? "none"}`; | ||
| await reap(own, () => tart.destroy(own)); | ||
|
Comment on lines
+17
to
+18
Contributor
There was a problem hiding this comment. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Refuse destructive cleanup when
As per coding guidelines: “Distinguish empty, zero, and unset” and “Validate untrusted input before ... side effects.” 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| // 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; | ||
|
Comment on lines
21
to
25
Contributor
There was a problem hiding this comment. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Handle VM discovery I/O failures explicitly.
As per coding guidelines: “Never swallow failures” and “Every error, abort, and timeout path must complete the operation.” 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| 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}`); | ||
| }); | ||
| } | ||
|
Comment on lines
+26
to
31
Contributor
There was a problem hiding this comment. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
root="$(pwd)"
ast-grep outline "$root/scripts/darwin-ci/lib/tart.ts" --items all
rg -n -C 8 'remove|delete|run' "$root/scripts/darwin-ci/lib/tart.ts"Repository: oven-sh/bun Length of output: 3289 🏁 Script executed: #!/bin/bash
set -euo pipefail
root="$(pwd)"
printf '%s\n' '--- pre-exit.ts ---'
cat -n "$root/scripts/darwin-ci/hooks/pre-exit.ts"
printf '%s\n' '--- shell.ts outline and relevant implementations ---'
ast-grep outline "$root/scripts/darwin-ci/lib/shell.ts" --items all
rg -n -C 12 'function (succeeds|probe|run|spawn)|export .*succeeds|succeeds|probe' "$root/scripts/darwin-ci/lib/shell.ts"
printf '%s\n' '--- pre-exit references and tart lifecycle callers ---'
rg -n -C 6 'pre-exit|reap\(|tart\.remove|tart\.destroy|pgrep' "$root/scripts/darwin-ci"Repository: oven-sh/bun Length of output: 16260 🌐 Web query:
💡 Result: On macOS, the pgrep utility follows standard BSD-style exit codes as documented in its manual page [1][2]. The exit statuses are defined as follows: 0: One or more processes were matched [1][2]. 1: No processes were matched [1][2]. 2: Invalid options were specified on the command line [1][2]. 3: An internal error occurred [1][2]. Because pgrep returns an exit status of 1 when no matching processes are found, it is common in shell scripts to handle this behavior specifically, especially when using set -e (which causes a script to exit immediately if any command returns a non-zero status) [3]. To prevent your script from exiting when no processes are matched, you can append || true or another appropriate fallback to the pgrep command [3]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
root="$(pwd)"
printf '%s\n' '--- tart delete and destroy implementation ---'
cat -n "$root/scripts/darwin-ci/lib/tart.ts" | sed -n '35,66p'
printf '%s\n' '--- read-only exit-code probe ---'
python3 - <<'PY'
import shutil
import subprocess
pgrep = shutil.which("pgrep")
if not pgrep:
print("pgrep: unavailable")
raise SystemExit(0)
cases = [
("no-match", [pgrep, "-f", "coderabbit-no-such-process-8f8c9d7b"]),
("invalid-pattern", [pgrep, "-f", "["]),
]
for label, argv in cases:
result = subprocess.run(argv, stdin=subprocess.DEVNULL,
stdout=subprocess.PIPE, stderr=subprocess.PIPE,
text=True)
print(f"{label}: exit={result.returncode}, stdout={result.stdout!r}, stderr={result.stderr!r}")
print(f"{label}: succeeds-equivalent={result.returncode == 0}")
PYRepository: oven-sh/bun Length of output: 1782 Fail closed on orphan-process detection.
Use 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| process.exit(failed ? 1 : 0); | ||
|
Check warning on line 32 in scripts/darwin-ci/hooks/pre-exit.ts
|
||
|
Comment on lines
+26
to
+32
Contributor
There was a problem hiding this comment. 🟡 The orphan-reap loop sets the same Extended reasoning...What changedBefore this PR,
Because both paths flip the same flag, a failure in (2) — cleanup of a VM this job never touched — makes pre-exit exit 1. Concrete triggerWith
The same happens for any other reason Why nothing prevents it
Why this is the wrong shapeREVIEW.md: "Operations the user explicitly requested fail the whole command on any failure … best-effort auxiliary steps (hints, optional deps) degrade quietly instead of aborting." Orphan reaping is textbook best-effort auxiliary work — it's cleaning up after other jobs, and there's already a nightly FixOnly let the own guest's destroy failure set for (const name of readdirSync(vms).filter(n => n.startsWith("bk-"))) {
...
try {
await tart.remove(name);
console.log(`pre-exit: reaped orphan guest ${name}`);
} catch (error) {
console.error(`pre-exit: could not remove orphan ${name}: ${error}`);
}
}(Or drop |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,22 @@ | ||
| import { join } from "node:path"; | ||
| import { ciUserHome, ciUserId } from "./ci-user"; | ||
| import { config } from "./config"; | ||
| import { bootstrapCheckout } from "./host"; | ||
| import { plist } from "./launchd"; | ||
| import { darwinReleaseTier } from "./release-tier.mjs"; | ||
| 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 +30,14 @@ async function agentToken(): Promise<string> { | |
| ); | ||
| } | ||
|
|
||
| export type TartAgentOptions = { release: number; spawn: number }; | ||
|
|
||
| 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 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,17 +48,17 @@ export async function installTartAgent({ release, spawn }: TartAgentOptions): Pr | |
| const hooks = join(config.installDir, "hooks"); | ||
| const owner = `${user}:staff`; | ||
|
|
||
| await run(["sudo", "install", "-d", "-o", user, "-g", "staff", builds, logs, join(home, ".buildkite-agent")]); | ||
|
|
||
| await sudoWrite( | ||
| configPath, | ||
| [ | ||
| `token="${await agentToken()}"`, | ||
| `name="%hostname-tart-${release}-%spawn"`, | ||
| `tags="queue=${config.queue},os=darwin,arch=aarch64,distro=macOS,release=${release},release-tier=${darwinReleaseTier(release)},tart=true"`, | ||
| `hooks-path="${hooks}"`, | ||
| `build-path="${builds}"`, | ||
| `spawn=${workers}`, | ||
| `cancel-grace-period=30`, | ||
| "", | ||
| ].join("\n"), | ||
|
|
@@ -71,11 +85,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 $(${tartAsCiUser} list --source local --quiet | grep '^bk-'); do ${tartAsCiUser} delete $vm; done`, | ||
| `rm -rf ${builds}/* /tmp/bk-*.log`, | ||
| "shutdown -r now", | ||
| ].join("; "); | ||
|
|
@@ -89,28 +104,33 @@ export async function installTartAgent({ release, spawn }: TartAgentOptions): Pr | |
| }), | ||
| ); | ||
|
|
||
| await spawn(["sudo", "launchctl", "bootout", "system/buildkite-agent"]); | ||
| await spawn(["sudo", "launchctl", "bootout", `system/${cleanupLabel}`]); | ||
| await unload(`gui/${uid}/${agentLabel}`); | ||
|
Comment on lines
+107
to
109
Contributor
There was a problem hiding this comment. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Do not discard
Accept only the known absent-service result. Throw all other As per coding guidelines, “Never swallow failures or signal success after failure; propagate I/O, syscall, cleanup, and requested-operation errors explicitly.” Also applies to: 132-135 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| 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 probe(["sudo", "tail", "-4", join(logs, "buildkite-agent.log")])) ?? "(agent log not written yet)", | ||
| ); | ||
| } | ||
|
|
||
| export async function installBareAgent(): Promise<void> { | ||
| const token = await agentToken(); | ||
| await runInheritOrThrow( | ||
| ["sudo", "env", `PATH=${path}`, `BUILDKITE_AGENT_TOKEN=${token}`, "node", "scripts/agent.mjs", "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 spawn(["sudo", "launchctl", "bootout", target]); | ||
| await poll(30, 2000, async () => ((await succeeds(["sudo", "launchctl", "print", target])) ? undefined : true)); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 1812
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 28404
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 18786
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 15986
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 50369
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 396
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 335
Keep the API token out of SSH arguments and validate the environment file.
guest.runpasses the command toBun.spawn(["ssh", ..., command]), soshellQuotedoes not hide the token from process arguments. Add the token to the protectedjob.envtransport and reject unset or empty credentials before starting SSH.In
pushEnv, validate environment keys before writing a file thatjob.shsources. The current filter accepts keys containing newlines, which can execute injected commands. Create/tmp/${vm}.envexclusively with private permissions;{ mode: 0o600 }does not change an existing file’s mode. Preserve the original push error if cleanup fails.🤖 Prompt for AI Agents
Source: Coding guidelines