Public: anyone can read this; the crew can change it.
A check never trips its own CPU guard: test shards follow the cell's CPU limit, not the host's #2260
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/2257-a-check-never-trips-its-own-cpu-guard-te → main 2 commits fork 3 threads · 3 unresolved
5 files
changed +87
−3
scripts/shards.ts +23 −1
| @@ -31,15 +31,23 @@ export const WORKER_MEMORY_BYTES = 512 * 1024 * 1024; | ||
| 31 | 31 | * not fewer: most of a forge test's time is spent waiting on git children |
| 32 | 32 | * and timers, not computing. `memory` absent is not a bound. |
| 33 | 33 | * `FORGE_CHECK_SHARDS` overrides it (still capped at the file count) so a |
| 34 | * small or busy host can turn it down. | |
| 34 | * small or busy host can turn it down; that is an explicit operator | |
| 35 | * choice and wins over the cell cap below. `cellCpus` (`FORGE_CELL_CPUS`, the | |
| 36 | * CPU limit of the cell the check runs in) caps the count at one less than | |
| 37 | * the limit, since the runner itself needs a core and the cell's guard | |
| 38 | * kills a check that uses more than its limit; unset or not a whole number | |
| 39 | * from 1 is no limit. | |
| 35 | 40 | */ |
| 36 | 41 | export function shardCount( |
| 37 | 42 | cpus: number, |
| 38 | 43 | files: number, |
| 39 | 44 | override?: string, |
| 40 | 45 | memory?: number, |
| 46 | cellCpus?: string, | |
| 41 | 47 | ): { ok: true; value: number } | { ok: false; error: string } { |
| 42 | 48 | let n = Math.min(SHARDS_MAX, Math.floor(cpus)); |
| 49 | if (cellCpus !== undefined && /^[1-9][0-9]{0,3}$/.test(cellCpus.trim())) | |
| 50 | n = Math.min(n, Number(cellCpus.trim()) - 1); | |
| 43 | 51 | if (memory !== undefined && Number.isFinite(memory) && memory > 0) |
| 44 | 52 | n = Math.min(n, Math.floor(memory / WORKER_MEMORY_BYTES)); |
| 45 | 53 | if (override !== undefined && override.trim() !== "") { |
| @@ -53,6 +61,20 @@ export function shardCount( | ||
| 53 | 61 | return { ok: true, value: Math.max(1, Math.min(n, files)) }; |
| 54 | 62 | } |
| 55 | 63 | |
| 64 | /** | |
| 65 | * {@link shardCount} from the process environment: `FORGE_CHECK_SHARDS` | |
| 66 | * (the override) and `FORGE_CELL_CPUS` (the cell's CPU limit, which the | |
| 67 | * forge sets for a check in `src/checks/service.ts`). | |
| 68 | */ | |
| 69 | export function shardCountFromEnv( | |
| 70 | env: Readonly<Record<string, string | undefined>>, | |
| 71 | cpus: number, | |
| 72 | files: number, | |
| 73 | memory?: number, | |
| 74 | ): { ok: true; value: number } | { ok: false; error: string } { | |
| 75 | return shardCount(cpus, files, env["FORGE_CHECK_SHARDS"], memory, env["FORGE_CELL_CPUS"]); | |
| 76 | } | |
| 77 | ||
| 56 | 78 | /** |
| 57 | 79 | * The memory a process may use: the least of the machine's and each |
| 58 | 80 | * cgroup limit it can read (`memory.max` under v2, `memory.limit_in_bytes` |
scripts/test-shards.ts +2 −2
| @@ -52,7 +52,7 @@ import { | ||
| 52 | 52 | parseTimes, |
| 53 | 53 | SHARDS_MAX, |
| 54 | 54 | WORKER_MEMORY_BYTES, |
| 55 | shardCount, | |
| 55 | shardCountFromEnv, | |
| 56 | 56 | sliceFiles, |
| 57 | 57 | splitArgs, |
| 58 | 58 | splitFiles, |
| @@ -345,7 +345,7 @@ const memory = memoryLimit(totalmem(), [ | ||
| 345 | 345 | readLimit("/sys/fs/cgroup/memory.max"), |
| 346 | 346 | readLimit("/sys/fs/cgroup/memory/memory.limit_in_bytes"), |
| 347 | 347 | ]); |
| 348 | const count = shardCount(cpus, files.length, shardsOverride, memory); | |
| 348 | const count = shardCountFromEnv(process.env, cpus, files.length, memory); | |
outdated
The pass-through of FORGE_CELL_CPUS is untested. Cover it, or move the env reading into a small tested function. |
||
| 349 | 349 | if (!count.ok) { |
| 350 | 350 | console.error(`test-shards: ${count.error}`); |
| 351 | 351 | process.exit(2); |
src/checks/service.ts +3
| @@ -792,6 +792,9 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle { | ||
| 792 | 792 | FORGE_CHECK: run.id, |
| 793 | 793 | FORGE_COMMIT: x.commitOid, |
| 794 | 794 | FORGE_CHECK_IMPACT: IMPACT_FILE, |
| 795 | // The cell's CPU limit, which `availableParallelism()` may not | |
| 796 | // report inside it; the shard count follows this (#2257). | |
| 797 | FORGE_CELL_CPUS: String(resolveLimits("step", manifest.limits).cpus), | |
Nothing fails if this line is removed. Add a test asserting the step env carries FORGE_CELL_CPUS equal to the resolved step limit. |
||
| 795 | 798 | ...(base === undefined |
| 796 | 799 | ? {} |
| 797 | 800 | : { |
test/check-tiers.test.ts +40
| @@ -35,6 +35,7 @@ import { | ||
| 35 | 35 | parseImpact, |
| 36 | 36 | reach, |
| 37 | 37 | } from "../src/checks/impact.ts"; |
| 38 | import { resolveLimits } from "../src/apps/contain.ts"; | |
| 38 | 39 | import { parseCheck } from "../src/apps/manifest.ts"; |
| 39 | 40 | import { IMPACT_FILE, keepImpact, seedImpact } from "../src/checks/impact-store.ts"; |
| 40 | 41 | |
| @@ -510,6 +511,45 @@ describe("the tiers in the forge's own checks service", () => { | ||
| 510 | 511 | expect(again.lines.join("\n")).not.toContain("FULL-GREEN"); |
| 511 | 512 | }); |
| 512 | 513 | |
| 514 | test("a check's environment carries its cell's CPU limit (#2257)", async () => { | |
| 515 | const seeded = await commitToBranch(app, alice, repo, { | |
| 516 | message: "Declare check limits", | |
| 517 | files: [ | |
| 518 | { | |
| 519 | path: "forge.json", | |
| 520 | content: JSON.stringify({ | |
| 521 | check: { run: "echo WHOLE", fast: 'echo "CELL-CPUS=$FORGE_CELL_CPUS"', limits: { cpus: 3 } }, | |
| 522 | }), | |
| 523 | }, | |
| 524 | ], | |
| 525 | }); | |
| 526 | if (!seeded.ok) throw new Error(JSON.stringify(seeded.error)); | |
| 527 | const branched = await commitToBranch(app, alice, repo, { | |
| 528 | branch: "cpu-limit", | |
| 529 | message: "Touch d", | |
| 530 | files: [{ path: "d.txt", content: "d\n" }], | |
| 531 | }); | |
| 532 | if (!branched.ok) throw new Error(JSON.stringify(branched.error)); | |
| 533 | const opened = await openPull(app, alice, repo, { | |
| 534 | title: "Limits", | |
| 535 | baseBranch: "main", | |
| 536 | headBranch: "cpu-limit", | |
| 537 | }); | |
| 538 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 539 | const deadline = Date.now() + 30_000; | |
| 540 | let run: Run | undefined; | |
| 541 | while (Date.now() < deadline) { | |
| 542 | run = listRuns(app.db, repo.id, { kind: "check", limit: 20 }).find( | |
| 543 | (r) => r.issueId === opened.value.id && r.commitOid === branched.value.commitOid, | |
| 544 | ); | |
| 545 | if (run && (run.state === "ok" || run.state === "failed")) break; | |
| 546 | await Bun.sleep(50); | |
| 547 | } | |
| 548 | expect(run?.state).toBe("ok"); | |
| 549 | const want = resolveLimits("step", { cpus: 3 }).cpus; | |
| 550 | expect(readRunLog(dataDir, run!.id).lines.join("\n")).toContain(`CELL-CPUS=${want}`); | |
| 551 | }); | |
| 552 | ||
| 513 | 553 | test("a red full gate names its tests and the pull learns the files", async () => { |
| 514 | 554 | const red = await commitToBranch(app, alice, repo, { |
| 515 | 555 | message: "Full goes red", |
test/shards.test.ts +19
| @@ -19,6 +19,7 @@ import { | ||
| 19 | 19 | refreshTimes, |
| 20 | 20 | parseShardSpec, |
| 21 | 21 | shardCount, |
| 22 | shardCountFromEnv, | |
| 22 | 23 | sliceFiles, |
| 23 | 24 | splitArgs, |
| 24 | 25 | splitFiles, |
| @@ -221,6 +222,24 @@ describe("shardCount", () => { | ||
| 221 | 222 | expect(shardCount(80, 3)).toEqual({ ok: true, value: 3 }); |
| 222 | 223 | }); |
| 223 | 224 | |
| 225 | test("the cell's CPU limit caps the shards below it, whatever the host reports (#2257)", () => { | |
| 226 | expect(shardCount(80, 400, undefined, undefined, "4")).toEqual({ ok: true, value: 3 }); | |
| 227 | expect(shardCount(80, 400, undefined, undefined, "16")).toEqual({ ok: true, value: 15 }); | |
| 228 | expect(shardCount(80, 2, undefined, undefined, "16")).toEqual({ ok: true, value: 2 }); | |
| 229 | expect(shardCount(80, 400, undefined, undefined, "1")).toEqual({ ok: true, value: 1 }); | |
| 230 | expect(shardCount(2, 400, undefined, undefined, "16")).toEqual({ ok: true, value: 2 }); | |
| 231 | // Unset, empty or nonsense: as today. | |
| 232 | expect(shardCount(80, 400, undefined, undefined, undefined)).toEqual({ ok: true, value: SHARDS_MAX }); | |
| 233 | expect(shardCount(80, 400, undefined, undefined, "")).toEqual({ ok: true, value: SHARDS_MAX }); | |
| 234 | expect(shardCount(80, 400, undefined, undefined, "lots")).toEqual({ ok: true, value: SHARDS_MAX }); | |
| 235 | }); | |
| 236 | ||
| 237 | test("the environment's FORGE_CELL_CPUS caps the count; FORGE_CHECK_SHARDS still overrides it (#2257)", () => { | |
| 238 | expect(shardCountFromEnv({ FORGE_CELL_CPUS: "4" }, 80, 400)).toEqual({ ok: true, value: 3 }); | |
| 239 | expect(shardCountFromEnv({}, 80, 400)).toEqual({ ok: true, value: SHARDS_MAX }); | |
| 240 | expect(shardCountFromEnv({ FORGE_CELL_CPUS: "4", FORGE_CHECK_SHARDS: "6" }, 80, 400)).toEqual({ ok: true, value: 6 }); | |
| 241 | }); | |
| 242 | ||
| 224 | 243 | test("the check cell's 16 CPUs and 8 GiB run 16 at once; less memory runs fewer (#2038)", () => { |
| 225 | 244 | const GiB = 1024 * 1024 * 1024; |
| 226 | 245 | expect(SHARDS_MAX).toBeGreaterThanOrEqual(16); |
new above
offline
Next: a small test that runs the script path, or a check that test-shards.ts really calls shardCountFromEnv(process.env, ...). Today removing process.env there passes every test.