fix(plugins): close LA5 ownership races

src/AcDream.Core/Plugins/LoadedPlugin.cs

@@ -7,17 +7,17 @@ namespace AcDream.Core.Plugins;
 /// Outcome of a plugin load attempt.
 /// <para>On success, <see cref="Plugin"/> is the instantiated plugin, <see cref="LoadContext"/>
 /// owns its assembly, and <see cref="Error"/> is null.</para>
-/// <para>On failure, <see cref="Plugin"/> and <see cref="LoadContext"/> are null,
-/// <see cref="Error"/> describes what went wrong, and
-/// <see cref="ReleasedLoadContext"/> weakly observes any collectible context
-/// that was already released during rollback.</para>
+/// <para>On failure, <see cref="Error"/> describes what went wrong. A partial
+/// <see cref="Plugin"/> and/or <see cref="LoadContext"/> may still be present;
+/// the caller owns their cleanup. The loader never requests collectible unload
+/// itself because the session must first roll back host registrations.</para>
 /// </summary>
 public sealed record LoadedPlugin(
     PluginManifest Manifest,
     IAcDreamPlugin? Plugin,
     AssemblyLoadContext? LoadContext,
-    Exception? Error,
-    WeakReference? ReleasedLoadContext = null)
+    Exception? Error)
 {
-    public bool Success => Plugin is not null && Error is null;
+    public bool Success =>
+        Plugin is not null && LoadContext is not null && Error is null;
 }

src/AcDream.Core/Plugins/PluginLoader.cs

@@ -11,6 +11,8 @@ public static class PluginLoader
     /// implementing <see cref="IAcDreamPlugin"/>, instantiate it, and call its
     /// <see cref="IAcDreamPlugin.Initialize"/> with the supplied host. Any failure
     /// is returned as a failed <see cref="LoadedPlugin"/> rather than thrown.
+    /// A returned partial plugin/context remains caller-owned; this method never
+    /// requests unload because the caller must close host registrations first.
     /// </summary>
     public static LoadedPlugin Load(string pluginDirectory, PluginManifest manifest, IPluginHost host)
     {
@@ -48,15 +50,12 @@ public static class PluginLoader
 
             if (pluginType is null)
             {
-                var released = new WeakReference(alc);
-                alc.Unload();
                 return new LoadedPlugin(
                     manifest,
                     Plugin: null,
-                    LoadContext: null,
+                    LoadContext: alc,
                     Error: new InvalidOperationException(
-                        $"no IAcDreamPlugin implementation found in {manifest.EntryDll}"),
-                    ReleasedLoadContext: released);
+                        $"no IAcDreamPlugin implementation found in {manifest.EntryDll}"));
             }
 
             instance = (IAcDreamPlugin)Activator.CreateInstance(pluginType)!;
@@ -65,20 +64,15 @@ public static class PluginLoader
         }
         catch (Exception ex)
         {
-            // Initialize may have attached host callbacks before it failed.
-            // Give that partial instance the same best-effort cleanup chance
-            // as an Enable failure before releasing the collectible context.
-            try { instance?.Disable(); }
-            catch { }
-            WeakReference? released = alc is null ? null : new WeakReference(alc);
-            try { alc?.Unload(); }
-            catch { }
+            // The caller owns rollback for a partial instance/context. In
+            // particular, Initialize may already have attached host callbacks;
+            // the per-plugin host scope must remove those registrations before
+            // Disable or any collectible unload request can run.
             return new LoadedPlugin(
                 manifest,
-                Plugin: null,
-                LoadContext: null,
-                Error: ex,
-                ReleasedLoadContext: released);
+                Plugin: instance,
+                LoadContext: alc,
+                Error: ex);
         }
     }
 }

src/AcDream.Core/Plugins/PluginSession.cs

@@ -214,9 +214,11 @@ public sealed class PluginSession : IDisposable
                     scope);
                 if (!loaded.Success)
                 {
+                    // Initialize can register callbacks before it fails. The
+                    // registration transaction closes before plugin cleanup
+                    // and, critically, before any ALC Unloading notification.
                     scope.Dispose();
-                    if (loaded.ReleasedLoadContext is { } released)
-                        _releasedContexts.Add(released);
+                    ReleaseFailedLoad(loaded);
                     AddError(
                         errors,
                         id,
@@ -304,6 +306,42 @@ public sealed class PluginSession : IDisposable
         }
     }
 
