test: a fingerprint that could not be computed must not look like one that was

[jdatcmd]

pgc_source_fingerprint could return a confident WRONG hash, silently, with
status 0. Three defects, one function, all closed here with arms in both
harnesses.

1. A failed digest was substituted as an empty one

printf '%s %s\n' "${_pgc_fp_f#"$dir"/}" \
    "$(md5sum < "$_pgc_fp_f" 2>/dev/null | cut -d' ' -f1)"

2>/dev/null inside a command substitution turns a FAILED md5sum into an EMPTY
digest rather than an error. One transient failure among many -- a fork that hits
EAGAIN, an OOM kill, a loaded runner -- changes the whole hash and nothing can
tell. Modelled with a stub md5sum that fails on its Nth call and is otherwise the
real binary:

baseline (real md5sum)      c8e6b23db1c9
one digest empty (call #2)  22897add806e
one digest empty (call #3)  58c76fdab962
exit status                 0

Three different confident answers over ONE unchanged tree.

On the READ side that costs one suite at random. #902's PG18 leg reported
FATAL: the binary under test was not built from this source for iceberg_rest
alone, 244 ran and one failed, and the stamp it disagreed with -- 6d122a7158d5 --
equals a clean local fingerprint of that same head. So the WRITE was right and one
READ was not.

On the WRITE side it is worse and deterministic. The controller stamps ONCE
per batch, so a failed digest there bakes a wrong hash and every suite reports
stale against a tree that is perfectly clean. Measured by @OffgridwithJD, stub
failing only during the write, all five reads healthy:

the tree's TRUE fingerprint     059c5c2f3cca
the stamp the controller wrote  79742b3335ab   (rc=0, nothing warned)
suite 1..5                      -> stale

stale is the FATAL. It names a stale binary, which is the diagnosis whoever hits
it will go and chase, on a correct tree.

The fix is not that the computation cannot fail. It is that a failure is REPORTED
as one: empty, which pgc_freshness_verdict already turns into unknown and the
controller already prints as freshness UNVERIFIED and deliberately does not fail.
The asymmetry is the whole argument. A false UNVERIFIED costs a line of output.
A false FATAL costs a matrix and teaches people to re-run past a freshness check,
which is the failure this controller exists to prevent.

2. One tree hashed three ways, depending on how the path was spelled

@OffgridwithJD's finding, reproduced and widened here. ${f#"$dir"/} strips a
prefix that must match character for character:

plain              92410d0598d6
trailing slash     bf101efc7c10   differs
dot segment /./    774152fff929   differs   (not in the original report)
via symlink        3f3c0e36905a   differs
dot-dot /src/..    92410d0598d6
relative .         92410d0598d6

/./ is the one worth keeping: it is what a $(dirname X)/./ composition
produces and it reads as harmless. The writer and the reader reach the tree by
different routes, so a disagreement between two spellings is a FATAL about
nothing.

Canonicalised once at the top with pgc_norm_path, the helper this file already
had, rather than a second cd && pwd -P of my own. A private copy would have
been the third normaliser in one tree, and drift between two implementations of
one idea is the defect this function has now produced four times.

3. The fix's own trap, caught before it shipped

Detecting a failed digest means capturing the per-file lines to inspect them, and
$(...) STRIPS THE TRAILING NEWLINE that the old straight pipe into md5sum
included. Without restoring it the same unchanged tree hashes differently before
and after this change, every stamp already on disk reads stale, and a fix for
false FATALs becomes a false FATAL for everyone holding a built worktree.
@OffgridwithJD hit it and warned me before I wrote it.

The matrix could not have caught that: it cp -as a fresh tree and re-stamps
every run, so it would have landed on developers and on nobody's CI. Hence
test_the_fix_does_not_rebaseline_stamps_already_on_disk, which transcribes the
previous implementation and requires the same answer. That is a COMPATIBILITY
assertion, not a tidiness one.

The arms, and the proof that each can fail

Written first and run RED before the fix existed: 6 failures, all of them the new
arms, each for the intended reason. Then GREEN, and then each fix reverted
separately with the mutation asserted applied:

revert                     .sh arms red   pytest red
the digest swallow         3              2
the canonicalisation       3              1
the trailing newline       1              1

Two controls, because the set is vacuous without them. a real content change still moves the fingerprint -- "every spelling agrees" is satisfied perfectly by
a fingerprint that ignores its input. And a stub premise: with nothing configured
to fail, the stub must agree with the real md5sum, or the arms measure the stub
rather than the fix.

A tree with no hashable file now reports NO fingerprint rather than the hash of an
empty stream, which is a stable comparable value that would have made two empty
trees "match".

Verified

harness_selftest.sh   381 passed + 0 failed + 0 unrunnable   PASSED  (main: 366)
pytest corpus          84 passed, --pgc-expect-tests 84      (main: 78)
docs_style.sh           9 checks                             PASSED

TESTS.md documents the six new tests and its totals are counted from the corpus
with the gate's own function, not derived in prose: harness=69 product=15 total=84,
inputs == sum(buckets).

What this does NOT claim. I proved the mechanism and that it is sufficient to
produce what #902's PG18 leg showed. I did NOT observe it happening in CI and
cannot -- that runner is gone. A full local PG18 matrix reproduced the
PRECONDITION once, MOVED 6d122a7158d5 -> 5adb0a7a7bcd with the file list
unchanged, but iceberg_rest passed locally, so the symptom did not reproduce.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK

[OffgridwithJD]

Reviewed by running it, not by reading it. Everything I care about checks out, including the one you flagged for my eye. Holding the formal approve until CI finishes — 10 SUCCESS, 2 suites pending.

The six properties, measured against your head

1. the five spellings agree
   /tmp/r909            22c3fe6e428e  OK
   /tmp/r909/           22c3fe6e428e  OK
   /tmp/./r909          22c3fe6e428e  OK
   /tmp/r909_link       22c3fe6e428e  OK
   /tmp/r909/src/..     22c3fe6e428e  OK

2. no re-baselining      old=22c3fe6e428e  new=22c3fe6e428e   SAME

3. the empty-manifest case
   old: [d41d8cd98f00]        <- md5 of nothing, confidently
   new: []                    <- empty

4. an unreadable tree, as a NON-root user
   mode 755: [450693895ce2]
   mode 000: []
   mode 000 + trailing slash: []
   verdict: recorded=abc123def456, current=<unreadable> -> unknown

5. a nonexistent directory: [] , with and without a trailing slash

6. a real content change still moves it

Property 6 matters more than it looks: without it, "every spelling agrees" is satisfied by a fingerprint that ignores its input entirely.

On pgc_norm_path — you were right not to take my version

Using the existing normaliser instead of my private cd && pwd -P is the better call, and for the reason this function keeps teaching us: a third implementation of one idea in one tree is how the fourth defect arrives. I would have shipped the third.

And I checked the fallback empirically rather than accepting the reasoning, because my first attempt at it was wrong: I tested mode 000 as root, got a confident hash, and nearly reported a defect. Root traverses mode 000 legitimately, so that was my instrument, not your code. As a non-root user it returns EMPTY in both spellings, and the verdict is unknown — a "freshness UNVERIFIED" line, not a false FATAL. Your reasoning holds and now it has a measurement under it.

The empty-manifest guard

[ -n "$out" ] || { printf ''; return 0; } is in, and it is the one arm with a real observation behind it rather than a model — d41d8cd98f00 is what my read-only-tree run actually produced. Without it that exact deviation still yields a confident hash that can never match a stamp.

What I checked about your claims

14 new check lines and 6 new pytest tests in the diff, consistent with the counts you quoted. I have not re-run your removal proofs; yours are asserted-applied and the three-arms-reddening cross-check you describe is a better applied-check than a grep, given grep -q "pwd -P" matching the pre-existing pgc_norm_path is exactly the ambiguity you hit.

Two things worth saying plainly

Your discriminator returning zero deviations over thousands of computations is evidence against your own leading hypothesis at that load level, and you reported it as such. Mine deviated once in 200 at a much tighter process cap. Both results are real and they are not in conflict — they bracket the load at which it becomes reachable. Neither of us has observed it in CI and neither of us can.

The honest position is unchanged: mechanism proved, reachable with the real binary, sufficient to explain every feature of #902's PG18 leg, not caught in the act. That is in your commit rather than only in a thread, which is where it belongs.

Approving as soon as the suites land green. If they redden I will say which arm.

[jdatcmd]

The failure now has a measurement with the REAL md5sum and no stub, and a third variant that @OffgridwithJD observed rather than modelled.

The third variant: the whole manifest comes back empty, and the old function hashes nothing

@OffgridwithJD found it on a read-only bind mount — content excluded by construction, not by argument — with 200 computations under process pressure: two distinct fingerprints over a tree incapable of changing, and the deviant value was d41d8cd98f00, which is md5 of the empty string.

That is not a corrupted manifest. It is no manifest, hashed confidently.

I reproduced it here as an A/B, same tree, same real binary, 20 samples per cell:

true fingerprint = eebe35d6eaed ; md5("") = d41d8cd98f00

OLD ulimit -u 45   correct=19  md5("")=1   refused=0  other=0  | sum=20 of 20
OLD ulimit -u 40   correct=8   md5("")=11  refused=0  other=1  | sum=20 of 20
NEW ulimit -u 45   correct=20  md5("")=0   refused=0  other=0  | sum=20 of 20
NEW ulimit -u 40   correct=20  md5("")=0   refused=0  other=0  | sum=20 of 20

At ulimit -u 40, main returns a confident answer about nothing in 11 runs out of 20. The branch returns the correct answer in all 40 samples and md5("") in none.

The guard that covers it is in this PR already — [ -n "$out" ] || { printf ''; return 0; } — and it is covered structurally, not statistically: the branch cannot return md5("") from an empty manifest, because a non-empty out has at least one line in it. I had shipped that guard as "a tree with no hashable file", a legitimate empty tree. It turns out to be the one arm in the set with a real observation behind it rather than a model, and TESTS.md now says so.

Both functions agree on a healthy run — eebe35d6eaed from each with no limit — which is the compatibility assertion arriving independently of the arm that asserts it.

What is still NOT guarded, said before anyone finds it

OLD ulimit -u 40 produced one other result: a value that was neither correct, nor md5(""), nor empty. On this branch that shape is the swallowed-digest case and the sentinel catches it, which is consistent with other=0 across 40 samples here. But a truncated manifest — find returning fewer files rather than none — would produce a plausible wrong hash that neither the sentinel nor the empty-manifest guard can see. I have no evidence that happens, and I did not observe it; I am naming it because the honest boundary of this change is "the three observed variants are closed" and not "the function is now infallible".

The limits of the causal claim, unchanged

I proved the mechanism and that it suffices to explain #902's PG18 leg. Neither of us observed the CI run and neither can — that runner is gone. This is causation by mechanism and exclusion, not by catching it in the act.

One instrument of mine was void and I am saying which

My first A/B looked like it favoured the fix and it proved nothing. I passed lim=0 meaning "no limit" and then executed ulimit -u 0, which forbids forking outright — so the baseline came back empty and every correct hash was flagged WRONG ANSWER, confident. The table above is the corrected run, with the baseline taken with no ulimit call at all. An A/B whose control is broken favours whichever arm you are hoping for.

[OffgridwithJD]

Re-checked at 2c19b4e9 rather than assuming my earlier verification carried. It does, and here is why that is a fact rather than a hope:

lib.sh, 7108952d vs 2c19b4e9 : byte-identical
changed files                : test/pytest/TESTS.md only, +27/-0

So the six properties I measured at 7108952d — the five spellings, no re-baselining, the empty manifest, the unreadable tree as a non-root user, the nonexistent directory, and a content change still moving it — all still hold, because the code they were measured against did not move.

And the push is exactly what selftest/350 demanded, which is the gate doing its job on the PR that fixes the function the gate depends on:

new tests in the PR : 6
undocumented        : 0
totals stated       : **84 tests in 6 files.**   (main: 78, +6)

I checked all six names against the document rather than trusting the count, and re-derived 84 from main's 78.

Still 8 pending. Approving on green, as before.

[jdatcmd]

Retraction: I said this branch's mechanism explains the CI failure. It does not, and this PR's own CI is the evidence against me.

suites (PG 17) failed here at 2c19b4e — the head that contains the whole fix — with:

FAIL  iceberg_rest_server
FATAL: the binary under test was not built from this source
       source now a735c673b129, binary built from 6d122a7158d5

Compare #902's PG18 failure:

FAIL  iceberg_rest
       source now a735c673b129, binary built from 6d122a7158d5

The same pair of hashes. Different branch, different major, different suite, and one of the two runs had the fix in it.

Why that refutes what I claimed

I argued the deviation was a transient md5sum failure. Three things kill it:

1. A stochastic digest failure produces a DIFFERENT wrong hash each time. My own stub proved that — 22897add806e, 58c76fdab962, 1d7c00654866, all different. An identical value twice is a deterministic state, not a random one.
2. PG17 and PG18 use different build directories (/tmp/pgcolumnar-matrix-17.XXXXXX vs -18.XXXXXX). Under the spelling defect the absolute path enters the digest, so two different build dirs would give two different deviant values. They gave the same one. So it is not path-dependent, which rules out the spelling defect as well.
3. The fix was present and did not prevent it. Verified rather than assumed — at 2c19b4e, test/lib.sh carries pgc_norm_path at :669, the sentinel at :715, the empty-manifest guard at :724 and the trailing newline at :736.

So a735c673b129 is a specific, reproducible, content-derived tree state that CI reaches and I have not.

What I have ruled out, by measurement rather than argument

• Not any single-file perturbation of the input set: I removed each of the 64 hashed files in turn, and added a .c, a .sql and an objstore/*.c, and none produced the target.
• Not objstore removed, nor objstore/Makefile removed.
• Not a path spelling of my tree: five spellings plus a symlink, none matched — and per (2) above the deviant value cannot be path-derived anyway.
• Not a generated file: DATA in the Makefile is a static list, and the build generates no .c, .h, .sql or .control.

What this PR is and is not

Is: three real defects in pgc_source_fingerprint, each measured, each with a removal proof in both harnesses. A failed digest returning a confident wrong hash; one tree hashing three ways by spelling; and the empty manifest that main hashes to md5("") in 11 runs of 20 at ulimit -u 40. Those stand on their own evidence and I would still want them in.

Is not: an explanation of the iceberg_rest* failures. I stated in the commit message that the mechanism "is sufficient to explain #902's PG18 leg". That sentence is wrong and I am retracting it rather than softening it. I will amend the commit message rather than leave a false causal claim in the permanent record.

I was careful to say I had not observed the failure in CI. I was not careful enough about the difference between "a mechanism that could produce this" and "the mechanism that did". The identical repeated hash was available to me the moment #909's CI went red, and it is the datum that decides it.

Nobody should merge #902 on the theory that this PR fixes its red. The red is unexplained and now reproduced on a second branch.

[OffgridwithJD]

Verified at b8da10aa, not carried forward — my earlier look predates d9547fa,
a94b6d5 and b8da10a, so it was stale and I re-ran everything.

I re-ran the claims rather than re-reading the diff.

A failed digest fails closed. I shadowed md5sum with a real failing
executable rather than simulating one:

correct                : 6d122a7158d5
with md5sum failing    : ''          (empty)

Empty cannot be mistaken for a fingerprint, which is the whole point of the PR.
Before this it returned a well-formed 12-hex value.

The canonicalisation fixes more than the two spellings it names. I reported to
you that pwd -P would not fix the symlinked-root case, because the loss happens
in find's traversal rather than the prefix strip. I was wrong, and the reason is
the ordering: normalising $dir before pgc_source_build_dirs runs means find
never walks the symlink at all.

real path : 6d122a7158d5
symlinked : 6d122a7158d5      (was cbc6688e0ac9 before this PR)

pgc_source_build_dirs called directly with a symlinked path still returns
src alone and drops objstore. Nothing in the tree does that today — every
caller either goes through the manifest or passes $PGC_SRCDIR — so it is a
latent hazard rather than a live defect, and canonicalising once inside the
manifest is the right place for it. Worth a sentence in the header, not a change.

harness_selftest   393 passed + 0 failed + 0 unrunnable, rc=0
pytest corpus       89 passed
CI                  12 of 12 green

Approving.

One thing this PR does not fix, in the file it edits

340-the-binary-must-be-built-from.sh:133 still writes into the real tree:

_bd_probe="$_bd_root/objstore/.pgc_fingerprint_probe.c"

That is the flake we spent the day on, and it is mine from #903. harness_selftest
is in SUITES and the matrix runs PGC_JOBS suites at once (default 6), so this
writes a 65th file into the shared tree while siblings fingerprint. I reproduced
it exactly:

baseline                                : 6d122a7158d5
with objstore/.pgc_fingerprint_probe.c  : a735c673b129
CI reported                             : a735c673b129

Not a reason to hold this PR: the defect is on main, this PR does not introduce it
and does not make it worse, and you have already said you are taking the fix. I am
recording it here only so the file's next reader does not have to rediscover why
it is racing.

The constraint I would hold the fix to is the one you stated: the arm exists
because objstore/ was not being read, so it has to keep proving discovery in a
tree shaped like the real one. Probe a copy, do not weaken or skip the arm.

[jdatcmd]

The racing probe is fixed here, at bf4f509 — which invalidates the approval at b8da10a. @OffgridwithJD, this needs your eyes again; I would not carry a stale approval across a commit that changes the file the approval was about.

test/selftest/340 wrote objstore/.pgc_fingerprint_probe.c into $PGC_SRCDIR. It now probes a hardlinked COPY, with a premise that the copy discovers the same build directories as the real tree — so the arm's intent survives, since it exists because the REAL tree's objstore/ was not being read.

That premise earned itself immediately. My first fix was cp -al SRC DST || cp -a SRC DST; /tmp is a different filesystem from the tree, so the hardlink copy failed AFTER creating DST and cp -a copied the tree INSIDE it — the copy's build dirs came out bfix src instead of objstore src.

Two observers, and I got the second one wrong twice. A before/after fingerprint comparison cannot catch this: the probe is created and rm -f'd inside the same suite, so the tree is byte-identical when the run ends. Then the static version looked for the tree root inside a redirection target and missed the two-step _bd_probe="\$_bd_root/..." … > "\$_bd_probe" shape, so it passed against the reverted defect. It now follows one indirection and names the line: got '340-the-binary-must-be-built-from.sh:184' want 'none'.

Kept in this PR rather than split out because it is the same file this PR already rewrites, and that file has collided three times today.

harness_selftest  395 passed + 0 failed + 0 unrunnable
pytest corpus      90 passed, --pgc-expect-tests 90
docs_style          9 checks PASSED
live tree after a full run: 0 probe files, fingerprint unchanged

Credit to @linuxhikerpm, who found it by reading the suite (#910) while two of us were measuring the wrong things.

[OffgridwithJD]

Re-verified at bf4f5091. My approval was standing on b8da10a, which this push
moved past — this repo does not dismiss reviews on push, so an unreviewed head was
carrying my APPROVED. It holds, and here is the run behind that.

I tested the property that was actually broken rather than reading the diff:
does the live tree's fingerprint stay constant while harness_selftest runs?
A sampler polls pgc_source_fingerprint against the real tree throughout.

post-fix (bf4f5091)   0 deviations
pre-fix  (b8da10a)    1 deviation -> a735c673b129

The control is the half that matters. Without it, "0 deviations" is
indistinguishable from a sampler that died on the first iteration — and the
deviation it caught is the exact value CI reported, so the instrument is
measuring the right thing.

harness_selftest at bf4f5091   395 passed + 0 failed + 0 unrunnable, rc=0

The three additions are the right ones. the probe is written outside the live source tree is the arm that would have caught the original, and the
PREMISE the copy discovers the same build directories arm earns its place —
your comment says it caught the cp -al fallback producing bfix src instead of
objstore src, which is precisely the failure that would have made
"a new file under objstore moves the fingerprint" pass for the wrong reason.

One thing to leave a note about, not to change

The copy is hardlinked, so the copy's files share inodes with the real tree.
That is safe for this arm and only this arm, because it exclusively CREATES a new
file. The obvious next edit is not safe:

# a future arm, entirely reasonable-looking:
printf 'int x;\n' >> "$_bd_copy/src/columnar.c"     # writes through to the REAL tree

>> and in-place sed -i on a hardlink both modify the original. The comment
explains why creating a file is safe; it does not warn that modifying one is not,
and the next person to extend this arm will be reading it for permission rather
than for prohibition. Given this arm's history, a sentence saying "create only,
never modify — these are hardlinks" is worth more here than almost anywhere else
in the file.

Not blocking, and I am not asking for a push to add it — fold it in if you touch
the file again.

[OffgridwithJD]

Approving at bf4f5091. You were right to block on this and right about the
mechanism: my APPROVED review carried commit=b8da10aa, and my re-verification of
bf4f5091 went in as a comment, which does not move the review record. A green
label naming an older sha is exactly the trap, and I put it there.

The twin, verified by reverting the defect rather than by reading it

Reverted _bd_copy to _bd_root in my own checkout of this head. Both halves
redden, and each names something a reader can act on:

pytest : no selftest part directs a write at the live source tree:
         got '340-the-binary-must-be-built-from.sh:184' want 'none'
shell  : the probe is written outside the live source tree:
         got [INSIDE /root/w909b] want [outside]
restored byte-exact; both green again

Your reasoning for making it a static scan is the part I want to endorse
explicitly, because it is the kind of thing that usually gets argued the other
way. A before/after run genuinely cannot catch this: the probe is created and
rm -f'd inside one suite, so the tree is byte-identical when the run ends, and
the only observer that could see it is one sampling during the window — which
would make the arm racy and pass whenever the timing missed. The observable
property really is the one in the source. The three premises plus the _bd_copy
control are what make the scan more than a pattern that matches nothing.

The property that was actually broken, measured

I tested the thing itself rather than the arms: does the live tree's fingerprint
stay constant while harness_selftest runs?

post-fix (bf4f5091)   0 deviations over a full run
pre-fix  (b8da10a)    1 deviation -> a735c673b129

The control is the half that matters — without it, "0 deviations" is
indistinguishable from a sampler that died on its first iteration — and the value
it caught is the one CI reported.

harness_selftest   395 passed + 0 failed + 0 unrunnable, rc=0
pytest corpus       90 passed
CI                  12/12

On the two you got wrong first

Both are worth more than the fix. The cp -al || cp -a fallback creating DST and
then copying the tree inside it is a failure mode I would not have predicted,
and the premise catching it as bfix src instead of objstore src is the whole
argument for premises over assumptions. And a first twin that passed against the
reverted defect is the same shape as my own bash counter reading the first digit
on the line: an instrument that agrees with a healthy tree and cannot see a sick
one.

One note for the file's next reader, not for this PR

The copy is hardlinked, so its files share inodes with the real tree. Safe for
this arm, which only ever CREATES a file. The obvious next edit is not:

printf 'int x;\n' >> "$_bd_copy/src/columnar.c"   # writes through to the REAL tree

>> and sed -i both modify the original through a hardlink. The comment
explains why creating is safe; it does not warn that modifying is not. Given this
arm's history, "create only, never modify — these are hardlinks" earns its line.
Fold it in whenever you next touch the file; not worth a push on its own.