perf(github): poke linked PRs only when it changes something

src/github/commands/update.ts

@@ -1,31 +1,18 @@
 import { evaluateIssue, evaluatePR } from "../engine/evaluate.js";
 import type { Command } from "../engine/types.js";
 import { UsageError } from "../engine/usage-error.js";
+import { rateLimiter } from "../rate-limiter.js";
 
 // A re-evaluation is the most expensive thing a comment can trigger, and
 // anyone may trigger it; the cap keeps one item from draining the
 // installation's rate limit. Members are exempt.
 const WINDOW_MS = 30 * 60 * 1000;
 const MAX_RUNS_PER_WINDOW = 10;
-const recentRuns = new Map<string, number[]>();
-
-/** Records a run for `key`; false when the window is already full. */
-function takeRunSlot(key: string): boolean {
-  const now = Date.now();
-  for (const [k, runs] of recentRuns) {
-    const live = runs.filter((t) => now - t < WINDOW_MS);
-    if (live.length === 0) recentRuns.delete(k);
-    else recentRuns.set(k, live);
-  }
-  const runs = recentRuns.get(key) ?? [];
-  if (runs.length >= MAX_RUNS_PER_WINDOW) return false;
-  recentRuns.set(key, [...runs, now]);
-  return true;
-}
+const runs = rateLimiter(MAX_RUNS_PER_WINDOW, WINDOW_MS);
 
 /** Test hook: forget every recorded run. */
 export function resetUpdateRateLimit(): void {
-  recentRuns.clear();
+  runs.reset();
 }
 
 export const update: Command = {
@@ -35,7 +22,7 @@ export const update: Command = {
 
   async handle(context) {
     const ref = { owner: context.repo.owner, repo: context.repo.name, number: context.number };
-    if (!takeRunSlot(`${context.repo.fullName}#${context.number}`)) {
+    if (!runs.take(`${context.repo.fullName}#${context.number}`)) {
       if (!(await context.senderIsMember())) {
         throw new UsageError(
           `at most ${MAX_RUNS_PER_WINDOW} updates per ${WINDOW_MS / 60_000} minutes`,

src/github/engine/dispatch.ts

@@ -1,7 +1,8 @@
 import { addLogContext, logContext, withCorrelationId } from "../../correlation.js";
 import { log } from "../../log.js";
 import { type ItemRef, slugOf } from "../../util/item-ref.js";
-import { withItemLock } from "../item-lock.js";
+import { hasQueued, withItemLock } from "../item-lock.js";
+import { rateLimiter } from "../rate-limiter.js";
 import { evaluatePR } from "./evaluate.js";
 import { EventType } from "./event.js";
 import type { CommandContext } from "./model/command-context.js";
@@ -256,13 +257,28 @@ async function applyEffects(context: RuleContext, effects: Effect[]): Promise<vo
   });
 }
 
+// A poke costs a whole evaluation of someone else's PR, and one edit can send
+// ten, so each PR takes a limited number per window.
+const pokes = rateLimiter(10, 30 * 60 * 1000);
+
+/** Test hook: forget every recorded poke. */
+export function resetPokeLimit(): void {
+  pokes.reset();
+}
+
 /**
  * Evaluate another PR without waiting for it: a dispatch that waits on a
  * second item's lock while holding its own deadlocks once that item pokes
- * back. A log context of its own keeps the target's identity off this
- * dispatch's remaining log lines.
+ * back. A dispatch already waiting for the target reads the change anyway,
+ * so a poke behind it is skipped. A log context of its own keeps the
+ * target's identity off this dispatch's remaining log lines.
  */
 function poke(context: RuleContext, key: string, target: ItemRef): void {
+  if (hasQueued(key)) return;
+  if (!pokes.take(key)) {
+    log.info("evaluate: poke limit reached", { target: key });
+    return;
+  }
   const correlationId = String(logContext()?.correlationId ?? "evaluate");
   withCorrelationId(correlationId, () =>
     withItemLock(key, () => evaluatePR(context.env, context.registry, context.github, target)),

src/github/item-lock.ts

@@ -49,6 +49,11 @@ export async function withItemLock<T>(key: string, run: () => Promise<T>): Promi
   }
 }
 
+/** Whether a dispatch for `key` is already waiting behind the running one. */
+export function hasQueued(key: string): boolean {
+  return (depth.get(key) ?? 0) > 1;
+}
+
 /** Test hook: drop every queue so cases don't wait on each other's. */
 export function resetItemLocks(): void {
   tails.clear();

src/github/rate-limiter.ts

@@ -0,0 +1,22 @@
+/** Counts recent runs per key, and refuses another once `max` fall within the window. */
+export function rateLimiter(max: number, windowMs: number) {
+  const recent = new Map<string, number[]>();
+  return {
+    /** Records a run for `key`; false when the window is already full. */
+    take(key: string): boolean {
+      const now = Date.now();
+      for (const [k, runs] of recent) {
+        const live = runs.filter((t) => now - t < windowMs);
+        if (live.length === 0) recent.delete(k);
+        else recent.set(k, live);
+      }
+      const runs = recent.get(key) ?? [];
+      if (runs.length >= max) return false;
+      recent.set(key, [...runs, now]);
+      return true;
+    },
+    reset(): void {
+      recent.clear();
+    },
+  };
+}

src/github/rules/poke-linked.ts

@@ -8,29 +8,26 @@ type HandledEvent =
   | EventType.PULL_REQUEST_OPENED
   | EventType.PULL_REQUEST_EDITED
   | EventType.PULL_REQUEST_CLOSED
-  | EventType.PULL_REQUEST_REOPENED
-  | EventType.PULL_REQUEST_REVIEW_SUBMITTED
-  | EventType.PULL_REQUEST_REVIEW_DISMISSED;
+  | EventType.PULL_REQUEST_REOPENED;
 
 /**
- * Pokes the PRs this one belongs together with on every change, so their own
- * rules see it. An edit also pokes the ones it dropped: they still list this
- * PR until they re-evaluate. A review only matters to siblings, which wait
- * for this PR's approval.
+ * Pokes the PRs this one belongs together with when that changes something
+ * for them, so their own rules see it. An edit of the description also pokes
+ * the ones it dropped: they still list this PR until they re-evaluate. A
+ * close or reopen only matters to the PRs that follow this one.
  */
 export function pokeLinked(links: PrLinks): Rule {
-  async function evaluate(ctx: RuleContext<HandledEvent>): Promise<RuleOutput> {
-    const bodies =
-      ctx.event.type === EventType.PULL_REQUEST_EDITED && ctx.event.previousBody !== undefined
-        ? [undefined, ctx.event.previousBody]
-        : [undefined];
-    const reviewed =
-      ctx.event.type === EventType.PULL_REQUEST_REVIEW_SUBMITTED ||
-      ctx.event.type === EventType.PULL_REQUEST_REVIEW_DISMISSED;
+  async function evaluate(ctx: RuleContext<HandledEvent>): Promise<RuleOutput | undefined> {
+    const edited = ctx.event.type === EventType.PULL_REQUEST_EDITED;
+    if (edited && ctx.event.previousBody === undefined) return undefined;
+    const bodies = edited ? [undefined, ctx.event.previousBody] : [undefined];
+    const stateChanged =
+      ctx.event.type === EventType.PULL_REQUEST_CLOSED ||
+      ctx.event.type === EventType.PULL_REQUEST_REOPENED;
     const linked = (await Promise.all(bodies.map((body) => linkedPRs(ctx, links, body)))).flat();
     return {
       effects: linked
-        .filter((l) => l.confirmed && (!reviewed || l.relation === "sibling"))
+        .filter((l) => l.confirmed && (!stateChanged || l.relation === "child"))
         .map((l) => ({ type: "evaluate", target: l.ref })),
     };
   }
@@ -44,8 +41,6 @@ export function pokeLinked(links: PrLinks): Rule {
         EventType.PULL_REQUEST_EDITED,
         EventType.PULL_REQUEST_CLOSED,
         EventType.PULL_REQUEST_REOPENED,
-        EventType.PULL_REQUEST_REVIEW_SUBMITTED,
-        EventType.PULL_REQUEST_REVIEW_DISMISSED,
       ],
       evaluate,
     ),

src/github/rules/sibling-approval.ts

@@ -31,7 +31,8 @@ async function approved(pr: PullRequest): Promise<boolean> {
 /**
  * Holds a PR while a sibling it has to be merged together with isn't
  * approved, so neither half lands alone. A hint, not a gate: removing the
- * link releases it.
+ * link releases it. It learns of a sibling's approval only through a poke,
+ * so it needs `pokeLinked` to poke siblings on reviews.
  */
 export function siblingApproval(links: PrLinks): Rule {
   return check({

tests/github/engine/apply-effects.test.ts

@@ -1,5 +1,5 @@
-import { describe, expect, it, vi } from "vitest";
-import { dispatch } from "../../../src/github/engine/dispatch.js";
+import { beforeEach, describe, expect, it, vi } from "vitest";
+import { dispatch, resetPokeLimit } from "../../../src/github/engine/dispatch.js";
 import { EventType } from "../../../src/github/engine/event.js";
 import { renderStatus } from "../../../src/github/engine/status/render.js";
 import type {
@@ -8,6 +8,7 @@ import type {
   RuleOutput,
   StatusSectionClaim,
 } from "../../../src/github/engine/types.js";
+import { resetItemLocks, withItemLock } from "../../../src/github/item-lock.js";
 import {
   BOT_LOGIN,
   createFakeGitHub,
@@ -522,6 +523,12 @@ describe("aggregate ha-bot status check from status sections", () => {
 
 describe("evaluate effect", () => {
   const target = { owner: "home-assistant", repo: "home-assistant.io", number: 9 };
+  const targetKey = "home-assistant/home-assistant.io#9";
+
+  beforeEach(() => {
+    resetPokeLimit();
+    resetItemLocks();
+  });
 
   function setup(eventType: EventType) {
     const github = createFakeGitHub();
@@ -564,6 +571,30 @@ describe("evaluate effect", () => {
     expect(ctx.repo.fullName).toBe("home-assistant/home-assistant.io");
   });
 
+  it("skips a poke while a run for the target is already waiting", async () => {
+    const { targetRule, run } = setup(EventType.PULL_REQUEST_EDITED);
+    let release = () => {};
+    const running = withItemLock(targetKey, () => new Promise<void>((r) => (release = r)));
+    const waiting = withItemLock(targetKey, async () => {});
+    await run();
+    release();
+    await Promise.all([running, waiting]);
+
+    await new Promise((resolve) => setTimeout(resolve, 20));
+    expect(targetRule).not.toHaveBeenCalled();
+  });
+
+  it("caps the pokes one PR takes", async () => {
+    const { targetRule, run } = setup(EventType.PULL_REQUEST_EDITED);
+    for (let i = 0; i < 11; i++) {
+      await run();
+      await vi.waitFor(() => expect(targetRule).toHaveBeenCalledTimes(Math.min(i + 1, 10)));
+    }
+
+    await new Promise((resolve) => setTimeout(resolve, 20));
+    expect(targetRule).toHaveBeenCalledTimes(10);
+  });
+
   it("is dropped from an on-demand dispatch", async () => {
     const { github, targetRule, run } = setup(EventType.ON_DEMAND);
     await run();

tests/github/rules/poke-linked.test.ts

@@ -8,25 +8,26 @@ const rule = pokeLinked(testLinks);
 const BACK = `${LINE} home-assistant/core#1`;
 const poke = (repo: string, number: number) => ({ type: "evaluate", target: ref(repo, number) });
 
-// The PR is home-assistant/core#1.
+// The PR is home-assistant/core#1; frontend PRs are its siblings, io PRs follow it.
 describe("poke-linked", () => {
   it("pokes the PRs that list this one back, not the others", async () => {
     const { context } = linkedContext(
       `${LINE} home-assistant/frontend#5 home-assistant/home-assistant.io#9`,
-      {
-        "frontend#5": { body: BACK },
-        "home-assistant.io#9": { body: "" },
-      },
+      { "frontend#5": { body: BACK }, "home-assistant.io#9": { body: "" } },
+      { type: EventType.PULL_REQUEST_OPENED },
     );
 
     expect((await runRule(rule, context))?.effects).toEqual([poke("frontend", 5)]);
   });
 
-  it("pokes when this PR closes", async () => {
+  it.each([
+    ["closes", EventType.PULL_REQUEST_CLOSED, "closed" as const],
+    ["reopens", EventType.PULL_REQUEST_REOPENED, "open" as const],
+  ])("pokes only the PRs that follow this one when it %s", async (_, type, state) => {
     const { context } = linkedContext(
-      `${LINE} home-assistant/home-assistant.io#9`,
-      { "home-assistant.io#9": { body: BACK } },
-      { type: EventType.PULL_REQUEST_CLOSED, state: "closed" },
+      `${LINE} home-assistant/frontend#5 home-assistant/home-assistant.io#9`,
+      { "frontend#5": { body: BACK }, "home-assistant.io#9": { body: BACK } },
+      { type, state },
     );
 
     expect((await runRule(rule, context))?.effects).toEqual([poke("home-assistant.io", 9)]);
@@ -48,17 +49,20 @@ describe("poke-linked", () => {
     ]);
   });
 
-  it("pokes only siblings on a review, since only they wait for it", async () => {
-    const { context } = linkedContext(
-      `${LINE} home-assistant/frontend#5 home-assistant/home-assistant.io#9`,
-      { "frontend#5": { body: BACK }, "home-assistant.io#9": { body: BACK } },
-      { type: EventType.PULL_REQUEST_REVIEW_SUBMITTED },
-    );
+  it("does nothing when an edit leaves the description alone", async () => {
+    const { github, context } = linkedContext(`${LINE} home-assistant/frontend#5`, {
+      "frontend#5": { body: BACK },
+    });
 
-    expect((await runRule(rule, context))?.effects).toEqual([poke("frontend", 5)]);
+    expect(await runRule(rule, context)).toBeUndefined();
+    expect(github.pulls.get).not.toHaveBeenCalled();
   });
 
-  it("doesn't run on demand, where pokes would be dropped", () => {
-    expect(rule.events[EventType.ON_DEMAND]).toBeUndefined();
+  it.each([
+    EventType.PULL_REQUEST_REVIEW_SUBMITTED,
+    EventType.PULL_REQUEST_REVIEW_DISMISSED,
+    EventType.ON_DEMAND,
+  ])("doesn't run on %s", (type) => {
+    expect(rule.events[type]).toBeUndefined();
   });
 });