Repository navigation
darwin-ci: address review of the tart host scripts - #37771
Jarred-Sumner wants to merge 1 commit into
Conversation
Follow-ups from the review of #37633, which added scripts/darwin-ci. The scripts now use Bun.spawn with argv arrays instead of Bun.$: a host runs them on whatever bun host.sh installed when it was provisioned, and Bun.$'s quoting differs between releases ($.escape does not produce bash-safe strings, so job.env broke on any commit message containing a quote; literal `*` words were glob-expanded, so collectReports never ran). - shell.ts: `run`/`output` throw on failure, `probe` is for commands whose failure is an answer; `shellQuote` for strings handed to a remote shell - tart.ts: the clone lock is a symlink holding the owner's pid, broken only when that pid is dead; stop/destroy tolerate exactly "not running" / "does not exist" and propagate everything else - bake.ts: validates the ref up front, quotes every argument passed into the guest, and swaps the image in under the clone lock keeping the previous image until the rename succeeds - release-tier.mjs: one copy of the release-tier thresholds shared with scripts/agent.mjs, so bare and tart hosts cannot disagree - job.sh: run BUILDKITE_COMMAND through `bash -e -o pipefail` (sharding already comes from the forwarded env), stop hiding warm-up install output - command hook: host steps also run under `bash -e -o pipefail`; job.env is 0600 and always removed; a cleanup failure no longer masks the test status - pre-exit: an orphan that fails to delete is logged and the rest still reaped - ci-user: createhomedir/dseditgroup only when needed and no longer nothrow; ciUserId rejects uid 0 - host.sh: brew installer and bun pinned by commit/version and sha256 - host.ts: profiles touched by explicit path, agent download cleaned up in finally; agent.ts nightly reap uses `sudo -H` like its siblings - main.ts: --release/--spawn must be positive integers No-Verification-Needed: CI provisioning scripts, no bun runtime surface
WalkthroughChangesDarwin CI reliability and provisioning
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/darwin-ci/hooks/command.ts`:
- Around line 93-99: Update the temporary file handling around writeFileSync and
g.push so creation is atomic and exclusive with 0o600 permissions, including
file creation and writing inside the try scope. Ensure cleanup runs only after a
successfully created file, and preserve the original operation error if
unlinkSync fails during cleanup.
- Around line 65-69: Update the command setup around jobEnv and g.run to pass
the API token through protected job.env transport rather than SSH command
arguments, and reject missing or empty credentials before starting SSH. In
pushEnv, validate environment keys against newlines and other unsafe content
before writing the sourced file. Create /tmp/${vm}.env exclusively with private
permissions, and preserve the original push error if cleanup also fails.
In `@scripts/darwin-ci/hooks/pre-exit.ts`:
- Around line 17-18: Validate BUILDKITE_JOB_ID before constructing own or
invoking the destructive reap/tart.destroy cleanup, rejecting both unset and
empty values. Only allow cleanup when the ID is a valid non-empty value, while
preserving the existing bk-prefixed ownership flow for valid IDs.
- Around line 21-25: Update the VM discovery flow around readdirSync, statSync,
and reap so directory-read failures are caught and recorded in the existing
failed status path, while per-entry stat failures are handled without aborting
discovery of later VMs. Ensure every discovery error is surfaced rather than
swallowed and the hook completes with an explicit nonzero exit status when any
failure occurs.
- Around line 26-31: Update the orphan detection around succeeds and pgrep to
fail closed: treat only exit code 1 as “no matching process,” and report any
other pgrep failure instead of deleting the guest. In the reap callback, replace
tart.remove(name) with tart.destroy(name) or equivalent lock-aware deletion that
retries until tart run releases the VM lock.
In `@scripts/darwin-ci/host.sh`:
- Around line 31-35: Update the Bun installation condition in host.sh to
validate the existing /usr/local/bin/bun against the freshly fetched and
verified archive binary, rather than checking only executability. Always fetch
and verify the pinned archive, compare its extracted bun binary with the
installed one, and run the existing sudo install steps when they differ; skip
installation only when the binaries match.
In `@scripts/darwin-ci/lib/agent.ts`:
- Around line 107-109: Update the cleanup flow around the launchctl bootout
spawn calls and unload/poll logic to inspect and propagate failures instead of
discarding results. Accept only the known absent-service exit result; throw
other bootout failures including stderr. When poll expires and returns
undefined, throw an error naming target and stating it did not unload before
bootstrap, including the analogous cleanup path.
In `@scripts/darwin-ci/lib/ci-user.ts`:
- Around line 16-19: Update ciUserId to validate the raw output from
output(["id", "-u", user]) as a canonical positive safe integer before
converting it to a number; reject decimal, Infinity, exponent, leading-zero,
malformed, and out-of-range values via the existing fail path. Add regression
tests covering malformed and unsafe UID output.
In `@scripts/darwin-ci/lib/shell.ts`:
- Around line 16-100: Add automated regression tests for the provisioning
contracts: in scripts/darwin-ci/lib/shell.ts lines 16-100 cover argv
preservation, stdin, stdout/stderr, failures, probes, and inherited output; in
scripts/darwin-ci/host.sh lines 7-38 cover checksum rejection and verified Bun
replacement; in scripts/darwin-ci/lib/host.ts lines 11-111 cover command
failures, bootstrap verification, and temporary-directory cleanup; in
scripts/darwin-ci/lib/ci-user.ts lines 9-81 cover UID parsing, membership, and
home-directory branches; in scripts/darwin-ci/lib/guest.ts lines 25-59 assert
exact SSH, SCP, and rsync argv; and in scripts/darwin-ci/guest/job.sh lines 9-28
cover environment, dependency-install, runner-status, and tail-cleanup failures.
In `@scripts/darwin-ci/lib/tart.ts`:
- Around line 67-91: Add automated regression tests for
scripts/darwin-ci/lib/tart.ts lines 67-91 covering concurrent acquisition,
dead-owner recovery, callback failure cleanup, and preservation of a live owner
lock; test scripts/darwin-ci/lib/bake.ts lines 53-68 for successful promotion
and restoration of the previous image when staging rename fails. Use the
existing test patterns and isolate filesystem/process behavior with appropriate
mocks or temporary fixtures.
- Around line 10-15: Update lockOwner() to catch the readlinkSync(imageLock)
error, return undefined only when its code is ENOENT, and rethrow every other
failure so withImageLock can report lock-resource errors instead of retrying
indefinitely.
In `@scripts/darwin-ci/main.ts`:
- Around line 64-67: Update positiveInteger to store Number(value) in a parsed
variable and reject it with Number.isSafeInteger(parsed) after the existing
format validation, preserving the positive-integer error behavior for unsafe or
overflowing values. Add regression coverage for 9007199254740992,
9007199254740993, and an overflowing decimal string.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93d9fca2-04c8-46a7-ba1e-a0e2a89445e6
📒 Files selected for processing (16)
scripts/agent.mjsscripts/darwin-ci/README.mdscripts/darwin-ci/guest/job.shscripts/darwin-ci/hooks/command.tsscripts/darwin-ci/hooks/pre-exit.tsscripts/darwin-ci/host.shscripts/darwin-ci/lib/agent.tsscripts/darwin-ci/lib/bake.tsscripts/darwin-ci/lib/ci-user.tsscripts/darwin-ci/lib/config.tsscripts/darwin-ci/lib/guest.tsscripts/darwin-ci/lib/host.tsscripts/darwin-ci/lib/release-tier.mjsscripts/darwin-ci/lib/shell.tsscripts/darwin-ci/lib/tart.tsscripts/darwin-ci/main.ts
💤 Files with no reviewable changes (1)
- scripts/darwin-ci/lib/config.ts
| 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); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="$(pwd)"
ast-grep outline "$root/scripts/darwin-ci/lib/guest.ts" --items all --type function --match 'run'
rg -n -C 8 'Bun\.spawn|ssh|command' "$root/scripts/darwin-ci/lib/guest.ts"Repository: oven-sh/bun
Length of output: 1812
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="$(pwd)"
printf '%s\n' '--- command.ts ---'
cat -n "$root/scripts/darwin-ci/hooks/command.ts"
printf '%s\n' '--- guest shell helpers ---'
cat -n "$root/scripts/darwin-ci/lib/shell.ts"
printf '%s\n' '--- job scripts and env references ---'
rg -n -C 10 'job\.env|BUILDKITE_AGENT_JOB_API|writeFileSync|unlinkSync|env\.' \
"$root/scripts/darwin-ci"Repository: oven-sh/bun
Length of output: 28404
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="$(pwd)"
printf '%s\n' '--- relevant call graph ---'
ast-grep outline "$root/scripts/darwin-ci/hooks/command.ts" --items all --type function
ast-grep outline "$root/scripts/darwin-ci/lib/shell.ts" --items all --type function
printf '%s\n' '--- command construction and execution ---'
rg -n -C 15 'runInherit|function run|const run|Bun\.spawn|writeFileSync|unlinkSync|job\.sh|job\.env' \
"$root/scripts/darwin-ci/hooks/command.ts" \
"$root/scripts/darwin-ci/lib/shell.ts" \
"$root/scripts/darwin-ci/guest"
printf '%s\n' '--- files containing job scripts ---'
git ls-files "$root/scripts/darwin-ci" | sed -n '/job\|env/p'Repository: oven-sh/bun
Length of output: 18786
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="$(pwd)"
printf '%s\n' '--- command.ts ---'
sed -n '1,150p' "$root/scripts/darwin-ci/hooks/command.ts"
printf '%s\n' '--- shell.ts ---'
sed -n '1,220p' "$root/scripts/darwin-ci/lib/shell.ts"
printf '%s\n' '--- guest job files ---'
rg --files "$root/scripts/darwin-ci" | rg '(^|/)(job(\.sh|\.env)?|.*job.*)$' || true
rg -n -C 8 'job\.env|BUILDKITE_AGENT_JOB_API|source .*env|set -a|export ' "$root/scripts/darwin-ci" || trueRepository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="$(pwd)"
python3 - "$root/scripts/darwin-ci/hooks/command.ts" <<'PY'
import sys
from pathlib import Path
p = Path(sys.argv[1])
for i, line in enumerate(p.read_text().splitlines(), 1):
if 1 <= i <= 130:
print(f"{i:4}: {line}")
PY
printf '%s\n' '--- all environment-file and token consumers ---'
rg -n -C 12 'jobEnv|job\.env|BUILDKITE_AGENT_JOB_API|envFile|environment' "$root/scripts/darwin-ci" || trueRepository: oven-sh/bun
Length of output: 15986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="$(pwd)"
python3 - "$root/scripts/darwin-ci/hooks/command.ts" <<'PY'
import sys
from pathlib import Path
lines = Path(sys.argv[1]).read_text().splitlines()
for start, end in ((1, 45), (55, 110)):
print(f"--- lines {start}-{end} ---")
for n in range(start, min(end, len(lines)) + 1):
print(f"{n:4}: {lines[n-1]}")
PY
printf '%s\n' '--- execution helper definitions ---'
rg -n -C 20 'runInherit|export async function run|function run|Bun\.spawn' \
"$root/scripts/darwin-ci" || true
printf '%s\n' '--- job.sh and job.env references ---'
rg -n -C 15 'job\.sh|job\.env|BUILDKITE_AGENT_JOB_API_SOCKET|BUILDKITE_AGENT_JOB_API_TOKEN' \
"$root" -g '!node_modules' -g '!build' -g '!dist' || trueRepository: oven-sh/bun
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os
import subprocess
import tempfile
def shell_quote(value):
return "'" + value.replace("'", "'\\''") + "'"
# Model the exact pushEnv serialization for a matching but invalid environment key.
key = "BUILDKITE_INJECT\nprintf vulnerable > \"$MARKER\" #"
value = "safe"
with tempfile.TemporaryDirectory() as d:
env_file = os.path.join(d, "job.env")
marker = os.path.join(d, "marker")
with open(env_file, "w") as f:
f.write(f"{key}={shell_quote(value)}\n")
result = subprocess.run(
["/bin/bash", "-c", "set -a; source \"$1\"; set +a"],
args=["bash", env_file],
env={**os.environ, "MARKER": marker},
capture_output=True,
text=True,
)
print("invalid-key-source-exit:", result.returncode)
print("invalid-key-created-file:", os.path.exists(marker))
print("invalid-key-stderr:", result.stderr.strip())
# Show that mode=0o600 does not make an existing predictable file private.
with tempfile.TemporaryDirectory() as d:
path = os.path.join(d, "bk-job.env")
with open(path, "w") as f:
f.write("old\n")
os.chmod(path, 0o644)
with open(path, "w") as f:
f.write("secret\n")
print("existing-file-mode-after-write:", oct(os.stat(path).st_mode & 0o777))
PY
printf '%s\n' '--- relevant filesystem API documentation availability ---'
node -e 'try { console.log(require.resolve("node:fs")) } catch { console.log("node:fs unavailable") }'Repository: oven-sh/bun
Length of output: 396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os
import subprocess
import tempfile
def shell_quote(value):
return "'" + value.replace("'", "'\\''") + "'"
key = "BUILDKITE_INJECT\nprintf vulnerable > \"$MARKER\" #"
value = "safe"
with tempfile.TemporaryDirectory() as d:
env_file = os.path.join(d, "job.env")
marker = os.path.join(d, "marker")
with open(env_file, "w") as f:
f.write(f"{key}={shell_quote(value)}\n")
result = subprocess.run(
["/bin/bash", "-c", 'set -a; source "$1"; set +a', "--", env_file],
env={**os.environ, "MARKER": marker},
capture_output=True,
text=True,
)
print("invalid-key-source-exit:", result.returncode)
print("invalid-key-created-file:", os.path.exists(marker))
print("invalid-key-stderr:", result.stderr.strip())
with tempfile.TemporaryDirectory() as d:
path = os.path.join(d, "bk-job.env")
with open(path, "w") as f:
f.write("old\n")
os.chmod(path, 0o644)
with open(path, "w") as f:
f.write("secret\n")
print("existing-file-mode-after-write:", oct(os.stat(path).st_mode & 0o777))
PYRepository: oven-sh/bun
Length of output: 335
Keep the API token out of SSH arguments and validate the environment file.
guest.run passes the command to Bun.spawn(["ssh", ..., command]), so shellQuote does not hide the token from process arguments. Add the token to the protected job.env transport and reject unset or empty credentials before starting SSH.
In pushEnv, validate environment keys before writing a file that job.sh sources. The current filter accepts keys containing newlines, which can execute injected commands. Create /tmp/${vm}.env exclusively 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/hooks/command.ts` around lines 65 - 69, Update the command
setup around jobEnv and g.run to pass the API token through protected job.env
transport rather than SSH command arguments, and reject missing or empty
credentials before starting SSH. In pushEnv, validate environment keys against
newlines and other unsafe content before writing the sourced file. Create
/tmp/${vm}.env exclusively with private permissions, and preserve the original
push error if cleanup also fails.
Source: Coding guidelines
| 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); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Verify Bun's node:fs writeFileSync mode and symlink behavior for existing paths, and identify the supported exclusive non-following temporary-file API.
💡 Result:
Bun's node:fs implementation of writeFileSync supports standard Node.js file system flags, including 'wx' [1]. The 'wx' flag is the standard mechanism for exclusive, non-following file creation; it combines O_EXCL and O_CREAT behavior, failing if the path already exists (including symlinks) [2][3][4][1]. Regarding the requested "exclusive non-following temporary-file API," Bun does not provide a single node:fs method that combines "exclusive," "non-following," and "temporary-file" operations in one atomic call beyond standard fs.open flags [5][6]. However, developers typically achieve this pattern in Bun (and Node.js) by using fs.openSync with a combination of flags—such as O_CREAT, O_EXCL, and O_NOFOLLOW—to safely open a unique file path [5][2][4]. Bun maintains high compatibility with the Node.js fs module [5][7]. Specifically: - Symlink behavior: When using writeFileSync or other fs methods, Bun follows symbolic links by default unless specific flags (like O_NOFOLLOW) are used via lower-level fs.open operations [5][2]. - Exclusive/Non-following: The 'wx' flag ensures that the operation fails if the path exists (regardless of whether it is a file or a symlink), fulfilling the "exclusive" and "non-following" requirements [2][3][4][1]. - Native optimization: While node:fs is recommended for compatibility, Bun also provides Bun.write(), which is optimized for performance and handles file creation/overwriting automatically but does not directly expose the same granular flag-based control as the node:fs module [7][8].
Citations:
- 1: fs: honor the flag option in appendFile / appendFileSync #33559
- 2: https://bun.sh/reference/node/fs/constants
- 3: https://manned.org/man.2ec9ef40/ubuntu-warty/creat.2
- 4: https://manned.org/open.2
- 5: https://bun.sh/reference/node/fs
- 6: https://bun.com/reference/node/fs/writeFileSync
- 7: https://bun.com/docs/runtime/file-io
- 8: https://bun.sh/reference/bun/write
🏁 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.
writeFileSync runs before the try, and { mode: 0o600 } does not protect an existing path. Create the file atomically with openSync(..., "wx", 0o600) or a unique temporary directory. Include creation and writing in the cleanup scope. Preserve the original operation error when unlinkSync also fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/hooks/command.ts` around lines 93 - 99, Update the
temporary file handling around writeFileSync and g.push so creation is atomic
and exclusive with 0o600 permissions, including file creation and writing inside
the try scope. Ensure cleanup runs only after a successfully created file, and
preserve the original operation error if unlinkSync fails during cleanup.
Source: Coding guidelines
| const own = `bk-${process.env.BUILDKITE_JOB_ID ?? "none"}`; | ||
| await reap(own, () => tart.destroy(own)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Refuse destructive cleanup when BUILDKITE_JOB_ID is missing or empty.
?? "none" converts an unset ID into bk-none. An empty ID produces bk-. If either VM exists, this hook can stop and delete an unrelated guest. Validate the job ID before calling tart.destroy.
As per coding guidelines: “Distinguish empty, zero, and unset” and “Validate untrusted input before ... side effects.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/hooks/pre-exit.ts` around lines 17 - 18, Validate
BUILDKITE_JOB_ID before constructing own or invoking the destructive
reap/tart.destroy cleanup, rejecting both unset and empty values. Only allow
cleanup when the ID is a valid non-empty value, while preserving the existing
bk-prefixed ownership flow for valid IDs.
Source: Coding guidelines
| 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; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle VM discovery I/O failures explicitly.
readdirSync(vms) and statSync(...) are outside reap. A missing or inaccessible Tart directory can abort the loop, skip later guests, and bypass the controlled failed status path. Mark directory failures, handle per-entry failures, and finish with an explicit nonzero status.
As per coding guidelines: “Never swallow failures” and “Every error, abort, and timeout path must complete the operation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/hooks/pre-exit.ts` around lines 21 - 25, Update the VM
discovery flow around readdirSync, statSync, and reap so directory-read failures
are caught and recorded in the existing failed status path, while per-entry stat
failures are handled without aborting discovery of later VMs. Ensure every
discovery error is surfaced rather than swallowed and the hook completes with an
explicit nonzero exit status when any failure occurs.
Source: 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}`); | ||
| }); | ||
| } |
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:
macOS pgrep exit status 0 1 2 3 no processes matched man page
💡 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:
- 1: https://www.unix.com/man_page/osx/1/pgrep/
- 2: https://man.freebsd.org/cgi/man.cgi?query=pgrep&sektion=1
- 3: https://unix.stackexchange.com/questions/603982/pgrep-f-exits-with-1
🏁 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.
succeeds treats every nonzero pgrep exit code as false. This includes invalid-option and internal-error statuses, so probe failures can trigger tart.remove(name). Accept only pgrep exit code 1 as “no match”; report all other failures.
Use tart.destroy(name) or equivalent lock-aware deletion logic. tart.remove(name) does not retry while tart run still holds the VM lock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/hooks/pre-exit.ts` around lines 26 - 31, Update the orphan
detection around succeeds and pgrep to fail closed: treat only exit code 1 as
“no matching process,” and report any other pgrep failure instead of deleting
the guest. In the reap callback, replace tart.remove(name) with
tart.destroy(name) or equivalent lock-aware deletion that retries until tart run
releases the VM lock.
Source: Coding guidelines
| export async function ciUserId(): Promise<number> { | ||
| const uid = Number(await output(["id", "-u", user])); | ||
| return uid > 0 ? uid : fail(`unexpected uid ${uid} for ${user}`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
bun bd -e '
for (const raw of ["1", "0", "-1", "1.5", "Infinity", "NaN", "1e3"]) {
const uid = Number(raw);
console.log(raw, "old=", uid > 0, "new=", Number.isSafeInteger(uid) && uid > 0 && String(uid) === raw);
}
'Repository: oven-sh/bun
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ROOT="$(pwd)"
printf '%s\n' "== ci-user.ts =="
sed -n '1,120p' "$ROOT/scripts/darwin-ci/lib/ci-user.ts"
printf '%s\n' "== output() definition and ciUserId() call sites =="
rg -n -A12 -B4 'function output|const output|export .*output|ciUserId\(' "$ROOT/scripts/darwin-ci"
printf '%s\n' "== related tests and review guidance =="
rg -n -i 'ci-user|ciUserId|launchctl|darwin-ci' "$ROOT/REVIEW.md" "$ROOT/scripts/darwin-ci" --glob '*test*' --glob '*spec*' 2>/dev/null || trueRepository: oven-sh/bun
Length of output: 7746
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const values = ["1", "0", "-1", "1.5", "Infinity", "NaN", "1e3", " 1 ", "01", "9007199254740992"];
for (const raw of values) {
const uid = Number(raw);
console.log(JSON.stringify(raw), {
old: uid > 0,
safeInteger: Number.isSafeInteger(uid),
canonical: Number.isSafeInteger(uid) && uid > 0 && String(uid) === raw,
});
}
JSRepository: oven-sh/bun
Length of output: 737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ROOT="$(pwd)"
printf '%s\n' "== ciUserId consumers and launchctl usage =="
rg -n -A18 -B8 'ciUserId|launchctl|gui/\$\{?uid|gui/' "$ROOT/scripts/darwin-ci"
printf '%s\n' "== shell command and failure helpers =="
sed -n '1,110p' "$ROOT/scripts/darwin-ci/lib/shell.ts"
printf '%s\n' "== darwin-ci test files =="
git ls-files "$ROOT/scripts/darwin-ci" | rg '(^|/)(test|tests|spec|__tests__)/|\\.(test|spec)\\.'Repository: oven-sh/bun
Length of output: 22395
Reject non-canonical UID output.
Number() accepts values such as "1.5", "Infinity", "1e3", and "01". Accept only a safe, canonical positive integer before constructing the launchctl target. Add regression tests for malformed and out-of-range output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/lib/ci-user.ts` around lines 16 - 19, Update ciUserId to
validate the raw output from output(["id", "-u", user]) as a canonical positive
safe integer before converting it to a number; reject decimal, Infinity,
exponent, leading-zero, malformed, and out-of-range values via the existing fail
path. Add regression tests covering malformed and unsafe UID output.
Source: Coding guidelines
| export const shellQuote = (arg: string) => `'${arg.replace(/'/g, `'\\''`)}'`; | ||
|
|
||
| export type Result = { exitCode: number; stdout: string; stderr: string }; | ||
| export type SpawnOptions = { | ||
| cwd?: string; | ||
| stdin?: string | Uint8Array<ArrayBuffer>; | ||
| env?: Record<string, string | undefined>; | ||
| }; | ||
|
|
||
| export async function spawn(argv: string[], { cwd, stdin, env }: SpawnOptions = {}): Promise<Result> { | ||
| const proc = Bun.spawn(argv, { | ||
| cwd, | ||
| env, | ||
| stdin: stdin === undefined ? "ignore" : new Blob([stdin]), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([ | ||
| new Response(proc.stdout).text(), | ||
| new Response(proc.stderr).text(), | ||
| proc.exited, | ||
| ]); | ||
| return { exitCode, stdout, stderr }; | ||
| } | ||
|
|
||
| // only the leading words: argv may carry a password further along | ||
| const describe = (argv: string[]) => argv.slice(0, 3).join(" "); | ||
|
|
||
| export async function run(argv: string[], options?: SpawnOptions): Promise<string> { | ||
| const { exitCode, stdout, stderr } = await spawn(argv, options); | ||
| if (exitCode !== 0) throw new Error(`${describe(argv)} exited ${exitCode}\n${stderr}`.trimEnd()); | ||
| return stdout; | ||
| } | ||
|
|
||
| // stdio passed through, for long-running commands whose progress the operator wants to see | ||
| export function runInherit(argv: string[], { cwd, env }: SpawnOptions = {}): Promise<number> { | ||
| return Bun.spawn(argv, { cwd, env, stdin: "ignore", stdout: "inherit", stderr: "inherit" }).exited; | ||
| } | ||
|
|
||
| export async function runInheritOrThrow(argv: string[], options?: SpawnOptions): Promise<void> { | ||
| const exitCode = await runInherit(argv, options); | ||
| if (exitCode !== 0) throw new Error(`${describe(argv)} exited ${exitCode}`); | ||
| } | ||
|
|
||
| export async function output(proc: $.ShellPromise): Promise<string> { | ||
| return (await proc.quiet().nothrow().text()).trim(); | ||
| export const output = async (argv: string[]) => (await run(argv)).trim(); | ||
|
|
||
| // for commands whose failure is an expected answer (a probe of something that may not exist yet) | ||
| export async function probe(argv: string[]): Promise<string | undefined> { | ||
| const { exitCode, stdout } = await spawn(argv); | ||
| return exitCode === 0 ? stdout.trim() || undefined : undefined; | ||
| } | ||
|
|
||
| export const succeeds = async (argv: string[]) => (await spawn(argv)).exitCode === 0; | ||
|
|
||
| export async function poll<T>( | ||
| attempts: number, | ||
| intervalMs: number, | ||
| probe: () => Promise<T | undefined>, | ||
| check: () => Promise<T | undefined>, | ||
| ): Promise<T | undefined> { | ||
| for (let i = 0; i < attempts; i++) { | ||
| const value = await probe(); | ||
| const value = await check(); | ||
| if (value !== undefined) return value; | ||
| await sleep(intervalMs); | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| export function portOpen(host: string, port: number): Promise<boolean> { | ||
| return succeeds($`nc -z -w2 ${host} ${port}`); | ||
| } | ||
| export const portOpen = (host: string, port: number) => succeeds(["nc", "-z", "-w2", host, String(port)]); | ||
|
|
||
| export async function sudoWrite( | ||
| path: string, | ||
| content: string | Uint8Array, | ||
| content: string | Uint8Array<ArrayBuffer>, | ||
| mode = "644", | ||
| owner = "root:wheel", | ||
| ): Promise<void> { | ||
| await $`sudo tee ${path} < ${new Response(content)}`.quiet(); | ||
| await $`sudo chown ${owner} ${path}`.quiet(); | ||
| await $`sudo chmod ${mode} ${path}`.quiet(); | ||
| await run(["sudo", "tee", path], { stdin: content }); | ||
| await run(["sudo", "chown", owner, path]); | ||
| await run(["sudo", "chmod", mode, path]); | ||
| } | ||
|
|
||
| export async function sudoRead(path: string): Promise<string | undefined> { | ||
| return (await output($`sudo cat ${path}`)) || undefined; | ||
| } | ||
| export const sudoRead = (path: string) => probe(["sudo", "cat", path]); | ||
|
|
||
| export async function consoleUser(): Promise<string | undefined> { | ||
| const user = await output($`stat -f %Su /dev/console`); | ||
| return user && user !== "root" ? user : undefined; | ||
| const user = await probe(["stat", "-f", "%Su", "/dev/console"]); | ||
| return user !== "root" ? user : undefined; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add automated regression coverage for the changed provisioning contracts. No test change is included for these behavior changes.
scripts/darwin-ci/lib/shell.ts#L16-L100: test argv preservation, stdin delivery, stdout and stderr capture, nonzero failures, probes, and inherited output.scripts/darwin-ci/host.sh#L7-L38: test checksum rejection and verified replacement of an existing Bun binary.scripts/darwin-ci/lib/host.ts#L11-L111: test mocked command failure, bootstrap verification, and temporary-directory cleanup paths.scripts/darwin-ci/lib/ci-user.ts#L9-L81: test valid and malformed UID output plus membership and home-directory branches.scripts/darwin-ci/lib/guest.ts#L25-L59: test the exact SSH, SCP, and rsync argv arrays.scripts/darwin-ci/guest/job.sh#L9-L28: test environment-load failure, dependency-install failure, runner status propagation, and tail cleanup.
As per coding guidelines, “Every behavioral change must include an automated regression test in the same change.”
📍 Affects 6 files
scripts/darwin-ci/lib/shell.ts#L16-L100(this comment)scripts/darwin-ci/host.sh#L7-L38scripts/darwin-ci/lib/host.ts#L11-L111scripts/darwin-ci/lib/ci-user.ts#L9-L81scripts/darwin-ci/lib/guest.ts#L25-L59scripts/darwin-ci/guest/job.sh#L9-L28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/lib/shell.ts` around lines 16 - 100, Add automated
regression tests for the provisioning contracts: in
scripts/darwin-ci/lib/shell.ts lines 16-100 cover argv preservation, stdin,
stdout/stderr, failures, probes, and inherited output; in
scripts/darwin-ci/host.sh lines 7-38 cover checksum rejection and verified Bun
replacement; in scripts/darwin-ci/lib/host.ts lines 11-111 cover command
failures, bootstrap verification, and temporary-directory cleanup; in
scripts/darwin-ci/lib/ci-user.ts lines 9-81 cover UID parsing, membership, and
home-directory branches; in scripts/darwin-ci/lib/guest.ts lines 25-59 assert
exact SSH, SCP, and rsync argv; and in scripts/darwin-ci/guest/job.sh lines 9-28
cover environment, dependency-install, runner-status, and tail-cleanup failures.
Source: Coding guidelines
| function lockOwner(): number | undefined { | ||
| try { | ||
| return Number(readlinkSync(imageLock)); | ||
| } catch { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate non-ENOENT lock-read failures.
If symlinkSync reports EEXIST and readlinkSync(imageLock) fails with EACCES or an I/O error, this function returns undefined. withImageLock then sleeps and retries forever instead of reporting the failed lock resource.
Only treat ENOENT as an absent lock. Rethrow every other error.
Proposed fix
function lockOwner(): number | undefined {
try {
return Number(readlinkSync(imageLock));
- } catch {
- return undefined;
+ } catch (error) {
+ if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined;
+ throw error;
}
}As per coding guidelines: “Never swallow failures or signal success after failure; propagate I/O, syscall, cleanup, and requested-operation errors explicitly.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function lockOwner(): number | undefined { | |
| try { | |
| return Number(readlinkSync(imageLock)); | |
| } catch { | |
| return undefined; | |
| } | |
| function lockOwner(): number | undefined { | |
| try { | |
| return Number(readlinkSync(imageLock)); | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined; | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/lib/tart.ts` around lines 10 - 15, Update lockOwner() to
catch the readlinkSync(imageLock) error, return undefined only when its code is
ENOENT, and rethrow every other failure so withImageLock can report
lock-resource errors instead of retrying indefinitely.
Source: Coding guidelines
| // concurrent clones of one image race, and so does swapping the image out under a clone; macOS has no flock(1) | ||
| async withImageLock<T>(fn: () => Promise<T>): Promise<T> { | ||
| for (;;) { | ||
| try { | ||
| mkdirSync(cloneLock); | ||
| symlinkSync(String(process.pid), imageLock); | ||
| break; | ||
| } catch { | ||
| if (waited >= 120) { | ||
| rmSync(cloneLock, { recursive: true, force: true }); | ||
| waited = 0; | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; | ||
| const owner = lockOwner(); | ||
| if (owner !== undefined && !processAlive(owner)) { | ||
| console.log(`${imageLock} held by dead pid ${owner}; removing`); | ||
| rmSync(imageLock, { force: true }); | ||
| continue; | ||
| } | ||
| await sleep(1000); | ||
| } | ||
| } | ||
| try { | ||
| await tart.clone(from, to); | ||
| return await fn(); | ||
| } finally { | ||
| rmdirSync(cloneLock); | ||
| if (lockOwner() === process.pid) unlinkSync(imageLock); | ||
| } | ||
| }, | ||
|
|
||
| cloneLocked: (from: string, to: string) => tart.withImageLock(() => tart.clone(from, to)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add automated regression tests for image locking and promotion.
This change adds new lock ownership, stale-lock recovery, promotion, and rollback behavior. No automated regression test accompanies these changes.
scripts/darwin-ci/lib/tart.ts#L67-L91: Test concurrent acquisition, dead-owner recovery, callback failure cleanup, and preservation of a live owner lock.scripts/darwin-ci/lib/bake.ts#L53-L68: Test successful promotion and restoration of the previous image when staging rename fails.
As per coding guidelines: “Every behavioral change must include an automated regression test in the same change.”
📍 Affects 2 files
scripts/darwin-ci/lib/tart.ts#L67-L91(this comment)scripts/darwin-ci/lib/bake.ts#L53-L68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/lib/tart.ts` around lines 67 - 91, Add automated regression
tests for scripts/darwin-ci/lib/tart.ts lines 67-91 covering concurrent
acquisition, dead-owner recovery, callback failure cleanup, and preservation of
a live owner lock; test scripts/darwin-ci/lib/bake.ts lines 53-68 for successful
promotion and restoration of the previous image when staging rename fails. Use
the existing test patterns and isolate filesystem/process behavior with
appropriate mocks or temporary fixtures.
Source: Coding guidelines
| function positiveInteger(name: "release" | "spawn"): number { | ||
| const value = values[name]!; | ||
| if (!/^[1-9]\d*$/.test(value)) fail(`--${name} must be a positive integer, got ${JSON.stringify(value)}`); | ||
| return Number(value); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target files ---'
git ls-files 'scripts/darwin-ci/*' 'test*' | sed -n '1,120p'
printf '%s\n' '--- outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline scripts/darwin-ci/main.ts --view compact || true
fi
printf '%s\n' '--- relevant source ---'
nl -ba scripts/darwin-ci/main.ts | sed -n '1,180p'
printf '%s\n' '--- symbol and flag usages ---'
rg -n --glob '!node_modules' --glob '!build' 'positiveInteger|values\.release|values\.spawn|--release|--spawn|tart|BUILDKITE_AGENT' scripts test tests 2>/dev/null | sed -n '1,240p'Repository: oven-sh/bun
Length of output: 26857
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- main.ts ---'
cat -n scripts/darwin-ci/main.ts | sed -n '1,180p'
printf '%s\n' '--- agent.ts ---'
cat -n scripts/darwin-ci/lib/agent.ts | sed -n '1,155p'
printf '%s\n' '--- README relevant sections ---'
cat -n scripts/darwin-ci/README.md | sed -n '1,90p'
printf '%s\n' '--- bake.ts ---'
cat -n scripts/darwin-ci/lib/bake.ts | sed -n '1,100p'
printf '%s\n' '--- direct usages ---'
rg -n 'positiveInteger|release|spawn' scripts/darwin-ci -g '*.ts' -g '*.mjs' -g '*.md' | sed -n '1,220p'Repository: oven-sh/bun
Length of output: 22540
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- JavaScript numeric conversion probe ---'
node - <<'JS'
const values = [
"1",
"9007199254740991",
"9007199254740992",
"9007199254740993",
"99999999999999999999999999999999999999999999999999",
];
for (const value of values) {
const parsed = Number(value);
console.log(JSON.stringify({
value,
parsed: String(parsed),
isFinite: Number.isFinite(parsed),
isInteger: Number.isInteger(parsed),
isSafeInteger: Number.isSafeInteger(parsed),
roundTrip: String(parsed) === value,
}));
}
JS
printf '%s\n' '--- release tier and config ---'
cat -n scripts/darwin-ci/lib/release-tier.mjs
cat -n scripts/darwin-ci/lib/config.ts
printf '%s\n' '--- possible dedicated coverage ---'
rg -n 'darwin-ci|install-agent|positive integer|safe integer|release-tier|TartAgentOptions' --glob '*.test.*' --glob '*.ts' --glob '*.mjs' --glob '*.md' . | sed -n '1,220p'Repository: oven-sh/bun
Length of output: 9059
Reject values outside the safe integer range.
Number(value) can round values such as 9007199254740993 or return Infinity before writing the Buildkite configuration. Use Number.isSafeInteger(parsed) after conversion. Add regression cases for 9007199254740992, 9007199254740993, and an overflowing decimal string.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/darwin-ci/main.ts` around lines 64 - 67, Update positiveInteger to
store Number(value) in a parsed variable and reject it with
Number.isSafeInteger(parsed) after the existing format validation, preserving
the positive-integer error behavior for unsafe or overflowing values. Add
regression coverage for 9007199254740992, 9007199254740993, and an overflowing
decimal string.
Source: Coding guidelines
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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}`); | ||
| }); | ||
| } | ||
| process.exit(failed ? 1 : 0); |
There was a problem hiding this comment.
🟡 The orphan-reap loop sets the same failed flag as the own-guest destroy, so a failure deleting some other job's leftover VM makes pre-exit exit 1 — e.g. with spawn=2, two concurrent pre-exits both see the same >10-min orphan, both call strict tart.remove, and the loser gets "does not exist" → failed = true. Orphan reaping is best-effort auxiliary cleanup (the nightly reboot daemon already backstops it); only a failure to destroy own should influence the exit code — log-and-continue for the orphan loop.
Extended reasoning...
What changed
Before this PR, tart.remove was .nothrow() and pre-exit.ts fell off the end with an implicit exit 0. Now tart.remove is strict run([bin, 'delete', vm]) (throws on nonzero), reap() catches any throw and sets a shared failed = true, and the script ends with process.exit(failed ? 1 : 0). The same reap() wrapper is used for two very different things:
- Destroying this job's own guest (
own = bk-${BUILDKITE_JOB_ID}) - Opportunistically deleting other jobs'
bk-*directories under~/.tart/vmsthat are >10 minutes old with notart runprocess
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 trigger
With spawn=2 (the config default in lib/config.ts), two agent workers run on the same host and their pre-exit hooks can execute concurrently. Suppose an earlier job died and left bk-OLD behind:
- Job A's pre-exit and Job B's pre-exit both run
readdirSync(~/.tart/vms)and both seebk-OLD(>10 min old, notart runprocess). - Both call
tart.remove('bk-OLD')→run([tart, 'delete', 'bk-OLD']). - Job A wins;
tart deletesucceeds. - Job B's
tart delete bk-OLDexits nonzero ("does not exist" / ENOENT),run()throws,reap()setsfailed = true. - Job B's pre-exit calls
process.exit(1)even though Job B's own tests passed and its own guest was destroyed cleanly.
The same happens for any other reason tart delete might fail on an unrelated leftover (corrupt VM dir, tart lock contention, permissions).
Why nothing prevents it
tart.destroy(own) (used for the job's own guest) does guard with if (!(await tart.exists(vm))) return, but the orphan loop calls tart.remove(name) directly, which is a bare strict run() with no exists-check and no "does not exist" tolerance. And even if it did check first, the check-then-delete would still be racy across two concurrent pre-exits.
Why this is the wrong shape
REVIEW.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 com.buildkite.cleanup daemon in lib/agent.ts that boots out the agent, tart deletes every bk-*, and reboots the host. A missed orphan here is caught within 24h regardless. The PR description itself says pre-exit "logs a guest it cannot delete and keeps reaping" — it does keep reaping, but then exits 1 at the end, which contradicts that intent for orphans.
Fix
Only let the own guest's destroy failure set failed; log and swallow errors from the orphan loop:
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 process.exit(failed ? 1 : 0) entirely, since the command hook already destroyed own and the nightly daemon backstops everything — but keeping it for own is defensible.)
| if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; | ||
| const owner = lockOwner(); | ||
| if (owner !== undefined && !processAlive(owner)) { | ||
| console.log(`${imageLock} held by dead pid ${owner}; removing`); | ||
| rmSync(imageLock, { force: true }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🟡 The stale-lock break has a TOCTOU: two waiters can both read the same dead pid, then B's rmSync lands after A has already re-created the lock with its own live pid — B removes A's lock, both enter fn(), and A's finally (lockOwner() !== process.pid) silently skips cleanup. Re-checking lockOwner() === owner immediately before rmSync (and skipping if not) closes almost all of the window; still an improvement over the old 120s unconditional break, so not blocking.
Extended reasoning...
What the bug is
withImageLock breaks a stale lock by (1) reading the symlink target, (2) checking that pid is dead, then (3) rmSync(imageLock, { force: true }) and retrying symlinkSync. Steps 1–2 and step 3 are not atomic with respect to another process doing the same thing, so two concurrent waiters that both observe the same dead owner can both reach step 3, and one of them will delete the other's freshly created live lock.
Concrete interleaving
With spawn=2, buildkite-agent launches each command hook as its own bun process, so A and B are separate OS processes and the kernel can preempt either between any two syscalls. Suppose a crashed bake (or a host restart) left /tmp/tart-image.lock pointing at a dead pid, and two jobs are dispatched together:
- A:
symlinkSync→ EEXIST;readlinkSync→ dead pid;processAlive→ false - B:
symlinkSync→ EEXIST;readlinkSync→ dead pid;processAlive→ false - A:
rmSync(lock) - A:
symlinkSync(A.pid)— succeeds, A holds the lock and entersfn() - B:
rmSync(lock)— removes A's fresh live lock (force: truemakes ENOENT silent, but here the file exists) - B:
symlinkSync(B.pid)— succeeds, B entersfn() - A and B now both run
tart clone(or A runspromote()'s rename while B is mid-clone) — exactly the race the lock exists to prevent.
When A later reaches its finally, lockOwner() returns B's pid, so the lockOwner() === process.pid guard makes A silently skip unlinkSync — the mutual-exclusion violation is never detected.
Why nothing prevents it
The symlinkSync acquisition itself is atomic (EEXIST correctly serializes contenders), but the break path is check-then-act with no compare-and-swap: rmSync unconditionally removes whatever is at the path, regardless of whether it is still the dead-pid link that was observed. There is no await between the read and the remove, so the window is only a handful of syscalls wide — but A and B are independent processes, and the OS is free to preempt B anywhere in that sequence.
Impact
The PR description states the lock "is broken only when that pid is dead"; this interleaving breaks it while the pid is very much alive. In practice the window is narrow (all synchronous, microsecond-scale), the precondition is rare (a stale lock left by a crashed holder plus two near-simultaneous job starts), and the consequence is a loud tart clone/rename failure that retryBoot or a job retry recovers from — not silent corruption. The old mkdirSync lock's unconditional 120s-timeout break was strictly worse, so this is not a regression.
Suggested fix
Narrow the window with a compare-before-delete — re-read the link and only remove it if it still names the dead pid you observed:
if (owner !== undefined && !processAlive(owner)) {
if (lockOwner() === owner) {
console.log(`${imageLock} held by dead pid ${owner}; removing`);
rmSync(imageLock, { force: true });
}
continue;
}This isn't a true CAS (there's still one syscall of gap between the second readlinkSync and unlink), but it shrinks the vulnerable window from "everything since the first EEXIST" to two adjacent syscalls, and the residual race additionally requires A to have completed both its rmSync and its symlinkSync inside that gap. If you want something fully atomic, macOS does have flock(2) (only flock(1) is missing), so an fs.openSync + flockSync on a plain file would give you a real lock — but that's a larger change than this nit warrants.
| collectReports: (remoteDir: string, localDir: string) => | ||
| $`rsync -a -e ${rsh} --include=*/ --include=*.xml --include=*.junit --exclude=* --prune-empty-dirs ${target}:${remoteDir}/ ${localDir}/` | ||
| .quiet() | ||
| .nothrow(), | ||
| run([ | ||
| "rsync", | ||
| "-a", | ||
| ...rsh, | ||
| "--include=*/", | ||
| "--include=*.xml", | ||
| "--include=*.junit", | ||
| "--exclude=*", | ||
| "--prune-empty-dirs", | ||
| `${target}:${remoteDir}/`, | ||
| `${localDir}/`, |
There was a problem hiding this comment.
🟡 collectReports() lost its .nothrow() and now uses run(), which throws on nonzero exit — but in hooks/command.ts it's awaited after the test status is captured and before it's returned, so an rsync failure discards a passing test status and fails the job. This is the same "must not override the test status" pattern the PR applies to cleanup(); consider giving the collectReports call the same catch-and-log treatment.
Extended reasoning...
What changed
Previously collectReports was:
$`rsync -a -e ${rsh} --include=*/ ... ${target}:${remoteDir}/ ${localDir}/`.quiet().nothrow()The .nothrow() meant a nonzero rsync exit was silently ignored. After this PR it's:
run(["rsync", "-a", ...rsh, "--include=*/", ..., `${target}:${remoteDir}/`, `${localDir}/`])and run() in lib/shell.ts throws new Error("rsync -a -e exited N\n<stderr>") on nonzero exit.
The code path
In hooks/command.ts:
let status = 1;
try {
status = await runInGuest();
} finally {
await cleanup();
}
process.exit(status);
async function runInGuest(): Promise<number> {
...
const status = await g.run(`${jobEnv.join(" ")} /bin/bash ~/job.sh`, forward); // <- test result captured here
console.log("--- :outbox_tray: collect reports");
await g.collectReports("work", checkout); // <- may now throw
return status; // <- never reached if it does
}Step-by-step
g.run(... job.sh ...)completes; suppose the test suite passed andstatus = 0.g.collectReports("work", checkout)runs rsync. Rsync exits nonzero (transient ssh drop, guest OOM-killed sshd, guest already shutting down, disk full on the host, etc.).run()throws.runInGuest()never reachesreturn status.- In the outer scope,
status = await runInGuest()throws before assigning, so the outerstatusstays at its initializer1. - The
try/finallyhas nocatch, sofinallyrunscleanup()(which now swallows its own errors), then the exception re-propagates. process.exit(status)is never reached; the top-level await rejects and Bun exits nonzero.
Result: a green test run is reported to Buildkite as failed.
Why nothing prevents it
The PR explicitly adds this guard to cleanup() — "pre-exit destroys it again and reaps whatever this misses, so a failure here must not override the test status" — with a .catch(e => console.error(...)). collectReports sits on the same post-status path and got no such guard. The PR description notes .nothrow() was removed because it hid the glob-expansion bug, which is a good reason to make the failure visible, but visible-and-logged is different from overrides-the-test-exit-code.
REVIEW.md's error-handling section draws exactly this line: operations the user explicitly requested fail hard, but "best-effort auxiliary steps ... degrade quietly instead of aborting." Report collection is an auxiliary post-step — the job's pass/fail signal is the job.sh exit code, not whether junit files were rsync'd back.
Impact
A spurious red CI job requiring a re-run. The trigger window is admittedly narrow — the guest was reachable over ssh for the entire test run and then rsync over the same transport fails immediately after — so this is a nit, not a blocker.
Fix
Mirror cleanup() at the call site in command.ts:
await g.collectReports("work", checkout).catch((error: unknown) =>
console.error(`failed to collect reports: ${error}`),
);
return status;(or equivalently keep collectReports throwing and wrap it here — the point is that the already-captured status is what gets returned.)
|
Updated 5:06 AM PT - Aug 12th, 2026
✅ @Jarred-Sumner, your commit 88bc38469cb3a3a6cbb14937bb80101533dc44f9 passed in 🧪 To try this PR locally: bunx bun-pr 37771That installs a local version of the PR into your bun-37771 --bun |
### What does this PR do? Homebrew now refuses to load a formula from a tap it has not been told to trust. `host.sh <name> tart` therefore fails at `brew install cirruslabs/cli/tart`: ``` Warning: Skipping cirruslabs/cli because it is not trusted. Run `brew trust cirruslabs/cli` to trust it. Error: Refusing to load formula cirruslabs/cli/softnet from untrusted tap cirruslabs/cli. ``` `brewInstall` now taps and trusts a tap-qualified formula (`org/repo/name`) before installing it. `brew trust` is allowed to fail because older Homebrew has no such command. Formulae without a tap, like `tailscale`, are unchanged. ### How did you verify your code works? Ran `brew trust cirruslabs/cli` by hand on a Mac host that failed with the error above, then re-ran `host.sh`; the tart install went through. This change moves that step into `brewInstall`. I checked that the file parses; the code only runs when provisioning a fresh host, so it has not been run end to end. Related: #37771 also edits `scripts/darwin-ci/lib/host.ts` and conflicts with main. This change is one function and is independent of it.
|
Carried on in #43710: this commit, unchanged, then a merge of main (which moved the CI scripts to TypeScript and to the generated toolchain script) and three small follow-ups. |
…43710) ### What does this PR do? Lands #37771 (Jarred's follow-ups to the review of `scripts/darwin-ci`) on current main. His commit is first and unchanged, re-signed. My commits come after it. - **Jarred's commit**: the scripts use `Bun.spawn` with argv arrays instead of `Bun.$`, plus the bake, hook and CI-user fixes listed in its message. - **Merge of main**: main moved the CI scripts to TypeScript, generates the toolchain script from `scripts/build/ci-images/spec.ts`, added the `beta` release tier and the tap trust in `brewInstall`. The conflicts keep main's behavior in the new style. `lib/release-tier.mjs` is dropped: it existed so `agent.mjs` could share it, and `scripts/agent.ts` now imports only Node. - **Image lock**: a flock(2) held by `/usr/bin/lockf`, released by the kernel when its holder exits. A killed job cannot leave a stale lock. - **Missing executables**: `spawn`/`runInherit` return exit 127 instead of throwing, so provisioning a fresh host works. - **Cleanup**: pre-exit never fails a job; a guest that will not delete is logged. - **Reports**: `collectReports` is removed. The runner uploads its reports from inside the guest, and nothing on the host read the copy (main's `Bun.$` form of it failed on every job, hidden by `.nothrow()`). - **Smaller**: private `sudoWrite`, a clear error if launchd will not unload the agent, the old image deleted under the lock, typecheck fixes, bun 1.4.2 pinned in `host.sh`. Running hosts keep their installed copy in `/usr/local/share/darwin-ci` until they are provisioned again. ### How did you verify your code works? Ran the changed functions on a Mac with bun 1.4: - Missing executable: `spawn` gives exit 127, `probe` gives undefined, `run` throws with exit 127. - Image lock: three holders in one process never overlap; a holder in another process blocks the lock, and after it is killed with `kill -9` the next waiter gets the lock in 67 ms. - `sudoWrite`'s write step creates a new file with mode 600. - `job.env` values with quotes, a newline, `$`, backticks and an empty string round-trip through `source` in bash. - `host.sh`'s own `fetch` checks the bun 1.4.2 zips against the pinned checksums. - `tsc` over `scripts/darwin-ci` with `scripts/tsconfig.json` shows no new errors. Not run: a full provision of a host, or a CI job on a host with these scripts. Supersedes #37771. --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
What does this PR do?
Follow-ups to the review comments on #37633, which added
scripts/darwin-ci(provisioning for the macOStest-darwinagents, where each test job runs in a Tart guest).The scripts now use
Bun.spawnwith argv arrays instead ofBun.$. A host runs them on whatever bunhost.shinstalled when it was provisioned, and the shell's quoting is not something to depend on across releases:$.escapedoes not produce bash-safe strings, so thejob.envthe command hook writes broke on any commit message containing a quote or newline, and literal--include=*/words were glob-expanded, socollectReportsnever ran (and.nothrow()hid it).Review items:
tart.ts: the clone lock is a symlink whose target is the holder's pid; it is broken only when that pid is dead, and released only by its owner.stop/destroytolerate exactly "not running" / nonexistent and propagate everything else;removeis strict.bake.ts: validates the ref before booting, quotes every argument passed into the guest, and swaps the image in under the same lock the hook clones with, keeping the previous image until the rename succeeds.release-tier.mjs: one copy of therelease-tierthresholds, shared withscripts/agent.mjs(whoseinstallcopies it alongside).job.sh: runsBUILDKITE_COMMANDthroughbash -e -o pipefail(sharding already arrives via the forwarded env), shows warm-up install output on failure, exits ifsource/cdfail.bash -e -o pipefail;job.envis 0600 and removed infinally; a cleanup failure no longer masks the test status. Pre-exit logs a guest it cannot delete and keeps reaping.ci-user.ts:createhomedir/dseditgrouponly when needed and no longernothrow; uid 0 rejected.shell.tsoutput()throws; probes useprobe().host.sh: Homebrew installer pinned to a commit and bun to 1.3.14, both sha256-checked.host.tscleans its temp dir infinally; nightly reap usessudo -H;main.tsvalidates--release/--spawn.Not addressed: the ci.mjs x64 note (retracted by the reviewer), the
scripts/tart/finding (that directory does not exist), and the resolve-dns failure (#37668).How did you verify your code works?
Ran the pieces that do not need a Tart host: the
shell.tshelpers (stdin, cwd, quoting round-trip throughset -a; source), the image lock (stale pid broken, live holder waited on, foreign lock left alone),job.shwith a fakejob.env(quoted args with spaces preserved,-ehonoured, exit code propagated), the command hook's host branch, andmain.tsargument validation;tsc --strict --noUnusedLocalsover the tree andprettier --checkare clean.provision,bakeand the image promotion have not been run against a real host.