fix: address review findings on tonight's commits
All checks were successful
CI / build-and-test (push) Successful in 35s

Code review on d14a33a..bab29b0 turned up three real issues, fixed here.

1. EngineLogging.CreateDefault no longer mutates Serilog.Log.Logger. The static set was a belt-and-suspenders attempt to catch any code path that reaches for the singleton, but it doesn't matter (engine code uses ILogger<T>, never Serilog.Log.*) and it raced under xUnit's parallel test execution.

2. IsoPipeline stops holding a RawFrame reference for stats. The receiver-side TappedChannelWriter callback now snapshots only Width/Height into volatile ints — frame's pixel buffer is allowed to GC on its normal schedule and a late stats poll can never resurrect a dropped frame. (Today the buffer is fully managed so a use-after-free wasn't actually possible, but the snapshot pattern is the right ownership shape.)

3. App.xaml.cs's ComponentDispatcher.ThreadFilterMessage subscription now lives in a field and is unsubscribed in OnExit. Mutex release is gated on a new _ownsSingleInstanceMutex flag so the 'lost the race; shut down silently' path doesn't accidentally try to release a handle it never owned.

Plus a load-bearing comment in NdiInteropPInvoke.CreateFinder explaining why we free the UTF-8 group buffers right after the native call returns — same lifetime contract Phase B-2's CreateReceiver / CreateSender have always relied on; if it's wrong, those would fail too. The loopback discovery integration test would catch a regression.

Tests: 74/74 unit + 9/9 NDI integration green.
This commit is contained in:
Zac Gaetano 2026-05-08 01:01:00 -04:00
parent bab29b02ab
commit 16e0a483e2
4 changed files with 48 additions and 13 deletions

View file

@ -26,6 +26,8 @@ public partial class App : Application
$"Local\\WildDragon.TeamsISO.SingleInstance.{Environment.UserName}"; $"Local\\WildDragon.TeamsISO.SingleInstance.{Environment.UserName}";
private System.Threading.Mutex? _singleInstanceMutex; private System.Threading.Mutex? _singleInstanceMutex;
private bool _ownsSingleInstanceMutex;
private ThreadMessageEventHandler? _bringToFrontHandler;
private ILoggerFactory? _loggerFactory; private ILoggerFactory? _loggerFactory;
private NdiInteropPInvoke? _interop; private NdiInteropPInvoke? _interop;
private IsoController? _controller; private IsoController? _controller;
@ -49,6 +51,7 @@ public partial class App : Application
// with the same default name, and two writers to config.json all raced. // with the same default name, and two writers to config.json all raced.
bool createdNew; bool createdNew;
_singleInstanceMutex = new System.Threading.Mutex(initiallyOwned: true, SingleInstanceMutexName, out createdNew); _singleInstanceMutex = new System.Threading.Mutex(initiallyOwned: true, SingleInstanceMutexName, out createdNew);
_ownsSingleInstanceMutex = createdNew;
if (!createdNew) if (!createdNew)
{ {
var bringToFront = RegisterWindowMessageW("WildDragon.TeamsISO.BringToFront"); var bringToFront = RegisterWindowMessageW("WildDragon.TeamsISO.BringToFront");
@ -59,9 +62,11 @@ public partial class App : Application
} }
// Listen for the broadcast — if a *new* instance launches and finds us already // Listen for the broadcast — if a *new* instance launches and finds us already
// running, it'll send this message; we surface our window in response. // running, it'll send this message; we surface our window in response. Hold the
// delegate in a field so OnExit can unsubscribe cleanly even though the
// AppDomain teardown would also drop it.
var bringToFrontMsg = RegisterWindowMessageW("WildDragon.TeamsISO.BringToFront"); var bringToFrontMsg = RegisterWindowMessageW("WildDragon.TeamsISO.BringToFront");
ComponentDispatcher.ThreadFilterMessage += (ref System.Windows.Interop.MSG msg, ref bool handled) => _bringToFrontHandler = (ref System.Windows.Interop.MSG msg, ref bool handled) =>
{ {
if (msg.message == (int)bringToFrontMsg && MainWindow is not null) if (msg.message == (int)bringToFrontMsg && MainWindow is not null)
{ {
@ -72,6 +77,7 @@ public partial class App : Application
handled = true; handled = true;
} }
}; };
ComponentDispatcher.ThreadFilterMessage += _bringToFrontHandler;
try try
{ {
@ -161,7 +167,18 @@ public partial class App : Application
} }
finally finally
{ {
try { _singleInstanceMutex?.ReleaseMutex(); } catch { /* not owned */ } // Unsubscribe the bring-to-front filter so the delegate doesn't outlive
// the App; ComponentDispatcher is process-static.
if (_bringToFrontHandler is not null)
{
ComponentDispatcher.ThreadFilterMessage -= _bringToFrontHandler;
_bringToFrontHandler = null;
}
// Release the Mutex iff we acquired it. The "lost the race" path above
// sets _ownsSingleInstanceMutex=false and we skip ReleaseMutex (which
// would throw ApplicationException on an unowned Mutex).
try { if (_ownsSingleInstanceMutex) _singleInstanceMutex?.ReleaseMutex(); }
catch { /* defensive: already-released or invalid handle */ }
_singleInstanceMutex?.Dispose(); _singleInstanceMutex?.Dispose();
} }
base.OnExit(e); base.OnExit(e);

