Skip to content

orb(installed-repos-sync): do not un-install repos from a page-capped installation-repositories list #10033

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

syncBrokeredInstalledRepos treats the list returned by fetchAllInstallationRepos as the complete,
authoritative repo set and reconciles destructively against it
(src/orb/installed-repos-sync.ts:66-79):

    const { token, installationId } = await fetchBrokeredInstallationToken(env, fetchImpl);
    const repos = await fetchAllInstallationRepos(token, fetchImpl);
    for (const repo of repos) {
      await upsertRepositoryFromGitHub(env, repo, installationId);
    }
    const freshFullNames = new Set(repos.map((repo) => repo.full_name));
    const previouslyInstalled = await listInstalledRepoFullNamesForInstallation(env, installationId);
    const staleFullNames = previouslyInstalled.filter((fullName) => !freshFullNames.has(fullName));
    await markRepositoriesRemovedFromInstallation(env, installationId, staleFullNames);

But fetchAllInstallationRepos has two exits, and the caller cannot tell them apart
(src/orb/installed-repos-sync.ts:30-44):

  for (let page = 1; page <= MAX_INSTALLATION_REPOS_PAGES; page += 1) {
    
    const batch = body.repositories ?? [];
    repos.push(...batch);
    if (batch.length < GITHUB_INSTALLATION_REPOS_PAGE_SIZE) break;
  }
  return repos;

The loop exits either because GitHub returned a short page (the list is complete) or because it hit
MAX_INSTALLATION_REPOS_PAGES with a full 100-item page still pending (the list is truncated). In the second
case the reconcile below runs anyway: every locally-installed repo beyond item 5,000 is absent from
freshFullNames, so markRepositoriesRemovedFromInstallation flips it to isInstalled: false and clears its
installationId (src/db/repositories.ts:270-278). Per this file's own header comment, every core feature is
gated on isInstalled, so those repos go silently dark — and nothing self-heals them: the next tick fetches
the same first 5,000 and un-installs the same tail again.

The cap comment (src/orb/installed-repos-sync.ts:17-20) explains why the bound exists but not what the
caller should do when it binds. Every sibling in this codebase treats a bound-hit as inconclusive rather
than as ground truth:

  • listMigrationFilenamesAtRef returns null on a truncated tree — "an incomplete live snapshot is
    inconclusive, not evidence" (src/github/migration-tree.ts:41-44).
  • fetchPullRequestFiles pushes an explicit truncation warning so the PR is held for human review rather than
    evaluated on a partial file set (src/github/backfill.ts:2469-2476).
  • fetchPagedSegment records a distinct "capped" segment status with its own warning and a resume cursor,
    never a "complete" one (src/github/backfill.ts:4785-4792).

This sync is the only one of the four that performs a destructive write off a possibly-truncated read, and
it is the only one with no capped/inconclusive signal at all.

