Skip to content

Commit 96c8fd1

Browse files
committed
Phase 2: job processor + BullMQ worker wiring (verified against real bullmq)
The processor orchestration and the BullMQ Worker construction, both with the wiring the research confirmed live against a real Valkey. processor.mjs -- the money-safety ORDER as a pure-ish function over injected deps, so it is testable without GitHub/Docker/Redis. The order IS the contract: mint token -> refuse an unprotected default branch -> prepare (resolve SHA, clone, materialise .pi/, write prompt) -> reserve budget -> ONLY THEN run the container. Budget is reserved as late as possible but strictly before the container, so a refusal from a free earlier gate (unprotected repo, clone failure) never consumes a daily slot. 9 tests assert the order directly: an unprotected repo never prepares or spends; over-budget prepares but never runs a container; exit 0 -> return, 2 -> return, 1 -> throw InfraRetry (retryable); cleanup runs on every path; a local-folder job skips minting and branch-check. index.mjs -- the Worker, with every load-bearing fact from the wiring research: - connection.maxRetriesPerRequest: null (REQUIRED, or bullmq throws at construction) - maxStalledCount: 0 (a stalled paid job FAILS, never silently re-runs -- confirmed live) - the processor declares EXACTLY 3 params (job, token, signal). bullmq only allocates an AbortController when processor.length >= 3, so dropping the unused token would silently disable the 30-minute timeout AND shutdown abort. A test asserts arity===3 precisely because the failure is silent -- verified passing with bullmq present. - the 30-min timeout is ours: a setTimeout fires worker.cancelJob(id), which raises the AbortSignal; the abort handler docker-stops the container. The same abort path serves SIGTERM/SIGINT shutdown -- one mechanism, two triggers. Verified: the timeout fires cancelJob, and an abort stops the right container. While testing I found and fixed a real robustness gap: runContainer must handle an already-aborted signal (the timeout can fire during a slow prepare), now stated in its contract. The remaining GitHub-dependent deps -- mintToken, isDefaultBranchProtected, prepareWorkspace (clone at SHA via git fetch --depth) -- need a GitHub App to verify end to end rather than guess, and are the injected seams the processor is already tested against.
1 parent 2bfaf9e commit 96c8fd1

4 files changed

Lines changed: 362 additions & 0 deletions

File tree

