Эх сурвалжийг харах

test(eviction): make test_priority_low_evicted_first deterministic

It failed ~10/12 runs in an unoptimised build and ~0/12 in Release. Not a
regression from this branch: at a fixed build type the bisect is flat, and the
merge base itself fails 10/12 at -O0. The deciding variable was the build
configuration, which is worth knowing because `cmake -B build -G Ninja` as
documented in CLAUDE.md sets no build type, so the documented command produces a
materially different binary from the released one. That is how three separate
measurements of this test disagreed.

The test called store.start() before filling, so the 20ms eviction tick raced its
own insert loop. At -O2 all 800 inserts land before the first tick; at -O0 only
~291 do, so eviction fires while `important` is still filling and `archive` does
not exist yet, evicts 150 from `important` alone, latches the v2.4.3 50%-per-
episode drain cap against that partial baseline and stays paused - after which
`archive` is filled to 400 and never touched.

The drain-cap latching is the v2.4.3 post-incident safety working to spec, so the
fix is in the test, not the product:

- start() moved after both fills, so eviction always sees 400 + 400.
- Preconditions asserted (both collections whole, pressure Hard or Emergency) so
  it cannot pass vacuously.
- sleep_for(500ms) replaced with poll-until-stable plus a checked 10s deadline.
- Converted to check()/g_fail with a real non-zero exit; main() previously
  returned 0 unconditionally, so a non-assert failure could not fail the suite.

No assertion loosened - the strict archiveLive < importantLive survives, and its
teeth were verified by inverting the two priorities (2 checks fail, exit 1).

0/12 failures at -O0 and 0/12 at Release. ctest 23/23.
fszontagh 1 сар өмнө
parent
commit
06baef7b88
1 өөрчлөгдсөн 111 нэмэгдсэн , 19 устгасан
  1. 111 19
      tests/test_eviction.cpp

+ 111 - 19
tests/test_eviction.cpp

