tests: offline integration test decides NetworkMonitor state by fixed sleeps (flaky under load)

[christianparpart]

tests/test_offline_integration.cpp, "Integration: offline queue replayed and backend switched on network recovery", decides two outcomes of NetworkMonitor's probe thread by fixed sleeps rather than by waiting for them.

• Line 104–105: sleep_for(80ms), then REQUIRE_FALSE(monitor.isOnline()), with probeInterval = 30ms and failureThreshold = 1. The assertion holds only if the probe thread was scheduled and ran its first probe within 80 ms.
• Line 111–117: sleep_for(150ms), then REQUIRE(replayed.size() == 3) and REQUIRE(queue.drain().empty()). These hold only if the recovery probe, onOnline, SyncWorker::run() and switchBackend all finished within 150 ms.

On a loaded runner (ctest -j), neither bound is guaranteed.

Observed: Linux / all optional features (clang) on PR #806, head e984ebe1, failed at line 105 with REQUIRE_FALSE( monitor.isOnline() ) expanded to !true:
https://github.com/LASTRADA-Software/morph/actions/runs/36127304451/job/108046632860

The same leg passed on the previous head, and the gcc leg passed on this one. #806 does not touch include/morph/offline or this test, and NetworkMonitor runs its own std::thread/condition_variable.

Fix: the pattern #446 and #396 applied elsewhere. Wait for the state with morph::testing::waitUntil instead of sleeping for a fixed time:

• REQUIRE(morph::testing::waitUntil([&] { return !monitor.isOnline(); }));
• after networkOnline.store(true), REQUIRE(morph::testing::waitUntil([&] { std::scoped_lock lock{replayMtx}; return replayed.size() == 3; })), then the drain and execute checks as now.

The test then fails only when the monitor really does not go offline or recover, not when its thread is late.

[Yaraslaut]

Triage: triage: valid, area: offline, bug

Premise checked against the current code (revision master at the time of
this triage, tests/test_offline_integration.cpp). Lines match the issue
exactly:

103	    // Wait for monitor to detect offline (one probe interval + margin).
104	    std::this_thread::sleep_for(80ms);
105	    REQUIRE_FALSE(monitor.isOnline());
...
110	    // Wait for monitor to detect recovery and fire onOnline (one probe interval + margin).
111	    std::this_thread::sleep_for(150ms);
112
113	    // All queued items must have been replayed.
114	    {
115	        std::scoped_lock lock{replayMtx};
116	        REQUIRE(replayed.size() == 3);
117	    }
118	    REQUIRE(queue.drain().empty());

NetworkMonitor::run() (include/morph/offline/network_monitor.hpp) blocks on
_cv.wait_for(lock, _cfg.probeInterval, …) before its first probe, so both
REQUIREs above are racing OS thread-start/scheduling latency against a fixed
wall-clock margin, exactly as described.

CI failure re-examined, not just cited. Pulled the actual log for the link
in the issue (run 36127304451, job 108046632860, PR #806 head e984ebe1):

tests/test_offline_integration.cpp:105: FAILED:
  REQUIRE_FALSE( monitor.isOnline() )
with expansion:
  !true

That run was a 3119-test ctest -j pass on a shared Linux runner — consistent
with the "scheduling delay under load" explanation rather than a functional
break. #806 itself touches neither include/morph/offline nor this test, and
the gcc leg of the same commit passed, and the previous head passed on the
same clang leg — so the flake correlates with runner load, not with the
change under test.

Reproduced the failure mode directly, rather than trusting the log alone.
Standalone repro (NetworkMonitor header included directly, no test
framework): same probeInterval=30ms, failureThreshold=1, sleep_for(80ms)
then check !monitor.isOnline(), against a busy-spin CPU-contention rig.

• 0 and 16 contention threads on this 8-core machine: 0/20 failures.
• 64 contention threads (8x oversubscription): reproduced — monitor.isOnline()
  was still true after the fixed 80ms sleep. Reproduced again on a second,
  independent run (64 threads/50 trials, and 96 threads/30 trials): one
  failure each, always on the first trial after contention starts (when the
  scheduler is most disrupted).
• In every one of those runs, the same state was checked with
  waitUntil(pred, 2000ms) instead of the fixed sleep, and it always
  succeeded — confirming the monitor does reach the correct state, just not
  within the fixed margin under load. This is a bound problem, not a
  functional one, exactly as the issue claims.

This is not a new pattern for this file. fb2fdf12 (#446, closing #396)
already converted this same test's last fixed-sleep site
(for (...) sleep_for(10ms) polling result.load()) to
REQUIRE(morph::testing::waitUntil(...)) — but #396's sweep was scoped to a
different named hazard (a poll REQUIRE aborting before a second, unrelated
invariant REQUIRE, mislabeling the failure) and explicitly did not touch the
two sites this issue names. tests/test_network_monitor.cpp already uses
exactly the fix this issue proposes for the same call
(REQUIRE(waitUntil([&] { return !mon.isOnline(); }));), so the corrected
form is an established, already-audited idiom in-tree
(tests/test_support.hpp's "liveness only" note above kDefaultWaitBudget),
not a new one.

Architecture check: no invariant is implicated beyond the general "never
brace a liveness fact with a fixed sleep" rule this repo already encodes in
waitUntil's own doc comment and its audit trail. The fix is test-only,
doesn't touch include/morph/offline, and doesn't change what is asserted —
only how the assertion waits for it. bug because it is a real defect in
shipped test behaviour (a spurious CI failure), not a missing feature.

What would change this verdict: if the two sites turned out to be
protecting against a case waitUntil's default 2000ms budget can't cover, or
if NetworkMonitor's probe thread had some path where it never reaches
_cv.wait_for after construction (make the online→offline transition
undetectable rather than merely late) — neither was observed here in ~130
repro trials at up to 12x oversubscription.