worker/src/index.mjs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
import { execFile } from "node:child_process";
2+
import { promisify } from "node:util";
3+
import { UnrecoverableError, Worker } from "bullmq";
4+
import { InfraRetry, runJob } from "./processor.mjs";
5+
6+
const exec = promisify(execFile);
7+
8+
export const QUEUE = "pi-jobs";
9+
export const JOB_TIMEOUT_MS = 30 * 60 * 1000; // REQ-JOB-TIMEOUT-30M
10+
11+
/**
12+
* Build the BullMQ processor.
13+
*
14+
* It MUST declare exactly three parameters (job, token, signal). BullMQ only allocates an
15+
* AbortController when `processor.length >= 3` (it inspects the function's arity at construction),
16+
* so dropping the unused `token` would silently disable BOTH the 30-minute timeout and the shutdown
17+
* abort -- with no error. A test asserts the arity precisely because the failure is silent.
18+
*
19+
* Dependencies are injected so this is testable without a live queue: `cancelJob` (fired by the
20+
* timeout), `stopContainer` (fired by the abort), and the orchestration deps.
21+
*/
22+
export function makeProcessor({ cancelJob, stopContainer, redis, cap, deps, timeoutMs = JOB_TIMEOUT_MS }) {
23+
return async function processor(job, token, signal) {
24+
const name = `pi-job-${job.id}`;
25+
const timer = setTimeout(() => {
26+
// BullMQ has no per-job kill timer; this is ours. cancelJob raises the AbortSignal.
27+
Promise.resolve(cancelJob(job.id, "job-timeout-30m")).catch(() => {});
28+
}, timeoutMs);
29+
30+
// Abort (timeout OR shutdown) => stop the container. docker stop sends SIGTERM then SIGKILL
31+
// after the grace period; the runner exits and runContainer returns/throws.
32+
const onAbort = () => {
33+
Promise.resolve(stopContainer(name)).catch(() => {});
34+
};
35+
signal.addEventListener("abort", onAbort, { once: true });
36+
37+
try {
38+
return await runJob(job.data, {
39+
redis,
40+
cap,
41+
...deps,
42+
runContainer: (ctx) => deps.runContainer({ ...ctx, name, signal }),
43+
});
44+
} catch (error) {
45+
if (error instanceof InfraRetry) throw error; // retryable: BullMQ retries per attempts
46+
// A non-retryable, non-infra error (our bug) must not retry forever. UnrecoverableError
47+
// records it as failed-and-distinct on the dashboard without a retry.
48+
throw new UnrecoverableError(error.message);
49+
} finally {
50+
clearTimeout(timer);
51+
signal.removeEventListener("abort", onAbort);
52+
}
53+
};
54+
}
55+
56+
export function createWorker({ connection, concurrency, cap, redis, deps, limiter }) {
57+
let worker; // referenced by cancelJob before assignment; only called later, so the TDZ is fine
58+
const processor = makeProcessor({
59+
cancelJob: (id, reason) => worker.cancelJob(id, reason),
60+
stopContainer: (name) => exec("docker", ["stop", "-t", "5", name]),
61+
redis,
62+
cap,
63+
deps,
64+
});
65+
66+
worker = new Worker(QUEUE, processor, {
67+
// maxRetriesPerRequest: null is REQUIRED for BullMQ's blocking connections, or it throws.
68+
connection: { ...connection, maxRetriesPerRequest: null },
69+
concurrency,
70+
maxStalledCount: 0, // a stalled paid job FAILS, never silently re-runs (verified live)
71+
...(limiter ? { limiter } : {}),
72+
});
73+
74+
const shutdown = async () => {
75+
// Abort active jobs (=> docker stop via onAbort), then close. Without the cancel,
76+
// worker.close() would wait up to 30 minutes for the container.
77+
await Promise.resolve(worker.cancelAllJobs?.("shutdown")).catch(() => {});
78+
await worker.close();
79+
process.exit(0);
80+
};
81+
process.once("SIGTERM", shutdown);
82+
process.once("SIGINT", shutdown);
83+
84+
return worker;
85+
}

worker/src/processor.mjs

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import { reserveBudget } from "./budget.mjs";
2+
import { EXIT_COMPLETED, EXIT_INFRA, EXIT_POLICY } from "./exit-code.mjs";
3+
4+
/**
5+
* The job orchestration. Deliberately a pure-ish function over INJECTED side-effecting deps, so
6+
* the money-safety ORDER can be tested without GitHub, Docker, or Redis.
7+
*
8+
* The order is the contract, and every step before `runContainer` must be free of provider spend:
9+
*
10+
* 1. mint a scoped token (GitHub jobs) -- CONST-TOKEN-SCOPED-PER-JOB
11+
* 2. REFUSE an unprotected default branch -- REQ-BRANCH-PROTECTION-PRECONDITION
12+
* 3. resolve the default-branch SHA (fresh API), clone at it, materialise .pi/, write the prompt
13+
* 4. reserve a budget slot -- CONST-BUDGET-BEFORE-TOKENS
14+
* 5. ONLY NOW run the container (the only step that spends provider tokens)
15+
* 6. map the container exit code to retry-vs-success
16+
*
17+
* Budget is reserved as late as possible but strictly before the container, so a refusal from an
18+
* earlier free gate (unprotected repo, clone failure) never consumes a daily slot. The container
19+
* is the only thing that spends money, so "before tokens" means "before this line".
20+
*
21+
* Returns a result object on a non-retryable outcome; THROWS on a retryable (infra) one so BullMQ
22+
* retries per `attempts`. The caller (the BullMQ processor) turns the thrown/returned distinction
23+
* into the queue's retry behaviour -- that is INT-RUNNER-EXIT-CODE-PROTOCOL.
24+
*/
25+
export async function runJob(job, deps) {
26+
const {
27+
redis,
28+
cap,
29+
mintToken, // (repo) => scoped 1h token | null for local-folder jobs
30+
isDefaultBranchProtected, // (repo, token) => boolean
31+
prepareWorkspace, // (job, token) => { workspaceDir, jobDir } (clone+materialise+prompt)
32+
// runContainer({ job, token, prepared, name, signal }) => exitCode. It MUST honour `signal`:
33+
// stop the container on abort, and reject/exit promptly if `signal.aborted` is already true
34+
// at entry (the timeout can fire during a slow prepare). The wiring injects name + signal.
35+
runContainer,
36+
cleanup, // (dirs) => void
37+
comment, // (job, text) => void (issue status; no-op for local jobs)
38+
log = () => {},
39+
now = new Date(),
40+
} = deps;
41+
42+
const isGitHub = job.kind === "github";
43+
let token = null;
44+
let prepared = null;
45+
let reserved = false;
46+
47+
try {
48+
if (isGitHub) {
49+
token = await mintToken(job.repo);
50+
51+
// REQ-BRANCH-PROTECTION-PRECONDITION. The agent's token can merge (contents:write covers
52+
// push AND merge), so branch protection is the only technical barrier to a self-merge.
53+
// Refuse before spending anything.
54+
if (!(await isDefaultBranchProtected(job.repo, token))) {
55+
await comment(job, "Refused: the default branch is not protected. See SECURITY.md.");
56+
log("refused_unprotected", { repo: job.repo });
57+
return { outcome: "policy", reason: "unprotected-branch" }; // return => not retried
58+
}
59+
}
60+
61+
prepared = await prepareWorkspace(job, token); // resolves SHA, clones, materialises .pi/, writes prompt
62+
63+
// Budget last-but-before-container. A refusal here spends nothing (no container starts).
64+
const budget = await reserveBudget(redis, { cap, now });
65+
reserved = true;
66+
if (!budget.allowed) {
67+
await comment(job, `Over the daily budget cap (${budget.cap}). Not run.`);
68+
log("over_budget", { reserved: budget.reserved, cap: budget.cap });
69+
return { outcome: "policy", reason: "over-budget" }; // return => not retried
70+
}
71+
72+
const exitCode = await runContainer({ job, token, prepared });
73+
log("container_exit", { exitCode });
74+
75+
switch (exitCode) {
76+
case EXIT_COMPLETED:
77+
return { outcome: "completed" };
78+
case EXIT_POLICY:
79+
return { outcome: "policy", reason: "runner-policy" };
80+
case EXIT_INFRA:
81+
throw new InfraRetry(`infra failure, container exit ${exitCode}`);
82+
default:
83+
throw new InfraRetry(`unknown container exit ${exitCode}`);
84+
}
85+
} finally {
86+
if (prepared) await cleanup(prepared).catch(() => {});
87+
}
88+
}
89+
90+
/** Thrown for the retryable (infra) class only. The BullMQ processor lets this propagate to retry. */
91+
export class InfraRetry extends Error {
92+
constructor(message) {
93+
super(message);
94+
this.name = "InfraRetry";
95+
this.piDispatchRetry = true;
96+
}
97+
}
98+
99+
export { EXIT_COMPLETED, EXIT_INFRA, EXIT_POLICY };

