trevato/forge

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

More
Forks1g fMapg aRuns14g rEventsFlowsAPI

Check run homes (/data/checks/*.home) are removed when their run ends or dies, not only at the next boot #7550

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/7545-check-run-homes-data-checks-home-are-rem → main 3 commits fork 2 threads · 2 unresolved

  • Merged. scout/7545-check-run-homes-data-checks-home-are-rem was merged into main as e976fa5.

Review

Sign in to review this change.
2 files changed +146 −5
src/checks/service.ts +40 −5
@@ -12,7 +12,7 @@
1212 */
1313
1414 import { pullDir, runDir } from "../core/pulls.ts";
15 import { existsSync, mkdirSync, readFileSync, readdirSync, rmSync } from "node:fs";
15 import { chmodSync, existsSync, lstatSync, mkdirSync, readFileSync, readdirSync, rmSync } from "node:fs";
1616 import { tmpdir } from "node:os";
1717 import { join } from "node:path";
1818 import type { App } from "../app.ts";
@@ -285,9 +285,43 @@ export function requeueAbandonedChecks(app: App, staleMs: number): number {
285285 appendRunLog(app.config.dataDir, run.id, run.summary);
286286 announceRun(app, run);
287287 }
288 // On the same timer: what a dead or lapsed run left on the disk.
289 sweepCheckouts(app);
junipercrew

This call is the fix, and it is untested. Add a test that goes through requeueAbandonedChecks.
288290 return swept.requeued.length + swept.failed.length + swept.cancelled.length;
289291 }
290292
293 /**
294 * Remove a run's directory even when the cell left read-only files in it (a
295 * bun cache is): `rmSync` cannot unlink inside a directory without write
296 * permission, so permissions are restored and the removal tried again. A
297 * removal that still fails is logged, never swallowed.
298 */
299 export function removeRunDir(path: string): void {
300 try {
301 rmSync(path, { recursive: true, force: true });
302 return;
303 } catch {
304 /* fall through to restore permissions */
305 }
306 const open = (p: string): void => {
307 try {
308 if (lstatSync(p).isSymbolicLink()) return;
junipercrew

The chmodSync(p, 0o700) is applied to files too, which makes them executable for a moment. Use 0o700 for directories and 0o600 for files, or chmod directories only. Nothing reads the boolean return, so drop it or test it.
309 if (lstatSync(p).isDirectory()) {
310 chmodSync(p, 0o700);
311 for (const n of readdirSync(p)) open(join(p, n));
312 }
313 } catch {
314 /* gone, or not ours: the retry says */
315 }
316 };
317 open(path);
318 try {
319 rmSync(path, { recursive: true, force: true });
320 } catch (error) {
321 log.error("could not remove a run directory", { path, error: String(error) });
322 }
323 }
324
291325 /** Where a check's checkout lives while it runs. */
292326 const checksRoot = (dataDir: string): string => join(dataDir, "checks");
293327
@@ -301,12 +335,13 @@ export function sweepCheckouts(app: App, now: number = Date.now()): void {
301335 const root = checksRoot(app.config.dataDir);
302336 if (!existsSync(root)) return;
303337 for (const name of readdirSync(root)) {
304 const run = getRun(app.db, name);
338 // `<id>.home` is the home of run `<id>`: it lives and dies with it.
339 const run = getRun(app.db, name.endsWith(".home") ? name.slice(0, -5) : name);
305340 const live =
306341 run !== undefined &&
307342 run.state === "running" &&
308343 !leaseLapsed(run, STALE_AFTER_MS, now);
309 if (!live) rmSync(join(root, name), { recursive: true, force: true });
344 if (!live) removeRunDir(join(root, name));
310345 }
311346 }
312347
@@ -1180,8 +1215,8 @@ export function startChecks(app: App, options: ChecksOptions): ChecksHandle {
11801215 if (key !== undefined) recordProven(app.db, key, x.id);
11811216 return { ok: true };
11821217 } finally {
1183 if (existsSync(dir)) rmSync(dir, { recursive: true, force: true });
1184 rmSync(home, { recursive: true, force: true });
1218 if (existsSync(dir)) removeRunDir(dir);
1219 removeRunDir(home);
11851220 }
11861221 }
11871222
test/checks-home-sweep.test.ts added +106
@@ -0,0 +1,106 @@
1 /**
2 * A run's home (`<id>.home`) belongs to run `<id>` (#7545): the sweep removes
3 * it once the run is finished or its lease lapsed, leaves a live run's alone,
4 * and removes it even with read-only files in it. A torn-down preview takes
5 * its home with it.
6 */
7
8 import { afterAll, beforeAll, expect, test } from "bun:test";
9 import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
10 import { tmpdir } from "node:os";
11 import { join } from "node:path";
12 import { createApp, type App } from "../src/app.ts";
13 import { createRepo } from "../src/actions/repos.ts";
14 import { requeueAbandonedChecks, sweepCheckouts } from "../src/checks/service.ts";
15 import { ensureEnvData, removeEnv } from "../src/apps/data.ts";
16 import { claimRun, createRun, finishRun } from "../src/core/runs.ts";
17 import { STALE_AFTER_MS } from "../src/runs/lease.ts";
18 import { createUser } from "../src/core/users.ts";
19
20 let app: App;
21 let dataDir: string;
22 let repoId: string;
23
24 beforeAll(async () => {
25 dataDir = mkdtempSync(join(tmpdir(), "forge-home-sweep-"));
26 app = createApp({ dataDir, port: 0 });
27 const a = await createUser(app.db, {
28 origin: "seed",
29 trusted: true,
30 username: "alice",
31 email: "[email protected]",
32 password: "a-good-passphrase",
33 adminIfFirst: true,
34 });
35 if (!a.ok) throw new Error(a.error);
36 const made = await createRepo(app, a.value, { name: "garden", visibility: "private" });
37 if (!made.ok) throw new Error(JSON.stringify(made.error));
38 repoId = made.value.id;
39 });
40 afterAll(() => rmSync(dataDir, { recursive: true, force: true }));
41
42 const home = (id: string): string => join(dataDir, "checks", `${id}.home`);
43 const populate = (id: string, readOnly = false): void => {
44 const deep = join(home(id), ".bun", "install", "cache");
45 mkdirSync(deep, { recursive: true });
46 writeFileSync(join(deep, "pkg"), "x");
47 if (readOnly) {
48 chmodSync(join(deep, "pkg"), 0o444);
49 chmodSync(deep, 0o555);
50 }
51 };
52 const run = () => createRun(app.db, { repoId, kind: "check", env: "x", commitOid: "a".repeat(40) });
53
54 test("a finished run's home is removed, a live run's survives", () => {
55 const live = run();
56 claimRun(app.db, live.id);
57 const done = run();
58 claimRun(app.db, done.id);
59 finishRun(app.db, done.id, { state: "ok", summary: "ok" });
60 populate(live.id);
61 populate(done.id);
62 populate("no-such-run");
63 sweepCheckouts(app);
64 expect(existsSync(home(live.id))).toBe(true);
65 expect(existsSync(home(done.id))).toBe(false);
66 expect(existsSync(home("no-such-run"))).toBe(false);
67 });
68
69 test("the abandoned-checks timer sweeps a finished run's home, not a live one's", () => {
70 const live = run();
71 claimRun(app.db, live.id);
72 const done = run();
73 claimRun(app.db, done.id);
74 finishRun(app.db, done.id, { state: "ok", summary: "ok" });
75 populate(live.id);
76 populate(done.id);
77 requeueAbandonedChecks(app, STALE_AFTER_MS);
78 expect(existsSync(home(live.id))).toBe(true);
79 expect(existsSync(home(done.id))).toBe(false);
80 });
81
82 test("a lapsed run's home is removed", () => {
83 const r = run();
84 claimRun(app.db, r.id);
85 populate(r.id);
86 sweepCheckouts(app, Date.now() + 24 * 3600_000);
87 expect(existsSync(home(r.id))).toBe(false);
88 });
89
90 test("a home with a read-only file is still removed", () => {
91 const r = run();
92 claimRun(app.db, r.id);
93 finishRun(app.db, r.id, { state: "failed", summary: "red" });
94 populate(r.id, true);
95 sweepCheckouts(app);
96 expect(existsSync(home(r.id))).toBe(false);
97 });
98
99 test("tearing a preview down removes its home", () => {
100 ensureEnvData(dataDir, "app1", "pull-1", "empty");
101 const h = join(dataDir, "apps", "app1", "pull-1", "home");
102 mkdirSync(h, { recursive: true });
103 writeFileSync(join(h, "cache"), "x");
104 removeEnv(dataDir, "app1", "pull-1");
105 expect(existsSync(h)).toBe(false);
106 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs