diff --git a/.changes/pr-pipeline/fix-verdict-reader-per-run-3b8d71e0.md b/.changes/pr-pipeline/fix-verdict-reader-per-run-3b8d71e0.md new file mode 100644 index 00000000000..4ce842000a6 --- /dev/null +++ b/.changes/pr-pipeline/fix-verdict-reader-per-run-3b8d71e0.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- In the merge queue, ci-status titles every verifier hold's error annotation `pr-verify hold` (merge-holds.mjs `VERIFIER_TITLE`), so a `verify:unreadable` refusal no longer reads as a body hold of the step it shares. The verdict reader's short incremental artifact listing is read again after a wait before it pages every artifact, and the cold-read headroom counts the near-expiry caps once for the incremental reads and once for the whole listings' top-ups (1,681 to 1,739 at 7,522 artifacts). diff --git a/.changes/source-control/fix-verify-listing-short-retry-5c1e9a42.md b/.changes/source-control/fix-verify-listing-short-retry-5c1e9a42.md new file mode 100644 index 00000000000..f47bf08805d --- /dev/null +++ b/.changes/source-control/fix-verify-listing-short-retry-5c1e9a42.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- **babysit-prs**: the pr-verify verdict reader reads its incremental artifact listing again after a wait when the pages near expiry leave the count short, instead of paging the whole repository at once. That short count is a page-boundary skip in the fresh pages, which a second read restores. It reads again only while the near-expiry caps left cover another such read. The whole listings' top-ups now get those caps again, so incremental reads that spent them do not starve a top-up. diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 9d9a7906f30..d76e33623f1 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -1403,6 +1403,11 @@ export function verdictHold(records, repo, branch = "main") { return pass ? null : "pr-verify has not passed this head"; } +// The annotation title of a verifier hold (verdictHold, linkHold, +// evidenceHold), so a reader of the job's annotations tells a pr-verify +// refusal from a body hold, member veto or hold path in the same step. +export const VERIFIER_TITLE = "pr-verify hold"; + // The refusals of babysit_verify_evidence.py a pass decides: verdictHold and // linkHold judge the pass here, and an approval lifts them as it lifts this. const PASS_REFUSALS = Object.freeze(["verify:missing", "verify:spec-changed", "verify:untrusted-spec"]); @@ -1673,6 +1678,9 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi if (widened && lifters.length) { console.log(`#${number}: ${lifters.join(", ")} ${how}, which lifts its trusted-actor list hold (${escape(widened)}).`); } + // The verifier's holds, recorded under their own annotation title + // (VERIFIER_TITLE): they share this step with the body and member holds. + const verifier = []; if (verdictsFile) { const unverified = verdictHold(lines(verdictsFile).map((line) => JSON.parse(line)), env.GITHUB_REPOSITORY); // An approval lifts the verifier hold whole, the link with it. @@ -1682,11 +1690,11 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi } else if (!unverified && approved.length === 0) { // pr-verify skips a head it already passed (resolve-pr.mjs `judged`), // so only a new head or an approval recovers it. - reasons.push(`${relinked}; pr-verify does not judge this head again, so push a new head for it to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`); + verifier.push(`${relinked}; pr-verify does not judge this head again, so push a new head for it to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`); } else if (approved.length) { console.log(`#${number}: no verifier hold: ${approved.join(", ")} approved this head, which lifts '${unverified}'.`); } else { - reasons.push( + verifier.push( `${unverified}; wait for pr-verify to pass it, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`, ); } @@ -1697,16 +1705,19 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi } else if (approved.length) { console.log(`#${number}: no pr-verify FAIL hold: ${approved.join(", ")} approved this head, which lifts '${escape(failed)}'.`); } else { - reasons.push(`${failed}; push a new head for pr-verify to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`); + verifier.push(`${failed}; push a new head for pr-verify to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`); } } for (const reason of reasons) { console.log(`::error::#${number}: ${escape(reason)}.`); } + for (const reason of verifier) { + console.log(`::error title=${VERIFIER_TITLE}::#${number}: ${escape(reason)}.`); + } if (!reasons.length) { console.log(`#${number}: no body hold, no member veto and no hold path.`); } - return reasons.length ? 1 : 0; + return reasons.length || verifier.length ? 1 : 0; } catch (error) { console.log(`::error::merge-holds: ${String(error?.message ?? error).replaceAll(/[\r\n%]/g, " ")}`); return 2; diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index b37188e7384..0c049528989 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -973,8 +973,18 @@ test("in the merge queue, a reader that refuses only on the pass leaves that to test("in the merge queue, a FAIL the reader counts holds a passed head", async () => { const { code, out } = await queued(report("verify:fail", { detail: "fails at the head" })); assert.equal(code, 1, out); - assert.match(out, /::error::#99: pr-verify's reader refuses this head \(verify:fail: fails at the head\); push a new head/); + assert.match(out, /::error title=pr-verify hold::#99: pr-verify's reader refuses this head \(verify:fail: fails at the head\); push a new head/); assert.doesNotMatch(out, /no pr-verify FAIL hold/); + // A verifier hold is not a body hold: no untitled error line. + assert.doesNotMatch(out, /^::error::/m); + assert.match(out, /^#99: no body hold, no member veto and no hold path\.$/m); +}); + +test("in the merge queue, the reader's verify:unreadable is a pr-verify hold beside a body hold", async () => { + const { code, out } = await queued(report("verify:unreadable", { detail: "1 whole listing never held" })); + assert.equal(code, 1, out); + assert.match(out, /^::error title=pr-verify hold::#99: pr-verify's reader refuses this head \(verify:unreadable: 1 whole listing never held\)/m); + assert.doesNotMatch(out, /^::error::/m); }); test("in the merge queue, a trusted approval of the head lifts a FAIL hold", async () => { @@ -993,7 +1003,7 @@ test("in the merge queue, a missing or unreadable reader report holds", async () for (const evidence of [undefined, "", "not json", JSON.stringify(report(null, { head: OLD }))]) { const { code, out } = await queued(evidence); assert.equal(code, 1, `${evidence}: ${out}`); - assert.match(out, /::error::#99: its pr-verify FAILs could not be read/, String(evidence)); + assert.match(out, /::error title=pr-verify hold::#99: its pr-verify FAILs could not be read/, String(evidence)); } }); diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 1fd2da6c17a..5af151221e2 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -6137,7 +6137,10 @@ jobs: # evidenceHold, which holds on every refusal but the three a pass decides # (those stay with verdictHold and linkHold): every FAIL class, # verify:unreadable and verify:too-old. The same approval of the head - # lifts it. The reader reads with this job's token, which holds no + # lifts it. Every verifier hold's error annotation is titled + # `pr-verify hold` (merge-holds.mjs VERIFIER_TITLE), so a reader of this + # step's annotations tells it from a body hold, member veto or hold + # path. The reader reads with this job's token, which holds no # issues scope: on this public repository its issue reads answer as # public reads, and one that does not read refuses verify:unreadable, # which holds. Its gh calls go to the gh binary, outside this step's @@ -6455,15 +6458,16 @@ jobs: # and LISTING_READS (3) whole listings, each up to # 1 + len(LISTING_WAITS) (3) attempts that page from the first, # and a top-up after each (LISTING_TOP_UP_PAGES, 7), plus at most - # NEAR_EXPIRY_PAGE_CAP (24) pages near expiry read again when a - # count is over, and NEAR_EXPIRY_ID_READ_CAP (34) artifacts read - # by id that those pages omit. A whole + # NEAR_EXPIRY_PAGE_CAP (24) pages near expiry read again and + # NEAR_EXPIRY_ID_READ_CAP (34) artifacts read by id that those + # pages omit, once for the incremental reads and once for the + # whole listings' top-ups. A whole # listing pages ceil(total_count / 100), plus one for uploads while # it pages, so a cold read costs at most - # 3 x FAILs + 3 x 13 + 1 + 3 x (3 x pages + 7) + 24 + 34, plus the + # 3 x FAILs + 3 x 13 + 1 + 3 x (3 x pages + 7) + 2 x (24 + 34), plus the # pages of this step's own listing, which every entry the queue - # builds at once also reads, cold_cost below: 1,681 at 7,522 - # artifacts and 264 FAILs, 2,079 at 10,450 and 300. + # builds at once also reads, cold_cost below: 1,739 at 7,522 + # artifacts and 264 FAILs, 2,137 at 10,450 and 300. # test_babysit_verify_evidence runs these lines against those caps # at several sizes. It covers this path only: an # entry with a carried run cache but no carried listing pays the @@ -6505,7 +6509,7 @@ jobs: exit 1 fi listing_pages=$(((total + 99) / 100 + 1)) - cold_cost=$((listing_pages + 3 * fails + 3 * 13 + 1 + 3 * (3 * listing_pages + 7) + 24 + 34)) + cold_cost=$((listing_pages + 3 * fails + 3 * 13 + 1 + 3 * (3 * listing_pages + 7) + 2 * (24 + 34))) rate=$(read_api "the GITHUB_TOKEN rate limit" rate_limit) || rate= if ! remaining=$(jq -er '.resources.core.remaining | numbers | floor' <<<"$rate" 2>/dev/null) || ! limit=$(jq -er '.resources.core.limit | numbers | floor' <<<"$rate" 2>/dev/null) || diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index f70630b9753..13d1a685567 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -2671,7 +2671,9 @@ kept only when it holds exactly that page's `total_count`. A count above it firs around kept entries near expiry, up to a page cap, since GitHub can delete an artifact up to an hour before its `expires_at` (`EARLY_DELETION_LEAD`, `NEAR_EXPIRY_PAGE_CAP`). Those pages can skip an entry at a boundary, so an entry they omit is dropped only when its own read answers 404, up to an -id-read cap (`NEAR_EXPIRY_ID_READ_CAP`). Any count still other than it, a missing, older or +id-read cap (`NEAR_EXPIRY_ID_READ_CAP`); the whole listings' top-ups get both caps again. A count +those pages leave short is a skip in the fresh pages and is read again after a wait while the caps +left cover another such read. Any count still other than it, a missing, older or malformed base, or any failed read lists every page, so a carried listing that leaves out a FAIL lists every page too (the rule and its basis: `babysit_verify_evidence.py`, "Carried reads"). A whole listing is held to the same count: deduplicated by id, added to the whole listings read diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py index ff39ac71a11..841b29e7cf2 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py @@ -157,11 +157,15 @@ around the kept entries near expiry are read again and decide every id they span, an id they omit by its own read (`Reader._near_expiry`), at a few pages a group whatever the repository's total and at most `NEAR_EXPIRY_PAGE_CAP` a -listing. They are read on an exact count too: an entry deleted early can stand +listing's incremental reads, and as many again for its whole listings' +top-ups. They are read on an exact count too: an entry deleted early can stand in for a FAIL the pages skipped. An over-count those pages do not settle goes to the one whole listing, an exact count they do not settle fails the listing (as transient when only `NEAR_EXPIRY_PAGE_CAP` or -`NEAR_EXPIRY_ID_READ_CAP` left it so, so a later Reader may list), a count they leave short goes to a whole listing with no wait, and a +`NEAR_EXPIRY_ID_READ_CAP` left it so, so a later Reader may list), a count +they leave short is a page-boundary skip in the fresh pages and is read +again after a wait while the caps left cover another such read (else a whole +listing), and a page or id read still failing on a 5xx after `LISTING_WAITS` fails it as transient. Such a listing could hide a FAIL by leaving it out, so it is kept only when it holds exactly @@ -411,22 +415,23 @@ def verdict_name(name: str) -> re.Match[str] | None: # line whose `listed` exceeds `total_count`. Twice the largest lead seen # (judgment on that data). EARLY_DELETION_LEAD = timedelta(hours=2) -# The most pages `Reader._near_expiry` reads for one listing, its incremental -# read and top-up together; past it an over-count stands and goes to the one +# The most pages `Reader._near_expiry` reads for one listing's incremental +# reads, and again for its whole listings' top-ups (`_complete_listing` resets +# both caps); past it an over-count stands and goes to the one # whole listing, and an exact count fails the listing. On the 2026-11-01 # carry behind #11903's refusal it read 12 pages; twice that (judgment on # that data), at `LISTING_PAGE_SECONDS` a page, stays well inside # verify-cap.mjs's 120 s limit and a fifth of that day's 133-page whole # listing. NEAR_EXPIRY_PAGE_CAP = 24 -# The most artifacts `Reader._near_expiry` reads by id for one listing, its -# incremental read and top-up together: each id its pages did not show is +# The most artifacts `Reader._near_expiry` reads by id for one listing's +# incremental reads, and again for its top-ups: each id its pages did not show is # dropped only on a 404 or an expired record. It drops at most the listing's # surplus over the live count, which was 31 to 34 in the three over-counts # behind #11903's refusal; the largest (judgment on that data). Past it an # over-count stands and an exact count fails the listing. Both caps reset -# with each listing the Reader reads, as after `refresh()`; -# pr-require-checks.yml's cold_cost counts them once. +# with each listing the Reader reads, as after `refresh()`, and again before +# its whole listings; pr-require-checks.yml's cold_cost counts them twice. NEAR_EXPIRY_ID_READ_CAP = 34 # The `cause` a `LISTING_MISS_PREFIX` line names when the caps above alone # leave an exact count unsettled: refused, but not noted for the job. @@ -463,10 +468,10 @@ def no_incremental_waits() -> bool: class _CountMissed: """`Reader._incremental`'s answer when its pages read whole but do not hold the live count: `COUNT_MISSED` (fewer), the one case read again on - the same base, or `COUNT_OVER` (more, or any count once the pages near - expiry were read again, except a top-up's exact count left short), which - goes to the whole listings, since no wait removes an entry GitHub no - longer counts.""" + the same base, or `COUNT_OVER` (more, a top-up's over-count left short, + or an incremental count left short once the near-expiry caps cannot + cover another read), which goes to the whole listings, since no wait + removes an entry GitHub no longer counts.""" # Fewer than the live count: the pages skipped an artifact, which another @@ -1483,6 +1488,9 @@ def _complete_listing(self) -> list[dict[str, Any]]: it when the caller's limit cut the whole listings' timeout and too little of it is left for another wait (`LISTING_WAITS`).""" self._whole_paged = False + # The whole listings' top-ups get near-expiry caps of their own, so + # incremental reads that spent theirs leave the top-ups theirs. + self._near_expiry_reads = self._artifact_reads = 0 cap, bound = listing_cap() callers = cap < LISTING_TIMEOUT_SECONDS if callers and cap <= 0: @@ -1604,12 +1612,12 @@ def _incremental( gives None, since an entry deleted early may stand in for a FAIL. The result must hold exactly the first page's `total_count` artifacts, GitHub's live count: fewer gives `COUNT_MISSED`, which - `_current_listing` reads again on the same base after a wait, and - more, or any count once the pages near expiry were read (a top-up's - exact count left short aside, which another whole listing can - restore), gives `COUNT_OVER`, which goes to the whole listings (one, - after an over-count), since no wait removes an entry GitHub no - longer counts; either prints one + `_current_listing` reads again on the same base after a wait (after + the pages near expiry were read, only while the caps left cover + another such read), and more, or a top-up's over-count left short, + gives `COUNT_OVER`, which goes to the whole listings (one, after an + over-count), since no wait removes an entry GitHub no longer counts; + either prints one `LISTING_MISS_PREFIX` line on stderr naming `stage`, the counts and the pages read. No base, an entry or page of another shape, a missing `Date`, or any failed read gives None (`_page_transient` says whether @@ -1620,6 +1628,8 @@ def _incremental( and sets `_near_expiry_spent` when only the caps, or the deadline running out during those reads, left it so.""" self._page_transient = self._near_expiry_spent = False + spent = (self._near_expiry_reads, self._artifact_reads) + started = _listing_clock() if base is not None and not all(_listed_entry(a) for a in base[1]): return None base = base or self._incremental_base() @@ -1726,11 +1736,31 @@ def _incremental( file=sys.stderr, flush=True, ) - # Once the pages near expiry were read, no wait on this base settles - # the count: the base still holds what they dropped. A top-up's exact - # count left short goes to another whole listing, which can restore a - # skip outside those pages. - if over or len(listing) > total or (near and stage != "top-up"): + if len(listing) > total: + return COUNT_OVER + # Short once the pages near expiry were read: they dropped only ids + # GitHub answered gone, so the fresh pages skipped an artifact at a + # page boundary (#10486), which a read after a wait can restore, as + # for any short count. It is read again only while the caps left + # cover another read like this one, and a caller's limit still fits + # that read, the longest wait and one whole listing; else a whole + # listing. A top-up's exact count left short goes to another whole + # listing, which can restore a skip outside those pages. + if stage == "incremental" and (over or near): + pages = self._near_expiry_reads - spent[0] + ids = self._artifact_reads - spent[1] + cap = listing_cap()[0] + took = _listing_clock() - started + if ( + NEAR_EXPIRY_PAGE_CAP - self._near_expiry_reads < pages + or NEAR_EXPIRY_ID_READ_CAP - self._artifact_reads < ids + or ( + cap < LISTING_TIMEOUT_SECONDS + and cap < took + max(INCREMENTAL_WAITS) + listing_seconds(total) + ) + ): + return COUNT_OVER + elif over: return COUNT_OVER return COUNT_MISSED diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py index 2fcbe4d8f94..ebdbb5f9d7d 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_evidence.py @@ -4422,19 +4422,124 @@ def test_the_pages_near_expiry_stop_at_their_cap(self) -> None: # Out of expiry order, the region takes two pages (3 and 4). Capped # at one, the count stays over: one whole listing, whose top-up # (page 1) holds the count, and no INCREMENTAL_WAITS. That top-up - # keeps 500, near expiry, with the cap spent: an exact count it - # cannot settle may lack a FAIL, so the gate refuses. + # keeps 500, near expiry, and reads its page under a cap of its own, + # which the incremental read did not spend: it settles and allows. base = self.tail((500, 30), (400, 20)) self.write_carry(base) fake = self.fake(base=[a for a in base if a["id"] != 400]) with mock.patch.object(ve, "NEAR_EXPIRY_PAGE_CAP", 1): reader, evidence = self.judge(fake) - self.assertEqual(evidence.refusal, "verify:unreadable") - self.assertIn("could not be checked against", evidence.detail or "") + self.assertIsNone(evidence.refusal, evidence.detail) + self.assertEqual(reader.listing_mode, "full") + self.assertEqual(fake.full_listings(), 1) + # Page 1, one capped region page, the top-up's page 1 and its one + # region page. + self.assertEqual(fake.pages(), 4) + self.assertEqual(reader._near_expiry_reads, 1) + self.assertEqual(self.sleep.call_args_list, []) + + def skips_once(self, fake: FakeGitHub, artifact_id: int) -> None: + """The first single page read leaves `artifact_id` out, as a page + read while an artifact above it is deleted skips the one at its + boundary (#10486).""" + rest = fake.rest + skipped = False + + def first_page_skips(args: list[str], *rest_args: Any) -> Any: + nonlocal skipped + if skipped or not args[-1].endswith("&page=1"): + return rest(args, *rest_args) + skipped = True + held, live = fake.artifacts, fake.live_total + # The live count still holds it. + fake.live_total = len({a["id"] for a in held}) + fake.artifacts = [a for a in held if a["id"] != artifact_id] + try: + return rest(args, *rest_args) + finally: + fake.artifacts, fake.live_total = held, live + + fake.rest = first_page_skips # type: ignore[method-assign] + + def early_deletion_and_a_new_fail(self) -> FakeGitHub: + """A carry holding 500, which GitHub deleted 30 minutes before its + expiry, and a FAIL at the head uploaded since (1100) that the first + page read skips once: the first read holds the live count, 500's + own read answers 404, and it is one short.""" + base = self.tail((500, 30)) + self.write_carry(base) + fail = dated(named(HEAD, "fail"), 2, 1100, LISTED_AT) + fake = self.fake(fail, base=[a for a in base if a["id"] != 500]) + self.skips_once(fake, 1100) + return fake + + def test_a_skip_after_the_pages_near_expiry_is_read_again_after_a_wait( + self, + ) -> None: + # The ci-status and enable-auto-merge refusals of 2026-11-02: one to + # three short once the pages near expiry were read, then 134 to 293 + # whole-listing pages. Read again after one wait, the pages hold the + # FAIL and the count, with no whole listing. + fake = self.early_deletion_and_a_new_fail() + reader, evidence = self.judge(fake) + self.assertEqual(evidence.refusal, "verify:fail", evidence.detail) + self.assertEqual(reader.listing_mode, "incremental") + self.assertEqual(fake.full_listings(), 0) + self.assertEqual([c.args[0] for c in self.sleep.call_args_list], [15.0]) + ids = [a["id"] for a in json.loads(self.saved.read_text())["artifacts"]] + self.assertEqual(ids, [1100, 1000, 900, 800, 700, 600, 400, 300]) + + def test_an_over_count_left_short_by_a_skip_is_read_again_after_a_wait( + self, + ) -> None: + # ci-status job 125052787044's shape: the carry holds 500 and 400, + # both deleted early, so the first read over-counts; the pages near + # expiry drop both and leave it one short of the FAIL the first page + # skipped. Read again after one wait, it holds the count: no whole + # listing, so the job's over-count slot and failure note stay unused. + base = self.tail((500, 30), (400, 40)) + self.write_carry(base) + fail = dated(named(HEAD, "fail"), 2, 1100, LISTED_AT) + fake = self.fake(fail, base=[a for a in base if a["id"] not in (500, 400)]) + self.skips_once(fake, 1100) + reader, evidence = self.judge(fake) + self.assertEqual(evidence.refusal, "verify:fail", evidence.detail) + self.assertEqual(reader.listing_mode, "incremental") + self.assertEqual(fake.full_listings(), 0) + self.assertEqual([c.args[0] for c in self.sleep.call_args_list], [15.0]) + overcount = pathlib.Path(f"{self.saved}{ve.OVERCOUNT_FALLBACK_SUFFIX}") + self.assertFalse(overcount.exists()) + self.assertFalse(self.failed.exists()) + + def test_a_skip_a_callers_limit_cannot_wait_for_takes_a_whole_listing( + self, + ) -> None: + # Under a 50 s caller limit, 35 s are left: not enough for the 30 s + # wait and a 12 s whole listing (5 pages and the top-up's 7), so the + # whole listing comes at once. + fake = self.early_deletion_and_a_new_fail() + with ( + mock.patch.dict("os.environ", {ve.DEADLINE_ENV: "50"}), + mock.patch.object(ve, "_elapsed", return_value=0.0), + ): + reader, evidence = self.judge(fake) + self.assertEqual(evidence.refusal, "verify:fail", evidence.detail) + self.assertEqual(reader.listing_mode, "full") + self.assertEqual(fake.full_listings(), 1) + self.assertEqual(self.sleep.call_args_list, []) + + def test_a_skip_the_caps_left_cannot_read_again_takes_a_whole_listing( + self, + ) -> None: + # As above with one region page a listing: the first read spends it, + # so no read on this base could settle 500 again. The whole listing, + # with no wait, holds the FAIL. + fake = self.early_deletion_and_a_new_fail() + with mock.patch.object(ve, "NEAR_EXPIRY_PAGE_CAP", 1): + reader, evidence = self.judge(fake) + self.assertEqual(evidence.refusal, "verify:fail", evidence.detail) self.assertEqual(reader.listing_mode, "full") self.assertEqual(fake.full_listings(), 1) - # Page 1, one capped region page, then the top-up's page 1. - self.assertEqual(fake.pages(), 3) self.assertEqual(self.sleep.call_args_list, []) def test_two_readers_that_over_count_take_one_whole_listing_a_job( @@ -5355,16 +5460,18 @@ def test_a_stale_carry_whose_early_deletion_hides_a_missed_fail(self) -> None: # The carry lacks FAIL 750 and still holds 600, which GitHub deleted # 30 minutes before its expiry: five against a live count of five. # Page 3 (700) does not reach 750, but 600's own read answers 404, - # so the count is one short and the whole listing holds the FAIL. - # No wait on this base could restore it: no INCREMENTAL_WAITS. + # so the count is one short. A short count reads again after each + # wait, as for a skip in the fresh pages; this one is the carry's, so + # every read stays short and the whole listing holds the FAIL. self.write_carry([*self.base, self.entry(600, timedelta(minutes=30))]) fake = self.fake(self.earlier_fail(750)) reader, evidence = self.judge(fake) self.assertEqual(evidence.refusal, "verify:earlier-fail", evidence.detail) self.assertEqual(reader.listing_mode, "full") self.assertEqual(fake.full_listings(), 1) - self.assertEqual(len(self.by_id(fake)), 1) - self.assertEqual(self.sleep.call_args_list, []) + self.assertEqual(len(self.by_id(fake)), ve.INCREMENTAL_READS) + waits = [c.args[0] for c in self.sleep.call_args_list] + self.assertEqual(waits, list(ve.INCREMENTAL_WAITS)) # An exact count is not an over-count: the job's slot stays free. overcount = pathlib.Path(f"{self.saved}{ve.OVERCOUNT_FALLBACK_SUFFIX}") self.assertFalse(overcount.exists()) @@ -5383,11 +5490,10 @@ def test_a_top_up_left_short_reads_another_whole_listing(self) -> None: self.assertFalse(self.failed.exists()) def test_a_dropped_entry_does_not_spend_the_top_ups_page_cap(self) -> None: - # As above, with 650 live and near expiry too: the incremental read - # takes pages 3 and 4, and the whole listing's top-up, which keeps - # 650, takes page 3 again: three of a cap of six. Re-read after each - # wait on the same base, the incremental read would take all six and - # leave the top-up unsettled. + # As above, with 650 live and near expiry too: each incremental read + # takes pages 3 and 4, all six of the cap after the two waits. The + # whole listing's top-up, which keeps 650, takes page 3 again under a + # cap of its own, and settles. near = self.entry(650, timedelta(minutes=30)) self.write_carry([*self.base, near, self.entry(600, timedelta(minutes=30))]) fake = self.fake(self.earlier_fail(750), near) @@ -5395,8 +5501,9 @@ def test_a_dropped_entry_does_not_spend_the_top_ups_page_cap(self) -> None: reader, evidence = self.judge(fake) self.assertEqual(evidence.refusal, "verify:earlier-fail", evidence.detail) self.assertEqual(reader.listing_mode, "full") - self.assertEqual(self.sleep.call_args_list, []) - self.assertEqual(reader._near_expiry_reads, 3) + waits = [c.args[0] for c in self.sleep.call_args_list] + self.assertEqual(waits, list(ve.INCREMENTAL_WAITS)) + self.assertEqual(reader._near_expiry_reads, 1) self.assertFalse(self.failed.exists()) def test_a_short_count_reads_no_pages_near_expiry(self) -> None: @@ -5582,16 +5689,16 @@ def test_the_workflow_cost_is_the_capped_worst_case_at_any_size(self) -> None: + ve.INCREMENTAL_READS * 13 + 1 + ve.LISTING_READS * (attempts * pages + ve.LISTING_TOP_UP_PAGES) - + ve.NEAR_EXPIRY_PAGE_CAP - + ve.NEAR_EXPIRY_ID_READ_CAP + # Once for the incremental reads, once for the top-ups. + + 2 * (ve.NEAR_EXPIRY_PAGE_CAP + ve.NEAR_EXPIRY_ID_READ_CAP) ) self.assertEqual(self.cold_cost(total, fails), worst) def test_the_cost_of_two_listings_differs_by_hand(self) -> None: # 10,450 artifacts (106 pages) and 300 FAILs against 7,522 (77) and - # 264: 2,079 against 1,681, worked by hand in ci-lanes-pr-hold.test.sh. - self.assertEqual(self.cold_cost(10_450, 300), 2_079) - self.assertEqual(self.cold_cost(7_522, 264), 1_681) + # 264: 2,137 against 1,739, worked by hand in ci-lanes-pr-hold.test.sh. + self.assertEqual(self.cold_cost(10_450, 300), 2_137) + self.assertEqual(self.cold_cost(7_522, 264), 1_739) # --- The --authority pr-merge path ------------------------------------------------ diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index f87c091b14f..f4c429d5dcc 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -869,7 +869,7 @@ fi # THE READER'S FAIL REFUSALS HOLD (merge-holds.mjs evidenceHold). evidence() { jq -nc --arg refusal "$1" --arg head "${2:-$SHA}" '{refusal: $refusal, head: $head, detail: "why"}'; } for refusal in verify:fail verify:earlier-fail verify:repeated-fail verify:unreadable verify:too-old; do - STUB_EVIDENCE="$(evidence "$refusal")" expect "in the merge queue, the reader's $refusal holds a passed head" 1 3 "::error::#7: pr-verify's reader refuses this head ($refusal: why)" + STUB_EVIDENCE="$(evidence "$refusal")" expect "in the merge queue, the reader's $refusal holds a passed head" 1 3 "::error title=pr-verify hold::#7: pr-verify's reader refuses this head ($refusal: why)" done STUB_EVIDENCE="$(evidence verify:spec-changed)" expect "in the merge queue, a refusal a pass decides is left to the pass checks" 0 3 "#7: no pr-verify FAIL hold: the reader refuses this head only on its pass (verify:spec-changed)." STUB_EVIDENCE="$(evidence verify:fail)" STUB_REVIEWS="[$(approval kyle-sexton 153232337 APPROVED 2026-10-23T11:00:00Z "$SHA")]" \ @@ -884,7 +884,7 @@ STUB_READER_RC=1 expect "in the merge queue, a reader that fails fails closed" 1 # holds, and a trusted approval of the head lifts it as it lifts a FAIL. for refusal in verify:unreadable verify:too-old; do STUB_NO_LISTING=1 STUB_EVIDENCE="$(evidence "$refusal")" \ - expect "in the merge queue, the reader's $refusal before any listing holds" 1 3 "::error::#7: pr-verify's reader refuses this head ($refusal: why)" + expect "in the merge queue, the reader's $refusal before any listing holds" 1 3 "::error title=pr-verify hold::#7: pr-verify's reader refuses this head ($refusal: why)" STUB_NO_LISTING=1 STUB_EVIDENCE="$(evidence "$refusal")" STUB_REVIEWS="[$(approval kyle-sexton 153232337 APPROVED 2026-10-23T11:00:00Z "$SHA")]" \ expect "in the merge queue, a trusted approval of the head lifts the reader's $refusal before any listing" 0 3 "no pr-verify FAIL hold: kyle-sexton approved this head" done @@ -896,32 +896,33 @@ STUB_NO_LISTING=1 STUB_EVIDENCE="$(jq -nc --arg head "$SHA" '{refusal: null, hea # plus the read floor left. A cold read's cost comes from the live listing # (STUB_TOTAL artifacts, STUB_LISTED_FAILS FAILs): 3 reads a FAIL, 3 incremental # reads of 13 pages, the count read, and 3 whole listings of 3 attempts of -# ceil(total / 100) + 1 pages, each with a 7-page top-up, and 24 pages near -# expiry read again, and 34 artifacts read by id. By default 7,522 +# ceil(total / 100) + 1 pages, each with a 7-page top-up, and twice (the +# incremental reads, then the top-ups) 24 pages near expiry read again +# and 34 artifacts read by id. By default 7,522 # artifacts (77 pages) and 264 FAILs: 77 for the step's own listing, plus -# 792 + 39 + 1 + 3 x (3 x 77 + 7) + 24 + 34 = 1681, so 1681 x 1 + 0.1 x min(limit, 1000) = 1781 here. +# 792 + 39 + 1 + 3 x (3 x 77 + 7) + 2 x (24 + 34) = 1739, so 1739 x 1 + 0.1 x min(limit, 1000) = 1839 here. rate() { jq -nc --argjson remaining "$1" '{resources: {core: {limit: 5000, remaining: $remaining, reset: 1793030000}}}'; } -STUB_COLD=1 STUB_RATE="$(rate 1781)" expect "in the merge queue, a cold reader with the headroom for it reads" 0 3 "#7: no pr-verify FAIL hold: the reader allows this head." +STUB_COLD=1 STUB_RATE="$(rate 1839)" expect "in the merge queue, a cold reader with the headroom for it reads" 0 3 "#7: no pr-verify FAIL hold: the reader allows this head." if [[ "$(readers)" -eq 1 ]]; then ok "a cold reader with headroom is started"; else fail "a cold reader with headroom ran $(readers) times"; fi -STUB_COLD=1 STUB_RATE="$(rate 1780)" expect "in the merge queue, a cold reader without the headroom holds without reading" 1 3 "::error::#7 is held: no carried run cache from pr-merge's last run, and GITHUB_TOKEN has 1780 requests left, under the 1781 a cold read needs" +STUB_COLD=1 STUB_RATE="$(rate 1838)" expect "in the merge queue, a cold reader without the headroom holds without reading" 1 3 "::error::#7 is held: no carried run cache from pr-merge's last run, and GITHUB_TOKEN has 1838 requests left, under the 1839 a cold read needs" if [[ "$(readers)" -eq 0 ]]; then ok "a cold reader without headroom is never started"; else fail "a cold reader without headroom ran: '$(grep '^reader ' "$TMP_ROOT/log")'"; fi STUB_COLD=1 STUB_RATE='' expect "in the merge queue, a cold reader whose rate limit does not read holds" 1 3 "::error::#7 is held: no carried run cache from pr-merge's last run, and GITHUB_TOKEN's remaining requests could not be read" if [[ "$(readers)" -eq 0 ]]; then ok "a cold reader with an unread rate limit is never started"; else fail "a cold reader with an unread rate limit ran"; fi STUB_COLD=1 STUB_RATE='{"resources":{"core":{"limit":5000}}}' expect "in the merge queue, a rate limit with no remaining count holds a cold reader" 1 3 "GITHUB_TOKEN's remaining requests could not be read" # 10,450 artifacts (105 pages, as on 2026-10-30, so 106) and 300 FAILs: -# 106 + 900 + 39 + 1 + 3 x (3 x 106 + 7) + 24 + 34 = 2079, plus the floor's 100. -STUB_COLD=1 STUB_TOTAL=10450 STUB_LISTED_FAILS=300 STUB_RATE="$(rate 2179)" expect "in the merge queue, a cold read's cost grows with the live listing" 0 3 "cold reads for 1 queue entries need 2179" -STUB_COLD=1 STUB_TOTAL=10450 STUB_LISTED_FAILS=300 STUB_RATE="$(rate 2178)" expect "in the merge queue, a larger live listing holds a cold reader the old cost would start" 1 3 "GITHUB_TOKEN has 2178 requests left, under the 2179 a cold read needs (2079 for each of the 1 entries" -# 150 artifacts (2 pages, so 3) and 10 FAILs: 3 + 30 + 39 + 1 + 3 x (3 x 3 + 7) + 24 + 34 = 179. -STUB_COLD=1 STUB_TOTAL=150 STUB_LISTED_FAILS=10 STUB_RATE="$(rate 279)" expect "in the merge queue, a small live listing needs only its own cost" 0 3 "cold reads for 1 queue entries need 279" +# 106 + 900 + 39 + 1 + 3 x (3 x 106 + 7) + 2 x (24 + 34) = 2137, plus the floor's 100. +STUB_COLD=1 STUB_TOTAL=10450 STUB_LISTED_FAILS=300 STUB_RATE="$(rate 2237)" expect "in the merge queue, a cold read's cost grows with the live listing" 0 3 "cold reads for 1 queue entries need 2237" +STUB_COLD=1 STUB_TOTAL=10450 STUB_LISTED_FAILS=300 STUB_RATE="$(rate 2236)" expect "in the merge queue, a larger live listing holds a cold reader the old cost would start" 1 3 "GITHUB_TOKEN has 2236 requests left, under the 2237 a cold read needs (2137 for each of the 1 entries" +# 150 artifacts (2 pages, so 3) and 10 FAILs: 3 + 30 + 39 + 1 + 3 x (3 x 3 + 7) + 2 x (24 + 34) = 237. +STUB_COLD=1 STUB_TOTAL=150 STUB_LISTED_FAILS=10 STUB_RATE="$(rate 337)" expect "in the merge queue, a small live listing needs only its own cost" 0 3 "cold reads for 1 queue entries need 337" STUB_COLD=1 STUB_TOTAL='' STUB_RATE="$(rate 9999)" expect "in the merge queue, a live listing that does not read holds a cold reader" 1 3 "::error::#7 is held: no carried run cache from pr-merge's last run, and the artifact listing could not be read" if [[ "$(readers)" -eq 0 ]]; then ok "a cold reader with an unread listing is never started"; else fail "a cold reader with an unread listing ran"; fi # Every entry the queue builds at once goes cold together on a missing cache, -# so the need scales with max_entries_to_build: 1681 x 5 + 100 = 8505. -STUB_COLD=1 STUB_RULES=5 STUB_RATE="$(rate 8505)" expect "in the merge queue, a cold reader reads with headroom for every entry the queue builds" 0 3 "#7: no pr-verify FAIL hold: the reader allows this head." -STUB_COLD=1 STUB_RULES=5 STUB_RATE="$(rate 8504)" expect "in the merge queue, a cold reader holds under the headroom of every entry the queue builds" 1 3 "GITHUB_TOKEN has 8504 requests left, under the 8505 a cold read needs" -STUB_COLD=1 STUB_RULES=2 STUB_RATE="$(rate 3461)" expect "in the merge queue, the cold headroom scales with max_entries_to_build" 1 3 "under the 3462 a cold read needs" -STUB_COLD=1 STUB_RULES='' STUB_RATE="$(rate 8505)" expect "in the merge queue, a merge queue rule that does not read holds a cold reader" 1 3 "the merge queue's max_entries_to_build could not be read" +# so the need scales with max_entries_to_build: 1739 x 5 + 100 = 8795. +STUB_COLD=1 STUB_RULES=5 STUB_RATE="$(rate 8795)" expect "in the merge queue, a cold reader reads with headroom for every entry the queue builds" 0 3 "#7: no pr-verify FAIL hold: the reader allows this head." +STUB_COLD=1 STUB_RULES=5 STUB_RATE="$(rate 8794)" expect "in the merge queue, a cold reader holds under the headroom of every entry the queue builds" 1 3 "GITHUB_TOKEN has 8794 requests left, under the 8795 a cold read needs" +STUB_COLD=1 STUB_RULES=2 STUB_RATE="$(rate 3577)" expect "in the merge queue, the cold headroom scales with max_entries_to_build" 1 3 "under the 3578 a cold read needs" +STUB_COLD=1 STUB_RULES='' STUB_RATE="$(rate 8795)" expect "in the merge queue, a merge queue rule that does not read holds a cold reader" 1 3 "the merge queue's max_entries_to_build could not be read" if [[ "$(readers)" -eq 0 ]]; then ok "a cold reader with an unread queue rule is never started"; else fail "a cold reader with an unread queue rule ran"; fi STUB_CARRIED='{"carried-run-record":{},"carried-fail-head-ref":{},"carried-on-branch":{}}' expect "in the merge queue, a carried cache with every section empty is cold" 1 3 "::error::#7 is held: no carried run cache from pr-merge's last run, and GITHUB_TOKEN's remaining requests could not be read" STUB_CARRIED='not json' expect "in the merge queue, a carried cache that does not parse is cold" 1 3 "GITHUB_TOKEN's remaining requests could not be read" @@ -973,7 +974,7 @@ STUB_RECORD='' STUB_REVIEWS='' STUB_ARTIFACTS="{\"pr-verify-7-$SHA-pass-notes-$DIGEST\":[11]}" expect "in the merge queue, a digest-named pass-notes at the head passes" 0 3 "no verifier hold: pr-verify passed this head" STUB_ARTIFACTS='{}' -expect "in the merge queue, no verdict fails the group" 1 3 "::error::#7: pr-verify has not passed this head; wait for pr-verify to pass it" +expect "in the merge queue, no verdict fails the group" 1 3 "::error title=pr-verify hold::#7: pr-verify has not passed this head; wait for pr-verify to pass it" if [[ "$(lookups)" -eq 0 && "$(runs)" -eq 0 ]]; then ok "no verdict makes no lookup by name and no run read" else