worker/test/processor.test.mjs

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
import assert from "node:assert/strict";
2+
import { test } from "node:test";
3+
import { InfraRetry, runJob } from "../src/processor.mjs";
4+
5+
/** A fake redis whose counter we can preset, to force over/under budget. */
6+
function fakeRedis(start = 0) {
7+
let n = start;
8+
return { async incr() { return ++n; }, async decr() { return --n; }, async expire() {} };
9+
}
10+
11+
/** Deps with call-order tracking, so tests can assert the money-safety ORDER, not just outcomes. */
12+
function deps(overrides = {}) {
13+
const calls = [];
14+
const base = {
15+
redis: fakeRedis(),
16+
cap: 10,
17+
mintToken: async (repo) => (calls.push(`mint:${repo}`), "tok"),
18+
isDefaultBranchProtected: async () => (calls.push("branch-check"), true),
19+
prepareWorkspace: async () => (calls.push("prepare"), { workspaceDir: "/w", jobDir: "/j" }),
20+
runContainer: async () => (calls.push("run-container"), 0),
21+
cleanup: async () => (calls.push("cleanup"), undefined),
22+
comment: async (_j, t) => calls.push(`comment:${t.slice(0, 12)}`),
23+
now: new Date("2026-07-16T10:00:00Z"),
24+
};
25+
return { deps: { ...base, ...overrides }, calls };
26+
}
27+
28+
const ghJob = { kind: "github", repo: "org/repo", provider: "anthropic", model: "m", maxTurns: 20 };
29+
30+
test("happy path: mint -> branch-check -> prepare -> budget -> container, in that order", async () => {
31+
const { deps: d, calls } = deps();
32+
const r = await runJob(ghJob, d);
33+
assert.equal(r.outcome, "completed");
34+
assert.deepEqual(calls, ["mint:org/repo", "branch-check", "prepare", "run-container", "cleanup"]);
35+
});
36+
37+
test("an unprotected branch refuses BEFORE any container -- and never prepares/spends", async () => {
38+
const { deps: d, calls } = deps({ isDefaultBranchProtected: async () => false });
39+
const r = await runJob(ghJob, d);
40+
assert.equal(r.outcome, "policy");
41+
assert.equal(r.reason, "unprotected-branch");
42+
assert.ok(!calls.includes("run-container"), "must not spend on an unprotected repo");
43+
assert.ok(!calls.includes("prepare"), "must not even clone an unprotected repo");
44+
});
45+
46+
test("over budget refuses AFTER prepare but BEFORE the container -- no provider spend", async () => {
47+
// counter starts at cap, so the reservation lands over-cap.
48+
const { deps: d, calls } = deps({ redis: fakeRedis(10), cap: 10 });
49+
const r = await runJob(ghJob, d);
50+
assert.equal(r.reason, "over-budget");
51+
assert.ok(calls.includes("prepare"), "prepared (free work) before the budget gate");
52+
assert.ok(!calls.includes("run-container"), "over budget => NO container, no money spent");
53+
});
54+
55+
test("container exit 0 => success, no retry", async () => {
56+
const { deps: d } = deps({ runContainer: async () => 0 });
57+
assert.equal((await runJob(ghJob, d)).outcome, "completed");
58+
});
59+
60+
test("container exit 2 => policy, RETURNS (not retried)", async () => {
61+
const { deps: d } = deps({ runContainer: async () => 2 });
62+
assert.equal((await runJob(ghJob, d)).outcome, "policy");
63+
});
64+
65+
test("container exit 1 => THROWS InfraRetry (BullMQ will retry)", async () => {
66+
const { deps: d } = deps({ runContainer: async () => 1 });
67+
await assert.rejects(() => runJob(ghJob, d), (e) => e instanceof InfraRetry && e.piDispatchRetry === true);
68+
});
69+
70+
test("an unknown exit code throws (retry-then-visible), never silent success", async () => {
71+
const { deps: d } = deps({ runContainer: async () => 137 });
72+
await assert.rejects(() => runJob(ghJob, d), InfraRetry);
73+
});
74+
75+
test("cleanup runs even when the container throws", async () => {
76+
const { deps: d, calls } = deps({ runContainer: async () => 1 });
77+
await runJob(ghJob, d).catch(() => {});
78+
assert.ok(calls.includes("cleanup"), "the job dir must be cleaned up on the infra path too");
79+
});
80+
81+
test("a local-folder job skips minting and branch-check entirely", async () => {
82+
const { deps: d, calls } = deps();
83+
const localJob = { kind: "local", folder: "/home/rob/proj", provider: "anthropic", model: "m", maxTurns: 5 };
84+
const r = await runJob(localJob, d);
85+
assert.equal(r.outcome, "completed");
86+
assert.ok(!calls.some((c) => c.startsWith("mint")), "no token for a local job");
87+
assert.ok(!calls.includes("branch-check"), "no branch check for a non-git folder");
88+
});

