trevato/forge

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

More
Forks1g fMapg aRuns13g rEventsFlows

deleteRepo orphans files when rm fails partway: presence is keyed on HEAD, not the directory #7173

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/7140-deleterepo-orphans-files-when-rm-fails-p → main 5 commits fork 2 threads · 2 unresolved

  • Merged. scout/7140-deleterepo-orphans-files-when-rm-fails-p was merged into main as eac4232.

Review

Sign in to review this change.
4 files changed +90 −5
src/core/reconcile.ts +2
@@ -36,6 +36,8 @@ export async function reconcile(app: App): Promise<void> {
3636 const row = rows[index];
3737 if (row === undefined) return;
3838 const dir = repoDir(app.reposRoot, row.owner_name, row.name);
39 // HEAD, not the directory: hooks can only be installed into a usable
40 // repository, and a half-removed one (no HEAD) is not that.
3941 if (await repoExists(dir)) {
4042 await installHooks(dir);
4143 refreshed++;
src/core/repos.ts +2 −2
@@ -15,7 +15,7 @@ import {
1515 detachFork,
1616 forkBareRepo,
1717 initBareRepo,
18 repoExists,
18 repoDirExists,
1919 repoDir,
2020 } from "../git/repos.ts";
2121 import { id } from "../kernel/id.ts";
@@ -902,7 +902,7 @@ export async function deleteRepo(
902902 // keeps the space actually free after the unlink instead of pinned to a
903903 // descriptor nobody will ever read again.
904904 closeReader(dir);
905 const present = await repoExists(dir);
905 const present = await repoDirExists(dir);
906906 // The directory goes first: when it will not, the row stays, so the
907907 // repository is still listed and a retry finds it. With no directory there
908908 // is nothing to orphan, so the row goes and the missing repository is
src/git/repos.ts +15 −3
@@ -5,7 +5,7 @@
55 */
66
77 import { join } from "node:path";
8 import { rm } from "node:fs/promises";
8 import { rm, stat } from "node:fs/promises";
99 import { defect } from "../kernel/error.ts";
1010 import { err, ok, type Result } from "../kernel/result.ts";
1111 import { runGit, type GitFailure } from "./env.ts";
@@ -119,14 +119,26 @@ export async function setDefaultBranch(
119119 return result.ok ? ok() : result;
120120 }
121121
122 /** Permanently delete a repository directory. The caller owns the ceremony. */
122 /** Is anything at this path? Keyed on the directory, so a half-removed repo counts. */
123 export async function repoDirExists(dir: string): Promise<boolean> {
124 return await stat(dir).then(
125 () => true,
126 () => false,
127 );
128 }
129
130 /**
131 * Permanently delete a repository directory. The caller owns the ceremony.
132 * Presence is the directory, not HEAD: a removal that died partway leaves no
133 * HEAD but still leaves files, and a retry must finish the job.
134 */
123135 export async function deleteRepoDir(
124136 dir: string,
125137 ): Promise<Result<void, { message: string }>> {
126138 if (!dir.endsWith(".git")) {
127139 defect("git.refuse_delete", `refusing to delete non-repo path: ${dir}`);
128140 }
129 if (!(await repoExists(dir)))
141 if (!(await repoDirExists(dir)))
130142 return err({ message: `not a repository: ${dir}` });
131143 try {
132144 await rm(dir, { recursive: true, force: true });
test/delete-repo-partial.test.ts added +71
@@ -0,0 +1,71 @@
1 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
2 import { chmodSync, existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs";
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 { createRepo, deleteRepo, getRepo } from "../src/core/repos.ts";
8 import { createUser } from "../src/core/users.ts";
9 import { repoDir } from "../src/git/repos.ts";
10
11 /**
12 * deleteRepo keys presence on the directory, not on HEAD: a removal that died
13 * partway (HEAD gone, a subdirectory stuck) keeps the row, and the retry
14 * finishes the job instead of dropping the row over orphaned files.
15 */
16
17 const root = process.getuid?.() === 0;
18
19 describe("deleteRepo after a partial removal", () => {
20 let dataDir: string;
21 let app: App;
22
23 beforeAll(async () => {
24 dataDir = await mkdtemp(join(tmpdir(), "forge-delpartial-"));
25 app = createApp({ dataDir, port: 0 });
26 });
27 afterAll(async () => {
28 app.db.close();
29 await rm(dataDir, { recursive: true, force: true });
30 });
31
32 test.skipIf(root)("a retry finishes what a failed removal left", async () => {
33 const made = await createUser(app.db, {
34 origin: "seed",
35 trusted: true,
36 username: "alice",
37 email: "[email protected]",
38 password: "a-good-passphrase",
39 adminIfFirst: true,
40 });
41 if (!made.ok) throw new Error("user");
42 const created = await createRepo(app.db, app.reposRoot, {
43 ownerId: made.value.id,
44 name: "stuck",
45 });
46 if (!created.ok) throw new Error("repo");
47 const repo = created.value;
48 const dir = repoDir(app.reposRoot, repo.ownerName, repo.name);
49
50 // HEAD is already gone; a locked subdirectory will not let rm finish.
51 rmSync(join(dir, "HEAD"));
52 const locked = join(dir, "objects");
53 writeFileSync(join(locked, "keep"), "x");
54 mkdirSync(join(locked, "sub"), { recursive: true });
55 chmodSync(locked, 0o500);
56
57 try {
58 const first = await deleteRepo(app.db, app.reposRoot, repo);
59 expect(first.ok).toBe(false);
60 expect(existsSync(dir)).toBe(true);
61 expect(getRepo(app.db, repo.ownerName, repo.name)).toBeDefined();
62 } finally {
63 chmodSync(locked, 0o700);
64 }
65
66 const second = await deleteRepo(app.db, app.reposRoot, repo);
67 expect(second.ok).toBe(true);
68 expect(existsSync(dir)).toBe(false);
69 expect(getRepo(app.db, repo.ownerName, repo.name)).toBeUndefined();
70 });
71 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs