Reopen on a device-list change instead of waiting out the backoff

A dying device reports itself through kAudioDevicePropertyDeviceIsAlive, but a
returning one has nothing to report, so a replugged named device was found only
by SinkRecovery's 2->30 s ladder. The hardware pass measured the cost: a device
physically back at 21 s was not reopened until 41 s, because the ladder had
already doubled past it.

Listen on kAudioHardwarePropertyDevices too, and give SinkRecovery a
rescan_soon() that brings an owed rescan forward to the next tick. It arms
nothing that is not already owed and the attempt still counts, so a burst of
plug events cannot spin.

Author: chrisuthe

docs/ROADMAP.md

@@ -2542,6 +2542,12 @@ frameworks, so the binary links only what every Mac already has.
   directly, so there is no `Pa_Terminate()`/`Pa_Initialize()` and no index renumbering: the
   listener flags the death off its HAL thread, `write()` spends the one in-place reopen, and
   `poll()` retries on `SinkRecovery`'s backoff.
+- **A replug is a notification too, not just a timeout.** A dying device says so itself; a
+  returning one cannot, so `kAudioHardwarePropertyDevices` is listened to as well and
+  `SinkRecovery::rescan_soon()` brings the owed rescan forward to the next tick. Without it the
+  backoff alone decides, and the hardware pass measured what that costs: a device physically back
+  at 21 s was not reopened until 41 s, because the 2 s ladder had already doubled past it. The
+  attempt still counts against the budget, so a burst of plug events cannot spin.
 - **A moved system default is followed, and is not a recovery.** A bare `-o coreaudio` also
   listens on `kAudioHardwarePropertyDefaultOutputDevice`; `poll()` reopens on the new device
   with the ring tail accounted as an outage gap. Deliberately outside `SinkRecovery`'s budget:
@@ -2592,19 +2598,18 @@ What it proved:
   player in the same group was in phase**, which is the only real test the DAC offset has, and the
   thing item 3 shipped without.
 
-Still open after that pass:
-
-- **The volume ramp.** The full→half change was heard as a step or click. The arithmetic is not at
-  fault — `volume_ramp_step(48000)` is 4473925 Q32/frame and 100→50 is a genuine 621-frame ramp —
-  and mute, which is the *slowest* ramp at a full 20 ms, was reported clean. The likeliest reading
-  is the slew rate being fast rather than absent, since `VOLUME_RAMP_MS` is a full-scale time and
-  100→50 is 12.9 ms. `open_unit_()` now logs the step at `debug` so the two cases can be told
-  apart without ears; a second listen is owed before anything is changed, and if it is the slew
-  rate then it belongs to item 13, which owns it for all three backends.
-- **The outage-gap figure.** The harness's own `recovery` driver restarted its pacing clock every
-  0.5 s, leaving the ring dry between slices — ~5000 ppm of loss and audible popping with no
-  outage at all, which swamped the measurement. Fixed in the harness; the number is owed from a
-  re-run. The sink's recovery *mechanisms* are not in doubt, only the accounting figure.
+A second round settled the two things the first left open, and turned up the listener gap above:
+
+- **The volume ramp is fine**, and the first round's "FAIL" was against a wrong expectation: 20 ms
+  is what a *full-scale* change takes, so 100→50 is 12.9 ms. `open_unit_()` now logs the step at
+  `debug` (4473925 Q32/frame at 48 kHz, non-zero, so ramping rather than snapping), and mute — the
+  largest change and therefore the slowest ramp — is clean. What is audible on full→half is the
+  slew rate, which is item 13's and shared by all three backends.
+- **The outage gap is reported, not swallowed.** Across a ~10 s unplug the reported frame count
+  froze exactly, then jumped by 33.9 s of audio in one step on replug; of roughly 30 s of silence
+  only ~84 ms went permanently unaccounted, and the residual is flat rather than growing. The
+  first round could not measure this because the harness restarted its pacing clock every 0.5 s
+  and lost ~5000 ppm of its own.
 
 Underneath the pass, CI carries the rest: the backend compiles clean under `-Werror` on the
 `macos-arm64` leg, that leg's `otool -L` guard reports only CoreAudio, AudioToolbox,

src/coreaudio_sink.cpp

