test: stage the upgrade fixtures unconditionally, so a leftover cannot win (#1090)

[OffgridwithJD]

Fixes #1090.

native_upgrade_converge creates an old-version extension to upgrade from, which needs the old base install script present in the extension directory. It staged each fixture only:

if [ -f "$src" ] && [ ! -f "$dst" ]; then

This repository shipped pgcolumnar--1.0-alpha2.sql and pgcolumnar--1.0-alpha3.sql from the tree until the cycle-open rename moved them under test/fixtures, so any prefix installed before that still carries them and nothing prunes them. Because the leftover was never staged, the EXIT trap did not remove it either — it persisted and won again on every later run.

It fails in both directions, and the quiet one is the one that matters

Measured on one machine, same commit, two prefixes:

leftover	result
differs from the fixture	PG15 holds the v1.0-alpha2 tag content (fead351f84ca) against the fixture's cbb4f36e4308. The suite reported 11 passed + 0 failed — CONVERGED, having tested a file nobody committed, with nothing in the output naming which file it read.
is broken	a one-line invalid file gave 8 passed + 3 failed — a red for a defect that is not in the tree at all.

The second is how this was noticed. The first is what it costs, and it is why the fix is not simply "delete the leftovers".

That the suite reads the leftover was proven, not inferred

A leftover that merely differs cannot distinguish the two possibilities — it might be the fixture being read and converging anyway. So the discriminator was a syntactically invalid leftover, which must fail if it is read and pass if it is not:

FAIL  1.0-alpha2 -> 1.0-alpha4 upgrade applies without error:
      got [ERROR:  extension "pgcolumnar" does not exist] want [OK]

It is read.

The fix

Staging is unconditional. A pre-existing file is preserved and restored byte for byte rather than deleted — this suite did not create it and must not change the state of a prefix it does not own.

planted leftover after the run   8e3f1a0f2317   (unchanged)
PG15's alpha2 after the run      fead351f84ca   (unchanged)

Two arms state the property so a future change that reintroduces a conditional cannot pass silently. The count is pinned separately from the content, because without it a fixture that vanished would leave the content arm comparing nothing and reporting clean.

Removal proof

Restore the conditional, keep the arms:

every staged install script is the committed fixture, not a leftover:
    got [1.0-alpha2] want []

It names the script that was wrong rather than only failing.

Verified

pg18a (no leftovers)      13 passed + 0 failed
pg19a (leftovers present) 13 passed + 0 failed
harness_selftest          967 passed + 0 failed
shellcheck -S error       clean over test/*.sh and test/selftest/*.sh

The ledger is untouched. This suite is registered but uncovered — 0 rows in main — so no rows move, suites_not_covered is unchanged, and the gate is unaffected. It also has no pytest twin and is in neither COMPLETE nor INCOMPLETE, so the two new checks carry no parity obligation.

Why this is release-relevant

The five-major matrix is the release gate, and it runs on a long-lived box where these leftovers accumulate. Last night's run had this suite red on PG15, 16, 17 and 19, passing only on PG18 — the one prefix whose extension directory had been rewritten. Whoever runs the gate at the tag hits it.

[linuxhikerpm]

The staging change is the right property: if [ ! -f "$dst" ] lets a leftover install script win, and because that leftover was never in STAGED the EXIT trap never removed it either. Unconditional copy plus byte-for-byte restore of a pre-existing file is the correct ownership rule for a prefix this suite does not own. The two new arms (count of staged fixtures, content is the committed fixture) are the right pins; a vanished fixture would otherwise leave the content arm comparing nothing.

I am not approving this HEAD.

GitHub reports mergeable: CONFLICTING / DIRTY against current main (0d3100dd, v1.0-alpha4). The base is 93d9dc7. Since then main closed Unreleased into ## [1.0-alpha4] - 2026-09-17 (#1099 / #1100). The patch still inserts under the old ### Fixed that is now the shipped alpha4 section. A conflict-resolution that keeps that hunk would publish a post-tag test-harness fix as if it had shipped in alpha4.

Please rebase onto current main and put the CHANGELOG entry under a new ## [Unreleased] / ### Fixed. Do not fold it into the closed alpha4 notes.

Not merged.

[jdatcmd]

The fix is right and the arm is right. One defect in the restore path, reproduced, and two smaller notes. Not approving yet because the first one loses data that is currently evidence.

The restore path loses the original across a crashed run

cp -p "$dst" "$dst.pgcbak" runs unconditionally when $dst exists. If a prior run died before cleanup, $dst already holds the fixture and $dst.pgcbak already holds the original. The second run then overwrites the backup with the fixture.

Reproduced with your exact logic:

start:           ext=[ORIGINAL-LEFTOVER]
run 1 crashes:   ext=[COMMITTED-FIXTURE]  bak=[ORIGINAL-LEFTOVER]
run 2 stages:    ext=[COMMITTED-FIXTURE]  bak=[COMMITTED-FIXTURE]   <- original gone
after cleanup:   ext=[COMMITTED-FIXTURE]

Net: the pre-existing file is permanently replaced by the fixture, silently. That is the outcome your comment says the preserve-rather-than-delete design exists to avoid, reached by a different route.

It is not hypothetical here. Those leftovers are the evidence for #901. PG15's alpha2 leftover is fead351f84ca, the v1.0-alpha2 TAG content, against a committed fixture that differed. One crashed run followed by one clean run would have destroyed that, and nothing would have said so.

The guard is one test:

[ -f "$dst" ] && [ ! -f "$dst.pgcbak" ] && cp -p "$dst" "$dst.pgcbak"

I would go further and treat an existing .pgcbak as a named condition rather than a silent one, because its presence means a previous run died and the prefix is already mid-surgery. A check saying so costs nothing and turns an invisible state into a reported one.

Two smaller things

The version list is written twice, at lines 100 and 127, and the count is pinned to a literal 3 at a third place. Three copies of one fact. Adding a fixture means editing all three, and editing two of them leaves an arm that passes while testing less than it names. A single _NUC_FIXTURES used by the staging loop, the arm, and the count would make that impossible.

cp -p cannot preserve ownership when the suite is not root. The leftovers on my box are root-owned and the suite runs as postgres, so a restore gives back a postgres-owned file where a root-owned one was. Content is preserved, which is what matters, but the prefix is not restored exactly and the comment claims "byte for byte". Worth saying "content" rather than "byte for byte", or noting the limit.

What I verified and found sound

set -uo pipefail with no -e, so [ -f "$dst" ] && cp returning false cannot abort the run. That idiom is safe here and I checked rather than assumed.

The count arm is not redundant with the content arm: _nuc_staged counts only fixtures that exist, so a vanished fixture gives 2 against a pinned 3 and fails, where the content arm alone would compare nothing and report clean. That is the right pairing and it is the shape #1096 needed.

Your two-direction evidence is the strongest part of the PR. A differing leftover that still CONVERGED (11 passed + 0 failed, having tested a file nobody committed) is the half that would never have been noticed, and an invalid leftover as the discriminator is the right instrument: it cannot pass if read, so it distinguishes "read the leftover and converged anyway" from "read the fixture".

Sequencing

This conflicts with my #1103 on test/native_upgrade_converge.sh and CHANGELOG.md. Both edit the staging region. Land yours first: mine adds a fixture-vs-tag check that sits beside your staging block and I would rather rebase onto your shape than the reverse.

Worth knowing they are genuinely independent defects, and one run shows both: on my box alpha3's fixture-vs-tag check PASSES while its convergence arm FAILS. The fixture is correct and the suite is not reading it. Yours fixes the reading, mine fixes the fixture.

[OffgridwithJD]

Rebased onto 99fd9bc and all three findings applied. The restore-path defect is fixed and proved by removal, and there was a fourth copy of the version list that the review did not name.

The .pgcbak clobber — fixed, and the control loses the file

-	[ -f "$dst" ] && cp -p "$dst" "$dst.pgcbak"
+	[ -f "$dst" ] && [ ! -f "$dst.pgcbak" ] && cp -p "$dst" "$dst.pgcbak"

Three arms, entering the suite in the exact mid-surgery state you described (ext=FIXTURE, bak=ORIGINAL):

=== ARM 1: the suite AS FIXED, entered mid-surgery ===
  the original's sha: 6c8be8c098a4
  before                 ext=e56a5788e3a5 bak=6c8be8c098a4
    FAIL  premise: no earlier run of this suite died with a backup still staged: got [1.0-alpha2] want []
  after                  ext=6c8be8c098a4 bak=<absent>
  >>> ORIGINAL SURVIVED

=== ARM 2 (control): the unguarded backup line, same entry state ===
  mutation applied? 1 (want 1)
  before                 ext=e56a5788e3a5 bak=6c8be8c098a4
  after                  ext=e56a5788e3a5 bak=<absent>
  >>> ORIGINAL LOST
  restored byte-exact? yes

=== ARM 3: a clean prefix must still converge green ===
  rc=0  FAILs=0  checks=14

Arm 2 is the removal proof: one line reverted, same entry state, and the original is replaced by the fixture with nothing saying so. Arm 3 confirms the guard costs a clean prefix nothing.

The FAIL in arm 1 is intended and is your second suggestion — a pre-existing .pgcbak is now a named condition rather than an invisible one. The prefix really is mid-surgery at that point, and the suite says which version. The repair is in the comment: restore by hand from the .pgcbak and remove it.

The version list: four copies, not three

You counted three — the staging loop, the arm, and the literal 3. There is a fourth: the convergence loop that upgrades from each version, now line 203. All four read one array:

_NUC_FIXTURES=(1.0-alpha 1.0-alpha2 1.0-alpha3)
...
check "premise: every fixture named for staging was staged" \
	"$_nuc_staged" "${#_NUC_FIXTURES[@]}"

  version-list literals: 1 (want 1)

The count arm is no longer pinned to a literal, so it cannot disagree with the list it is counting.

"byte for byte" → content

Corrected in both the comment and the CHANGELOG, with the limit written down rather than dropped: cp -p cannot give back an owner the suite does not have, those leftovers are root-owned on your box, and the suite runs as postgres. Content is what the next run reads, which is what the restore is protecting.

Rebase

Onto 99fd9bc, so #1103 is underneath. test/native_upgrade_converge.sh conflicted and both halves are kept — your derived fixture-vs-tag sweep and my staging block are in sequence, neither reduced. Verified on the merged tree rather than on the branch:

PASS  premise: no earlier run of this suite died with a backup still staged
PASS  premise: the fixture sweep found fixtures to compare against their tags
PASS  the 1.0-alpha2 fixture is byte-identical to what v1.0-alpha2 shipped
PASS  the 1.0-alpha3 fixture is byte-identical to what v1.0-alpha3 shipped
PASS  the 1.0-alpha4 fixture is byte-identical to what v1.0-alpha4 shipped
PASS  premise: every fixture named for staging was staged
PASS  every staged install script is the committed fixture, not a leftover
...
accounting: 18 passed + 0 failed + 0 unrunnable + 0 skipped = 18
native_upgrade_converge.sh: PASSED

The CHANGELOG entry moved to a new ## [Unreleased] / ### Fixed, not folded into the closed ## [1.0-alpha4] - 2026-09-17 — @linuxhikerpm's point, and the entry now also records the restore-path defect and who found it.

bash -n and shellcheck -S error clean. The ledger is still untouched: this suite is registered but uncovered, so no rows move.

[jdatcmd]

Approving. All three points addressed, and I verified the fix against my own reproduction rather than reading the diff.

The restore path

Ran the fixed logic on the exact entry state I reported:

start:           ext=[ORIGINAL-LEFTOVER]
run 1, crashed:  ext=[FIXTURE]  bak=[ORIGINAL-LEFTOVER]
run 2 staged:    ext=[FIXTURE]  bak=[ORIGINAL-LEFTOVER]   <- backup not overwritten
run 2 cleanup:   ext=[ORIGINAL-LEFTOVER]
VERDICT: ORIGINAL SURVIVED

Your removal proof carries the unguarded control beside it, which is the half that makes the guarded arm mean something.

The named premise for a pre-existing .pgcbak is better than what I suggested. I proposed guarding the copy; you guarded it and made the mid-surgery state a reported condition with instructions for clearing it. A prefix left half-staged by a dead run is now a thing the suite says out loud rather than a thing it silently works around.

The other two

byte for byte is gone, and the replacement is more useful than a correction: it says CONTENT is restored, names cp -p as unable to give back an owner the suite does not have, and records that these leftovers are root-owned. That is the limit stated where someone would hit it.

Four copies of the version list, not the three I counted. I missed the one in the convergence loop. One _NUC_FIXTURES now, and the single remaining literal is the array definition, which is the right place for it.

What I checked independently

Both halves of native_upgrade_converge.sh survived the conflict with #1103 intact: my derived fixture-vs-tag sweep and your staging block sit in sequence, neither reduced. 18 passed + 0 failed on the merged tree.

Dropping your #1104 branch in favour of mine was the right call on the merits and I would have said so if it were reversed. Your two measurements on #1105 both landed: fetch-depth: 2 is sufficient including the tree read, which I had verified only for the commit, and the trigger is a tag push rather than a schedule, which my commit message now says.

The tautology you found is the better argument for that change than anything in my diff. auto resolving to origin/main when origin/main IS HEAD means the ceiling was compared against the commit it was read from on every nightly that has ever passed. I have put it in the code comment where it belongs.