View file

@ -41,6 +41,13 @@ public sealed class NdiInteropPInvoke : INdiInterop, IDisposable
{ {
// Empty/whitespace -> default (Public). Otherwise allocate a UTF-8 buffer // Empty/whitespace -> default (Public). Otherwise allocate a UTF-8 buffer
// for the comma-separated group list and pin a settings struct around it. // for the comma-separated group list and pin a settings struct around it.
//
// Memory ownership: the NDI SDK's NDIlib_find_create_v2 (and _send_create,
// _recv_create_v3) copy the strings out of the settings struct synchronously
// before returning — they don't retain pointers into our buffers. This is the
// same lifetime contract CreateReceiver / CreateSender below have relied on
// since Phase B-2; if it ever turns out to be wrong, those will fail too. The
// loopback discovery integration test would catch a regression here.
var trimmed = string.IsNullOrWhiteSpace(groups) ? null : groups!.Trim(); var trimmed = string.IsNullOrWhiteSpace(groups) ? null : groups!.Trim();
if (trimmed is null) if (trimmed is null)
{ {

View file

@ -77,11 +77,11 @@ public static class EngineLogging
flushToDiskInterval: TimeSpan.FromMilliseconds(250)) // flush often so support tools see live tails flushToDiskInterval: TimeSpan.FromMilliseconds(250)) // flush often so support tools see live tails
.CreateLogger(); .CreateLogger();
// Set the Serilog static singleton too. SerilogLoggerFactory writes through the // Note: deliberately NOT setting Serilog.Log.Logger here. xUnit can run tests
// explicit logger we pass in, but anything in the engine that reaches for // in parallel and two factories from different tests would otherwise race on
// Serilog.Log.* directly would otherwise miss our sinks. Belt & suspenders. // the static singleton. SerilogLoggerFactory writes through the explicit
Serilog.Log.Logger = serilog; // logger we pass in, so the static fallback isn't needed; engine code uses
// the injected ILogger<T>, not Serilog.Log.* directly.
var factory = new SerilogLoggerFactory(serilog, dispose: true); var factory = new SerilogLoggerFactory(serilog, dispose: true);
// Surface the path at startup so support has it without digging. // Surface the path at startup so support has it without digging.
factory.CreateLogger("TeamsISO.Engine") factory.CreateLogger("TeamsISO.Engine")

View file

@ -24,7 +24,13 @@ public sealed class IsoPipeline : IAsyncDisposable
// restart. Reads via Volatile.Read are safe from any thread (UI's stats poll). // restart. Reads via Volatile.Read are safe from any thread (UI's stats poll).
private NdiReceiver? _liveReceiver; private NdiReceiver? _liveReceiver;
private NdiSender? _liveSender; private NdiSender? _liveSender;
private RawFrame? _lastReceivedFrame;
// Last-frame metadata, snapshotted out of the RawFrame on capture so we don't
// hold a reference to the frame's pixel buffer past its useful life. Two ints
// and a DateTimeOffset are atomically writable on x64; we accept tearing on x86
// (purely advisory stats display).
private int _lastWidth;
private int _lastHeight;
private DateTimeOffset? _lastReceivedAt; private DateTimeOffset? _lastReceivedAt;
public Guid ParticipantId { get; } public Guid ParticipantId { get; }
@ -41,7 +47,8 @@ public sealed class IsoPipeline : IAsyncDisposable
{ {
var receiver = Volatile.Read(ref _liveReceiver); var receiver = Volatile.Read(ref _liveReceiver);
var sender = Volatile.Read(ref _liveSender); var sender = Volatile.Read(ref _liveSender);
var lastFrame = Volatile.Read(ref _lastReceivedFrame); var w = Volatile.Read(ref _lastWidth);
var h = Volatile.Read(ref _lastHeight);
var lastAt = _lastReceivedAt; var lastAt = _lastReceivedAt;
if (receiver is null || sender is null) if (receiver is null || sender is null)
@ -54,8 +61,8 @@ public sealed class IsoPipeline : IAsyncDisposable
FramesDuplicated: 0, // same — last-frame re-emits aren't counted yet FramesDuplicated: 0, // same — last-frame re-emits aren't counted yet
LastFrameAt: lastAt, LastFrameAt: lastAt,
IncomingFps: 0, // running rate computation is a follow-up IncomingFps: 0, // running rate computation is a follow-up
IncomingWidth: lastFrame?.Width ?? 0, IncomingWidth: w,
IncomingHeight: lastFrame?.Height ?? 0); IncomingHeight: h);
} }
/// <summary> /// <summary>
@ -110,7 +117,11 @@ public sealed class IsoPipeline : IAsyncDisposable
}, },
onFrame: frame => onFrame: frame =>
{ {
Volatile.Write(ref _lastReceivedFrame, frame); // Snapshot dimensions only — don't hold the RawFrame reference past
// the channel write so the GC can reclaim it on schedule, and so a
// late stats read can never resurrect a dropped frame's pixel buffer.
Volatile.Write(ref _lastWidth, frame.Width);
Volatile.Write(ref _lastHeight, frame.Height);
_lastReceivedAt = DateTimeOffset.UtcNow; _lastReceivedAt = DateTimeOffset.UtcNow;
}); });
} }