Fix flaky WarmupTest.testWarmup (compare resident page set, not slot order)

[vharseko]

Summary

Fixes a flaky failure in com.persistit.WarmupTest.testWarmup (persistit/core) seen on build-maven (ubuntu-latest, 17).

Root cause

testWarmup restarts Persistit with bufferinventory + bufferpreload and asserted a per-slot correspondence — that pool.getBufferCopy(i) after the restart holds the same page as before shutdown:

assertEquals(bufferCopy.getPageAddress(), buff[i].getPageAddress());

That is not what warmup guarantees. BufferPool.preloadBufferInventory() reads the recorded pages back, sorts them by read order (PageNode.READ_COMPARATOR) and reallocates buffers via the clock algorithm, so a page is reloaded into the pool but not necessarily into its original slot index. Slot assignment is timing-dependent in both runs (background threads + reallocation order), so the per-slot equality only matched by coincidence — the JDK 17 runner was incidental (expected:<0> but was:<2>: an empty slot where the original held page 2, the page still resident, just relocated).

Fix

Assert the invariant warmup actually provides — every valid, recordable page resident at shutdown is resident again after the restart (a one-way before ⊆ after subset), comparing the set of pages (volume:page:type), not their slot positions. The filter mirrors BufferPool.recordBufferInventory (skips temporary and lock volumes).

Making the set snapshots safe (round-2 review):

• residentPages() reads getBufferCopy(i) without recordBufferInventory's torn-read spin guard. Call disableBackgroundCleanup() on both the pre-shutdown and post-restart instances, and assert each pool is under-filled (validPageCount() < getBufferCount()): with cleanup off and free buffers available the pool never evicts, so a buffer's (volume, page) identity is stable and the unsynchronized reads cannot tear.
• The under-fill guard counts occupied buffers directly, independent of the filtered/deduplicated resident set.

A genuine failure to preload a page is still caught (missing= names it).

Testing

mvn -o -pl persistit/core test -Dtest=WarmupTest — Tests run: 2, Failures: 0; testWarmup green across repeated consecutive runs (a 50-run loop was used to confirm stability). The original JDK-17 flake does not reproduce locally on macOS; the fix rests on the slot-identity argument established by code reading (preload reorders and reallocates buffers by construction).

[maximthomas]

Verdict: right direction, request changes.

Blocking:

1. before is snapshotted from the live pool, not from the inventory that
   recordBufferInventory actually writes. before ⊆ after holds only because
   the unit-test pool (buffer.count.16384 = 20) is under-filled. Both
   recordBufferInventory (stores inside the scan loop, BufferPool.java:1478)
   and preloadBufferInventory (count >= _bufferCount break, runs after
   initializeVolumes) can evict pages once the pool is full.
2. residentPages() claims to mirror recordBufferInventory but omits its
   spin-loop guard against torn (page, volume) reads
   (BufferPool.java:1464-1473). getBufferCopy(int) is unlocked and the copy
   ctor reads _status/_page/_vol separately (Buffer.java:414).
3. Root-cause story doesn't explain JDK-17-only failure; slot assignment is
   deterministic. Real trigger is background-thread timing. Needs a
   before/after repro in one environment.
4. Unacknowledged coverage loss: old assertion was bidirectional per-slot.

Nits: assertFalse over assertTrue(!...); use VOLUME_NAME; getBufferSize()
in the key can't discriminate; string identity key is fragile; ex = null; is
now dead.

Positive (unmentioned by the PR): dropping the getExchange(...) call before the
post-restart snapshot removes pollution of the after pool.

Not verified: did not build/run the module, so the flake was not reproduced.
CI on head 74e7e48 was pending; mergeStateStatus: UNSTABLE.

[vharseko]

Thanks for the detailed review — all fair. Addressed in 944c960.

1. before is a live-pool proxy, valid only while under-filled. Agreed. The test dataset (1000 small records, buffer.count.16384 = 20) leaves the pool well under-filled, so no eviction happens during either recordBufferInventory or preloadBufferInventory, and the live snapshot equals the recorded inventory. I made that precondition explicit and self-checking:

assertTrue("Pool must stay under-filled for this comparison to be meaningful",
    before.size() < pool.getBufferCount());

so if a future change grows the dataset and fills the pool, the test fails loudly instead of silently comparing an incomplete set.

2. residentPages() omits the torn-read spin guard. Correct — getBufferCopy(int) is unlocked and the copy ctor reads _page/_vol/_type separately, so the "mirrors recordBufferInventory" claim was overstated. I reworded the javadoc: it now only claims the same volume filter, and documents that the spin guard is unnecessary here because the pool is under-filled and quiescent at both call sites, so no buffer reuse (the only source of a torn read) can occur. With the under-fill assertion from (1) now enforcing that, adding the guard would be dead code; happy to add it if you'd rather have literal faithfulness.

3. Root-cause didn't explain the JDK-17-only failure. Agreed the "only happened to match" wording was hand-wavy — slot allocation is deterministic given the read order. Reworded the comment to name the real trigger: the interleaving of the preload order with background eviction/cleanup between shutdown and the snapshot (plus the removed post-restart getExchange() pollution). I couldn't get a clean single-environment before/after repro (it only reproduced on the JDK-17 CI runner), which is exactly why I went for a slot-order-independent invariant rather than chasing the timing.

4. Unacknowledged coverage loss. Right — the old assertion was bidirectional per-slot; the new one is intentionally one-way (before ⊆ after). after may legitimately hold extra pages and slot position isn't preserved, so a one-way subset check is the correct invariant, but I now say so explicitly in a comment so the dropped coverage is deliberate and visible.