@@ -25,6 +25,19 @@ using namespace smartbotic::database;
 
 namespace {
 
+// Shared pass/fail counters. New assertions use check() rather than assert()
+// so a failure names what it wanted and the binary keeps running to report
+// everything, instead of aborting on the first one. (The older tests in this
+// file still use assert(); tests/CMakeLists.txt applies -UNDEBUG so those are
+// live in every build type - see the v2.4.3 note there.)
+int g_pass = 0;
+int g_fail = 0;
+
+void check(bool cond, const char* msg) {
+    if (cond) { ++g_pass; }
+    else { ++g_fail; std::cerr << "FAIL: " << msg << "\n"; }
+}
+
 MemoryStore::Config evictionTestConfig(uint64_t maxMb = 1, uint32_t chunkSize = 100) {
     MemoryStore::Config cfg;
     cfg.nodeId = "test";
@@ -110,22 +123,48 @@ void test_hot_write_floor_protects_recent_writes() {
               << liveDocs << "/500 preserved)\n";
 }
 
+// Priority bias: with two equally-sized collections, eviction must take from
+// the Low-priority one before the High-priority one.
+//
+// This test USED to call store.start() before filling, then sleep 500ms and
+// compare survivor counts. That raced its own fill loop against the 20ms
+// eviction tick, and the outcome depended on how fast inserts happened to be:
+//
+//   - Optimised build: all 800 inserts land inside the first 20ms tick, so
+//     eviction sees both collections at 400 and the bias assertion holds.
+//   - Unoptimised build (a plain `cmake -B build -G Ninja`, i.e. no
+//     CMAKE_BUILD_TYPE, which is what CLAUDE.md documents): only ~290 inserts
+//     land in 20ms, so the first tick fires while "important" is still filling
+//     and "archive" is empty or absent. Eviction then evicts only from
+//     "important", latches the v2.4.3 50%-per-episode drain cap against that
+//     partial resident set, and stays PAUSED for the rest of the episode.
+//     "archive" is filled afterwards and never evicted at all, so
+//     archiveLive (400) > importantLive — a guaranteed failure that says
+//     nothing about the priority logic.
+//
+// Measured 10 failures in 12 runs unoptimised, 0 in 12 optimised, at the
+// relations merge-base as well as at branch HEAD. The product was never wrong;
+// the test's precondition was unstated.
+//
+// So the precondition is now established BEFORE the eviction thread exists:
+// fill both collections with the store stopped, assert the resident set is
+// exactly what the comparison assumes, and only then start eviction. The
+// settle wait polls until the live count stops moving instead of trusting a
+// fixed 500ms, and a timeout fails loudly rather than asserting on a
+// half-evicted store.
 void test_priority_low_evicted_first() {
-    // Use a 4 MB cap with target=50% so eviction frees down to ~2 MB,
-    // leaving survivors we can compare. With docs ~2.5 KB each and 400 docs
-    // total (~1 MB data + overhead), we'll be just over hard threshold and
-    // eviction will pick victims with Low priority first.
+    // 4 MB cap, target 25%: the fill lands well above the hard threshold so
+    // eviction has real work, and there are survivors left to compare.
     auto cfg = evictionTestConfig(4);
     cfg.hotWriteFloorMs = 0;           // disable hot-write protection
     cfg.evictionCheckIntervalMs = 20;
     cfg.memorySoftPercent = 30;        // early trickle
     cfg.memoryHardPercent = 50;        // hit hard with our fill
     cfg.memoryEmergencyPercent = 90;
-    cfg.evictionTargetPercent = 25;    // clear down to 25% — leaves headroom
+    cfg.evictionTargetPercent = 25;    // clear down to 25% - leaves headroom
     cfg.evictionChunkSize = 50;        // small chunks, biased selection has effect
     cfg.maxEvictionPassesPerTrigger = 10;
     MemoryStore store(cfg);
-    store.start();
 
     // Two collections: one HIGH priority, one LOW
     CollectionOptions highOpts;
@@ -136,25 +175,74 @@ void test_priority_low_evicted_first() {
     lowOpts.memoryPriority = MemoryPriority::Low;
     store.createCollection("archive", lowOpts);
 
-    fillStore(store, "important", 400, 2048);
-    fillStore(store, "archive", 400, 2048);
+    // NOTE: store.start() has deliberately NOT been called yet. Nothing in
+    // insert() needs the eviction or expiration thread, so the fill below runs
+    // with no concurrent evictor and the resident set is fully determined.
+    const uint64_t kPerCollection = 400;
+    fillStore(store, "important", static_cast<int>(kPerCollection), 2048);
+    fillStore(store, "archive", static_cast<int>(kPerCollection), 2048);
+
+    auto liveCounts = [&]() {
+        uint64_t important = 0, archive = 0;
+        for (const auto& c : store.getMemoryStatsSnapshot().collections) {
+            if (c.collection == "important") important = c.documentCount;
+            if (c.collection == "archive")   archive = c.documentCount;
+        }
+        return std::pair<uint64_t, uint64_t>{important, archive};
+    };
+
+    // Precondition, asserted rather than assumed: both collections are whole,
+    // and the store is over the hard threshold so eviction is guaranteed to
+    // run. If a future change makes the fill cheap enough not to trip
+    // pressure, this fails loudly instead of the test passing vacuously.
+    {
+        auto [important0, archive0] = liveCounts();
+        check(important0 == kPerCollection,
+              "precondition: 'important' fully resident before eviction starts");
+        check(archive0 == kPerCollection,
+              "precondition: 'archive' fully resident before eviction starts");
+        const MemoryPressure p0 = store.pressure();
+        check(p0 == MemoryPressure::Hard || p0 == MemoryPressure::Emergency,
+              "precondition: fill must put the store under hard/emergency pressure");
+    }
 
-    // Let eviction run several passes and settle
-    std::this_thread::sleep_for(std::chrono::milliseconds(500));
+    store.start();
 
-    auto stats = store.getMemoryStatsSnapshot();
-    uint64_t importantLive = 0, archiveLive = 0;
-    for (const auto& c : stats.collections) {
-        if (c.collection == "important") importantLive = c.documentCount;
-        if (c.collection == "archive")   archiveLive = c.documentCount;
+    // Settle: poll until the live count has stopped moving. Eviction pauses
+    // itself once it hits the drain cap or reaches the target, so this
+    // converges quickly; the deadline exists only so a hang reports rather
+    // than comparing a half-evicted store.
+    const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10);
+    uint64_t lastTotal = kPerCollection * 2;
+    int stableTicks = 0;
+    bool evictionRan = false;
+    bool settled = false;
+    while (std::chrono::steady_clock::now() < deadline) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(25));
+        auto [important, archive] = liveCounts();
+        const uint64_t total = important + archive;
+        if (total < kPerCollection * 2) evictionRan = true;
+        if (evictionRan && total == lastTotal) {
+            if (++stableTicks >= 8) { settled = true; break; }  // ~200ms of no change
+        } else {
+            stableTicks = 0;
+        }
+        lastTotal = total;
     }
+    check(evictionRan, "eviction must run at all given the fill exceeds the cap");
+    check(settled, "eviction must settle within the deadline");
+
+    auto [importantLive, archiveLive] = liveCounts();
 
     // LOW priority should be evicted at least as much as HIGH.
-    assert(archiveLive <= importantLive);
+    check(archiveLive <= importantLive,
+          "low-priority collection must not outlive the high-priority one");
     // Some eviction MUST have happened given our fill vs cap.
-    assert(importantLive < 400 || archiveLive < 400);
+    check(importantLive < kPerCollection || archiveLive < kPerCollection,
+          "some eviction must have happened");
     // Expect strict bias once any eviction has run.
-    assert(archiveLive < importantLive);
+    check(archiveLive < importantLive,
+          "low-priority collection must be evicted strictly more than high");
 
     store.stop();
     std::cout << "PASS: priority=Low collection evicted more than priority=High "
@@ -259,6 +347,10 @@ int main() {
     test_priority_low_evicted_first();
     test_eviction_chunk_size_honored();
     test_eviction_never_drains_the_store();
-    std::cout << "\nAll eviction tests PASSED!\n";
+    if (g_fail != 0) {
+        std::cerr << "\n" << g_fail << " check(s) FAILED (" << g_pass << " passed)\n";
+        return 1;
+    }
+    std::cout << "\nAll eviction tests PASSED! (" << g_pass << " checks)\n";
     return 0;
 }