diff --git a/core/src/session_types.cpp b/core/src/session_types.cpp index b9d3a5c..3db4387 100644 --- a/core/src/session_types.cpp +++ b/core/src/session_types.cpp @@ -11,8 +11,6 @@ You may obtain a copy of the License at #include "stplugin/session_types.h" -#include - namespace stplugin { const char *describePixelFormat(PixelFormat format) @@ -230,8 +228,27 @@ bool StallWatchdog::poll(std::chrono::steady_clock::time_point now) // 8s, ... up to max_backoff_ -- so a publisher that is genuinely gone // gets progressively less frequent toggles instead of one every 2 // seconds for the rest of the show. - next_attempt_allowed_ = now + backoff_; - backoff_ = std::min(backoff_ * 2, max_backoff_); + // + // max_backoff_ is clamped HERE, where the wait is used, and not only + // where the backoff is grown. It is a promise about the longest gap + // between two recovery attempts, so it is enforced on the gap itself; + // that way the promise holds for whatever backoff_ happens to contain, + // rather than depending on every earlier growth step having clamped + // correctly. A capped release build on Windows got that one step wrong + // (the ceiling engaged one attempt late, so a single 32s gap slipped + // past the 30s ceiling), which is exactly the kind of drift this + // clamp-at-use makes unrepresentable. + const std::chrono::milliseconds wait = backoff_ < max_backoff_ ? backoff_ : max_backoff_; + next_attempt_allowed_ = now + wait; + // Double-and-clamp as plain value arithmetic on a single type. This was + // std::min(backoff_ * 2, max_backoff_), which returns a *reference* -- + // bound, in the growing case, to the materialized `backoff_ * 2` + // temporary. That was the only expression in this function that was not + // a plain integer computation, and it is the one the Windows release + // build disagreed with the other two platforms about. Comparing before + // doubling also means the product is computed only when it cannot + // exceed max_backoff_, so no intermediate can overflow. + backoff_ = (wait > max_backoff_ / 2) ? max_backoff_ : wait * 2; return true; } diff --git a/core/tests/test_session.cpp b/core/tests/test_session.cpp index c11a0e0..e2bf809 100644 --- a/core/tests/test_session.cpp +++ b/core/tests/test_session.cpp @@ -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();