Public: anyone can read this; the crew can change it.
A test pins contributing when the fork never arrives or its mirror fails: the answer says so and nothing is half-made #7161
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/7152-a-test-pins-contributing-when-the-fork-n → main 1 commit fork 2 threads · 2 unresolved
2 files
changed +249
−3
src/actions/contribute.ts +17 −3
| @@ -44,7 +44,7 @@ import { | ||
| 44 | 44 | type Origin, |
| 45 | 45 | } from "../core/origins.ts"; |
| 46 | 46 | import { getPull, type Pull } from "../core/pulls.ts"; |
| 47 | import { getRepoById, type Repo } from "../core/repos.ts"; | |
| 47 | import { deleteRepo as destroyRepo, getRepoById, type Repo } from "../core/repos.ts"; | |
| 48 | 48 | import type { User } from "../core/users.ts"; |
| 49 | 49 | import { isSafeRef } from "../git/guard.ts"; |
| 50 | 50 | import { repoDir } from "../git/repos.ts"; |
| @@ -71,6 +71,13 @@ import { requireUser } from "./access.ts"; | ||
| 71 | 71 | */ |
| 72 | 72 | const FORK_DEADLINE_MS = 30_000; |
| 73 | 73 | |
| 74 | /** | |
| 75 | * The deadline `forkOrigin` uses when its caller names none. Mutable for the | |
| 76 | * same reason `firstFetchRetry` is: a test that wants the timeout shortens it | |
| 77 | * and puts back what it found. | |
| 78 | */ | |
| 79 | export const forkDeadline: { ms: number } = { ms: FORK_DEADLINE_MS }; | |
| 80 | ||
| 74 | 81 | /** Backoff between polls, in ms; the last value repeats until the deadline. */ |
| 75 | 82 | const FORK_BACKOFF = [250, 500, 1_000, 2_000, 3_000] as const; |
| 76 | 83 | |
| @@ -160,7 +167,7 @@ export async function forkOrigin( | ||
| 160 | 167 | ? await awaitFork( |
| 161 | 168 | provider.value, |
| 162 | 169 | `${origin.login}/${name}`, |
| 163 | options.waitMs ?? FORK_DEADLINE_MS, | |
| 170 | options.waitMs ?? forkDeadline.ms, | |
| 164 | 171 | ) |
| 165 | 172 | : undefined; |
| 166 | 173 | if ( |
| @@ -179,7 +186,7 @@ export async function forkOrigin( | ||
| 179 | 186 | const ready = await awaitFork( |
| 180 | 187 | provider.value, |
| 181 | 188 | asked.value.fullName, |
| 182 | options.waitMs ?? FORK_DEADLINE_MS, | |
| 189 | options.waitMs ?? forkDeadline.ms, | |
| 183 | 190 | ); |
| 184 | 191 | if (!ready.ok) return ready; |
| 185 | 192 | remote = ready.value; |
| @@ -230,6 +237,13 @@ export async function forkOrigin( | ||
| 230 | 237 | } |
| 231 | 238 | |
| 232 | 239 | const done = await settled(job.id); |
| 240 | // A clone that failed leaves an empty repository behind, which is right for | |
| 241 | // a mirror someone asked for (they can retry it) and wrong for a workbench | |
| 242 | // nobody will see: the work cannot start, so none of it stays. | |
| 243 | if (done?.stage === "failed" && done.repo !== undefined) { | |
| 244 | await destroyRepo(app.db, app.reposRoot, done.repo); | |
| 245 | return ok({ remote, mirror: undefined, job, existed }); | |
| 246 | } | |
| 233 | 247 | const mirror = done?.repo |
| 234 | 248 | ? getRepoById(app.db, done.repo.id) |
| 235 | 249 | : localMirror(app, origin, remote.fullName); |
test/contribute-fork-failures.test.ts added +232
| @@ -0,0 +1,232 @@ | ||
| 1 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 2 | import { existsSync } from "node:fs"; | |
| 3 | import { mkdtemp, readdir, 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 { forkDeadline } from "../src/actions/contribute.ts"; | |
| 8 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 9 | import { firstFetchRetry } from "../src/origins/mirror.ts"; | |
| 10 | import { | |
| 11 | mirrorAndWait, | |
| 12 | newFakeState, | |
| 13 | serveFakeForge, | |
| 14 | type FakeForge, | |
| 15 | } from "./fake-origin.ts"; | |
| 16 | import { proveAll } from "./helpers/verified.ts"; | |
| 17 | import { promote } from "./helpers/members.ts"; | |
| 18 | import { tokenFor } from "./helpers/tokens.ts"; | |
| 19 | ||
| 20 | /** | |
| 21 | * `contribute` to a repository the caller cannot push to makes a fork first. | |
| 22 | * When the fork never appears (`fork_timeout`) or its local mirror cannot be | |
| 23 | * made (`mirror_failed`), the answer is a refusal in words and nothing is | |
| 24 | * half-made: no branch, commit or pull at the origin, no new repository here. | |
| 25 | */ | |
| 26 | ||
| 27 | const ME = "me"; | |
| 28 | const OWNER = "octo"; | |
| 29 | const PASSWORD = "fork-failures-pass"; | |
| 30 | ||
| 31 | const read = async <T>(response: Response): Promise<T> => | |
| 32 | (await response.json()) as T; | |
| 33 | ||
| 34 | describe("contribute when the fork fails", () => { | |
| 35 | let dataDir = ""; | |
| 36 | let originDataDir = ""; | |
| 37 | let app: App; | |
| 38 | let servers: Servers; | |
| 39 | let originApp: App; | |
| 40 | let originServers: Servers; | |
| 41 | let fake: FakeForge; | |
| 42 | let aliceToken = ""; | |
| 43 | let mirrorPath = ""; | |
| 44 | const found = { | |
| 45 | deadline: forkDeadline.ms, | |
| 46 | retry: firstFetchRetry.ms, | |
| 47 | }; | |
| 48 | ||
| 49 | const mkAccount = async (server: Servers, username: string) => { | |
| 50 | await fetch(`${server.url}/api/v1/users`, { | |
| 51 | method: "POST", | |
| 52 | headers: { "content-type": "application/json" }, | |
| 53 | body: JSON.stringify({ | |
| 54 | username, | |
| 55 | email: `${username}@f.dev`, | |
| 56 | password: PASSWORD, | |
| 57 | }), | |
| 58 | }); | |
| 59 | return tokenFor(server === originServers ? originApp : app, username, "admin"); | |
| 60 | }; | |
| 61 | ||
| 62 | const contribute = (): Promise<Response> => | |
| 63 | fetch(`${servers.url}/api/v1/repos/${mirrorPath}/contribute`, { | |
| 64 | method: "POST", | |
| 65 | headers: { | |
| 66 | "content-type": "application/json", | |
| 67 | authorization: `Bearer ${aliceToken}`, | |
| 68 | }, | |
| 69 | body: JSON.stringify({ | |
| 70 | branch: "fix-typo", | |
| 71 | message: "fix", | |
| 72 | files: [{ path: "a.txt", content: "one, fixed\n" }], | |
| 73 | openPull: { title: "fix" }, | |
| 74 | }), | |
| 75 | }); | |
| 76 | ||
| 77 | const count = (sql: string): number => | |
| 78 | app.db.query<{ n: number }, []>(sql).get()?.n ?? -1; | |
| 79 | ||
| 80 | const localDirs = async (): Promise<string[]> => { | |
| 81 | const out: string[] = []; | |
| 82 | const root = join(dataDir, "repos"); | |
| 83 | for (const owner of await readdir(root)) | |
| 84 | for (const name of await readdir(join(root, owner))) | |
| 85 | out.push(`${owner}/${name}`); | |
| 86 | return out.sort(); | |
| 87 | }; | |
| 88 | ||
| 89 | /** Everything a half-made contribution would have left behind. */ | |
| 90 | const world = async () => ({ | |
| 91 | fakeBranches: [...fake.state.branches.entries()], | |
| 92 | fakeCommits: [...fake.state.commits.entries()], | |
| 93 | fakeWrites: fake.state.requests.filter( | |
| 94 | (r) => r.method !== "GET" && !r.path.endsWith("/forks"), | |
| 95 | ).length, | |
| 96 | repos: count("SELECT COUNT(*) AS n FROM repo"), | |
| 97 | pulls: count("SELECT COUNT(*) AS n FROM pull"), | |
| 98 | dirs: await localDirs(), | |
| 99 | }); | |
| 100 | ||
| 101 | beforeAll(async () => { | |
| 102 | dataDir = await mkdtemp(join(tmpdir(), "forge-cff-")); | |
| 103 | originDataDir = await mkdtemp(join(tmpdir(), "forge-cff-up-")); | |
| 104 | ||
| 105 | originApp = createApp({ dataDir: originDataDir, port: 0 }); | |
| 106 | originServers = await serve(originApp); | |
| 107 | await mkAccount(originServers, OWNER); | |
| 108 | const originToken = await mkAccount(originServers, ME); | |
| 109 | proveAll(originApp); | |
| 110 | promote(originApp.db); | |
| 111 | ||
| 112 | const as = `Basic ${Buffer.from(`${OWNER}:${PASSWORD}`).toString("base64")}`; | |
| 113 | const up = (path: string, body: unknown) => | |
| 114 | fetch(`${originServers.url}${path}`, { | |
| 115 | method: "POST", | |
| 116 | headers: { "content-type": "application/json", authorization: as }, | |
| 117 | body: JSON.stringify(body), | |
| 118 | }); | |
| 119 | await up("/api/v1/repos", { name: "gears", visibility: "public" }); | |
| 120 | await up(`/api/v1/repos/${OWNER}/gears/contents`, { | |
| 121 | branch: "main", | |
| 122 | message: "first", | |
| 123 | files: [{ path: "a.txt", content: "one\n" }], | |
| 124 | }); | |
| 125 | ||
| 126 | const state = newFakeState("forgejo", ME, originToken); | |
| 127 | state.repos.push({ | |
| 128 | id: 1, | |
| 129 | fullName: `${OWNER}/gears`, | |
| 130 | cloneUrl: `${originServers.url}/${OWNER}/gears.git`, | |
| 131 | defaultBranch: "main", | |
| 132 | push: false, | |
| 133 | admin: false, | |
| 134 | }); | |
| 135 | fake = serveFakeForge(state); | |
| 136 | ||
| 137 | app = createApp({ dataDir, port: 0 }); | |
| 138 | servers = await serve(app); | |
| 139 | aliceToken = await mkAccount(servers, "alice"); | |
| 140 | proveAll(app); | |
| 141 | promote(app.db); | |
| 142 | ||
| 143 | const headers = { | |
| 144 | "content-type": "application/json", | |
| 145 | authorization: `Bearer ${aliceToken}`, | |
| 146 | }; | |
| 147 | const connected = await fetch(`${servers.url}/api/v1/user/origins`, { | |
| 148 | method: "POST", | |
| 149 | headers, | |
| 150 | body: JSON.stringify({ host: fake.url, token: originToken }), | |
| 151 | }); | |
| 152 | expect(connected.status).toBe(201); | |
| 153 | const originId = (await read<{ id: string }>(connected)).id; | |
| 154 | const job = await mirrorAndWait( | |
| 155 | await fetch(`${servers.url}/api/v1/user/origins/${originId}/mirror`, { | |
| 156 | method: "POST", | |
| 157 | headers, | |
| 158 | body: JSON.stringify({ fullName: `${OWNER}/gears` }), | |
| 159 | }), | |
| 160 | ); | |
| 161 | expect(job?.stage).toBe("done"); | |
| 162 | mirrorPath = `${job!.repo!.ownerName}/${job!.repo!.name}`; | |
| 163 | ||
| 164 | // Shortened for the tests below; put back in afterAll. | |
| 165 | forkDeadline.ms = 100; | |
| 166 | firstFetchRetry.ms = []; | |
| 167 | }); | |
| 168 | ||
| 169 | afterAll(async () => { | |
| 170 | forkDeadline.ms = found.deadline; | |
| 171 | firstFetchRetry.ms = found.retry; | |
| 172 | fake.stop(); | |
| 173 | servers.stop(); | |
| 174 | originServers.stop(); | |
| 175 | app.close(); | |
| 176 | originApp.close(); | |
| 177 | await rm(dataDir, { recursive: true, force: true }); | |
| 178 | await rm(originDataDir, { recursive: true, force: true }); | |
| 179 | }); | |
| 180 | ||
| 181 | test("a fork that never appears is `fork_timeout`, and nothing is made", async () => { | |
| 182 | fake.state.forkDelayReads = 1_000_000; | |
| 183 | const before = await world(); | |
| 184 | const started = Date.now(); | |
| 185 | const response = await contribute(); | |
| 186 | expect(Date.now() - started).toBeLessThan(5_000); | |
| 187 | ||
| 188 | expect(response.status).toBe(504); | |
| 189 | const refusal = await read<{ error: string; message: string }>(response); | |
| 190 | expect(refusal.error).toBe("fork_timeout"); | |
| 191 | expect(refusal.message).toContain("fork has not appeared"); | |
| 192 | expect(await world()).toEqual(before); | |
| 193 | // The origin was asked to fork, and that is all it was asked. | |
| 194 | expect( | |
| 195 | fake.state.requests.some((r) => r.method === "POST" && r.path.endsWith("/forks")), | |
| 196 | ).toBe(true); | |
| 197 | expect(fake.state.pendingForks.size).toBeGreaterThan(0); | |
| 198 | }); | |
| 199 | ||
| 200 | test("a fork whose clone fails is `mirror_failed`, and no mirror is left behind", async () => { | |
| 201 | fake.state.pendingForks.clear(); | |
| 202 | fake.state.forkDelayReads = 0; | |
| 203 | // (The first test left the fork the origin made, hidden behind its delay.) | |
| 204 | const fork = fake.state.repos.find((r) => r.fullName === `${ME}/gears`); | |
| 205 | expect(fork).toBeDefined(); | |
| 206 | fork!.cloneUrl = "http://127.0.0.1:1/me/gears.git"; | |
| 207 | const before = await world(); | |
| 208 | const response = await contribute(); | |
| 209 | ||
| 210 | expect(response.status).toBe(502); | |
| 211 | const refusal = await read<{ error: string; message: string }>(response); | |
| 212 | expect(refusal.error).toBe("mirror_failed"); | |
| 213 | expect(refusal.message).toContain("local copy"); | |
| 214 | expect(await world()).toEqual(before); | |
| 215 | expect(existsSync(join(dataDir, "repos", "alice", "gears"))).toBe(false); | |
| 216 | ||
| 217 | // Nothing stands in the way of trying again once the fork's git answers. | |
| 218 | fork!.cloneUrl = `${originServers.url}/${OWNER}/gears.git`.replace( | |
| 219 | `/${OWNER}/`, | |
| 220 | `/${ME}/`, | |
Build the URL directly: |
||
| 221 | ); | |
| 222 | await fetch(`${originServers.url}/api/v1/repos/${OWNER}/gears/forks`, { | |
| 223 | method: "POST", | |
| 224 | headers: { | |
| 225 | "content-type": "application/json", | |
| 226 | authorization: `Basic ${Buffer.from(`${ME}:${PASSWORD}`).toString("base64")}`, | |
| 227 | }, | |
| 228 | body: "{}", | |
| 229 | }); | |
| 230 | expect((await contribute()).status).toBe(201); | |
| 231 | }); | |
| 232 | }); |
new above
offline
destroyRepo returns Result<void, "delete_failed"> and it is ignored. If the delete fails, the empty mirror stays and the caller is told mirror_failed anyway. Log the failure here (log.error with the repo name), and still return the refusal.