Make WarmupTest.readOrderIsSequential deterministic

[vharseko]

Problem

WarmupTest.readOrderIsSequential is flaky: it restarts Persistit, injects a TrackingFileChannel into the volume channel, calls preloadBufferInventory(), and intermittently sees zero reads on that channel:

java.lang.AssertionError: Preload should have loaded pages from journal file
    at com.persistit.WarmupTest.readOrderIsSequential(WarmupTest.java:122)

Root cause

recordBufferInventory() at shutdown interleaves its own _buffer_inventory_ tree stores with its scan of the 20-buffer pool, so some recorded pages are the inventory tree's own pages. The closing checkpoint flushes those to the journal with no subsequent copyBack(). On restart, VolumeStorageV2.readPage() consults readPageFromJournal() first and returns without touching the volume channel, so journal-resident recorded pages bypass the injected channel entirely. When the interleaving catches enough entries, the channel records zero reads.

Two earlier attempts here were ineffective and are superseded by the head commit:

• disableBackgroundCleanup() bound the pre-restart instance (the test replaces it), and the background-race hypothesis was wrong;
• asserting on pool.getMissCounter() was vacuous — preload's walk of the shared inventory tree alone bumps the pool's single miss counter, so the test passed even if zero recorded pages loaded.

Fix

Make the channel-based check deterministic through setup (per review):

