diff --git a/src/db/repositories.ts b/src/db/repositories.ts index f7f4a9916e..d82d073e78 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4031,6 +4031,47 @@ export async function listSignalSnapshots(env: Env, signalType: string, targetKe return rows.map(toSignalSnapshotRecord); } +/** Bulk variant of `listSignalSnapshots` for callers that need the LATEST snapshot per target key across many + * keys in one round trip (#3202 review finding: a per-repo loop here made the daily repo-doc refresh sweep + * scale linearly in DB round trips with the installed-repo count). Keyed by the exact `targetKey` string, same + * casing convention as `listSignalSnapshots` -- callers that key by lowercased repo name must lowercase both + * the input and the returned map's keys themselves. */ +export async function listLatestSignalSnapshotsForTargets( + env: Env, + signalType: string, + targetKeys: readonly string[], +): Promise> { + const result = new Map(); + if (targetKeys.length === 0) return result; + const placeholders = targetKeys.map(() => "?").join(", "); + const { results } = await env.DB.prepare( + ` + SELECT id, signal_type, target_key, repo_full_name, generated_at + FROM ( + SELECT + id, signal_type, target_key, repo_full_name, generated_at, + row_number() OVER (PARTITION BY target_key ORDER BY generated_at DESC, id DESC) AS snapshot_rank + FROM signal_snapshots + WHERE signal_type = ? AND target_key IN (${placeholders}) + ) + WHERE snapshot_rank = 1 + `, + ) + .bind(signalType, ...targetKeys) + .all<{ id: string; signal_type: string; target_key: string; repo_full_name: string | null; generated_at: string }>(); + for (const row of results) { + result.set(row.target_key, { + id: row.id, + signalType: row.signal_type, + targetKey: row.target_key, + repoFullName: row.repo_full_name, + payload: {}, + generatedAt: row.generated_at, + }); + } + return result; +} + export async function listLatestSignalSnapshotsByTarget( env: Env, options: { limit?: number; generatedAfter?: string; maxTargetKeyChars?: number } = {}, diff --git a/src/github/repo-doc-refresh-runner.ts b/src/github/repo-doc-refresh-runner.ts index 251e1cd1f6..e2b066c751 100644 --- a/src/github/repo-doc-refresh-runner.ts +++ b/src/github/repo-doc-refresh-runner.ts @@ -9,10 +9,11 @@ // this, matching #3002's own "manifest-only, no DB layer" precedent for this whole feature. The marker is // recorded here (not in the sweep itself) so a MANUAL trigger also resets that clock, keeping the sweep from // immediately re-checking a repo an operator just refreshed by hand. -import { getRepositorySettings, listSignalSnapshots, persistSignalSnapshot } from "../db/repositories"; +import { getRepositorySettings, listLatestSignalSnapshotsForTargets, listSignalSnapshots, persistSignalSnapshot } from "../db/repositories"; import { resolveRepoActionMode } from "./client"; import { openRepoDocPullRequest, type RepoDocPullRequestResult } from "./repo-doc-pr"; import { nowIso } from "../utils/json"; +import type { SignalSnapshotRecord } from "../types"; const REPO_DOC_REFRESH_ATTEMPT_SIGNAL_TYPE = "repo-doc-refresh-attempt"; @@ -23,6 +24,15 @@ export async function getLastRepoDocRefreshAttemptedAt(env: Env, repoFullName: s return snapshots[0]?.generatedAt ?? null; } +/** Bulk variant for the sweep's fan-out (#3202 review finding): one round trip for every candidate repo + * instead of one `getLastRepoDocRefreshAttemptedAt` call per repo. Keyed by the exact `repoFullName` string + * passed in, same casing convention as the single-repo lookup above. A repo absent from the returned map has + * never been attempted -- callers should read `.get(repoFullName)?.generatedAt ?? null`, same pattern as the + * single-repo lookup above. */ +export async function getLastRepoDocRefreshAttemptedAtBulk(env: Env, repoFullNames: readonly string[]): Promise> { + return listLatestSignalSnapshotsForTargets(env, REPO_DOC_REFRESH_ATTEMPT_SIGNAL_TYPE, repoFullNames); +} + async function recordRepoDocRefreshAttempt(env: Env, repoFullName: string): Promise { await persistSignalSnapshot(env, { id: crypto.randomUUID(), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 70d9c613a4..93b6aeca75 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -357,7 +357,7 @@ import { loadRepoReviewContext, } from "../signals/focus-manifest-loader"; import { resolveRepositorySettings } from "../settings/repository-settings"; -import { getLastRepoDocRefreshAttemptedAt, performRepoDocRefresh } from "../github/repo-doc-refresh-runner"; +import { getLastRepoDocRefreshAttemptedAtBulk, performRepoDocRefresh } from "../github/repo-doc-refresh-runner"; import { isRepoDocRefreshDue } from "../review/repo-doc-refresh-schedule"; import type { LocalBranchAnalysisInput } from "../signals/local-branch"; import { @@ -1725,14 +1725,22 @@ async function fanOutRepoDocRefreshSweepJobs(env: Env, requestedBy: "schedule" | const now = nowIso(); const repoFullNames = (await listRepositories(env)).map((repo) => repo.fullName); const manifests = await loadRepoFocusManifests(env, repoFullNames); - const due: string[] = []; - for (const repoFullName of repoFullNames) { + const enabledRepos = repoFullNames.flatMap((repoFullName) => { const manifest = manifests.get(repoFullName.toLowerCase()); - if (!manifest?.repoDocGeneration.enabled) continue; - const lastAttemptedAt = await getLastRepoDocRefreshAttemptedAt(env, repoFullName); - if (!isRepoDocRefreshDue(lastAttemptedAt, manifest.repoDocGeneration.refreshIntervalDays, now)) continue; - due.push(repoFullName); - } + return manifest?.repoDocGeneration.enabled ? [{ repoFullName, manifest }] : []; + }); + // Bulk-loaded in ONE round trip rather than one `getLastRepoDocRefreshAttemptedAt` call per repo (#3202 + // review finding) -- this sweep runs daily across every installed repo, so a per-repo query here would scale + // linearly in DB round trips with the installed-repo count. + const lastAttempts = await getLastRepoDocRefreshAttemptedAtBulk( + env, + enabledRepos.map((entry) => entry.repoFullName), + ); + const due = enabledRepos + .filter((entry) => + isRepoDocRefreshDue(lastAttempts.get(entry.repoFullName)?.generatedAt ?? null, entry.manifest.repoDocGeneration.refreshIntervalDays, now), + ) + .map((entry) => entry.repoFullName); await Promise.all( due.map((repoFullName, index) => { const message: JobMessage = { type: "repo-doc-refresh-sweep", requestedBy, repoFullName }; diff --git a/test/unit/data-spine.test.ts b/test/unit/data-spine.test.ts index f32ba98d50..11cfa01efe 100644 --- a/test/unit/data-spine.test.ts +++ b/test/unit/data-spine.test.ts @@ -20,6 +20,7 @@ import { listPullRequestReviews, listRecentMergedPullRequests, listRepoLabels, + listLatestSignalSnapshotsForTargets, listRepoSyncStates, listSignalSnapshots, countOpenIssues, @@ -489,3 +490,26 @@ describe("data spine repositories", () => { await expect(updatePullRequestSlopAssessment(env, "owner/sloppr", 999, { slopRisk: 5, slopBand: "low" })).resolves.toBeUndefined(); }); }); + +describe("listLatestSignalSnapshotsForTargets (#3202 — bulk latest-per-target lookup)", () => { + it("returns an empty map without querying the DB for an empty target-key list", async () => { + const env = createTestEnv(); + expect(await listLatestSignalSnapshotsForTargets(env, "queue-health", [])).toEqual(new Map()); + }); + + it("resolves the LATEST snapshot per target key in one call, ignores other signal types, and omits an unmatched key", async () => { + const env = createTestEnv(); + await persistSignalSnapshot(env, { id: "a-old", signalType: "repo-doc-refresh-attempt", targetKey: "owner/a", payload: {}, generatedAt: "2026-01-01T00:00:00.000Z" }); + await persistSignalSnapshot(env, { id: "a-new", signalType: "repo-doc-refresh-attempt", targetKey: "owner/a", payload: {}, generatedAt: "2026-06-01T00:00:00.000Z" }); + await persistSignalSnapshot(env, { id: "b-only", signalType: "repo-doc-refresh-attempt", targetKey: "owner/b", payload: {}, generatedAt: "2026-03-01T00:00:00.000Z" }); + // Same target key, but a DIFFERENT signal type -- must not leak into the result. + await persistSignalSnapshot(env, { id: "a-other-signal", signalType: "queue-health", targetKey: "owner/a", payload: {}, generatedAt: "2026-12-01T00:00:00.000Z" }); + + const result = await listLatestSignalSnapshotsForTargets(env, "repo-doc-refresh-attempt", ["owner/a", "owner/b", "owner/c"]); + + expect(result.size).toBe(2); + expect(result.get("owner/a")).toMatchObject({ id: "a-new", generatedAt: "2026-06-01T00:00:00.000Z" }); + expect(result.get("owner/b")).toMatchObject({ id: "b-only", generatedAt: "2026-03-01T00:00:00.000Z" }); + expect(result.has("owner/c")).toBe(false); + }); +}); diff --git a/test/unit/repo-doc-refresh-runner.test.ts b/test/unit/repo-doc-refresh-runner.test.ts index 1e32a48e57..3e19a8c45b 100644 --- a/test/unit/repo-doc-refresh-runner.test.ts +++ b/test/unit/repo-doc-refresh-runner.test.ts @@ -1,6 +1,6 @@ import { generateKeyPairSync } from "node:crypto"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { getLastRepoDocRefreshAttemptedAt, performRepoDocRefresh } from "../../src/github/repo-doc-refresh-runner"; +import { getLastRepoDocRefreshAttemptedAt, getLastRepoDocRefreshAttemptedAtBulk, performRepoDocRefresh } from "../../src/github/repo-doc-refresh-runner"; import { upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { createTestEnv } from "../helpers/d1"; @@ -36,6 +36,26 @@ describe("getLastRepoDocRefreshAttemptedAt (#3003)", () => { }); }); +describe("getLastRepoDocRefreshAttemptedAtBulk (#3202 — N+1 fix)", () => { + it("returns an empty map without querying the DB for an empty repo list", async () => { + const env = createTestEnv(); + expect(await getLastRepoDocRefreshAttemptedAtBulk(env, [])).toEqual(new Map()); + }); + + it("resolves attempted repos in one call and omits a repo that was never attempted", async () => { + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "attempted", full_name: "owner/attempted", private: false, owner: { login: "owner" } }); + // repoDocGeneration stays disabled -- performRepoDocRefresh still records an attempt marker on decline. + const result = await performRepoDocRefresh(env, "owner/attempted"); + expect(result.opened).toBe(false); + const attemptedAt = await getLastRepoDocRefreshAttemptedAt(env, "owner/attempted"); + + const bulk = await getLastRepoDocRefreshAttemptedAtBulk(env, ["owner/attempted", "owner/never-attempted"]); + expect(bulk.get("owner/attempted")?.generatedAt).toBe(attemptedAt); + expect(bulk.has("owner/never-attempted")).toBe(false); + }); +}); + describe("performRepoDocRefresh (#3003)", () => { afterEach(() => { vi.unstubAllGlobals();