@@ -682,6 +682,11 @@ void CoreAudioSink::poll(int64_t now_ms) {
     if (this->last_format_.sample_rate == 0) {
         return;  // nothing was ever configured, so there is nothing to reopen at
     }
+    // Cleared only once a rescan is owed, so a replug that lands before the escalation still
+    // counts; a stale flag costs one early attempt and nothing else.
+    if (this->devices_changed_.exchange(false)) {
+        this->recovery_.rescan_soon();
+    }
     if (!this->recovery_.rescan_due(now_ms)) {
         return;
     }
@@ -882,6 +887,7 @@ bool CoreAudioSink::open_unit_(AudioDeviceID device, uint32_t sample_rate, uint8
 
     // Cleared before the listeners go on, so a death between the two is not lost.
     this->device_lost_.store(false);
+    this->devices_changed_.store(false);
     this->add_listeners_(device);
 
     err = AudioOutputUnitStart(unit);
@@ -1064,6 +1070,14 @@ void CoreAudioSink::add_listeners_(AudioDeviceID device) {
             this->listening_default_ = true;
         }
     }
+
+    // A device that died tells us so itself; a device that comes back cannot, so the host's
+    // device list is what turns a replug into a reopen instead of a wait on the backoff.
+    const AudioObjectPropertyAddress devices = address_of(kAudioHardwarePropertyDevices);
+    if (AudioObjectAddPropertyListener(kAudioObjectSystemObject, &devices,
+                                       &CoreAudioSink::property_listener, this) == noErr) {
+        this->listening_devices_ = true;
+    }
 }
 
 void CoreAudioSink::remove_listeners_() {
@@ -1081,6 +1095,12 @@ void CoreAudioSink::remove_listeners_() {
                                           &CoreAudioSink::property_listener, this);
         this->listening_default_ = false;
     }
