diff --git a/.changes/pr-pipeline/perf-lane-room-demotion-changed-keeps-d76f08af.md b/.changes/pr-pipeline/perf-lane-room-demotion-changed-keeps-d76f08af.md new file mode 100644 index 0000000000..e119a79344 --- /dev/null +++ b/.changes/pr-pipeline/perf-lane-room-demotion-changed-keeps-d76f08af.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Changed + +- `enable-auto-merge` no longer spends most of its lane-room time limit on a full post-merge check when the run's report was refused only because an issue the lanes App opened was edited since (`demotion-changed`). The room reads the demotion records alone and keeps the report when they are unchanged, so a slow API window no longer risks running past the time limit and disarming every armed lanes App pull request. diff --git a/.changes/source-control/perf-lane-room-demotion-changed-keeps-d76f08af.md b/.changes/source-control/perf-lane-room-demotion-changed-keeps-d76f08af.md new file mode 100644 index 0000000000..00ab830de2 --- /dev/null +++ b/.changes/source-control/perf-lane-room-demotion-changed-keeps-d76f08af.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Changed + +- `babysit_verify_evidence.py lane-room` no longer runs the full post-merge check when the only reason the gate would not reuse the run's answer is `demotion-changed`, which any issue the lanes App opens and triage then edits sets. It reads the demotion records alone (`read_demotion`) and keeps the answer when they read as the answer recorded them; a demotion that turned active or moved still runs the full check, so a real demotion still holds the rung off. `shared_refusal` now checks the demotion last, so `demotion-changed` is reported only when nothing else refuses: a lane merge the answer does not count reports `new-merge:#` first. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index e6d2dd9772..5f352679dc 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -58,9 +58,11 @@ // arms; at the cap it arms nothing more. A room that cannot be read (no // post-merge report, a failed read) holds as a refusal does: nothing is // armed and every armed lanes App pull request is disarmed. A report the -// gate would not reuse is checked again as the gate does; one past its -// reuse age only for a lane merge still unverified is first kept while -// nothing it was read from has moved. A held room raises one warning annotation and step-summary line +// gate would not reuse, for any reason, is checked again as the gate does; +// one past its reuse age only for a lane merge still unverified is first +// kept while nothing it was read from has moved, and one refused only for +// a demotion record filed or changed since is first kept when the +// demotion records alone read as they were. A held room raises one warning annotation and step-summary line // naming why and each lane pull request left unarmed or, report-only, // that a disarm would have turned off, and one error and summary line // naming each one still armed because its disarm failed. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index db5816514f..fc7cdd8d06 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1135,9 +1135,9 @@ count; an unread count leaves it unarmed with no comment, and the next run reads armed one is not disarmed on the count. Each such run also waits for the same run's `post-merge-dispatch` job and reads the lane's room from the post-merge report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report it -holds as a refusal does; one the gate would not reuse (`shared_refusal`: too old, a demotion record -changed since, or a lane merge landed since that it does not count) is checked again as the gate -does; one past the gate's +holds as a refusal does; one the gate would not reuse, for any `shared_refusal`, is checked again +as the gate does; one refused only because a demotion record was filed or changed since is first +kept when the demotion records alone read as they were; one past the gate's two-minute reuse age only because a lane merge is still unverified is kept while main's head, its config commit, the demotion records and the unverified merges' full and canary runs are as they were (no run reached a conclusion since, no merge is past the six-hour dispatch deadline, and no diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index ef3a013567..2f042a6b90 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -4494,11 +4494,13 @@ def shared_refusal( demotion), be no older than `SHARED_MAX_AGE` (`SHARED_UNVERIFIED_MAX_AGE` while it passes with a merge unverified), have no demotion record filed or changed since (`_demotion_changed`), and judge every merge the - post-merge check would judge now. The binding comes before the age, so a - candidate bound elsewhere reports `SHARED_BINDING_REFUSALS` and does not - replace the run's answer. The floor is compared under the shared answer's - demotion, which may be out of date; that changes only the reason and the - write-back, since any refusal reads afresh. + post-merge check would judge now. The demotion comes last, so + `demotion-changed` means nothing else refused (`lane_room` keeps such an + answer when the demotion reads the same). The binding comes before the + age, so a candidate bound elsewhere reports `SHARED_BINDING_REFUSALS` and + does not replace the run's answer. The floor is compared under the + shared answer's demotion, which may be out of date; that changes only the + reason and the write-back, since any refusal reads afresh. The check judges `branch` from the floor, never the base, so the answer holds for any base with that floor. A newer default-branch head changes @@ -4535,33 +4537,32 @@ def shared_refusal( refusal = _shared_age_refusal(shared, age) if refusal: return refusal - if not shared.demotion.active and _demotion_changed(owner, name, shared, identity): - return "demotion-changed" status, payload = _get(f"repos/{owner}/{name}/commits/{quote(branch, safe='')}") head = payload.get("sha") if status == 200 and is_json_object(payload) else None - if head == shared.main_sha: - return None - counted = { - json_object(m).get("number") - for m in json_array(shared.gate.report.get("autoMerges")) - } - try: - merges = _merged_since(owner, name, branch, shared.floor) - if merges is None: - return "merged-history-truncated" - for pr in merges: - # Treating no actor as human counts a superset of the check's - # merges, so no merge it would judge is missed. - merge = derive_auto_merge( - pr, - lambda actor: False, - identity, - lane_merges_only=shared.lane_merges_only, - ) - if isinstance(merge, AutoMerge) and merge.number not in counted: - return f"new-merge:#{merge.number}" - except PolicyReadError: - return "merged-unreadable" + if head != shared.main_sha: + counted = { + json_object(m).get("number") + for m in json_array(shared.gate.report.get("autoMerges")) + } + try: + merges = _merged_since(owner, name, branch, shared.floor) + if merges is None: + return "merged-history-truncated" + for pr in merges: + # Treating no actor as human counts a superset of the check's + # merges, so no merge it would judge is missed. + merge = derive_auto_merge( + pr, + lambda actor: False, + identity, + lane_merges_only=shared.lane_merges_only, + ) + if isinstance(merge, AutoMerge) and merge.number not in counted: + return f"new-merge:#{merge.number}" + except PolicyReadError: + return "merged-unreadable" + if not shared.demotion.active and _demotion_changed(owner, name, shared, identity): + return "demotion-changed" return None @@ -5094,12 +5095,14 @@ def lane_room( `assess` applies to every lane candidate, read once at the default branch head, in its order. `shared` is this run's `babysit_postmerge.py` answer (`babysit_postmerge.read_shared`), as the gate's `--postmerge-result`. - `post_merge` is `assess`'s: an answer the gate would not reuse - (`shared_refusal`) is read again as the gate reads it + `post_merge` is `assess`'s: an answer the gate would not reuse, for any + `shared_refusal`, is read again as the gate reads it (`_recompute_shared`, with the room's self logins, none), and a readable - one stands in for `shared`; one refused only as `unverified-stale` is - first kept when nothing it was read from has moved (`_still_current`, - with `runs_unchanged(shared, branch)` for its unverified merges' runs). + one stands in for `shared`. Two refusals are first kept: one refused only + as `unverified-stale` when nothing it was read from has moved + (`_still_current`, with `runs_unchanged(shared, branch)` for its + unverified merges' runs), and one refused only as `demotion-changed` + when `read_demotion` alone reads the shared demotion again. - A config naming another authority stops (`enabled_authority`). - A post-merge refusal stops (`post-merge:`). @@ -5111,9 +5114,8 @@ def lane_room( Any read failure, a missing `shared` and an in-flight count that could not be read included, is `unreadable`. So is a `shared` the gate would - not reuse (too old, a demotion record filed or changed since, a lane - merge landed since that it does not count) when there is no `post_merge` - to read it again, or that read fails or is not readable.""" + not reuse (any `shared_refusal`) when there is no `post_merge` to read it + again, or that read fails or is not readable.""" if shared is None: return LaneRoom(unreadable="post-merge answer missing or unreadable") try: @@ -5143,13 +5145,23 @@ def lane_room( ) if unusable and post_merge is not None: # As the gate, any refusal reads afresh; an unverified-stale answer - # is first kept when nothing it was read from moved. + # is first kept when nothing it was read from moved, a + # demotion-changed one when the demotion reads the same (an issue + # the lane opened that triage edited, most runs). try: - current = ( - unusable == "unverified-stale" - and runs_unchanged is not None - and _still_current(repo, default, identity, shared, runs_unchanged) - ) + if unusable == "demotion-changed": + current = shared.demotion == read_demotion( + repo, + agent_identity=identity, + self_logins=frozenset(), + permission=permission_reader(repo), + ) + else: + current = ( + unusable == "unverified-stale" + and runs_unchanged is not None + and _still_current(repo, default, identity, shared, runs_unchanged) + ) except (RuntimeError, ValueError, OSError): current = False if not current: 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 6a9e0ad78c..274dc1885d 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 @@ -1992,9 +1992,10 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: (`merge_policy.lane_room`, `LaneRoom.report`), from this run's `babysit_postmerge.py` report and the `CLAUDE_LANES_MERGE_RUNG_CAP` value the workflow read (empty is no cap; missing holds the rung off). A report - the gate would not reuse is checked again; one refused only as - `unverified-stale` is first kept while nothing it was read from has - moved.""" + the gate would not reuse, for any `shared_refusal`, is checked again; one + refused only as `unverified-stale` is first kept while nothing it was + read from has moved, and one refused only as `demotion-changed` when the + demotion reads the same.""" parser = argparse.ArgumentParser( prog="babysit_verify_evidence.py", allow_abbrev=False ) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py index a638f95577..265200fa79 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py @@ -16,6 +16,7 @@ import pathlib import sys import unittest +from dataclasses import replace from datetime import UTC, datetime, timedelta from unittest import mock @@ -133,6 +134,9 @@ def room( fresh_demotion: policy.Demotion | None = None, ) -> policy.LaneRoom: seen: dict[str, object] = {} + reads = mock.Mock( + return_value=fresh_demotion or policy.Demotion(False, None, ()) + ) def in_flight(repo: str, **kwargs: object) -> policy.InFlight: seen.update(kwargs) @@ -168,11 +172,7 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight: return_value=("main", head), ), mock.patch.object(policy, "permission_reader"), - mock.patch.object( - policy, - "read_demotion", - return_value=fresh_demotion or policy.Demotion(False, None, ()), - ), + mock.patch.object(policy, "read_demotion", reads), mock.patch.object(policy, "merged_since_floor", return_value=fresh_floor), ): kwargs = {"post_merge": post_merge} if post_merge is not None else {} @@ -185,7 +185,7 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight: now=now, **kwargs, # type: ignore[arg-type] ) - self.seen = seen + self.seen, self.reads = seen, reads return result def test_a_clear_lane_counts_its_merges_in_flight_against_the_cap(self) -> None: @@ -239,17 +239,16 @@ def test_at_the_cap_it_reports_the_cap_and_refuses_nothing(self) -> None: (room.refusal, room.unreadable, room.count, room.cap), (None, None, 5, 5) ) - # The gate's reuse refusals other than unverified-stale (shared_refusal): - # past SHARED_MAX_AGE (a re-run job reads the run's first report), from - # the future, and a demotion record filed or changed since, inside the - # two minutes the gate reuses a pending answer for. + # Reuse refusals the room reads afresh (shared_refusal): past + # SHARED_MAX_AGE (a re-run job reads the run's first report) and from the + # future. UNUSABLE = { "stale": dict(now=FLOOR + policy.SHARED_MAX_AGE + timedelta(seconds=1)), "future": dict(now=FLOOR - timedelta(seconds=1)), - "demotion changed": dict( - demotion_changed=True, now=FLOOR + timedelta(minutes=1) - ), } + # A demotion record filed or changed since, inside the two minutes the + # gate reuses a pending answer for. + DEMOTION_CHANGED = dict(demotion_changed=True, now=FLOOR + timedelta(minutes=1)) def test_an_answer_the_gate_would_not_reuse_is_read_again(self) -> None: # The gate reads afresh on any refusal (assess); so does the room, @@ -287,34 +286,89 @@ def test_an_unusable_answer_is_never_kept_on_the_cheap_path(self) -> None: self.assertEqual(len(recheck.asked), 1) runs.assert_not_called() - def test_a_demotion_read_again_holds_the_room_off(self) -> None: + def test_a_no_op_demotion_edit_keeps_the_answer(self) -> None: + # Triage editing an issue the lane opened refuses the answer as + # `demotion-changed`; a demotion read alone that finds it unchanged + # keeps it, so the room does not pay the full post-merge check. recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) + answer = shared() room = self.room( + answer, post_merge=recheck.post_merge, - fresh_demotion=policy.Demotion(True, "record-open", (8,)), - **self.UNUSABLE["demotion changed"], # type: ignore[arg-type] + **self.DEMOTION_CHANGED, # type: ignore[arg-type] ) - self.assertEqual(room.refusal, "rung-off:demotion") + self.assertEqual( + (room.refusal, room.unreadable, room.count, room.cap), (None, None, 2, 5) + ) + self.assertEqual(recheck.asked, [], "no full post-merge check") + self.reads.assert_called_once() + self.assertEqual(self.reads.call_args.kwargs["self_logins"], frozenset()) + # Counted from the kept answer: its floor and its verified merges. + self.assertEqual(self.seen["merged_since"], FLOOR) + self.assertIs(self.seen["verified"], answer.gate.verified) + + def test_a_demotion_that_moved_reads_the_answer_again(self) -> None: + # Active, the rung is off; inactive with other records or another + # clear, the floor may differ. Either way the full read runs. + cases = { + "active": ( + policy.Demotion(True, "record-open", (8,)), + "rung-off:demotion", + ), + "another record": (policy.Demotion(False, None, (9,)), None), + } + for name, (demotion, refusal) in cases.items(): + with self.subTest(name): + recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) + room = self.room( + post_merge=recheck.post_merge, + fresh_demotion=demotion, + **self.DEMOTION_CHANGED, # type: ignore[arg-type] + ) + self.assertEqual((room.refusal, room.unreadable), (refusal, None)) + self.assertEqual(recheck.asked, [(NEW_FLOOR, IDENTITY)]) + + def test_an_answer_bound_to_other_self_logins_is_read_with_none(self) -> None: + # The room binds no self logins, so a fresh read uses none and the + # in-flight count excludes none: not the answer's own. + answer = replace(shared(), self_logins=frozenset({"lane-self[bot]"})) + recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) + room = self.room(answer, post_merge=recheck.post_merge) + self.assertEqual((room.refusal, room.unreadable), (None, None)) + self.assertEqual(recheck.asked, [(NEW_FLOOR, IDENTITY)]) + self.assertEqual(self.reads.call_args.kwargs["self_logins"], frozenset()) + self.assertEqual(self.seen["self_logins"], frozenset()) def test_an_unusable_answer_with_no_fresh_read_is_unreadable(self) -> None: cases = { - "no check": None, - "check raises": Recheck(error=RuntimeError("runs 502")).post_merge, - "check unreadable": Recheck( - policy.PostMergeGate("runs-unreadable", {"readable": False}, None) - ).post_merge, + "no check": (None, self.UNUSABLE["stale"], "stale"), + "no check, demotion changed": ( + None, + self.DEMOTION_CHANGED, + "demotion-changed", + ), + "check raises": ( + Recheck(error=RuntimeError("runs 502")).post_merge, + self.UNUSABLE["stale"], + "stale", + ), + "check unreadable": ( + Recheck( + policy.PostMergeGate("runs-unreadable", {"readable": False}, None) + ).post_merge, + self.UNUSABLE["stale"], + "stale", + ), } - for name, post_merge in cases.items(): + for name, (post_merge, kwargs, reason) in cases.items(): with self.subTest(name): room = self.room( post_merge=post_merge, - **self.UNUSABLE["demotion changed"], # type: ignore[arg-type] + **kwargs, # type: ignore[arg-type] ) self.assertIsNone(room.refusal) self.assertTrue( - str(room.unreadable).startswith( - "post-merge answer demotion-changed" - ), + str(room.unreadable).startswith(f"post-merge answer {reason}"), room.unreadable, ) self.assertFalse(self.seen, "no in-flight read") @@ -340,6 +394,7 @@ def refusal(repo: str, answer: object, **kwargs: object) -> str: (room.refusal, room.unreadable), (None, "post-merge answer new-merge:#7"), ) + self.assertFalse(self.seen, "no in-flight read after an unusable answer") # Bound as the gate binds a candidate at the default branch head. self.assertEqual( { diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py index a2b05f56bc..993af22cc8 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py @@ -6203,6 +6203,23 @@ def test_a_demotion_record_filed_since_the_answer_is_not_missed(self) -> None: reads.assert_called_once() self.assertEqual(seen, [policy.datetime.fromisoformat(ENABLED_AT)]) + def test_demotion_changed_is_reported_only_when_nothing_else_refuses( + self, + ) -> None: + # lane_room keeps an answer refused only as `demotion-changed` once a + # demotion read agrees, so a lane merge it does not count must not + # hide behind that reason. + gh = self.moved(merge_pages([[merged(7, bot("agent-lanes"), None)]])) + gh.rest[self.issues_since()] = [ + {"number": 12, "user": {"login": OPERATOR}, "state": "closed"} + ] + self.flight() + result = self.lane_assess( + post_merge=lambda floor, who: gate(), + shared=self.shared(gate(), demotion=policy.Demotion(False, None, (12,))), + ) + self.assertEqual(result.details["postMergeShared"], "unused:new-merge:#7") + def test_an_unrelated_issue_or_a_pull_request_leaves_the_answer_bound( self, ) -> None: