diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..831cf8b --- /dev/null +++ b/.gitattributes @@ -0,0 +1,4 @@ +# program.md is a frozen contract copied verbatim into the user's repository, +# and init.test.js parses it. Pin it to LF so a Windows checkout does not turn +# every paragraph boundary into CRLF under the test's feet. +templates/program.md text eol=lf diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1797a1a..b3def96 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,7 +10,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, macos-latest] + os: [ubuntu-latest, macos-latest, windows-latest] node: ['20', '22'] runs-on: ${{ matrix.os }} steps: diff --git a/README.md b/README.md index 0411974..c0308c4 100644 --- a/README.md +++ b/README.md @@ -173,17 +173,21 @@ Three ways to end a run, in increasing order of force: under way finishes, is measured and scored normally; the agent sees `"stop_requested": true` on that verdict, applies it as usual (`KEEP` stays, anything else resets), then exits the loop. Nothing is thrown away. -2. **`autor3search-javascript stop --force`** — writes the same request, then - sends `SIGTERM` to the running `eval`, which tears down its own child - process groups so no Vitest worker is left burning CPU. The agent sees - `"status": "ABORTED"`, exit code 2, and no `results.tsv` row — nothing was - measured, so nothing was recorded. `stop --force` reports what HEAD looks - like afterward; it does not touch the repository for you. +2. **`autor3search-javascript stop --force`** — writes a sticky forced-stop + marker. The running `eval` polls for it and cancels itself, and that + cancellation is what tears down the benchmark process tree, so no Vitest + worker is left burning CPU. The agent sees `"status": "ABORTED"`, exit + code 2, and no `results.tsv` row — nothing was measured, so nothing was + recorded. The marker is sticky on purpose: if nothing was running when you + forced the stop, it stands and aborts the *next* `eval` to start instead, + until you clear it with `stop --clear`. `stop --force` reports what HEAD + looks like afterward; it does not touch the repository for you. 3. **Ctrl+C** — the same abort path as `stop --force`, sent directly to a foreground `eval`. -`autor3search-javascript stop --clear` cancels a pending stop request so the -loop continues — that is your decision, never the agent's to make on its own. +`autor3search-javascript stop --clear` cancels both a pending stop request and +a pending forced stop, so the loop continues — that is your decision, never +the agent's to make on its own. ## Commands @@ -382,19 +386,26 @@ Two things to know if you are wiring this up on a fork or a new package: |---|---| | **Linux** | supported — CI runs the full suite on Node 20 and 22 | | **macOS** | supported — CI runs the full suite on Node 20 and 22 | -| **Windows** | **not supported** | - -On Windows nearly the whole suite passes, so measurement itself works. What does not -work is stopping: Node cannot deliver SIGINT to a child process group there -the way it does on POSIX, so an interrupted `eval` never reaches the `ABORTED` -path — it exits with a null code instead of 2 and can leave its claim behind. -An unattended harness that cannot be reliably stopped is not something to be -quiet about, so `doctor` says so on Windows rather than letting you find out -at 3am. Two further failures are the test suite's own POSIX assumptions -(a `chmod`-unreadable directory, and path separators) rather than product bugs. - -WSL reports as Linux and is unaffected. Node 20 or newer is required -everywhere. +| **Windows** | supported — CI runs the full suite on Node 20 and 22 | + +All three behave identically, by design. `stop` and `stop --force` write marker +files into the run's state directory rather than signalling a process: the +running `eval` polls for a forced stop and cancels itself, and that cancellation +is what tears the benchmark process tree down. A signal-based forced stop could +not behave the same way everywhere — Windows has no process-to-process SIGTERM, +so `eval` would be terminated outright and would never record what it abandoned. +Ctrl+C still works everywhere, because a console interrupt is real on Windows +too. + +Three small things differ on Windows, none of them behavioural. `doctor` has no +load average to read there, so it makes one fewer check and says so rather than +reporting the fabricated zero the platform hands it. The state directory's +ownership and permission check is skipped, because Windows reports synthetic +mode bits and has no owner to compare against. And one test is skipped — the +one that makes a directory unlistable with `chmod`, which has no Windows +analogue. + +WSL reports as Linux and is unaffected. Node 20 or newer is required everywhere. ## Limitations diff --git a/package-lock.json b/package-lock.json index 1b16f6d..9405a71 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@autor3search/javascript", - "version": "0.2.1", + "version": "0.2.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@autor3search/javascript", - "version": "0.2.1", + "version": "0.2.2", "license": "MIT", "dependencies": { "@babel/parser": "^7.25.0", diff --git a/package.json b/package.json index b80c585..2a5354d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@autor3search/javascript", - "version": "0.2.1", + "version": "0.2.2", "description": "Autonomous AI-driven performance optimization for any JavaScript repository", "type": "module", "license": "MIT", diff --git a/src/cli/cmd-eval.js b/src/cli/cmd-eval.js index 4628d6b..f9dedd1 100644 --- a/src/cli/cmd-eval.js +++ b/src/cli/cmd-eval.js @@ -16,10 +16,13 @@ import { RUN_LOG_NAME, evalOnce } from '../pipeline.js' import { RESULTS_PATH, appendRow, loadRows } from '../results.js' import { BASELINE_FILE, WORKTREE_NAME, loadBaseline } from '../state/index.js' import { claimEval } from '../state/lock.js' -import { stopRequested } from '../state/stop.js' +import { forceRequested, stopRequested } from '../state/stop.js' import { REASON, STATUS, exitCode } from '../verdict.js' import { expandSingleDashFlags, loadRepoConfig, resolveRun } from './context.js' +/** How often a running eval looks for a forced-stop marker. */ +const FORCE_POLL_MS = 500 + /** * @param {string[]} args * @param {{out: {write(s: string): void}, err: {write(s: string): void}}} io @@ -72,9 +75,23 @@ export async function runEval(args, io) { const onSignal = () => controller.abort() process.on('SIGINT', onSignal) process.on('SIGTERM', onSignal) - const offSignals = () => { + // The forced stop is a polled marker, not a signal — see state/stop.js for + // why. It is installed here, beside the handlers and BEFORE claimEval, for + // exactly the reason spelled out above: a marker already pending when this + // run starts must abort it at its first checkpoint, not after it has claimed + // the run and begun measuring. + const poll = setInterval(() => { + forceRequested(run.stateDir) + .then((forced) => { + if (forced) controller.abort() + }) + .catch(() => {}) + }, FORCE_POLL_MS) + poll.unref() + const detach = () => { process.off('SIGINT', onSignal) process.off('SIGTERM', onSignal) + clearInterval(poll) } let claim @@ -83,7 +100,7 @@ export async function runEval(args, io) { } catch (err) { // dispatch() also runs in-process under test, so a leaked listener here // would accumulate across runs rather than dying with the process. - offSignals() + detach() io.err.write(`${err.message}\n`) return 2 } @@ -128,11 +145,21 @@ export async function runEval(args, io) { if (controller.signal.aborted) { // ABORTED is not a verdict: nothing was measured, so NO results.tsv row // is written. The agent treats it as it would a FAIL. + // + // A sticky forced stop aborts every later eval on this tag too, so an + // abort that finds the marker still pending must say so and name the + // cure. It goes in `message` — which writeJson already emits and the + // human path already writes to io.err — rather than in a new line of + // output, because --json's contract is that stdout carries the verdict + // object and nothing else. + const forced = await forceRequested(run.stateDir).catch(() => false) const aborted = { status: STATUS.ABORTED, reason: REASON.STOP_FORCED, score: 0, - message: 'the experiment was interrupted before it could be measured', + message: forced + ? 'the experiment was abandoned: a forced stop is pending for this tag — clear it with `stop --clear` before the next eval' + : 'the experiment was interrupted before it could be measured', regressions: [], warnings: [], } @@ -149,7 +176,7 @@ export async function runEval(args, io) { } throw err } finally { - offSignals() + detach() await closeLog(logStream) await claim.release() } diff --git a/src/cli/cmd-status.js b/src/cli/cmd-status.js index 5412af6..bd076b3 100644 --- a/src/cli/cmd-status.js +++ b/src/cli/cmd-status.js @@ -8,7 +8,7 @@ import * as gitx from '../gitx.js' import { RESULTS_PATH, loadRows } from '../results.js' import { BASELINE_FILE, WORKTREE_NAME, loadBaseline } from '../state/index.js' import { evalRunning } from '../state/lock.js' -import { stopRequested } from '../state/stop.js' +import { forceRequested, stopRequested } from '../state/stop.js' import { expandSingleDashFlags, resolveRun } from './context.js' /** @@ -42,6 +42,7 @@ export async function runStatus(args, io) { evalLine = `unknown (${err.message})` } const stopped = await stopRequested(run.stateDir) + const forced = await forceRequested(run.stateDir) const field = (name, value) => io.out.write(`${name.padEnd(14)} ${value}\n`) field('run tag', run.tag) @@ -60,7 +61,14 @@ export async function runStatus(args, io) { `${counts.crash} crash) — next is #${rows.length + 1}`, ) field('eval', evalLine) - field('stop', stopped ? 'requested — the agent will end the run after the current experiment' : 'not requested') + field( + 'stop', + forced + ? 'FORCED — the next eval will abort at once; clear it with `autor3search-javascript stop --clear`' + : stopped + ? 'requested — the agent will end the run after the current experiment' + : 'not requested', + ) io.out.write('\nto stop after the current experiment: autor3search-javascript stop\n') io.out.write('to stop now, abandoning it: autor3search-javascript stop --force\n') diff --git a/src/cli/cmd-stop.js b/src/cli/cmd-stop.js index 752e969..a4ec500 100644 --- a/src/cli/cmd-stop.js +++ b/src/cli/cmd-stop.js @@ -6,15 +6,15 @@ * and only then does the loop exit. Nothing is thrown away. * * `--force` is for when a long benchmark cannot be waited out. It writes the - * same request, then signals the running eval to abandon the experiment. It - * REPORTS what state that leaves the repository in; it does not drop anything - * for you, because deciding what to do with a half-finished experiment is the - * human's call. + * same request, then marks the run for the running eval to poll and abandon + * the experiment on its own. It REPORTS what state that leaves the repository + * in; it does not drop anything for you, because deciding what to do with a + * half-finished experiment is the human's call. */ import { parseArgs } from 'node:util' import * as gitx from '../gitx.js' import { evalRunning } from '../state/lock.js' -import { clearStop, requestStop } from '../state/stop.js' +import { clearStop, forceRequested, requestForceStop, requestStop } from '../state/stop.js' import { expandSingleDashFlags, resolveRun } from './context.js' /** @@ -50,35 +50,44 @@ export async function runStop(args, io) { await requestStop(run.stateDir) io.out.write(`stop requested for ${run.tag}: the agent will end the run after the current experiment\n`) if (!values.force) { - io.out.write('the experiment under way will still be measured, scored and applied\n') + // A plain `stop` only promises the current experiment survives when + // nothing is already asking to abandon it. A forced stop from EARLIER — + // `stop --force` issued when nothing was running, then never cleared — + // is still pending and sticky, and it will abort the next eval to start + // regardless of what this plain `stop` just requested. + if (await forceRequested(run.stateDir)) { + io.out.write('a forced stop from earlier is still pending, though, and will abort the next eval to start\n') + } else { + io.out.write('the experiment under way will still be measured, scored and applied\n') + } io.out.write(`to cancel: autor3search-javascript stop -tag ${run.tag} --clear\n`) return 0 } - // A corrupt pid file must not crash `stop --force` — it is refused by - // evalRunning rather than guessed at, but the request above already stands - // regardless, so fall back to "nothing known to signal" and keep going. + await requestForceStop(run.stateDir) + + // Nothing is signalled between processes: the running eval polls this marker + // and cancels itself. That behaves identically on every platform — Windows + // has no process-to-process SIGTERM — and it cannot reach an unrelated + // process the way signalling a pid read from a file can, if the OS has + // recycled that pid since. + // + // evalRunning is therefore reporting, not mechanism. A corrupt pid file + // costs a line of the report and nothing else; the marker already stands. let pid = null let running = false try { ;({ pid, running } = await evalRunning(run.stateDir)) } catch (err) { - io.err.write(`could not read the eval lock (${err.message}); assuming there is nothing to signal\n`) + io.err.write(`could not read the eval lock (${err.message}); cannot say whether an eval is running\n`) } if (running) { - // SIGTERM, not SIGKILL: eval installs a handler that tears down its child - // process groups. Killing it outright would leave Vitest workers running, - // burning CPU and corrupting every later measurement on this machine. - try { - process.kill(pid, 'SIGTERM') - io.out.write(`signalled eval (pid ${pid}) to abandon the current experiment\n`) - } catch (err) { - io.err.write(`could not signal eval (pid ${pid}): ${err.message}\n`) - } + io.out.write(`eval (pid ${pid}) will abandon the current experiment within a second\n`) } else { - io.out.write('no eval is running, so there was nothing to interrupt — the request stands\n') + io.out.write('no eval is running, so there was nothing to interrupt — the forced stop stands and will abort the next eval to start\n') } + io.out.write(`to cancel it: autor3search-javascript stop -tag ${run.tag} --clear\n`) // Report, do not act — whether or not anything was actually interrupted, // this is what the repository looks like right now. The commit an diff --git a/src/doctor.js b/src/doctor.js index a0a067d..ec20824 100644 --- a/src/doctor.js +++ b/src/doctor.js @@ -37,13 +37,7 @@ export async function check(dir) { const absDir = resolvePath(dir) const platformCheck = - platform() === 'darwin' - ? ['power', checkDarwin] - : platform() === 'linux' - ? ['cpu governor', checkLinux] - : platform() === 'win32' - ? ['platform', checkWindows] - : null + platform() === 'darwin' ? ['power', checkDarwin] : platform() === 'linux' ? ['cpu governor', checkLinux] : null const checks = [ ['node', checkNode], @@ -106,6 +100,13 @@ function checkCpu() { } function checkLoad() { + // os.loadavg() is not implemented on Windows: it returns a hardcoded + // [0, 0, 0]. Reporting that would tell a user under heavy load that their + // machine is perfectly idle — worse than saying nothing, in a tool that + // exists to predict whether measurement will be reliable. + if (platform() === 'win32') { + return { name: 'load', detail: 'no load average on this platform — check the machine is quiet yourself', severity: SEVERITY.NA } + } const [one] = loadavg() const perCore = one / Math.max(1, cpus().length) return { @@ -144,28 +145,6 @@ async function checkVitest(dir) { } } -/** - * Windows is not a supported platform, and the reason is the stop path. - * - * 448 of 458 tests pass there, so most of the harness works — but Node cannot - * deliver SIGINT to a child process group on Windows the way it does on POSIX, - * so an interrupted `eval` does not reach the ABORTED path: it exits with a - * null code instead of 2, and the run may leave a claim behind. An unattended - * harness that cannot be reliably stopped is the wrong thing to be quiet about, - * so this says it up front rather than at 3am. - * - * WSL reports linux and is unaffected. - */ -function checkWindows() { - return { - name: 'platform', - detail: - 'Windows is not a supported platform: an interrupted eval does not reach the ABORTED path, so a run ' + - 'may not stop cleanly and can leave its claim behind. Measurement itself works. Use WSL for a supported setup.', - severity: SEVERITY.WARN, - } -} - function checkHeapHint() { // Reported up front so a run does not discover mid-flight that the hint is // unavailable, which would otherwise look like the benchmark allocating diff --git a/src/pipeline.js b/src/pipeline.js index 6832388..10ec407 100644 --- a/src/pipeline.js +++ b/src/pipeline.js @@ -97,6 +97,17 @@ const GATE_VERDICT = { * @returns {Promise<{result: object, measurements: {time: object[], bytes: object[]|null}|null}>} */ export async function evalOnce(o) { + // A sticky forced stop that is already pending when a run starts must not + // cost a full gate run before it is honoured: the scope diff, config hash, + // freeze restore and the gates themselves are all real work, and none of + // it should happen for an experiment that is about to be thrown away + // unmeasured. Checked here, before anything below touches git or the + // filesystem, rather than only after the gates (line 263's neighbour) — + // that later check exists for an abort that arrives WHILE gating is under + // way; this one is for an abort that was already true before evalOnce was + // ever called. + if (o.signal?.aborted) throw new Error('aborted before the experiment began') + const timeoutMs = parseDuration(o.cfg.timeout) // ---- 1. Scope -------------------------------------------------------- diff --git a/src/runner.js b/src/runner.js index 220ce86..61a5f59 100644 --- a/src/runner.js +++ b/src/runner.js @@ -183,6 +183,21 @@ class Capped { * Signals a child's whole process group. A negative pid addresses the group. * Failures are swallowed: the group is already gone in the common case, and a * teardown that throws would mask the real result being reported. + * + * On Windows there are no process groups, so `process.kill(-pid, …)` throws and + * the fallback terminates only the direct child — yet grandchildren still die, + * and runner.test.js proves it on every platform. The reason is libuv: it + * assigns each non-detached child to a global job object with + * JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, so when the direct child dies, its job + * handle closes and everything it spawned goes with it. + * + * That holds only because EVERY subprocess this harness starts is + * process.execPath — see adapters/bench/vitest.js and adapters/gates/*.js — so + * every intermediate is a Node process carrying such a job object. Two changes + * would silently break it and leave vitest's workers running on Windows, + * burning CPU and corrupting every later measurement: running a gate through + * `npm`/`npx` (a .cmd, so cmd.exe becomes the intermediate and there is no job + * object), or adding `detached: true` to an inner spawn. */ function killGroup(pid, signal) { if (!pid) return diff --git a/src/state/lock.js b/src/state/lock.js index aa4b9cb..2d748bf 100644 --- a/src/state/lock.js +++ b/src/state/lock.js @@ -137,15 +137,21 @@ async function readLock(lockPath) { } } // pid <= 1 is refused, not just pid <= 0: kill(1, ...) targets init/launchd - // (every process on the system on some platforms), and kill(-1, ...) - the - // exact form `stop --force` uses for its own valid pids - means "every - // process the caller may signal". A pid file holding either must never be - // read back and handed to process.kill. + // (every process on the system on some platforms), and kill(-1, ...) means + // "every process the caller may signal". Nothing in src/cli/ hands a pid to + // process.kill any more — stop --force writes a marker, not a signal (see + // state/stop.js) — but alive() below still calls process.kill(pid, 0) as a + // liveness probe, and pid 1 or -1 would make that probe ALWAYS succeed. A + // lock is only reclaimed once BOTH the pid is dead and the heartbeat is + // cold (isStale, below); a pid this "alive" check can never call dead would + // leave that lock permanently unreclaimable, refusing every later eval on + // the tag forever. A pid file holding either value must never be read back + // and treated as one this process may trust. if (pid <= 1) { return { pid: null, heartbeat, - error: new Error(`${join(lockPath, 'pid')}: pid ${pid} is not a process this command will signal`), + error: new Error(`${join(lockPath, 'pid')}: pid ${pid} is not a pid this run will trust`), } } return { pid, heartbeat, error: null } diff --git a/src/state/stop.js b/src/state/stop.js index 7660622..4a7896b 100644 --- a/src/state/stop.js +++ b/src/state/stop.js @@ -21,6 +21,26 @@ import { join } from 'node:path' */ export const STOP_REQUEST_FILE = 'stop.request' +/** + * Marks that the human has asked the run to abandon what it is measuring RIGHT + * NOW, rather than after the current experiment. + * + * A marker rather than a signal, because a signal cannot cross processes on + * every platform this harness supports. Windows has no process-to-process + * SIGTERM or SIGINT; Node implements `process.kill` there as TerminateProcess, + * so the target dies where it stands — its `finally` never runs, `eval` exits + * with a null code instead of 2, and its claim is left behind to go stale. + * Polling a file behaves identically everywhere. It also closes a race that + * exists on POSIX, where signalling a pid read from a file can reach an + * unrelated process the OS has since recycled that pid onto. + * + * STICKY, and deliberately so: an unattended agent loop starts a fresh `eval` + * seconds after the last one exits, so a marker consumed by the eval it aborts + * would be gone before the loop's next iteration read it — the human hits the + * brake and the run carries on regardless. Only `clearStop` removes it. + */ +export const STOP_FORCE_FILE = 'stop.force' + /** * Asks the run in stateDir to end after the current experiment. * @@ -34,9 +54,39 @@ export async function requestStop(stateDir) { await writeFile(join(stateDir, STOP_REQUEST_FILE), 'stop requested\n') } -/** Cancels a pending request. Clearing one never made is not an error. */ +/** + * Asks the eval running in stateDir to abandon its experiment at once. + * + * Creates stateDir for the same reason requestStop does: a human reaching for + * the brake should never be told the directory does not exist yet. + */ +export async function requestForceStop(stateDir) { + await ensureSecureDir(stateDir) + await writeFile(join(stateDir, STOP_FORCE_FILE), 'forced stop requested\n') +} + +/** + * Reports whether a forced stop is pending. Boolean rather than throwing, for + * the same reason stopRequested is: an unreadable sentinel and an absent one + * deserve the same answer, and a marker that cannot be read must never abort a + * run by itself. + * + * @returns {Promise} + */ +export async function forceRequested(stateDir) { + return stat(join(stateDir, STOP_FORCE_FILE)).then( + () => true, + () => false, + ) +} + +/** + * Cancels both pending requests — one brake, one release. Clearing one never + * made is not an error. + */ export async function clearStop(stateDir) { await rm(join(stateDir, STOP_REQUEST_FILE), { force: true }) + await rm(join(stateDir, STOP_FORCE_FILE), { force: true }) } /** diff --git a/test/cli/eval.test.js b/test/cli/eval.test.js index cb7234b..4234e87 100644 --- a/test/cli/eval.test.js +++ b/test/cli/eval.test.js @@ -6,12 +6,14 @@ import { fileURLToPath } from 'node:url' import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { STATE_HOME_ENV, stateDir } from '../../src/state/index.js' import { claimEval, evalRunning } from '../../src/state/lock.js' -import { requestStop } from '../../src/state/stop.js' +import { requestForceStop, requestStop } from '../../src/state/stop.js' import { loadRows } from '../../src/results.js' import { runCli } from '../helpers/cli.js' import { FAST_WORDCOUNT, WORDCOUNT_TEST, makeBenchRepo } from '../helpers/bench-repo.js' import { commitFiles, writeFiles } from '../helpers/repo.js' +const posix = typeof process.getuid === 'function' + const original = process.env[STATE_HOME_ENV] beforeEach(async () => { process.env[STATE_HOME_ENV] = await mkdtemp(join(tmpdir(), 'a3s-eval-home-')) @@ -139,11 +141,12 @@ describe('eval', () => { await expect(stat(join(dir, 'run.log'))).rejects.toThrow() }) - it('reports ABORTED on SIGINT: no results row, claim released, stop state honest', async () => { + it('reports ABORTED on a forced stop: no results row, claim released, stop state honest', async () => { // The whole abort contract in one test. Nothing was measured, so nothing // may be recorded — and a stranded claim would block every later eval. // This spawns the REAL binary (rather than runCli's in-process dispatch) - // because a real OS signal has to land on a real process. + // because the marker has to be seen by a separate process's poller, which + // is the whole mechanism under test. const dir = await ready() await commitFiles(dir, { 'src/wordcount.js': FAST_WORDCOUNT }) const bin = fileURLToPath(new URL('../../bin/autor3search-javascript.js', import.meta.url)) @@ -175,10 +178,10 @@ describe('eval', () => { await new Promise((r) => setTimeout(r, 100)) } // Sanity check on the wait itself: if the child had already finished - // before we got to send the signal, everything below would still pass + // before we got to write the marker, everything below would still pass // but for the wrong reason (a completed run, not an aborted one). expect(closed).toBe(false) - child.kill('SIGINT') + await requestForceStop(state) const code = await new Promise((resolve) => child.on('close', resolve)) expect(code).toBe(2) @@ -192,18 +195,19 @@ describe('eval', () => { expect((await evalRunning(state)).running).toBe(false) }) - it('reports ABORTED, not FAIL, when SIGINT lands during the test gate rather than measurement', async () => { + it('reports ABORTED, not FAIL, when a forced stop lands during the test gate rather than measurement', async () => { // The gate phase used to ignore the abort signal entirely: Ctrl+C during // `tsc`/`eslint`/`vitest` would not be seen until measure() started, and // a subprocess killed mid-gate would look like a genuinely failing test // (FAIL, with a results.tsv row) rather than an aborted experiment. This // proves both: the interrupt is now honoured promptly, and it still // produces ABORTED with no row when it hits the slowest gate — the test - // gate — instead of the measurement phase the other SIGINT test covers. + // gate — instead of the measurement phase the POSIX-only SIGINT test + // further below covers. const dir = await makeBenchRepo() await runCli(['init', '-C', dir]) // A frozen test that sleeps well past both this test's patience and the - // point where the other SIGINT test's interrupt already lands (during + // point where the SIGINT test's interrupt already lands (during // measurement) guarantees THIS interrupt instead lands while the test // gate's vitest subprocess is still running. testTimeout is raised so // vitest's own per-test timeout never fires first and masks the point @@ -232,9 +236,10 @@ describe('eval', () => { closed = true }) - // Wait until the claim is held (the run has started), same as the other - // SIGINT test, then give the fast typecheck/lint gates a moment to clear - // so the interrupt below is guaranteed to land on the slow test gate. + // Wait until the claim is held (the run has started), same as the SIGINT + // test further below, then give the fast typecheck/lint gates a moment + // to clear so the interrupt below is guaranteed to land on the slow test + // gate. const state = await stateDir(dir, 't') const claimDeadline = Date.now() + 60_000 while (Date.now() < claimDeadline && !closed) { @@ -245,7 +250,7 @@ describe('eval', () => { await new Promise((r) => setTimeout(r, 1500)) expect(closed).toBe(false) // still inside the 8s test gate, not finished on its own - child.kill('SIGINT') + await requestForceStop(state) const code = await new Promise((resolve) => child.on('close', resolve)) const elapsed = Date.now() - start @@ -260,4 +265,93 @@ describe('eval', () => { expect(await loadRows(join(dir, 'results.tsv'))).toHaveLength(0) expect((await evalRunning(state)).running).toBe(false) }) + + it('honours a forced-stop marker that is already pending before the run starts', async () => { + // Every abort test above interrupts a run already under way. The sticky + // marker's whole point is the opposite case: `stop --force` left the + // marker behind after an earlier eval aborted, and it is STILL there, + // unconsumed, when the next eval starts — spec says that must be caught + // within one poll interval (~500ms), not after a full correctness gate. + // A repo whose test gate takes 8s if ever reached turns "checked late" + // into an unmissably slow test rather than a subtle wrong verdict. + const dir = await makeBenchRepo() + await runCli(['init', '-C', dir]) + await writeFiles(dir, { + 'src/wordcount.test.js': `${WORDCOUNT_TEST}\ntest('slow gate', async () => {\n await new Promise((r) => setTimeout(r, 8000))\n})\n`, + 'vitest.config.js': 'export default { test: { testTimeout: 30000 } }\n', + '.autor3search/config.yaml': 'benchmarks: ["countWords"]\ncount: 4\nscope: ["src/**"]\nheap_hint: false\n', + }) + await commitFiles(dir, {}, 'init') + await runCli(['baseline', '-C', dir, '-tag', 't']) + await commitFiles(dir, { 'src/wordcount.js': FAST_WORDCOUNT }) + + // The marker exists BEFORE eval is even spawned — nothing to interrupt, + // just a sentinel already sitting on disk from an earlier, unrelated + // `stop --force`. + const state = await stateDir(dir, 't') + await requestForceStop(state) + + const bin = fileURLToPath(new URL('../../bin/autor3search-javascript.js', import.meta.url)) + const start = Date.now() + const child = spawn(process.execPath, [bin, 'eval', '-C', dir, '--json', '-desc', 'pre-aborted'], { + env: process.env, + stdio: ['ignore', 'pipe', 'pipe'], + }) + let out = '' + child.stdout.on('data', (chunk) => { + out += chunk + }) + + const code = await new Promise((resolve) => child.on('close', resolve)) + const elapsed = Date.now() - start + + // Generous ceiling, not a tight bound: well under the 8s the test gate + // would cost if the marker were only noticed after claiming and gating, + // with headroom for windows-latest, the slowest runner in the matrix. + expect(elapsed).toBeLessThan(6000) + expect(code).toBe(2) + const payload = JSON.parse(out) + expect(payload.status).toBe('ABORTED') + expect(payload.reason).toBe('stop_forced') + expect(await loadRows(join(dir, 'results.tsv'))).toHaveLength(0) + expect((await evalRunning(state)).running).toBe(false) + }) + + // Windows has no process-to-process signal, so this can only be asserted on + // POSIX. The forced-stop tests above cover the same abort path everywhere; + // this one exists so the signal handler itself does not rot. + it.skipIf(!posix)('reports ABORTED on SIGINT, the path Ctrl+C uses', async () => { + const dir = await ready() + await commitFiles(dir, { 'src/wordcount.js': FAST_WORDCOUNT }) + const bin = fileURLToPath(new URL('../../bin/autor3search-javascript.js', import.meta.url)) + const child = spawn(process.execPath, [bin, 'eval', '-C', dir, '--json', '-desc', 'sigint run'], { + env: process.env, + stdio: ['ignore', 'pipe', 'pipe'], + }) + let out = '' + child.stdout.on('data', (chunk) => { + out += chunk + }) + let closed = false + child.on('close', () => { + closed = true + }) + + const state = await stateDir(dir, 't') + const deadline = Date.now() + 60_000 + while (Date.now() < deadline && !closed) { + if ((await evalRunning(state)).running) break + await new Promise((r) => setTimeout(r, 100)) + } + expect(closed).toBe(false) + child.kill('SIGINT') + + const code = await new Promise((resolve) => child.on('close', resolve)) + expect(code).toBe(2) + const payload = JSON.parse(out) + expect(payload.status).toBe('ABORTED') + expect(payload.reason).toBe('stop_forced') + expect(await loadRows(join(dir, 'results.tsv'))).toHaveLength(0) + expect((await evalRunning(state)).running).toBe(false) + }) }) diff --git a/test/cli/init.test.js b/test/cli/init.test.js index 402c01f..a43b487 100644 --- a/test/cli/init.test.js +++ b/test/cli/init.test.js @@ -114,7 +114,7 @@ describe('program.md stays in sync with the code it documents', () => { // document, so this can assert set equality in both directions without // tripping over unrelated backticked words (file names, config keys, // command names) that appear elsewhere in the file. - const match = text.match(/`reason` is a stable machine-readable code: ([\s\S]*?)\.\n/) + const match = text.match(/`reason` is a stable machine-readable code: ([\s\S]*?)\.\r?\n/) expect(match, 'could not find the reason-code paragraph in program.md').toBeTruthy() const documented = new Set([...match[1].matchAll(/`([a-z_]+)`/g)].map((m) => m[1])) expect(documented).toEqual(new Set(Object.values(REASON))) diff --git a/test/cli/status.test.js b/test/cli/status.test.js index 725c94f..30de74a 100644 --- a/test/cli/status.test.js +++ b/test/cli/status.test.js @@ -5,7 +5,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest' import * as gitx from '../../src/gitx.js' import { STATE_HOME_ENV, stateDir } from '../../src/state/index.js' import { claimEval } from '../../src/state/lock.js' -import { requestStop } from '../../src/state/stop.js' +import { requestForceStop, requestStop } from '../../src/state/stop.js' import { runCli } from '../helpers/cli.js' import { makeBenchRepo } from '../helpers/bench-repo.js' import { commitFiles, writeFiles } from '../helpers/repo.js' @@ -70,6 +70,15 @@ describe('status', () => { await claim.release() }) + it('shows a pending forced stop distinctly from a graceful one', async () => { + const dir = await ready() + const state = await stateDir(dir, 't') + await requestForceStop(state) + const { out } = await runCli(['status', '-C', dir]) + expect(out).toMatch(/FORCED/) + expect(out).toMatch(/stop --clear/) + }) + it('works from another branch when given -tag', async () => { const dir = await ready() await gitx.checkout(dir, 'main') diff --git a/test/cli/stop.test.js b/test/cli/stop.test.js index 38405b0..60840e6 100644 --- a/test/cli/stop.test.js +++ b/test/cli/stop.test.js @@ -3,7 +3,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { STATE_HOME_ENV, stateDir } from '../../src/state/index.js' -import { stopRequested } from '../../src/state/stop.js' +import { forceRequested, stopRequested } from '../../src/state/stop.js' import { runCli } from '../helpers/cli.js' import { makeBenchRepo } from '../helpers/bench-repo.js' import { commitFiles } from '../helpers/repo.js' @@ -48,14 +48,50 @@ describe('stop', () => { const { code, out } = await runCli(['stop', '-C', dir, '--force']) expect(code).toBe(0) expect(out).toMatch(/no eval is running/i) + expect(out).toMatch(/will abort the next eval/i) }) - it('still writes the request when --force finds nothing to signal', async () => { + it('still writes the request when --force finds no eval running to abandon', async () => { const dir = await ready() await runCli(['stop', '-C', dir, '--force']) expect(await stopRequested(await stateDir(dir, 't'))).toBe(true) }) + it('warns a later plain stop that a forced stop from earlier is still pending', async () => { + // stop --force issued while nothing was running leaves the marker sticky + // — it aborts the NEXT eval, not this session's. A human who later runs + // plain `stop` (forgetting --clear) must not be told the current + // experiment will be measured: there may be no current experiment, and + // the marker will still take out whichever eval starts next. + const dir = await ready() + await runCli(['stop', '-C', dir, '--force']) + const { code, out } = await runCli(['stop', '-C', dir]) + expect(code).toBe(0) + expect(out).toMatch(/forced stop from earlier is still pending/i) + expect(out).not.toMatch(/will still be measured/i) + }) + + it('writes the forced marker, not just the graceful request', async () => { + const dir = await ready() + await runCli(['stop', '-C', dir, '--force']) + expect(await forceRequested(await stateDir(dir, 't'))).toBe(true) + }) + + it('leaves no forced marker behind for a graceful stop', async () => { + // The two brakes mean different things: graceful finishes and scores the + // experiment, forced throws it away. + const dir = await ready() + await runCli(['stop', '-C', dir]) + expect(await forceRequested(await stateDir(dir, 't'))).toBe(false) + }) + + it('--clear removes a forced marker too', async () => { + const dir = await ready() + await runCli(['stop', '-C', dir, '--force']) + await runCli(['stop', '-C', dir, '--clear']) + expect(await forceRequested(await stateDir(dir, 't'))).toBe(false) + }) + it('reports the repository state and does not change it', async () => { const dir = await ready() const { out } = await runCli(['stop', '-C', dir, '--force']) diff --git a/test/doctor.test.js b/test/doctor.test.js index bee1223..a732417 100644 --- a/test/doctor.test.js +++ b/test/doctor.test.js @@ -28,10 +28,23 @@ describe('check', () => { expect(named(await check(dir), 'git repo').severity).toBe(SEVERITY.FAIL) }) - it('reports the cpu count and load average', async () => { + it('reports the cpu count, and a load average only where one exists', async () => { const findings = await check(await makeRepo()) expect(named(findings, 'cpu').detail).toMatch(/\d+ logical/) - expect(named(findings, 'load').detail).toMatch(/\d/) + if (process.platform === 'win32') { + // os.loadavg() fabricates [0, 0, 0] on Windows. A tool whose whole job + // is predicting measurement reliability must not report a perfectly idle + // machine it never measured. + expect(named(findings, 'load').severity).toBe(SEVERITY.NA) + expect(named(findings, 'load').detail).toMatch(/no load average/i) + } else { + expect(named(findings, 'load').detail).toMatch(/\d/) + } + }) + + it('does not warn about the platform itself on Windows', async () => { + const findings = await check(await makeRepo()) + expect(findings.some((f) => f.name === 'platform')).toBe(false) }) it('reports free disk space', async () => { diff --git a/test/pipeline.test.js b/test/pipeline.test.js index 37cf941..2948ecd 100644 --- a/test/pipeline.test.js +++ b/test/pipeline.test.js @@ -12,6 +12,8 @@ import { REASON, STATUS } from '../src/verdict.js' import { FAST_WORDCOUNT, makeBenchRepo } from './helpers/bench-repo.js' import { commitFiles, writeFiles } from './helpers/repo.js' +const posix = typeof process.getuid === 'function' + /** Builds a repository with a baseline already recorded, ready for eval. */ async function setup(overrides = {}) { const root = await makeBenchRepo() @@ -181,7 +183,11 @@ describe('evalOnce — gates', () => { // strip read permission from its directory — a real "structural change to // the tree", and not one that shows up in `git diff` (permissions aren't // part of a tracked change), so it does not interact with the scope gate. - it('fails when a frozen file is restored but its directory cannot be listed', async () => { + // chmod is a no-op on Windows: there is no way to make a directory + // unlistable but still traversable there, so this specific gate cannot be + // provoked. Skipped and visible, rather than weakened on every platform to + // accommodate one. + it.skipIf(!posix)('fails when a frozen file is restored but its directory cannot be listed', async () => { const ctx = await setup() await chmod(join(ctx.root, 'src'), 0o111) // execute-only: lookup by path still works, readdir does not try { @@ -277,6 +283,31 @@ describe('evalOnce — gates', () => { }) }) +describe('evalOnce — abort', () => { + // cmd-eval.js's sticky-marker poll only runs every 500ms, so a forced stop + // already pending when a NEW eval starts cannot flip its AbortController + // any faster than that — by the time evalOnce is actually invoked the + // signal is essentially never aborted yet, which makes the CLI-level + // timing unsuitable for pinning down what evalOnce itself does with a + // signal that IS already aborted (as one is, for instance, whenever SIGINT + // lands before claimEval finishes). This checks that directly instead. + // + // baseline.commit is deliberately set to a commit that does not exist. An + // evalOnce that does not check the signal before its first real statement + // would reach the scope diff (gitx.changedSince, the very first thing it + // does) and fail with a git error about the bad revision, not with the + // abort message — a mock-free way to prove the checkpoint runs before any + // work at all, consistent with this file never stubbing out gitx. + it('checks an already-aborted signal before doing any work, not just before the gates', async () => { + const ctx = await setup() + await commitFiles(ctx.root, { 'src/wordcount.js': FAST_WORDCOUNT }) + ctx.baseline.commit = '0000000000000000000000000000000000000000' + const controller = new AbortController() + controller.abort() + await expect(run({ ...ctx, signal: controller.signal })).rejects.toThrow(/aborted before the experiment began/) + }) +}) + describe('evalOnce — measurement', () => { it('returns a decided verdict and time deltas for a real experiment', async () => { const ctx = await setup() diff --git a/test/state/lock.test.js b/test/state/lock.test.js index f1aa3b0..c25553e 100644 --- a/test/state/lock.test.js +++ b/test/state/lock.test.js @@ -58,14 +58,15 @@ describe('claimEval', () => { await expect(claimEval(dir)).rejects.toThrow(/already running/) }) - it('refuses a pid file holding a value it must never signal', async () => { - // `stop --force` signals the pid's process GROUP, and kill(-1) means every - // process the caller may signal — so a pid file holding 1 would turn a - // wedged experiment into a session-wide kill. + it('refuses a pid file holding a value it must never trust', async () => { + // alive() below calls process.kill(pid, 0) as a liveness probe, and + // kill(1, 0) or kill(-1, 0) would report "alive" regardless of whether + // anything real is running — so a pid file holding either would make + // isStale() never reclaim the lock, stranding it forever. await mkdir(join(dir, LOCK_DIR), { recursive: true }) await writeFile(join(dir, LOCK_DIR, 'pid'), '1') await writeFile(join(dir, LOCK_DIR, 'heartbeat'), String(Date.now())) - await expect(evalRunning(dir)).rejects.toThrow(/not a process this command will signal/) + await expect(evalRunning(dir)).rejects.toThrow(/not a pid this run will trust/) }) it('refuses an unparseable pid rather than guessing', async () => { diff --git a/test/state/state.test.js b/test/state/state.test.js index 9abb9ee..c8be9fc 100644 --- a/test/state/state.test.js +++ b/test/state/state.test.js @@ -1,6 +1,6 @@ import { chmod, lstat, mkdtemp, readFile, readlink, stat, symlink, writeFile, mkdir } from 'node:fs/promises' import { tmpdir } from 'node:os' -import { dirname, join } from 'node:path' +import { dirname, join, sep } from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { STATE_HOME_ENV, @@ -56,7 +56,7 @@ describe('stateDir', () => { it('lives under the state home, keyed by repository and tag', async () => { const dir = await stateDir(repo, 'sep4') expect(dir.startsWith(home)).toBe(true) - expect(dir.endsWith('/sep4')).toBe(true) + expect(dir.endsWith(`${sep}sep4`)).toBe(true) }) it('gives different repositories different keys', async () => { diff --git a/test/state/stop.test.js b/test/state/stop.test.js index 9b69b37..cd6aef3 100644 --- a/test/state/stop.test.js +++ b/test/state/stop.test.js @@ -2,7 +2,7 @@ import { mkdtemp, rm } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { beforeEach, describe, expect, it } from 'vitest' -import { clearStop, requestStop, stopRequested } from '../../src/state/stop.js' +import { clearStop, forceRequested, requestForceStop, requestStop, stopRequested } from '../../src/state/stop.js' let dir beforeEach(async () => { @@ -43,4 +43,44 @@ describe('stop sentinel', () => { await rm(dir, { recursive: true, force: true }) expect(await stopRequested(dir)).toBe(false) }) + + it('reports no forced stop before one is made', async () => { + expect(await forceRequested(dir)).toBe(false) + }) + + it('creates the state directory for a forced stop rather than failing on a missing one', async () => { + await requestForceStop(dir) + expect(await forceRequested(dir)).toBe(true) + }) + + it('is idempotent for a forced stop', async () => { + await requestForceStop(dir) + await requestForceStop(dir) + expect(await forceRequested(dir)).toBe(true) + }) + + it('keeps the two sentinels independent', async () => { + // A graceful stop must not read as a forced one: the first lets the + // experiment finish and be scored, the second throws it away. + await requestStop(dir) + expect(await forceRequested(dir)).toBe(false) + await rm(dir, { recursive: true, force: true }) + await requestForceStop(dir) + expect(await stopRequested(dir)).toBe(false) + }) + + it('clears both sentinels at once', async () => { + // One brake, one release. A --clear that left the forced marker behind + // would abort every later eval on this tag with no way to tell why. + await requestStop(dir) + await requestForceStop(dir) + await clearStop(dir) + expect(await stopRequested(dir)).toBe(false) + expect(await forceRequested(dir)).toBe(false) + }) + + it('reports false rather than throwing when the forced sentinel is unreadable', async () => { + await rm(dir, { recursive: true, force: true }) + expect(await forceRequested(dir)).toBe(false) + }) })