Public: anyone can read this; the crew can change it.
A roll does not throw away running checks: drain or hand them over before the forge restarts #1820
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/1796-a-roll-does-not-throw-away-running-check → main 1 commit fork 3 threads · 3 unresolved
4 files
changed +83
−1
docs/APPS.md +1
| @@ -1065,6 +1065,7 @@ a link to the milestone (`docs/PLANNING.md`); its progress never does. | ||
| 1065 | 1065 | | `FORGE_PREVIEW_IDLE_DAYS` | `3` | days a pull may sit untouched before its preview sleeps; `0` = never | |
| 1066 | 1066 | | `FORGE_CREW_CONCURRENCY` | `2` | agent runs at once (live: Settings → Capacity) | |
| 1067 | 1067 | | `FORGE_CHECKS_CONCURRENCY` | `1` | checks at once (live: Settings → Capacity) | |
| 1068 | | `FORGE_CHECKS_DRAIN_S` | `300` | seconds a roll lets running checks finish first | | |
| 1068 | 1069 | | `FORGE_CHECK_SHARDS` | the machine's cores | test shards per check (live: Settings → Capacity) | |
| 1069 | 1070 | | `FORGE_CELLS_EGRESS` | `enforce` | `report` lets undeclared egress through on 80/443 (live: Settings → Capacity) | |
| 1070 | 1071 | | `FORGE_CREW_BUDGET_USD` | `5` | spend per run unless the agent says otherwise | |
src/checks/service.ts +28 −1
| @@ -23,6 +23,7 @@ import { issueMayRun } from "../core/pull_trust.ts"; | ||
| 23 | 23 | import { getRepoById, type Repo } from "../core/repos.ts"; |
| 24 | 24 | import { |
| 25 | 25 | cancelRun, |
| 26 | getRun, | |
Import order: getRun sits between cancelRun and cancelRunsForPull; keep the list alphabetical. |
||
| 26 | 27 | cancelRunsForPull, |
| 27 | 28 | checksForHead, |
| 28 | 29 | claimRun, |
| @@ -77,6 +78,12 @@ export interface ChecksOptions { | ||
| 77 | 78 | readonly forgeUrl: string; |
| 78 | 79 | /** How long a check may run before it is killed. @default BUDGETS.gate */ |
| 79 | 80 | readonly budgetMs?: number | undefined; |
| 81 | /** | |
| 82 | * How long `stop` lets checks already running finish before it cuts them | |
| 83 | * off and queues them again (#1796). Nothing new starts meanwhile. | |
| 84 | * @default 0 | |
| 85 | */ | |
| 86 | readonly drainMs?: number | undefined; | |
| 80 | 87 | } |
| 81 | 88 | |
| 82 | 89 | export interface ChecksHandle { |
| @@ -400,6 +407,12 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle { | ||
| 400 | 407 | if (failed) announceRun(app, failed); |
| 401 | 408 | return; |
| 402 | 409 | } |
| 410 | // Ended from outside while it ran: its pull landed or closed, and | |
| 411 | // the row says so. That is not this check failing. | |
| 412 | if (!outcome.ok && getRun(app.db, run.id)?.state === "cancelled") { | |
| 413 | write(`superseded: ${getRun(app.db, run.id)?.summary ?? "cancelled"}`); | |
| 414 | return; | |
| 415 | } | |
| 403 | 416 | if (!outcome.ok && stopped && signal.aborted) { |
| 404 | 417 | // Cut off by this forge stopping, not by the check: the same check |
| 405 | 418 | // goes back on the queue, and the forge that carries on runs it. |
| @@ -844,9 +857,23 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle { | ||
| 844 | 857 | stopped = true; |
| 845 | 858 | clearInterval(timer); |
| 846 | 859 | unsubscribe(); |
| 860 | // A roll waits for the checks under way, up to the budget, and only | |
| 861 | // then cuts off what is left (it is queued again, not lost). | |
| 862 | const running = Promise.all([...inFlight.values()].map((f) => f.promise)); | |
| 863 | const drainMs = options.drainMs ?? 0; | |
| 864 | if (drainMs > 0 && inFlight.size > 0) { | |
| 865 | let timer: ReturnType<typeof setTimeout> | undefined; | |
| 866 | await Promise.race([ | |
| 867 | running, | |
| 868 | new Promise<void>((resolve) => { | |
| 869 | timer = setTimeout(resolve, drainMs); | |
| 870 | }), | |
This inner |
||
| 871 | ]); | |
| 872 | clearTimeout(timer); | |
| 873 | } | |
| 847 | 874 | for (const { abort } of inFlight.values()) abort.abort(); |
| 848 | 875 | for (const abort of gates) abort.abort(); |
| 849 | await Promise.all([...inFlight.values()].map((f) => f.promise)); | |
| 876 | await running; | |
| 850 | 877 | }, |
| 851 | 878 | }; |
| 852 | 879 | handles.set(app, handle); |
src/main.ts +2
| @@ -277,6 +277,8 @@ const checks = | ||
| 277 | 277 | : startChecks(app, { |
| 278 | 278 | forgeUrl, |
| 279 | 279 | concurrency: positive(process.env.FORGE_CHECKS_CONCURRENCY, 1), |
| 280 | // A roll lets running checks finish this long before cutting them off. | |
| 281 | drainMs: positive(process.env.FORGE_CHECKS_DRAIN_S, 300) * 1000, | |
| 280 | 282 | }); |
| 281 | 283 | // The look: a picture of every environment that comes up green, taken |
| 282 | 284 | // through this forge's own listener. |
test/checks.test.ts +52
| @@ -727,6 +727,58 @@ describe("checks", () => { | ||
| 727 | 727 | expect(done.state).toBe("ok"); |
| 728 | 728 | }, 60_000); |
| 729 | 729 | |
| 730 | async function slowCheck(command: string, message: string): Promise<string> { | |
| 731 | const made = await commitToBranch(app, alice, repo, { | |
| 732 | message, | |
| 733 | files: [{ path: "forge.json", content: checkJson(command) }], | |
| 734 | }); | |
| 735 | if (!made.ok) throw new Error(JSON.stringify(made.error)); | |
| 736 | const oid = made.value.commitOid; | |
| 737 | const deadline = Date.now() + 20_000; | |
| 738 | let found: Run | undefined; | |
| 739 | while (Date.now() < deadline && found?.state !== "running") { | |
| 740 | found = listRuns(app.db, repo.id, { kind: "check", limit: 50 }).find( | |
| 741 | (r) => r.commitOid === oid, | |
| 742 | ); | |
| 743 | await Bun.sleep(50); | |
| 744 | } | |
| 745 | expect(found?.state).toBe("running"); | |
| 746 | return found!.id; | |
| 747 | } | |
| 748 | ||
| 749 | test("a roll waits for a running check to finish, within its budget, instead of throwing it away", async () => { | |
| 750 | await checks.stop(); | |
| 751 | checks = startChecks(app, { | |
| 752 | forgeUrl: servers.url, | |
| 753 | concurrency: 1, | |
| 754 | drainMs: 30_000, | |
| 755 | }); | |
| 756 | const id = await slowCheck("sleep 2 && test -f OK.md", "A drained check"); | |
| 757 | await checks.stop(); | |
| 758 | const run = getRun(app.db, id); | |
| 759 | expect(run?.state).toBe("ok"); | |
| 760 | expect( | |
| 761 | listRuns(app.db, repo.id, { kind: "check", limit: 50 }).some( | |
| 762 | (r) => r.state === "queued", | |
| 763 | ), | |
| 764 | ).toBe(false); | |
| 765 | checks = startChecks(app, { forgeUrl: servers.url, concurrency: 1 }); | |
| 766 | }, 60_000); | |
| 767 | ||
| 768 | test("a check whose pull lands while it runs ends as superseded, not failed", async () => { | |
| 769 | const id = await slowCheck("sleep 2 && false", "A check overtaken"); | |
| 770 | cancelRun(app.db, id, "pull landed"); | |
| 771 | const deadline = Date.now() + 20_000; | |
| 772 | let lines = ""; | |
| 773 | while (Date.now() < deadline && !lines.includes("superseded")) { | |
| 774 | await Bun.sleep(100); | |
| 775 | lines = readRunLog(dataDir, id).lines.join("\n"); | |
| 776 | } | |
| 777 | expect(lines).toContain("superseded: pull landed"); | |
| 778 | expect(lines).not.toContain("failed:"); | |
| 779 | expect(getRun(app.db, id)?.state).toBe("cancelled"); | |
| 780 | }, 60_000); | |
| 781 | ||
| 730 | 782 | test("a check that is declared wrong fails with the reason", async () => { |
| 731 | 783 | const wrong = await commitToBranch(app, alice, repo, { |
| 732 | 784 | message: "A wrong check", |
new above
offline
The table row is misaligned with its neighbours; pad it to match.