Reset per-run global state at the end of Execute (cross-invocation leaks, FT-1)#450
Merged
ChrisonSimtian merged 3 commits intoJul 7, 2026
Conversation
This was referenced Jun 30, 2026
ITaluone
approved these changes
Jun 30, 2026
4 tasks
ITaluone
force-pushed
the
engine/ft1-cross-invocation-leaks
branch
from
4591a6f to
dd3b12f
Compare
ChrisonSimtian
force-pushed
the
engine/ft1-cross-invocation-leaks
branch
from
dd3b12f to
3bc1de0
Compare
ChrisonSimtian
force-pushed
the
engine/ft1-cross-invocation-leaks
branch
from
3bc1de0 to
92a6fe4
Compare
Collaborator
Author
|
@dennisdoomen this one please, should be quick :-) |
dennisdoomen
approved these changes
Jul 5, 2026
dennisdoomen
left a comment
dennisdoomen
left a comment
Collaborator
There was a problem hiding this comment.
🤔 Can't we add some test to verify this behavior?
ChrisonSimtian
force-pushed
the
engine/ft1-cross-invocation-leaks
branch
from
7accd11 to
974c83a
Compare
…ion leaks) 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.
Drop the `s_` prefix on the private static field in BuildManager, per CSharpGuidelines AV1705 (no `_`/`m_`/`g_`/`s_` field prefixes) and AV1702 (camelCase private fields). Localized cleanup while we are in this file for FT-1; a repo-wide naming rule is handled separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds tests for the cross-invocation reset entry points BuildManager.Execute's finally now calls, so the leak-removal behaviour is pinned: - ToolPathResolverResetSpecs: NuGet/NpmToolPathResolver.Reset() null every package-location field. Shares the resolvers' static state with ToolTasksToolPathSpecs, so both sit in a non-parallelized collection. - InMemorySinkSpecs: InMemorySink.Clear() (and Dispose(), which delegates to it) drop accumulated events. - ValueInjectionUtilitySpecs: a value is cached on first read; ClearCache() forces re-injection — made observable with a counting injection attribute. The full reentrant Execute<T> run stays with the FT-9 harness. Tooling.Specs 66, Build.Specs 108, all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ITaluone
force-pushed
the
engine/ft1-cross-invocation-leaks
branch
from
974c83a to
e72d72b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First step of the engine de-statification ([Foundation] epic #315) — FT-1 / #306. Non-breaking groundwork before the
BuildContextfoundation (FT-2).BuildManager.Executeleaked 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.Createdhandlers were re-subscribed every call and never removed — accumulating;Finish(capturing a stalebuild);Logging.InMemorySink.Instancecarried log events over;ValueInjectionUtility's value cache and the NuGet/Npm tool-path resolver config persisted.Fix: the two global handlers are held in locals and the
finallyundoes exactly what the run set — unsubscribes both, dropsFinishfrom the cancellation list, and resets the carried-over state (InMemorySink.Clear,ValueInjectionUtility.ClearCache,NuGet/NpmToolPathResolver.Reset).new T()moved ahead of the subscriptions so a failed build creation leaks nothing.No public API change. The reentrancy guarantee itself is asserted by FT-9 (the isolation harness); this PR removes the leaks it will assert against. Full build + test suite green.
🤖 Generated with Claude Code