+    private void ReleaseFailedLoad(LoadedPlugin loaded)
+    {
+        if (loaded.Plugin is not null)
+        {
+            try
+            {
+                loaded.Plugin.Disable();
+            }
+            catch (Exception error)
+            {
+                SafeLog(
+                    static (log, message, exception) =>
+                        log.Error(message, exception),
+                    $"plugin cleanup after initialize failure failed: {loaded.Manifest.Id}",
+                    error);
+            }
+        }
+
+        if (loaded.LoadContext is null)
+            return;
+
+        _releasedContexts.Add(new WeakReference(loaded.LoadContext));
+        try
+        {
+            loaded.LoadContext.Unload();
+        }
+        catch (Exception error)
+        {
+            SafeLog(
+                static (log, message, exception) =>
+                    log.Error(message, exception),
+                $"plugin unload after load failure failed: {loaded.Manifest.Id}",
+                error);
+        }
+    }
+
     private void Report(PluginSessionStatus status)
     {
         if (_report is null)

src/AcDream.Core/Plugins/ScopedPluginHost.cs

@@ -4,13 +4,14 @@ namespace AcDream.Core.Plugins;
 
 /// <summary>
 /// Per-plugin host view that owns every registration made through the public
-/// event/UI surfaces. Disposal is the host's rollback boundary: it removes
+/// event/selection/UI surfaces. Disposal is the host's rollback boundary: it removes
 /// registrations even when plugin Initialize/Enable/Disable code throws.
 /// </summary>
 internal sealed class ScopedPluginHost : IPluginHost, IDisposable
 {
     private readonly IPluginHost _inner;
     private readonly ScopedEvents _events;
+    private readonly ScopedSelectionService _selection;
     private readonly ScopedUiRegistry _ui;
     private bool _disposed;
 
@@ -18,6 +19,7 @@ internal sealed class ScopedPluginHost : IPluginHost, IDisposable
     {
         _inner = inner ?? throw new ArgumentNullException(nameof(inner));
         _events = new ScopedEvents(inner.Events);
+        _selection = new ScopedSelectionService(inner.Selection);
         _ui = new ScopedUiRegistry(inner.Ui);
     }
 
@@ -25,7 +27,7 @@ internal sealed class ScopedPluginHost : IPluginHost, IDisposable
     public IPluginLogger Log => _inner.Log;
     public IGameState State => _inner.State;
     public IEvents Events => _events;
-    public ISelectionService Selection => _inner.Selection;
+    public ISelectionService Selection => _selection;
     public IUiRegistry Ui => _ui;
 
     public void Dispose()
@@ -34,9 +36,108 @@ internal sealed class ScopedPluginHost : IPluginHost, IDisposable
             return;
         _disposed = true;
         _events.Dispose();
+        _selection.Dispose();
         _ui.Dispose();
     }
 
+    private sealed class ScopedSelectionService(ISelectionService inner)
+        : ISelectionService,
+          IDisposable
+    {
+        private readonly object _gate = new();
+        private readonly List<Action<SelectionChangedEvent>> _registrations = [];
+        private bool _disposed;
+
+        public uint? SelectedObjectId => inner.SelectedObjectId;
+        public uint? PreviousObjectId => inner.PreviousObjectId;
+
+        public event Action<SelectionChangedEvent> Changed
+        {
+            add
+            {
+                ArgumentNullException.ThrowIfNull(value);
+                try
+                {
+                    inner.Changed += value;
+                }
+                catch
+                {
+                    try { inner.Changed -= value; }
+                    catch { }
+                    throw;
+                }
+                lock (_gate)
+                {
+                    if (!_disposed)
+                    {
+                        _registrations.Add(value);
+                        return;
+                    }
+                }
+
+                try { inner.Changed -= value; }
+                catch { }
+                throw new ObjectDisposedException(nameof(ScopedSelectionService));
+            }
+            remove
+            {
+                if (value is null)
+                    return;
+                inner.Changed -= value;
+                lock (_gate)
+                    RemoveLast(value);
+            }
+        }
+
+        public bool Select(uint objectId)
+        {
+            lock (_gate)
+            {
+                ObjectDisposedException.ThrowIf(_disposed, this);
+                return inner.Select(objectId);
+            }
+        }
+
+        public bool Clear()
+        {
+            lock (_gate)
+            {
+                ObjectDisposedException.ThrowIf(_disposed, this);
+                return inner.Clear();
+            }
+        }
+
+        public void Dispose()
+        {
+            Action<SelectionChangedEvent>[] registrations;
+            lock (_gate)
+            {
+                if (_disposed)
+                    return;
+                _disposed = true;
+                registrations = _registrations.ToArray();
+                _registrations.Clear();
+            }
+
+            for (int index = registrations.Length - 1; index >= 0; index--)
+            {
+                try { inner.Changed -= registrations[index]; }
+                catch { }
+            }
+        }
+
+        private void RemoveLast(Action<SelectionChangedEvent> handler)
+        {
+            for (int index = _registrations.Count - 1; index >= 0; index--)
+            {
+                if (_registrations[index] != handler)
+                    continue;
+                _registrations.RemoveAt(index);
+                return;
+            }
+        }
+    }
+
     private sealed class ScopedEvents(IEvents inner) : IEvents, IDisposable
     {
         private readonly object _gate = new();