Merge branch 'main' into chrisuthe/task/restore-direct-address-dialling-to-s-alongside

Author: chrisuthe

src/alsa_sink.cpp

@@ -717,7 +717,7 @@ void AlsaAudioSink::poll(int64_t now_ms) {
     }
     if (this->stopping_.load()) {
         // stop() can land during the open above; release the device now.
-        this->recovery_.rescan_done(true);  // shutting down; there is nothing left to retry for
+        this->recovery_.rescan_abandoned();  // shutting down; there is nothing left to retry for
         this->close_device_();
         return;
     }

src/pipewire_sink.cpp

@@ -560,7 +560,7 @@ void PipeWireSink::poll(int64_t now_ms) {
     }
     if (this->stopping_.load()) {
         // stop() can land during the reconnect above; release the stream now.
-        this->recovery_.rescan_done(true);  // shutting down; there is nothing left to retry for
+        this->recovery_.rescan_abandoned();  // shutting down; there is nothing left to retry for
         this->close_stream_();
         this->stop_loop_();
         return;

src/portaudio_sink.cpp

@@ -577,9 +577,6 @@ void PortAudioSink::poll(int64_t now_ms) {
     if (!this->recovery_.rescan_due(now_ms)) {
         return;
     }
-    // Reported up front and always as recovered: a device-list rebuild is one-shot.
-    this->recovery_.rescan_done(true);
-
     const StreamFormat format = this->last_format_;
     // The stream goes first whatever happens next: Pa_Terminate() with one open is undefined,
     // and every PaDeviceIndex -- device_index_ among them, which this clears -- dies with it.
@@ -592,6 +589,7 @@ void PortAudioSink::poll(int64_t now_ms) {
         // PortAudio is down, so the sink is inert until it comes back.
         cli_log(LogLevel::ERROR, "portaudio: could not restart PortAudio to look for '%s': %s",
                 this->name().c_str(), this->pa_.error());
+        this->recovery_.rescan_abandoned();  // a device-list rebuild is one-shot
         return;
     }
 
@@ -602,16 +600,20 @@ void PortAudioSink::poll(int64_t now_ms) {
                 "portaudio: '%s' is still gone after a device rescan -- discarding until the "
                 "next stream (%s)",
                 this->name().c_str(), error.c_str());
+        this->recovery_.rescan_abandoned();
         return;
     }
     if (!this->open_stream_(device, format.sample_rate, format.channels, format.bit_depth)) {
-        return;  // open_stream_() has already said why, once
+        this->recovery_.rescan_abandoned();  // open_stream_() has already said why, once
+        return;
     }
     if (this->stopping_.load()) {
         // stop() can land during the slow cycle above; release the device now.
+        this->recovery_.rescan_abandoned();
         this->close_stream_();
         return;
     }
+    this->recovery_.rescan_done(true);
     // Name the device found: a rescan can renumber indices.
     const PaDeviceInfo* info = Pa_GetDeviceInfo(device);
     cli_log(LogLevel::INFO, "portaudio: '%s' is back after a device rescan, on '%s'",

src/pulse_sink.cpp

@@ -609,7 +609,7 @@ void PulseAudioSink::poll(int64_t now_ms) {
     }
     if (this->stopping_.load()) {
         // stop() can land during the reconnect above; release the stream now.
-        this->recovery_.rescan_done(true);  // shutting down; there is nothing left to retry for
+        this->recovery_.rescan_abandoned();  // shutting down; there is nothing left to retry for
         this->close_stream_();
         return;
     }

src/sink_recovery.cpp

@@ -61,7 +61,12 @@ void SinkRecovery::rescan_done(bool recovered) {
         return;
     }
     this->rescan_in_flight_ = false;
-    if (recovered || this->rescan_attempts_ >= SINK_RESCAN_ATTEMPTS) {
+    if (recovered) {
+        // The gap stays owed: only the next timed write can retire it.
+        this->refill_();
+        return;
+    }
+    if (this->rescan_attempts_ >= SINK_RESCAN_ATTEMPTS) {
         this->rescan_spent_ = true;
         this->rescan_owed_.store(false, std::memory_order_relaxed);
         return;
@@ -71,6 +76,14 @@ void SinkRecovery::rescan_done(bool recovered) {
     this->rescan_owed_.store(true, std::memory_order_relaxed);
 }
 
+void SinkRecovery::rescan_abandoned() {
+    if (!this->rescan_in_flight_) {
+        return;
+    }
+    this->rescan_in_flight_ = false;
+    this->rescan_spent_ = true;
+}
+
 bool SinkRecovery::pending() const {
     return this->rescan_owed_.load(std::memory_order_relaxed);
 }
@@ -91,13 +104,17 @@ void SinkRecovery::forget_discarded_frames() {
 }
 
 void SinkRecovery::reset() {
+    this->refill_();
+    this->discarded_frames_ = 0;
+}
+
+void SinkRecovery::refill_() {
     this->reopen_spent_ = false;
     this->rescan_spent_ = false;
     this->rescan_in_flight_ = false;
     this->rescan_attempts_ = 0;
     this->rescan_owed_.store(false, std::memory_order_relaxed);
     this->rescan_at_ms_ = NOT_STAMPED;
-    this->discarded_frames_ = 0;
 }
 
 void SinkRecovery::escalate_() {

src/sink_recovery.h

@@ -28,15 +28,16 @@ inline constexpr int64_t SINK_RESCAN_DELAY_MS = 2000;
 /// Ceiling the doubling delay between retried rescans grows to.
 inline constexpr int64_t SINK_RESCAN_MAX_DELAY_MS = 30000;
 
-/// Rescan attempts allowed per configured stream.
+/// Rescan attempts allowed before the budget is refilled.
 inline constexpr int SINK_RESCAN_ATTEMPTS = 5;
 
 /// Decides when a sink reopens a dead device in place and when it rescans or reconnects.
-/// Budget is per configured stream: one reopen, then up to SINK_RESCAN_ATTEMPTS rescans.
+/// Budget: one reopen, then up to SINK_RESCAN_ATTEMPTS rescans. Only a recovered rescan or a newly
+/// configured stream refills it; a recovered reopen leaves the next outage the rescans alone.
 /// Every method but pending() must be called under the lock that serialises the sink's stream.
 class SinkRecovery {
 public:
-    /// Whether write() should reopen the device in place; true at most once per stream.
+    /// Whether write() should reopen the device in place; true at most once per refill.
     /// @return true if the caller should reopen now and report to reopen_done().
     bool reopen_due();
 
@@ -48,10 +49,14 @@ class SinkRecovery {
     /// @return true once the delay is up; again only after rescan_done(false).
     bool rescan_due(int64_t now_ms);
 
-    /// Records the second attempt's outcome; a one-shot backend reports true regardless.
+    /// Records the second attempt's outcome; a recovery refills the budget for the next outage.
     /// A report with no attempt outstanding does nothing.
     void rescan_done(bool recovered);
 
+    /// Ends the outstanding attempt with nothing refilled and nothing more owed: a shutdown, or a
+    /// one-shot backend's failure. A call with no attempt outstanding does nothing.
+    void rescan_abandoned();
+
     /// True while a rescan is still owed. The one method safe to call without the lock.
     bool pending() const;
 
@@ -70,6 +75,8 @@ class SinkRecovery {
 
 private:
     void escalate_();
+    /// Refills the attempt budget, leaving the discarded-frame count alone.
+    void refill_();
 
     static constexpr int64_t NOT_STAMPED = INT64_MIN;
 

tests/sink_recovery_test.cpp

@@ -233,7 +233,7 @@ TEST(SinkRecovery, ACallerThatReportsNothingGetsExactlyOneRescan) {
     escalate(recovery);
     ASSERT_GE(rescan_fires_at(recovery, T0, T0 + 10 * SINK_RESCAN_DELAY_MS), T0);
 
-    // A caller that never reports gets exactly one rescan, as PortAudioSink relies on.
+    // A caller that never reports gets exactly one rescan.
     EXPECT_FALSE(recovery.reopen_due());
     EXPECT_FALSE(recovery.pending());
     EXPECT_EQ(rescan_fires_at(recovery, T0, T0 + 100 * SINK_RESCAN_DELAY_MS), -1);
@@ -251,6 +251,84 @@ TEST(SinkRecovery, ASuccessfulRescanIsNotRetried) {
     EXPECT_EQ(rescan_fires_at(recovery, fired_at, fired_at + 100 * SINK_RESCAN_DELAY_MS), -1);
 }
 
+TEST(SinkRecovery, ASecondOutageInTheSameStreamRecovers) {
+    SinkRecovery recovery;
+    escalate(recovery);
+    int64_t now = rescan_fires_at(recovery, T0, T0 + 10 * SINK_RESCAN_DELAY_MS);
+    ASSERT_GT(now, 0);
+    recovery.rescan_done(false);
+    now = rescan_fires_at(recovery, now, now + 10 * SINK_RESCAN_DELAY_MS);
+    ASSERT_GT(now, 0);
+    recovery.rescan_done(true);
+
+    // The device died again: the reopen is back in hand, and so is a fresh ladder.
+    escalate(recovery);
+    EXPECT_TRUE(recovery.pending());
+    EXPECT_EQ(rescan_fires_at(recovery, now, now + 10 * SINK_RESCAN_DELAY_MS),
+              now + SINK_RESCAN_DELAY_MS);
+}
+
+TEST(SinkRecovery, EveryOutageAfterARecoveryStillGivesUp) {
+    SinkRecovery recovery;
+    escalate(recovery);
+    int64_t now = rescan_fires_at(recovery, T0, T0 + 10 * SINK_RESCAN_DELAY_MS);
+    ASSERT_GT(now, 0);
+    recovery.rescan_done(true);
+
+    escalate(recovery);
+    for (int attempt = 0; attempt < SINK_RESCAN_ATTEMPTS; ++attempt) {
+        now = rescan_fires_at(recovery, now, now + 100 * SINK_RESCAN_DELAY_MS);
+        ASSERT_GT(now, 0) << "attempt " << attempt << " never fired";
+        recovery.rescan_done(false);
+    }
+
+    EXPECT_FALSE(recovery.pending());
+    EXPECT_FALSE(recovery.reopen_due());
+    EXPECT_EQ(rescan_fires_at(recovery, now, now + 100 * SINK_RESCAN_DELAY_MS), -1);
+}
+
+TEST(SinkRecovery, TheDiscardedGapSurvivesTheRefillIntoTheNextOutage) {
+    SinkRecovery recovery;
+    escalate(recovery);
+    recovery.discard_frames(48'000U);
+    ASSERT_GE(rescan_fires_at(recovery, T0, T0 + 10 * SINK_RESCAN_DELAY_MS), T0);
+    recovery.rescan_done(true);
+
+    // Died again before a timed write took the first gap: both are still owed.
+    escalate(recovery);
+    recovery.discard_frames(1'000U);
+
+    EXPECT_EQ(recovery.take_discarded_frames(), 49'000U);
+}
+
+TEST(SinkRecovery, AnAbandonedRescanRefillsNothing) {
+    SinkRecovery recovery;
+    escalate(recovery);
+    const int64_t fired_at = rescan_fires_at(recovery, T0, T0 + 10 * SINK_RESCAN_DELAY_MS);
+    ASSERT_GT(fired_at, 0);
+
+    recovery.rescan_abandoned();
+
+    EXPECT_FALSE(recovery.pending());
+    EXPECT_FALSE(recovery.reopen_due());
+    EXPECT_EQ(rescan_fires_at(recovery, fired_at, fired_at + 100 * SINK_RESCAN_DELAY_MS), -1);
+
+    // A late recovered report has no attempt to answer, so it cannot refill either.
+    recovery.rescan_done(true);
+    EXPECT_FALSE(recovery.reopen_due());
+}
+
+TEST(SinkRecovery, AbandoningWithNothingOutstandingDoesNothing) {
+    SinkRecovery recovery;
+    escalate(recovery);
+
+    recovery.rescan_abandoned();
+
+    EXPECT_TRUE(recovery.pending());
+    EXPECT_EQ(rescan_fires_at(recovery, T0, T0 + 10 * SINK_RESCAN_DELAY_MS),
+              T0 + SINK_RESCAN_DELAY_MS);
+}
+
 TEST(SinkRecovery, AFailedRescanIsTriedAgainAfterALongerDelay) {
     SinkRecovery recovery;
     escalate(recovery);