Skip to content

Commit 0a67db2

Browse files
authored
fix(services): batch the per-repo outcome-patterns snapshot read (#10165)
loadRepoOutcomePatternsMap fired one listSignalSnapshots query per registered repo, concurrently and unbatched, and discarded 99 of the up-to-100 full payloads each one returned to use exactly one — scaling DB round trips linearly with the installed-repo count on the contributor decision-pack build path. The bulk helper for exactly this shape already exists: listRecentSignalSnapshotsForTargets selects payload_json, batches at SIGNAL_SNAPSHOT_TARGET_KEY_SQL_BATCH keys per round trip, and takes an explicit maxPerTarget. Read every registered repo's latest snapshot in one bulk call (listRecentSignalSnapshotsForTargets(env, SIGNAL, fullNames, 1)) instead of the per-repo Promise.all loop, lowercasing the returned keys on the way out to preserve the map's existing lowercased-key contract (the helper keys by the exact targetKey string). The isRegistered filter, the single-repo loadOrComputeRepoOutcomePatternsResponse path, computeRepoOutcomePatterns, and listSignalSnapshots' signature are all unchanged; a registered repo with no snapshot is still absent from the map. Closes #10024
1 parent ffc4f09 commit 0a67db2

2 files changed

Lines changed: 75 additions & 8 deletions

File tree

src/services/repo-outcome-patterns.ts

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
listPullRequestDetailSyncStates,
55
listPullRequests,
66
listRecentMergedPullRequests,
7+
listRecentSignalSnapshotsForTargets,
78
listRepoPullRequestFiles,
89
listRepoPullRequestReviews,
910
listSignalSnapshots,
@@ -57,14 +58,19 @@ export async function loadOrComputeRepoOutcomePatternsResponse(env: Env, fullNam
5758

5859
export async function loadRepoOutcomePatternsMap(env: Env, repositories: Array<{ fullName: string; isRegistered: boolean }>): Promise<Map<string, RepoOutcomePatterns>> {
5960
const map = new Map<string, RepoOutcomePatterns>();
60-
await Promise.all(
61-
repositories
62-
.filter((repo) => repo.isRegistered)
63-
.map(async (repo) => {
64-
const latest = (await listSignalSnapshots(env, REPO_OUTCOME_PATTERNS_SIGNAL, repo.fullName))[0];
65-
if (latest) map.set(repo.fullName.toLowerCase(), latest.payload as unknown as RepoOutcomePatterns);
66-
}),
67-
);
61+
// #10024: one BULK read (batched internally at SIGNAL_SNAPSHOT_TARGET_KEY_SQL_BATCH keys/round-trip) instead
62+
// of one listSignalSnapshots query per registered repo, each of which pulled up to 100 full payloads to use
63+
// exactly one. listRecentSignalSnapshotsForTargets (not the Latest variant) is the one that selects
64+
// payload_json; maxPerTarget 1 = only the newest snapshot per repo. Mirrors repo-doc-refresh-runner's sweep.
65+
const fullNames = repositories.filter((repo) => repo.isRegistered).map((repo) => repo.fullName);
66+
const byTargetKey = await listRecentSignalSnapshotsForTargets(env, REPO_OUTCOME_PATTERNS_SIGNAL, fullNames, 1);
67+
for (const repo of repositories) {
68+
if (!repo.isRegistered) continue;
69+
// listRecentSignalSnapshotsForTargets keys by the exact targetKey string, so read by fullName and lowercase
70+
// on the way out to preserve the map's existing lowercased-key contract (decision-pack.ts's lookups).
71+
const latest = byTargetKey.get(repo.fullName)?.[0];
72+
if (latest) map.set(repo.fullName.toLowerCase(), latest.payload as unknown as RepoOutcomePatterns);
73+
}
6874
return map;
6975
}
7076

test/unit/repo-outcome-patterns-service.test.ts

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,4 +188,65 @@ describe("loadRepoOutcomePatternsMap", () => {
188188
]);
189189
expect([...map.keys()]).toEqual(["owner/a"]);
190190
});
191+
192+
const seedSnapshot = async (env: ReturnType<typeof createTestEnv>, fullName: string, summary: string, targetKey = fullName) => {
193+
await persistSignalSnapshot(env, {
194+
id: crypto.randomUUID(),
195+
signalType: REPO_OUTCOME_PATTERNS_SIGNAL,
196+
targetKey,
197+
repoFullName: fullName,
198+
payload: snapshotPayload(fullName, summary) as unknown as Record<string, never>,
199+
generatedAt: new Date().toISOString(),
200+
});
201+
};
202+
203+
it("#10024: returns exactly the registered repos' lowercased keys with their payloads; unregistered is absent", async () => {
204+
const env = createTestEnv();
205+
await seedSnapshot(env, "owner/one", "s1");
206+
await seedSnapshot(env, "owner/two", "s2");
207+
await seedSnapshot(env, "owner/three", "s3");
208+
await seedSnapshot(env, "owner/nope", "s-nope"); // unregistered
209+
const map = await loadRepoOutcomePatternsMap(env, [
210+
{ fullName: "owner/one", isRegistered: true },
211+
{ fullName: "owner/two", isRegistered: true },
212+
{ fullName: "owner/three", isRegistered: true },
213+
{ fullName: "owner/nope", isRegistered: false },
214+
]);
215+
expect([...map.keys()].sort()).toEqual(["owner/one", "owner/three", "owner/two"]);
216+
expect(map.get("owner/one")).toMatchObject({ summary: "s1" });
217+
expect(map.has("owner/nope")).toBe(false);
218+
});
219+
220+
it("#10024: the DB round-trip count does NOT grow with the registered-repo count (one batch, 3 vs 12 repos)", async () => {
221+
const countPrepares = async (repoCount: number): Promise<number> => {
222+
const env = createTestEnv();
223+
let prepares = 0;
224+
const realPrepare = env.DB.prepare.bind(env.DB);
225+
env.DB.prepare = ((sql: string) => {
226+
prepares += 1;
227+
return realPrepare(sql);
228+
}) as never;
229+
const repos = Array.from({ length: repoCount }, (_, i) => ({ fullName: `owner/repo-${i}`, isRegistered: true }));
230+
await loadRepoOutcomePatternsMap(env, repos);
231+
return prepares;
232+
};
233+
// 3 and 12 both fit one batch (< SIGNAL_SNAPSHOT_TARGET_KEY_SQL_BATCH = 90), so the prepare count is equal.
234+
expect(await countPrepares(3)).toBe(await countPrepares(12));
235+
});
236+
237+
it("#10024 REGRESSION: a stored targetKey whose casing differs from the requested fullName still resolves to a lowercased map key", async () => {
238+
const env = createTestEnv();
239+
// The request uses "owner/Mixed"; listRecentSignalSnapshotsForTargets keys by the exact requested string,
240+
// and the caller lowercases on the way out — so the map key is the lowercased form, never dropped.
241+
await seedSnapshot(env, "owner/Mixed", "mixed", "owner/Mixed");
242+
const map = await loadRepoOutcomePatternsMap(env, [{ fullName: "owner/Mixed", isRegistered: true }]);
243+
expect([...map.keys()]).toEqual(["owner/mixed"]);
244+
expect(map.get("owner/mixed")).toMatchObject({ summary: "mixed" });
245+
});
246+
247+
it("#10024: no registered repos ⇒ empty map with no bulk read", async () => {
248+
const env = createTestEnv();
249+
const map = await loadRepoOutcomePatternsMap(env, [{ fullName: "owner/x", isRegistered: false }]);
250+
expect(map.size).toBe(0);
251+
});
191252
});

0 commit comments

Comments
 (0)