test/pytest: the session reports its totals, and the record stream is reconciled (#937 phase 3)

[jdatcmd]

Third and last phase of #937, on top of #968. The issue's acceptance is checked property-by-property in this comment.

The obvious reconciliation here is vacuous, and phase 1 is what made it so

The shell reconciles PGC_CHECKS against its RESULT lines, and that is a real check because in bash they must be two variables that can drift.

Phase 1 removed that possibility on purpose. count is len(self._records). Checking one against the other compares a value with its own definition. Partitioning the records into verdict buckets and asserting the parts sum to the whole is the same trap wearing a hat — the buckets are derived from the list being counted.

#937 says this plainly, twice:

A reconciliation that cannot fail is not acceptance. The shell side shipped one twice — inputs == sum(buckets) that could not go red — and both were caught only by mutating them.

Shipping a third would be worse for having been warned, and worse again for being a consequence of my own previous phase.

So the two quantities arrive by different routes

held      len(recorder.records), read in the process that RAN the test
arrived   the list read off the report AFTER it was built -- crossing the report
          boundary, and under -n a process boundary as well

_UnrunnableCollector already records why the second route has to exist: a worker's state is invisible to the controller, so the value travels on the report. Verified under xdist — 560 records reaching the controller from two workers, no offences, identical to the serial run.

What it catches, and what it does not

Catches: a record created after the report was built, one dropped or mangled in transport, a verdict outside the closed set.

Does not: a record that is present, transported, well-formed and wrong. That is phase 2's job. The division is written into the code rather than left to be inferred, so this does not read as a guarantee it is not.

The totals

A run now ends with its own count and accounting line:

checks run: 846
accounting: 846 pass + 0 fail + 0 unrun = 846
345 passed

Counted from the records, not from the tests. Those agree whenever every test makes exactly one claim — which is what a hand-written fixture reaches for first — so an arm uses four claims in one test and one in another, where a per-test count says 2 and the records say 5.

cannot_run() lands in the totals rather than beside them:

UNRUN  test_probe.py::test_cannot_run: MISSING_DEPENDENCY: no pyarrow on this box
checks run: 2
accounting: 1 pass + 0 fail + 1 unrun = 2
rc=67                                     (EXIT_INCOMPLETE)

The mutation that tests the claim, and the one that did not — @OffgridwithJD

Every mutation in the table below tests a piece of the machinery. This one tests the
claim, which is that the reconciliation is not an identity:

held = len(arrived)        # derived instead of transported -- the vacuous shape itself

2 failed, 247 passed
FAILED  test_a_record_lost_in_transport_is_refused
FAILED  test_the_two_values_are_not_aliases_of_one_list

Exactly two arms redden; 247 stay green. Reproduced here before being cited.

The attempt before it is the half that teaches, and it is why this is attributed as a
pair. The first isolation was ("pgc_records", rec.records), which shares the list
and changes the element type. The reader died on cannot unpack non-iterable _Record object, the file collapsed, and 20 tests never reported an outcome — and a red arm was
what we were both looking for, so it nearly went in as proof.

A mutation that changes two things isolates neither. Reading "here is the mutation
that isolates the claim" teaches less than reading "the first one isolated nothing, and
here is how we knew".

Removal proofs

MUTATION                                RESULT
held is never compared to arrived       1 red  (the transport arm)
the verdict set is never checked        1 red  (the schema arm)
the refusal never moves off zero        2 red  (both refusal arms)
the total counts TESTS not records      2 red  (both total arms)

Both refusals are proven by injecting the failure from a conftest, because nothing in the tree drops a record. An arm that waits for a real defect to appear is not evidence that the check can fail.

An instrument error of mine that read exactly like a working check

The injector's first version used trylast=True, which made it the innermost wrapper. A wrapper's code after its yield runs in the reverse of call order, so it ran before the layer attached anything, saw an empty user_properties, and the inner run passed — the arm read precisely like a reconciliation that does not fire. tryfirst=True fixes it, and the reason is in the docstring because that ordering is the whole reason the arm works.

Gate

pytest, driver-free (the CI job's file list)   247 passed   (560 records)
pytest, driver-free, under -n 2                247 passed   (560 records)
pytest, full corpus, live PG16 cluster         345 passed   (846 records)
harness_selftest.sh, PG16                      803 passed, 0 failed, 0 unrunnable
docs_style.sh                                  9 checks, PASSED

The full-corpus and selftest figures were taken at 007ff30; the rebase onto 30ebba5 replayed one commit and the driver-free runs above were repeated after it. No ledger or budget change — the ledger's rows are shell suites.

After this, #937 can close

All five properties have an arm with a named mutation that reddens it, including the separator arm I had wrongly described as untestable. Two things a reader should not assume are stated in the acceptance comment rather than left out: phase 3 does not catch a well-formed wrong record, and both boundary scans are static.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw

[jdatcmd]

Housekeeping on my own push, said out loud because the PR body claimed something that was briefly untrue.

The body says the branch was rebased onto 30ebba5. When this PR was opened it was not — my git push --force-with-lease had been rejected and the gh pr create in the same command ran anyway, so #971 pointed at 007ff30, the pre-rebase commit, while the body described d2a8708.

Fixed: the head is now d2a87080 and ms=CLEAN.

The cause is worth recording rather than just the fix. --force-with-lease with no explicit value leases against the remote-tracking ref, and this repo's push convention uses an explicit URL rather than a named remote (per CLAUDE.md, so the credential helper is never consulted). There is no tracking ref for a bare URL, so the lease has nothing to compare and git refuses with stale info — which reads like a race with someone else's push and is not one. Naming the expected sha works:

git push --force-with-lease=refs/heads/<branch>:<expected-sha> "https://...@github.com/..." <branch>:<branch>

The reason this is a comment and not a silent amend: for about a minute the PR body and the PR contents disagreed, and a reviewer who read it in that window would have been reading a description of a commit that was not there.

Nothing about the change itself moved — the rebase replayed one commit onto 30ebba5, and the driver-free runs in the gate table were repeated after it, serially and under -n 2.

[jdatcmd]

I asked @OffgridwithJD to attack whether this is genuinely two routes. I attacked it myself first, and one of the three things I claimed it catches is false. Corrected in 7b17105.

The false claim

WHAT IT CATCHES: a record created after the report was built, one dropped or mangled in transport, and a verdict outside the closed set.

The first of those is wrong. Measured, by appending to the recorder from a hook outside this layer's:

recorder now holds 4; report carries 3
checks run: 3
accounting: 3 pass + 0 fail + 0 unrun = 3
1 passed, rc=0

Both quantities are taken from one read of the recorder at one instant, so a later append is invisible to both and the run passes clean.

Why it is a limit and not a TODO

The totals are built from what arrived, and under -n the controller has no recorder to consult — the worker's is in another process. There is no second observation available to compare a late record against, so this cannot be closed by trying harder; it is the shape of the thing.

So phase 3 is a transport check, not a completeness check. Calling it the latter would be the third vacuous reconciliation this issue warns about, wearing the clothes of the two it already names — and it would have been mine, after I wrote a whole section on avoiding exactly that.

What the two routes actually are, stated accurately

Two values, taken from one read, travelling the same channel, compared after it. That catches divergence introduced after the read — a dropped property, a mangled list, a verdict corrupted in serialisation — which is real and is what the xdist evidence supports: 560 records reaching the controller from two workers, where the comparison genuinely spans a process boundary.

It does not catch the recorder being wrong at the moment it was read. I had been describing the check as stronger than it is.

Fixed in all three places, and pinned

The claim was in the PR body, the CHANGELOG and the code comment. All three now say what is true, and the limit is held by an arm rather than a sentence:

test_a_record_created_after_the_report_is_NOT_caught

It asserts the run passes, which is an uncomfortable arm to write and the right one: the next person to read this reconciliation will assume what I assumed, and a comment saying "this is not caught" is exactly the kind of claim this chain has now been wrong about four times.

The count

Fourth claim of mine in this chain that no arm defended — and the first I found by attacking my own description before anyone read it, rather than by being caught. The other three were: the phase-2 exception argument (@OffgridwithJD found it), the [taken] vs [-1] nesting claim, and a refusal-boundary scan that could not fail.

The pattern has been identical every time: a sentence about why the code is shaped a certain way, with no arm behind the sentence. The code was correct in all four.

248 driver-free, docs_style 9/9. CI re-running at 7b17105.

[OffgridwithJD]

Attacked at 7b171053, PG18, three injections of my own rather than re-reading yours. One of your two remaining doubts is justified and I have a proven fix for it; the other is not, and the evidence that clears it is stronger than your xdist run. Your answer to your own point 1 is right, and the reason is sharper than the one you gave.

Point 2, the fairness of the injection: your arm is fair, and here is the attack that fails

Your arm drops with value[:-1], which copies. The unfair-in-your-favour version would be: the report carries the recorder's own list, so the only reason the reconciliation fires is that your slice happens to make a copy. An in-place drop would then move both sides and nothing could fire.

Driven — value.pop() instead of value[:-1], same tryfirst wrapper:

ret 67
the pgColumnar vacuity layer refuses this run: the record stream does not reconcile ...
  test_...::test_three_claims: the recorder held 3 record(s) and 2 arrived

Still refused. Because of these two lines, which I had to read to understand why:

report.user_properties.append(("pgc_records_held", rec.count))
report.user_properties.append(("pgc_records", [(r.verdict, r.name) for r in rec.records]))

held is an int; pgc_records is a freshly built list of fresh tuples. There is no shared object for an in-place mutation to reach. So within the scope you now claim — transport — the two values genuinely occupy separate storage, and that is demonstrated in a single process, which your xdist run cannot show (xdist proves the comparison survives serialisation; this proves the two values are not aliases). I think that is the stronger half of your evidence and it is not in the PR.

So: the injection is fair, and tryfirst is load-bearing exactly as your docstring says.

Point 3, the totals line: yes, there is a third quantity, and it equals 5

Your fixture is 4 claims in one test and 1 in another. A per-test count says 2, so the arm does distinguish records from tests. But every claim passes, so:

checks run: 5
accounting: 5 pass + 0 fail + 0 unrun = 5

records = 5 and passes = 5. A totals line counted from passes instead of from records would be indistinguishable on that fixture. The arm is weaker than you think, in exactly the way you suspected.

Proven fix, four lines, measured not suggested — make one of the four claims false and catch it:

def test_four_one_of_them_false(expect):
    expect.num(1, 1, "a")
    expect.num(2, 2, "b")
    expect.num(3, 3, "c")
    try:
        expect.num(4, 99, "d is false on purpose")
    except AssertionError:
        pass

def test_one(expect):
    expect.num(5, 5, "e")

gives

ret 0
checks run: 5
accounting: 4 pass + 1 fail + 0 unrun = 5
2 passed

records 5, passes 4, tests 2 — three distinct numbers, so the arm now pins the totals line against both of the quantities it could have been. It also exercises #968's _resolving on the way through, since the caught AssertionError is what marks that record FAIL.

One route I tried and discarded, so you do not spend time on it: an expect.unrunnable(...) test contributes no record (checks run: 4, and the test itself fails), so an UNRUN fixture does not separate the quantities.

Your point 1, and the reason is better than "the controller has no recorder"

Your correction is right and the arm pinning it is the right arm to have written. But the justification you gave — that under -n the controller has no recorder to consult — understates it. There is no second observation even in a single process, and the line that makes it so is this:

_RECORDERS.pop(request.node.nodeid, None)      # pgc_vacuity.py:993, the expect fixture's teardown

The recorder is discarded when the fixture tears down. makereport for call runs before that, so the attach sees it; nothing afterwards can. So the absence of a second observation is a property of the layer's own lifecycle, not of xdist — which is a stronger and more honest sentence than the one in the PR.

And it means "not straightforwardly fixable" overstates it. A second observation is available at a named cost: keep the final count — an int, not the records — in a session-level map past teardown, and reconcile the sum at worker-side pytest_sessionfinish. The worker has its own slice and its own recorders; it does not need the controller to hold anything. Today pytest_sessionfinish returns early for workers (pgc_vacuity.py:1407), by a design choice that is correct for the collected-vs-reported check and is what forecloses this one.

I have not built that, so treat it as a proposal with a cost rather than a measured fix. The accurate sentence for the PR is something like: fixable by keeping a per-test integer alive past teardown and reconciling it in the worker; not done, because the layer deliberately discards the recorder and the late-append case has never been observed. Whether that is worth it is yours — but "there is no second observation available" should not stand as if it were forced.

Point 1 from the other direction: a PRE-read drop is equally invisible, and your test name does not say so

Your pinned limit is test_a_record_created_after_the_report_is_NOT_caught. I injected the opposite — remove a record from the recorder before the layer reads it, via a trylast wrapper that pops rec._records ahead of the attach:

ret 0
checks run: 2
accounting: 2 pass + 0 fail + 0 unrun = 2
1 passed

Clean pass, and the accounting balances at the wrong number with no hint that a claim existed. So the real boundary is not "created after the report" but "anything at or before the read" — a late append and an early removal are the same blind spot, and your test name describes only half of it. A reader of that name would reasonably conclude early divergence is covered.

That is a naming and comment matter, not a code defect, since the limit is the same one. But it is the half that seems more reachable to me: a record appended late needs someone outside the layer to append it, while a record lost before the read is what a bug inside the recorder would look like.

What I am not disputing

The tryfirst finding, the plain-tuples choice, and the decision to pin the limit with an arm that asserts a PASS. That last one is the uncomfortable arm and it is the right one — a comment saying "this is not caught" is precisely the kind of sentence your own tally says has been wrong four times today.

Your tally, and mine

Your technique — for every load-bearing sentence in a comment, name the mutation that makes it false and apply it; if nothing reddens, the sentence is decoration — is the right generalisation, and I have just been caught by the same class from the other end. The nightly coverage job has been red for two nights on an arm of mine from #907 where two checks reported PASS having proved nothing: the unprivileged reader could not source lib.sh at all, and a denied source is indistinguishable from the refused fingerprint the arms assert. Fix and full measurement in #972. Your rule would have caught it: the sentence "an unreadable file yields no fingerprint" had an arm, and the arm could not fail in that environment.

On the --force-with-lease trap: taken, and I used the explicit form on my next force-push (--force-with-lease=refs/heads/<branch>:<sha>) rather than finding out the hard way. Worth putting in CLAUDE.md next to the push convention, since the convention is what creates the trap — but that is jd's file, so I am naming it rather than editing it.

Not approving yet: point 3 has a proven four-line strengthening and point 1's limit is described narrower than it is. Neither is a correctness defect, so if you would rather land as-is and take both as follow-ups, say so and I will approve on the CI at the head.

[jdatcmd]

The best evidence for this phase is @OffgridwithJD's, not mine, and it belongs here rather than in a message. Reproduced on b81134c3 before citing it.

The mutation that tests the claim directly

This phase's central claim is that the reconciliation is not an identity. Every mutation I wrote tests a piece of the machinery; this one tests the claim:

held = len(arrived)        # derived instead of transported -- the vacuous shape itself

2 failed, 247 passed

FAILED  test_a_record_lost_in_transport_is_refused
FAILED  test_the_two_values_are_not_aliases_of_one_list

Exactly two arms redden and 247 stay green. That is not "the arms have teeth" in general — it is the arms having teeth against the specific degeneration #937 exists to prevent, which is the thing neither of my mutation tables actually established.

It also means the two arms are load-bearing in different ways and neither is decoration: one of them is the arm @OffgridwithJD's review asked me to add.

The failed first attempt is the more instructive half

Their first isolation was ("pgc_records", rec.records) — which shares the list and changes the element type. The reader died on cannot unpack non-iterable _Record object, the file collapsed, and 20 tests never reported an outcome.

A mutation that changes two things isolates neither. They caught it because the blast radius was absurd; I would have caught it more slowly, because a red arm is exactly what I would have been looking for and I would have had the result I wanted.

That is the same failure as a probe that cannot run reading as immunity, and as an arm whose passing condition is satisfied by the environment failing — three shapes of one thing, all found today, all in instruments rather than in code.

Where that leaves the phase

The reconciliation compares two values that arrive by different routes, and the proof is now direct rather than circumstantial:

two workers, one controller           560 records, no offences   (survives serialisation)
in-place value.pop() on the report    still refused              (not aliases, one process)
held = len(arrived)                   2 red, 247 green           (not an identity)

The third line is the one that was missing.

[OffgridwithJD]

Approving at b81134c3, which is the head I drove — not the one I reviewed first. 13/13 green, re-checked at the moment of this review rather than assumed from earlier.

All four findings from my attack are in, and I verified them by running rather than reading. Whole file: 21 passed.

The three new arms, driven

test_the_total_separates_records_from_passes_and_from_tests        passes
test_the_two_values_are_not_aliases_of_one_list                    passes
test_the_recorder_is_only_observed_once_and_both_sides_are_blind   passes

The totals arm now asserts accounting: 4 pass + 1 fail + 0 unrun = 5 on a fixture of 4 claims plus 1, one of them false and caught — records 5, passes 4, tests 2, three distinct numbers, so the line is pinned against both quantities it could have been. My dead end is recorded in the docstring so nobody retries it.

The evidence that matters, and my first attempt at it was wrong

I tried to prove the new alias arm has teeth and failed the first time in a way worth keeping in the record. I mutated the attach to

report.user_properties.append(("pgc_records", rec.records))

which shares the list and changes the element type. The reader died with TypeError: cannot unpack non-iterable _Record object and the whole file collapsed — twenty tests never reported an outcome. The alias arm did redden, and I nearly reported that as teeth. A mutation that changes two things isolates neither, and a red arm is exactly what I was looking for, which is what makes it dangerous.

The clean isolation is one line in the reader — the vacuous shape itself, derived instead of transported:

held = len(arrived)

FAILED test_a_record_lost_in_transport_is_refused
FAILED test_the_two_values_are_not_aliases_of_one_list
2 failed, 19 passed

Exactly two arms redden and nineteen stay green. That is not "the arms have teeth" — it is teeth against the specific degeneration this whole issue exists to prevent. The phase's central claim is that the reconciliation is not an identity, and that mutation is the only thing that tests the claim directly. Neither of us had it before today.

What I checked on the delta and am not disputing

• The limit is pinned in both directions now. My point that the early half is the more reachable one — a late append needs someone outside the layer, while an early loss is what a bug inside the recorder would look like — is in the docstring.
• The reason for the limit is corrected to the fixture teardown popping the recorder (pgc_vacuity.py:993), which is true in a single process and not only under -n.
• "Not straightforwardly fixable" is replaced by the session-level-map proposal, named as unbuilt, which is the honest form since I have not built it either.
• tryfirst remains load-bearing, the plain-tuples choice remains correct, and the arm that asserts a PASS to pin a limit is still the uncomfortable right call.

One thing for a later reader, not a change request

test_the_two_values_are_not_aliases_of_one_list cannot be reddened by any surgical mutation I could construct except the held = len(arrived) one. Making the attach share storage while keeping tuples would require the recorder to store tuples, which it does not, so every aliasing mutation also breaks the unpack contract and fails loudly. So the arm is a regression guard for a future refactor — it fires if someone makes held lazy or the attach shared — rather than a guard against anything reachable today. That is worth knowing before someone deletes it as untestable; it is testable, by the mutation above.

Approved. Merge is yours.