From 09881ca34e163934e04cca75f8dab885e81c62d9 Mon Sep 17 00:00:00 2001 From: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Thu, 22 Oct 2026 10:06:07 -0400 Subject: [PATCH] test(sweep): pin the no-catch-up recovery of delayed reviewers after missed schedule runs After the schedule misses runs for longer than sweep.lookback, a pull request whose review clock expired before now - delay - lookback is neither listed, read nor requested by the delayed-reviewers step, and a longer sweep.lookback is what recovers it. Pinned in a unit suite (__tests__/cronJobTest/sweepDelayOutage.test.ts) and against dist/index.js (__tests__/bundle/blunderbussDelayOutage.test.ts); the blunderbuss.delay paragraph of docs/configuration.md now records the expectation. Refs #554 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> --- .../bundle/blunderbussDelayOutage.test.ts | 123 +++++++++++++++ .../cronJobTest/sweepDelayOutage.test.ts | 149 ++++++++++++++++++ docs/configuration.md | 6 +- 3 files changed, 277 insertions(+), 1 deletion(-) create mode 100644 __tests__/bundle/blunderbussDelayOutage.test.ts create mode 100644 __tests__/cronJobTest/sweepDelayOutage.test.ts diff --git a/__tests__/bundle/blunderbussDelayOutage.test.ts b/__tests__/bundle/blunderbussDelayOutage.test.ts new file mode 100644 index 00000000..4e540903 --- /dev/null +++ b/__tests__/bundle/blunderbussDelayOutage.test.ts @@ -0,0 +1,123 @@ +import type { FakeGithub } from './fakeGithub' +import { Buffer } from 'node:buffer' +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest' + +import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' +import pullReqListPulls from '../fixtures/pullReq/pullReqListPulls.json' +import { blobSha } from '../utils/ownersData' +import { start } from './fakeGithub' +import { repo, token } from './helpers' +import { runBundle } from './runBundle' + +vi.setConfig({ testTimeout: 30_000 }) + +// The recovery expectation of the delayed-reviewers sweep after the schedule +// missed runs for longer than `sweep.lookback` (src/cronJobs/sweep.ts, `sweep` +// → `recentlyUpdatedPulls` with `due.from`): the list stops at +// `now − delay − lookback`, so a pull request nobody touched since its review +// clock expired before that instant is neither listed, read nor requested. +// There is no unbounded catch-up; a longer `sweep.lookback` (capped at 24h) is +// what widens the window. +describe('dist/index.js delayed blunderbuss after missed schedule runs', () => { + let gh: FakeGithub + const owners = { OWNERS: 'reviewers:\n- bob\n' } + + beforeAll(async () => { + gh = await start() + }) + afterEach(() => gh.reset()) + afterAll(() => gh.close()) + + function ago(minutes: number) { + return new Date(Date.now() - minutes * 60_000).toISOString() + } + + function pull(number: number, minutesAgo: number) { + return { + ...structuredClone(pullReqListPulls[0]), + number, + created_at: ago(minutesAgo), + updated_at: ago(minutesAgo), + labels: [], + requested_reviewers: [], + requested_teams: [], + assignees: [], + draft: false, + user: { login: 'Codertocat' }, + base: { ref: 'master', sha: 'basesha' }, + head: { sha: `sha${number}`, repo: { full_name: 'Codertocat/Hello-World' } }, + } + } + + // #1 opened 90 minutes ago sits inside the default due window [now − 2h, now − 1h]; + // #2 opened 2.5 hours ago fell out of it while no sweep ran, and nobody touched it since + const pulls = [pull(1, 90), pull(2, 150)] + + function serve(config: string) { + const file = structuredClone(labelFileContents) + file.content = Buffer.from(config).toString('base64') + gh.route('GET', '/repos/Codertocat/.project/contents/prow.yaml', { status: 200, body: file }) + gh.route('GET', new RegExp(`^${repo}/pulls\\?`), { status: 200, body: pulls }) + for (const pr of pulls) { + gh.route('GET', `${repo}/pulls/${pr.number}`, { status: 200, body: pr }) + gh.route('GET', `${repo}/pulls/${pr.number}/files`, { status: 200, body: [{ filename: 'x.go', status: 'modified' }] }) + gh.route('GET', `${repo}/issues/${pr.number}/timeline`, { status: 200, body: [] }) + gh.route('GET', `${repo}/pulls/${pr.number}/reviews`, { status: 200, body: [] }) + gh.route('POST', `${repo}/pulls/${pr.number}/requested_reviewers`, { status: 201, body: {} }) + // a candidate inside the plain window runs every sweep step, not only the delayed reviewers + gh.route('GET', `${repo}/issues/${pr.number}`, { status: 200, body: { labels: [] } }) + gh.route('GET', `${repo}/issues/${pr.number}/comments`, { status: 200, body: [] }) + gh.route('POST', `${repo}/issues/${pr.number}/comments`, { status: 201, body: {} }) + } + gh.route('GET', `${repo}/branches/master`, { status: 200, body: { name: 'master', commit: { sha: 'basesha' } } }) + const tree = Object.keys(owners).map(path => ({ path, type: 'blob', sha: blobSha(path) })) + gh.route('GET', `${repo}/git/trees/master`, { status: 200, body: { sha: 'master', truncated: false, tree } }) + gh.route('GET', `${repo}/git/trees/basesha`, { status: 200, body: { sha: 'basesha', truncated: false, tree } }) + for (const [path, contents] of Object.entries(owners)) { + gh.route('GET', `${repo}/git/blobs/${blobSha(path)}`, { status: 200, body: { encoding: 'base64', content: Buffer.from(contents).toString('base64') } }) + } + gh.route('GET', `${repo}/labels`, { status: 200, body: [{ name: 'approved' }, { name: 'lgtm' }] }) + } + + function runSweep() { + return runBundle({ eventName: 'schedule', payload: {}, inputs: { ...token, jobs: 'sweep' }, apiUrl: gh.url }) + } + + function requested() { + return gh.requestsMatching('POST', /\/requested_reviewers$/).map(r => r.path).sort() + } + + function timelineReads() { + return gh.requestsMatching('GET', /\/timeline\?per_page=100$/).map(r => r.path).sort() + } + + it('does not catch up a pull request whose review clock expired before now − delay − lookback: not listed, read or requested', async () => { + serve('blunderbuss:\n delay: 1h\n') + + const result = await runSweep() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('sweep: 0 candidates updated since') + expect(result.stdout).toContain('sweep: 1 more checked for delayed reviewers') + expect(requested()).toEqual([`${repo}/pulls/1/requested_reviewers`]) + expect(timelineReads()).toEqual([`${repo}/issues/1/timeline?per_page=100`]) + // one list page, nothing else about #2: the sweep never asks after it + expect(gh.requestsMatching('GET', /\/pulls\?/).map(r => r.path)).toEqual([`${repo}/pulls?state=open&sort=updated&direction=desc&per_page=100&page=1`]) + expect(gh.requests.filter(r => /\/(?:pulls|issues)\/2(?:\/|$|\?)/.test(r.path))).toEqual([]) + }) + + it('a longer sweep.lookback is what recovers it: the window reaches back to it and it gets reviewers', async () => { + serve('blunderbuss:\n delay: 1h\nsweep:\n lookback: 2h\n') + + const result = await runSweep() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + // with the plain window now [now − 2h, now], #1 is an ordinary candidate and #2 the delayed-only one + expect(result.stdout).toContain('sweep: 1 candidate updated since') + expect(result.stdout).toContain('sweep: 1 more checked for delayed reviewers') + expect(requested()).toEqual([`${repo}/pulls/1/requested_reviewers`, `${repo}/pulls/2/requested_reviewers`]) + expect(timelineReads()).toEqual([`${repo}/issues/1/timeline?per_page=100`, `${repo}/issues/2/timeline?per_page=100`]) + }) +}) diff --git a/__tests__/cronJobTest/sweepDelayOutage.test.ts b/__tests__/cronJobTest/sweepDelayOutage.test.ts new file mode 100644 index 00000000..d2622321 --- /dev/null +++ b/__tests__/cronJobTest/sweepDelayOutage.test.ts @@ -0,0 +1,149 @@ +import { Buffer } from 'node:buffer' +import * as core from '@actions/core' +import { http } from 'msw' +import { setupServer } from 'msw/node' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' + +import { sweep } from '../../src/cronJobs/sweep' +import { resetOwnersCaches } from '../../src/utils/owners' +import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' +import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' +import * as utils from '../testUtils' +import { blobSha, branchHandler } from '../utils/ownersFixtures' + +const server = setupServer() +beforeAll(() => server.listen(utils.failOnUnhandledRequest)) +afterEach(() => server.resetHandlers()) +afterAll(() => server.close()) + +const repo = `${utils.api}/repos/Codertocat/Hello-World` +const now = new Date('2026-09-16T12:00:00Z') + +function ago(minutes: number): string { + return new Date(now.getTime() - minutes * 60_000).toISOString() +} + +function json(body: unknown, status = 200) { + return new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }) +} + +function prowYaml(text: string) { + const file = structuredClone(labelFileContents) + file.content = Buffer.from(text).toString('base64') + return http.get(utils.contentsUrl('.github/prow.yaml'), utils.mockResponse(200, file)) +} + +function pull(number: number, minutesAgo: number) { + return { + number, + state: 'open', + locked: false, + draft: false, + merged: false, + mergeable: true, + mergeable_state: 'clean', + labels: [], + created_at: ago(minutesAgo), + updated_at: ago(minutesAgo), + requested_reviewers: [], + requested_teams: [], + assignees: [], + user: { login: 'some-author' }, + head: { sha: `sha${number}` }, + base: { ref: 'master', sha: 'basesha' }, + } +} + +// The recovery expectation of the delayed-reviewers sweep (src/cronJobs/sweep.ts, +// `sweep` → `recentlyUpdatedPulls` with `due.from`) after the schedule missed runs +// for longer than `sweep.lookback`: the list stops at `now − delay − lookback`, so +// a pull request nobody touched since its review clock expired before that instant +// is neither listed, read nor requested, however due it is. There is no unbounded +// catch-up; a longer `sweep.lookback` (capped at 24h) is what widens the window. +describe('sweep with blunderbuss.delay after missed schedule runs', () => { + const owners = { OWNERS: 'approvers:\n- alice\nreviewers:\n- carol\n' } + let context: utils.MockContext + let calls: string[] + + function recordCall({ request }: { request: Request }) { + const url = new URL(request.url) + calls.push(`${request.method} ${url.pathname}${url.search}`) + } + + beforeEach(() => { + utils.setupJobsEnv('sweep') + vi.useFakeTimers({ now, toFake: ['Date'] }) + resetOwnersCaches() + context = new utils.MockContext(pullReqOpenedEvent) + context.eventName = 'schedule' + calls = [] + server.events.on('request:start', recordCall) + }) + + afterEach(() => { + server.events.removeListener('request:start', recordCall) + vi.useRealTimers() + }) + + // #1 opened 90 minutes ago sits inside the default due window [now − 2h, now − 1h]; + // #2 opened 2.5 hours ago fell out of it while no sweep ran, and nobody touched it since + const pulls = [pull(1, 90), pull(2, 150)] + + function serve(config: string) { + const tree = Object.keys(owners).map(path => ({ path, type: 'blob', sha: blobSha(path) })) + const requested: number[] = [] + const timelineReads: number[] = [] + server.use( + prowYaml(config), + ...utils.noOrgOrRepoConfigExcept(), + utils.defaultBranchTree(Object.keys(owners)), + branchHandler('basesha'), + http.get(`${repo}/git/trees/basesha`, utils.mockResponse(200, { sha: 'basesha', truncated: false, tree })), + ...Object.entries(owners).map(([path, contents]) => http.get(`${repo}/git/blobs/${blobSha(path)}`, utils.mockResponse(200, { encoding: 'base64', content: Buffer.from(contents).toString('base64') }))), + http.get(`${repo}/pulls`, utils.mockResponse(200, pulls)), + http.get(`${repo}/pulls/:number`, ({ params }) => json(pulls.find(pr => pr.number === Number(params.number)))), + http.get(`${repo}/pulls/:number/files`, utils.mockResponse(200, [{ filename: 'x.go', status: 'modified' }])), + http.get(`${repo}/issues/:number/timeline`, ({ params }) => { + timelineReads.push(Number(params.number)) + return json([]) + }), + http.get(`${repo}/pulls/:number/reviews`, utils.mockResponse(200, [])), + // a candidate inside the plain window runs every sweep step, not only the delayed reviewers + http.get(`${repo}/issues/:number`, utils.mockResponse(200, { labels: [] })), + http.get(`${repo}/issues/:number/comments`, utils.mockResponse(200, [])), + http.post(`${repo}/issues/:number/comments`, utils.mockResponse(201, {})), + utils.repoHasLabels(['approved', 'lgtm']), + http.post(`${repo}/pulls/:number/requested_reviewers`, ({ params }) => { + requested.push(Number(params.number)) + return json({}, 201) + }), + ) + return { requested, timelineReads } + } + + it('does not catch up a pull request whose review clock expired before now − delay − lookback: not listed, read or requested', async () => { + const { requested, timelineReads } = serve('blunderbuss:\n delay: 1h\n') + const info = vi.spyOn(core, 'info') + + await expect(sweep(context)).resolves.toEqual({ candidates: [], merged: [], enqueued: [], failures: [] }) + + expect(requested).toEqual([1]) + expect(timelineReads).toEqual([1]) + expect(info).toHaveBeenCalledWith(`sweep: 1 more checked for delayed reviewers, updated since ${ago(120)}`) + // one list page, nothing else about #2: the sweep never asks after it + expect(calls.filter(call => call.includes('/pulls?'))).toEqual([`GET /repos/Codertocat/Hello-World/pulls?state=open&sort=updated&direction=desc&per_page=100&page=1`]) + expect(calls.filter(call => /\/(?:pulls|issues)\/2(?:\/|$|\?)/.test(call))).toEqual([]) + }) + + it('a longer sweep.lookback is what recovers it: the window reaches back to it and it gets reviewers', async () => { + const { requested, timelineReads } = serve('blunderbuss:\n delay: 1h\nsweep:\n lookback: 2h\n') + const info = vi.spyOn(core, 'info') + + // with the plain window now [now − 2h, now], #1 is an ordinary candidate and #2 the delayed-only one + await expect(sweep(context)).resolves.toEqual({ candidates: [1], merged: [], enqueued: [], failures: [] }) + + expect([...requested].sort()).toEqual([1, 2]) + expect([...timelineReads].sort()).toEqual([1, 2]) + expect(info).toHaveBeenCalledWith(`sweep: 1 more checked for delayed reviewers, updated since ${ago(180)}`) + }) +}) diff --git a/docs/configuration.md b/docs/configuration.md index 2c3f0111..91852682 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -338,7 +338,11 @@ that nobody has been asked to review yet. The clock starts at `opened`, or at th sweep's list reaches back the extra `delay`, so a pull request nobody touched since it opened is still found; those older pull requests get this step only. Install the sweep job even under `pull_request_target`; its other steps are idempotent. A request lands up to one sweep interval -after it is due, plus GitHub's scheduling delay. `/auto-cc` is never delayed. +after it is due, plus GitHub's scheduling delay. The sweep does not catch up any further back: +when the schedule misses runs for longer than `lookback`, a pull request whose review clock +expired before `now − delay − lookback` without a sweep seeing it is never requested by this +step. Size `lookback` (capped at 24h) to the longest outage to ride out, or request reviewers by +hand or with `/auto-cc`. `/auto-cc` is never delayed. ```yaml blunderbuss: