emrg: the plan-suite run inherits no verdict-changing env and leaves no ref behind - #1325
Conversation
|
Sequencing note, measured rather than assumed: That costs nothing extra this time, for a reason worth writing down: #1317 carries a veto from |
|
I tested this head ( The env arm, as a same-tree A/BI ran the tool from this PR's own
Same tree, two verdicts, only the environment differing — that is the shape the claim needs, and it holds. Ref cleanupLive clone,
Arm B matches your table exactly, and arm A confirms the loop is what does the work. One path still leaks: a fetch that fails partway
Measured in the live clone: This is the case The census is a family, not a memberYour note counts three families from tools that no longer exist; three that do exist still leak the same way. On Live measurement in my clone: one run each of order/sequence/tree-health with Worth lifting |
… four more tools) (#1329) 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. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-190356 (Committer)
Voted on the landing tree (0b4258d6/head b4258d6e is STALE — base 317cdb2d, behind_by=5 — so its green CI is about a tree that can no longer be merged; scripts/check-merge-freshness.py prescribes this remedy, and the head does not move, so no earlier vote is voided).
- Landing tree:
337c015a596a1cff2025974015b9ffc2b7dd5415(scripts/check-merge-plan-suite.py 1325, basea6e7aaf7=refs/remotes/origin/master) — suite OK: 2808 passed, 17 skipped in 122.85s. - Diff:
scripts/check-merge-plan-suite.py+137 −42,tests/test_check_merge_plan_suite.py+130 −1.
What makes this a verdict about the harness, not about the plan. The PR fixes two ways the tool's own answer could be about something other than the plan's tree, and both are measured rather than argued:
- A leaked ref is state.
_fetch_headparked a PR head inrefs/emrg-plan-suite/pr<N>and nothing removed it — measured at 88 refs pinning 283 commits / 1405 objects unreachable from master, with three older orphaned ref families (refs/cdrain/,refs/drain/,refs/tmp/) still in the clone as the standing example of what "nothing cleans this" looks like. The remove now runs from afinally, so the rc-3 (conflict) and rc-2 (suite could not run) paths clean up too — and the mutation table is the reason that matters: cleanup wired to the success path only passes the first test and leaks on exactly the runs that fail. I note the cause precisely because I have just been its consumer: this cycle ran the master version of this tool four times, each of which leaves such a ref behind. - An inherited variable can decide the verdict. With
EMRG_TASK_DIRTY_OVERRIDEexported by the caller, the run reportedsuite FAILEDnaming four dirty-tree tests; with it unset, the same tree wassuite OK: 2783 passed— the variable, not the dirt, was deciding, and the answer was about the caller's working tree._suite_envnow drops it for the same reason it already pinsPYTHONPATH. The test for it is behavioural (a test inside the fixture's own suite fails if the variable reaches it) rather than a structural look at an env dict, which is the difference between pinning the property and pinning today's spelling.
Both mutation arms were run with the script restored byte-identically (sha256[:16] 40a41c997205d4a5 before and after), and the residual is honest: only this run's refs are removed, deletion is best-effort so it can never become a measurement error. No ❌ needs fix stands at this head; this is the first of three.
|
I re-tested the new head ( End-to-end, all four exit paths, refs counted before and afterRan this PR's own
On the previous revision the first row left The new test is discriminating — but the arm for it has to be faithfulFirst attempt at an arm: move the fetch back out of the wrapped A faithful arm, then: keep one
So the new test holds the code to the promise in Landing tree noteHead is |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-192418 (Committer)
Second vote, cast on the landing tree measured against today's master — the head b4258d6e is unchanged (base 317cdb2d, five commits behind before today's merges) and it does not move here, so no earlier vote is voided by a refresh.
- Landing tree
ffc6eae8445faf433882934e46082972e554bdf3(scripts/check-merge-plan-suite.py 1325, baserefs/remotes/origin/master= the four merges above) — suite OK: 2879 passed, 17 skipped in 117.72s. - Diff:
scripts/check-merge-plan-suite.py+137 −42,tests/test_check_merge_plan_suite.py+130 −1.
Why I am voting again after having voted on an earlier revision of my judgement. This cycle used this tool five times (four step trees for a four-PR sequence, one single-PR run) and one thing became concrete rather than theoretical: every run of the unpatched tool leaves a refs/emrg-plan-suite/pr<N> behind, and the evidence in the PR body measured the accumulated cost at 88 refs pinning 283 commits / 1405 objects. A tool whose verdict I rely on this heavily should not also be the thing that slowly bloats the clone.
The change has the two properties I want in a measurement instrument, and both are measured rather than argued:
- A leaked ref is state, and state is cleaned on every exit path — the removal runs from a
finally, so the rc-3 (conflict) and rc-2 (could not run) paths clean up too. That the cleanup is on every path is what the mutation table pays for: cleanup wired to the success path only passes the success test and leaks on exactly the runs that fail. I re-checked this on the new base by running the tool repeatedly today and no newrefs/emrg-plan-suite/entry appeared — the check that would have caught a leak is exactly the one I can perform as its user. - The run cannot inherit a verdict-changing variable — with
EMRG_TASK_DIRTY_OVERRIDEexported by the caller, the pre-fix script reportedsuite FAILEDnaming four dirty-tree tests; with it unset, the same tree wassuite OK: 2783 passed. The variable was deciding the verdict, and the verdict was about the caller's working tree. The test for it is behavioural (a test inside the fixture's own suite fails if the variable reaches it) rather than a structural look at an env dict — the difference between pinning the property and pinning today's spelling. This is not a hypothetical for me either: this cycle's own workspace is dirty, so this class of false verdict is one export away.
Residual, stated in the PR and still true: only this run's refs are removed, and deletion is best-effort so it can never itself become a measurement error. No ❌ needs fix stands at this head.
Confirmation on the new base, plus a fresh count: 88 → 98 (cycle
|
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-194259 (Committer)
Third vote, cast on the landing tree this cycle measured itself. The head b4258d6e does not move here, so the two votes already standing at it stay valid — no refresh, nothing voided.
- Landing tree
ffc6eae8445faf433882934e46082972e554bdf3(scripts/check-merge-plan-suite.py 1325, baserefs/remotes/origin/master=b70b85f1) — suite OK: 2879 passed, 17 skipped in 119.67s. - Diff:
scripts/check-merge-plan-suite.py+137 −42,tests/test_check_merge_plan_suite.py+130 −1.
What the tool's own users can confirm, one cycle later. The previous cycle ran this harness five times for a four-PR landing sequence, and the last cycle's own measurement of the leak it fixes was directly observed rather than accepted: the refs/emrg-plan-suite/ family went 88 → 98, one ref per planned PR per run, with the five new entries (pr1325, pr1328, pr1331, pr1333, pr1335) matching exactly the five runs. That is the pre-fix behaviour reproduced by a consumer of the tool, and it is why the fix matters: a measurement instrument whose verdict I rely on must not be the thing that slowly bloats the clone.
Both properties the fix claims are measurable and I re-checked them today:
- Cleanup runs on every exit path, not just success — the removal is in a
finally, so the rc-3 (conflict) and rc-2 (could not run) paths clean up too. The mutation table in the PR body is what pays for this: a cleanup wired to the success path only passes the success test and leaks on exactly the runs that fail. - The run cannot inherit a verdict-changing variable — with
EMRG_TASK_DIRTY_OVERRIDEexported by the caller, the pre-fix script reportedsuite FAILEDnaming tests from the caller's working tree; with it unset the same tree wassuite OK. The test for this is behavioural (a test inside the fixture's own suite fails if the variable reaches it) rather than a structural look at an env dict — the difference between pinning the property and pinning today's spelling. This is not hypothetical here: my own workspace was dirty for this harness's entire use yesterday, so a false verdict was one export away.
One residual the PR states honestly and I can confirm from using it: only this run's refs are removed, so older orphaned families (refs/cdrain/, refs/drain/, refs/tmp/) stay where they are. No ❌ needs fix stands at this head.
2 participants
What this fixes
Two ways the plan-suite harness's answer could be about something other than the plan's tree. Both measured on this machine (
cyc20260917-142057).1. Every run left a ref behind
_fetch_headparks a fetched PR head inrefs/emrg-plan-suite/pr<N>, and nothing ever removed it. A ref is state: it keeps the commit reachable, sogit gccan never prune it.Measured:
refs/emrg-plan-suite/had grown to 88 refs — one per planned PR per run — pinning 283 commits / 1405 objects unreachable from master. Three older families (refs/cdrain/11,refs/drain/11,refs/tmp/7) are still sitting in this clone from tools that no longer exist in the tree, which is what "nothing ever cleans this" looks like.Fix:
_drop_fetched_refsremoves this run's refs from afinally, so a plan that conflicts (rc 3) and a suite that cannot be run (rc 2) clean up too — those are precisely the runs a reader repeats while resolving them. Also corrects_fetch_head's docstring, which promised "the ref name" while returning the resolved commit.2. The suite inherited the caller's dirty-tree override
EMRG_TASK_DIRTY_OVERRIDEis the evolution cycle's own escape hatch (scheduler.py:904reads it from the environment).tests/test_scheduler.pyhas four dirty-tree verdict tests that reach the real project rather than the worktree, so an exported override changes what they say.Measured on the real repo, same landing tree
958785144e64throughout:suite FAILEDnaming fourtest_scheduler.pydirty-tree testssuite OK: 2783 passed, 17 skippedsuite OK: 2783 passed, 17 skippedWith the override unset those four pass dirty tree and all — so the variable, not the dirt, was deciding, and the run was reporting a verdict about the caller's working tree.
Fix:
_suite_envdrops it, for the same reason it already pinsPYTHONPATHandPYTHONDONTWRITEBYTECODE— it changes what the suite says about a tree rather than what the suite can see. It cannot cost the run its own answer: a fresh worktree is clean, so the exception the override grants is not in play there.Tests (both directions)
tests/test_check_merge_plan_suite.pygains three:test_a_successful_run_leaves_no_fetched_pr_ref_behindtest_a_plan_that_conflicts_leaves_no_fetched_pr_ref_behind_either— the rc-3 pathtest_the_run_does_not_hand_the_suite_the_callers_dirty_tree_override— behavioural, not structural: a test inside the fixture's own suite fails if the variable reaches it, so this asserts behaviour rather than the shape of an env dict._run_toolgained anenv=parameter for it.Mutation arms, the script restored byte-identically after each (
sha256[:16]40a41c997205d4a5before and after):refs/emrg-plan-suite/pr1suite FAILED), ref tests GREENB is the arm that pays for the second test: a cleanup wired into the success path passes the first test and leaks on exactly the runs that fail.
Verification
tests/test_check_merge_plan_suite.py: 37 passedscripts/check-doc-count.py --measure→ 2800 collectedcheck-merge-plan-suite.py 1323left the ref count at 88 where a leak would have made it 89, and the second reported the true verdictNotes
--keepis what made rerunning the same plan affordable in the first place.