Requirements

  • fetchAllInstallationRepos MUST report whether the crawl completed. It MUST return the repo list together
    with a truncation indicator that is true exactly when the loop exited by exhausting
    MAX_INSTALLATION_REPOS_PAGES while the last fetched page was full, and false when it exited on a short
    page.
  • syncBrokeredInstalledRepos MUST skip the markRepositoriesRemovedFromInstallation call entirely when the
    crawl was truncated. The upsertRepositoryFromGitHub loop MUST still run for the repos that WERE fetched —
    a partial list is still valid positive evidence, it is just not valid negative evidence.
  • A truncated sync MUST still return a "synced" result (not "failed") so the cron does not treat it as an
    outage, and its removedCount MUST be 0.
  • A truncated sync MUST emit one structured console.error line ({ level: "error", event: …, installationId, repoCount }) so an operator can see that the reconcile was suppressed, matching the structured-log shape
    used elsewhere in src/orb/** (e.g. src/orb/webhook.ts:129-141).
  • What must NOT change: MAX_INSTALLATION_REPOS_PAGES (50) and GITHUB_INSTALLATION_REPOS_PAGE_SIZE (100)
    keep their current values; the isOrbBrokerMode early return still yields { status: "skipped" }; a thrown
    broker/GitHub error still yields { status: "failed", reason }; a COMPLETE crawl still upserts and still
    reconciles exactly as it does today, including removedCount.
  • The InstalledReposSyncResult union MUST keep its existing three members and their existing field names —
    extend the "synced" member if you need to surface truncation, do not rename or remove anything.

⚠️ Required pattern: mirror listMigrationFilenamesAtRef's posture at src/github/migration-tree.ts:41-44
— a bound-hit is inconclusive, so the code that would act on "absence" simply does not run. What does NOT
satisfy this issue: (a) raising MAX_INSTALLATION_REPOS_PAGES instead of handling the cap — the cap will
still bind eventually and the defect is unchanged; (b) removing the reconcile entirely, which reintroduces
the stale-isInstalled rows this sync exists to clean up; (c) returning { status: "failed" } on
truncation, which makes the cron report an outage for a sync that did useful work; (d) a test-only PR.

Deliverables

  • fetchAllInstallationRepos in src/orb/installed-repos-sync.ts reports truncation. Exact expectation:
    given a fetchImpl that returns a full 100-repo page for every one of 50 pages, it reports
    truncated: true with 5,000 repos; given one that returns 100 then 40, it reports truncated: false
    with 140 repos; given one that returns exactly 100 then 0, it reports truncated: false with 100 repos.
  • syncBrokeredInstalledRepos calls markRepositoriesRemovedFromInstallation zero times on a
    truncated crawl, and still calls upsertRepositoryFromGitHub once per fetched repo. Asserted in
    test/unit/orb-installed-repos-sync.test.ts with spies on both.
  • A test in test/unit/orb-installed-repos-sync.test.ts pinning the unchanged complete-crawl path: a
    locally-installed repo absent from a complete fresh list IS passed to
    markRepositoriesRemovedFromInstallation, and removedCount reflects it.
  • A test in test/unit/orb-installed-repos-sync.test.ts asserting the truncated result is
    { status: "synced", installationId, repoCount: 5000, removedCount: 0 } and that one structured
    console.error line was emitted.
  • A regression test at test/unit/orb-installed-repos-sync.test.ts named for this bug (e.g.
    "REGRESSION: a page-capped installation-repositories crawl must not un-install the untraversed tail").

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that returns the truncation flag from fetchAllInstallationRepos but never consults it before the
markRepositoriesRemovedFromInstallation call — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts; src/orb/installed-repos-sync.ts is
measured and gated.

Both arms of every branch this change touches need a test: the page <= MAX_INSTALLATION_REPOS_PAGES loop
condition (bound reached and not reached), the batch.length < GITHUB_INSTALLATION_REPOS_PAGE_SIZE short-page
exit (true and false), the new truncated-vs-complete decision at the reconcile call site, and the existing
!res.ok throw path and isOrbBrokerMode early return, which must both stay covered.

This change is NOT in packages/loopover-engine/src/**, so the dual-upload engine-coverage rule does not apply.

Expected Outcome

A brokered self-host whose installation-repositories crawl hits its 50-page bound keeps upserting the repos it
did see but no longer flips the untraversed tail to isInstalled: false, so those repos do not silently lose
every feature gated on installation state on every sync tick. A complete crawl reconciles exactly as before,
and the suppressed reconcile is visible to an operator in the logs instead of being indistinguishable from a
normal sync.

Links & Resources

  • src/orb/installed-repos-sync.ts:30-44fetchAllInstallationRepos, whose two exits are indistinguishable
  • src/orb/installed-repos-sync.ts:66-79 — the destructive reconcile that runs regardless
  • src/db/repositories.ts:270-278markRepositoriesRemovedFromInstallation, which clears isInstalled and installationId
  • src/orb/installed-repos-sync.ts:1-9 — the header explaining that every core feature is gated on isInstalled
  • src/github/migration-tree.ts:41-44 — "an incomplete live snapshot is inconclusive, not evidence"
  • src/github/backfill.ts:2469-2476, src/github/backfill.ts:4785-4792 — the other two bound-hit-is-inconclusive precedents

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions