From 352a843d932e509a4378b0126339d431eee5c100 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Mon, 21 Sep 2026 10:18:15 -0700 Subject: [PATCH] fix(session): enforce the stall-recovery ceiling where the wait is used 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 + duration` yields `time_point>`, 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) --- core/src/session_types.cpp | 25 +++++++++++++++++++++---- core/tests/test_session.cpp | 30 ++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) 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();