worker/test/wiring.test.mjs

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
import assert from "node:assert/strict";
2+
import { test } from "node:test";
3+
4+
// index.mjs imports bullmq, so this skips below the node floor / without deps and runs in CI,
5+
// where PI_DISPATCH_REQUIRE_WORKER_TESTS=1 turns a skip into a hard failure.
6+
let mod;
7+
let importError;
8+
try {
9+
mod = await import("../src/index.mjs");
10+
} catch (error) {
11+
importError = error;
12+
}
13+
if (!mod && process.env.PI_DISPATCH_REQUIRE_WORKER_TESTS === "1") {
14+
throw new Error(`worker wiring tests are REQUIRED here but bullmq could not import.\n${importError}`);
15+
}
16+
const skip = mod ? false : `bullmq not installed (node ${process.version} < 22.19.0); CI runs these`;
17+
18+
test("the processor declares arity 3 -- the silent trap that would disable the timeout", { skip }, () => {
19+
// BullMQ only allocates an AbortController when processor.length >= 3. If a refactor drops the
20+
// unused `token` param, the 30-minute timeout and shutdown abort silently stop working. This is
21+
// the single most important assertion in the worker's wiring, because nothing at runtime reports
22+
// its failure -- the container just runs unbounded.
23+
const processor = mod.makeProcessor({
24+
cancelJob: () => {},
25+
stopContainer: () => {},
26+
redis: {},
27+
cap: 10,
28+
deps: {},
29+
});
30+
assert.equal(processor.length, 3, "processor must declare (job, token, signal) or the abort dies");
31+
});
32+
33+
test("the timeout fires cancelJob after timeoutMs", { skip }, async () => {
34+
let cancelled = null;
35+
const processor = mod.makeProcessor({
36+
cancelJob: (id, reason) => (cancelled = { id, reason }),
37+
stopContainer: () => {},
38+
redis: { async incr() { return 1; }, async expire() {} },
39+
cap: 10,
40+
timeoutMs: 20,
41+
deps: {
42+
mintToken: async () => "t",
43+
isDefaultBranchProtected: async () => true,
44+
prepareWorkspace: async () => ({}),
45+
// a real container exits when docker stop runs; mirror that -- reject on abort.
46+
runContainer: ({ signal }) =>
47+
new Promise((_, reject) => signal.addEventListener("abort", () => reject(new Error("stopped")), { once: true })),
48+
cleanup: async () => {},
49+
comment: async () => {},
50+
},
51+
});
52+
53+
const ac = new AbortController();
54+
const job = { id: "j1", data: { kind: "github", repo: "o/r" } };
55+
const running = processor(job, "tok", ac.signal).catch(() => {});
56+
await new Promise((r) => setTimeout(r, 60));
57+
assert.equal(cancelled?.id, "j1");
58+
assert.equal(cancelled?.reason, "job-timeout-30m");
59+
ac.abort(); // let the hung runContainer's abort path settle
60+
await running;
61+
});
62+
63+
test("an abort stops the container", { skip }, async () => {
64+
let stopped = null;
65+
const processor = mod.makeProcessor({
66+
cancelJob: () => {},
67+
stopContainer: (name) => (stopped = name),
68+
redis: { async incr() { return 1; }, async expire() {} },
69+
cap: 10,
70+
timeoutMs: 100000,
71+
deps: {
72+
mintToken: async () => "t",
73+
isDefaultBranchProtected: async () => true,
74+
prepareWorkspace: async () => ({}),
75+
runContainer: ({ signal }) =>
76+
new Promise((_, reject) => signal.addEventListener("abort", () => reject(new Error("stopped")), { once: true })),
77+
cleanup: async () => {},
78+
comment: async () => {},
79+
},
80+
});
81+
const ac = new AbortController();
82+
const running = processor({ id: "j2", data: { kind: "github", repo: "o/r" } }, "tok", ac.signal).catch(() => {});
83+
// Let the job reach the running container before aborting -- in reality the container has been
84+
// up for minutes when the 30-min timeout fires.
85+
await new Promise((r) => setTimeout(r, 10));
86+
ac.abort();
87+
await new Promise((r) => setTimeout(r, 10));
88+
assert.equal(stopped, "pi-job-j2");
89+
await running;
90+
});

0 commit comments

Comments
 (0)