Dedup the PR changed-files fetch shared by autoLabelBot and nitpickBot (#8584)

Stack from [ghstack](https://github.com/ezyang/ghstack/tree/0.14.0)
(oldest at bottom):
* __->__ #8584
* #8582
* #8580
* #8579
* #8578
* #8576

**Impact:** PyTorchBot webhook handlers (autoLabelBot, nitpickBot) —
GitHub API usage only, no behavior change
**Risk:** low

## What
Adds a short-TTL, head-sha-keyed cache (`getFilesChangedByPrCached`)
around the paginated `GET /pulls/{n}/files` fetch, and points both
autoLabelBot and nitpickBot at it.

## Why
Both bots run on the same `pull_request` delivery and each independently
paginated the PR's changed files, doubling that read on every PR push.
The two handlers are staggered (nitpick loads its config first), so
caching the resolved result — not just an in-flight promise — lets the
second handler reuse the first's fetch. This halves the file-list reads
per push, part of the broader effort to cut PyTorchBot's GitHub
rate-limit consumption.

## Notes
- Cache is keyed on head sha so a re-push misses naturally; the 60s TTL
only bounds memory.
- Failed fetches are dropped from the cache immediately so a later
handler re-fetches, matching the previous uncached on-error behavior.
- Tests clear the cache in `beforeEach` to keep them independent.

Signed-off-by: Jean Schmidt <contato@jschmidt.me>

Author: jeanschmidt

torchci/lib/bot/autoLabelBot.ts

@@ -12,7 +12,7 @@ import {
   addLabels,
   CachedIssueTracker,
   CachedLabelerConfigTracker,
-  getFilesChangedByPr,
+  getFilesChangedByPrCached,
   hasApprovedPullRuns,
   hasWritePermissions,
   isPyTorchbotSupportedOrg,
@@ -544,11 +544,13 @@ function myBot(app: Probot): void {
       );
       const repo = context.payload.repository.name;
       const title = context.payload.pull_request.title;
-      const filesChanged = await getFilesChangedByPr(
+      const filesChanged = await getFilesChangedByPrCached(
         context.octokit,
+        context.id,
         owner,
         repo,
-        context.payload.pull_request.number
+        context.payload.pull_request.number,
+        context.payload.pull_request.head.sha
       );
       context.log({ labels, title, filesChanged });
 

torchci/lib/bot/nitpickBot.ts

@@ -1,7 +1,7 @@
 import * as yaml from "js-yaml";
 import { minimatch } from "minimatch";
 import { Context, Probot } from "probot";
-import { getFilesChangedByPr, isPyTorchPyTorch } from "./utils";
+import { getFilesChangedByPrCached, isPyTorchPyTorch } from "./utils";
 
 // Implements logic similar to https://github.com/ethanis/nitpicker.
 // Reads `.github/nitpicks.yml` from the repo's default branch and posts
@@ -162,11 +162,13 @@ export default function nitpickBot(app: Probot): void {
         return;
       }
 
-      const filesChanged = await getFilesChangedByPr(
+      const filesChanged = await getFilesChangedByPrCached(
         context.octokit,
+        context.id,
         owner,
         repo,
-        prNum
+        prNum,
+        context.payload.pull_request.head.sha
       );
       const matched = getMatchingRules(filesChanged, rules);
       const newBody = formNitpickComment(matched);

torchci/lib/bot/utils.ts

@@ -406,3 +406,63 @@ export async function getFilesChangedByPr(
   );
   return filesChangedRes.map((f: any) => f.filename);
 }
+
+export const FILES_CHANGED_CACHE_TTL_MS = 60 * 1000;
+
+interface FilesChangedCacheEntry {
+  promise: Promise<string[]>;
+  expiresAt: number;
+}
+
+// autoLabelBot and nitpickBot both fetch a PR's changed files on the same
+// pull_request delivery, staggered (nitpick loads its config first). Caching
+// the resolved fetch — not just the in-flight promise — lets the second
+// handler reuse the first's paginated GET /pulls/{n}/files. The delivery id
+// scopes an entry to a single webhook delivery: GitHub computes the file list
+// against the PR's base, and retargeting the base changes that list without
+// changing head.sha, so an entry is only ever safe to reuse within the
+// delivery that produced it. The TTL only bounds memory.
+const filesChangedCache = new Map<string, FilesChangedCacheEntry>();
+
+export function getFilesChangedByPrCached(
+  octokit: Octokit,
+  deliveryId: string,
+  owner: string,
+  repo: string,
+  prNumber: number,
+  headSha: string
+): Promise<string[]> {
+  const key = `${deliveryId}/${owner}/${repo}/${prNumber}/${headSha}`;
+  const now = Date.now();
+
+  const cached = filesChangedCache.get(key);
+  if (cached && cached.expiresAt > now) {
+    return cached.promise;
+  }
+
+  for (const [k, entry] of filesChangedCache) {
+    if (entry.expiresAt <= now) {
+      filesChangedCache.delete(k);
+    }
+  }
+
+  const promise = getFilesChangedByPr(octokit, owner, repo, prNumber);
+  filesChangedCache.set(key, {
+    promise,
+    expiresAt: now + FILES_CHANGED_CACHE_TTL_MS,
+  });
+  // A failed fetch must not be served for the rest of the TTL; drop it so a
+  // later handler re-fetches (matching the uncached on-error behavior).
+  promise.catch(() => {
+    if (filesChangedCache.get(key)?.promise === promise) {
+      filesChangedCache.delete(key);
+    }
+  });
+
+  return promise;
+}
+
+/** Clear the in-memory files-changed cache (useful for testing). */
+export function clearFilesChangedCache(): void {
+  filesChangedCache.clear();
+}

torchci/test/autoLabelBot.test.ts

@@ -12,6 +12,10 @@ import { mockAddLabels } from "./utils";
 
 nock.disableNetConnect();
 
+beforeEach(() => {
+  botUtils.clearFilesChangedCache();
+});
+
 describe("auto-label-bot", () => {
   let probot: Probot;
   function emptyMockConfig(repoFullName: string) {

torchci/test/nitpickBot.test.ts

@@ -5,6 +5,7 @@ import nitpickBot, {
   NITPICK_COMMENT_START,
   parseNitpickConfig,
 } from "lib/bot/nitpickBot";
+import { clearFilesChangedCache } from "lib/bot/utils";
 import nock from "nock";
 import { Probot } from "probot";
 import { handleScope, requireDeepCopy } from "./common";
@@ -175,6 +176,7 @@ describe("nitpickBot probot integration", () => {
 `;
 
   beforeEach(() => {
+    clearFilesChangedCache();
     probot = utils.testProbot();
     probot.load(nitpickBot);
     utils.mockAccessToken();

torchci/test/utils.test.ts

@@ -1,4 +1,8 @@
-import { hasApprovedPullRuns } from "lib/bot/utils";
+import {
+  clearFilesChangedCache,
+  getFilesChangedByPrCached,
+  hasApprovedPullRuns,
+} from "lib/bot/utils";
 import nock from "nock";
 import { Probot } from "probot";
 import * as utils from "./utils";
@@ -89,3 +93,82 @@ describe("utils: hasApprovedPullRuns", () => {
     await checkhasApprovedPullRunsReturns(false);
   });
 });
+
+describe("utils: getFilesChangedByPrCached", () => {
+  beforeEach(() => clearFilesChangedCache());
+  afterEach(() => clearFilesChangedCache());
+
+  test("dedupes by head sha; re-fetches on sha change or after clear", async () => {
+    const octokit = {
+      paginate: jest
+        .fn()
+        .mockResolvedValue([{ filename: "a.py" }, { filename: "b.py" }]),
+    } as any;
+    const call = (sha: string) =>
+      getFilesChangedByPrCached(
+        octokit,
+        "delivery1",
+        "pytorch",
+        "pytorch",
+        1,
+        sha
+      );
+
+    expect(await call("sha1")).toEqual(["a.py", "b.py"]);
+    expect(await call("sha1")).toEqual(["a.py", "b.py"]);
+    expect(octokit.paginate).toHaveBeenCalledTimes(1);
+
+    await call("sha2");
+    expect(octokit.paginate).toHaveBeenCalledTimes(2);
+
+    clearFilesChangedCache();
+    await call("sha1");
+    expect(octokit.paginate).toHaveBeenCalledTimes(3);
+  });
+
+  test("dedupes within one delivery; re-fetches across deliveries", async () => {
+    const octokit = {
+      paginate: jest.fn().mockResolvedValue([{ filename: "a.py" }]),
+    } as any;
+    const call = (deliveryId: string) =>
+      getFilesChangedByPrCached(
+        octokit,
+        deliveryId,
+        "pytorch",
+        "pytorch",
+        1,
+        "sha1"
+      );
+
+    await call("delivery1");
+    await call("delivery1");
+    expect(octokit.paginate).toHaveBeenCalledTimes(1);
+
+    await call("delivery2");
+    expect(octokit.paginate).toHaveBeenCalledTimes(2);
+  });
+
+  test("a later delivery sees its own file list after a base retarget", async () => {
+    const octokit = {
+      paginate: jest
+        .fn()
+        .mockResolvedValueOnce([{ filename: "torch/csrc/foo.cpp" }])
+        .mockResolvedValueOnce([{ filename: "docs/readme.md" }]),
+    } as any;
+    // Retargeting a PR's base fires pull_request.edited without moving
+    // head.sha, so every key component except the delivery id is identical.
+    const call = (deliveryId: string) =>
+      getFilesChangedByPrCached(
+        octokit,
+        deliveryId,
+        "pytorch",
+        "pytorch",
+        1,
+        "sha1"
+      );
+
+    expect(await call("delivery1")).toEqual(["torch/csrc/foo.cpp"]);
+    expect(await call("delivery2")).toEqual(["docs/readme.md"]);
+    expect(octokit.paginate).toHaveBeenCalledTimes(2);
+  });
+});