Review sweep: fix real bugs found across the service + per-app + settings changes

Parallel code review of this session's changes surfaced several real bugs; fixed the substantive ones (judgment/cleanup calls held for Ed): - HIGH Clearing "remembered peers" was resurrected on the next launch: the per-profile -> global migration re-ran every startup and re-unioned the profile file's stale copy. Added a one-time AppConfig.RememberedPeersMigrated marker so migration runs once and a cleared list stays cleared. Self-test pins the clear-then-reload scenario. - MED PushModeWasapiBackend.Start rethrew on a device-open failure; nothing up the stack wraps it, so device churn (a push-eligible single WASAPI source unplugged mid-open) could crash the app. Now logs and stays stopped like MixingEngine/ASIO; the device watcher / self-heal re-open when a device returns. - MED Service self-heal: (a) the re-open "no send sources" path left PerformanceMode ON and presence up while streaming nothing - now releases cleanly; (b) the 3-attempt ladder never refunded, so 3 hiccups over a days-long stint meant permanent silence - now refunds when real audio is heard, and resets on a device hot-plug. - MED ApplyProfile resolved peers (DNS) and enumerated devices INSIDE the gate lock - a boot-time DNS hang as SYSTEM stalled Suspend()/yield/self-heal. Moved outside the lock. - LOW AudioSessionStartWatcher leaked the AudioSessionManager on every Rehook (the WASAPI handle-leak fingerprint) - now disposed. New lifecycle self-test. - LOW stale docstrings (send-all master toggle; ServiceUpdate in-place scheme; Profile .SendAllApplications "neither reads nor writes"). Gate: 42/42 (added peers-migration + session-watcher-lifecycle tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-17 15:49:01 +01:00
parent 4b2baa58b5
commit 984bcd042e
10 changed files with 143 additions and 44 deletions
@@ -3870,10 +3870,10 @@ public sealed class MainForm : Form
        sessionStartWatcher = null;
    }

-    /// <summary>Restores the WASAPI send mode, the "Send all applications" master toggle and the ticked
-    /// app names from a loaded profile. Remembered apps that aren't running right now are seeded into the
-    /// list (ticked, marked "not running") so they resume capture the moment they reappear. On Windows
-    /// too old for process loopback the mode is forced back to devices.</summary>
+    /// <summary>Restores the WASAPI send mode and the ticked app names from a loaded profile (the main
+    /// window's "Send all applications" master toggle was removed 2026-07-16). Remembered apps that aren't
+    /// running right now are seeded into the list (ticked, marked "not running") so they resume capture the
+    /// moment they reappear. On Windows too old for process loopback the mode is forced back to devices.</summary>
    private void RestoreSendModeFromProfile(Profile p)
    {
        suppressSendAppEvents = true;
@@ -71,6 +71,8 @@ internal static class SelfTest
        RunStep(results, "Service silent-capture self-heal (issue #23 boot re-open ladder)", ServiceSilentCaptureSelfHeal);
        RunStep(results, "Send-app capture change-detection (catch an app the instant it opens)", SendAppCaptureChangeDetection);
        RunStep(results, "Remembered applications list is global + clearable", RememberedApplicationsGlobal);
+        RunStep(results, "Remembered peers migrate once (cleared list not resurrected)", RememberedPeersMigrationOnce);
+        RunStep(results, "Session-start watcher lifecycle (construct/rehook/dispose)", SessionStartWatcher);
        RunStep(results, "Send-app lists semantics (ticked → Active, out of Remembered)", SendAppListSemantics);
        RunStep(results, "Service registration args", ServiceRegistrationArgs);
        RunStep(results, "Service self-contained install (own bin + user stop rights)", ServiceSelfContainedInstall);
@@ -1717,6 +1719,68 @@ internal static class SelfTest
        finally { store.SaveRememberedApplications(original); }
    }

+    /// <summary>The peers list went machine-wide (AppConfig) with a ONE-TIME migration from each old
+    /// profile's per-profile list. Regression guard for the bug where the migration re-ran every launch
+    /// and RESURRECTED peers the user had just cleared: after a clear, re-loading the same profile (whose
+    /// JSON still holds the old peers) must NOT bring them back. Touches the real AppConfig; saves/restores.</summary>
+    private static string? RememberedPeersMigrationOnce()
+    {
+        var store = new RemSoundSettingsStore("RemSound");
+        var saved = AppConfig.Load();
+        var savedPeers = saved.RememberedPeers;
+        var savedMigrated = saved.RememberedPeersMigrated;
+        try
+        {
+            // Clean slate: no global peers, migration not yet done.
+            var c0 = AppConfig.Load(); c0.RememberedPeers = new(); c0.RememberedPeersMigrated = false; c0.Save();
+
+            var p = new Profile { Title = "peers-migration-selftest" };
+            p.RememberedPeers = new List<string> { "Alice", "Bob" };
+
+            // First load of a profile that has legacy peers migrates them and sets the one-time flag.
+            store.ApplyProfile(p);
+            var migrated = store.LoadRememberedPeers();
+            Check(migrated.Contains("Alice") && migrated.Contains("Bob"), "legacy per-profile peers must migrate into the global list");
+            Check(AppConfig.Load().RememberedPeersMigrated, "the one-time migration flag must be set after migrating");
+
+            // User clears the global peers list (the Preferences button).
+            store.SaveRememberedPeers(Array.Empty<string>());
+            Check(store.LoadRememberedPeers().Count == 0, "clearing must empty the global peers list");
+
+            // Re-loading the SAME profile (its JSON still lists Alice/Bob) must NOT resurrect them.
+            store.ApplyProfile(p);
+            Check(store.LoadRememberedPeers().Count == 0,
+                "a cleared peers list must NOT be resurrected by re-loading a profile (migration is one-time)");
+            return "peers migrate once; a cleared list stays cleared across profile re-loads";
+        }
+        finally
+        {
+            var c = AppConfig.Load(); c.RememberedPeers = savedPeers; c.RememberedPeersMigrated = savedMigrated; c.Save();
+        }
+    }
+
+    /// <summary>The instant capture-on-app-open watcher (AudioSessionStartWatcher) must construct, re-hook
+    /// its default-device notification without throwing, and dispose idempotently — the plumbing behind
+    /// "catch a per-app send from its very start" and the service's boot session-kick. (It hooks live
+    /// WASAPI, so this proves lifecycle safety, not delivery of a real session event.)</summary>
+    private static string? SessionStartWatcher()
+    {
+        RemSound.Sender.AudioSessionStartWatcher w;
+        try { w = new RemSound.Sender.AudioSessionStartWatcher(_ => { }, _ => { }); }
+        catch (Exception ex) { return Skip($"session watcher could not construct (no audio endpoint?): {ex.GetType().Name}: {ex.Message}"); }
+        try
+        {
+            w.Rehook(); // re-point at the current default device — must never throw
+            w.Rehook();
+            return "constructed, re-hooked twice, and disposed idempotently without throwing";
+        }
+        finally
+        {
+            w.Dispose();
+            w.Dispose(); // idempotent
+        }
+    }
+
    /// <summary>Pins the two-list semantics Ed specified 2026-07-16 (no send-all option): a TICKED app
    /// must live in the Active list — even when it isn't running, marked "(not running)", so it can
    /// always be found and unticked — and must NOT appear in Remembered; an UNTICKED remembered app
@@ -137,7 +137,9 @@ public sealed class ServiceSendHost : IDisposable
        // silence keepalive feeding back, which is why the peak matters.
        var peak = sender.TakeMaxSenderPreEncodePeak();
        if (peak > pulsePeakMax) pulsePeakMax = peak;
-        if (peak >= SilentPeak) { everHeardAudio = true; deafSinceTick = 0; }
+        // Real audio proves the capture works — refund the self-heal ladder so a LATER hiccup (hours or
+        // days into an always-on stream) gets fresh re-open attempts instead of finding the budget spent.
+        if (peak >= SilentPeak) { everHeardAudio = true; deafSinceTick = 0; reopenAttempts = 0; }
        pulseFramesSent += sender.TakeSenderAudioFramesSent();

        // What is the DEVICE playing? The endpoint's own meter, independent of our capture stream.
