Public: anyone can read this; the crew can change it.
A test pins removing an account: a confirm is required, and a repository that cannot be removed stops the delete #7096
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/7085-a-test-pins-removing-an-account-a-confir → main 3 commits fork 2 threads · 2 unresolved
3 files
changed +125
−4
src/core/repos.ts +13 −3
| @@ -15,6 +15,7 @@ import { | ||
| 15 | 15 | detachFork, |
| 16 | 16 | forkBareRepo, |
| 17 | 17 | initBareRepo, |
| 18 | repoExists, | |
| 18 | 19 | repoDir, |
| 19 | 20 | } from "../git/repos.ts"; |
| 20 | 21 | import { id } from "../kernel/id.ts"; |
| @@ -866,7 +867,10 @@ export function listReposByOwner( | ||
| 866 | 867 | } |
| 867 | 868 | |
| 868 | 869 | /** |
| 869 | * Delete a repo. Row first, then disk — inverse of creation, same invariant. | |
| 870 | * Delete a repo. Disk first, then the row: when the directory will not go, | |
| 871 | * the repository stays listed and a retry finds it (`delete_failed`). A | |
| 872 | * directory that is already missing leaves nothing to orphan, so the row still | |
| 873 | * goes and `delete_failed` is still reported. | |
| 870 | 874 | * Before removing the objects, detach every fork that borrows them (repack so |
| 871 | 875 | * the fork owns its objects, then drop its alternates link) — otherwise a |
| 872 | 876 | * fork would be silently corrupted. This is the detach-before-delete rule. |
| @@ -893,14 +897,20 @@ export async function deleteRepo( | ||
| 893 | 897 | ); |
| 894 | 898 | } |
| 895 | 899 | |
| 896 | db.query("DELETE FROM repo WHERE id = ?1").run(repo.id); | |
| 897 | forgetListings(db); | |
| 898 | 900 | const dir = repoDir(reposRoot, repo.ownerName, repo.name); |
| 899 | 901 | // A pooled `cat-file` holds this directory open. Closing it first is what |
| 900 | 902 | // keeps the space actually free after the unlink instead of pinned to a |
| 901 | 903 | // descriptor nobody will ever read again. |
| 902 | 904 | closeReader(dir); |
| 905 | const present = await repoExists(dir); | |
| 906 | // The directory goes first: when it will not, the row stays, so the | |
| 907 | // repository is still listed and a retry finds it. With no directory there | |
| 908 | // is nothing to orphan, so the row goes and the missing repository is | |
| 909 | // still reported. | |
| 903 | 910 | const removed = await deleteRepoDir(dir); |
| 911 | if (!removed.ok && present) return err("delete_failed"); | |
Right order. The remaining gap is a partial rm. |
||
| 912 | db.query("DELETE FROM repo WHERE id = ?1").run(repo.id); | |
| 913 | forgetListings(db); | |
| 904 | 914 | if (!removed.ok) return err("delete_failed"); |
| 905 | 915 | return ok(); |
| 906 | 916 | } |
src/git/repos.ts +5 −1
| @@ -128,6 +128,10 @@ export async function deleteRepoDir( | ||
| 128 | 128 | } |
| 129 | 129 | if (!(await repoExists(dir))) |
| 130 | 130 | return err({ message: `not a repository: ${dir}` }); |
| 131 | await rm(dir, { recursive: true, force: true }); | |
| 131 | try { | |
| 132 | await rm(dir, { recursive: true, force: true }); | |
| 133 | } catch (e) { | |
| 134 | return err({ message: e instanceof Error ? e.message : String(e) }); | |
| 135 | } | |
| 132 | 136 | return ok(); |
| 133 | 137 | } |
test/account-remove-refusals.test.ts added +107
| @@ -0,0 +1,107 @@ | ||
| 1 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 2 | import { chmodSync, existsSync } from "node:fs"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { dirname, join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { removeAccount, retireAndRemoveAccount } from "../src/actions/accounts.ts"; | |
| 8 | import { createRepo } from "../src/actions/repos.ts"; | |
| 9 | import { signUp } from "../src/actions/users.ts"; | |
| 10 | import { listReposOwnedBy } from "../src/core/repos.ts"; | |
| 11 | import { getUserByName, type User } from "../src/core/users.ts"; | |
| 12 | import { repoDir } from "../src/git/repos.ts"; | |
| 13 | import { proveAll } from "./helpers/verified.ts"; | |
| 14 | import { fixtureSlot } from "./helpers/slot.ts"; | |
| 15 | import { promote } from "./helpers/members.ts"; | |
| 16 | import { seedAdmin } from "./helpers/admin.ts"; | |
| 17 | ||
| 18 | describe("removing an account: what stops it", () => { | |
| 19 | let dataDir: string; | |
| 20 | let app: App; | |
| 21 | let admin: User; | |
| 22 | let nosy: User; | |
| 23 | ||
| 24 | beforeAll(async () => { | |
| 25 | dataDir = await mkdtemp(join(tmpdir(), "forge-remove-refusals-")); | |
| 26 | app = createApp({ dataDir, port: 0, url: "https://forge.example", edgeToken: "e" }); | |
| 27 | const made = await seedAdmin(app, { username: "olive", email: "[email protected]", password: "a-long-passphrase" }); | |
| 28 | if (!made.ok) throw new Error(made.error); | |
| 29 | admin = made.value; | |
| 30 | nosy = await mk("nosy"); | |
| 31 | }); | |
| 32 | afterAll(async () => { | |
| 33 | app.close(); | |
| 34 | await rm(dataDir, { recursive: true, force: true }); | |
| 35 | }); | |
| 36 | ||
| 37 | const mk = async (username: string): Promise<User> => { | |
| 38 | const u = await signUp(app, { | |
| 39 | username, | |
| 40 | email: `${username}@f.dev`, | |
| 41 | password: "a-long-passphrase", | |
| 42 | }, { slot: fixtureSlot(app) }); | |
| 43 | if (!u.ok) throw new Error(u.error); | |
| 44 | proveAll(app); | |
| 45 | return getUserByName(app.db, username)!; | |
| 46 | }; | |
| 47 | const withRepo = async (name: string): Promise<User> => { | |
| 48 | const u = await mk(name); | |
| 49 | promote(app.db, u); | |
| 50 | const r = await createRepo(app, u, { name: "ledger" }); | |
| 51 | if (!r.ok) throw new Error(r.error); | |
| 52 | return u; | |
| 53 | }; | |
| 54 | ||
| 55 | test("without confirmed: true the answer is confirm_required and nothing changes", async () => { | |
| 56 | const u = await withRepo("kept"); | |
| 57 | for (const options of [undefined, {}, { confirmed: false }]) { | |
| 58 | const out = await retireAndRemoveAccount(app, admin, "kept", options); | |
| 59 | expect(out).toEqual({ ok: false, error: "confirm_required" }); | |
| 60 | } | |
| 61 | expect(getUserByName(app.db, "kept")!.removedAt).toBeNull(); | |
| 62 | expect(listReposOwnedBy(app.db, u.id).length).toBe(1); | |
| 63 | expect(existsSync(repoDir(app.reposRoot, "kept", "ledger"))).toBe(true); | |
| 64 | }); | |
| 65 | ||
| 66 | test("with confirmed: true the account and its repositories go", async () => { | |
| 67 | const u = await withRepo("gone"); | |
| 68 | const out = await retireAndRemoveAccount(app, admin, "gone", { confirmed: true }); | |
| 69 | expect(out.ok && out.value).toEqual({ removed: ["gone/ledger", "gone"], failed: null }); | |
| 70 | expect(getUserByName(app.db, "gone")!.removedAt).not.toBeNull(); | |
| 71 | expect(listReposOwnedBy(app.db, u.id).length).toBe(0); | |
| 72 | expect(existsSync(repoDir(app.reposRoot, "gone", "ledger"))).toBe(false); | |
| 73 | }); | |
| 74 | ||
| 75 | // chmod does not stop root, so there is nothing to make unremovable. | |
| 76 | test.skipIf(process.getuid?.() === 0)("a repository that cannot be removed stops the delete and the account row stays", async () => { | |
| 77 | const u = await withRepo("stuck"); | |
| 78 | const dir = repoDir(app.reposRoot, "stuck", "ledger"); | |
| 79 | const parent = dirname(dir); | |
| 80 | chmodSync(parent, 0o555); | |
| 81 | try { | |
| 82 | const out = await removeAccount(app, admin, "stuck"); | |
| 83 | expect(out).toEqual({ ok: false, error: "delete_failed" }); | |
| 84 | expect(getUserByName(app.db, "stuck")!.removedAt).toBeNull(); | |
| 85 | expect(existsSync(dir)).toBe(true); | |
| 86 | expect(listReposOwnedBy(app.db, u.id).length).toBe(1); | |
| 87 | } finally { | |
| 88 | chmodSync(parent, 0o755); | |
| 89 | } | |
| 90 | // Nothing was orphaned: with the directory removable again, a retry | |
| 91 | // finishes the job. | |
| 92 | expect((await removeAccount(app, admin, "stuck")).ok).toBe(true); | |
| 93 | expect(existsSync(dir)).toBe(false); | |
| 94 | expect(getUserByName(app.db, "stuck")!.removedAt).not.toBeNull(); | |
| 95 | }); | |
| 96 | ||
| 97 | test("a non-admin and a signed-out caller are refused before anything else", async () => { | |
| 98 | await withRepo("safe"); | |
| 99 | for (const actor of [nosy, undefined]) { | |
| 100 | expect(await retireAndRemoveAccount(app, actor, "safe")).toEqual({ ok: false, error: "not_found" }); | |
| 101 | expect(await retireAndRemoveAccount(app, actor, "safe", { confirmed: true })).toEqual({ ok: false, error: "not_found" }); | |
| 102 | expect(await removeAccount(app, actor, "safe")).toEqual({ ok: false, error: "not_found" }); | |
| 103 | } | |
| 104 | expect(getUserByName(app.db, "safe")!.removedAt).toBeNull(); | |
| 105 | expect(existsSync(repoDir(app.reposRoot, "safe", "ledger"))).toBe(true); | |
| 106 | }); | |
| 107 | }); |
new above
offline
The order here is right, but the docstring on this function (about line 869) still says "Row first, then disk". Update it to say the directory goes first and why. Separately, and not blocking:
presentis based on HEAD, so a partialrmthat removed HEAD makes the next attempt drop the row and orphan whatever is left on disk.