From 06452b7e0cc4c72e348e6a06843b570b39e915e9 Mon Sep 17 00:00:00 2001 From: Ariel Ebersberger Date: Wed, 11 Nov 2026 20:36:27 +0100 Subject: [PATCH] Poke linked PRs only when it changes something for them --- src/github/commands/update.ts | 21 +++--------- src/github/engine/dispatch.ts | 22 +++++++++++-- src/github/item-lock.ts | 5 +++ src/github/rate-limiter.ts | 22 +++++++++++++ src/github/rules/poke-linked.ts | 31 ++++++++---------- src/github/rules/sibling-approval.ts | 3 +- tests/github/engine/apply-effects.test.ts | 35 ++++++++++++++++++-- tests/github/rules/poke-linked.test.ts | 40 +++++++++++++---------- 8 files changed, 120 insertions(+), 59 deletions(-) create mode 100644 src/github/rate-limiter.ts diff --git a/src/github/commands/update.ts b/src/github/commands/update.ts index 45b4453..933f891 100644 --- a/src/github/commands/update.ts +++ b/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(); - -/** 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`, diff --git a/src/github/engine/dispatch.ts b/src/github/engine/dispatch.ts index df7d8a0..d3e92e9 100644 --- a/src/github/engine/dispatch.ts +++ b/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 withItemLock(key, () => evaluatePR(context.env, context.registry, context.github, target)), diff --git a/src/github/item-lock.ts b/src/github/item-lock.ts index e5817d7..1515968 100644 --- a/src/github/item-lock.ts +++ b/src/github/item-lock.ts @@ -49,6 +49,11 @@ export async function withItemLock(key: string, run: () => Promise): 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(); diff --git a/src/github/rate-limiter.ts b/src/github/rate-limiter.ts new file mode 100644 index 0000000..6161c4d --- /dev/null +++ b/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(); + 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(); + }, + }; +} diff --git a/src/github/rules/poke-linked.ts b/src/github/rules/poke-linked.ts index 423f3b4..53f6f6b 100644 --- a/src/github/rules/poke-linked.ts +++ b/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): Promise { - 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): Promise { + 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, ), diff --git a/src/github/rules/sibling-approval.ts b/src/github/rules/sibling-approval.ts index 55fca26..4daa94d 100644 --- a/src/github/rules/sibling-approval.ts +++ b/src/github/rules/sibling-approval.ts @@ -31,7 +31,8 @@ async function approved(pr: PullRequest): Promise { /** * 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({ diff --git a/tests/github/engine/apply-effects.test.ts b/tests/github/engine/apply-effects.test.ts index 1eb5412..2d645d4 100644 --- a/tests/github/engine/apply-effects.test.ts +++ b/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((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(); diff --git a/tests/github/rules/poke-linked.test.ts b/tests/github/rules/poke-linked.test.ts index 39227ab..9a6872e 100644 --- a/tests/github/rules/poke-linked.test.ts +++ b/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(); }); });