test: the port walk wraps instead of walking off the ceiling (#548)

[ChronicallyJD]

Closes #548. Analysis and issue by @ChronicallyJD. Reproduced here with a
forced PID before anything was changed, because this is shared harness code and
arithmetic alone is not a demonstration. I am not merging it.

Reproduced

Band [29768,31768), width 2000, nothing listening:

base at HI-1 (31767):  SB=31767, RS increments to 31768, hits the bound
                       -> FAIL no free port for the restore, 1,999 ports free
base at HI-2:          ok
mid band:              ok

pick_sb_port seeds base from $$. Both draws return the same port by
construction — $$ inside $( ) is the invoking shell's PID, not the
subshell's — which is why the loop opens by testing for that collision. It then
incremented and hard-failed at PGC_AUX_PORT_HI instead of wrapping to LO.

About 1 replication run in 2000; across five majors roughly 1 CI run in 400,
which matches the observed rare, unattributable red that moved between majors.
It took down #545's PG17 leg on a diff containing nothing but CONTEXT.md.

Not a sizing problem, so the band is untouched

The failure needs a base within one port of the ceiling, and that stays a fixed
fraction of the width whatever the width is. A 20,000-port band fails
identically, just less often, and still with the whole band free beneath it.

Both halves

The walk wraps, and it is bounded by the band width so a genuinely full
band reports itself full rather than spinning forever. The message now
distinguishes sweeping the whole band from walking off the end of it — those want
different responses from whoever reads the log, and the old one asserted the
first when only the second had been established. That is #537's defect in a
different file.

pick_sb_port was a second copy of pgc_pick_free_port carrying the same bug.
It now delegates. Two copies of a walk is how one of them gets fixed.

The tests were vacuous first, and my own removal proof caught it

harness_selftest 54 → 61.

The first version PASSED with the no-wrap walk restored — 60 of 60 against the
defect. Both reasons were mine:

• I asked the picker for a port with the band empty, where the old code also
  succeeds. A truncated scan only fails when the top of the band is busy.
• The "wraps to the floor" check computed the wrap from PGC_AUX_PORT_LO and
  HI directly without calling the picker at all — a tautology over my own
  expression.

Rewritten to stub pgc_port_free so the top 400 are busy and the rest free. A
walk that stops at hi finds nothing from a base in that region; a walk that
wraps lands below it.

Proved by removal — restoring the old walk fails by name:

FAIL  a seed at the ceiling wraps past a busy top and still finds a port
FAIL  and the port it found is below the busy region, which is where wrapping lands

A full-band stub proves termination rather than a hang, and a premise asserts the
real prober was restored, since a stub left installed would make every later
check lie.

One that nearly shipped

I wrote the portlib comment in C style out of habit and bash -n passed
it. The * lines are globs and */ is a word, so it parses cleanly and would
have executed as commands inside the function at runtime, breaking port selection
outright. A syntax check cannot see that a comment is not a comment. Caught by
reading the rendered function rather than trusting the parse.

Gate

PG17 assert 132 ran, PASS. PG19 assert 137 ran, one failure — temporal,
btree_gist absent from this container, identical on unmodified main. Gated on
the full set rather than on units because portlib is in the path of every suite.

[ChronicallyJD]

I filed #548, so this is a review and not a self-approval. The wrap is correct, the deduplication is the best part of it, and there is one finding: this PR fixes the message-honesty defect in the walk it rewrote and leaves the identical defect in the picker it rewrote, in the same file, in the same commit.

Correct, and I checked the arithmetic rather than the intent

p=$(( lo + ((base - lo + i) % width) ))

wraps properly for any base in [lo, hi), and span=300; [ "$width" -lt "$span" ] && span=$width stops a narrow band from scanning past itself. The width <= 0 guard makes a degenerate band return rather than divide by zero.

pick_sb_port delegating instead of carrying a second copy of the walk is the part that matters most, and more than the wrap: the duplicate is why there were two defects to fix. Two copies of a walk is how one of them stays broken.

The replication walk is exactly what #548 asked for, both halves:

_rs_width=$(( PGC_AUX_PORT_HI - PGC_AUX_PORT_LO ))
...
if [ "$_rs_tries" -ge "$_rs_width" ]; then
        echo "FAIL  ... swept all $_rs_width ports in [...] and every one was busy or the standby's"

Bounded by the band width, and the message states what was actually established. That is the right standard.

The finding: the picker is held to a different one

pgc_pick_free_port is bounded at 300, not at width. On the 2000-port aux band it probes 15% and returns failure — and its caller then says:

echo "FAIL  no free port for the restore cluster in [$PGC_AUX_PORT_LO,$PGC_AUX_PORT_HI)"

That message names the whole band on the strength of 300 probes. It is the same sentence the walk fifteen lines below it was just corrected for: asserting a sweep that did not happen.

The comment above the bound argues for a bound — "without it a genuinely full band spins forever rather than reporting itself full" — and I agree. It does not argue for 300 over width, and the PR chooses width in one place and 300 in the other without saying why they differ.

Both resolutions are fine and they are not the same:

1. Scan the width. Consistent with the walk, and the caller's message becomes true as written. Costs up to 2000 TCP probes on a genuinely full band, which only happens when something is badly wrong anyway.
2. Keep 300 and make the message honest — have the picker report how many it probed, so the caller can say "no free port in 300 probes from N" rather than naming a band it never swept.

I would take (1) for the aux band, because the expensive case is the one that already indicates trouble, and because one rule is easier to keep than two. But (2) is defensible and cheaper; what is not defensible is the current pairing, where the honest message sits next to the one this PR was filed to fix.

On the checks

Your removal proof is the right shape and the stub is the reason it works — the defect is unobservable with the band empty, because the old code succeeds there too, which is why your first version passed against it. Stubbing pgc_port_free so the top 400 are busy is the only condition under which a no-wrap walk can be seen to fail, and asserting the real prober was restored afterwards closes the hole where a leftover stub makes every later check lie.

I have not re-run them; the reasoning is checkable from the diff and you reproduced the original defect by forcing the PID, which is more than I did when I filed it.

Not blocking

The flake is fixed either way — with the wrap in place, failure now needs 300 consecutive busy ports rather than one unlucky seed. The finding is about what the failure will say when it happens, which is the thing #537 exists to protect and which this PR otherwise upholds carefully.

I have not merged this and will not.

[ChronicallyJD]

Finding closed. Verified at 5d1bbe4 by running it, including a removal proof of my own finding rather than reading the diff.

The picker sweeps the band now — span=$width — and the caller messages say "the picker swept the whole band", which is true only because it now does. That was the whole finding: the message asserted a sweep that had not happened.

Removal proof, re-imposing the old 300-probe budget on the current tree:

FAIL  a free port beyond the old 300-probe bound is still found: got [NONE] want [30268]

Exactly the check that pins it, and nothing else. Baseline on the same tree, run without PGC_SKIP_BUILD so the build path actually runs: harness_selftest 63 PASSED, replication 41 PASSED.

Measuring 191 ms against 856 ms before choosing the full sweep is better than the reasoning I offered when I raised it — I asserted the cost was acceptable without checking, and the argument that settles it is yours: a full sweep is only ever paid on the failure path, because the picker returns on the first free port.

The discriminating test is the part I would have got wrong. A second wrap check passes on the old 300-bound code; one free port stubbed 500 past the floor separates the two properties, so the wrap and the bound are pinned independently rather than by one check that happens to cover both.

No objections. I have not merged this and will not.