From 60a2ed757e94f4f40fe6c900ab6c50a8895cf400 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Fri, 31 Jul 2026 19:02:08 +0900 Subject: [PATCH] fix(orb): do not un-install repos from a page-capped installation-repositories crawl syncBrokeredInstalledRepos reconciled destructively against fetchAllInstallationRepos' list, but that loop exits either on a short page (list complete) OR on hitting MAX_INSTALLATION_REPOS_PAGES with a full page still pending (list truncated). On truncation the reconcile ran anyway: every locally-installed repo beyond item 5,000 was absent from the fresh set, so markRepositoriesRemovedFromInstallation flipped it to isInstalled: false and cleared its installationId -- and since every core feature is gated on isInstalled, those repos went silently dark with no self-heal. Make fetchAllInstallationRepos report a truncation flag (true only when the page cap is exhausted with a full last page) and skip markRepositoriesRemovedFromInstallation entirely when truncated -- a partial list is valid positive evidence (still upsert every fetched repo) but not valid negative evidence. A truncated sync still returns 'synced' (the cron must not read it as an outage) with removedCount 0, and logs one structured error so the suppressed reconcile is observable. Mirrors listMigrationFilenamesAtRef's 'a bound-hit is inconclusive' posture. A complete crawl upserts and reconciles exactly as before; MAX_INSTALLATION_REPOS_PAGES and the result union members are unchanged. Closes #10033 --- src/orb/installed-repos-sync.ts | 20 +++++-- test/unit/orb-installed-repos-sync.test.ts | 62 +++++++++++++++++++++- 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/src/orb/installed-repos-sync.ts b/src/orb/installed-repos-sync.ts index 497ef242e9..da8c0d4ed6 100644 --- a/src/orb/installed-repos-sync.ts +++ b/src/orb/installed-repos-sync.ts @@ -27,8 +27,12 @@ export type InstalledReposSyncResult = /** Fetch every repo currently accessible to this brokered installation via GitHub's own * `GET /installation/repositories`, paginated. Uses the broker token directly -- its response already carries * the bound installationId, so no separate token mint is needed for this call. */ -async function fetchAllInstallationRepos(token: string, fetchImpl: typeof fetch): Promise { +export async function fetchAllInstallationRepos(token: string, fetchImpl: typeof fetch): Promise<{ repos: GitHubRepositoryPayload[]; truncated: boolean }> { const repos: GitHubRepositoryPayload[] = []; + // #10033: TRUE only when the loop exhausts MAX_INSTALLATION_REPOS_PAGES with a still-full last page -- a + // short page means the list is complete, but a full final page means there is an untraversed tail. The caller + // must not treat a truncated list as negative evidence and un-install the repos it never saw. + let truncated = false; for (let page = 1; page <= MAX_INSTALLATION_REPOS_PAGES; page += 1) { const res = await fetchImpl(`https://api.github.com/installation/repositories?per_page=${GITHUB_INSTALLATION_REPOS_PAGE_SIZE}&page=${page}`, { headers: githubHeaders({ token }), @@ -39,8 +43,9 @@ async function fetchAllInstallationRepos(token: string, fetchImpl: typeof fetch) const batch = body.repositories ?? []; repos.push(...batch); if (batch.length < GITHUB_INSTALLATION_REPOS_PAGE_SIZE) break; + if (page === MAX_INSTALLATION_REPOS_PAGES) truncated = true; } - return repos; + return { repos, truncated }; } /** @@ -65,10 +70,19 @@ export async function syncBrokeredInstalledRepos( if (!isOrbBrokerMode(env)) return { status: "skipped" }; try { const { token, installationId } = await fetchBrokeredInstallationToken(env, fetchImpl); - const repos = await fetchAllInstallationRepos(token, fetchImpl); + const { repos, truncated } = await fetchAllInstallationRepos(token, fetchImpl); + // A partial list is valid POSITIVE evidence: still upsert every repo we DID fetch as installed. for (const repo of repos) { await upsertRepositoryFromGitHub(env, repo, installationId); } + if (truncated) { + // #10033: a page-capped crawl is not valid NEGATIVE evidence -- reconciling against it would flip every + // installed repo in the untraversed tail to isInstalled: false and take them silently dark. Skip the + // destructive reconcile entirely, but still report `synced` (the sync did useful upsert work; the cron + // must not read this as an outage) with removedCount 0, and log so an operator sees the suppression. + console.error(JSON.stringify({ level: "error", event: "installed_repos_sync_truncated", installationId, repoCount: repos.length })); + return { status: "synced", installationId, repoCount: repos.length, removedCount: 0 }; + } const freshFullNames = new Set(repos.map((repo) => repo.full_name)); const previouslyInstalled = await listInstalledRepoFullNamesForInstallation(env, installationId); const staleFullNames = previouslyInstalled.filter((fullName) => !freshFullNames.has(fullName)); diff --git a/test/unit/orb-installed-repos-sync.test.ts b/test/unit/orb-installed-repos-sync.test.ts index 5a78a2d9b5..345c625668 100644 --- a/test/unit/orb-installed-repos-sync.test.ts +++ b/test/unit/orb-installed-repos-sync.test.ts @@ -1,8 +1,13 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import * as repositories from "../../src/db/repositories"; import { getRepository } from "../../src/db/repositories"; -import { syncBrokeredInstalledRepos } from "../../src/orb/installed-repos-sync"; +import { fetchAllInstallationRepos, syncBrokeredInstalledRepos } from "../../src/orb/installed-repos-sync"; import { createTestEnv } from "../helpers/d1"; +afterEach(() => { + vi.restoreAllMocks(); +}); + type Call = { url: string; init?: RequestInit | undefined }; /** Router-style fetch stub: the broker token exchange goes to `.../v1/orb/token`, everything else is treated @@ -105,4 +110,57 @@ describe("syncBrokeredInstalledRepos", () => { const result = await syncBrokeredInstalledRepos(env, fetchImpl); expect(result).toEqual({ status: "synced", installationId: 42, repoCount: 0, removedCount: 0 }); }); + + // #10033: fetchAllInstallationRepos must report truncation so the caller does not treat a page-capped list + // as negative evidence and un-install the untraversed tail. + const fullPage = (start: number) => Response.json({ repositories: Array.from({ length: 100 }, (_, i) => repoPayload(`owner/repo-${start + i}`)) }); + + it("#10033: fetchAllInstallationRepos reports truncated=true only when the page cap is hit with a full last page", async () => { + // 50 full pages → the cap is exhausted while a full page is still pending → truncated, 5000 repos. + const capped = routedFetch({ tokenResponse: tokenResponse(42), pages: Array.from({ length: 50 }, (_, p) => fullPage(p * 100)) }); + await expect(fetchAllInstallationRepos("tok", capped.fetchImpl)).resolves.toMatchObject({ truncated: true, repos: expect.any(Array) }); + expect((await fetchAllInstallationRepos("tok", routedFetch({ tokenResponse: tokenResponse(42), pages: Array.from({ length: 50 }, (_, p) => fullPage(p * 100)) }).fetchImpl)).repos).toHaveLength(5000); + + // 100 then 40 (short page) → complete, 140 repos. + const short = routedFetch({ tokenResponse: tokenResponse(42), pages: [fullPage(0), Response.json({ repositories: Array.from({ length: 40 }, (_, i) => repoPayload(`owner/tail-${i}`)) })] }); + await expect(fetchAllInstallationRepos("tok", short.fetchImpl)).resolves.toMatchObject({ truncated: false }); + + // Exactly 100 then 0 (empty page) → complete, 100 repos. + const emptyTail = routedFetch({ tokenResponse: tokenResponse(42), pages: [fullPage(0), Response.json({ repositories: [] })] }); + const res = await fetchAllInstallationRepos("tok", emptyTail.fetchImpl); + expect(res).toMatchObject({ truncated: false }); + expect(res.repos).toHaveLength(100); + }); + + it("#10033: a truncated crawl calls markRepositoriesRemovedFromInstallation ZERO times but still upserts every fetched repo", async () => { + const env = createTestEnv({ ORB_ENROLLMENT_SECRET: "orbsec_x" }); + vi.spyOn(console, "error").mockImplementation(() => {}); + const removeSpy = vi.spyOn(repositories, "markRepositoriesRemovedFromInstallation"); + const upsertSpy = vi.spyOn(repositories, "upsertRepositoryFromGitHub"); + const capped = routedFetch({ tokenResponse: tokenResponse(42), pages: Array.from({ length: 50 }, (_, p) => fullPage(p * 100)) }); + await syncBrokeredInstalledRepos(env, capped.fetchImpl); + expect(removeSpy).not.toHaveBeenCalled(); + expect(upsertSpy).toHaveBeenCalledTimes(5000); + }); + + it("#10033 REGRESSION: a page-capped installation-repositories crawl must not un-install the untraversed tail", async () => { + const env = createTestEnv({ ORB_ENROLLMENT_SECRET: "orbsec_x" }); + // First a COMPLETE sync installs a repo that a later truncated crawl will not surface. + const first = routedFetch({ tokenResponse: tokenResponse(42), pages: [Response.json({ repositories: [repoPayload("owner/in-the-tail")] })] }); + await syncBrokeredInstalledRepos(env, first.fetchImpl); + await expect(getRepository(env, "owner/in-the-tail")).resolves.toMatchObject({ isInstalled: true }); + + // Now a TRUNCATED crawl (50 full pages, none of them the tail repo): the reconcile must be suppressed. + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const capped = routedFetch({ tokenResponse: tokenResponse(42), pages: Array.from({ length: 50 }, (_, p) => fullPage(p * 100)) }); + const result = await syncBrokeredInstalledRepos(env, capped.fetchImpl); + + expect(result).toEqual({ status: "synced", installationId: 42, repoCount: 5000, removedCount: 0 }); + // The tail repo the truncated crawl never saw stays installed — NOT flipped to isInstalled: false. + await expect(getRepository(env, "owner/in-the-tail")).resolves.toMatchObject({ isInstalled: true, installationId: 42 }); + // One structured error line records the suppressed reconcile. + const truncWarns = errorSpy.mock.calls.map((c) => String(c[0])).filter((m) => m.includes("installed_repos_sync_truncated")); + expect(truncWarns).toHaveLength(1); + expect(truncWarns[0]).toContain("\"repoCount\":5000"); + }); });