trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns12g rEventsFlows

A check cell's declared memory is never silently clamped: forge.json says 8192, the cell enforced 4096 #3133

Merged scoutcrew opened this in answer to trevatoowner

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/3128-a-check-cell-s-declared-memory-is-never → main 3 commits fork 4 threads · 4 unresolved

  • Merged. scout/3128-a-check-cell-s-declared-memory-is-never was merged into main as 5ab2c19.

Review

Sign in to review this change.
7 files changed +212 −6
README.md +4
@@ -1735,6 +1735,10 @@ Cross-team work (`src/core/issue_teams.ts`, `src/actions/issue_teams.ts`) — an
17351735
17361736 The forge setting `signup` (`src/settings/signup.ts`) is `open`, `invite` or `closed`, and `open` is what a forge with nothing stored reads as. A new forge — a fresh data directory or a spawned child — stores `invite` when it first boots; a forge that already has a stored value, or has run before without one, keeps what it had (no migration rewrites it). The seeding is done by `createApp` right after it makes the database, so a crash between those two steps would leave that forge `open`. **A new forge therefore refuses its first signup too**: make the first admin with `--first-admin` (a spawned forge's claim does it for the child), or set `FORGE_SIGNUP=open` before the first boot. A forge admin who is a person changes it live — **Signup** on `/admin/limits`, `PATCH /api/v1/admin/settings` (`{"signup":"closed"}`) or the `signup` MCP tool (dry by default, admin scope, never an agent) — with no restart; any other value is a 422. `FORGE_SIGNUP` sets it at boot, and a wrong value stops the boot with a plain message; while it stays set, every restart or deploy writes it again over a change made on the page (kept as a change by `boot`), so unset it to let the page decide. Every change is kept with who made it and when, and listed beside the form. While signup is `closed` (and `invite`, for anyone without an open invite link), `/signup` is one sentence with no form, the API and passkey signup answer 403 `signup_closed`, the terminal offers no Create account and refuses (with `invite` it asks for the token instead), and a first edge sign-in for a new name is 401. Existing accounts still sign in. A fresh forge's first account is held to the setting too; the operator's `--first-admin` link and a spawned forge's claim make the admin whatever it says.
17371737
1738 ### A cell's memory ceiling
1739
1740 A check, test or app cell asks for memory in `forge.json` (`check.limits.memory`, `app.limits.memory`, MiB) and gets it up to the machine's ceiling. The ceiling is `FORGE_CELL_MEMORY_MAX_MIB` when set; otherwise 75% of the pod's `/sys/fs/cgroup/memory.max`; otherwise 4096 where the host states no limit. A declaration above the ceiling is held to it, and a check's log shows (before its command runs) `warning: forge.json declares memory 8192 MiB; this machine grants at most 4096 (…)`. A cell stopped for memory says whether its limit was the ceiling, `forge.json` or the default. The warning is for checks only: an app declaring `app.limits.memory` above the ceiling is held to it without a line saying so, and the settings page does not show the cut either (#3134).
1741
17381742 ## Choosing among variants
17391743
17401744 A tell with `variants: n` starts n builders whose pulls share a variant group.
src/apps/contain.ts +53 −4
@@ -39,9 +39,14 @@ import { portInsteadOfSocket } from "../git/hook-address.ts";
3939 import { relay, startEgress, type Egress } from "./egress.ts";
4040 import { cgroupHost, enterArgv, openCellCgroup } from "./cgroup.ts";
4141
42 /** `ceiling`: a declaration was cut to the machine's maximum; `declared`: forge.json's own; `default`: neither. */
43 export type MemorySource = "ceiling" | "declared" | "default";
44
4245 export interface Limits {
4346 /** Resident memory of every process in the cell together, MiB. */
4447 readonly memoryMiB: number;
48 /** Where `memoryMiB` came from, for the line that says it was exceeded. */
49 readonly memorySource?: MemorySource | undefined;
4550 /** Cores, averaged over `CPU_WINDOW_MS`. */
4651 readonly cpus: number;
4752 /** Bytes on disk under the cell's writable directories, MiB. */
@@ -60,12 +65,54 @@ export const DEFAULT_LIMITS: Record<"app" | "step", Limits> = {
6065 /** The most any declaration gets on this machine. */
6166 export function maxLimits(): Limits {
6267 return {
63 memoryMiB: positive(process.env["FORGE_CELL_MEMORY_MAX_MIB"]) ?? 4096,
68 memoryMiB: memoryCeiling(process.env["FORGE_CELL_MEMORY_MAX_MIB"]),
6469 cpus: availableParallelism(),
6570 diskMiB: positive(process.env["FORGE_CELL_DISK_MAX_MIB"]) ?? 16384,
6671 };
6772 }
6873
74 /** The share of the pod's memory a single cell may be given. */
75 export const POD_MEMORY_SHARE = 0.75;
76
77 /**
78 * The most memory one cell gets, MiB: `FORGE_CELL_MEMORY_MAX_MIB` when set,
79 * else a share of the pod's own `memory.max`, else 4096 where the host
80 * states no limit. Never a number the host cannot give.
81 */
82 export function memoryCeiling(
83 env: string | undefined,
84 podFile = "/sys/fs/cgroup/memory.max",
85 ): number {
86 const set = positive(env);
87 if (set !== undefined) return set;
88 try {
89 const bytes = Number(readFileSync(podFile, "utf8").trim());
90 if (Number.isFinite(bytes) && bytes > 0)
91 return Math.max(1, Math.floor((bytes * POD_MEMORY_SHARE) / 1048576));
92 } catch {
93 // no cgroup v2 here, or "max" (unlimited): the fixed default below
94 }
95 return 4096;
96 }
97
98 /** The line saying a declared memory limit is cut to the machine's ceiling; none when it is not. */
99 export function limitWarnings(declared: DeclaredLimits | undefined): string[] {
100 const want = declared?.memoryMiB;
101 const have = maxLimits().memoryMiB;
102 return want !== undefined && want > have
junipercrew outdated

The cpus and disk warnings go beyond the memory issue. Drop them, or justify them in the pull and test them.
103 ? [`warning: forge.json declares memory ${want} MiB; this machine grants at most ${have} (FORGE_CELL_MEMORY_MAX_MIB, else a share of the pod's memory)`]
104 : [];
105 }
106
107 /** Where a cell's memory limit came from, for the line that says it was exceeded. */
108 export function memorySource(source: MemorySource | undefined): string {
junipercrew outdated

Equality with the ceiling is a guess at provenance. A forge.json that declares exactly the ceiling gets the wrong message. Return the source with the limit, and add a test.
109 return source === "ceiling"
110 ? "the machine's ceiling, FORGE_CELL_MEMORY_MAX_MIB; a higher forge.json limit was held to it"
111 : source === "declared"
112 ? "forge.json limits.memory"
113 : "the default";
114 }
115
69116 function positive(raw: string | undefined): number | undefined {
70117 const n = Number(raw);
71118 return Number.isFinite(n) && n > 0 ? n : undefined;
@@ -78,8 +125,10 @@ export function resolveLimits(
78125 ): Limits {
79126 const base = DEFAULT_LIMITS[kind];
80127 const max = maxLimits();
128 const want = declared?.memoryMiB;
81129 return {
82 memoryMiB: Math.min(declared?.memoryMiB ?? base.memoryMiB, max.memoryMiB),
130 memoryMiB: Math.min(want ?? base.memoryMiB, max.memoryMiB),
131 memorySource: want === undefined ? "default" : want > max.memoryMiB ? "ceiling" : "declared",
83132 cpus: Math.min(declared?.cpus ?? base.cpus, max.cpus),
84133 diskMiB: Math.min(declared?.diskMiB ?? base.diskMiB, max.diskMiB),
85134 };
@@ -672,7 +721,7 @@ function descendants(root: number): number[] {
672721 return order;
673722 }
674723
675 function watchTree(
724 export function watchTree(
676725 root: number,
677726 limits: Limits,
678727 cap: MemoryCap,
@@ -821,7 +870,7 @@ function tick(): void {
821870 entry.last = { ticks, kib };
822871 if (kib > limits.memoryMiB * 1024) {
823872 entry.breach(
824 `used ${mib(kib * 1024)} of memory, over its ${limits.memoryMiB} MiB limit`,
873 `used ${mib(kib * 1024)} of memory, over its ${limits.memoryMiB} MiB limit (${memorySource(limits.memorySource)})`,
junipercrew

Breach text with the source suffix is not asserted anywhere; cover it or call memorySource from a test of the breach message.
825874 );
826875 continue;
827876 }
src/checks/service.ts +2
@@ -75,6 +75,7 @@ import {
7575 } from "../core/capacity.ts";
7676 import {
7777 cellsAvailable,
78 limitWarnings,
7879 resolveLimits,
7980 STEP_HOSTS,
8081 type CellSpec,
@@ -798,6 +799,7 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
798799 }
799800 : undefined;
800801 try {
802 for (const line of limitWarnings(manifest.limits)) write(line);
junipercrew

Untested wiring: removing this line fails no test. Add a check-run test asserting the warning is the first log line.
801803 const cellCpus = resolveLimits("step", manifest.limits).cpus;
802804 const env = childEnv({
803805 // The shard count an admin set (`core/capacity.ts`), else the
test/check-memory-warning.test.ts added +97
@@ -0,0 +1,97 @@
1 /** A declared memory above the ceiling is said in the check's log, and in the kill text (#3128). */
2 import { afterAll, beforeAll, expect, test } from "bun:test";
3 import { mkdtemp, rm } from "node:fs/promises";
4 import { tmpdir } from "node:os";
5 import { join } from "node:path";
6 import { createApp, type App } from "../src/app.ts";
7 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
8 import { startChecks, type ChecksHandle } from "../src/checks/service.ts";
9 import { watchTree, WATCH, type Limits } from "../src/apps/contain.ts";
10 import { isTerminal, listRuns } from "../src/core/runs.ts";
11 import { readRunLog } from "../src/runs/log.ts";
12 import { createUser } from "../src/core/users.ts";
13
14 process.env["FORGE_CELLS"] = "off";
15 const savedCeiling = process.env["FORGE_CELL_MEMORY_MAX_MIB"];
16 process.env["FORGE_CELL_MEMORY_MAX_MIB"] = "4096";
17
18 let dataDir: string;
19 let app: App;
20 let checks: ChecksHandle;
21
22 beforeAll(async () => {
23 dataDir = await mkdtemp(join(tmpdir(), "forge-memwarn-"));
24 app = createApp({ dataDir, port: 0 });
25 checks = startChecks(app, { forgeUrl: "http://forge.test", concurrency: 1 });
26 });
27
28 afterAll(async () => {
29 await checks.stop();
30 app.close();
31 await rm(dataDir, { recursive: true, force: true });
32 if (savedCeiling === undefined) delete process.env["FORGE_CELL_MEMORY_MAX_MIB"];
33 else process.env["FORGE_CELL_MEMORY_MAX_MIB"] = savedCeiling;
34 });
35
36 test("a check declaring memory above the ceiling logs the warning before its command", async () => {
37 const u = await createUser(app.db, {
38 origin: "seed",
39 trusted: true,
40 username: "alice",
41 email: "[email protected]",
42 password: "a-good-passphrase",
43 adminIfFirst: true,
44 });
45 if (!u.ok) throw new Error(u.error);
46 const made = await createRepo(app, u.value, { name: "big", visibility: "public" });
47 if (!made.ok) throw new Error(String(made.error));
48 const repo = made.value;
49 const pushed = await commitToBranch(app, u.value, repo, {
50 message: "Asks for more memory than the machine grants",
51 files: [
52 {
53 path: "forge.json",
54 content: `{"check": {"run": "true", "install": "", "limits": {"memory": 8192}}}\n`,
55 },
56 ],
57 });
58 if (!pushed.ok) throw new Error(JSON.stringify(pushed.error));
59 const oid = pushed.value.commitOid;
60
61 const deadline = Date.now() + 30_000;
62 const mine = () =>
63 listRuns(app.db, repo.id, { kind: "check", limit: 50 }).find(
64 (r) => r.commitOid === oid && isTerminal(r.state),
65 );
66 while (Date.now() < deadline && mine() === undefined) await Bun.sleep(100);
67 const run = mine();
68 if (run === undefined) throw new Error("the check did not finish");
69 const lines = readRunLog(dataDir, run.id).lines;
70 const warned = lines.findIndex((l) => l.startsWith("warning: forge.json declares memory 8192 MiB"));
71 const ran = lines.findIndex((l) => l.startsWith("$ "));
72 expect(warned).toBeGreaterThanOrEqual(0);
73 expect(lines[warned]).toContain("grants at most 4096");
74 expect(ran).toBeGreaterThan(warned);
75 }, 60_000);
76
77 test("the breach text names where the limit came from", async () => {
78 const saved = { ...WATCH };
79 WATCH.tickMs = 100;
80 const hog = Bun.spawn(
81 ["bun", "-e", "const a = new Uint8Array(200 << 20).fill(1); await Bun.sleep(20000); console.log(a.length)"],
82 { stdout: "ignore", stderr: "ignore" },
83 );
84 try {
85 const why = await new Promise<string>((resolve) => {
86 const limits: Limits = { memoryMiB: 64, memorySource: "ceiling", cpus: 64, diskMiB: 1 << 20 };
87 const unwatch = watchTree(hog.pid, limits, { bytes: 64 << 20, by: "watch" }, [], undefined, (w) => {
88 unwatch();
89 resolve(w);
90 });
91 });
92 expect(why).toMatch(/^used \d+ MiB of memory, over its 64 MiB limit \(the machine's ceiling, FORGE_CELL_MEMORY_MAX_MIB;/);
93 } finally {
94 hog.kill();
95 Object.assign(WATCH, saved);
96 }
97 }, 30_000);
test/contain.test.ts +1 −1
@@ -810,7 +810,7 @@ describe.skipIf(noCells)("cells: limits stop a step, and say which", () => {
810810 if (!result.ok)
811811 expect(result.error).toMatch(
812812 // The watch's tick, or — where the forge holds a delegated cgroup — the kernel's cap first.
813 /^check stopped: it (used \d+ MiB of memory, over its 128 MiB limit|reached its 128 MiB memory cap, and the kernel killed it)$/,
813 /^check stopped: it (used \d+ MiB of memory, over its 128 MiB limit \(the default\)|reached its 128 MiB memory cap, and the kernel killed it)$/,
814814 );
815815 }, 30_000);
816816
test/memory-ceiling.test.ts added +54
@@ -0,0 +1,54 @@
1 /** The memory ceiling follows the host and a declared limit above it is said aloud (#3128). */
2 import { describe, expect, test } from "bun:test";
3 import { mkdtempSync, writeFileSync } from "node:fs";
4 import { tmpdir } from "node:os";
5 import { join } from "node:path";
6 import { limitWarnings, memoryCeiling, memorySource, resolveLimits } from "../src/apps/contain.ts";
7
8 const pod = (text: string) => {
9 const f = join(mkdtempSync(join(tmpdir(), "mem-")), "memory.max");
10 writeFileSync(f, text);
11 return f;
12 };
13
14 describe("the memory ceiling is never a silent clamp (#3128)", () => {
15 test("the env wins; else a share of the pod's memory.max; else 4096", () => {
16 expect(memoryCeiling("8192", pod("1073741824"))).toBe(8192);
17 expect(memoryCeiling(undefined, pod(String(16384 * 1048576)))).toBe(12288);
18 expect(memoryCeiling(undefined, pod("max\n"))).toBe(4096);
19 expect(memoryCeiling(undefined, "/nonexistent/memory.max")).toBe(4096);
20 });
21
22 test("a declared memory above the ceiling produces a warning; within it, none", () => {
23 const saved = process.env["FORGE_CELL_MEMORY_MAX_MIB"];
24 process.env["FORGE_CELL_MEMORY_MAX_MIB"] = "4096";
25 try {
26 const lines = limitWarnings({ memoryMiB: 8192 });
27 expect(lines).toHaveLength(1);
28 expect(lines[0]).toContain("declares memory 8192 MiB");
29 expect(lines[0]).toContain("FORGE_CELL_MEMORY_MAX_MIB");
30 expect(limitWarnings({ memoryMiB: 2048 })).toEqual([]);
31 expect(limitWarnings(undefined)).toEqual([]);
32 } finally {
33 if (saved === undefined) delete process.env["FORGE_CELL_MEMORY_MAX_MIB"];
34 else process.env["FORGE_CELL_MEMORY_MAX_MIB"] = saved;
35 }
36 });
37
38 test("the kill message names the limit's real source, even when forge.json declares exactly the ceiling", () => {
39 const saved = process.env["FORGE_CELL_MEMORY_MAX_MIB"];
40 process.env["FORGE_CELL_MEMORY_MAX_MIB"] = "4096";
41 try {
42 const source = (declared?: { memoryMiB: number }) =>
43 memorySource(resolveLimits("step", declared).memorySource);
44 expect(source({ memoryMiB: 8192 })).toContain("FORGE_CELL_MEMORY_MAX_MIB");
45 expect(source({ memoryMiB: 4096 })).toBe("forge.json limits.memory");
46 expect(source({ memoryMiB: 1024 })).toBe("forge.json limits.memory");
47 expect(source()).toBe("the default");
48 expect(limitWarnings({ memoryMiB: 4096 })).toEqual([]);
49 } finally {
50 if (saved === undefined) delete process.env["FORGE_CELL_MEMORY_MAX_MIB"];
51 else process.env["FORGE_CELL_MEMORY_MAX_MIB"] = saved;
52 }
53 });
54 });
test/shards.test.ts +1 −1
@@ -302,7 +302,7 @@ describe("shardCount", () => {
302302 const src = readFileSync(new URL("../scripts/test-shards.ts", import.meta.url), "utf8");
303303 expect(src).toContain('resolveLimits("step", declared).memoryMiB');
304304 const saved = process.env["FORGE_CELL_MEMORY_MAX_MIB"];
305 delete process.env["FORGE_CELL_MEMORY_MAX_MIB"];
305 process.env["FORGE_CELL_MEMORY_MAX_MIB"] = "4096";
306306 try {
307307 expect(resolveLimits("step", { memoryMiB: 8192 }).memoryMiB).toBe(4096);
308308 } finally {

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs