-
Notifications
You must be signed in to change notification settings - Fork 66
fix(pr-proof): resolve real Cargo executable outside version-manager shims #1624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: fix/1602-authoritative-live-workers
Are you sure you want to change the base?
Changes from all commits
9e2ea3f
a44fd0a
afbeaa5
636d001
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,216 @@ | ||
| // Unit tests for the fail-fast Cargo resolver used by the 1602 PR-proof | ||
| // case runner. | ||
| // | ||
| // Every test drops a real (fake) executable script into a temp directory and | ||
| // exercises the resolver against it. The resolver's timeout is enforced by | ||
| // Node's execFileSync `timeout` + `killSignal: 'SIGKILL'`, so a probe that | ||
| // blocks forever is guaranteed to die within the budget. | ||
| // | ||
| // Run with: | ||
| // node --test tests/relayflows/cases/1602-parentless-worker-inventory/resolve-cargo.test.mjs | ||
|
|
||
| import test from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { chmod, mkdir, mkdtemp, realpath, rm, writeFile } from 'node:fs/promises'; | ||
| import os from 'node:os'; | ||
| import path from 'node:path'; | ||
|
|
||
| import { CargoNotResolvableError, resolveCargo } from './run.mjs'; | ||
|
|
||
| async function makeExecutable(filePath, body) { | ||
| await mkdir(path.dirname(filePath), { recursive: true }); | ||
| await writeFile(filePath, body); | ||
| await chmod(filePath, 0o755); | ||
| } | ||
|
|
||
| async function makeFakeCargo(filePath, { version = '1.75.0' } = {}) { | ||
| await makeExecutable( | ||
| filePath, | ||
| `#!/bin/sh\nif [ "$1" = "--version" ]; then\n echo "cargo ${version} (fake)"\nfi\n` | ||
| ); | ||
| } | ||
|
|
||
| async function makeHangingCargo(filePath) { | ||
| // Ignore SIGTERM so a hung probe survives a soft signal; only SIGKILL takes | ||
| // it out. Sleep in a small loop so `trap` can service the signal ignore. | ||
| await makeExecutable( | ||
| filePath, | ||
| '#!/bin/sh\ntrap "" TERM\nwhile true; do sleep 60; done\n' | ||
| ); | ||
| } | ||
|
|
||
| async function makeWrongOutputCargo(filePath) { | ||
| await makeExecutable(filePath, '#!/bin/sh\necho "not cargo at all"\n'); | ||
| } | ||
|
|
||
| async function makeTmpRoot(prefix) { | ||
| const raw = await mkdtemp(path.join(os.tmpdir(), prefix)); | ||
| // macOS resolves /var to /private/var — normalize so string comparisons | ||
| // against paths inside the returned root line up. | ||
| return realpath(raw); | ||
| } | ||
|
|
||
| test('resolveCargo returns a direct hit on PATH when `cargo --version` looks right', async () => { | ||
| const root = await makeTmpRoot('resolve-cargo-path-'); | ||
| try { | ||
| const binDir = path.join(root, 'bin'); | ||
| const cargo = path.join(binDir, 'cargo'); | ||
| await makeFakeCargo(cargo); | ||
|
|
||
| const resolved = resolveCargo({ | ||
| env: { PATH: binDir }, | ||
| pathEntries: [binDir], | ||
| extraSystemPaths: [], | ||
| }); | ||
| assert.equal(resolved, cargo); | ||
| } finally { | ||
| await rm(root, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| test('resolveCargo prefers CARGO_HOME/bin/cargo when it works', async () => { | ||
| const root = await makeTmpRoot('resolve-cargo-cargohome-'); | ||
| try { | ||
| const cargoHome = path.join(root, '.cargo'); | ||
| const cargo = path.join(cargoHome, 'bin', 'cargo'); | ||
| await makeFakeCargo(cargo); | ||
|
|
||
| const resolved = resolveCargo({ | ||
| env: { PATH: '', CARGO_HOME: cargoHome }, | ||
| pathEntries: [], | ||
| extraSystemPaths: [], | ||
| }); | ||
| assert.equal(resolved, cargo); | ||
| } finally { | ||
| await rm(root, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| test('resolveCargo enumerates HOME/.rustup/toolchains/*/bin/cargo', async () => { | ||
| const root = await makeTmpRoot('resolve-cargo-rustup-'); | ||
| try { | ||
| const toolchainCargo = path.join( | ||
| root, | ||
| '.rustup', | ||
| 'toolchains', | ||
| 'stable-x86_64-unknown-linux-gnu', | ||
| 'bin', | ||
| 'cargo' | ||
| ); | ||
| await makeFakeCargo(toolchainCargo); | ||
|
|
||
| const resolved = resolveCargo({ | ||
| env: { PATH: '', HOME: root }, | ||
| pathEntries: [], | ||
| extraSystemPaths: [], | ||
| }); | ||
| assert.equal(resolved, toolchainCargo); | ||
| } finally { | ||
| await rm(root, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| test('resolveCargo falls back to extraSystemPaths when nothing else works', async () => { | ||
| const root = await makeTmpRoot('resolve-cargo-system-'); | ||
| try { | ||
| const systemCargo = path.join(root, 'opt', 'cargo', 'bin', 'cargo'); | ||
| await makeFakeCargo(systemCargo); | ||
|
|
||
| const resolved = resolveCargo({ | ||
| env: { PATH: '' }, | ||
| pathEntries: [], | ||
| extraSystemPaths: [systemCargo], | ||
| }); | ||
| assert.equal(resolved, systemCargo); | ||
| } finally { | ||
| await rm(root, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| test('resolveCargo skips a candidate that prints unexpected --version output and picks the next one', async () => { | ||
| const root = await makeTmpRoot('resolve-cargo-badoutput-'); | ||
| try { | ||
| // First candidate on PATH is a shim that prints garbage. | ||
| const badDir = path.join(root, 'bad', 'bin'); | ||
| const badCargo = path.join(badDir, 'cargo'); | ||
| await makeWrongOutputCargo(badCargo); | ||
|
|
||
| // Second candidate on PATH is the real thing. | ||
| const goodDir = path.join(root, 'good', 'bin'); | ||
| const goodCargo = path.join(goodDir, 'cargo'); | ||
| await makeFakeCargo(goodCargo); | ||
|
|
||
| const resolved = resolveCargo({ | ||
| env: { PATH: `${badDir}${path.delimiter}${goodDir}` }, | ||
| pathEntries: [badDir, goodDir], | ||
| extraSystemPaths: [], | ||
| }); | ||
| assert.equal(resolved, goodCargo); | ||
| } finally { | ||
| await rm(root, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| test( | ||
| 'resolveCargo throws CargoNotResolvableError when every candidate hangs past the timeout', | ||
| { timeout: 15_000 }, | ||
| async () => { | ||
| const root = await makeTmpRoot('resolve-cargo-hung-'); | ||
| try { | ||
| const hungA = path.join(root, 'a', 'cargo'); | ||
| const hungB = path.join(root, 'b', 'cargo'); | ||
| await makeHangingCargo(hungA); | ||
| await makeHangingCargo(hungB); | ||
|
|
||
| const timeoutMs = 400; | ||
| const started = Date.now(); | ||
| let threw; | ||
| try { | ||
| resolveCargo({ | ||
| env: { PATH: `${path.dirname(hungA)}${path.delimiter}${path.dirname(hungB)}` }, | ||
| pathEntries: [path.dirname(hungA), path.dirname(hungB)], | ||
| timeoutMs, | ||
| extraSystemPaths: [], | ||
| }); | ||
| } catch (error) { | ||
| threw = error; | ||
| } | ||
| const elapsed = Date.now() - started; | ||
|
|
||
| assert.ok(threw instanceof CargoNotResolvableError, `expected CargoNotResolvableError, got ${threw}`); | ||
| assert.equal(threw.attempts.length, 2, 'both hung candidates must be recorded'); | ||
| for (const attempt of threw.attempts) { | ||
| assert.match(attempt.reason, /killed by SIGKILL after 400ms/); | ||
| } | ||
| assert.match(threw.message, /could not resolve a working cargo executable/); | ||
| // Two 400ms probes + SIGKILL should be under 5s even on a busy runner. | ||
| assert.ok(elapsed < 5000, `probes must respect timeout budget, elapsed=${elapsed}ms`); | ||
| } finally { | ||
| await rm(root, { recursive: true, force: true }); | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| test('resolveCargo dedupes identical candidates across enumeration sources', async () => { | ||
| const root = await makeTmpRoot('resolve-cargo-dedupe-'); | ||
| try { | ||
| const cargoHome = path.join(root, '.cargo'); | ||
| const cargo = path.join(cargoHome, 'bin', 'cargo'); | ||
| await makeFakeCargo(cargo); | ||
|
|
||
| // CARGO_HOME/bin/cargo, HOME/.cargo/bin/cargo, and the PATH-based candidate | ||
| // all resolve to the same string. Dedup must not double-execute it. | ||
| const resolved = resolveCargo({ | ||
| env: { | ||
| PATH: path.join(cargoHome, 'bin'), | ||
| CARGO_HOME: cargoHome, | ||
| HOME: root, | ||
| }, | ||
| pathEntries: [path.join(cargoHome, 'bin')], | ||
| extraSystemPaths: [cargo], | ||
| }); | ||
| assert.equal(resolved, cargo); | ||
| } finally { | ||
| await rm(root, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| import { spawn } from 'node:child_process'; | ||
| import { constants as fsConstants } from 'node:fs'; | ||
| import { access, readFile, readdir, realpath, writeFile } from 'node:fs/promises'; | ||
| import { execFileSync, spawn } from 'node:child_process'; | ||
| import { accessSync, constants as fsConstants, readdirSync } from 'node:fs'; | ||
| import { readFile, writeFile } from 'node:fs/promises'; | ||
| import path from 'node:path'; | ||
| import { pathToFileURL } from 'node:url'; | ||
|
|
||
| const CASE_ID = '1602-parentless-worker-inventory'; | ||
| const MARKER = 'RELAY_PR_PROOF_OBSERVATION='; | ||
|
|
@@ -37,48 +38,96 @@ function proofChildEnvironment() { | |
| return env; | ||
| } | ||
|
|
||
| async function isExecutable(filePath) { | ||
| try { | ||
| await access(filePath, fsConstants.X_OK); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| export class CargoNotResolvableError extends Error { | ||
| constructor(attempts) { | ||
| const detail = attempts | ||
| .map((a) => `${a.path} (${a.reason})`) | ||
| .join('; '); | ||
| super( | ||
| `could not resolve a working cargo executable; attempted: ${detail || '(no candidates)'}` | ||
| ); | ||
| this.name = 'CargoNotResolvableError'; | ||
| this.attempts = attempts; | ||
| } | ||
| } | ||
|
|
||
| async function resolveCargo() { | ||
| const pathEntries = (process.env.PATH ?? '').split(path.delimiter).filter(Boolean); | ||
| for (const entry of pathEntries) { | ||
| const candidate = path.join(entry, 'cargo'); | ||
| if (!(await isExecutable(candidate))) continue; | ||
| const resolved = await realpath(candidate).catch(() => candidate); | ||
| if (path.basename(resolved) === 'cargo') return resolved; | ||
| } | ||
| // Ground truth: a candidate is a real cargo iff `cargo --version` prints | ||
| // `cargo <semver>` within a bounded budget. Node's execFileSync `timeout` + | ||
| // `killSignal: 'SIGKILL'` is the OS calling kill(9), so a hung/blocking child | ||
| // can't out-live the probe — no sentinel race, no SIGTERM→SIGKILL escalation, | ||
| // no shim-vs-real heuristics. | ||
| export function resolveCargo(options = {}) { | ||
| const { | ||
| env = process.env, | ||
| pathEntries = (env.PATH ?? '').split(path.delimiter).filter(Boolean), | ||
| timeoutMs = 3000, | ||
| extraSystemPaths = [ | ||
| '/usr/local/bin/cargo', | ||
| '/opt/homebrew/bin/cargo', | ||
| '/root/.cargo/bin/cargo', | ||
| '/home/daytona/.cargo/bin/cargo', | ||
| ], | ||
| } = options; | ||
|
|
||
| const candidates = []; | ||
| const seen = new Set(); | ||
| const add = (candidate) => { | ||
| if (!candidate || seen.has(candidate)) return; | ||
| seen.add(candidate); | ||
| candidates.push(candidate); | ||
| }; | ||
|
|
||
| const homes = new Set(); | ||
| for (const entry of pathEntries) { | ||
| const normalized = entry.replaceAll('\\', '/'); | ||
| for (const suffix of ['/.local/share/mise/shims', '/.cargo/bin']) { | ||
| if (normalized.endsWith(suffix)) homes.add(normalized.slice(0, -suffix.length)); | ||
| if (env.CARGO_HOME) add(path.join(env.CARGO_HOME, 'bin', 'cargo')); | ||
| if (env.HOME) add(path.join(env.HOME, '.cargo', 'bin', 'cargo')); | ||
| if (env.HOME) { | ||
| const toolchainsDir = path.join(env.HOME, '.rustup', 'toolchains'); | ||
| let entries = []; | ||
| try { | ||
| entries = readdirSync(toolchainsDir); | ||
| } catch { | ||
| // no toolchains directory | ||
| } | ||
| for (const entry of entries.slice().sort().reverse()) { | ||
| add(path.join(toolchainsDir, entry, 'bin', 'cargo')); | ||
| } | ||
| } | ||
| if (process.env.RUSTUP_HOME) { | ||
| homes.add(path.dirname(path.resolve(process.env.RUSTUP_HOME))); | ||
| } | ||
|
|
||
| for (const home of homes) { | ||
| const toolchains = path.join(home, '.rustup', 'toolchains'); | ||
| const entries = await readdir(toolchains).catch(() => []); | ||
| for (const entry of entries.sort().reverse()) { | ||
| const candidate = path.join(toolchains, entry, 'bin', 'cargo'); | ||
| if (await isExecutable(candidate)) return candidate; | ||
| for (const entry of pathEntries) add(path.join(entry, 'cargo')); | ||
| for (const candidate of extraSystemPaths) add(candidate); | ||
|
|
||
| const attempts = []; | ||
| for (const candidate of candidates) { | ||
| try { | ||
| accessSync(candidate, fsConstants.X_OK); | ||
| } catch { | ||
| attempts.push({ path: candidate, reason: 'missing or not executable' }); | ||
| continue; | ||
| } | ||
| let stdout; | ||
| try { | ||
| stdout = execFileSync(candidate, ['--version'], { | ||
| timeout: timeoutMs, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The timeout only kills the process that execFileSync launched directly; any child that a version-manager shim spawned (the exact hung-shim-on-network/lock scenario this PR targets) survives SIGKILL as an orphan and keeps running after resolveCargo returns. The comment's claim that "a hung/blocking child can't out-live the probe" is only true for the direct child, not its descendants. If the orphan holds the cargo home lock prompting the hang, later builds can still block. Spawn with Prompt for AI agents |
||
| killSignal: 'SIGKILL', | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| encoding: 'utf8', | ||
| }); | ||
| } catch (error) { | ||
| const signal = error?.signal; | ||
| const status = error?.status; | ||
| const reason = signal | ||
| ? `killed by ${signal} after ${timeoutMs}ms` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: When a candidate dies from a signal for any reason (not just the timeout), the diagnostic reports it as a timeout: Prompt for AI agents |
||
| : `exit ${status ?? 'unknown'}${error?.code ? ` (${error.code})` : ''}`; | ||
| attempts.push({ path: candidate, reason }); | ||
| continue; | ||
| } | ||
| if (!/^cargo \d+\.\d+\.\d+/.test(stdout)) { | ||
| const preview = (stdout || '').slice(0, 80).replace(/\n/g, '\\n'); | ||
| attempts.push({ path: candidate, reason: `unexpected --version output: ${preview}` }); | ||
| continue; | ||
| } | ||
| return candidate; | ||
| } | ||
|
|
||
| for (const candidate of ['/usr/local/cargo/bin/cargo', '/opt/rust/bin/cargo']) { | ||
| if (await isExecutable(candidate)) return candidate; | ||
| } | ||
| throw new Error('could not resolve a real Cargo executable outside a version-manager shim'); | ||
| throw new CargoNotResolvableError(attempts); | ||
| } | ||
|
|
||
| function run(command, args, options = {}) { | ||
|
|
@@ -479,7 +528,9 @@ async function main() { | |
| ); | ||
| } | ||
|
|
||
| main().catch((error) => { | ||
| console.error(error instanceof Error ? error.message : String(error)); | ||
| process.exitCode = 1; | ||
| }); | ||
| if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { | ||
| main().catch((error) => { | ||
| console.error(error instanceof Error ? error.message : String(error)); | ||
| process.exitCode = 1; | ||
| }); | ||
| } | ||
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.
P3: The dedupe test's stated intent (that the identical candidate is not executed twice) is not verified. The test only asserts the returned path equals
cargo; if theseendedup regressed, the candidate would still be probed and returned with the same value, so the test would pass and would not act as a regression fence for the dedup it claims to cover. Record execution count (e.g., have the fake cargo append to a marker file and assert it ran exactly once) or soften the comment.Prompt for AI agents