From f820eb258d4cd8dc30364de121b72d1cbac29b7d Mon Sep 17 00:00:00 2001 From: Erik Date: Fri, 14 Aug 2026 19:28:14 +0200 Subject: [PATCH] fix(plugins): close LA5 ownership races --- docs/plans/2026-08-14-launcher-campaign.md | 10 +- .../2026-08-14-launcher-campaign-design.md | 4 +- src/AcDream.Core/Plugins/LoadedPlugin.cs | 14 +- src/AcDream.Core/Plugins/PluginLoader.cs | 28 ++- src/AcDream.Core/Plugins/PluginSession.cs | 42 ++++- src/AcDream.Core/Plugins/ScopedPluginHost.cs | 105 ++++++++++- .../Plugins/HeadlessPluginHost.cs | 21 ++- .../Launching/SessionConfigDocument.cs | 6 +- .../Entities/RuntimeEntityDirectory.cs | 176 ++++++++++++------ .../Entities/RuntimeEntityObjectViews.cs | 4 + .../Plugins/GraphicalPluginSessionTests.cs | 64 ++++++- .../Plugins/PluginLoaderTests.cs | 2 + .../HeadlessPluginSessionTests.cs | 24 ++- .../HostPlugin.cs | 74 +++++++- 14 files changed, 467 insertions(+), 107 deletions(-) diff --git a/docs/plans/2026-08-14-launcher-campaign.md b/docs/plans/2026-08-14-launcher-campaign.md index 00913fb2..0de911ff 100644 --- a/docs/plans/2026-08-14-launcher-campaign.md +++ b/docs/plans/2026-08-14-launcher-campaign.md @@ -155,7 +155,11 @@ Field rules: for gui/guiSelect/probe. - `credential`: always `{ "provider": "standardInput", "reference": "session" }` for launcher-composed configs. -- `plugins`/`loginCommands`/`loginCommandDelayMs`/`statusFile`: optional, +- `plugins`: absent/null means load all discovered plugins (preserving the + developer flow); explicit `[]` means load none. Launcher-composed + normal-empty and probe sessions emit `[]` so they cannot load arbitrary + machine-local plugins. +- `loginCommands`/`loginCommandDelayMs`/`statusFile`: optional, omitted-when-unset (never null, never `[]` for empty). Absent `loginCommandDelayMs` means 500. @@ -297,7 +301,9 @@ are backed by Core-owned types already; only `Ui` (`BufferedUiRegistry`) is genuinely App-only. Headless has zero plugin hosting today (confirmed). 1. Session-config `Plugins` allow-list filters the discovery result on BOTH - hosts (absent list = load all, preserving today's dev behavior). + hosts (absent/null list = load all, preserving today's dev behavior; + explicit `[]` = load none). Launcher-composed normal-empty and probe + sessions emit `[]`. 2. `HeadlessPluginHost : IPluginHost` in Headless over the same Core-owned `State`/`Events`/`Selection`; `Ui` is an explicit no-op behind a new capability flag on `IPluginHost` (e.g. `HasUi`) so plugins can detect diff --git a/docs/superpowers/specs/2026-08-14-launcher-campaign-design.md b/docs/superpowers/specs/2026-08-14-launcher-campaign-design.md index 42360f1d..fb4a37d6 100644 --- a/docs/superpowers/specs/2026-08-14-launcher-campaign-design.md +++ b/docs/superpowers/specs/2026-08-14-launcher-campaign-design.md @@ -168,7 +168,9 @@ Hand-editability is a property of the format, not a required workflow. `HeadlessConfiguration` shape extended with: - `Plugins: string[]` — plugin names to load from the standard - `PluginsDirectory`; hosts load exactly this set. + `PluginsDirectory`; absent/null loads all discovered plugins, while an + explicit empty array loads none. Launcher-composed normal-empty and probe + sessions emit the empty array. - `LoginCommands: string[]` — ordered chat-typed strings. - Graphical host: `Character` selector may be ABSENT → character-select screen instead of auto-enter. diff --git a/src/AcDream.Core/Plugins/LoadedPlugin.cs b/src/AcDream.Core/Plugins/LoadedPlugin.cs index ad48f6db..9f1f534a 100644 --- a/src/AcDream.Core/Plugins/LoadedPlugin.cs +++ b/src/AcDream.Core/Plugins/LoadedPlugin.cs @@ -7,17 +7,17 @@ namespace AcDream.Core.Plugins; /// Outcome of a plugin load attempt. /// On success, is the instantiated plugin, /// owns its assembly, and is null. -/// On failure, and are null, -/// describes what went wrong, and -/// weakly observes any collectible context -/// that was already released during rollback. +/// On failure, describes what went wrong. A partial +/// and/or 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. /// 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; } diff --git a/src/AcDream.Core/Plugins/PluginLoader.cs b/src/AcDream.Core/Plugins/PluginLoader.cs index 54042a51..1d729f2a 100644 --- a/src/AcDream.Core/Plugins/PluginLoader.cs +++ b/src/AcDream.Core/Plugins/PluginLoader.cs @@ -11,6 +11,8 @@ public static class PluginLoader /// implementing , instantiate it, and call its /// with the supplied host. Any failure /// is returned as a failed rather than thrown. + /// A returned partial plugin/context remains caller-owned; this method never + /// requests unload because the caller must close host registrations first. /// 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); } } } diff --git a/src/AcDream.Core/Plugins/PluginSession.cs b/src/AcDream.Core/Plugins/PluginSession.cs index dfbbb3ef..1436fb6b 100644 --- a/src/AcDream.Core/Plugins/PluginSession.cs +++ b/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) diff --git a/src/AcDream.Core/Plugins/ScopedPluginHost.cs b/src/AcDream.Core/Plugins/ScopedPluginHost.cs index 61786b22..ee1667f1 100644 --- a/src/AcDream.Core/Plugins/ScopedPluginHost.cs +++ b/src/AcDream.Core/Plugins/ScopedPluginHost.cs @@ -4,13 +4,14 @@ namespace AcDream.Core.Plugins; /// /// 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. /// 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> _registrations = []; + private bool _disposed; + + public uint? SelectedObjectId => inner.SelectedObjectId; + public uint? PreviousObjectId => inner.PreviousObjectId; + + public event Action 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[] 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 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(); diff --git a/src/AcDream.Headless/Plugins/HeadlessPluginHost.cs b/src/AcDream.Headless/Plugins/HeadlessPluginHost.cs index 5eda1706..ca5b6d5a 100644 --- a/src/AcDream.Headless/Plugins/HeadlessPluginHost.cs +++ b/src/AcDream.Headless/Plugins/HeadlessPluginHost.cs @@ -52,8 +52,9 @@ internal sealed class HeadlessPluginHost public ISelectionService Selection => _runtime.ActionOwner.Selection; public IUiRegistry Ui => NoOpUiRegistry.Instance; - /// Test-only barrier after the borrowed replay snapshot is - /// captured and before delivery starts. + /// Test-only barrier invoked after the first replay item is + /// captured while Runtime's exact active-membership read lease is still + /// held. internal Action? ReplayCapturedForTest { get; set; } /// @@ -88,11 +89,12 @@ internal sealed class HeadlessPluginHost // Arm the pending queue before borrowing Runtime's snapshot. This // avoids a host-lock/Runtime-lock inversion while the identity // dedup below collapses any registration present in both views. - var visitor = new SnapshotVisitor(_runtime); + var visitor = new SnapshotVisitor( + _runtime, + ReplayCapturedForTest); _runtime.Entities.Visit(visitor); ReplayEntity[] replay = visitor.Items.ToArray(); - ReplayCapturedForTest?.Invoke(); foreach (ReplayEntity item in replay) { lock (_eventGate) @@ -234,16 +236,23 @@ internal sealed class HeadlessPluginHost catch { } } - private sealed class SnapshotVisitor(GameRuntime runtime) + private sealed class SnapshotVisitor( + GameRuntime runtime, + Action? captureBarrier = null) : IRuntimeEntityVisitor { + private Action? _captureBarrier = captureBarrier; + internal List Items { get; } = new(runtime.Entities.Count); - public void Visit(in RuntimeEntitySnapshot entity) => + public void Visit(in RuntimeEntitySnapshot entity) + { Items.Add(new ReplayEntity( entity.Identity, Convert(runtime, entity))); + Interlocked.Exchange(ref _captureBarrier, null)?.Invoke(); + } } private void RebuildLiveSnapshotLocked() diff --git a/src/AcDream.Launcher.Core/Launching/SessionConfigDocument.cs b/src/AcDream.Launcher.Core/Launching/SessionConfigDocument.cs index 0f013f53..1fc31f4d 100644 --- a/src/AcDream.Launcher.Core/Launching/SessionConfigDocument.cs +++ b/src/AcDream.Launcher.Core/Launching/SessionConfigDocument.cs @@ -98,8 +98,10 @@ public sealed class SessionDescriptor public SessionCredentialDescriptor Credential { get; init; } = new(); - /// Omitted (never an empty array) when the character has no - /// configured plugin set. + /// Plugin allow-list. Omitted or JSON null means load all + /// discovered plugins (the developer flow); an explicit empty array means + /// load none. Launcher-composed normal-empty and probe sessions therefore + /// emit []. public List? Plugins { get; init; } /// Omitted (never an empty array) when the character has no diff --git a/src/AcDream.Runtime/Entities/RuntimeEntityDirectory.cs b/src/AcDream.Runtime/Entities/RuntimeEntityDirectory.cs index fe733332..98dbd9cb 100644 --- a/src/AcDream.Runtime/Entities/RuntimeEntityDirectory.cs +++ b/src/AcDream.Runtime/Entities/RuntimeEntityDirectory.cs @@ -16,6 +16,7 @@ public sealed class RuntimeEntityDirectory public const uint LastLocalEntityId = 0x3FFF_FFFFu; private readonly InboundPhysicsStateController _inbound = new(); + private readonly object _activeGate = new(); private readonly Dictionary _activeByGuid = new(); private readonly Dictionary<(uint Guid, ushort Incarnation), RuntimeEntityRecord> _teardownByIncarnation = new(); @@ -35,10 +36,27 @@ public sealed class RuntimeEntityDirectory _nextLocalEntityId = firstLocalEntityId; } - public int Count => _activeByGuid.Count; + public int Count + { + get + { + lock (_activeGate) + return _activeByGuid.Count; + } + } public int PendingTeardownCount => _teardownByIncarnation.Count; - public int ClaimedLocalIdCount => _byLocalId.Count; + public int ClaimedLocalIdCount + { + get + { + lock (_activeGate) + return _byLocalId.Count; + } + } public ulong SessionLifetimeVersion { get; private set; } + /// Update-thread-only borrowed collection. Cross-thread hosts use + /// through IRuntimeEntityView.Visit + /// so membership cannot change during enumeration. public IReadOnlyCollection ActiveRecords => _activeByGuid.Values; public IReadOnlyCollection TeardownRecords => _teardownByIncarnation.Values; @@ -87,34 +105,48 @@ public sealed class RuntimeEntityDirectory public RuntimeEntityRecord AddActive(WorldSession.EntitySpawn snapshot) { - if (_activeByGuid.ContainsKey(snapshot.Guid)) + lock (_activeGate) { - throw new InvalidOperationException( - $"Live entity 0x{snapshot.Guid:X8} already has an active incarnation."); - } + if (_activeByGuid.ContainsKey(snapshot.Guid)) + { + throw new InvalidOperationException( + $"Live entity 0x{snapshot.Guid:X8} already has an active incarnation."); + } - var record = new RuntimeEntityRecord(snapshot); - _activeByGuid.Add(snapshot.Guid, record); - try - { - ClaimLocalId(record); - return record; - } - catch - { - _activeByGuid.Remove(snapshot.Guid); - throw; + var record = new RuntimeEntityRecord(snapshot); + _activeByGuid.Add(snapshot.Guid, record); + try + { + ClaimLocalId(record); + return record; + } + catch + { + _activeByGuid.Remove(snapshot.Guid); + throw; + } } } - public bool RemoveActive(uint guid, out RuntimeEntityRecord? record) => - _activeByGuid.Remove(guid, out record); + public bool RemoveActive(uint guid, out RuntimeEntityRecord? record) + { + lock (_activeGate) + return _activeByGuid.Remove(guid, out record); + } public bool RemoveActive(RuntimeEntityRecord expected) { - if (!IsCurrent(expected)) - return false; - return _activeByGuid.Remove(expected.ServerGuid); + lock (_activeGate) + { + if (!_activeByGuid.TryGetValue( + expected.ServerGuid, + out RuntimeEntityRecord? current) + || !ReferenceEquals(current, expected)) + { + return false; + } + return _activeByGuid.Remove(expected.ServerGuid); + } } public void RetainTeardown(RuntimeEntityRecord record) @@ -157,46 +189,52 @@ public sealed class RuntimeEntityDirectory public uint ClaimLocalId(RuntimeEntityRecord record) { - if (!IsKnown(record)) + lock (_activeGate) { - throw new InvalidOperationException( - "A local id can only be claimed for an active or retained incarnation."); + if (!IsKnown(record)) + { + throw new InvalidOperationException( + "A local id can only be claimed for an active or retained incarnation."); + } + + if (record.LocalEntityId is { } existing) + return existing; + + uint start = _nextLocalEntityId; + do + { + uint candidate = _nextLocalEntityId; + _nextLocalEntityId = candidate == LastLocalEntityId + ? FirstLocalEntityId + : candidate + 1u; + if (_byLocalId.ContainsKey(candidate)) + continue; + + _byLocalId.Add(candidate, record); + record.LocalEntityId = candidate; + return candidate; + } + while (_nextLocalEntityId != start); + + throw new InvalidOperationException("The live entity id namespace is exhausted."); } - - if (record.LocalEntityId is { } existing) - return existing; - - uint start = _nextLocalEntityId; - do - { - uint candidate = _nextLocalEntityId; - _nextLocalEntityId = candidate == LastLocalEntityId - ? FirstLocalEntityId - : candidate + 1u; - if (_byLocalId.ContainsKey(candidate)) - continue; - - _byLocalId.Add(candidate, record); - record.LocalEntityId = candidate; - return candidate; - } - while (_nextLocalEntityId != start); - - throw new InvalidOperationException("The live entity id namespace is exhausted."); } public bool ReleaseLocalId(RuntimeEntityRecord record) { - if (record.LocalEntityId is not { } localId) - return false; - if (_byLocalId.TryGetValue(localId, out RuntimeEntityRecord? retained) - && ReferenceEquals(retained, record)) + lock (_activeGate) { - _byLocalId.Remove(localId); - } + if (record.LocalEntityId is not { } localId) + return false; + if (_byLocalId.TryGetValue(localId, out RuntimeEntityRecord? retained) + && ReferenceEquals(retained, record)) + { + _byLocalId.Remove(localId); + } - record.LocalEntityId = null; - return true; + record.LocalEntityId = null; + return true; + } } public ulong AdvanceLifetimeMutation(uint serverGuid) @@ -219,15 +257,39 @@ public sealed class RuntimeEntityDirectory public bool CompleteSessionClearIfConverged() { - if (_activeByGuid.Count != 0 || _teardownByIncarnation.Count != 0) - return false; + lock (_activeGate) + { + if (_activeByGuid.Count != 0 || _teardownByIncarnation.Count != 0) + return false; - _byLocalId.Clear(); + _byLocalId.Clear(); + } ParentAttachments.Clear(); _inbound.Clear(); return true; } + /// + /// Enters the exact active-membership read boundary. Add/remove and local-id + /// membership commits serialize behind this allocation-free lease; record + /// ownership remains canonical here and no copied gameplay collection is + /// introduced. + /// + internal ActiveReadLease AcquireActiveRead() => new(_activeGate); + + internal readonly struct ActiveReadLease : IDisposable + { + private readonly object _gate; + + internal ActiveReadLease(object gate) + { + _gate = gate; + Monitor.Enter(gate); + } + + public void Dispose() => Monitor.Exit(_gate); + } + public void RefreshSnapshot( RuntimeEntityRecord record, WorldSession.EntitySpawn accepted, diff --git a/src/AcDream.Runtime/Entities/RuntimeEntityObjectViews.cs b/src/AcDream.Runtime/Entities/RuntimeEntityObjectViews.cs index 26f27e23..8ac2d909 100644 --- a/src/AcDream.Runtime/Entities/RuntimeEntityObjectViews.cs +++ b/src/AcDream.Runtime/Entities/RuntimeEntityObjectViews.cs @@ -91,6 +91,8 @@ internal sealed class RuntimeEntityObjectViews uint serverGuid, out RuntimeEntitySnapshot entity) { + using RuntimeEntityDirectory.ActiveReadLease lease = + owner.AcquireActiveRead(); if (owner.TryGetActive( serverGuid, out RuntimeEntityRecord record)) @@ -106,6 +108,8 @@ internal sealed class RuntimeEntityObjectViews public void Visit(IRuntimeEntityVisitor visitor) { ArgumentNullException.ThrowIfNull(visitor); + using RuntimeEntityDirectory.ActiveReadLease lease = + owner.AcquireActiveRead(); foreach (RuntimeEntityRecord record in owner.ActiveRecords) { RuntimeEntitySnapshot entity = Snapshot(record); diff --git a/tests/AcDream.App.Tests/Plugins/GraphicalPluginSessionTests.cs b/tests/AcDream.App.Tests/Plugins/GraphicalPluginSessionTests.cs index 8f4a2e10..97850c53 100644 --- a/tests/AcDream.App.Tests/Plugins/GraphicalPluginSessionTests.cs +++ b/tests/AcDream.App.Tests/Plugins/GraphicalPluginSessionTests.cs @@ -15,6 +15,8 @@ public sealed class GraphicalPluginSessionTests { private const string FixtureId = "acdream.test.host-fixture"; private const string ThrowingId = "acdream.test.throwing-fixture"; + private const string InitializeThrowingId = + "acdream.test.initialize-throwing-fixture"; [Fact] public void ConfiguredSetLoadsOnlyAllowedPluginAndReportsBothOutcomes() @@ -114,12 +116,13 @@ public sealed class GraphicalPluginSessionTests string.Empty); string statusPath = Path.Combine(temporary.Path, "status.jsonl"); var events = new WorldEvents(); + var selection = new SelectionState(); var ui = new BufferedUiRegistry(); var host = new AppPluginHost( new CapturingLogger(), new WorldGameState(), events, - new SelectionState(), + selection, ui); using GraphicalPluginSession plugins = GraphicalPluginSession.Create( @@ -138,6 +141,65 @@ public sealed class GraphicalPluginSessionTests 2u, default, System.Numerics.Quaternion.Identity)); + Assert.True(((ISelectionService)selection).Select(7u)); + Assert.False(File.Exists( + Path.Combine(pluginDirectory, "unexpected-callback"))); + Assert.Equal( + ["started", "pluginFailed"], + EventNames(ReadStatuses(statusPath))); + + WeakReference context = Assert.Single( + plugins.CaptureLoadContextWeakReferences()); + plugins.Dispose(); + Collect(context); + Assert.False(context.IsAlive); + } + + [Fact] + public void InitializeFailureRollsBackEveryRegistrationBeforeUnload() + { + using var temporary = new TemporaryDirectory(); + ApplicationPathSet paths = Paths(temporary.Path); + string pluginDirectory = InstallFixture( + paths.PluginsDirectory, + InitializeThrowingId, + "initialize-throwing-fixture"); + File.WriteAllText( + Path.Combine(pluginDirectory, "throw-during-initialize"), + string.Empty); + string statusPath = Path.Combine(temporary.Path, "status.jsonl"); + var events = new WorldEvents(); + var selection = new SelectionState(); + var ui = new BufferedUiRegistry(); + var host = new AppPluginHost( + new CapturingLogger(), + new WorldGameState(), + events, + selection, + ui); + + using GraphicalPluginSession plugins = GraphicalPluginSession.Create( + paths, + [InitializeThrowingId], + "gui-session", + host, + new SessionStatusWriter(statusPath)); + plugins.Start(); + + Assert.Equal(0, plugins.LoadedCount); + Assert.Empty(ui.Drain()); + Assert.Equal(0, ui.RegistrationCount); + Assert.Equal( + "ui=True;events=True;selection=True", + File.ReadAllText(Path.Combine( + pluginDirectory, + "unload-observation"))); + events.FireEntitySpawned(new WorldEntitySnapshot( + 1u, + 2u, + default, + System.Numerics.Quaternion.Identity)); + Assert.True(((ISelectionService)selection).Select(9u)); Assert.False(File.Exists( Path.Combine(pluginDirectory, "unexpected-callback"))); Assert.Equal( diff --git a/tests/AcDream.Core.Tests/Plugins/PluginLoaderTests.cs b/tests/AcDream.Core.Tests/Plugins/PluginLoaderTests.cs index 3deebb94..e35755a7 100644 --- a/tests/AcDream.Core.Tests/Plugins/PluginLoaderTests.cs +++ b/tests/AcDream.Core.Tests/Plugins/PluginLoaderTests.cs @@ -125,5 +125,7 @@ public class PluginLoaderTests Assert.False(loaded.Success); Assert.Contains("IAcDreamPlugin", loaded.Error!.Message); + Assert.NotNull(loaded.LoadContext); + loaded.LoadContext!.Unload(); } } diff --git a/tests/AcDream.Headless.Tests/HeadlessPluginSessionTests.cs b/tests/AcDream.Headless.Tests/HeadlessPluginSessionTests.cs index e2aa76ad..c2f332ef 100644 --- a/tests/AcDream.Headless.Tests/HeadlessPluginSessionTests.cs +++ b/tests/AcDream.Headless.Tests/HeadlessPluginSessionTests.cs @@ -125,6 +125,8 @@ public sealed class HeadlessPluginSessionTests _ = session.Start(); Assert.Equal(0, session.Plugins.LoadedCount); _ = session.Runtime.EntityObjects.RegisterEntity(Spawn(0x50000001u, 1f)); + Assert.True(((ISelectionService)session.Runtime.ActionOwner.Selection) + .Select(7u)); Assert.False(File.Exists( Path.Combine(pluginDirectory, "unexpected-callback"))); Assert.Equal( @@ -203,9 +205,25 @@ public sealed class HeadlessPluginSessionTests Task subscribe = Task.Run(() => host.Events.EntitySpawned += handler); Assert.True(replayCaptured.Wait(TimeSpan.FromSeconds(10))); - _ = session.Runtime.EntityObjects.RegisterEntity(Spawn(0x50000002u, 2f)); - releaseReplay.Set(); - await subscribe.WaitAsync(TimeSpan.FromSeconds(10)); + using var registrationStarted = new ManualResetEventSlim(); + Task registration = Task.Run(() => + { + registrationStarted.Set(); + _ = session.Runtime.EntityObjects.RegisterEntity( + Spawn(0x50000002u, 2f)); + }); + Assert.True(registrationStarted.Wait(TimeSpan.FromSeconds(10))); + try + { + await Task.Delay(TimeSpan.FromMilliseconds(100)); + Assert.False(registration.IsCompleted); + } + finally + { + releaseReplay.Set(); + } + await Task.WhenAll(subscribe, registration) + .WaitAsync(TimeSpan.FromSeconds(10)); host.Events.EntitySpawned -= handler; Assert.Equal([1_000_000u, 1_000_001u], observed); diff --git a/tests/AcDream.Plugin.Tests.Fixtures.HostPlugin/HostPlugin.cs b/tests/AcDream.Plugin.Tests.Fixtures.HostPlugin/HostPlugin.cs index 5349fcd7..b294f697 100644 --- a/tests/AcDream.Plugin.Tests.Fixtures.HostPlugin/HostPlugin.cs +++ b/tests/AcDream.Plugin.Tests.Fixtures.HostPlugin/HostPlugin.cs @@ -1,4 +1,5 @@ using AcDream.Plugin.Abstractions; +using System.Runtime.Loader; namespace AcDream.Plugin.Tests.Fixtures.HostPlugin; @@ -13,6 +14,7 @@ public sealed class HostPlugin : IAcDreamPlugin private IPluginHost? _host; private string? _assemblyDirectory; private bool _throwAfterRegistration; + private bool _throwDuringInitialize; private int _entitiesSeen; public void Initialize(IPluginHost host) @@ -22,17 +24,24 @@ public sealed class HostPlugin : IAcDreamPlugin typeof(HostPlugin).Assembly.Location); _throwAfterRegistration = File.Exists( Path.Combine(_assemblyDirectory!, "throw-after-register")); + _throwDuringInitialize = File.Exists( + Path.Combine(_assemblyDirectory!, "throw-during-initialize")); host.Log.Info($"fixture-initialized:hasUi={host.HasUi}"); + if (_throwDuringInitialize) + { + RegisterHostCallbacks(host); + AssemblyLoadContext.GetLoadContext(typeof(HostPlugin).Assembly)! + .Unloading += OnUnloading; + throw new InvalidOperationException( + "fixture initialize failed after registering UI, entity, and selection callbacks"); + } } public void Enable() { IPluginHost host = _host ?? throw new InvalidOperationException("The fixture was not initialized."); - host.Ui.AddMarkupPanel( - Path.Combine(AppContext.BaseDirectory, "fixture-panel.xml"), - this); - host.Events.EntitySpawned += OnEntitySpawned; + RegisterHostCallbacks(host); if (_throwAfterRegistration) { throw new InvalidOperationException( @@ -47,24 +56,75 @@ public sealed class HostPlugin : IAcDreamPlugin IPluginHost? host = _host; if (host is null) return; - if (_throwAfterRegistration) + if (_throwAfterRegistration || _throwDuringInitialize) { throw new InvalidOperationException( "fixture disable intentionally refuses cleanup"); } host.Events.EntitySpawned -= OnEntitySpawned; + host.Selection.Changed -= OnSelectionChanged; host.Log.Info($"fixture-disabled:entitiesSeen={_entitiesSeen}"); _host = null; } + private void RegisterHostCallbacks(IPluginHost host) + { + host.Ui.AddMarkupPanel( + Path.Combine(AppContext.BaseDirectory, "fixture-panel.xml"), + this); + host.Events.EntitySpawned += OnEntitySpawned; + host.Selection.Changed += OnSelectionChanged; + } + private void OnEntitySpawned(WorldEntitySnapshot snapshot) { _entitiesSeen++; - if (_throwAfterRegistration && _assemblyDirectory is not null) + RecordUnexpectedCallback(snapshot.Id); + } + + private void OnSelectionChanged(SelectionChangedEvent change) => + RecordUnexpectedCallback(change.SelectedObjectId ?? 0u); + + private void RecordUnexpectedCallback(uint objectId) + { + if ((_throwAfterRegistration || _throwDuringInitialize) + && _assemblyDirectory is not null) { File.AppendAllText( Path.Combine(_assemblyDirectory, "unexpected-callback"), - $"{snapshot.Id}{Environment.NewLine}"); + $"{objectId}{Environment.NewLine}"); + } + } + + private void OnUnloading(AssemblyLoadContext context) + { + IPluginHost host = _host!; + bool uiClosed = Rejects(() => host.Ui.AddMarkupPanel( + Path.Combine(AppContext.BaseDirectory, "unloading-panel.xml"), + this)); + bool eventsClosed = Rejects(() => + { + host.Events.EntitySpawned += OnEntitySpawned; + }); + bool selectionClosed = Rejects(() => + { + host.Selection.Changed += OnSelectionChanged; + }); + File.WriteAllText( + Path.Combine(_assemblyDirectory!, "unload-observation"), + $"ui={uiClosed};events={eventsClosed};selection={selectionClosed}"); + } + + private static bool Rejects(Action action) + { + try + { + action(); + return false; + } + catch (ObjectDisposedException) + { + return true; } } }