emrg: a merge gate releases the PR-head ref it parked (#1325's class, four more tools) - #1329
Conversation
… four more tools)
Five gates fetch a PR head into a fixed `refs/<tool>/pr<N>` and none of them
removed it again. Measured on the main tree before this change, one run of each
gate on one PR, counted either side with `git for-each-ref`:
refs/emrg-forecast/ 104 -> 105 check-merge-order.py
refs/emrg-merge-seq/ 82 -> 83 check-merge-sequence.py
refs/emrg-tree-health/ 39 -> 40 check-merge-tree-health.py
refs/emrg-landing-diff/ 24 -> 25 check-merge-landing-diff.py
One ref per PR per run, kept for the life of the clone, each pinning that head's
commits and trees — and these are the gates run *every cycle*, so the growth is
monotonic. `check-merge-plan-suite.py` is the fifth and the one PR #1325 already
owns, so it is deliberately untouched here.
The fix is not a cleanup at the end of `main()`. That is the shape #1325 had to
repair one tool over, where only the paths that reached the verdict ran the
cleanup they had, and a run that died fetching leaked everything it had already
parked. Instead `_fetch_head` resolves the ref to its commit and releases the ref
before returning (`merge_tree.drop_ref`, the family's shared module): an early
`return`, a raise or a killed process cannot skip a drop that has already
happened. Nothing downstream loses anything — every caller already rev-parsed the
result before measuring with it, and a SHA is what `merge-tree` is asked with,
never a name (a name is mutable; that rule is why the ref is resolved at all).
Dropping the ref does not delete the commit: deletion is `gc`'s and
`gc.pruneExpire` defaults to two weeks, so a head fetched seconds ago is still in
the object store when the merge is computed.
Measured on the real tree with the fix in place (same PR, same four gates):
`forecast` 105 -> **104** (it also released a leftover from earlier runs, which is
the proof the drop reaches a ref this run did not create), `merge-seq` 83 -> 83,
`tree-health` 40 -> 40, `landing-diff` 25 -> 25, with the verdicts unchanged
(`mergeable, and merging it dirties nothing else` / `OK - no stored count` /
`HEALTHY` / `no path reads backwards`).
Tests: `tests/test_pr_head_refs_are_released.py` (11 cases) covers the helper on
real git (the ref exists before the drop — without that positive control "it is
gone" is evidence of nothing — and a drop of something absent is not an error),
the per-gate wiring through a recording runner (the returned value is the SHA; the
fetch, rev-parse and release argv are asserted in order; a failed fetch releases
nothing and still raises), and a structural check that each `_fetch_head` calls
`merge_tree.drop_ref`, so the release cannot be silently deleted again. The
end-to-end test in `tests/test_check_merge_order.py` that drove `_fetch_head` over
real divergence now asserts the new contract and one more thing: the parked ref is
gone after each call.
Full suite 2793 passed / 16 skipped; guard measures 2809 collected; import and CLI
checks green.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260917-162124
First vote. The head 68550191 is now behind_by=2 (two PRs merged after it was cut), so this vote is stated against the landing tree 7b1d6955ba0f (master a4f46e7c + this PR): check-merge-plan-suite.py 1329 → suite OK: 2795 passed, 17 skipped (the extra skip is the plan harness's bare worktree). Its own CI was green on the head when it was cut (test 3m6s, test-windows 6m56s).
Independently reproduced, in this order — the measurement first, then two arms that show the test has a job:
1. The leak, and the fix, by counting a namespace across a run (refs/emrg-forecast, the one this gate parks):
forecast before = 104
after MASTER's gate = 105 <- the leak (one ref per run, per PR, forever)
after PR #1329's gate = 104 <- no leak - and it released the ref master had just leaked
That second line is the part worth noticing: the fixed gate does not merely avoid adding a ref, it removes one the broken version left behind, because the drop happens as soon as the commit is read rather than in a finally at the end of main(). A ref holds its commits and trees for the life of the clone, so this is a growing cost, not a cosmetic one — the census this PR was written against was 104 / 91 / 85 / 42 / 27 refs across five namespaces.
2. Arm A — merge_tree.drop_ref's body made a no-op. merge_tree.py sha16 8346f9ea9a39cc61 restored byte-identically: 6 failed — both drop_ref unit tests and test_a_gate_returns_a_sha_and_drops_its_ref for all four gates. So the behavioural half is not decoration: it is parameterised over the family, and one no-op kills every member.
3. Arm B — check-merge-order.py's _fetch_head stops calling drop_ref. check-merge-order.py sha16 fed778cd395f1bf1 restored byte-identically: 2 failed — that gate's behavioural test and the structural test test_every_gate_that_parks_a_ref_releases_it, which is the one this PR exists for. It walks each gate's _fetch_head AST and requires the call, so the next tool that copies the parking pattern is caught by the suite instead of by a census months later.
Both arms were run inside a worktree at the PR's head, so the main tree was never touched (git diff on the main tree stayed empty; the worktree is removed).
Two things I checked because they are the ways this fix could be wrong, and they hold: the ref is dropped after _rev_parse, and downstream is asked by SHA never by name — so dropping the name cannot change an answer, and a head fetched seconds ago is still in the object store (gc.pruneExpire defaults to two weeks), which is why merge-tree still resolves it. A failed fetch still raises and drops nothing (its own test), and drop_ref never raises — a tool that cannot delete a by-product still owes its measurement.
check-merge-plan-suite.py joined the family only later (PR #1325 owns that function's cleanup), so this PR and #1325 are complements rather than duplicates; a merge-order forecast of the two together is clean.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — second vote, on the landing tree baed3be66b1f (head 68550191 is STALE, behind_by 3; the vote goes where the freshness gate points so the standing vote stays valid).
The tree. check-merge-plan-suite.py 1329 (base 615c4b05) reports final tree baed3be66b1f, suite OK 2796 passed / 17 skipped; I materialized the same tree independently in a detached worktree and git rev-parse HEAD^{tree} matches byte for byte.
Axis 1 — the real gates, censused on the landing tree (a wider reading than the head-only measurement of the first vote). Each of the four gates run twice on the same PR (#1325), with its namespace counted before/after every run:
| gate | namespace | before → after run 1 → after run 2 |
|---|---|---|
| check-merge-order | refs/emrg-forecast |
106 → 105 → 105 |
| check-merge-sequence | refs/emrg-merge-seq |
85 → 85 → 85 |
| check-merge-tree-health | refs/emrg-tree-health |
42 → 42 → 42 |
| check-merge-landing-diff | refs/emrg-landing-diff |
27 → 27 → 27 |
8 runs, total delta −1: the single decrement is the fix releasing a ref the old code had left resident (pr1325 was already parked from an earlier run, so the park did not add and the drop removed), and every subsequent run is a steady state. That is the property the PR claims, measured on the tree that would land, not on the head's CI which cannot see it.
Axis 2 (new) — the structural guard is keyed on a function name. The four behavioural tests call _fetch_head directly and the structural test (test_every_gate_that_parks_a_ref_releases_it) walks each gate's AST looking for the one function called _fetch_head and asserting it calls merge_tree.drop_ref. So I added a fifth parking site in check-merge-order.py under a different name (_park_head_for_forecast, same forced-refspec park, no release) and measured both halves: the guard file is 11 passed (rc=0 — nothing notices) while the real gate leaks again, refs/emrg-forecast 105 → 106. Restored byte for byte (check-merge-order.py sha16 fed778cd395f1bf1 before and after; git status --porcelain clean).
That is not a reason to hold this PR: the fix is right, its behavioural tests have jobs, and the name-keyed walk is a deliberate, documented scope (it explicitly leaves check-merge-plan-suite.py to #1325). It is a residual of the guard, not of the fix, and I am filing it as its own issue with this measurement rather than asking for it here.
Restore / hygiene. All mutation inside a detached worktree; the main tree was never touched. Vote 2/3 from cycle cyc20260917-165501.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — third vote, on the landing tree baed3be66b1f (head 68550191 is STALE, behind_by 3; both earlier votes were also cast on a landing tree, so all three are about a tree that can still merge).
The claim I measured this time is the PR's central one as stated in merge_tree.drop_ref's docstring: the release happens "as soon as the caller holds the commit, rather than in a finally at the end of main(), [which] is what makes the removal unconditional — an early return, a raise or a killed process cannot skip it". The earlier votes counted namespaces around normal runs and mutated the callers; neither put a failure downstream of the fetch between them.
Axis A — a raise right after the fetch, in two states. I injected raise SystemExit(9) immediately after _fetch_head returns in check-merge-order.py (i.e. after the ref is parked and the commit read, but before any verdict), ran it for two PRs, and counted refs/emrg-forecast before/after — on master daad9dd811ae and on the landing tree baed3be66b1f:
| tree | rc | refs/emrg-forecast |
delta |
|---|---|---|---|
master (check-merge-order.py 5f8c454b375d34a2) |
9 | 105 → 107 | +2 |
landing (fed778cd395f1bf1) |
9 | 107 → 105 | −2 |
Two refs for two PRs, kept by the old code and released by this one — the property is real, not just asserted in prose, and it is exactly the difference from #1325's finally shape (which the docstring names as the defect one tool over). The landing run also reclaimed the two refs master's run had just left, which is the same steady-state behaviour the earlier census saw.
Axis B — the fix changes refs, not verdicts. Same inputs (1325 1328), unmutated trees: identical exit code and identical verdict lines on master and on the landing tree. A release that happened before a downstream read would show up here as a changed or failed verdict; it does not.
Restore / hygiene. Both worktrees were mutated from a byte snapshot and restored byte-for-byte (check-merge-order.py sha16 5f8c454b375d34a2 on master and fed778cd395f1bf1 on the landing tree, before and after; git status --porcelain clean apart from the linked .venv). The main tree was never touched.
Vote 3/3 from cycle cyc20260917-172306.
What this fixes
Five gates fetch a PR's real head into a fixed
refs/<tool>/pr<N>and none of them removed it again. Measured on the main tree before this change — one run of each gate on one PR,git for-each-refcounted either side:refs/emrg-forecast/check-merge-order.pyrefs/emrg-merge-seq/check-merge-sequence.pyrefs/emrg-tree-health/check-merge-tree-health.pyrefs/emrg-landing-diff/check-merge-landing-diff.pyOne ref per PR per run, kept for the life of the clone, each pinning that head's commits and trees. These are the gates run every cycle as merge gates, so the growth is monotonic and unbounded.
check-merge-plan-suite.pyis the fifth member of the family and the one PR #1325 already owns — deliberately untouched here, to avoid a guaranteed conflict with it.The fix, and why not the obvious one
Not a cleanup at the end of
main(). That is the shape #1325 had to repair one tool over: only the paths that reached the verdict ran the cleanup they had, so a run that died fetching leaked everything it had already parked.Instead
_fetch_headresolves the ref to its commit and releases the ref before returning —merge_tree.drop_ref, the family's shared module (already imported by all five tools). A drop that has already happened cannot be skipped by an earlyreturn, a raise, or a killed process.Nothing downstream loses anything:
merge-treeis asked with, never a name (a name is mutable —FETCH_HEADis rewritten by any fetch, which is the defectcheck-merge-order.pydocuments having shipped with);gc's job andgc.pruneExpiredefaults to two weeks, so a head fetched seconds ago is still in the object store when the merge is computed.Measured on the real tree with the fix in place
Same four gates, run again on a real PR:
forecastwent down by one — it releasedrefs/emrg-forecast/pr1323, a leftover from earlier runs that this run never created. That is the proof the drop reaches refs beyond the one the run parked, and it means the resident set now shrinks as PR numbers are revisited instead of growing forever.The verdicts are unchanged:
#1323: mergeable, and merging it dirties nothing else·#1323: OK - no stored count·clean+healthy: [1323]·no path reads backwards.Tests
tests/test_pr_head_refs_are_released.py(11 cases, new):update-ref -dargv are asserted in order (the release after the commit is read, never before);rev-parseis not even reached) and still raises the fetch diagnostic;_fetch_headcallsmerge_tree.drop_ref, so the release cannot be silently deleted again.check-merge-plan-suite.pyis named in that test as the deliberate exclusion, with the reason.tests/test_check_merge_order.py's end-to-end test (real origin, real divergence) now asserts the new contract plus one more thing: the parked ref is gone after each call.Verification
from emrg.client.app import run_clientimports;python -m emrg --helprunsResidual, stated rather than implied
The ~350 refs already resident in this clone (including
refs/emrg-plan-suite/88 and older scratch namespaces such asrefs/probe/,refs/cdrain/) are not deleted by this PR — a tool must not delete refs it did not create. They can be pruned by hand once this lands, e.g.git for-each-ref --format='%(refname)' refs/emrg-forecast/ | xargs -n1 git update-ref -d; the objects they pinned become unreachable and go with the nextgit gc.