From 1a54010950236031fd2ed45141ff38f446a6a39a Mon Sep 17 00:00:00 2001 From: Sean Mousseau Date: Sun, 24 May 2026 22:58:40 -0400 Subject: [PATCH] fix: prevent duplicate milestones & labels on every sync (#299) Paginate /milestones and /labels with Link header + X-Total-Count fallback, and pass state=all on milestones so closed milestones aren't re-POSTed on every sync. Caches newly-created items in the in-memory dedup set. --- src/lib/gitea-milestone-label-dedup.test.ts | 200 ++++++++++++++++++++ src/lib/gitea.ts | 115 +++++++++-- 2 files changed, 295 insertions(+), 20 deletions(-) create mode 100644 src/lib/gitea-milestone-label-dedup.test.ts diff --git a/src/lib/gitea-milestone-label-dedup.test.ts b/src/lib/gitea-milestone-label-dedup.test.ts new file mode 100644 index 0000000..a324e02 --- /dev/null +++ b/src/lib/gitea-milestone-label-dedup.test.ts @@ -0,0 +1,200 @@ +/** + * Regression test for duplicate milestone & label creation on every sync. + * + * Observed in production (May 2026): a Gitea instance accumulated + * 11,847 duplicate closed milestones across 4 mirrored repos after only + * a handful of scheduled syncs. Two compounding bugs in + * `mirrorGitRepoMilestonesToGitea`: + * + * (1) The existing-milestones GET to Gitea did NOT pass `state=all`. + * Gitea's /milestones endpoint defaults to `state=open`, so the + * `existingMilestones` Set never contained any closed milestone + * title. Every closed GitHub milestone was misclassified as + * missing and re-POSTed on every sync. + * + * (2) The existing-milestones GET was a single unpaginated call. + * Gitea caps response size at `[api].MAX_RESPONSE_ITEMS` + * (default 50), so any repo with more milestones than that + * silently truncates even when (1) is fixed. + * + * `mirrorGitRepoLabelsToGitea` has the same pagination bug (2). It + * doesn't have bug (1) because /labels has no state filter, and it + * hadn't yet shown duplicates in production only because no mirrored + * repo had crossed 50 distinct labels — but it would the moment one + * did. + * + * These tests assert on the *structure* of the source rather than + * invoking the functions, because behavioral tests for the metadata + * pipeline require heavy module mocks that pollute other test files + * (bun's mock.module is process-wide). Same convention as + * `gitea-issue-dedup-on-retry.test.ts` and + * `gitea-mirror-failure-recovery.test.ts`. + */ +import { describe, test, expect } from "bun:test"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +const SOURCE = readFileSync(join(import.meta.dir, "gitea.ts"), "utf8"); + +/** + * Locate the body of a function declaration by name. Walks from the + * declaration, balances parens to skip the parameter list (which can + * contain destructured object literals with their own braces), then + * finds the body's opening brace and its matching close. + */ +function extractFunctionBody(source: string, declarationStart: RegExp): string { + const match = source.match(declarationStart); + if (!match) { + throw new Error(`Could not locate declaration ${declarationStart}`); + } + let i = match.index! + match[0].length; + while (i < source.length && source[i] !== "(") i++; + if (source[i] !== "(") { + throw new Error(`No '(' after ${declarationStart}`); + } + let parenDepth = 0; + for (; i < source.length; i++) { + if (source[i] === "(") parenDepth++; + else if (source[i] === ")") { + parenDepth--; + if (parenDepth === 0) { + i++; + break; + } + } + } + while (i < source.length && source[i] !== "{") i++; + if (source[i] !== "{") { + throw new Error(`No body '{' for ${declarationStart}`); + } + let braceDepth = 0; + const startIdx = i; + for (; i < source.length; i++) { + if (source[i] === "{") braceDepth++; + else if (source[i] === "}") { + braceDepth--; + if (braceDepth === 0) { + return source.slice(startIdx, i + 1); + } + } + } + throw new Error(`Unterminated body for ${declarationStart}`); +} + +describe("milestone dedup on sync", () => { + const body = extractFunctionBody( + SOURCE, + /export async function mirrorGitRepoMilestonesToGitea\b/ + ); + + test("body contains the per-milestone create branch we expect to guard", () => { + // Sanity check: anchor the rest of this suite to the right path. + expect( + body.includes("existingMilestones"), + "expected the existingMilestones set/map used for dedup" + ).toBe(true); + expect( + body.match( + /await httpPost\(\s*`\$\{config\.giteaConfig\.url\}\/api\/v1\/repos\/\$\{giteaOwner\}\/\$\{repoName\}\/milestones`/ + ), + "expected the create-milestone httpPost call" + ).toBeTruthy(); + }); + + test("existing-milestones GET must include state=all", () => { + // Without state=all, Gitea returns only open milestones, so every + // closed GitHub milestone is misclassified as missing and re-POSTed + // on every sync. This was the root cause of the production blowup. + const getMatch = body.match( + /httpGet\(\s*`\$\{config\.giteaConfig\.url\}\/api\/v1\/repos\/\$\{giteaOwner\}\/\$\{repoName\}\/milestones\?[^`]*`/ + ); + expect( + getMatch, + "expected a milestones httpGet call with a query string" + ).toBeTruthy(); + expect( + /state=all/.test(getMatch![0]), + "the existing-milestones GET must pass state=all (Gitea defaults to state=open)" + ).toBe(true); + }); + + test("existing-milestones GET must paginate with both Link and X-Total-Count fallback", () => { + // Even with state=all, a single unpaginated call only ever sees + // the first 50 milestones (Gitea's MAX_RESPONSE_ITEMS cap). + // + // Gitea's /milestones endpoint does NOT emit a Link header — only + // `X-Total-Count`. A strict Link-only check terminates after page 1 + // and re-POSTs every milestone past index 50 on every sync (the + // 9-milestone leak observed on Subnet-Calculator after the + // Link-only version of this fix shipped). + expect( + /milestonesPage\s*\+=\s*1/.test(body), + "expected a page-increment loop for existing milestones" + ).toBe(true); + expect( + /\.headers\.get\(\s*["']link["']\s*\)/.test(body) && + /rel="next"/.test(body), + "the milestones pagination loop must check the Link header (rel=\"next\")" + ).toBe(true); + expect( + /\.headers\.get\(\s*["']x-total-count["']\s*\)/.test(body), + "the milestones pagination loop must fall back to X-Total-Count when Link header is absent (Gitea /milestones only emits X-Total-Count)" + ).toBe(true); + }); + + test("newly-created milestone must be cached into existingMilestones", () => { + // Defensive: if `milestones` ever contains a same-named entry + // twice (unlikely but cheap to guard), we shouldn't POST it twice. + expect( + /existingMilestones\.add\(\s*milestone\.title\s*\)/.test(body), + "after a successful create, the new milestone title must be added to existingMilestones" + ).toBe(true); + }); +}); + +describe("label dedup on sync", () => { + const body = extractFunctionBody( + SOURCE, + /export async function mirrorGitRepoLabelsToGitea\b/ + ); + + test("body contains the per-label create branch we expect to guard", () => { + expect( + body.includes("existingLabels"), + "expected the existingLabels set used for dedup" + ).toBe(true); + expect( + body.match( + /await httpPost\(\s*`\$\{config\.giteaConfig\.url\}\/api\/v1\/repos\/\$\{giteaOwner\}\/\$\{repoName\}\/labels`/ + ), + "expected the create-label httpPost call" + ).toBeTruthy(); + }); + + test("existing-labels GET must paginate with both Link and X-Total-Count fallback", () => { + // Same Gitea MAX_RESPONSE_ITEMS=50 cap as milestones / issues. + // Gitea's /labels endpoint, like /milestones, does NOT emit a Link + // header — only `X-Total-Count`. Strict Link-only check would + // silently truncate after page 1. + expect( + /labelsPage\s*\+=\s*1/.test(body), + "expected a page-increment loop for existing labels" + ).toBe(true); + expect( + /\.headers\.get\(\s*["']link["']\s*\)/.test(body) && + /rel="next"/.test(body), + "the labels pagination loop must check the Link header (rel=\"next\")" + ).toBe(true); + expect( + /\.headers\.get\(\s*["']x-total-count["']\s*\)/.test(body), + "the labels pagination loop must fall back to X-Total-Count when Link header is absent (Gitea /labels only emits X-Total-Count)" + ).toBe(true); + }); + + test("newly-created label must be cached into existingLabels", () => { + expect( + /existingLabels\.add\(\s*label\.name\s*\)/.test(body), + "after a successful create, the new label name must be added to existingLabels" + ).toBe(true); + }); +}); diff --git a/src/lib/gitea.ts b/src/lib/gitea.ts index 6d8a1c7..07ef2a5 100644 --- a/src/lib/gitea.ts +++ b/src/lib/gitea.ts @@ -3408,17 +3408,49 @@ export async function mirrorGitRepoLabelsToGitea({ return; } - // Get existing labels from Gitea - const giteaLabelsRes = await httpGet( - `${config.giteaConfig.url}/api/v1/repos/${giteaOwner}/${repoName}/labels`, - { - Authorization: `token ${decryptedConfig.giteaConfig.token}`, - } - ); + // Get existing labels from Gitea. Paginate because Gitea caps + // response size at `[api].MAX_RESPONSE_ITEMS` (default 50), so a + // single unpaginated GET only sees the first 50 labels. Once a repo + // crosses that threshold every label past it would be re-POSTed as a + // duplicate on every sync. + // + // Pagination signal: prefer Link header (RFC 5988) when present, but + // Gitea's /labels and /milestones endpoints do NOT emit Link headers + // — they only emit `X-Total-Count`. Without the fallback, a strict + // Link-only check terminated after page 1 and re-POSTed every label + // past index 50 on every sync. (Repro found during the milestone + // dedup fix: 9 unique milestones leaked past page 1 on a 74-row + // /milestones response.) + const existingLabels = new Set(); + const labelsPerPage = 50; + let labelsPage = 1; + let labelsFetched = 0; + while (true) { + const giteaLabelsRes = await httpGet( + `${config.giteaConfig.url}/api/v1/repos/${giteaOwner}/${repoName}/labels?page=${labelsPage}&limit=${labelsPerPage}`, + { + Authorization: `token ${decryptedConfig.giteaConfig.token}`, + } + ); + const pageLabels = Array.isArray(giteaLabelsRes.data) ? giteaLabelsRes.data : []; + if (!pageLabels.length) break; + for (const lbl of pageLabels) existingLabels.add(lbl.name); + labelsFetched += pageLabels.length; - const existingLabels = new Set( - giteaLabelsRes.data.map((label: any) => label.name) - ); + const linkHeader = giteaLabelsRes.headers.get("link") || ""; + if (/\brel="next"/.test(linkHeader)) { + labelsPage += 1; + continue; + } + // No Link header (or no rel=next). Fall back to X-Total-Count. + const totalStr = giteaLabelsRes.headers.get("x-total-count"); + const total = totalStr ? Number.parseInt(totalStr, 10) : NaN; + if (Number.isFinite(total) && labelsFetched < total) { + labelsPage += 1; + continue; + } + break; + } let mirroredCount = 0; for (const label of labels) { @@ -3435,6 +3467,9 @@ export async function mirrorGitRepoLabelsToGitea({ Authorization: `token ${decryptedConfig.giteaConfig.token}`, } ); + // Track locally so a duplicate in `labels` (shouldn't happen, + // but defensive) doesn't trigger a second POST in the same run. + existingLabels.add(label.name); mirroredCount++; } catch (error) { console.error( @@ -3508,17 +3543,54 @@ export async function mirrorGitRepoMilestonesToGitea({ return; } - // Get existing milestones from Gitea - const giteaMilestonesRes = await httpGet( - `${config.giteaConfig.url}/api/v1/repos/${giteaOwner}/${repoName}/milestones`, - { - Authorization: `token ${decryptedConfig.giteaConfig.token}`, - } - ); + // Get existing milestones from Gitea. Two correctness requirements: + // 1. `state=all` — Gitea's /milestones endpoint defaults to OPEN + // only, so without this every CLOSED GitHub milestone is + // misclassified as missing and re-POSTed on every sync. This + // was the root cause of the 11k+ duplicate-closed-milestone + // blowup observed in production. + // 2. Pagination via Link header (RFC 5988) — Gitea caps response + // size at `[api].MAX_RESPONSE_ITEMS` (default 50), so any repo + // with more than ~50 milestones in a given state silently + // truncates without it. Same Gitea-side cap that bit the + // issues / PRs pre-fetch in commit b76073b. + // Pagination signal: prefer Link header (RFC 5988) when present, but + // Gitea's /milestones endpoint does NOT emit a Link header — it only + // emits `X-Total-Count`. A strict Link-only check terminates after + // page 1 and re-POSTs every milestone past index 50 on every sync. + // (Repro: post-fix deploy on Subnet-Calculator leaked 9 unique + // milestones past page 1 of a 74-row /milestones response.) + const existingMilestones = new Set(); + const milestonesPerPage = 50; + let milestonesPage = 1; + let milestonesFetched = 0; + while (true) { + const giteaMilestonesRes = await httpGet( + `${config.giteaConfig.url}/api/v1/repos/${giteaOwner}/${repoName}/milestones?state=all&page=${milestonesPage}&limit=${milestonesPerPage}`, + { + Authorization: `token ${decryptedConfig.giteaConfig.token}`, + } + ); + const pageMilestones = Array.isArray(giteaMilestonesRes.data) + ? giteaMilestonesRes.data + : []; + if (!pageMilestones.length) break; + for (const ms of pageMilestones) existingMilestones.add(ms.title); + milestonesFetched += pageMilestones.length; - const existingMilestones = new Set( - giteaMilestonesRes.data.map((milestone: any) => milestone.title) - ); + const linkHeader = giteaMilestonesRes.headers.get("link") || ""; + if (/\brel="next"/.test(linkHeader)) { + milestonesPage += 1; + continue; + } + const totalStr = giteaMilestonesRes.headers.get("x-total-count"); + const total = totalStr ? Number.parseInt(totalStr, 10) : NaN; + if (Number.isFinite(total) && milestonesFetched < total) { + milestonesPage += 1; + continue; + } + break; + } let mirroredCount = 0; for (const milestone of milestones) { @@ -3536,6 +3608,9 @@ export async function mirrorGitRepoMilestonesToGitea({ Authorization: `token ${decryptedConfig.giteaConfig.token}`, } ); + // Track locally so a duplicate within `milestones` (shouldn't + // happen, but defensive) doesn't trigger a second POST. + existingMilestones.add(milestone.title); mirroredCount++; } catch (error) { console.error(