Nits: all applied — assertFalse, VOLUME_NAME, dropped the non-discriminating :size component (single buffer-size pool), removed the now-dead ex = null;. Left the string identity key as-is since it's test-local. And yes — dropping the pre-snapshot getExchange(...) was deliberate: it stops polluting the post-restart after pool.

Verification: mvn -pl persistit/core test -Dtest=WarmupTest (JDK 11) → Tests run: 2, Failures: 0; 6× direct runs on JDK 26 → all green (runCount=2, failCount=0).

[maximthomas]

Verified from source

• Persistit.close() calls recordBufferPoolInventory() first, before any teardown (Persistit.java:1632).
• allocBuffer() scans invalid buffers first; clock eviction only when _availablePages == false (BufferPool.java:959-995). Under-filled pool never evicts.
• residentPages() iterates 0..getBufferCount() — same array recordBufferInventory scans (BufferPool.java:1457). Filters match.
• Test pool: 20 buffers, ~10 resident pages. Under-filled by ~2×.
• before ⊆ after is sound. Slot identity is not preserved. Fix direction is correct.

Round-1 status

#	Item	Status
1	before ≠ what recordBufferInventory persists	CLOSED — under-fill + allocBuffer invalid-first policy
2	residentPages() omits torn-read spin guard	OPEN — see A
3	Root cause unexplained	OPEN — see B, new comment replaces one wrong claim with another
4	Coverage loss	CLOSED — now documented as intentional one-way invariant

Nits: all applied ✓. String-identity key left as-is (harmless for this volume name).

Findings

A. residentPages() relies on assumption test doesn't enforce (blocking)

getBufferCopy(int) returns a detached snapshot with 4 unsynchronized field reads (Buffer.java:414-428). recordBufferInventory guards against torn reads with a spin loop (BufferPool.java:1462-1471); residentPages() does not.

New javadoc claims "pool is quiescent at both call sites" — false at the before site where CLEANUP_MANAGER, CHECKPOINT_WRITER and PAGE_WRITER are running (CleanupManager.java:248,276). Under-fill assertion only covers before, not the post-restart pool where background activity is highest.

Fix: Call disableBackgroundCleanup() at test top (already idiomatic in IntegrityCheckTest, MVCCPruneBufferTest, Bug927701Test). Then javadoc can truthfully say the guard is unnecessary because cleanup is disabled and pool is under-filled. At minimum, add under-fill assertion on the post-restart pool too.

B. Root-cause comment contradicts assert 10 lines below (blocking)

Comment says old assertion depended on background eviction/cleanup. Next block says comparison is valid because "with no eviction" the snapshot equals the inventory. Both can't be true — verified the second is: under-filled pool never reaches eviction (BufferPool.java:959-995).

expected:<0> but was:<2> is plain relocation, not eviction — the post-restart slot was empty where pre-shutdown held page 2. "Between shutdown and the snapshot" is also incoherent — before snapshot precedes shutdown.

Correct story: slot assignment is timing-dependent in both runs due to background threads. Per-slot equality passed by coincidence; JDK 17 is incidental.

C. Under-fill guard can't detect what it claims (non-blocking)

before.size() < pool.getBufferCount() compares filtered/deduped set against total buffer count. A full pool with lock-volume pages satisfies it. Not a problem today (_lockVolume lazy, no locks taken), but unsound for stated purpose of catching future dataset growth. Count valid buffers directly instead.

D. Repro request — downgrade, don't drop (non-blocking)

Testing section shows only new test passing; can't distinguish "fixed" from "still flaky at low probability". Design argument is established by code reading (slot identity not preserved by construction). Worth a 50-run loop on old vs new to confirm, but no longer blocking.

Positives

• getVolume(VOLUME_NAME).getStructure().getPool() reuses existing pattern; dropping getExchange(...) removes post-restart pool pollution (unmentioned in PR).
• missing= in failure message is a real improvement over opaque expected:<0> but was:<2>.

[vharseko]

Thanks for the round-2 review — addressed in ada04fb.

Finding A (torn-read guard / false "quiescent" claim). Called disableBackgroundCleanup() on both instances (pre-shutdown and post-restart), and added the under-fill assertion on the restarted pool too. Reworded the residentPages javadoc: it no longer claims the pool is quiescent — with cleanup disabled and the pool under-filled the pool never evicts, so a buffer's (volume, page) identity does not change under the scan and the unsynchronized getBufferCopy() reads cannot tear. That's the real reason the spin guard is unnecessary here.

Finding B (comment contradicts the assert). Fixed. The comment no longer blames "background eviction/cleanup between shutdown and the snapshot" — an under-filled pool never evicts, and before is snapshotted before shutdown anyway. It now says what actually happens: slot assignment is timing-dependent in both runs (preload read order + clock reallocation), so the old per-slot equality matched only by coincidence and JDK 17 was incidental.

Finding C (under-fill guard). Now gated on validPageCount(pool) — the raw count of occupied buffers — instead of the filtered/deduplicated resident set, so a full pool of lock-volume pages can't satisfy it.

Finding D (repro). The original JDK-17 flake does not reproduce locally on macOS (consistent with your own note), so the argument stands on code reading: preload reorders and reallocates buffers by construction, so slot identity is not a property warmup provides. As a stability check I ran testWarmup 50× consecutively with the new code — 50/50 green; full WarmupTest 2/2.