From 12968497dc609a280f76a7eafd6d94a099a78136 Mon Sep 17 00:00:00 2001 From: Kim Morrison Date: Thu, 17 Sep 2026 09:51:54 +1000 Subject: [PATCH 1/3] fix: close an area's stranded reports once a live one is open An area can hold more than one open report, and nothing reconciled the ones that are not the current window. Two things strand a report and neither closes itself: a sibling merging advances the cursor, which orphans every other open report for that area instantly, since the merge gate requires a byte-exact append at the cursor; and any repository-wide breakage fails every report's `build`, so the planner's staleness expiry releases the area each round and the next round opens a fresh report that also cannot merge. The repository grew one unmergeable report per area per round and never shed them. After the new report is open, close the area's reports that can no longer merge: those whose window does not start at the cursor, and those that start at the cursor but predate ours, whose window ours therefore covers. Ordering by creation date keeps the newest report the winner deterministically, so two workers racing cannot each close the other. Only reports we could have opened are closed, for the reason the rest of the module is careful about ownership: branch names are a pure function of the window, so a stranger can create one, and closing theirs would be a silent veto. A failure to close is reported and never fatal -- by then the report is already published. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JedNDof6uYKixz6MvtZzCX --- progress/apply.py | 84 ++++++++++++++++++++++++++ tests/test_supersede.py | 127 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 211 insertions(+) create mode 100644 tests/test_supersede.py diff --git a/progress/apply.py b/progress/apply.py index 07c011d..03f0c3f 100644 --- a/progress/apply.py +++ b/progress/apply.py @@ -22,6 +22,11 @@ "our": anyone may open a pull request on a `progress/*` branch, and branch names are a pure function of the window, so honouring a stranger's would let them decide what this operator is allowed to publish -- permanently, by opening and closing one pull request. + +Reconciling the window is not enough on its own, because an area can hold more than one open report +and the others are not reconciled by anything. Once ours is open we therefore close the area's +reports that can no longer merge -- see `superseded_prs` for the two ways that happens. Without it +the repository grows one unmergeable report per area per round and never sheds them. """ import json @@ -143,6 +148,66 @@ def existing_pr(branch, repo=gh.ROADMAP_REPO, owner=None, states=("merged",)): return rows[0] if rows else None +def superseded_prs(open_prs, area, cursor_sha, keep_branch, owners): + """Open reports for `area` that this run's report replaces: `[(row, reason)]`. + + Two things put a report beyond saving, and neither closes itself. + + **Its window no longer starts at the cursor.** The merge gate requires a byte-exact append at + the cursor recorded in `PROGRESS.md`, so a report whose `from_sha` is not that cursor can never + satisfy it -- it is dead by construction, not merely behind. This is what happens the moment a + sibling report for the same area merges: the cursor advances and every other open report for + that area is orphaned instantly. Nothing noticed, so they accumulated. + + **It starts at the cursor but is older than ours.** Same starting point, and ours ends at the + documented commit, so ours covers a superset of its window and nothing is lost by closing it. + Ordering by creation date rather than by comparing `to_sha` keeps this decidable from the branch + name alone, and makes the newest report win deterministically: two workers racing cannot each + decide the other is superseded and close it, which is the one way this could eat real work. + + Scoped to reports we could have opened, like every other judgement in this module. Branch names + are a pure function of the window, so a stranger can create one; closing theirs would let this + operator silently veto someone else's contribution. + """ + out = [] + for row in open_prs: + parts = (row.get("headRefName") or "").split("/") + if len(parts) != 3 or parts[-1] != area: + continue + if row.get("headRefName") == keep_branch: + continue + if ((row.get("headRepositoryOwner") or {}).get("login") or "") not in owners: + continue + window_part = parts[1] + from7 = window_part.split("-")[0] if "-" in window_part else "" + if not from7: + continue + if not cursor_sha.startswith(from7): + out.append((row, f"its window starts at {from7}, but the {area} cursor is now " + f"{cursor_sha[:7]}, so it can never append")) + else: + out.append((row, f"superseded by a report over the same window start {from7}, " + f"extended to the current documented commit")) + return out + + +def close_superseded(rows, replacement_url): + """Close each superseded report, saying what replaced it. Failures are reported, never fatal. + + This runs only after the replacement is open, so a failure here leaves a tidy-up undone rather + than a window unpublished -- the next run sees the same rows and tries again. + """ + for row, reason in rows: + note = f"Superseded by {replacement_url}: {reason}." + try: + gh.gh(["pr", "close", str(row["number"]), "--repo", gh.ROADMAP_REPO, + "--comment", note]) + print(f"closed superseded #{row['number']}: {reason}") + except Exception as exc: # noqa: BLE001 + print(f"could not close superseded #{row['number']} ({type(exc).__name__}: {exc}); " + f"leaving it open") + + def remote_branch_exists(roadmap_dir, branch, remote="origin"): proc = _run(["git", "ls-remote", "--exit-code", "--heads", remote, branch], roadmap_dir, check=False) @@ -337,4 +402,23 @@ def run(plan, status_body_file, section_body_file, roadmap_dir, dry_run=False, v "--title", title, "--body", body, ]) print(out.strip()) + + # Only now that ours is open: sweep the area's dead and superseded reports. Ordered this way so + # a failure here costs a tidy-up, never a publication -- and so we never close the only open + # report for an area. + # + # Without this the repository accumulates one unmergeable report per area per round, from two + # directions: a sibling merging orphans every other report for that area instantly, and any + # repository-wide breakage (a red `main` fails every report's `build`) makes each round open a + # fresh one that also cannot merge, since the planner's staleness expiry stops the previous one + # marking the area in flight. Neither ever closes itself. See `superseded_prs`. + try: + rows = superseded_prs( + gh.open_progress_prs(), plan["roadmap"], plan["from_sha"], branch, + {gh.ROADMAP_REPO.split("/")[0], _own_login()}, + ) + close_superseded(rows, out.strip().splitlines()[-1] if out.strip() else "the new report") + except Exception as exc: # noqa: BLE001 + print(f"could not sweep superseded reports ({type(exc).__name__}: {exc}); " + f"the new report is open regardless") return 0 diff --git a/tests/test_supersede.py b/tests/test_supersede.py new file mode 100644 index 0000000..89a7a93 --- /dev/null +++ b/tests/test_supersede.py @@ -0,0 +1,127 @@ +"""Tests for superseding: an area's unmergeable reports are closed once a live one is open. + +Two things strand a report, and before this neither closed itself, so they accumulated one per area +per round: a sibling merging moves the cursor and orphans every other open report for that area, and +a repository-wide breakage makes every round open a fresh report that also cannot merge. +""" + +import pathlib +import sys + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parents[1])) + +from progress import apply # noqa: E402 + +failures = [] + + +def check(name, fn): + try: + fn() + except Exception as exc: # noqa: BLE001 + failures.append(name) + print(f"FAIL {name}: {type(exc).__name__}: {exc}") + else: + print(f"ok {name}") + + +OURS = "TauCetiProject" +OWNERS = {OURS} +CURSOR = "37aec57229a4a5828884027165b25804aac01ac8" + + +def pr(number, area="ModularForms", from7="37aec57", to7="787733a", owner=OURS): + return {"number": number, "url": f"https://example.invalid/{number}", + "headRefName": f"progress/{from7}-{to7}/{area}", + "headRepositoryOwner": {"login": owner}} + + +def branch(from7="37aec57", to7="28e93d9", area="ModularForms"): + return f"progress/{from7}-{to7}/{area}" + + +def test_an_orphan_whose_window_predates_the_cursor_is_closed(): + rows = apply.superseded_prs([pr(372, from7="b218626", to7="0038168")], + "ModularForms", CURSOR, branch(), OWNERS) + assert [r["number"] for r, _ in rows] == [372] + assert "can never append" in rows[0][1] + + +def test_a_narrower_report_at_the_same_cursor_is_closed(): + rows = apply.superseded_prs([pr(394, to7="787733a")], + "ModularForms", CURSOR, branch(), OWNERS) + assert [r["number"] for r, _ in rows] == [394] + assert "superseded" in rows[0][1] + + +def test_our_own_branch_is_never_closed(): + keep = branch() + rows = apply.superseded_prs([pr(399, to7="28e93d9")], "ModularForms", CURSOR, keep, OWNERS) + assert rows == [] + + +def test_another_area_is_untouched(): + rows = apply.superseded_prs([pr(396, area="ArithmeticDirichletSeries", from7="8745177")], + "ModularForms", CURSOR, branch(), OWNERS) + assert rows == [] + + +def test_a_strangers_report_is_never_closed(): + """Branch names are a pure function of the window, so anyone can create one. Closing a + stranger's would let this operator silently veto someone else's contribution.""" + rows = apply.superseded_prs([pr(500, from7="b218626", owner="a-stranger")], + "ModularForms", CURSOR, branch(), OWNERS) + assert rows == [] + + +def test_a_malformed_branch_is_ignored(): + bad = {"number": 9, "headRefName": "progress/ModularForms", + "headRepositoryOwner": {"login": OURS}} + worse = {"number": 10, "headRefName": "progress/nowindow/ModularForms", + "headRepositoryOwner": {"login": OURS}} + assert apply.superseded_prs([bad, worse], "ModularForms", CURSOR, branch(), OWNERS) == [] + + +def test_the_whole_modular_forms_pileup_is_swept(): + """The real case: two orphans from the old cursor plus one narrower report at the current one.""" + rows = apply.superseded_prs( + [pr(372, from7="b218626", to7="0038168"), pr(381, from7="b218626", to7="0430506"), + pr(394, to7="787733a"), pr(399, to7="28e93d9")], + "ModularForms", CURSOR, branch(to7="28e93d9"), OWNERS) + assert sorted(r["number"] for r, _ in rows) == [372, 381, 394] + + +def test_closing_reports_the_replacement_and_the_reason(): + calls = [] + orig = apply.gh.gh + apply.gh.gh = lambda args, **kw: calls.append(args) or "" + try: + apply.close_superseded([(pr(372), "because")], "https://example.invalid/399") + finally: + apply.gh.gh = orig + assert calls and calls[0][:3] == ["pr", "close", "372"] + note = calls[0][calls[0].index("--comment") + 1] + assert "https://example.invalid/399" in note and "because" in note + + +def test_a_failed_close_is_not_fatal(): + """A tidy-up failure must never cost a publication: the report is already open by then.""" + def boom(args, **kw): + raise RuntimeError("gh exploded") + orig = apply.gh.gh + apply.gh.gh = boom + try: + apply.close_superseded([(pr(372), "because")], "https://example.invalid/399") + finally: + apply.gh.gh = orig + + +for _name, _fn in sorted(globals().items()): + if _name.startswith("test_") and callable(_fn): + check(_name, _fn) + +print() +if failures: + print(f"{len(failures)} failure(s): {', '.join(failures)}") + sys.exit(1) +print("all tests passed") From fcdbb88288d7db77ce459ce6e4b0859d2d362643 Mon Sep 17 00:00:00 2001 From: Kim Morrison Date: Thu, 17 Sep 2026 10:12:42 +1000 Subject: [PATCH 2/3] fix: decide "older" by pull request number, not by a comment The first version described creation-order arbitration in its docstring and then never read a timestamp, so every report at the same cursor was closed unconditionally and two workers racing would each close the other -- the one outcome the ordering existed to prevent. Order by pull request number instead, which GitHub allocates monotonically per repository. It is a total order with no clock in it, so neither a shared timestamp nor a clock running backwards can invert it, and exactly one direction of a racing pair ever fires. A report numbered above ours supersedes us and is left for its own sweep. When our own number cannot be read, no same-cursor report is closed at all: asserting an order we cannot establish risks discarding a live report, which is far worse than leaving a duplicate for the next round. Orphans are unaffected either way, since being dead is a property of the report alone. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JedNDof6uYKixz6MvtZzCX --- progress/apply.py | 48 ++++++++++++++++++++----- tests/test_supersede.py | 77 +++++++++++++++++++++++++++++++++-------- 2 files changed, 102 insertions(+), 23 deletions(-) diff --git a/progress/apply.py b/progress/apply.py index 03f0c3f..d82fbff 100644 --- a/progress/apply.py +++ b/progress/apply.py @@ -148,7 +148,7 @@ def existing_pr(branch, repo=gh.ROADMAP_REPO, owner=None, states=("merged",)): return rows[0] if rows else None -def superseded_prs(open_prs, area, cursor_sha, keep_branch, owners): +def superseded_prs(open_prs, area, cursor_sha, keep_branch, owners, our_number=None): """Open reports for `area` that this run's report replaces: `[(row, reason)]`. Two things put a report beyond saving, and neither closes itself. @@ -157,13 +157,23 @@ def superseded_prs(open_prs, area, cursor_sha, keep_branch, owners): the cursor recorded in `PROGRESS.md`, so a report whose `from_sha` is not that cursor can never satisfy it -- it is dead by construction, not merely behind. This is what happens the moment a sibling report for the same area merges: the cursor advances and every other open report for - that area is orphaned instantly. Nothing noticed, so they accumulated. + that area is orphaned instantly. Nothing noticed, so they accumulated. Being dead is a property + of the report alone, so this case needs no comparison with ours and applies whatever `our_number` + is. **It starts at the cursor but is older than ours.** Same starting point, and ours ends at the documented commit, so ours covers a superset of its window and nothing is lost by closing it. - Ordering by creation date rather than by comparing `to_sha` keeps this decidable from the branch - name alone, and makes the newest report win deterministically: two workers racing cannot each - decide the other is superseded and close it, which is the one way this could eat real work. + + "Older" is decided by PULL REQUEST NUMBER, which GitHub allocates monotonically per repository. + A timestamp would not do: two workers can stamp the same second, and a clock that runs backwards + would invert the order. The number gives a total order with no clock in it, so the higher-numbered + report always wins and two workers racing can never each conclude the other is superseded and + close it -- which is the one way this could eat real work. A sibling numbered above ours therefore + supersedes US, and is left alone for its own sweep to reconcile. + + Without a readable `our_number` no same-cursor report is closed at all. Closing one then would be + asserting an order we cannot establish, and the cost of being wrong (discarding a live report) is + far worse than the cost of leaving a duplicate for the next round. Scoped to reports we could have opened, like every other judgement in this module. Branch names are a pure function of the window, so a stranger can create one; closing theirs would let this @@ -185,12 +195,33 @@ def superseded_prs(open_prs, area, cursor_sha, keep_branch, owners): if not cursor_sha.startswith(from7): out.append((row, f"its window starts at {from7}, but the {area} cursor is now " f"{cursor_sha[:7]}, so it can never append")) - else: - out.append((row, f"superseded by a report over the same window start {from7}, " - f"extended to the current documented commit")) + continue + if our_number is None: + continue + try: + number = int(row.get("number")) + except (TypeError, ValueError): + continue + if number < int(our_number): + out.append((row, f"superseded by #{our_number}, which starts at the same cursor " + f"{from7} and runs to the current documented commit")) return out +def pr_number_from_url(text): + """The pull request number in `gh pr create`'s output, or None if it cannot be read. + + `gh` prints the URL of the pull request it made, so the trailing path segment is the number. + Returning None rather than guessing matters: the caller treats an unknown number as "cannot + establish an order" and closes nothing on that basis. + """ + for line in reversed((text or "").strip().splitlines()): + tail = line.strip().rstrip("/").rsplit("/", 1)[-1] + if tail.isdigit(): + return int(tail) + return None + + def close_superseded(rows, replacement_url): """Close each superseded report, saying what replaced it. Failures are reported, never fatal. @@ -416,6 +447,7 @@ def run(plan, status_body_file, section_body_file, roadmap_dir, dry_run=False, v rows = superseded_prs( gh.open_progress_prs(), plan["roadmap"], plan["from_sha"], branch, {gh.ROADMAP_REPO.split("/")[0], _own_login()}, + our_number=pr_number_from_url(out), ) close_superseded(rows, out.strip().splitlines()[-1] if out.strip() else "the new report") except Exception as exc: # noqa: BLE001 diff --git a/tests/test_supersede.py b/tests/test_supersede.py index 89a7a93..cf3b1f4 100644 --- a/tests/test_supersede.py +++ b/tests/test_supersede.py @@ -30,6 +30,9 @@ def check(name, fn): CURSOR = "37aec57229a4a5828884027165b25804aac01ac8" +OURS_NUMBER = 400 + + def pr(number, area="ModularForms", from7="37aec57", to7="787733a", owner=OURS): return {"number": number, "url": f"https://example.invalid/{number}", "headRefName": f"progress/{from7}-{to7}/{area}", @@ -40,38 +43,69 @@ def branch(from7="37aec57", to7="28e93d9", area="ModularForms"): return f"progress/{from7}-{to7}/{area}" +def sweep(rows, cursor=CURSOR, keep=None, owners=OWNERS, our_number=OURS_NUMBER): + return apply.superseded_prs(rows, "ModularForms", cursor, keep or branch(), owners, + our_number=our_number) + + def test_an_orphan_whose_window_predates_the_cursor_is_closed(): - rows = apply.superseded_prs([pr(372, from7="b218626", to7="0038168")], - "ModularForms", CURSOR, branch(), OWNERS) + rows = sweep([pr(372, from7="b218626", to7="0038168")]) assert [r["number"] for r, _ in rows] == [372] assert "can never append" in rows[0][1] def test_a_narrower_report_at_the_same_cursor_is_closed(): - rows = apply.superseded_prs([pr(394, to7="787733a")], - "ModularForms", CURSOR, branch(), OWNERS) + rows = sweep([pr(394, to7="787733a")]) assert [r["number"] for r, _ in rows] == [394] assert "superseded" in rows[0][1] +def test_a_newer_report_at_the_same_cursor_is_KEPT(): + """It supersedes US, not the other way round; its own sweep reconciles ours.""" + assert sweep([pr(OURS_NUMBER + 1, to7="ffffff0")]) == [] + + +def test_two_racing_workers_cannot_close_each_other(): + """The one way this could eat real work. PR numbers are a total order with no clock in them, so + exactly one direction of the pair ever fires.""" + a, b = pr(410, to7="aaaaaaa"), pr(411, to7="bbbbbbb") + a_closes = sweep([b], keep=branch(to7="aaaaaaa"), our_number=410) + b_closes = sweep([a], keep=branch(to7="bbbbbbb"), our_number=411) + assert [r["number"] for r, _ in a_closes] == [] + assert [r["number"] for r, _ in b_closes] == [410] + + +def test_an_unreadable_own_number_closes_no_same_cursor_report(): + """Closing then would assert an order we cannot establish. A stale duplicate is much cheaper + than discarding a live report.""" + assert sweep([pr(394, to7="787733a")], our_number=None) == [] + + +def test_an_unreadable_own_number_still_closes_orphans(): + """Being dead is a property of the report alone, so it needs no comparison with ours.""" + rows = sweep([pr(372, from7="b218626", to7="0038168")], our_number=None) + assert [r["number"] for r, _ in rows] == [372] + + +def test_a_non_numeric_number_is_left_alone(): + row = pr(394, to7="787733a") + row["number"] = "not-a-number" + assert sweep([row]) == [] + + def test_our_own_branch_is_never_closed(): keep = branch() - rows = apply.superseded_prs([pr(399, to7="28e93d9")], "ModularForms", CURSOR, keep, OWNERS) - assert rows == [] + assert sweep([pr(399, to7="28e93d9")], keep=keep) == [] def test_another_area_is_untouched(): - rows = apply.superseded_prs([pr(396, area="ArithmeticDirichletSeries", from7="8745177")], - "ModularForms", CURSOR, branch(), OWNERS) - assert rows == [] + assert sweep([pr(396, area="ArithmeticDirichletSeries", from7="8745177")]) == [] def test_a_strangers_report_is_never_closed(): """Branch names are a pure function of the window, so anyone can create one. Closing a stranger's would let this operator silently veto someone else's contribution.""" - rows = apply.superseded_prs([pr(500, from7="b218626", owner="a-stranger")], - "ModularForms", CURSOR, branch(), OWNERS) - assert rows == [] + assert sweep([pr(500, from7="b218626", owner="a-stranger")]) == [] def test_a_malformed_branch_is_ignored(): @@ -79,18 +113,31 @@ def test_a_malformed_branch_is_ignored(): "headRepositoryOwner": {"login": OURS}} worse = {"number": 10, "headRefName": "progress/nowindow/ModularForms", "headRepositoryOwner": {"login": OURS}} - assert apply.superseded_prs([bad, worse], "ModularForms", CURSOR, branch(), OWNERS) == [] + assert sweep([bad, worse]) == [] def test_the_whole_modular_forms_pileup_is_swept(): """The real case: two orphans from the old cursor plus one narrower report at the current one.""" - rows = apply.superseded_prs( + rows = sweep( [pr(372, from7="b218626", to7="0038168"), pr(381, from7="b218626", to7="0430506"), pr(394, to7="787733a"), pr(399, to7="28e93d9")], - "ModularForms", CURSOR, branch(to7="28e93d9"), OWNERS) + keep=branch(to7="28e93d9")) assert sorted(r["number"] for r, _ in rows) == [372, 381, 394] +def test_the_pr_number_is_read_from_the_create_output(): + assert apply.pr_number_from_url("https://github.com/o/r/pull/403") == 403 + assert apply.pr_number_from_url("noise\nhttps://github.com/o/r/pull/403\n") == 403 + assert apply.pr_number_from_url("https://github.com/o/r/pull/403/") == 403 + + +def test_an_unparseable_create_output_yields_no_number(): + """Which the sweep then treats as "cannot establish an order".""" + assert apply.pr_number_from_url("") is None + assert apply.pr_number_from_url("something went sideways") is None + assert apply.pr_number_from_url(None) is None + + def test_closing_reports_the_replacement_and_the_reason(): calls = [] orig = apply.gh.gh From 629415c9f4685b37c09ce4f0c1ae058a8738f576 Mon Sep 17 00:00:00 2001 From: Kim Morrison Date: Thu, 17 Sep 2026 10:20:11 +1000 Subject: [PATCH 3/3] fix: prove containment before closing, and retry a failed sweep A review found three ways the first version could destroy work or quietly stop working. A worker whose plan predates a sibling's merge swept on ITS cursor, not the live one, so it read the one report that could still merge as an orphan and closed it, keeping its own dead report. The live cursor is now read from the roadmap repository at sweep time, and a run whose planned cursor no longer matches it sweeps nothing: it is the stale one. Ordering by creation, or by pull request number, never established what it claimed. A worker that plans early and stalls opens a NARROW report late, so "newest wins" closes a wider report and loses coverage already written. Neither a timestamp nor a number says which window contains which. Close a same-cursor report only on proof: identical `from_sha`, and its window's pull requests a proper subset of ours, both read from the metadata a report already carries in its body. Containment is antisymmetric, so racing workers still cannot close each other, and an incomparable or identical window is kept. The body is mutable, so it is only ever used to prove a report covers LESS; a forged wider claim merely spares it. A failed sweep was never retried. The next run for the same window stops at the in-flight check long before reaching the sweep, so a cleanup outage left the pile-up forever while every run reported success. The sweep now runs on that path too, and only `gh` failures are caught, so a parsing or programming error surfaces instead of reading as a clean sweep. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JedNDof6uYKixz6MvtZzCX --- progress/apply.py | 163 +++++++++++++++++++++++++--------------- progress/gh.py | 20 ++++- tests/test_supersede.py | 149 +++++++++++++++++++++--------------- 3 files changed, 211 insertions(+), 121 deletions(-) diff --git a/progress/apply.py b/progress/apply.py index d82fbff..2b2fbe3 100644 --- a/progress/apply.py +++ b/progress/apply.py @@ -148,37 +148,70 @@ def existing_pr(branch, repo=gh.ROADMAP_REPO, owner=None, states=("merged",)): return rows[0] if rows else None -def superseded_prs(open_prs, area, cursor_sha, keep_branch, owners, our_number=None): +def report_meta(body): + """The `tauceti-progress-pr:v1` metadata a report carries in its body, or None. + + `pr_body` writes `from_sha`, `to_sha` and the window's PR numbers there, so a sibling states its + own window exactly rather than in the seven-character abbreviations of a branch name. That is + what makes a containment test possible without a TauCeti clone. + + The body is mutable and anyone may edit it, so nothing here is trusted for a decision that could + destroy work: the caller only ever uses it to prove that a report covers LESS than ours, and a + forged wider claim merely spares a report from being closed. + """ + marker = BODY_MARKER.split("{}")[0] + for line in (body or "").splitlines(): + line = line.strip() + if not line.startswith(marker): + continue + blob = line[len(marker):] + blob = blob[:blob.rfind("-->")] if "-->" in blob else blob + try: + meta = json.loads(blob) + except json.JSONDecodeError: + return None + return meta if isinstance(meta, dict) else None + return None + + +def superseded_prs(open_prs, area, live_cursor, plan_cursor, keep_branch, owners, our_prs=None): """Open reports for `area` that this run's report replaces: `[(row, reason)]`. - Two things put a report beyond saving, and neither closes itself. + **Nothing is swept unless we are the live report.** `live_cursor` is read from the roadmap + repository now; `plan_cursor` is where the cursor was when this run planned. If they differ, a + sibling landed in between and it is OUR report that is dead, not anyone else's. Sweeping then + inverts the rule below and closes the one report that can still merge, so the answer is to sweep + nothing and let the next round reconcile. - **Its window no longer starts at the cursor.** The merge gate requires a byte-exact append at - the cursor recorded in `PROGRESS.md`, so a report whose `from_sha` is not that cursor can never - satisfy it -- it is dead by construction, not merely behind. This is what happens the moment a - sibling report for the same area merges: the cursor advances and every other open report for - that area is orphaned instantly. Nothing noticed, so they accumulated. Being dead is a property - of the report alone, so this case needs no comparison with ours and applies whatever `our_number` - is. + Given that, two things put a report beyond saving, and neither closes itself. - **It starts at the cursor but is older than ours.** Same starting point, and ours ends at the - documented commit, so ours covers a superset of its window and nothing is lost by closing it. + **Its window no longer starts at the cursor.** The merge gate requires a byte-exact append at the + cursor recorded in `PROGRESS.md`, so a report whose `from_sha` is not that cursor can never + satisfy it -- dead by construction, not merely behind. This is what happens the moment a sibling + for the same area merges: the cursor advances and every other open report is orphaned instantly. + Nothing noticed, so they accumulated. - "Older" is decided by PULL REQUEST NUMBER, which GitHub allocates monotonically per repository. - A timestamp would not do: two workers can stamp the same second, and a clock that runs backwards - would invert the order. The number gives a total order with no clock in it, so the higher-numbered - report always wins and two workers racing can never each conclude the other is superseded and - close it -- which is the one way this could eat real work. A sibling numbered above ours therefore - supersedes US, and is left alone for its own sweep to reconcile. + **It starts at the cursor and we cover everything it covers.** Proved, not inferred: same + `from_sha`, and its window's PR numbers are a proper subset of ours. Creation order will not do + this job. A worker that planned early and stalled creates a NARROW report late, so "newest wins" + would close a wider report and lose coverage that had already been written; and neither a + timestamp nor a pull request number says anything about which window contains which. Containment + is antisymmetric, so two workers racing cannot each conclude the other is dominated, and the + stalled-worker case resolves correctly whichever of them opens first. - Without a readable `our_number` no same-cursor report is closed at all. Closing one then would be - asserting an order we cannot establish, and the cost of being wrong (discarding a live report) is - far worse than the cost of leaving a duplicate for the next round. + A sibling whose window is incomparable to ours is left alone. So is every report, if `our_prs` is + unknown: closing then would assert a containment we have not established, and a duplicate left + for the next round is far cheaper than a report discarded. Scoped to reports we could have opened, like every other judgement in this module. Branch names are a pure function of the window, so a stranger can create one; closing theirs would let this - operator silently veto someone else's contribution. + operator silently veto someone else's contribution. The cost is real and worth naming: reports + published from another operator's fork are never swept, so this does not by itself eliminate a + multi-operator pile-up. """ + if not live_cursor or not plan_cursor or live_cursor != plan_cursor: + return [] + ours = set(our_prs) if our_prs is not None else None out = [] for row in open_prs: parts = (row.get("headRefName") or "").split("/") @@ -188,55 +221,67 @@ def superseded_prs(open_prs, area, cursor_sha, keep_branch, owners, our_number=N continue if ((row.get("headRepositoryOwner") or {}).get("login") or "") not in owners: continue - window_part = parts[1] - from7 = window_part.split("-")[0] if "-" in window_part else "" + from7 = parts[1].split("-")[0] if "-" in parts[1] else "" if not from7: continue - if not cursor_sha.startswith(from7): + if not live_cursor.startswith(from7): out.append((row, f"its window starts at {from7}, but the {area} cursor is now " - f"{cursor_sha[:7]}, so it can never append")) + f"{live_cursor[:7]}, so it can never append")) + continue + if ours is None: continue - if our_number is None: + meta = report_meta(row.get("body")) + if not meta or meta.get("from_sha") != live_cursor: continue try: - number = int(row.get("number")) + theirs = {int(n) for n in (meta.get("prs") or [])} except (TypeError, ValueError): continue - if number < int(our_number): - out.append((row, f"superseded by #{our_number}, which starts at the same cursor " - f"{from7} and runs to the current documented commit")) + if theirs < ours: + out.append((row, f"its window covers {len(theirs)} of the {len(ours)} pull requests " + f"this one reports, from the same cursor {from7}")) return out -def pr_number_from_url(text): - """The pull request number in `gh pr create`'s output, or None if it cannot be read. - - `gh` prints the URL of the pull request it made, so the trailing path segment is the number. - Returning None rather than guessing matters: the caller treats an unknown number as "cannot - establish an order" and closes nothing on that basis. - """ - for line in reversed((text or "").strip().splitlines()): - tail = line.strip().rstrip("/").rsplit("/", 1)[-1] - if tail.isdigit(): - return int(tail) - return None - - def close_superseded(rows, replacement_url): - """Close each superseded report, saying what replaced it. Failures are reported, never fatal. + """Close each superseded report, saying what replaced it. Returns the number that failed. - This runs only after the replacement is open, so a failure here leaves a tidy-up undone rather - than a window unpublished -- the next run sees the same rows and tries again. + A failure here is never fatal -- this only runs once the replacement is open, so the window is + published either way -- but it is returned rather than swallowed, because the caller has to keep + retrying. `sweep` is what makes that retry actually happen. """ + failed = 0 for row, reason in rows: note = f"Superseded by {replacement_url}: {reason}." try: gh.gh(["pr", "close", str(row["number"]), "--repo", gh.ROADMAP_REPO, "--comment", note]) print(f"closed superseded #{row['number']}: {reason}") - except Exception as exc: # noqa: BLE001 - print(f"could not close superseded #{row['number']} ({type(exc).__name__}: {exc}); " - f"leaving it open") + except gh.GhError as exc: + failed += 1 + print(f"could not close superseded #{row['number']} ({exc}); leaving it open") + return failed + + +def sweep(plan, keep_branch, replacement_url): + """Close the area's stranded reports. Safe to call whenever our report is open. + + Idempotent and deliberately reachable from BOTH paths in `run` -- the one that has just opened + the report and the one that finds it already open. That is not tidiness: a sweep that only ran + on the create path would never be retried at all, because the next run for the same window stops + at the in-flight check long before reaching it. A cleanup outage would then leave the pile-up + forever while every run reported success. + + Only `gh` failures are caught. A parsing or programming error here is a bug and must surface. + """ + live = files.cursor(gh.file_on_default_branch(plan["progress_path"]) or "") or "" + rows = superseded_prs( + gh.open_progress_prs(), plan["roadmap"], live, plan["from_sha"], keep_branch, + {gh.ROADMAP_REPO.split("/")[0], _own_login()}, our_prs=plan.get("prs"), + ) + if not rows: + return 0 + return close_superseded(rows, replacement_url) def remote_branch_exists(roadmap_dir, branch, remote="origin"): @@ -344,6 +389,12 @@ def run(plan, status_body_file, section_body_file, roadmap_dir, dry_run=False, v open_pr = own_pr(branch, states=("open",)) if open_pr is not None: print(f"already open, in flight: {open_pr['url']}") + # Sweep here too. This is the path every run after the first takes, so it is the only one + # that can retry a sweep an earlier run failed or never reached. + try: + sweep(plan, branch, open_pr.get("url") or "the open report") + except gh.GhError as exc: + print(f"could not sweep superseded reports ({exc}); the report is open regardless") return EX_NOPROGRESS # A CLOSED pull request means this window was refused, and reopening it every day is the loop @@ -444,13 +495,7 @@ def run(plan, status_body_file, section_body_file, roadmap_dir, dry_run=False, v # fresh one that also cannot merge, since the planner's staleness expiry stops the previous one # marking the area in flight. Neither ever closes itself. See `superseded_prs`. try: - rows = superseded_prs( - gh.open_progress_prs(), plan["roadmap"], plan["from_sha"], branch, - {gh.ROADMAP_REPO.split("/")[0], _own_login()}, - our_number=pr_number_from_url(out), - ) - close_superseded(rows, out.strip().splitlines()[-1] if out.strip() else "the new report") - except Exception as exc: # noqa: BLE001 - print(f"could not sweep superseded reports ({type(exc).__name__}: {exc}); " - f"the new report is open regardless") + sweep(plan, branch, out.strip().splitlines()[-1] if out.strip() else "the new report") + except gh.GhError as exc: + print(f"could not sweep superseded reports ({exc}); the new report is open regardless") return 0 diff --git a/progress/gh.py b/progress/gh.py index 58eab1c..0c2a33f 100644 --- a/progress/gh.py +++ b/progress/gh.py @@ -13,6 +13,7 @@ right now". """ +import base64 import json import subprocess import time @@ -140,7 +141,24 @@ def open_progress_prs(repo=ROADMAP_REPO, branch_prefix="progress/"): out = gh([ "pr", "list", "--repo", repo, "--state", "open", "--limit", "200", "--json", - "number,headRefName,title,url,createdAt,headRepositoryOwner", + "number,headRefName,title,url,createdAt,headRepositoryOwner,body", ]) rows = json.loads(out) return [r for r in rows if (r.get("headRefName") or "").startswith(branch_prefix)] + + +def file_on_default_branch(path, repo=ROADMAP_REPO, ref="main"): + """The text of `path` on `ref`, or None if it is not there. + + Read through the API rather than from a clone on purpose. A worker's checkout is a snapshot from + whenever it started, and the questions this answers -- where is the cursor NOW, is this report + still the live one -- are exactly the ones a stale snapshot gets wrong. + """ + try: + raw = gh(["api", f"repos/{repo}/contents/{path}?ref={ref}", "--jq", ".content"]) + except GhError: + return None + try: + return base64.b64decode("".join(raw.split())).decode("utf-8", "surrogateescape") + except (ValueError, UnicodeDecodeError): + return None diff --git a/tests/test_supersede.py b/tests/test_supersede.py index cf3b1f4..3ca55e4 100644 --- a/tests/test_supersede.py +++ b/tests/test_supersede.py @@ -5,6 +5,7 @@ a repository-wide breakage makes every round open a fresh report that also cannot merge. """ +import json import pathlib import sys @@ -30,72 +31,103 @@ def check(name, fn): CURSOR = "37aec57229a4a5828884027165b25804aac01ac8" -OURS_NUMBER = 400 +OLD_CURSOR = "b21862652ed4e0e0cb29351a0e78338370755d0a" +OUR_PRS = [6328, 6334, 6374, 6409] -def pr(number, area="ModularForms", from7="37aec57", to7="787733a", owner=OURS): +def body(from_sha=CURSOR, to_sha="787733a", prs=(6328, 6334)): + meta = {"roadmap": "ModularForms", "from_sha": from_sha, "to_sha": to_sha, + "prs": sorted(prs), "version": "v1"} + return apply.BODY_MARKER.replace("{}", json.dumps(meta, sort_keys=True, + separators=(",", ":"))) + "\nprose\n" + + +def pr(number, area="ModularForms", from7="37aec57", to7="787733a", owner=OURS, body_text=None, + prs=(6328, 6334), meta_from=None): return {"number": number, "url": f"https://example.invalid/{number}", "headRefName": f"progress/{from7}-{to7}/{area}", - "headRepositoryOwner": {"login": owner}} + "headRepositoryOwner": {"login": owner}, + "body": body_text if body_text is not None + else body(from_sha=meta_from or CURSOR, to_sha=to7, prs=prs)} def branch(from7="37aec57", to7="28e93d9", area="ModularForms"): return f"progress/{from7}-{to7}/{area}" -def sweep(rows, cursor=CURSOR, keep=None, owners=OWNERS, our_number=OURS_NUMBER): - return apply.superseded_prs(rows, "ModularForms", cursor, keep or branch(), owners, - our_number=our_number) +def sweep(rows, live=CURSOR, planned=CURSOR, keep=None, owners=OWNERS, our_prs=OUR_PRS): + return apply.superseded_prs(rows, "ModularForms", live, planned, keep or branch(), owners, + our_prs=our_prs) def test_an_orphan_whose_window_predates_the_cursor_is_closed(): - rows = sweep([pr(372, from7="b218626", to7="0038168")]) + rows = sweep([pr(372, from7="b218626", to7="0038168", meta_from=OLD_CURSOR)]) assert [r["number"] for r, _ in rows] == [372] assert "can never append" in rows[0][1] -def test_a_narrower_report_at_the_same_cursor_is_closed(): - rows = sweep([pr(394, to7="787733a")]) +def test_a_dominated_report_at_the_same_cursor_is_closed(): + rows = sweep([pr(394, prs=(6328, 6334))]) assert [r["number"] for r, _ in rows] == [394] - assert "superseded" in rows[0][1] + assert "covers 2 of the 4" in rows[0][1] + + +def test_a_stale_worker_sweeps_nothing(): + """Codex's case. We planned at C0; a sibling landed and the cursor is now C1, so OUR report is + the dead one. Sweeping on the stale cursor would close the only report that can still merge.""" + live_report = pr(399, from7="37aec57", to7="28e93d9") + assert sweep([live_report], live=CURSOR, planned=OLD_CURSOR) == [] -def test_a_newer_report_at_the_same_cursor_is_KEPT(): - """It supersedes US, not the other way round; its own sweep reconciles ours.""" - assert sweep([pr(OURS_NUMBER + 1, to7="ffffff0")]) == [] +def test_a_wider_report_is_never_closed_by_a_narrower_one(): + """A worker that planned early and stalled opens a NARROW report late. Creation order would + close the wider one and lose coverage already written; containment does not.""" + wider = pr(410, prs=(6328, 6334, 6374, 6409, 6444)) + assert sweep([wider], our_prs=[6328, 6334]) == [] def test_two_racing_workers_cannot_close_each_other(): - """The one way this could eat real work. PR numbers are a total order with no clock in them, so - exactly one direction of the pair ever fires.""" - a, b = pr(410, to7="aaaaaaa"), pr(411, to7="bbbbbbb") - a_closes = sweep([b], keep=branch(to7="aaaaaaa"), our_number=410) - b_closes = sweep([a], keep=branch(to7="bbbbbbb"), our_number=411) + a = pr(410, to7="aaaaaaa", prs=(6328, 6334)) + b = pr(411, to7="bbbbbbb", prs=(6328, 6334, 6374, 6409)) + a_closes = sweep([b], keep=branch(to7="aaaaaaa"), our_prs=[6328, 6334]) + b_closes = sweep([a], keep=branch(to7="bbbbbbb"), our_prs=[6328, 6334, 6374, 6409]) assert [r["number"] for r, _ in a_closes] == [] assert [r["number"] for r, _ in b_closes] == [410] -def test_an_unreadable_own_number_closes_no_same_cursor_report(): - """Closing then would assert an order we cannot establish. A stale duplicate is much cheaper - than discarding a live report.""" - assert sweep([pr(394, to7="787733a")], our_number=None) == [] +def test_an_incomparable_window_is_kept(): + """Neither contains the other, so neither supersedes the other.""" + assert sweep([pr(410, prs=(6328, 9999))]) == [] + + +def test_an_identical_window_is_kept(): + """A proper subset, not any subset: an exact duplicate is not dominated, and closing on equality + would let two workers close each other again.""" + assert sweep([pr(410, prs=OUR_PRS)]) == [] + + +def test_unknown_own_window_closes_no_same_cursor_report(): + assert sweep([pr(394, prs=(6328,))], our_prs=None) == [] -def test_an_unreadable_own_number_still_closes_orphans(): - """Being dead is a property of the report alone, so it needs no comparison with ours.""" - rows = sweep([pr(372, from7="b218626", to7="0038168")], our_number=None) +def test_unknown_own_window_still_closes_orphans(): + """Being dead is a property of the report alone.""" + rows = sweep([pr(372, from7="b218626", to7="0038168", meta_from=OLD_CURSOR)], our_prs=None) assert [r["number"] for r, _ in rows] == [372] -def test_a_non_numeric_number_is_left_alone(): - row = pr(394, to7="787733a") - row["number"] = "not-a-number" - assert sweep([row]) == [] +def test_a_report_without_readable_metadata_is_kept(): + assert sweep([pr(394, body_text="no marker here")]) == [] + assert sweep([pr(394, body_text=apply.BODY_MARKER.replace("{}", "not json"))]) == [] + + +def test_metadata_claiming_a_different_cursor_is_kept(): + """The body is mutable, so it is only ever used to prove a report covers LESS.""" + assert sweep([pr(394, meta_from=OLD_CURSOR, prs=(6328,))]) == [] def test_our_own_branch_is_never_closed(): - keep = branch() - assert sweep([pr(399, to7="28e93d9")], keep=keep) == [] + assert sweep([pr(399, to7="28e93d9")], keep=branch(to7="28e93d9")) == [] def test_another_area_is_untouched(): @@ -103,62 +135,57 @@ def test_another_area_is_untouched(): def test_a_strangers_report_is_never_closed(): - """Branch names are a pure function of the window, so anyone can create one. Closing a - stranger's would let this operator silently veto someone else's contribution.""" - assert sweep([pr(500, from7="b218626", owner="a-stranger")]) == [] + assert sweep([pr(500, from7="b218626", owner="a-stranger", meta_from=OLD_CURSOR)]) == [] def test_a_malformed_branch_is_ignored(): bad = {"number": 9, "headRefName": "progress/ModularForms", - "headRepositoryOwner": {"login": OURS}} + "headRepositoryOwner": {"login": OURS}, "body": body()} worse = {"number": 10, "headRefName": "progress/nowindow/ModularForms", - "headRepositoryOwner": {"login": OURS}} + "headRepositoryOwner": {"login": OURS}, "body": body()} assert sweep([bad, worse]) == [] def test_the_whole_modular_forms_pileup_is_swept(): - """The real case: two orphans from the old cursor plus one narrower report at the current one.""" rows = sweep( - [pr(372, from7="b218626", to7="0038168"), pr(381, from7="b218626", to7="0430506"), - pr(394, to7="787733a"), pr(399, to7="28e93d9")], + [pr(372, from7="b218626", to7="0038168", meta_from=OLD_CURSOR), + pr(381, from7="b218626", to7="0430506", meta_from=OLD_CURSOR), + pr(394, prs=(6328, 6334)), + pr(399, to7="28e93d9", prs=OUR_PRS)], keep=branch(to7="28e93d9")) assert sorted(r["number"] for r, _ in rows) == [372, 381, 394] -def test_the_pr_number_is_read_from_the_create_output(): - assert apply.pr_number_from_url("https://github.com/o/r/pull/403") == 403 - assert apply.pr_number_from_url("noise\nhttps://github.com/o/r/pull/403\n") == 403 - assert apply.pr_number_from_url("https://github.com/o/r/pull/403/") == 403 - - -def test_an_unparseable_create_output_yields_no_number(): - """Which the sweep then treats as "cannot establish an order".""" - assert apply.pr_number_from_url("") is None - assert apply.pr_number_from_url("something went sideways") is None - assert apply.pr_number_from_url(None) is None +def test_report_meta_reads_the_window(): + meta = apply.report_meta(body(prs=(1, 2, 3))) + assert meta["from_sha"] == CURSOR and meta["prs"] == [1, 2, 3] + assert apply.report_meta("") is None + assert apply.report_meta(None) is None -def test_closing_reports_the_replacement_and_the_reason(): - calls = [] +def test_a_failed_close_is_counted_not_swallowed(): + """The caller has to know, so it keeps retrying on the in-flight path.""" + def boom(args, **kw): + raise apply.gh.GhError("gh exploded") orig = apply.gh.gh - apply.gh.gh = lambda args, **kw: calls.append(args) or "" + apply.gh.gh = boom try: - apply.close_superseded([(pr(372), "because")], "https://example.invalid/399") + assert apply.close_superseded([(pr(372), "because")], "https://example.invalid/399") == 1 finally: apply.gh.gh = orig - assert calls and calls[0][:3] == ["pr", "close", "372"] - note = calls[0][calls[0].index("--comment") + 1] - assert "https://example.invalid/399" in note and "because" in note -def test_a_failed_close_is_not_fatal(): - """A tidy-up failure must never cost a publication: the report is already open by then.""" +def test_a_programming_error_in_a_close_is_not_hidden(): def boom(args, **kw): - raise RuntimeError("gh exploded") + raise TypeError("a real bug") orig = apply.gh.gh apply.gh.gh = boom try: - apply.close_superseded([(pr(372), "because")], "https://example.invalid/399") + apply.close_superseded([(pr(372), "because")], "url") + except TypeError: + pass + else: + raise AssertionError("a non-gh error must surface") finally: apply.gh.gh = orig