mirror of
https://github.com/stablyai/orca.git
synced 2026-10-08 08:02:32 +00:00
Neil
3763103084
## In plain terms Orca lets a project define scripts that run at certain moments — one when a workspace is set up, one just before it is deleted. Those scripts get a time limit. When the limit ran out, Orca asked the script to stop and then believed whatever the script said on its way out — so a script written to shut down politely could be cut off halfway through its work and still report that it had finished. Anything relying on that answer was relying on a guess. Now the verdict comes from the clock, not from the script: if it ran out of time, that is what is reported, whatever exit code it managed on the way out. Orca also stops the script's *children* rather than just the script, so a background process it started can no longer outlive it. Split out of #20153 so the gate that consumes this answer is reviewed separately. `Refs #19334` rather than `Fixes`, because it does not close the issue on its own. ## The bug `exec({ timeout })` sends SIGTERM and then reports what the child did. A hook that traps SIGTERM and exits 0 therefore comes back with a **null error** — success — despite having been cut off. ```js exec("trap 'exit 0' TERM; sleep 5", { timeout: 200 }, (err) => …) // err === null ``` That is not an `exited` vs `unverifiable` nicety: it is a failed hook reported as a passing one. Realistic triggers are ordinary — a Node wrapper with a graceful `process.on('SIGTERM')`, an rsync wrapper that cleans up on signal. ## What changed **`runHook` owns the deadline.** The verdict comes from running out of time rather than from the corpse's exit code, and it is settled *at* the deadline rather than whenever the child gets around to dying — a hook that traps the signal and keeps running must not hold its caller open. **A timeout withholds the exit code.** So does a spawn failure, where `exec` reports a *string* code (`ENOENT`); the `typeof code === 'number'` guard is what keeps a hook that never ran out of the "exited" verdict. Callers that distinguish "exited N" from "outcome never observed" can now trust that distinction: | failure mode | `error.code` | signal | verdict | | --- | --- | --- | --- | | non-zero exit | `23` | — | `exited 23` | | command not found | `127` | — | `exited 127` | | killed | `null` | SIGKILL | outcome not observed | | deadline expired | *(the deadline, not the exit)* | SIGTERM→SIGKILL | outcome not observed | | deadline expired, hook traps SIGTERM and exits 0 | `0` | — | outcome not observed | | spawn failure | `"ENOENT"` *(string)* | — | outcome not observed | **Termination reaches the process group.** The script is a shell and the work is its children, so signalling only the shell leaves a `sleep` or an `rsync` alive holding the pipes open. SIGTERM first, then SIGKILL after a grace. **One `classifyHookProcessResult`** now serves the native and WSL branches, which had been mapping a finished process to a hook verdict by hand, identically. That duplication predates this change. ## Terminating the tree, and a test that could not fail The escalation went wrong once in review, in a way worth recording. A first attempt skipped the SIGKILL when the *direct child* had already exited — a dead child needs no signal. That is correct about the child and wrong about the group: a hook that backgrounds a server typically loses its shell leader to the first SIGTERM while the server keeps running, so the skip fired in exactly the case the escalation exists for. The escalation now probes the **group** with signal 0: `ESRCH` means nothing is left to kill, anything else gets the signal. **The residual trade-off, stated rather than implied.** Signalling by negative pid names whatever group owns that pid *now*. Once the leader is reaped its pid can be recycled, and a probe cannot distinguish a surviving descendant from a stranger that inherited the number. Killing a runaway hook is both the likelier event and the one the deadline promises, so the group is signalled whenever it answers; the remaining window is pid wraparound inside the grace. **A test that cannot fail is worse than no test.** The first regression test drove `runHook` with `process.kill` intercepted — and passed against *both* the broken and the fixed version, because with signals intercepted nothing dies, so the child never reached the exited state the bad guard keyed on. It was false assurance, not coverage. `terminateHookTree` is therefore exported and the regression pinned directly against it: it fails on the old version (`expected [] to deeply equal [[-4242, 'SIGKILL']]`) and passes on this one. ## Behaviour change for `setup` hooks Both hook kinds share `runHook`, so this is not confined to archive hooks. **A setup hook that backgrounds a long-running server now has that server SIGTERM'd — then SIGKILL'd — with the rest of its process group when the deadline expires, where previously it was orphaned and survived.** Arguably the better behaviour, since an orphaned server is a leak, but it is a real change and should be a decision rather than a discovery. ## Evidence Against real shells and real signals, because this bug is invisible to a mock (`hook-archive-timeout-observation.test.ts`, through `runHook` itself rather than an extracted helper): ``` ✓ fails a hook that traps SIGTERM and exits zero, despite its zero exit ✓ settles at the deadline even when the hook refuses to die ✓ passes a hook that finishes inside its deadline ✓ reports an observed non-zero exit as the exit it is ``` Plus `hooks-archive-exit-observation.test.ts` for the wiring — including the string-`ENOENT` case — and `hook-archive-termination-safety.test.ts` for the escalation branching. ## Checks `pnpm tc` · `oxlint src` · `oxfmt --check` · 113 tests across `src/main/hooks*`. The classification table above is measured against real `exec`, not reasoned.
53 lines
2.4 KiB
TypeScript
53 lines
2.4 KiB
TypeScript
import { describe, expect, it, vi } from 'vitest'
|
|
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
|
|
import { tmpdir } from 'node:os'
|
|
import { join } from 'node:path'
|
|
import type { Repo } from '../shared/repo-types'
|
|
|
|
vi.mock('./effective-hook-config', () => ({
|
|
getEffectiveHooksFromConfig: (_repo: unknown, hooks: unknown) => hooks
|
|
}))
|
|
|
|
const REPO: Repo = { id: 'r', path: '/repo', displayName: 'r', badgeColor: '#000', addedAt: 0 }
|
|
|
|
/** Run a real archive script in a real shell, under a deadline short enough to test. */
|
|
async function runArchive(script: string, timeoutMs = 400) {
|
|
const { runHook } = await import('./hooks')
|
|
const dir = mkdtempSync(join(tmpdir(), 'orca-hook-deadline-'))
|
|
writeFileSync(join(dir, 'orca.yaml'), `scripts:\n archive: |\n ${script}\n`)
|
|
try {
|
|
return await runHook('archive', dir, REPO, dir, undefined, timeoutMs)
|
|
} finally {
|
|
rmSync(dir, { recursive: true, force: true })
|
|
}
|
|
}
|
|
|
|
// Why a real shell (#19334): this bug is invisible to a mock. Node's `exec({ timeout })` SIGTERMs
|
|
// the child and reports whatever it chose to do, so a hook that traps SIGTERM and exits 0 came
|
|
// back as a PASS — a hook cut off mid-archive, indistinguishable from one that finished its work.
|
|
describe.skipIf(process.platform === 'win32')('archive hook deadline', () => {
|
|
it('fails a hook that traps SIGTERM and exits zero, despite its zero exit', async () => {
|
|
const result = await runArchive("trap 'exit 0' TERM; sleep 30")
|
|
expect(result.success).toBe(false)
|
|
// Withheld, so the removal gate reads `unverifiable` rather than a pass.
|
|
expect(result.exitCode).toBeUndefined()
|
|
expect(result.output).toContain('timed out')
|
|
}, 20_000)
|
|
|
|
it('settles at the deadline even when the hook refuses to die', async () => {
|
|
const started = Date.now()
|
|
const result = await runArchive("trap '' TERM; sleep 30")
|
|
expect(result.success).toBe(false)
|
|
// A hook that ignores the signal must not hold a removal open until it finishes.
|
|
expect(Date.now() - started).toBeLessThan(10_000)
|
|
}, 20_000)
|
|
|
|
it('passes a hook that finishes inside its deadline', async () => {
|
|
await expect(runArchive('echo archived')).resolves.toMatchObject({ success: true })
|
|
})
|
|
|
|
it('reports an observed non-zero exit as the exit it is', async () => {
|
|
await expect(runArchive('exit 23')).resolves.toMatchObject({ success: false, exitCode: 23 })
|
|
})
|
|
})
|