From e0401b2d840e938ea6eafc982ae8aa3bce372ed3 Mon Sep 17 00:00:00 2001 From: Chrison Simtian Date: Tue, 30 Jun 2026 21:53:06 +1200 Subject: [PATCH 1/3] Reset per-run global state at the end of Execute (FT-1, cross-invocation leaks) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BuildManager.Execute leaked process-global state across invocations, so a second build in the same process (tests, hosted/reentrant scenarios) inherited the first's residue: - Console.CancelKeyPress / ToolOptions.Created handlers were re-subscribed every call and never removed (accumulating); - the CancellationHandler list kept each run's Finish (capturing a stale build); - Logging.InMemorySink.Instance carried log events over; - ValueInjectionUtility's value cache and the NuGet/Npm tool-path resolver config persisted. Fix (non-breaking): the two global handlers are now held in locals and the finally undoes exactly what the run set — unsubscribes both, removes Finish from the cancellation list, and resets the carried-over state (InMemorySink.Clear, ValueInjectionUtility.ClearCache, NuGet/NpmToolPathResolver.Reset). Build creation moved ahead of the subscriptions so a failed `new T()` leaks nothing. Groundwork for the BuildContext foundation (FT-2). Full build + test suite green; reentrancy is verified by FT-9. --- src/Fallout.Build/Execution/BuildManager.cs | 21 +++++++++++++++++-- .../Execution/ValueInjectionUtility.cs | 3 +++ src/Fallout.Build/Logging.cs | 8 ++++++- src/Fallout.Tooling/NpmToolPathResolver.cs | 6 ++++++ src/Fallout.Tooling/NuGetToolPathResolver.cs | 9 ++++++++ 5 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/Fallout.Build/Execution/BuildManager.cs b/src/Fallout.Build/Execution/BuildManager.cs index 5925fceca..3fd62a459 100644 --- a/src/Fallout.Build/Execution/BuildManager.cs +++ b/src/Fallout.Build/Execution/BuildManager.cs @@ -8,6 +8,7 @@ using Fallout.Common.Tooling; using Fallout.Common.Utilities; using Fallout.Common.Utilities.Collections; +using Fallout.Common.ValueInjection; using Serilog; #pragma warning disable CA2255 @@ -38,11 +39,17 @@ public static int Execute(Expression>[] defaultTargetExpressi { Console.OutputEncoding = Encoding.UTF8; Console.InputEncoding = Encoding.UTF8; - Console.CancelKeyPress += (_, _) => s_cancellationHandlers.ForEach(x => x()); - ToolOptions.Created += (options, _) => VerbosityMapping.Apply((ToolOptions)options); var build = new T(); + // Hold the per-run global subscriptions in locals so the finally can undo exactly them — + // otherwise each Execute in the same process (tests, hosted scenarios) accumulates handlers. + // FT-1 / #306. + ConsoleCancelEventHandler onCancelKeyPress = (_, _) => s_cancellationHandlers.ForEach(x => x()); + EventHandler onToolOptionsCreated = (options, _) => VerbosityMapping.Apply((ToolOptions)options); + Console.CancelKeyPress += onCancelKeyPress; + ToolOptions.Created += onToolOptionsCreated; + try { Logging.Configure(build); @@ -88,6 +95,16 @@ public static int Execute(Expression>[] defaultTargetExpressi { Finish(); Log.CloseAndFlush(); + + // FT-1 (#306): undo this run's process-global state so a subsequent Execute in the same + // process starts clean — no accumulated handlers, no carried-over log events / caches / config. + CancellationHandler -= Finish; + Console.CancelKeyPress -= onCancelKeyPress; + ToolOptions.Created -= onToolOptionsCreated; + Logging.InMemorySink.Instance.Clear(); + ValueInjectionUtility.ClearCache(); + NuGetToolPathResolver.Reset(); + NpmToolPathResolver.Reset(); } void Finish() diff --git a/src/Fallout.Build/Execution/ValueInjectionUtility.cs b/src/Fallout.Build/Execution/ValueInjectionUtility.cs index 8d637a3e9..06e424523 100644 --- a/src/Fallout.Build/Execution/ValueInjectionUtility.cs +++ b/src/Fallout.Build/Execution/ValueInjectionUtility.cs @@ -12,6 +12,9 @@ internal static class ValueInjectionUtility { private static readonly Dictionary s_valueCache = new(); + /// Clears the per-run injected-value cache so a subsequent build in the same process re-injects. FT-1 / #306. + internal static void ClearCache() => s_valueCache.Clear(); + public static T TryGetValue(Expression> parameterExpression) where T : class { diff --git a/src/Fallout.Build/Logging.cs b/src/Fallout.Build/Logging.cs index 9d99f5052..139572a69 100644 --- a/src/Fallout.Build/Logging.cs +++ b/src/Fallout.Build/Logging.cs @@ -228,10 +228,16 @@ public void Emit(LogEvent logEvent) _logEvents.Add(logEvent); } - public void Dispose() + /// Drops accumulated events so a subsequent build in the same process starts clean. FT-1 / #306. + public void Clear() { _logEvents.Clear(); } + + public void Dispose() + { + Clear(); + } } internal class ExecutingTargetLogEventEnricher : ILogEventEnricher diff --git a/src/Fallout.Tooling/NpmToolPathResolver.cs b/src/Fallout.Tooling/NpmToolPathResolver.cs index d94f91d66..1ec405e29 100644 --- a/src/Fallout.Tooling/NpmToolPathResolver.cs +++ b/src/Fallout.Tooling/NpmToolPathResolver.cs @@ -8,6 +8,12 @@ public static class NpmToolPathResolver { public static AbsolutePath NpmPackageJsonFile; + /// Resets the per-run package.json location so a subsequent build in the same process starts from defaults. FT-1 / #306. + public static void Reset() + { + NpmPackageJsonFile = null; + } + public static string GetNpmExecutable(string npmExecutable) { Assert.FileExists(NpmPackageJsonFile); diff --git a/src/Fallout.Tooling/NuGetToolPathResolver.cs b/src/Fallout.Tooling/NuGetToolPathResolver.cs index 78e335480..6df96e92a 100644 --- a/src/Fallout.Tooling/NuGetToolPathResolver.cs +++ b/src/Fallout.Tooling/NuGetToolPathResolver.cs @@ -16,6 +16,15 @@ public static class NuGetToolPathResolver public static string NuGetAssetsConfigFile; public static string PaketPackagesConfigFile; + /// Resets the per-run package-location config so a subsequent build in the same process starts from defaults. FT-1 / #306. + public static void Reset() + { + EmbeddedPackagesDirectory = null; + NuGetPackagesConfigFile = null; + NuGetAssetsConfigFile = null; + PaketPackagesConfigFile = null; + } + public static string GetPackageExecutable(string packageId, string packageExecutable, string version = null, string framework = null) { Assert.True(packageId != null && packageExecutable != null); From 9884d445dba0da254af277214c60b1d6cd4db54c Mon Sep 17 00:00:00 2001 From: Chrison Simtian Date: Tue, 30 Jun 2026 22:01:23 +1200 Subject: [PATCH 2/3] Introduce the per-run BuildContext foundation (FT-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds BuildContext — an internal, per-run, AsyncLocal-ambient scope activated at the top of BuildManager.Execute (`using var context = BuildContext.Activate()`) and disposed at method exit. It owns the per-run global-state lifecycle: the Console.CancelKeyPress / ToolOptions.Created subscriptions, the cancellation-handler list, and the teardown FT-1 centralised (now run on dispose). BuildManager.CancellationHandler becomes a thin facade over BuildContext.Current — the first "static surface over the context" seam, and the rail subsequent steps ride (FT-4 ParameterService, FT-5 tool-path config, FT-6 logging scope move their state onto the context). Internal-only; not a public contract until the SDK (milestone #7). Disposing on method exit means a failed `new T()` also leaks nothing. Full build + test suite green. --- src/Fallout.Build/Execution/BuildContext.cs | 61 +++++++++++++++++++++ src/Fallout.Build/Execution/BuildManager.cs | 32 +++-------- 2 files changed, 70 insertions(+), 23 deletions(-) create mode 100644 src/Fallout.Build/Execution/BuildContext.cs diff --git a/src/Fallout.Build/Execution/BuildContext.cs b/src/Fallout.Build/Execution/BuildContext.cs new file mode 100644 index 000000000..cd76edf78 --- /dev/null +++ b/src/Fallout.Build/Execution/BuildContext.cs @@ -0,0 +1,61 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using Fallout.Common.Tooling; +using Fallout.Common.Utilities.Collections; +using Fallout.Common.ValueInjection; + +namespace Fallout.Common.Execution; + +/// +/// Per-run, process-ambient build state. Activated once at the top of +/// and disposed at the end of the run, so the process-global statics a build touches are owned by a +/// scope rather than leaking across invocations (the cleanup FT-1 centralised now lives here). +/// The static surface (e.g. ) reads through +/// ; AsyncLocal keeps concurrent/nested runs isolated. +/// +/// +/// FT-2 / #307. Intentionally +/// internal — not a public contract until the SDK lands (milestone #7). Subsequent steps move +/// the per-run services (parameters, logging scope, tool-path config) onto this context. +/// +internal sealed class BuildContext : IDisposable +{ + private static readonly AsyncLocal s_current = new(); + + /// The context for the current build run, or null outside a run. + public static BuildContext Current => s_current.Value; + + private readonly LinkedList _cancellationHandlers = new(); + private readonly ConsoleCancelEventHandler _onCancelKeyPress; + private readonly EventHandler _onToolOptionsCreated; + + private BuildContext() + { + _onCancelKeyPress = (_, _) => _cancellationHandlers.ForEach(x => x()); + _onToolOptionsCreated = (options, _) => VerbosityMapping.Apply((ToolOptions)options); + Console.CancelKeyPress += _onCancelKeyPress; + ToolOptions.Created += _onToolOptionsCreated; + } + + /// Creates the ambient context for a build run and installs it as . + public static BuildContext Activate() => s_current.Value = new BuildContext(); + + public void RegisterCancellationHandler(Action handler) => _cancellationHandlers.AddFirst(handler); + + public void UnregisterCancellationHandler(Action handler) => _cancellationHandlers.Remove(handler); + + public void Dispose() + { + // Undo this run's process-global state (the FT-1 cleanup, now owned by the scope). + Console.CancelKeyPress -= _onCancelKeyPress; + ToolOptions.Created -= _onToolOptionsCreated; + Logging.InMemorySink.Instance.Clear(); + ValueInjectionUtility.ClearCache(); + NuGetToolPathResolver.Reset(); + NpmToolPathResolver.Reset(); + + if (ReferenceEquals(s_current.Value, this)) + s_current.Value = null; + } +} diff --git a/src/Fallout.Build/Execution/BuildManager.cs b/src/Fallout.Build/Execution/BuildManager.cs index 3fd62a459..33fe49dc3 100644 --- a/src/Fallout.Build/Execution/BuildManager.cs +++ b/src/Fallout.Build/Execution/BuildManager.cs @@ -8,7 +8,6 @@ using Fallout.Common.Tooling; using Fallout.Common.Utilities; using Fallout.Common.Utilities.Collections; -using Fallout.Common.ValueInjection; using Serilog; #pragma warning disable CA2255 @@ -18,12 +17,12 @@ internal static class BuildManager { private const int ErrorExitCode = -1; - private static readonly LinkedList s_cancellationHandlers = new(); - + // Facade over the active BuildContext (FT-2): callers register/unregister during a run; the + // context owns the handler list and discards it on dispose. public static event Action CancellationHandler { - add => s_cancellationHandlers.AddFirst(value); - remove => s_cancellationHandlers.Remove(value); + add => BuildContext.Current?.RegisterCancellationHandler(value); + remove => BuildContext.Current?.UnregisterCancellationHandler(value); } [ModuleInitializer] @@ -40,16 +39,11 @@ public static int Execute(Expression>[] defaultTargetExpressi Console.OutputEncoding = Encoding.UTF8; Console.InputEncoding = Encoding.UTF8; + // The per-run scope owns the global subscriptions + teardown (FT-2 / #307). Disposed at + // method exit, so re-invocation in the same process starts clean even if `new T()` throws. + using var context = BuildContext.Activate(); var build = new T(); - // Hold the per-run global subscriptions in locals so the finally can undo exactly them — - // otherwise each Execute in the same process (tests, hosted scenarios) accumulates handlers. - // FT-1 / #306. - ConsoleCancelEventHandler onCancelKeyPress = (_, _) => s_cancellationHandlers.ForEach(x => x()); - EventHandler onToolOptionsCreated = (options, _) => VerbosityMapping.Apply((ToolOptions)options); - Console.CancelKeyPress += onCancelKeyPress; - ToolOptions.Created += onToolOptionsCreated; - try { Logging.Configure(build); @@ -95,16 +89,8 @@ public static int Execute(Expression>[] defaultTargetExpressi { Finish(); Log.CloseAndFlush(); - - // FT-1 (#306): undo this run's process-global state so a subsequent Execute in the same - // process starts clean — no accumulated handlers, no carried-over log events / caches / config. - CancellationHandler -= Finish; - Console.CancelKeyPress -= onCancelKeyPress; - ToolOptions.Created -= onToolOptionsCreated; - Logging.InMemorySink.Instance.Clear(); - ValueInjectionUtility.ClearCache(); - NuGetToolPathResolver.Reset(); - NpmToolPathResolver.Reset(); + // Per-run teardown (handler unsubscription + state reset) is owned by the BuildContext, + // run when `context` is disposed at method exit. } void Finish() From 119fee548fc69436b14ae225a8f60034b3804e8f Mon Sep 17 00:00:00 2001 From: Chrison Simtian Date: Tue, 30 Jun 2026 22:06:35 +1200 Subject: [PATCH 3/3] Move ParameterService onto the BuildContext (FT-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The active ParameterService is now the per-run instance held on BuildContext.Parameters; the static ParameterService.Instance becomes a facade over BuildContext.Current. This is the first service to ride the FT-2 rail: production and tests now exercise the same instance form (killing the test/prod divergence — tests already `new ParameterService(funcs)`), and the per-run instance + its mutated fields (ArgumentsFromFilesService / ArgumentsFromCommitMessageService) no longer leak across runs. A lazy fallback (s_ambient) covers any access outside a build run (no cross-run concern there); it can retire once that path is confirmed dead. Non-breaking — the static API is unchanged. Full build + test suite green. --- src/Fallout.Build/Execution/BuildContext.cs | 5 +++++ .../Execution/ParameterService.Statics.cs | 11 ++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/Fallout.Build/Execution/BuildContext.cs b/src/Fallout.Build/Execution/BuildContext.cs index cd76edf78..4699b9249 100644 --- a/src/Fallout.Build/Execution/BuildContext.cs +++ b/src/Fallout.Build/Execution/BuildContext.cs @@ -30,6 +30,11 @@ internal sealed class BuildContext : IDisposable private readonly ConsoleCancelEventHandler _onCancelKeyPress; private readonly EventHandler _onToolOptionsCreated; + /// The per-run parameter service (FT-4 / #309) — replaces the process-global + /// ParameterService.Instance, so prod and tests exercise the same instance form. + public ParameterService Parameters { get; } = + new(() => EnvironmentInfo.ArgumentParser, () => EnvironmentInfo.Variables); + private BuildContext() { _onCancelKeyPress = (_, _) => _cancellationHandlers.ForEach(x => x()); diff --git a/src/Fallout.Build/Execution/ParameterService.Statics.cs b/src/Fallout.Build/Execution/ParameterService.Statics.cs index e8e026b13..0eec065bd 100644 --- a/src/Fallout.Build/Execution/ParameterService.Statics.cs +++ b/src/Fallout.Build/Execution/ParameterService.Statics.cs @@ -2,15 +2,20 @@ using System.Linq; using System.Linq.Expressions; using System.Reflection; +using Fallout.Common.Execution; using Fallout.Common.Utilities; namespace Fallout.Common; internal partial class ParameterService { - internal static ParameterService Instance = new( - () => EnvironmentInfo.ArgumentParser, - () => EnvironmentInfo.Variables); + // FT-4 (#309): the active instance is the per-run one held on BuildContext, so prod uses the + // same instance form as tests and nothing leaks across runs. The lazy fallback covers the rare + // access outside a build run (no cross-run concern there); it can retire once that's confirmed dead. + private static readonly Lazy s_ambient = new( + () => new ParameterService(() => EnvironmentInfo.ArgumentParser, () => EnvironmentInfo.Variables)); + + internal static ParameterService Instance => BuildContext.Current?.Parameters ?? s_ambient.Value; public static T GetParameter(string name, char? separator = null) {