From b18e5e9f6119e24cabbe41f5bca8c68e5e711b74 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 14 Sep 2026 17:49:43 +0300 Subject: [PATCH 1/9] Add a forced-stop sentinel beside the graceful one 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, and the eval claim is stranded. Polling a file behaves identically everywhere. This sentinel is deliberately sticky. 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. Only clearStop removes it. Nothing reads the marker yet. Later tasks make eval poll it and make `stop --force` write it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- src/state/stop.js | 52 ++++++++++++++++++++++++++++++++++++++++- test/state/stop.test.js | 42 ++++++++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 2 deletions(-) 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/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) + }) }) From 3745ab301261e590bbbc38881ca71b6a48ec36a0 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 14 Sep 2026 18:01:12 +0300 Subject: [PATCH 2/9] Abort eval from a polled marker, not only from a signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit added stop.force but nothing read it. eval already has an AbortController fed by SIGINT/SIGTERM; that controller is now also fed by a 500ms poll of forceRequested, installed at the same spot as the signal handlers and for the same reason — a marker already pending when a run starts must abort it at its first checkpoint, not after it has claimed the run and begun measuring. The poller is unref'd so it never keeps the process alive on its own, and its interval is torn down on every exit path alongside the signal handlers, including the early return when claimEval itself throws. This makes the abort path identical on every platform: Windows never sees SIGINT or SIGTERM cross processes, but a file it can always see. `stop --force` itself still signals rather than writing the marker — that switch is a later task. Because the marker is sticky, an abort that finds it still pending means every later eval on this tag will abort too until someone clears it. The ABORTED verdict now says so and names the cure, rather than reporting the same generic "interrupted" message regardless of cause — folded into the existing `message` field so --json's stdout still carries nothing but the verdict object. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- src/cli/cmd-eval.js | 35 ++++++++++++++++++++++++---- test/cli/eval.test.js | 53 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 77 insertions(+), 11 deletions(-) diff --git a/src/cli/cmd-eval.js b/src/cli/cmd-eval.js index 4628d6b..faaa785 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,21 @@ 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() + }) + }, FORCE_POLL_MS) + poll.unref() + const detach = () => { process.off('SIGINT', onSignal) process.off('SIGTERM', onSignal) + clearInterval(poll) } let claim @@ -83,7 +98,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 +143,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 +174,7 @@ export async function runEval(args, io) { } throw err } finally { - offSignals() + detach() await closeLog(logStream) await claim.release() } diff --git a/test/cli/eval.test.js b/test/cli/eval.test.js index cb7234b..46d6180 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)) @@ -178,7 +181,7 @@ describe('eval', () => { // before we got to send the signal, 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,7 +195,7 @@ 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 @@ -245,7 +248,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 +263,42 @@ describe('eval', () => { 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) + }) }) From 57139fcda6925aa669130f65e3455fb7f2829bf3 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 14 Sep 2026 18:10:45 +0300 Subject: [PATCH 3/9] Make stop --force write a marker instead of signalling a pid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous two commits built the marker and the poller that reads it; this one finally stops sending a signal. `stop --force` now calls requestForceStop unconditionally, before it even looks at the eval lock, so the marker stands regardless of whether a pid was ever readable. A running eval picks it up at its next 500ms checkpoint and aborts itself through the AbortController Task 2 wired up; nothing is signalled between processes, so the path is identical on Windows and POSIX, and it can no longer reach an unrelated process the OS has recycled a stale pid onto. evalRunning is demoted from mechanism to reporting: its only job now is to say, for the human's benefit, whether an eval happens to be running right now and to name its pid in the message. A corrupt pid file used to be swallowed with "assuming there is nothing to signal" because it once gated the SIGTERM; now that the marker already stands before evalRunning is even called, a corrupt lock costs the report one line and nothing else. The "what this leaves you with" report below is untouched — it still only reports, and dropping an abandoned commit remains the human's call. status gains a way to see this from the outside: `forced` is checked alongside `stopped`, and a pending force renders as its own FORCED line rather than being folded into "requested", because the two markers mean different things to someone reading the terminal — one lets the current experiment finish, the other means it already didn't. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- src/cli/cmd-status.js | 12 ++++++++++-- src/cli/cmd-stop.js | 38 +++++++++++++++++++------------------- test/cli/status.test.js | 11 ++++++++++- test/cli/stop.test.js | 24 +++++++++++++++++++++++- 4 files changed, 62 insertions(+), 23 deletions(-) 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..bc08424 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, requestForceStop, requestStop } from '../state/stop.js' import { expandSingleDashFlags, resolveRun } from './context.js' /** @@ -55,30 +55,30 @@ export async function runStop(args, io) { 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/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..2df6bf0 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,6 +48,7 @@ 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 () => { @@ -56,6 +57,27 @@ describe('stop', () => { expect(await stopRequested(await stateDir(dir, 't'))).toBe(true) }) + 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']) From 6f75753a6cc60d1c0b4a0b620fb9d15f42bbf0d6 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 14 Sep 2026 18:18:04 +0300 Subject: [PATCH 4/9] Fix three test assumptions that only hold on POSIX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI run 34195066520 failed five tests: two were product defects (now fixed in Tasks 1-3 by wiring up marker-based abortion instead of signals), and three were test defects assuming POSIX conventions that Windows does not share. The path separator `/` is POSIX-only; Windows uses `\`. `stateDir` returns a filesystem path that ends correctly on either platform, but the test asserted a hardcoded `/sep4` in test/state/state.test.js:59. Now it uses the sep export from node:path, just as the code under test does. Line endings vary by platform and repository configuration. The Windows runner checks templates/program.md out with CRLF, turning every `\n` into `\r\n`. The regex in test/cli/init.test.js:117 required a bare `\n` at a paragraph boundary. The new `.gitattributes` pins program.md to LF on all platforms so a checkout never surprises the test, and the regex now accepts both line endings so it stays honest against a CRLF copy that might arrive some other way. On Windows, `chmod` is a no-op — there is no permission model to enforce. The test in test/pipeline.test.js:184 tried to make a directory unlistable while keeping it traversable, a distinction that exists only on POSIX. The test is now wrapped with `it.skipIf(!posix)` and a comment explaining why it cannot run on Windows, matching this repo's established idiom for platform-specific tests (see test/state/state.test.js:191-223). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- .gitattributes | 4 ++++ test/cli/init.test.js | 2 +- test/pipeline.test.js | 8 +++++++- test/state/state.test.js | 4 ++-- 4 files changed, 14 insertions(+), 4 deletions(-) create mode 100644 .gitattributes 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/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/pipeline.test.js b/test/pipeline.test.js index 37cf941..912048b 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 { 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 () => { From 67f66f27a17f96bc7b9fbe0ebbec980d1b1f7a68 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Mon, 14 Sep 2026 18:23:50 +0300 Subject: [PATCH 5/9] Stop doctor warning about Windows, and stop it inventing a load average MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tasks 1-4 removed the actual reason Windows was unsupported: stop --force now writes a polled marker instead of relying on POSIX signals, and three test assumptions have been fixed. The checkWindows function's warning about interrupted evals not reaching the ABORTED path is no longer honest — measurement works on Windows now that the stop mechanism is reliable. Where a platform lacks a tool or feature, doctor degrades gracefully: one check reports NA rather than the whole report failing. This is the intended design, stated in the check() header comment. checkWindows violated that by warning about the platform itself; now it simply contributes no platform line on Windows, exactly as every platform other than darwin and linux already does. os.loadavg() is not implemented on Windows — it returns a hardcoded [0, 0, 0] to avoid breaking portable code. Reporting that would tell a user under heavy load that their machine is perfectly idle, which is worse than saying nothing in a tool whose whole job is predicting whether measurement will be reliable. checkLoad now returns NA on Windows with clear guidance to check the machine manually, matching the tool's principle of informing, never blocking. The two new tests guard against regression: on Windows (Task 7), they verify both changes work; on macOS/Linux, both pass trivially because the second does not produce a platform finding at all, and the first enters the else branch unchanged. They earn their keep on Windows CI. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- src/doctor.js | 37 ++++++++----------------------------- test/doctor.test.js | 17 +++++++++++++++-- 2 files changed, 23 insertions(+), 31 deletions(-) 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/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 () => { From 280bf11ddda26f9f7cdadafc8d5997da1418efa7 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Tue, 15 Sep 2026 10:03:37 +0300 Subject: [PATCH 6/9] Record why group teardown still reaches grandchildren on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The doc comment above killGroup now explains the platform-specific mechanism that makes grandchildren die even on Windows despite the absence of process groups. On Windows, libuv assigns each non-detached child to a job object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, so terminating the direct child closes its job handle and kills everything it spawned. This holds only because every subprocess started by this harness is process.execPath — see the updated comment for the two changes that would silently break it, leaving vitest's workers running and corrupting later measurements. The runner.test.js assertions confirm this on every platform, including Windows CI, by recording each grandchild's pid and asserting it cannot be signaled. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- src/runner.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) 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 From f3f0025335b59ffeade074dcad6ba0d559f7f9e8 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Tue, 15 Sep 2026 10:13:54 +0300 Subject: [PATCH 7/9] Put Windows in the CI matrix and say so in the README MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tasks 1-6 removed the only reason Windows was excluded: stop --force now writes a sticky marker that a running eval polls into the same AbortController Ctrl+C uses, instead of sending a signal Windows cannot deliver across processes. Nothing in the abort path is platform-specific anymore, so the matrix should stop pretending otherwise. Adding windows-latest alongside ubuntu-latest and macos-latest is what actually turns "Windows works now" from a claim into something CI checks on every push, which is the whole point of doing this last. The README's Platform support section is rewritten to match: the table reads "supported" on all three rows, and the prose explains the marker mechanism and the handful of genuinely cosmetic differences that remain (doctor's missing load average, the skipped ownership check, one Windows-inapplicable chmod test) rather than warning readers away from the platform doctor no longer warns about itself. The "Three ways to end a run" item for stop --force still described the old SIGTERM behavior, which Task 3 made false — it no longer signals anything. Rewritten to say what actually happens: the marker is written, the running eval polls for it and cancels itself, and that cancellation is what tears down the process tree. The stop --clear sentence below it undersold what the command does now that there are two sentinel files to clear; it read as cancelling one request when clearStop has cleared both since the forced-stop marker was introduced, so it now says so explicitly. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- .github/workflows/ci.yml | 2 +- README.md | 51 +++++++++++++++++++++++----------------- 2 files changed, 31 insertions(+), 22 deletions(-) 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..1ec4177 100644 --- a/README.md +++ b/README.md @@ -173,17 +173,19 @@ 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. `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 +384,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 From 57ee868d208df62285d5d9671c9c7be615efb05e Mon Sep 17 00:00:00 2001 From: Gal Be Date: Tue, 15 Sep 2026 10:55:35 +0300 Subject: [PATCH 8/9] Fix the two Important findings from the whole-branch review, plus four minor ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit evalOnce had no abort checkpoint before the gate phase: the scope diff, config hash and freeze.restore all ran unconditionally, and the first read of o.signal.aborted was after runGates returned. Runner.run only ever registers an 'abort' listener, and an AbortSignal that is already aborted by the time a listener is added never dispatches to it — so a gate whose Runner.run() happened to be invoked after the sticky-marker poll had already fired would run to completion (or its own per-child timeout) before the harness ever noticed. cmd-eval.js's own comment claimed a pending marker aborts a run "at its first checkpoint, not after it has begun measuring", which was false because no such checkpoint existed. Added one as the literal first statement of evalOnce, matching the existing post-gate check's idiom. Investigating this closely turned up that runGates()'s own post-gate abort check, combined with typecheck's built-in per-file abort awareness, already protects a plain-JS repo with no ESLint config from the worst of this — the newly added CLI-level test (marker written before eval is spawned) passes on both the fixed and unfixed tree for that reason. A second, deterministic unit test against evalOnce() directly (baseline.commit pointed at a nonexistent commit, so an unchecked signal reaches the real scope diff and fails with a git error instead of the abort message) gives a genuine red/green cycle without depending on timing. state/lock.js's guard against a pid file holding 1 or -1 justified itself by "the exact form stop --force uses for its own valid pids" and its error text promised the pid "is not a process this command will signal". Neither is true after 57139fc: nothing in src/cli/ hands a pid to process.kill any more. The guard itself still matters for a completely different reason — alive() uses process.kill(pid, 0) as a liveness probe, and pid 1 or -1 would make that probe always succeed, permanently stranding the lock via isStale() — so the comment and error text now say that instead, with no change to the guard's logic. The test asserting on the old error string, and its own comment repeating the same stale "signals the pid's process group" claim, are both updated to match. Also: the poll's forceRequested().then() in cmd-eval.js gets a .catch(), matching its sibling call; four leftover comments across the eval and stop tests that still described the pre-Windows signal-based mechanism now describe the marker instead, with each comment's actual point kept intact; the README's stop --force item now says plainly that the marker is sticky and will abort the next eval if nothing was running when it was issued; and a plain `stop` now checks whether a forced stop from an earlier, uncleared `stop --force` is still pending before promising the current experiment will be measured, since that promise is false in exactly that case. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- README.md | 6 ++-- src/cli/cmd-eval.js | 8 +++-- src/cli/cmd-stop.js | 13 +++++++-- src/pipeline.js | 11 +++++++ src/state/lock.js | 16 ++++++---- test/cli/eval.test.js | 65 +++++++++++++++++++++++++++++++++++++---- test/cli/stop.test.js | 16 +++++++++- test/pipeline.test.js | 25 ++++++++++++++++ test/state/lock.test.js | 11 +++---- 9 files changed, 147 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 1ec4177..c0308c4 100644 --- a/README.md +++ b/README.md @@ -178,8 +178,10 @@ Three ways to end a run, in increasing order of force: 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. `stop --force` reports what HEAD looks like afterward; it does - not touch the repository for you. + 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`. diff --git a/src/cli/cmd-eval.js b/src/cli/cmd-eval.js index faaa785..f9dedd1 100644 --- a/src/cli/cmd-eval.js +++ b/src/cli/cmd-eval.js @@ -81,9 +81,11 @@ export async function runEval(args, io) { // 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() - }) + forceRequested(run.stateDir) + .then((forced) => { + if (forced) controller.abort() + }) + .catch(() => {}) }, FORCE_POLL_MS) poll.unref() const detach = () => { diff --git a/src/cli/cmd-stop.js b/src/cli/cmd-stop.js index bc08424..a4ec500 100644 --- a/src/cli/cmd-stop.js +++ b/src/cli/cmd-stop.js @@ -14,7 +14,7 @@ import { parseArgs } from 'node:util' import * as gitx from '../gitx.js' import { evalRunning } from '../state/lock.js' -import { clearStop, requestForceStop, requestStop } from '../state/stop.js' +import { clearStop, forceRequested, requestForceStop, requestStop } from '../state/stop.js' import { expandSingleDashFlags, resolveRun } from './context.js' /** @@ -50,7 +50,16 @@ 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 } 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/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/test/cli/eval.test.js b/test/cli/eval.test.js index 46d6180..4234e87 100644 --- a/test/cli/eval.test.js +++ b/test/cli/eval.test.js @@ -178,7 +178,7 @@ 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) await requestForceStop(state) @@ -202,11 +202,12 @@ describe('eval', () => { // (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 @@ -235,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) { @@ -264,6 +266,57 @@ describe('eval', () => { 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. diff --git a/test/cli/stop.test.js b/test/cli/stop.test.js index 2df6bf0..60840e6 100644 --- a/test/cli/stop.test.js +++ b/test/cli/stop.test.js @@ -51,12 +51,26 @@ describe('stop', () => { 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']) diff --git a/test/pipeline.test.js b/test/pipeline.test.js index 912048b..2948ecd 100644 --- a/test/pipeline.test.js +++ b/test/pipeline.test.js @@ -283,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 () => { From 090ea894641cdb176c8bae53bcb87acffe569c92 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Tue, 15 Sep 2026 11:06:03 +0300 Subject: [PATCH 9/9] Release 0.2.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows is a supported platform as of this release. `stop --force` no longer signals the running eval: it writes a sticky marker that eval polls into the AbortController it already had, so the abort path — ABORTED, exit 2, no results.tsv row, claim released — is identical on Linux, macOS and Windows. Nothing is signalled between processes on any platform, which also closes a race on POSIX where a pid read from a file could have been recycled onto an unrelated process. CI now runs the full suite on windows-latest alongside the other two, on Node 20 and 22. That is what makes the README's support claim a fact the next release has to keep true, rather than a sentence someone wrote once. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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",