+    if (this->listening_devices_) {
+        const AudioObjectPropertyAddress devices = address_of(kAudioHardwarePropertyDevices);
+        AudioObjectRemovePropertyListener(kAudioObjectSystemObject, &devices,
+                                          &CoreAudioSink::property_listener, this);
+        this->listening_devices_ = false;
+    }
 }
 
 OSStatus CoreAudioSink::property_listener(AudioObjectID /*object*/, UInt32 count,
@@ -1097,6 +1117,9 @@ OSStatus CoreAudioSink::property_listener(AudioObjectID /*object*/, UInt32 count
             case kAudioHardwarePropertyDefaultOutputDevice:
                 self->default_moved_.store(true);
                 break;
+            case kAudioHardwarePropertyDevices:
+                self->devices_changed_.store(true);
+                break;
             default:
                 break;
         }

src/coreaudio_sink.h

@@ -148,9 +148,13 @@ class CoreAudioSink final : public AudioSink {
     std::atomic<bool> device_lost_{false};
     /// Set by the property listener when the system default output moves; poll() clears it.
     std::atomic<bool> default_moved_{false};
+    /// Set by the property listener when a device is added or removed anywhere on the host.
+    /// Kept until a rescan is owed, so a replug during the backoff is not waited out.
+    std::atomic<bool> devices_changed_{false};
     /// Whether each listener is registered, so each is removed exactly once. Guarded by mutex_.
     bool listening_alive_{false};
     bool listening_default_{false};
+    bool listening_devices_{false};
     /// The device the death listener is registered on. Guarded by mutex_.
     AudioDeviceID listening_device_{kAudioObjectUnknown};
 

src/sink_recovery.cpp

@@ -40,14 +40,18 @@ bool SinkRecovery::rescan_due(int64_t now_ms) {
     if (!this->rescan_owed_.load(std::memory_order_relaxed)) {
         return false;
     }
-    if (this->rescan_at_ms_ == NOT_STAMPED) {
-        // Stamped on the first tick after escalation; the delay grows with attempts made.
-        this->rescan_at_ms_ = now_ms + delay_for_(this->rescan_attempts_);
-        return false;
-    }
-    if (now_ms < this->rescan_at_ms_) {
-        return false;
+    // The backoff is only for waiting out a device nothing has said anything about.
+    if (!this->rescan_immediate_) {
+        if (this->rescan_at_ms_ == NOT_STAMPED) {
+            // Stamped on the first tick after escalation; the delay grows with attempts made.
+            this->rescan_at_ms_ = now_ms + delay_for_(this->rescan_attempts_);
+            return false;
+        }
+        if (now_ms < this->rescan_at_ms_) {
+            return false;
+        }
     }
+    this->rescan_immediate_ = false;
     ++this->rescan_attempts_;
     this->rescan_in_flight_ = true;
     // In flight: nothing more is owed until rescan_done() reports.
@@ -71,6 +75,13 @@ void SinkRecovery::rescan_done(bool recovered) {
     this->rescan_owed_.store(true, std::memory_order_relaxed);
 }
 
+void SinkRecovery::rescan_soon() {
+    // Not while one is in flight: it is already running, and rescan_done() re-stamps after it.
+    if (this->rescan_owed_.load(std::memory_order_relaxed) && !this->rescan_in_flight_) {
+        this->rescan_immediate_ = true;
+    }
+}
+
 bool SinkRecovery::pending() const {
     return this->rescan_owed_.load(std::memory_order_relaxed);
 }
@@ -94,6 +105,7 @@ void SinkRecovery::reset() {
     this->reopen_spent_ = false;
     this->rescan_spent_ = false;
     this->rescan_in_flight_ = false;
+    this->rescan_immediate_ = false;
     this->rescan_attempts_ = 0;
     this->rescan_owed_.store(false, std::memory_order_relaxed);
     this->rescan_at_ms_ = NOT_STAMPED;

src/sink_recovery.h

@@ -52,6 +52,11 @@ class SinkRecovery {
     /// A report with no attempt outstanding does nothing.
     void rescan_done(bool recovered);
 
+    /// Brings an owed rescan forward to the next tick, for a backend whose OS has told it the
+    /// device list changed. Never arms one that is not owed, and the attempt it releases still
+    /// counts against the budget, so a burst of notifications stays bounded.
+    void rescan_soon();
+
     /// True while a rescan is still owed. The one method safe to call without the lock.
     bool pending() const;
 
@@ -81,6 +86,8 @@ class SinkRecovery {
     bool rescan_spent_{false};
     /// Set from handing out an attempt until rescan_done(); blocks re-arming and double counting.
     bool rescan_in_flight_{false};
+    /// Set by rescan_soon(); makes the next rescan_due() skip the backoff exactly once.
+    bool rescan_immediate_{false};
     int rescan_attempts_{0};
     /// Frames accepted with no device to play them, not yet retired; see discard_frames().
     uint32_t discarded_frames_{0};

tests/sink_recovery_test.cpp

@@ -266,6 +266,76 @@ TEST(SinkRecovery, AFailedRescanIsTriedAgainAfterALongerDelay) {
     EXPECT_EQ(second, first + (2 * SINK_RESCAN_DELAY_MS));
 }
 
+// rescan_soon(): the OS saying the device list moved, instead of waiting out the backoff.
+
+TEST(SinkRecovery, ADeviceListChangeFiresTheRescanWithoutWaitingOutTheDelay) {
+    SinkRecovery recovery;
+    escalate(recovery);
+
+    // Stamps the deadline, which is still a long way off.
+    ASSERT_FALSE(recovery.rescan_due(T0));
+    recovery.rescan_soon();
+
+    // The very next tick, rather than T0 + SINK_RESCAN_DELAY_MS.
+    EXPECT_TRUE(recovery.rescan_due(T0 + 10));
+}
+
+TEST(SinkRecovery, ADeviceListChangeSkipsTheBackoffExactlyOnce) {
+    SinkRecovery recovery;
+    escalate(recovery);
+
+    ASSERT_FALSE(recovery.rescan_due(T0));
+    recovery.rescan_soon();
+    ASSERT_TRUE(recovery.rescan_due(T0 + 10));
+    recovery.rescan_done(false);
+
+    // The next attempt is back on the ladder: one notification buys one attempt, not a spin.
+    const int64_t next = rescan_fires_at(recovery, T0 + 10, T0 + (100 * SINK_RESCAN_DELAY_MS));
+    EXPECT_EQ(next, T0 + 10 + (2 * SINK_RESCAN_DELAY_MS));
+}
+
+TEST(SinkRecovery, ADeviceListChangeArmsNothingWhenNoRescanIsOwed) {
+    SinkRecovery recovery;
+
+    // Nothing has died, so there is nothing to bring forward.
+    recovery.rescan_soon();
+    EXPECT_FALSE(recovery.pending());
+    EXPECT_FALSE(recovery.rescan_due(T0));
+    EXPECT_FALSE(recovery.rescan_due(T0 + (100 * SINK_RESCAN_DELAY_MS)));
+}
+
+TEST(SinkRecovery, ADeviceListChangeStillSpendsTheAttemptBudget) {
+    SinkRecovery recovery;
+    escalate(recovery);
+
+    // A burst of notifications must not buy unlimited attempts.
+    int fired = 0;
+    for (int64_t now = T0; now < T0 + (1000 * SINK_RESCAN_DELAY_MS); now += 10) {
+        recovery.rescan_soon();
+        if (recovery.rescan_due(now)) {
+            ++fired;
+            recovery.rescan_done(false);
+        }
+    }
+    EXPECT_EQ(fired, SINK_RESCAN_ATTEMPTS);
+    EXPECT_FALSE(recovery.pending());
+}
+
+TEST(SinkRecovery, ADeviceListChangeDoesNotDisturbARescanInFlight) {
+    SinkRecovery recovery;
+    escalate(recovery);
+
+    const int64_t fired = rescan_fires_at(recovery, T0, T0 + (10 * SINK_RESCAN_DELAY_MS));
+    ASSERT_GT(fired, 0);
+
+    // In flight: the attempt is already running, so this must not queue a second one.
+    recovery.rescan_soon();
+    EXPECT_FALSE(recovery.rescan_due(fired + 10));
+
+    recovery.rescan_done(true);
+    EXPECT_FALSE(recovery.pending());
+}
+
 TEST(SinkRecovery, TheRetriesRunOutAndTheDelayStopsGrowing) {
     SinkRecovery recovery;
     escalate(recovery);