fix(session): enforce the stall-recovery ceiling where the wait is used
Build / macOS (macos-latest) (push) Successful in 53s
Build / Linux (ubuntu-24.04) (push) Successful in 1m24s
Release / macOS (macos-latest) (push) Successful in 55s
Release / Linux (ubuntu-24.04) (push) Successful in 1m18s
Build / Windows (windows-latest) (push) Failing after 3m41s
Release / Windows (windows-latest) (push) Failing after 3m39s
Release / Create Gitea Release (push) Skipped
Build / macOS (macos-latest) (push) Successful in 53s
Build / Linux (ubuntu-24.04) (push) Successful in 1m24s
Release / macOS (macos-latest) (push) Successful in 55s
Release / Linux (ubuntu-24.04) (push) Successful in 1m18s
Build / Windows (windows-latest) (push) Failing after 3m41s
Release / Windows (windows-latest) (push) Failing after 3m39s
Release / Create Gitea Release (push) Skipped
The Windows release build failed testStallWatchdogBacksOffRatherThanLooping (11/124 checks) while Linux and macOS passed, so v0.1.1 never published. Reconstructing the failure from the log rather than guessing: the reported FAIL lines (line 329 first, then 327/329 alternating for the rest of the capped-backoff loop, 11 of the loop's 12 checks) are produced by exactly one behaviour, and the deliberately-broken build in this commit's verification reproduced that log byte-for-byte on Linux -- the backoff ceiling engaged one attempt LATE. Windows waited 32000ms once (the uncapped doubling of 16000ms) before settling at the 30000ms ceiling. Every other candidate produces a different count and a different order: an exact-equality boundary bug gives 6 failures, and a ceiling that never engages at all gives 8, neither matching. That rules out the obvious suspect, a lossy duration conversion. There isn't one, and there cannot be: `time_point<Clock, D1> + duration<D2>` yields `time_point<Clock, common_type_t<D1, D2>>`, and converting that back to `steady_clock::time_point` to store it in next_attempt_allowed_ only compiles when the conversion is exact. If MSVC's steady_clock could not represent a whole millisecond exactly, this file would not build there. All of the watchdog's time arithmetic is exact integer arithmetic on every platform, and the exact-equality comparison at the deadline is sound -- the Windows log itself shows later polls firing at exactly their deadline. What is left is `std::min(backoff_ * 2, max_backoff_)`: the one expression in poll() that was not plain value arithmetic on a single type, returning a *reference* bound, in the growing case, to a materialized temporary. So: - The ceiling is now clamped where the wait is USED, not only where the backoff is grown. max_backoff_ is a promise about the longest gap between two recovery attempts, so it is enforced on the gap itself and holds for whatever backoff_ contains. Verified: with the growth step deliberately mis-capping exactly the way Windows did, the whole suite still passes -- the fix does not depend on having correctly identified MSVC's mechanism. - The doubling is an explicit compare-and-clamp instead of std::min, so no reference to a temporary is involved and the product is only computed when it cannot exceed the ceiling. Both changes are provably no-ops on Linux and macOS, where backoff_ never exceeded the ceiling in the first place. Also pins the behaviour with a new regression test using a ceiling that is NOT a power-of-two multiple of the timeout (1000 -> 2000 -> 4000 -> 5000), which fails on the step the ceiling first binds rather than six 30-second iterations later. 146 checks in test_session now, was 124; all 6 CTest suites pass locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -339,6 +339,35 @@ void testStallWatchdogBacksOffRatherThanLooping()
|
||||
ST_ASSERT_EQ(w.attemptsThisStall(), 1);
|
||||
}
|
||||
|
||||
// The ceiling has to engage on the FIRST attempt whose doubled backoff would
|
||||
// exceed it, not one attempt later -- a Windows release build got exactly
|
||||
// that step wrong (it waited 32s once before settling at the 30s ceiling),
|
||||
// which is why StallWatchdog::poll() clamps the wait where it is used rather
|
||||
// than trusting every growth step. A cap that is NOT a power-of-two multiple
|
||||
// of the timeout pins the clamp itself: 1000 -> 2000 -> 4000 -> 5000 (not
|
||||
// 8000, and not 4000 again), and 5000 forever after.
|
||||
void testStallWatchdogNeverWaitsLongerThanTheCeiling()
|
||||
{
|
||||
const auto t0 = std::chrono::steady_clock::now();
|
||||
StallWatchdog w(std::chrono::milliseconds(1000), std::chrono::milliseconds(5000));
|
||||
w.setExpectingFrames(true, t0);
|
||||
|
||||
auto now = t0 + std::chrono::milliseconds(1000);
|
||||
ST_ASSERT(w.poll(now));
|
||||
|
||||
// Expected gaps between consecutive attempts: 1000, 2000, 4000, then the
|
||||
// ceiling for good. Each gap is checked on both sides of its boundary, so
|
||||
// a gap that is even one millisecond too long or too short fails here.
|
||||
const int expected_gaps[] = {1000, 2000, 4000, 5000, 5000, 5000, 5000};
|
||||
int attempt = 1;
|
||||
for (int gap : expected_gaps) {
|
||||
ST_ASSERT(!w.poll(now + std::chrono::milliseconds(gap - 1)));
|
||||
now += std::chrono::milliseconds(gap);
|
||||
ST_ASSERT(w.poll(now));
|
||||
ST_ASSERT_EQ(w.attemptsThisStall(), ++attempt);
|
||||
}
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Real SDK, failure paths only (no LiveKit server available headlessly)
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -468,6 +497,7 @@ int main()
|
||||
testStallWatchdogFiresAfterThreshold();
|
||||
testStallWatchdogDoesNotFireWhenNotExpectingFrames();
|
||||
testStallWatchdogBacksOffRatherThanLooping();
|
||||
testStallWatchdogNeverWaitsLongerThanTheCeiling();
|
||||
|
||||
LiveKitSession::globalInitialize();
|
||||
testConnectRejectsIncompleteConfig();
|
||||
|
||||
Reference in New Issue
Block a user