@@ -256,7 +258,20 @@ public sealed class ServiceSendHost : IDisposable
            {
                sender.Stop();
                var specs = BuildSendSpecs(profile); // re-resolve (the default device may have moved)
-                if (specs.Count == 0) { log?.Invoke("service: re-open found no send sources — capture left stopped"); running = false; return; }
+                if (specs.Count == 0)
+                {
+                    // The source went away (e.g. the only loopback device was unplugged). Release the whole
+                    // send stack — presence, meter readers, the session watcher AND the perf-mode overrides
+                    // — instead of sitting "running" with High priority / EcoQoS-off held while streaming
+                    // nothing. The device-change watcher re-opens (via ApplyProfile) when a device returns.
+                    log?.Invoke("service: re-open found no send sources — releasing until a device returns");
+                    try { presence.Stop(); } catch { }
+                    SwapMeterDevices(Array.Empty<CaptureSourceSpec>());
+                    try { sessionKick?.Dispose(); } catch { } sessionKick = null;
+                    try { PerformanceMode.Apply(false, msg => log?.Invoke($"service: {msg}")); } catch { }
+                    running = false;
+                    return;
+                }
                sender.Configure(specs);
                sender.Start();
                SwapMeterDevices(specs);
@@ -297,11 +312,16 @@ public sealed class ServiceSendHost : IDisposable
    /// false (and stays stopped) if the profile has nothing to send or no reachable peers.</summary>
    public bool ApplyProfile(Profile profile)
    {
+        // Resolve sources + peer addresses OUTSIDE the lock. BuildEndpoints does DNS (Dns.GetHostAddresses)
+        // and BuildSendSpecs enumerates devices — either can block for seconds at boot as SYSTEM before the
+        // network/audio stack is fully up. Doing that while holding `gate` would stall Suspend() (yielding
+        // to the interactive app), the RunLoop tick and the self-heal for the whole timeout.
+        if (disposed) return false;
+        var specs = BuildSendSpecs(profile);
+        var endpoints = BuildEndpoints(profile);
        lock (gate)
        {
            if (disposed) return false;
-            var specs = BuildSendSpecs(profile);
-            var endpoints = BuildEndpoints(profile);
            if (specs.Count == 0) { log?.Invoke("service: profile has no WASAPI send sources — nothing to stream"); return false; }
            if (endpoints.Count == 0) { log?.Invoke("service: profile has no reachable peers — nothing to stream to"); return false; }

@@ -536,6 +556,10 @@ public sealed class ServiceSendHost : IDisposable
        }
        var profile = loadProfile();
        if (profile is null) return;
+        // A device hot-plug re-plumbs the audio graph much like a power resume or a fresh boot — refill the
+        // self-heal ladder so a brand-new device that comes up momentarily deaf still gets its re-opens.
+        everHeardAudio = false;
+        reopenAttempts = 0;
        Suspend();
        ApplyProfile(profile);
    }
@@ -17,9 +17,9 @@ namespace RemSound.App;
 /// uncertainty (folder unknown, file missing mid-swap, unparseable version) means "don't act". After the
 /// copy+restart the running bin == the app version, so it never re-triggers.</para>
 ///
-/// <para>Trust note: the service copies from a user-writable folder and runs it as SYSTEM — the same trust
-/// posture as the previous in-place scheme. Acceptable for this personal app; a hardened build would
-/// code-sign and verify before copying.</para>
+/// <para>Trust note: the service copies from a user-writable folder (the app's install location) and runs
+/// it as SYSTEM. That is a local-privilege-escalation surface — a hardened build would code-sign the app
+/// and verify the signature before copying. Accepted deliberately for this personal app.</para>
 /// </summary>
 internal static class ServiceUpdate
 {