Skip to content

fix: the orphan scan counted one ledger row in two buckets (#1270) - #1273

Merged
jdatcmd merged 1 commit into
mainfrom
fix/1270-orphan-scan-double-count
Sep 25, 2026
Merged

jdatcmd merged 1 commit into
mainfrom
fix/1270-orphan-scan-double-count

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

The orphan-scan half of #1270. Deliberately no closing keyword: the
second cause described below is still open, and GitHub ignores the
qualifier in a phrase like "fixes the X half of #N" and closes #N anyway.

The ledger orphan scan refused its own arithmetic, and that refusal reddened
PG19 in run_all_versions.sh on a clean tree with every suite passing.

The bug

matched was drawn from every ledger row while the other three buckets
partition checkable:

  checkable = {k for k in rows if (k[0],k[1]) in parts and (rows[k][0] & run_majors)}
  unchecked = sorted(set(rows) - checkable)
  ...
  matched   = len(set(rows) & now)        # <- the wrong side of that line

So a row the run still emits, whose ledger majors do not name the running
major, landed in matched and in unchecked at once. The four categories
stopped partitioning and the integrity assertion refused.

matched is now checkable & now. The row belongs in not checked: this scan
asks whether the ledger names a check that no longer exists, and a row claiming
majors this run is not cannot be answered here in either direction.

Red, then green, on the real artifact

The new pytest test fails first, for its own reason, with both premises passing:

  a row outside the run's majors is classified once, not counted twice: got 1 want 0
  accounting: 2 pass + 1 fail + 0 unrun = 3

After the fix, and on the actual log that aborted the scan:

  new test                      8 pass + 0 fail
  test_mutation_ledger.py       39 passed, 209 checks, no regressions
  pg19-projection_scan_io.log   rc=2 -> rc=0
  every pg19 log                logs with a status outside {0,1}: 1 -> 0

The test carries the control that matters: a run on the claimed major must
still be clean with not checked=0. Without it the test passes on a build where
matched is always zero, which would break every legitimate match.

guard_tests goes 403 -> 404, re-derived with the collect-only command
expected_tests.txt documents rather than incremented by hand.

This does NOT make PG19 green, and I am not claiming it does

A clean tree still reports FAIL PG19, now for a second and unrelated reason:

  not in the ledger: projection_scan_io  premise: a covering path is still offered ...  (on major 19)
  not in the ledger: projection_scan_io  premise: the projection row still exists ...   (on major 19)
  not in the ledger: projection_scan_io  a covering projection whose storage ...        (on major 19)
  PG19 has a check the ledger has never seen, which is not a pass

Those three checks run on 19 while their ledger rows claim only 15;16;17;18.
That refusal is correct: it names the three checks and prints the merge
command that fixes it, which needs one log per gated major. I verified it is
present on pristine main before this change, so it is separate maintenance and
not something to smuggle in here.

Both causes were live at once, which is why the first diagnosis only found one:
the orphan scan aborted before the picture was complete.

Gate

Rebased onto 5b45f3d so it sits on top of #1271.

  build_all_versions   built 5 of 5, 0 warnings
  PASS   PG18  (267 ran, 2 skipped, 0 incomplete)
  FAIL   PG19  (269 ran, 0 skipped, 0 incomplete)   <- the never-seen checks above
  non-PASS suites: (none)
  ledger/orphan abort verdicts: (none)              <- this PR's subject, gone

The orphan-scan abort that this PR is about no longer appears. An earlier run of
this same gate failed docs_style on two em dashes I had written into my own
CHANGELOG entry; the plain-language checker caught me and they are gone.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

The fix is right, and ask 1 is not the judgement call you think it is — it is
forced.
Not approving yet: pytest (harness guards, no database) is FAILURE
on this head.

The choice is settled by arithmetic, not by taste

Reading the surrounding code rather than the diff alone:

  checkable  = {k for k in rows if (k[0],k[1]) in parts and (rows[k][0] & run_majors)}
  unchecked  = set(rows) - checkable
  absent     = checkable - now
  orphans    = [k for k in absent if ...]
  unprunable = [k for k in absent if ...]

checkable ⊎ unchecked = rows exactly, and absent ⊎ (checkable ∩ now) = checkable, with orphans ⊎ unprunable = absent. So:

  matched + orphans + unprunable + unchecked
    = |checkable ∩ now| + |absent| + |unchecked|
    = |checkable| + |unchecked|
    = |rows|

The assertion holds identically with matched = checkable & now, and cannot
hold with set(rows) & now whenever a row is in both unchecked and now —
which is exactly the three projection_scan_io rows. 11 + 1766 = 1777 is not
a symptom to be explained, it is the double-count the algebra predicts.

So the answer to "should a row the run emits count as matched regardless of its
majors?"
is: not without redefining checkable. Once the other three
buckets are drawn from the checkable/unchecked line, matched has no choice.
Anyone wanting the other behaviour has to argue about the majors term in
checkable, which is a different and pre-existing decision.

I agree with where you put the row, and separately from the arithmetic: the two
tools ask different questions. The orphan scan asks "does the ledger name a
check that no longer exists?"
and cannot answer that for a major the row does
not claim. The ledger gate asks "has this check been seen red on this major?"
and correctly refuses. Making the scan call it matched would let a majors
mismatch launder itself into a pass in the tool that is not looking for it.

Ask 2: the control is right, and here is precisely what it does not cover

It bounds the failure you named — matched always zero — and the subject test
bounds the other direction, since a matched that counted too much breaks the
sum. Together they pin matched from both sides. That is stronger than you
credited it.

Neither arm tests checkable's own definition. Both take the majors term as
given. If the majors question were ever revisited, both tests would keep passing
while the answer changed underneath them. Not a reason to add an arm now — it is
a reason not to cite these two as evidence about that question later.

Your flag observation deserves the separate issue

verfail carrying two independent conditions is the same defect that was
already fixed one level down, where _orph_orphan and _orph_broken were split
with the comment "Two independent conditions need two independent flags." An
abort stopping the picture being completed is why you saw one cause and not two
— and that is a property of the mechanism, not of your attention. File it; I
would review it.

The CHANGELOG correction is the part I would keep

You had written "no longer reports FAIL PG19 on a clean tree" and the gate
caught that it still does, for a second reason. An entry that overstates a fix
is the kind of thing nobody re-measures, and it survives because prose is not
executed — which is the same shape as the __cxa_finalize comment that misled
you earlier today.

Blocking only on the red

pytest (harness guards, no database) is FAILURE. expected_tests.txt goes
guard_tests 403 -> 404 for one new test function, which looks right, so I am
not guessing at the cause — I could not retrieve the job log. Worth checking the
collected count against the file rather than the arithmetic.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n

The ledger orphan scan refused its own arithmetic, and that refusal reddened
PG19 in run_all_versions.sh on a clean tree with every suite passing.

matched was set(rows) & now, drawn from every ledger row, while orphans,
unprunable and not-checked all partition checkable. A row the run still emits,
whose ledger majors do not name the running major, therefore landed in matched
and in not-checked at once, and the integrity assertion correctly refused a
classification that did not partition. matched is now checkable & now, from the
same side of that line.

The row belongs in not-checked. This scan asks whether the ledger names a check
that no longer exists, and a row claiming majors this run is not cannot be
answered here in either direction.

Measured on pg19-projection_scan_io.log, the one log that aborted the scan:

  before   matched 11 + orphans 0 + unprunable 0 + not checked 1766 != 1774  rc=2
  after    parts in the run=1, rows in those parts=8, orphans=0              rc=0

and across every pg19 log, logs returning a status outside {0,1} went 1 to 0.

This does NOT make PG19 green. A clean tree still reports FAIL PG19 for a
second, unrelated reason: three projection_scan_io checks run on 19 while their
ledger rows claim only 15;16;17;18, so the ledger gate refuses a check it has
never seen. That refusal is correct, names the three checks, and prints the
merge command that fixes it. It is present on pristine main before this change
and is separate maintenance.

The integrity assertion is not the defect. It caught a real classification
error rather than reporting a clean scan.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
@jdatcmd
jdatcmd force-pushed the fix/1270-orphan-scan-double-count branch from 0e92400 to 8b3b01d Compare September 25, 2026 19:40
@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Correcting my own review: I endorsed the verfail finding without checking
it, and it was wrong.

I wrote "File it; I would review it" about two independent conditions sharing
one flag. @jdatcmd has since withdrawn that on #1270, and I have verified the
withdrawal rather than accepting it on trust:

  run_all_versions.sh:1727   "PG$major has a check the ledger has never seen ..."
  run_all_versions.sh:1729   "PG$major could not run the ledger gate at all ..."
  run_all_versions.sh:1824   "PG$major could not run the orphan scan on at least one log ..."

Three distinct messages at three distinct sites — and the comment at 1722 records
that collapsing them was already found and fixed. verfail is the per-major
verdict aggregate that many conditions set by design; it was never the
diagnostic. There is nothing to file.

Why I endorsed it. I checked the arithmetic in this PR line by line — the
partition, the collapse to |checkable| + |unchecked|, why 1777 is predicted
rather than symptomatic. Then I took the mechanism claim at face value because
it sounded like a pattern I had been applying all day, and an unapplied-lesson
story is a satisfying shape. I verified the parts that looked like arithmetic
and trusted the part that looked like a diagnosis.

That is the same failure the withdrawal itself describes, one remove out.
@jdatcmd read their own gate wrapper's summary — whose grep enumerated three
conditions and was silent about the fourth — and concluded the runner had hidden
something. I read their conclusion and concluded the same. A count is a claim
about its glob
, and so is an endorsement.

Everything else in my review stands, and none of it depended on the withdrawn
claim.

TESTS.md was the whole cause of all three reds, guard_tests 403 -> 404 was
never the problem, and running the full guard set the way CI does rather than
one file is what separates 404 collected / 1131 checks / 0 failures from three
reds.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 8b3b01d6. 15 of 15, pytest (harness guards, no database)
SUCCESS, both suites SUCCESS, CLEAN. TESTS.md now names the new test and
guard_tests reads 404.

The substance was settled in my earlier comment and nothing has changed it. The
short form:

The fix is forced, not chosen. checkable ⊎ unchecked = rows,
absent ⊎ (checkable ∩ now) = checkable, orphans ⊎ unprunable = absent, so
the sum collapses to |checkable| + |unchecked| = |rows| identically with
checkable & now and cannot hold with set(rows) & now whenever a row sits in
both unchecked and now. 11 + 1766 = 1777 is what the algebra predicts.

The control is stronger than you credited — it and the subject pin matched
from both sides. Neither covers checkable's own definition, which is worth
remembering rather than fixing.

Two corrections stand on the record, and I would rather they did:

  • I endorsed the verfail finding without checking it. Verified since: the
    runner emits the conditions at three distinct sites, and the comment at
    run_all_versions.sh:1722 records that collapsing them was already found and
    fixed. I checked your arithmetic line by line and trusted your diagnosis
    because it sounded like a lesson I had been applying all day.
  • The TESTS.md omission was the whole cause of all three reds.
    guard_tests 403 -> 404 was never the problem, and running the full guard set
    the way CI does rather than one file is what separates 404 collected / 1131
    checks / 0 failures from three reds.

Not merging this; it is yours.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n

@jdatcmd
jdatcmd merged commit 4ea566e into main Sep 25, 2026
15 checks passed
OffgridwithJD pushed a commit that referenced this pull request Sep 25, 2026
The ported test adds one collected test to the cluster half, and
`--pgc-expect-tests` is exact rather than a floor -- both cluster jobs would
collect 484 against a declared 483 and fail. Caught by @jdatcmd before CI
reached it.

RE-DERIVED BY COLLECTION, not incremented, using the command the file
documents:

    guard files 24, cluster files 53
    guard collected   403
    cluster collected 484

The file says why the distinction matters: the numbers are collected tests, not
test functions, so a later parametrization expands one function into several and
an increment would be wrong in a way a rerun would not show.

`guard_tests` needs nothing here. This branch carries 403 and main carries 404
from #1273; the line is untouched on this side and merges clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
OffgridwithJD pushed a commit that referenced this pull request Sep 25, 2026
Every test in the corpus must be named there, and the ported test was not --
so `test_every_file_and_test_is_named_in_the_document` reddened all three pytest
jobs from one omission. Same cause as #1273, which @jdatcmd had hit and warned
me about an hour earlier; I added the test and not the entry.

It joins section 81's "Every test" table rather than taking a section of its
own, matching the two tests already in that file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants