From 9cb1cc7b3d5ec73db33228995a7feb8021ea78bd Mon Sep 17 00:00:00 2001 From: Zac Gaetano Date: Sat, 9 May 2026 09:34:16 -0400 Subject: [PATCH] fix: review findings on the polish + active-speaker batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real concerns from the code review on ab07297..b266623: 1. ActiveSpeaker removal poisoned the rename-window heuristic. ParticipantTracker.HandleRemoved appends to _recentlyRemoved keyed by MachineName alone; the next Participant Add on the same machine consulted that list with no kind discrimination, so an active-speaker disappearance immediately followed by a participant joining (very common: Teams renames its outputs as participants enter/leave) would cause the new participant to inherit the auto-mix's deterministic v5 GUID. New HandleAutoMixRemoved deliberately skips _recentlyRemoved — the auto-mix row's identity is already stable via the deterministic Id, so re-add restores it without the rename window. 2. IsoPipeline.State writes were not synchronized. Supervisor loop sets State on its own thread; UI thread reads from GetStats. Without volatility, the JIT could cache the field in a register and the UI would stay stuck on Receiving even after Error. Backing field is now an int read/written via Volatile.Read/Volatile.Write, matching the pattern already used for _liveReceiver / _liveSender / _liveProcessor. Tests: 79/79 (was 78) — added ParticipantTrackerTests.ActiveSpeakerRemove_DoesNotPoisonRenameWindowForLaterParticipant which would have caught (1). --- .../Discovery/ParticipantTracker.cs | 19 ++++++++++++- src/TeamsISO.Engine/Pipeline/IsoPipeline.cs | 10 ++++++- .../Discovery/ParticipantTrackerTests.cs | 28 +++++++++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/TeamsISO.Engine/Discovery/ParticipantTracker.cs b/src/TeamsISO.Engine/Discovery/ParticipantTracker.cs index df8b8b4..ca23971 100644 --- a/src/TeamsISO.Engine/Discovery/ParticipantTracker.cs +++ b/src/TeamsISO.Engine/Discovery/ParticipantTracker.cs @@ -38,7 +38,7 @@ public sealed class ParticipantTracker HandleRemoved(r.Source); break; case DiscoveryEvent.Removed r when r.Source.Kind == NdiSourceKind.ActiveSpeaker: - HandleRemoved(r.Source); + HandleAutoMixRemoved(r.Source); break; } } @@ -125,6 +125,23 @@ public sealed class ParticipantTracker _recentlyRemoved.Add(new RecentlyRemoved(existing.Id, source.MachineName, now)); } + /// + /// Removes the synthetic auto-mix row's CurrentSource. Crucially does NOT add to + /// — the auto-mix row's identity is already stable + /// via the deterministic v5 GUID, so re-add restores it without needing the + /// rename-window heuristic, and we must not let an active-speaker disappearance + /// poison the rename matcher for a Participant joining the same machine within + /// the window. + /// + private void HandleAutoMixRemoved(NdiSource source) + { + var stableId = DeterministicGuid("auto-mix:" + source.MachineName); + var existing = _participants.FirstOrDefault(p => p.Id == stableId); + if (existing is null) return; + existing.CurrentSource = null; + existing.LastSeen = _now(); + } + private void PruneRecentlyRemoved(DateTimeOffset now) { _recentlyRemoved.RemoveAll(rr => now - rr.RemovedAt > _renameWindow); diff --git a/src/TeamsISO.Engine/Pipeline/IsoPipeline.cs b/src/TeamsISO.Engine/Pipeline/IsoPipeline.cs index 76e43bd..a6f4c1a 100644 --- a/src/TeamsISO.Engine/Pipeline/IsoPipeline.cs +++ b/src/TeamsISO.Engine/Pipeline/IsoPipeline.cs @@ -45,7 +45,15 @@ public sealed class IsoPipeline : IAsyncDisposable private readonly object _frameTimesGate = new(); public Guid ParticipantId { get; } - public IsoState State { get; private set; } = IsoState.Idle; + + // Backing field for State, accessed via Volatile.Read/Write so the supervisor + // loop's writes are observed promptly by the UI thread's stats poll. + private int _state = (int)IsoState.Idle; + public IsoState State + { + get => (IsoState)Volatile.Read(ref _state); + private set => Volatile.Write(ref _state, (int)value); + } public int ConsecutiveFailures => _consecutiveFailures; /// diff --git a/src/tests/TeamsISO.Engine.Tests/Discovery/ParticipantTrackerTests.cs b/src/tests/TeamsISO.Engine.Tests/Discovery/ParticipantTrackerTests.cs index c125685..385e4df 100644 --- a/src/tests/TeamsISO.Engine.Tests/Discovery/ParticipantTrackerTests.cs +++ b/src/tests/TeamsISO.Engine.Tests/Discovery/ParticipantTrackerTests.cs @@ -135,4 +135,32 @@ public class ParticipantTrackerTests tracker.Participants.Should().HaveCount(1); tracker.Participants[0].Id.Should().Be(firstId); } + + [Fact] + public void ActiveSpeakerRemove_DoesNotPoisonRenameWindowForLaterParticipant() + { + // Regression: the rename-window heuristic matches by MachineName alone, so a + // disappearing ActiveSpeaker source on a machine could cause the next + // Participant joining that same machine to inherit the auto-mix's GUID, AND + // the auto-mix row would be renamed to the participant's display name. + // HandleAutoMixRemoved deliberately skips _recentlyRemoved. + var tracker = new ParticipantTracker(TimeSpan.FromSeconds(5), () => T0); + var autoMix = new NdiSource("WOOGLIN (MS Teams - Active Speaker)", "WOOGLIN", NdiSourceKind.ActiveSpeaker, null); + var jane = new NdiSource("WOOGLIN (MS Teams - Jane)", "WOOGLIN", NdiSourceKind.Participant, "Jane"); + + tracker.Apply(new DiscoveryEvent.Added(autoMix)); + var autoMixId = tracker.Participants[0].Id; + tracker.Apply(new DiscoveryEvent.Removed(autoMix)); + tracker.Apply(new DiscoveryEvent.Added(jane)); + + // Two distinct rows: the auto-mix (offline, no source) and Jane (a brand-new participant). + tracker.Participants.Should().HaveCount(2); + var jp = tracker.Participants.Single(p => p.DisplayName == "Jane"); + jp.Id.Should().NotBe(autoMixId, + because: "Jane is a fresh Participant, not a renamed auto-mix"); + var asRow = tracker.Participants.Single(p => p.DisplayName == "Active Speaker"); + asRow.Id.Should().Be(autoMixId); + asRow.CurrentSource.Should().BeNull( + because: "the auto-mix source went away and hasn't been re-added"); + } }