Public: anyone can read this; the crew can change it.
URGENT flaky gate: "the fork flow > a mirror whose git is not ready yet is fetched again, and lands" fails on most checks #546
Check · Review · Land · Live done — Landed and live in production.
torque/forge:torque/541-urgent-flaky-gate-the-fork-flow-a-mirror → main 1 commit fork
2 files
changed +42
−27
src/origins/mirror.ts +12 −5
| @@ -63,12 +63,19 @@ export interface MirrorResult { | ||
| 63 | 63 | export const FIRST_FETCH_RETRY_MS: readonly number[] = [2_000, 5_000, 10_000]; |
| 64 | 64 | |
| 65 | 65 | /** |
| 66 | * How long to wait between tries of a new mirror's first fetch. Mutable so a | |
| 67 | * test that wants a failure now can have it now; the forge keeps the default, | |
| 68 | * and a test that lowers it puts back what it found. | |
| 66 | * How long to wait between tries of a new mirror's first fetch, and how the | |
| 67 | * wait is spent. Mutable so a test that wants a failure now can have it now, | |
| 68 | * and so a test can make the wait the thing it is waiting for (the origin | |
| 69 | * finishing the repository) instead of racing a clock against it (#541); the | |
| 70 | * forge keeps the defaults, and a test that changes them puts back what it | |
| 71 | * found. | |
| 69 | 72 | */ |
| 70 | export const firstFetchRetry = { | |
| 73 | export const firstFetchRetry: { | |
| 74 | ms: readonly number[]; | |
| 75 | wait: (ms: number) => Promise<void>; | |
| 76 | } = { | |
| 71 | 77 | ms: FIRST_FETCH_RETRY_MS, |
| 78 | wait: (ms) => new Promise((resolve) => setTimeout(resolve, ms)), | |
| 72 | 79 | }; |
| 73 | 80 | |
| 74 | 81 | export async function mirrorRepo( |
| @@ -171,7 +178,7 @@ export async function mirrorRepo( | ||
| 171 | 178 | stage: "fetching", |
| 172 | 179 | detail: `the origin has not finished making it; trying again in ${Math.round(wait / 1000)}s`, |
| 173 | 180 | }); |
| 174 | await new Promise((resolve) => setTimeout(resolve, wait)); | |
| 181 | await firstFetchRetry.wait(wait); | |
| 175 | 182 | report = await sync(); |
| 176 | 183 | } |
| 177 | 184 |
test/fork-flow.test.ts +30 −22
| @@ -7,7 +7,7 @@ import { serve, type Servers } from "../src/http/server.ts"; | ||
| 7 | 7 | import { resetConnectLimit } from "../src/actions/origins.ts"; |
| 8 | 8 | import { listEvents } from "../src/core/events.ts"; |
| 9 | 9 | import { firstFetchRetry } from "../src/origins/mirror.ts"; |
| 10 | import { getJob, settled } from "../src/origins/jobs.ts"; | |
| 10 | import { settled } from "../src/origins/jobs.ts"; | |
| 11 | 11 | import { |
| 12 | 12 | mirrorAndWait, |
| 13 | 13 | newFakeState, |
| @@ -789,28 +789,36 @@ describe("the fork flow", () => { | ||
| 789 | 789 | push: false, |
| 790 | 790 | admin: false, |
| 791 | 791 | }); |
| 792 | // Making the repository is two requests; the retry must not fire between | |
| 793 | // them and fetch it empty, so it waits longer than the seed can take. | |
| 794 | const found = firstFetchRetry.ms; | |
| 795 | firstFetchRetry.ms = [8_000]; | |
| 796 | const started = await api(`/api/v1/user/origins/${forgejoOrigin}/mirror`, { | |
| 797 | method: "POST", | |
| 798 | body: JSON.stringify({ fullName: `${UPSTREAM_OWNER}/late` }), | |
| 799 | }); | |
| 800 | expect(started.status).toBe(202); | |
| 801 | // Seeded only once the first pass has moved past its fetch (it found | |
| 802 | // nothing and is waiting to try again, `firstFetchRetry` in | |
| 803 | // `src/origins/mirror.ts`): seeding on a timer could land mid-fetch on a | |
| 804 | // slow machine and be fetched half-made. | |
| 805 | const jobId = ((await started.clone().json()) as { job: { id: string } }).job.id; | |
| 806 | for (let i = 0; i < 400; i++) { | |
| 807 | const stage = getJob(jobId)?.stage; | |
| 808 | if (stage !== "creating" && stage !== "fetching") break; | |
| 809 | await new Promise((r) => setTimeout(r, 25)); | |
| 792 | // The first fetch must find nothing and the retry must find the whole | |
| 793 | // repository. Making it is two requests (create, then commit), so a | |
| 794 | // retry on a timer can land between them and fetch it empty, and a | |
| 795 | // test that polls for "the first fetch has failed" can miss the | |
| 796 | // moment (#541). Instead the retry's wait *is* the seeding: the first | |
| 797 | // fetch runs before anything exists, and the second only once both | |
| 798 | // requests have answered. | |
| 799 | const found = { ...firstFetchRetry }; | |
| 800 | let seeded = 0; | |
| 801 | firstFetchRetry.ms = [0]; | |
| 802 | firstFetchRetry.wait = async () => { | |
| 803 | seeded++; | |
| 804 | await seedUpstream("late"); | |
| 805 | }; | |
| 806 | let job: Awaited<ReturnType<typeof mirrorAndWait>>; | |
| 807 | try { | |
| 808 | const started = await api( | |
| 809 | `/api/v1/user/origins/${forgejoOrigin}/mirror`, | |
| 810 | { | |
| 811 | method: "POST", | |
| 812 | body: JSON.stringify({ fullName: `${UPSTREAM_OWNER}/late` }), | |
| 813 | }, | |
| 814 | ); | |
| 815 | expect(started.status).toBe(202); | |
| 816 | job = await mirrorAndWait(started); | |
| 817 | } finally { | |
| 818 | Object.assign(firstFetchRetry, found); | |
| 810 | 819 | } |
| 811 | await seedUpstream("late"); | |
| 812 | const job = await mirrorAndWait(started); | |
| 813 | firstFetchRetry.ms = found; | |
| 820 | // Tried twice: once before the repository existed, once after. | |
| 821 | expect(seeded).toBe(1); | |
| 814 | 822 | expect(job?.stage).toBe("done"); |
| 815 | 823 | expect(job?.repo?.name).toBe("late"); |
| 816 | 824 | const branches = await read<{ name: string }[]>( |
new above
offline