diff --git a/docs/adr/0002-plugin-activation-lifecycle.md b/docs/adr/0002-plugin-activation-lifecycle.md index 069bc82..f12e95a 100644 --- a/docs/adr/0002-plugin-activation-lifecycle.md +++ b/docs/adr/0002-plugin-activation-lifecycle.md @@ -23,6 +23,11 @@ On disable, the base clears the active Client reference first, invokes `OnClient reverse order, drains Client-owned CE resources while the host is still attached, and finally disposes the scope, provider, and configuration. Cleanup is best-effort and aggregates failures only after every step has been attempted. +Before an activation is published, construction has a separate rollback path. It releases only the stages that were +actually acquired—scope, provider, then the host-created configuration—in reverse construction order. Each stage gets +one independent cleanup attempt. The construction exception remains the primary failure; cleanup failures follow it as +diagnostics. Module callbacks and Client-owned CE resource draining are never run for an incomplete activation. + ## Invariants - SDK calls are forbidden from plugin constructors, field initializers, and static initialization. SDK-dependent @@ -33,6 +38,8 @@ provider, and configuration. Cleanup is best-effort and aggregates failures only epoch. `ICheatEngineClient.Epoch` and `Stopping` identify the active lifetime. - Public Client operations are synchronous. They do not retain Lua state across an `await`, and main-thread work enters the SDK dispatcher as a bounded operation. +- The DI container owns disposal of services it created. Hosting owns the `ConfigurationManager` instance it created + and releases it once after the scope and provider; it never disposes resolved services individually. ## Consequences and project value diff --git a/libs/CheatEngine.Client.Hosting/CheatEngineClientPlugin.cs b/libs/CheatEngine.Client.Hosting/CheatEngineClientPlugin.cs index 84eafec..6c4dd0a 100644 --- a/libs/CheatEngine.Client.Hosting/CheatEngineClientPlugin.cs +++ b/libs/CheatEngine.Client.Hosting/CheatEngineClientPlugin.cs @@ -11,7 +11,7 @@ namespace CheatEngine.Client.Hosting; -/// Base class that activates a scoped for each Cheat Engine enable epoch. +/// Base class that activates an activation-owned for each Cheat Engine enable epoch. /// /// The SDK constructs a plugin through a parameterless factory and reuses that instance across enable/disable cycles. /// This base class therefore creates a fresh validated provider and scope only from , when the @@ -73,12 +73,13 @@ protected sealed override void OnEnable() } CheatEnginePluginBuilder builder = new(); + ActivationConstruction construction = new(builder); Activation? activation = null; try { Configure(builder); - activation = CreateActivation(builder); + activation = CreateActivation(construction); Volatile.Write(ref _activation, activation); activation.Lifecycle.Enable(OnClientEnabled); @@ -88,14 +89,15 @@ protected sealed override void OnEnable() { if (activation is null) { - builder.ReleaseConfiguration(); - throw; + RethrowAfterCleanup(enableFailure, CleanupUnpublishedActivation(construction)); + } + else + { + Interlocked.CompareExchange(ref _activation, null, activation); + ClientHostingLog.ActivationRollingBack(activation.Logger, activation.Client.Epoch); + RethrowAfterCleanup(enableFailure, + CleanupActivation(activation)); } - - Interlocked.CompareExchange(ref _activation, null, activation); - ClientHostingLog.ActivationRollingBack(activation.Logger, activation.Client.Epoch); - RethrowAfterCleanup(enableFailure, - CleanupActivation(activation)); } } @@ -125,37 +127,26 @@ protected sealed override void OnDisable() /// Builds and resolves the complete activation graph before publishing it to the plugin instance. /// /// The activation is not returned until options validation, the Client facade, modules, logging, and cleanup support - /// have all resolved from one scope. If any step fails, this method releases the scope, provider, and configuration - /// before propagating the original failure so a partially built epoch can never become observable. + /// have all resolved from one scope. The outer activation transaction records each acquired component before the + /// next step, so its failure-independent rollback can release a partial graph without invoking lifecycle callbacks. /// - private static Activation CreateActivation(CheatEnginePluginBuilder builder) + private static Activation CreateActivation(ActivationConstruction construction) { - ServiceProvider? provider = null; - IServiceScope? scope = null; - - try - { - provider = builder.BuildServiceProvider(); - scope = provider.CreateScope(); - - // IOptions.Value invokes the generated validator in plugin hosts that do not run Generic Host startup. - _ = scope.ServiceProvider.GetRequiredService>().Value; - ICheatEngineClient client = scope.ServiceProvider.GetRequiredService(); - ICheatEngineClientModule[] modules = GetModules(scope.ServiceProvider); - ILogger logger = - scope.ServiceProvider.GetRequiredService>(); - ICheatEngineClientActivationCleanup cleanup = - scope.ServiceProvider.GetRequiredService(); - - return new Activation(builder, provider, scope, client, modules, logger, cleanup); - } - catch - { - scope?.Dispose(); - provider?.Dispose(); - builder.ReleaseConfiguration(); - throw; - } + ServiceProvider provider = construction.Builder.BuildServiceProvider(); + construction.Provider = provider; + IServiceScope scope = provider.CreateScope(); + construction.Scope = scope; + + // IOptions.Value invokes the generated validator in plugin hosts that do not run Generic Host startup. + _ = scope.ServiceProvider.GetRequiredService>().Value; + ICheatEngineClient client = scope.ServiceProvider.GetRequiredService(); + ICheatEngineClientModule[] modules = GetModules(scope.ServiceProvider); + ILogger logger = + scope.ServiceProvider.GetRequiredService>(); + ICheatEngineClientActivationCleanup cleanup = + scope.ServiceProvider.GetRequiredService(); + + return new Activation(construction.Builder, provider, scope, client, modules, logger, cleanup); } private static ICheatEngineClientModule[] GetModules(IServiceProvider services) @@ -181,6 +172,29 @@ private static void RethrowAfterCleanup(Exception enableFailure, List cleanupFailures); } + /// Releases a partial activation without invoking callbacks that were never admitted. + /// + /// Provider construction happens before scope construction, and configuration outlives both. Each completed + /// construction stage is therefore released once in reverse order. Cleanup failures are recorded instead of + /// interrupting the remaining stages, so the original construction failure remains the first reported failure. + /// + private static List CleanupUnpublishedActivation(ActivationConstruction construction) + { + List failures = []; + if (construction.Scope is not null) + { + TryCleanup(failures, construction.Scope.Dispose); + } + + if (construction.Provider is not null) + { + TryCleanup(failures, construction.Provider.Dispose); + } + + TryCleanup(failures, construction.Builder.ReleaseConfiguration); + return failures; + } + /// Closes an activation in the only safe disposal order and collects every cleanup failure. /// /// Module callbacks and Client-owned Cheat Engine resources run while the SDK context is valid. The activation scope, @@ -256,6 +270,18 @@ private static void ThrowCleanupFailures(List failures) throw new AggregateException("Client deactivation encountered one or more cleanup failures.", failures); } + private static void TryCleanup(List failures, Action cleanup) + { + try + { + cleanup(); + } + catch (Exception exception) + { + failures.Add(exception); + } + } + private ICheatEngineClient GetActiveClient() { Activation? activation = Volatile.Read(ref _activation); @@ -269,6 +295,26 @@ private ICheatEngineClient GetActiveClient() "The Cheat Engine client is available only while the plugin is enabled."); } + private sealed class ActivationConstruction(CheatEnginePluginBuilder builder) + { + internal CheatEnginePluginBuilder Builder + { + get; + } = builder; + + internal ServiceProvider? Provider + { + get; + set; + } + + internal IServiceScope? Scope + { + get; + set; + } + } + private sealed class Activation { internal Activation( diff --git a/libs/CheatEngine.Client.Hosting/README.md b/libs/CheatEngine.Client.Hosting/README.md index b746f15..af78db4 100644 --- a/libs/CheatEngine.Client.Hosting/README.md +++ b/libs/CheatEngine.Client.Hosting/README.md @@ -31,6 +31,13 @@ On disable—or if activation fails—the host invokes `OnClientDisabling` and m Client-owned Cheat Engine resources while the SDK context remains valid, disposes the scope and provider, and finally disposes activation configuration. Cleanup failures are aggregated after all cleanup opportunities have run. +Construction failure has a narrower rollback: only resources acquired before publication are released, once each, in +reverse construction order (`scope → provider → configuration`). Every stage is attempted even if an earlier disposal +fails. The original configuration, validation, or service-resolution failure remains primary; cleanup failures are +attached as secondary diagnostics. No module callback or Client-owned resource drain runs until an activation has been +created and published. The DI container disposes services it creates; Hosting explicitly releases its host-created +`ConfigurationManager` only after the scope and provider have been released. + Add configuration sources explicitly and keep reload disabled. The following is the normal plugin shape: ```csharp diff --git a/tests/CheatEngine.Client.Hosting.Tests/CheatEngineClientPluginTests.cs b/tests/CheatEngine.Client.Hosting.Tests/CheatEngineClientPluginTests.cs index 3bab725..e0bad9d 100644 --- a/tests/CheatEngine.Client.Hosting.Tests/CheatEngineClientPluginTests.cs +++ b/tests/CheatEngine.Client.Hosting.Tests/CheatEngineClientPluginTests.cs @@ -21,6 +21,7 @@ using CheatEngine.Client.Tables; using CheatEngine.Client.Timers; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; namespace CheatEngine.Client.Hosting.Tests; @@ -226,6 +227,119 @@ public void ConfigureFailureDoesNotPublishAnActivation() plugin.DisableForTest(); } + [Fact] + public void FailedConstructionCleansEveryAcquiredStageAndAllowsANewEnableEpoch() + { + List events = []; + FakeClient client = new(48); + RecordingCleanup cleanup = new(events); + int configureAttempt = 0; + TestPlugin plugin = new(builder => + { + events.Add("configure"); + if (configureAttempt++ == 0) + { + AddFailingConstructionRegistrations(builder, events); + return; + } + + builder.Services.AddSingleton(client); + builder.Services.AddSingleton(cleanup); + }); + + AggregateException exception = Assert.Throws(plugin.EnableForTest); + + Assert.Collection( + exception.InnerExceptions, + failure => Assert.Equal("client resolution", failure.Message), + failure => Assert.Equal("scope dispose", failure.Message), + failure => Assert.Equal("provider dispose", failure.Message), + failure => Assert.Equal("configuration dispose", failure.Message)); + Assert.Equal( + ["configure", "client.resolve", "scope.dispose", "provider.dispose", "configuration.dispose"], + events); + Assert.Throws(plugin.GetRequiredClientForTest); + + plugin.DisableForTest(); + plugin.EnableForTest(); + + Assert.Same(client, plugin.GetRequiredClientForTest()); + plugin.DisableForTest(); + Assert.Equal(1, cleanup.DrainCount); + Assert.Equal(2, configureAttempt); + } + + [Fact] + public void ConfigureFailureRemainsPrimaryWhenConfigurationReleaseAlsoFails() + { + List events = []; + TestPlugin plugin = new(builder => + { + builder.Configuration.Sources.Add(new ThrowingDisposeConfigurationSource(events)); + throw new InvalidOperationException("configuration"); + }); + + AggregateException exception = Assert.Throws(plugin.EnableForTest); + + Assert.Collection( + exception.InnerExceptions, + failure => Assert.Equal("configuration", failure.Message), + failure => Assert.Equal("configuration dispose", failure.Message)); + Assert.Equal(["configuration.dispose"], events); + Assert.Throws(plugin.GetRequiredClientForTest); + } + + [Fact] + public void FailedModuleEnableRollsBackAndTheSamePluginCanEnableAgain() + { + List events = []; + FakeClient client = new(49); + RecordingCleanup cleanup = new(events); + FailOnceEnableState state = new(); + TestPlugin plugin = CreatePlugin(events, client, cleanup, builder => + { + builder.Services.AddSingleton(state); + builder.Client.AddModule(); + }); + + InvalidOperationException failure = Assert.Throws(plugin.EnableForTest); + + Assert.Equal("module enable", failure.Message); + Assert.Equal( + [ + "configure", "module.enabled", "cleanup.enter", "module.disabling", "cleanup.drain", "cleanup.exit" + ], + events); + Assert.Throws(plugin.GetRequiredClientForTest); + + plugin.EnableForTest(); + Assert.Same(client, plugin.GetRequiredClientForTest()); + plugin.DisableForTest(); + + Assert.Equal(2, cleanup.DrainCount); + Assert.Equal( + [ + "configure", "module.enabled", "cleanup.enter", "module.disabling", "cleanup.drain", "cleanup.exit", + "configure", "module.enabled", "client.enabled", "cleanup.enter", "client.disabling", "module.disabling", + "cleanup.drain", "cleanup.exit" + ], + events); + } + + private static void AddFailingConstructionRegistrations(CheatEnginePluginBuilder builder, List events) + { + builder.Configuration.Sources.Add(new ThrowingDisposeConfigurationSource(events)); + builder.Services.AddScoped(_ => new ThrowingScopedDisposable(events)); + builder.Services.AddSingleton(_ => new ThrowingSingletonDisposable(events)); + builder.Services.AddScoped(services => + { + _ = services.GetRequiredService(); + _ = services.GetRequiredService(); + events.Add("client.resolve"); + throw new InvalidOperationException("client resolution"); + }); + } + private static TestPlugin CreatePlugin( List events, FakeClient client, @@ -377,6 +491,33 @@ public void OnDisabling(ICheatEngineClient client) } } + public sealed class FailOnceEnableModule(List events, FailOnceEnableState state) : ICheatEngineClientModule + { + public void OnEnabled(ICheatEngineClient client) + { + events.Add("module.enabled"); + if (!state.HasFailed) + { + state.HasFailed = true; + throw new InvalidOperationException("module enable"); + } + } + + public void OnDisabling(ICheatEngineClient client) + { + events.Add("module.disabling"); + } + } + + public sealed class FailOnceEnableState + { + public bool HasFailed + { + get; + set; + } + } + private sealed class RecordingCleanup( List events, Exception? enterFailure = null, @@ -431,6 +572,41 @@ public void Dispose() } } + private sealed class ThrowingScopedDisposable(List events) : IDisposable + { + public void Dispose() + { + events.Add("scope.dispose"); + throw new InvalidOperationException("scope dispose"); + } + } + + private sealed class ThrowingSingletonDisposable(List events) : IDisposable + { + public void Dispose() + { + events.Add("provider.dispose"); + throw new InvalidOperationException("provider dispose"); + } + } + + private sealed class ThrowingDisposeConfigurationSource(List events) : IConfigurationSource + { + public IConfigurationProvider Build(IConfigurationBuilder builder) + { + return new ThrowingDisposeConfigurationProvider(events); + } + } + + private sealed class ThrowingDisposeConfigurationProvider(List events) : ConfigurationProvider, IDisposable + { + public void Dispose() + { + events.Add("configuration.dispose"); + throw new InvalidOperationException("configuration dispose"); + } + } + private sealed class FakeClient(long epoch) : ICheatEngineClient { public long Epoch => epoch;