diff --git a/.changes/source-control/perf-lane-room-reuse-6a4d19c2.md b/.changes/source-control/perf-lane-room-reuse-6a4d19c2.md new file mode 100644 index 0000000000..7c343465e7 --- /dev/null +++ b/.changes/source-control/perf-lane-room-reuse-6a4d19c2.md @@ -0,0 +1,8 @@ +--- +bump: patch +--- + +### Changed + +- `/source-control:babysit-prs`: `settle_shared` keeps a post-merge answer refused as `unverified-stale` when the only thing that tripped is the demotion probe and `read_demotion` reads the same demotion, as it already keeps a `demotion-changed` answer. Before, any edit to an issue the lane opened made the lane room run a full post-merge check, and made the merge job's `settle` and post-merge-dispatch's idle-tick reuse refuse the report. All three take the new keep. A demotion that differs, or a demotion read that fails, still refuses. The merge gate's own keep in `assess` is unchanged. +- The `lane-room` report gains `postMerge`: `answer` (`reused`, `kept`, `recomputed` or `refused`), `reason` (the reuse refusal behind any but `reused`) and `moved` (for an `unverified-stale` answer not kept: `head-moved`, `config-moved`, `demotion-changed`, `runs-moved` or `unreadable`). enable-auto-merge's `lane room:` log line prints it, so a slow room read shows why it ran the check. 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 5a0212e3ed..2239c9858e 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 @@ -36,6 +36,7 @@ from collections.abc import Callable, Collection, Iterator, Mapping, Sequence from dataclasses import asdict, dataclass, field, replace from datetime import UTC, datetime, timedelta +from functools import partial from pathlib import Path from typing import Any from urllib.parse import quote @@ -5270,7 +5271,7 @@ class PostMergeGate: # How old a shared answer still waiting on a lane merge's verifying run may # be on age alone: that run may have reached a conclusion since. Past it, # `assess` keeps the answer up to `SHARED_MAX_AGE` only on evidence that -# nothing it was read from moved (`_still_current`, with the caller's +# nothing it was read from moved (`_moved`, with the caller's # `runs_unchanged`), else reads it again. SHARED_UNVERIFIED_MAX_AGE = run_cache.MAX_AGE # Refusals that bind the answer to another candidate, not to an older state, @@ -5421,7 +5422,7 @@ def shared_refusal( base the floor its own read would (`merged_since_floor` with the shared demotion), be no older than `SHARED_MAX_AGE` (`SHARED_UNVERIFIED_MAX_AGE` while it passes with a merge unverified, a refusal its callers may - overrule on run evidence: `_still_current`) nor past a quarantine excuse's + overrule on run evidence: `_moved`) nor past a quarantine excuse's end (`_shared_age_refusal`), have no demotion record filed or changed since (`_demotion_changed`), and judge every merge the post-merge check would judge now. The demotion comes last, so @@ -5553,7 +5554,7 @@ def assess( (`merged_since_floor`) and must be given: without it the path refuses. `shared`, the run's one answer for every candidate, replaces the demotion, floor and post-merge reads when `shared_refusal` passes it, or - when it refuses it only as `unverified-stale` and `_still_current` with + when it refuses it only as `unverified-stale` and `_moved` with `runs_unchanged(shared, branch)` (no verifying run of its unverified merges reached a conclusion since) shows nothing it was read from moved (`postMergeShared: kept:unverified-stale`); a kept answer is not handed @@ -5955,7 +5956,9 @@ class LaneRoom: a read failed, which the caller holds on as on a refusal. Otherwise `count` merges are in flight of `cap`, `queued` holds the merge queue's pull request numbers, and `rung` is the effective rung the gate scores - each candidate at ("off" unless the lane has room).""" + each candidate at ("off" unless the lane has room). `post_merge` is how + the run's post-merge answer settled (`settle_shared`'s `outcome`), None + when a read failed before it was settled.""" refusal: str | None = None unreadable: str | None = None @@ -5963,6 +5966,7 @@ class LaneRoom: cap: int = 1 queued: frozenset[int] = frozenset() rung: str = "off" + post_merge: Mapping[str, str | None] | None = None def report(self) -> dict[str, Any]: return { @@ -5972,30 +5976,49 @@ def report(self) -> dict[str, Any]: "cap": self.cap, "queued": sorted(self.queued), "rung": self.rung, + "postMerge": None if self.post_merge is None else dict(self.post_merge), } -def _still_current( +def _moved( repo: str, default: str, identity: str, shared: SharedPostMerge, runs_unchanged: Callable[[SharedPostMerge, str], bool], -) -> bool: - """Whether the post-merge check would answer `shared` again, in order: - the default branch head is its `main_sha`, the head's config commit is its - `config_sha` (with the demotion unchanged, the same floor), no demotion - record was filed or changed since (`_demotion_changed`), and - `runs_unchanged`: none of its unverified merges' verifying runs reached a - conclusion since. Raises `PolicyReadError`.""" + *, + reread_demotion: bool = False, +) -> str | None: + """What `shared` was read from that has moved since, or None when the + post-merge check would answer it again, in order: the default branch head + is its `main_sha` (else `head-moved`), the head's config commit is its + `config_sha` (with the demotion unchanged, the same floor; else + `config-moved`), no demotion record was filed or changed since + (`_demotion_changed`, else `demotion-changed`), and `runs_unchanged`: + none of its unverified merges' verifying runs reached a conclusion since + (else `runs-moved`). With `reread_demotion`, a tripped demotion probe + counts only when `read_demotion`, read after the runs, differs from the + shared demotion in any field, as `settle_shared` keeps a + `demotion-changed` answer. Raises `PolicyReadError`.""" owner, name = _split_repo(repo) _, head = _default_branch_head(repo, default) - return ( - head == shared.main_sha - and newest_config_commit(repo, head) == shared.config_sha - and not _demotion_changed(owner, name, shared, identity) - and runs_unchanged(shared, default) - ) + if head != shared.main_sha: + return "head-moved" + if newest_config_commit(repo, head) != shared.config_sha: + return "config-moved" + probed = _demotion_changed(owner, name, shared, identity) + if probed and not reread_demotion: + return "demotion-changed" + if not runs_unchanged(shared, default): + return "runs-moved" + if probed and shared.demotion != read_demotion( + repo, + agent_identity=identity, + self_logins=shared.self_logins, + permission=permission_reader(repo), + ): + return "demotion-changed" + return None def _kept_unverified( @@ -6008,12 +6031,12 @@ def _kept_unverified( ) -> bool: """Whether an answer refused only as `unverified-stale` is kept up to `SHARED_MAX_AGE`: nothing it was read from moved and no verifying run of - its unverified merges reached a conclusion since (`_still_current`). No + its unverified merges reached a conclusion since (`_moved`). No `runs_unchanged` or any failed read is not kept.""" if unusable != "unverified-stale" or runs_unchanged is None: return False try: - return _still_current(repo, default, identity, shared, runs_unchanged) + return _moved(repo, default, identity, shared, runs_unchanged) is None except (RuntimeError, ValueError, OSError): return False @@ -6107,18 +6130,27 @@ def settle_shared( runs_unchanged: Callable[[SharedPostMerge, str], bool] | None = None, keep: bool = True, post_merge: Callable[[datetime, str], PostMergeGate] | None = None, + outcome: dict[str, str | None] | None = None, ) -> tuple[SharedPostMerge | None, str | None]: """The answer to use for the run's post-merge check in place of `shared`, or None and why not (`post-merge answer `): the gate's own reuse rule (`shared_refusal`) as a candidate at the default branch head `base_sha` with the gate's self logins (none). With `keep`, two refusals keep `shared`: `unverified-stale` when nothing it was read from - moved (`_still_current`, with `runs_unchanged`), and `demotion-changed` - when `read_demotion` reads the shared demotion again. Any other refusal + moved (`_moved`, with `runs_unchanged` and `reread_demotion`), and + `demotion-changed` when `read_demotion` reads the shared demotion again. Any other refusal is read again with `post_merge` (`_recompute_shared`), or with none refuses. Without `code_unchanged` (`check_code_unchanged`), `shared` came from other check code: `code-changed`, never reused or kept. One rule for - the lane room and the merge job's reuse of post-merge-dispatch's report.""" + the lane room and the merge job's reuse of post-merge-dispatch's report. + + `outcome`, when given, is filled with how it settled: `answer` is + `reused`, `kept`, `recomputed` or `refused`, `reason` the + `shared_refusal` (or `code-changed`) behind any but `reused`, and `moved` + what `_moved` found for an `unverified-stale` answer not kept (or + `unreadable`).""" + note = outcome if outcome is not None else {} + note.update(answer="refused", reason=None, moved=None) unusable = ( shared_refusal( repo, @@ -6135,10 +6167,13 @@ def settle_shared( else "code-changed" ) if not unusable: + note["answer"] = "reused" return shared, None + note["reason"] = unusable # An unverified-stale answer is 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). + # lane opened that triage edited, most runs); the unverified keep takes + # that same demotion read when the probe trips. try: if not keep: current = False @@ -6149,13 +6184,18 @@ def settle_shared( self_logins=frozenset(), permission=permission_reader(repo), ) - else: - current = _kept_unverified( - repo, default, identity, shared, unusable, runs_unchanged + elif unusable == "unverified-stale" and runs_unchanged is not None: + note["moved"] = _moved( + repo, default, identity, shared, runs_unchanged, reread_demotion=True ) + current = note["moved"] is None + else: + current = False except (RuntimeError, ValueError, OSError): + note["moved"] = "unreadable" if unusable == "unverified-stale" else None current = False if current: + note["answer"] = "kept" return shared, None if post_merge is None: return None, f"post-merge answer {unusable}" @@ -6165,6 +6205,7 @@ def settle_shared( return None, f"post-merge answer {unusable}: {exc}" if fresh is None: return None, f"post-merge answer {unusable}: unreadable" + note["answer"] = "recomputed" return fresh, None @@ -6189,8 +6230,9 @@ def lane_room( (`_recompute_shared`, with the room's self logins, none), and a readable 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` + (`_moved`, with `runs_unchanged(shared, branch)` for its + unverified merges' runs and the demotion read again when its probe + trips), and one refused only as `demotion-changed` when `read_demotion` alone reads the shared demotion again. - A config naming another authority stops (`enabled_authority`). @@ -6217,6 +6259,7 @@ def lane_room( identity = config.agent_identity if not identity: return LaneRoom(unreadable="no agent identity") + note: dict[str, str | None] = {} settled, unusable = settle_shared( repo, shared, @@ -6229,12 +6272,14 @@ def lane_room( # With no check to read it again, the room keeps nothing either. keep=post_merge is not None, post_merge=post_merge, + outcome=note, ) + room = partial(LaneRoom, post_merge=note) if settled is None: - return LaneRoom(unreadable=unusable) + return room(unreadable=unusable) shared = settled if shared.gate.refusal: - return LaneRoom(refusal=f"post-merge:{shared.gate.refusal}") + return room(refusal=f"post-merge:{shared.gate.refusal}") cap = rung_cap(repo, authority=PR_MERGE, lane_value=lane_rung_cap) rung = effective_rung( config_rung=config.rung, @@ -6243,7 +6288,7 @@ def lane_room( demoted=shared.demotion.active, ) if rung.rung == "off": - return LaneRoom(refusal=f"rung-off:{rung.limited_by}") + return room(refusal=f"rung-off:{rung.limited_by}") flight_cap, _ = _run_max_in_flight(repo, default, config.base_sha, config) flight = in_flight( repo, @@ -6257,8 +6302,8 @@ def lane_room( ) counted = str(flight.reason).startswith(FLIGHT_COUNTED) if flight.queued is None or not (flight.clear or counted): - return LaneRoom(unreadable=f"in-flight:{flight.reason}", cap=flight_cap) - return LaneRoom( + return room(unreadable=f"in-flight:{flight.reason}", cap=flight_cap) + return room( count=flight.count if flight.clear else flight_cap, cap=flight_cap, queued=flight.queued, 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 23a56c047b..fad5efeb90 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 @@ -599,7 +599,10 @@ def test_anything_moved_since_reads_the_answer_again(self) -> None: "(b) the config commit moved": dict( config_commit="f" * 40, fresh_floor=FLOOR ), - "(c) a demotion record changed": dict(demotion_changed=True), + "(c) a demotion record was filed": dict( + demotion_changed=True, + fresh_demotion=policy.Demotion(False, None, (9,)), + ), "(d) a verifying run reached a conclusion": dict(runs=False), } for name, kwargs in cases.items(): @@ -608,6 +611,79 @@ def test_anything_moved_since_reads_the_answer_again(self) -> None: self.assertEqual(len(recheck.asked), 1, "one full post-merge check") self.assertEqual((room.refusal, room.unreadable), (None, None)) + def test_an_unverified_answer_whose_demotion_reads_the_same_is_kept( + self, + ) -> None: + # The demotion probe trips on any edit to an issue the lane opened; + # the room already keeps a demotion-changed answer when the demotion + # reads the same, and an unverified-stale one is no different. + room, recheck, _ = self.current(demotion_changed=True) + self.assertEqual(recheck.asked, [], "no full post-merge check") + self.assertEqual((room.refusal, room.unreadable), (None, None)) + self.assertEqual(self.reads.call_args.kwargs["self_logins"], frozenset()) + + def test_the_report_names_how_the_post_merge_answer_was_settled(self) -> None: + # The brief: whether the answer was reused or recomputed and, when + # recomputed, the refusal; for an unverified-stale answer not kept, + # which of what it was read from moved. + def settled(room: policy.LaneRoom) -> object: + return room.report()["postMerge"] + + def unreadable(answer: object, branch: str) -> bool: + raise policy.PolicyReadError("runs 502") + + self.assertEqual( + settled(self.room()), {"answer": "reused", "reason": None, "moved": None} + ) + self.assertEqual( + settled(self.current()[0]), + {"answer": "kept", "reason": "unverified-stale", "moved": None}, + ) + recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) + self.assertEqual( + settled( + self.room( + post_merge=recheck.post_merge, + **self.DEMOTION_CHANGED, # type: ignore[arg-type] + ) + ), + {"answer": "kept", "reason": "demotion-changed", "moved": None}, + ) + for moved, kwargs in { + "head-moved": dict(head=NEW_MAIN), + "config-moved": dict(config_commit="f" * 40, fresh_floor=FLOOR), + "runs-moved": dict(runs=False), + "unreadable": dict(runs=unreadable), + "demotion-changed": dict( + demotion_changed=True, + fresh_demotion=policy.Demotion(False, None, (9,)), + ), + }.items(): + with self.subTest(moved): + self.assertEqual( + settled(self.current(**kwargs)[0]), + { + "answer": "recomputed", + "reason": "unverified-stale", + "moved": moved, + }, + ) + recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) + self.assertEqual( + settled( + self.room(shared(), post_merge=recheck.post_merge, code_unchanged=False) + ), + {"answer": "recomputed", "reason": "code-changed", "moved": None}, + ) + # With no check to read it again, the room refuses it. + self.assertEqual( + settled(self.room(shared(report=PENDING), now=STALE_NOW)), + {"answer": "refused", "reason": "unverified-stale", "moved": None}, + ) + # A read that fails before the answer is settled names nothing. + failed = self.room(config_error=policy.PolicyReadError("contents 502")) + self.assertIsNone(settled(failed)) + def test_a_quarantine_that_expired_since_reads_the_answer_again(self) -> None: # (e): the answer excused a failing suite under a quarantine that # expired a minute after it was read, so the same red run now demotes. @@ -829,6 +905,7 @@ def settle( runs: bool | None = None, keep: bool = True, code_unchanged: bool = True, + demotion_error: Exception | None = None, ) -> tuple[policy.SharedPostMerge | None, str | None]: merges = [{"number": n} for n in lane_merges] @@ -836,7 +913,8 @@ def derive(pr: dict[str, object], *args: object, **kwargs: object) -> object: return policy.AutoMerge(int(pr["number"]), "a" * 40, "b" * 40, None) # type: ignore[call-overload] self.reads = mock.Mock( - return_value=fresh_demotion or policy.Demotion(False, None, ()) + return_value=fresh_demotion or policy.Demotion(False, None, ()), + side_effect=demotion_error, ) self.runs = mock.Mock(return_value=runs) with ( @@ -913,6 +991,32 @@ def test_an_unverified_answer_past_two_minutes_is_refused_unless_nothing_moved( self.settle(answer, now=FLOOR + timedelta(minutes=1)), (answer, None) ) + def test_an_unverified_stale_answer_keeps_through_a_demotion_that_reads_the_same( + self, + ) -> None: + answer = shared(report=PENDING) + stale = dict(now=STALE_NOW, runs=True, demotion_changed=True) + # The probe tripped but the demotion reads the same: kept. + self.assertEqual(self.settle(answer, **stale), (answer, None)) # type: ignore[arg-type] + # A record filed since, or a demotion read that fails: refused. + moved = policy.Demotion(False, None, (9,)) + for name, kwargs in { + "moved": dict(fresh_demotion=moved), + "unreadable": dict(demotion_error=policy.PolicyReadError("issues 502")), + }.items(): + with self.subTest(name): + self.assertEqual( + self.settle(answer, **stale, **kwargs), # type: ignore[arg-type] + (None, "post-merge answer unverified-stale"), + ) + # A verifying run that reached a conclusion refuses before the + # demotion is read again. + self.assertEqual( + self.settle(answer, now=STALE_NOW, runs=False, demotion_changed=True), + (None, "post-merge answer unverified-stale"), + ) + self.reads.assert_not_called() + def test_a_changed_demotion_is_refused_unless_it_reads_the_same(self) -> None: answer = shared(report=self.VERIFIED) moved = policy.Demotion(False, None, (9,)) @@ -1426,7 +1530,13 @@ def room( runs_unchanged=runs_unchanged, code_unchanged=code_unchanged, ) - return policy.LaneRoom(count=1, cap=5, queued=frozenset({3}), rung="C2") + return policy.LaneRoom( + count=1, + cap=5, + queued=frozenset({3}), + rung="C2", + post_merge={"answer": "recomputed", "reason": "stale", "moved": None}, + ) out = io.StringIO() with ( @@ -1467,6 +1577,7 @@ def room( "cap": 5, "queued": [3], "rung": "C2", + "postMerge": {"answer": "recomputed", "reason": "stale", "moved": None}, }, ) self.assertTrue(callable(hooks["post_merge"])) 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 baee22a26d..3e16f1d265 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 @@ -7481,7 +7481,7 @@ def test_an_unreadable_recompute_is_not_handed_back(self) -> None: class AStaleUnverifiedAnswerIsKeptOnRunEvidence(unittest.TestCase): """Past `SHARED_UNVERIFIED_MAX_AGE`, a passing answer with lane merge #8040 unverified is kept up to `SHARED_MAX_AGE` when nothing it was read - from moved (`_still_current`) and no verifying run of #8040 reached a + from moved (`_moved`) and no verifying run of #8040 reached a conclusion since its `computedAt` (the real `babysit_postmerge.pending_runs_unchanged`, its listing faked at `_read`). The expected decisions are a recompute's: with no run concluded, the @@ -7499,7 +7499,7 @@ class AStaleUnverifiedAnswerIsKeptOnRunEvidence(unittest.TestCase): def aged(self, minutes: float) -> GhFake: """Bound, with the answer computed `minutes` ago and main's head - on the answer's config commit, as `_still_current` reads it.""" + on the answer's config commit, as `_moved` reads it.""" gh = AStaleSharedAnswerIsRefreshed.aged(self, minutes) gh.rest[NEWEST_CONFIG.replace(SHA, MAIN_SHA)] = [{"sha": CONFIG_SHA}] return gh @@ -7636,7 +7636,7 @@ def test_a_failed_or_partial_run_read_reads_the_answer_again(self) -> None: self.assertEqual(len(checks), 1) def test_a_moved_head_reads_the_answer_again(self) -> None: - # `_still_current` binds the keep to main's head: a newer head reads + # `_moved` binds the keep to main's head: a newer head reads # the check afresh even with no run concluded. self.aged(5) self.judged_flight()