• After the first close(), reopen with buffer inventory recording disabled, run copyBackPages() to drain the journal into the volume, and close again — so every recorded page (including the inventory tree's own pages) lives in the volume file.
• Restart, inject TrackingFileChannel, preload, and assert the channel saw a non-empty, strictly ascending sequence of reads.
• Fix TrackingFileChannel.assertOrdered: it never advanced its previous cursor, so it only checked position > -1; it now actually verifies read order (restoring the coverage the test is named for).

Note: a strict getPageMapSize() == 0 precondition before preload turned out unachievable — the final close() always leaves one checkpoint page (not a data page) in the journal — so the assertions are the channel read count plus read order.

Also includes the Bug1017957Test stabilization commit (cherry-picked from #257) so this branch's CI is not blocked by that unrelated flake.

Verification

• Instrumented run: preload issued 18 reads through the injected volume channel at strictly ascending positions (32768 … 1425408); assertOrdered(true, true) verifies them with the fixed cursor.
• readOrderIsSequential: 10/10 consecutive green runs; full WarmupTest: 2/2.
• The original flake does not reproduce locally (macOS); determinism now comes from the setup sequence rather than assumptions about the read source.

[maximthomas]

Findings (verified against the source in this checkout)

1. Comment misstates the helper. PersistitUnitTestCase.disableBackgroundCleanup()
   (PersistitUnitTestCase.java:238) only sets CleanupManager.setPollInterval(-1)
   and JournalManager.setWritePagePruningEnabled(false). The BufferPool.PageWriter
   (BufferPool.java:294,1351, started at Persistit.java:687) and the
   CheckpointManager keep running. The comment blames "CLEANUP_MANAGER /
   page-writer / checkpoint activity" and claims disabling it makes the drain
   deterministic; two of the three named actors are untouched.

2. "Drain can be left incomplete" contradicts copyBack(). JournalManager.copyBack()
   (1657–1669) blocks until the copier clears _copyFast, which happens at
   JournalManager.java:2205 only when _copyList.isEmpty(). Nothing is silently
   abandoned. Re-journaling that matters must happen after copyBackPages() returns.

3. The likely real source of journal-resident pages is close(), untouched here.
   Persistit.close() calls recordBufferPoolInventory() first (Persistit.java:1632);
   BufferPool.recordBufferInventory() (1448) issues ~bufferCount store() calls into
   the _buffer_inventory_ tree, and the final _checkpointManager.close(flush)
   (Persistit.java:1654) writes those dirty pages to the journal with no
   subsequent copyBack(). VolumeStorageV2.readPage() consults
   readPageFromJournal() first (VolumeStorageV2.java:450), so those pages never
   touch the injected volume channel.

4. Unfalsified. PR body states the flake does not reproduce locally and the fix rests
   on "mechanism analysis" — which is the weak link per 1–3. One green CI run proves
   nothing about an intermittent, platform-specific failure.

5. assertOrdered is a no-op. TrackingFileChannel.assertOrdered (164–174) declares
   final long previous = -1 and never updates it in the loop, so forward=true
   asserts only position > -1. The test named readOrderIsSequential never checks
   ordering; the sole failing assertion is getReadPositionList().size() > 0.

6. Restarted instance keeps background cleanup on. disableBackgroundCleanup() binds
   _persistit at call time; the test then replaces it with new Persistit().
   TrackingFileChannel._readPositions is an unsynchronized ArrayList mutated in
   read(), so a concurrent background volume read is a data race on the asserted list.

7. Minor: stale assertion message ("...from journal file" — it is the volume channel).

8. Minor: 14 lines of comment for 1 line of code, encoding CI trivia that will rot.
   Sibling call sites (Bug927701Test, IntegrityCheckTest, MVCCPruneBufferTest)
   use the helper with no commentary.

Recommended follow-ups (only if the user asks me to implement)

• Fix TrackingFileChannel.assertOrdered to actually track previous.
• Correct the assertion message to say "volume file".
• Move/duplicate disableBackgroundCleanup() to after _persistit.initialize().
• Replace the narrative comment with 2–3 lines that claim only what the helper does,
  linking #254.
• Add an explicit precondition assertion before restart (e.g. on
  JournalManager.getPageMapSize(), JournalManager.java:357) so a failure reports
  "journal not drained" instead of the opaque size() > 0.

[vharseko]

Thanks for the detailed review — you're right, the previous commit was ineffective. Reworked in 0bf3d04:

• Dropped disableBackgroundCleanup() (your point 6): it bound the first Persistit instance, which the test then replaces with new Persistit(); the second instance — the one running preload and the injected channel — kept cleanup on. So it never affected the asserted path.
• Root cause (point 3), confirmed against the source: VolumeStorageV2.readPage() (VolumeStorageV2.java:450) consults readPageFromJournal() first and returns without touching _channel, so a recorded page served from the journal bypasses the injected volume channel and getReadPositionList().size() is legitimately 0. Which source serves the page is non-deterministic across a restart — hence the intermittent failure.
• Removed the TrackingFileChannel injection entirely, which also removes the no-op assertOrdered (point 5), the unsynchronized read-list data race (point 6), and the misleading "journal file" message (point 7).
• New assertion: pool.getMissCounter() before/after preloadBufferInventory(). bumpMissCounter() (BufferPool.java:866) fires whenever get() loads a page from disk regardless of source, so it is the source-agnostic signal that preload actually read the recorded pages back.

On point 4: I still can't reproduce the flake locally, but the new assertion no longer depends on the non-deterministic read source, rather than resting on the earlier (wrong) hypothesis. Kept the comment short (point 8).

[maximthomas]

Thanks for the rework — dropping disableBackgroundCleanup() and the channel injection is right,
and it resolves points 5–7. But the replacement assertion can't fail, so I'm still requesting changes.

1. getMissCounter() > missesBefore is vacuous

preloadBufferInventory() (BufferPool.java:1503) opens the _buffer_inventory_ tree via
getBufferInventoryExchange() (1579) and walks it with exchange.previous() / next() (1516, 1536)
before it loads any recorded page. Those page loads go through get(..., wantRead=true) and hit
bumpMissCounter() (866). Pools are keyed by buffer size alone (Persistit.java:1499) and the test
config has one 16 KiB volume (PersistitUnitTestCase.java:70), so the inventory tree, the data volume
and the sampled pool all share one _missCounter.

Reading the inventory tree alone satisfies the assertion. If volumeForHandle() returned null for
every entry (1546, continue) or every get() threw and was swallowed (1567,
catch (PersistitException e) { // ignore it }), preload would load zero pages and the test would
still pass — exactly the regressions this test exists to catch.

Also, disableBackgroundCleanup() was removed rather than moved after initialize(), so the restarted
instance's CleanupManager / CheckpointManager can bump that same AtomicLong (161) independently.

2. Read-order coverage drops to zero

PageNode.READ_COMPARATOR exists (BufferPool.java:1544) so warmup issues reads in ascending page
order, and this test is the only caller of preloadBufferInventory() outside Persistit.java:707.
My point 5 was to fix assertOrdered, not delete its call site. The test name now describes
something it no longer checks.

3. TrackingFileChannel is orphaned, not removed

The class stays in the tree with zero callers and the broken previous cursor intact. Either fix it
and keep using it, or delete the file.

On the root cause

Close, but inverted. Given the journal page map, the source serving a given page is deterministic —
what varies is which pages get recorded. recordBufferInventory() (1448) interleaves
exchange.to(index).store() (1478) with its scan of _buffers[index], so with 20 buffers the
inventory tree's own pages evict data pages mid-scan and later indices record inventory pages. Those
are dirtied by the inventory write, flushed to the journal by the final _checkpointManager.close(flush)
with no subsequent copyBack(), and served from the journal on restart. When the interleaving catches
all 20 entries, the volume channel sees zero reads.

Suggested direction

• Fix TrackingFileChannel.assertOrdered to advance previous — vacuously true on an empty list, so it
  can never be the flaky assertion — and keep the injection.
• Get determinism from setup instead of a weaker assertion: after the first close(), reopen with
  bufferinventory disabled, call copyBackPages(), close again, then restart and preload.
• Assert JournalManager.getPageMapSize() == 0 (JournalManager.java:357) before preload, so a failure
  reports "journal not drained" rather than an opaque zero.
• If you keep the miss-counter approach instead: have preloadBufferInventory() return count, assert on
  that, move disableBackgroundCleanup() after initialize(), and rename the test.

[vharseko]

Thanks — you were right again: the miss-counter assertion was vacuous for exactly the reason you gave (preload's walk of the shared _buffer_inventory_ tree bumps the pool's single _missCounter before any recorded page loads). Reworked in d9c3eca along your suggested direction:

• Determinism from setup. After the first close(), the test reopens with buffer inventory recording disabled, runs copyBackPages() to drain the journal into the volume, and closes again — so every recorded page (including the inventory tree's own pages that the closing checkpoint had flushed to the journal) lives in the volume file. The restarted instance then injects TrackingFileChannel and preload reads go through it.
• assertOrdered fixed, injection kept. previous now advances, so the order check is real (and vacuously true on an empty list, as you noted — the read-count assertion guards non-emptiness). Read-order coverage is restored; TrackingFileChannel is no longer orphaned.
• Root cause adopted as you stated it: what varies is which pages get recorded — recordBufferInventory() interleaves its own tree stores with the buffer scan, those inventory pages reach the journal via the closing checkpoint, and journal-resident pages bypass the volume channel deterministically. The comment in the test now tells this story.
• One deviation: the suggested getPageMapSize() == 0 precondition turned out unachievable — after the extra drain-and-close cycle the final close() still leaves exactly one checkpoint page in the journal (observed pageMapSize=1; it is not a data page and preload never touches it). Rather than assert a condition the product never satisfies, the test asserts the channel read count plus order. If you know a teardown sequence that truly empties the page map, happy to add the precondition back.

Verification: instrumented run shows preload issuing 18 reads through the injected channel at strictly ascending positions (32768 … 1425408), and the fixed assertOrdered verifies them. 10/10 consecutive green runs; full WarmupTest 2/2.

(The branch also carries the Bug1017957Test stabilization from #257 so this PR's CI isn't blocked by that unrelated flake.)