Reentrant Execute harness + the file-sink leak it found (FT-9) - #454
Draft
ChrisonSimtian wants to merge 6 commits into
Draft
Reentrant Execute harness + the file-sink leak it found (FT-9)#454ChrisonSimtian wants to merge 6 commits into
ChrisonSimtian wants to merge 6 commits into
Conversation
ChrisonSimtian
added
target/vNext
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 mutable fields (ArgumentsFromFilesService / ArgumentsFromCommitMessageService) no longer leak across runs. A lazy fallback covers access outside a build run (no cross-run state to leak there); it can retire once that path is confirmed dead. Non-breaking — the static API is unchanged.
Four cases on the FT-4 seam: the static facade resolves the running context's instance, it falls back to a stable ambient instance outside a run, each run gets its own service, and a mutated per-run field does not survive into the next run.
Logging.InMemorySink is now owned per-run by BuildContext.LogSink; the static InMemorySink.Instance becomes a facade over BuildContext.Current, with a lazy ambient fallback for logging outside a run. ConfigureInMemory wires the sink at the top of the run and WriteErrorsAndWarnings reads it at the end — both inside the scope — so a build's warnings and errors are discarded with its context instead of resurfacing in the next in-process invocation. The FT-1 InMemorySink.Clear() on dispose is dropped: there is no shared sink left to clear. Clear() itself stays for explicit resets. The sink constructor becomes internal so a context (and a spec) can create its own. Non-breaking — the public Logging surface is unchanged.
BuildContextSpecs mirrors the FT-4 parameter-service cases for the sink: the facade resolves the running context's sink, falls back to a stable ambient sink outside a run, and each run gets its own. Two existing cases assumed a process-wide sink and had to move with the mechanism: - Dispose_runs_the_per_run_teardown used the shared sink as its witness and fails once dispose stops clearing it — it becomes Log_events_do_not_carry_into_the_next_run, asserting the guarantee directly (the next run's sink never saw the previous run's events); - Disposing_a_superseded_context_leaves_the_shared_state_intact drops its sink witness and keeps the resolver-config one, since the sink is no longer shared state. InMemorySinkSpecs now exercises its own instance rather than the singleton, so it no longer needs the process-global collection or a normalising reset between cases.
…s over Host.WriteErrorsAndWarnings replaces Log.Logger with a console-only logger to re-render the run's warnings. The logger it replaced owns that run's file sinks, and assigning Log.Logger does not dispose it — so it leaked and kept `build.log` open for the life of the process. The next in-process run then threw in Logging.DeleteOldLogFiles trying to truncate that file, and did so silently: the throw lands before Configure has reassigned Log.Logger, so Execute's own Host.Error call went to an already-disposed logger. The run just returned the error exit code with no output. Any second run whose predecessor logged a warning hit this. Dispose the replaced logger. Nothing is lost — from that point the run logs through the console-only logger regardless, so the file sinks were already out of the picture.
Runs a fixture build end-to-end, twice in one process, and asserts what the de-static work promised: both runs succeed, the second gets its own context/parameter service/log sink, it opens on an empty sink, and the static facades inside a run point at that run. This is what the original FT-9 commit set out to do. Its unit-level assertions have since landed with the steps they belong to (FT-2 in Fallout-build#451, FT-4 in Fallout-build#452, FT-6 in Fallout-build#453), so what is left here is the end-to-end case they could not cover — and it immediately found the leaked file-sink logger fixed in the previous commit. Verified as a real guard: reverting that fix fails 4 of these 5 specs. Two process-wide inputs are pinned to make a build runnable under a test host — the argument parser (swapped and restored) and the statically-resolved root directory (asserted to carry the .fallout marker, since UpdateNotification prompts for input without it). Both are de-statification candidates in their own right.
ChrisonSimtian
force-pushed
the
engine/ft9-reentrancy-harness
branch
from
July 26, 2026 10:52
030c3fe to
7ac7242
Compare
ChrisonSimtian
added
target/vCurrent
ChrisonSimtian
changed the title
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
1 participant
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.
⏳ Stacked on #453 → #452 (FT-6 → FT-4) — the diff carries their commits until they merge. FT-1 and FT-2 have landed and are gone from it.
Engine de-statification ([Foundation] epic #315) — FT-9 / #314.
Rewritten. The original FT-9 commit added a
BuildContextSpecs.cswhose five assertions have all since landed with the steps they belong to — FT-2 (#451, merged), FT-4 (#452), FT-6 (#453). Replaying it onto today'smainwould have been a conflict resolving to nothing. What was never delivered is the part its own description deferred: an actual reentrantExecute<T>run. That is what this PR is now.BuildManagerReentrancySpecsruns a fixture build end-to-end — targets, logging pipeline, build extensions — twice in one process, asserting: both runs exit 0; the second gets its ownBuildContext/ParameterService/InMemorySink; it opens on an empty sink; and the static facades inside a run resolve to that run.It found a bug — fixed in the first commit
Host.WriteErrorsAndWarningsswapsLog.Loggerfor a console-only logger to re-render a run's warnings. The logger it replaces owns that run's file sinks, and assigningLog.Loggerdoes not dispose it — so it leaked and heldbuild.logopen for the life of the process.Any second in-process run whose predecessor logged a warning then threw in
Logging.DeleteOldLogFilestruncating that file, and returned the error exit code silently — the throw lands beforeConfigurereassignsLog.Logger, soExecute's ownHost.Errorwent to an already-disposed logger. Fix: dispose the replaced logger. Nothing is lost; from that point the run logs through the console-only logger regardless.Verified as a real guard: reverting the fix fails 4 of the 5 new specs.
Harness caveats
Two process-wide inputs have to be pinned for a build to run under a test host, and both are de-statification candidates in their own right:
EnvironmentInfo.ArgumentParser—Executeresolves invoked targets from the process command line, which under a test host is the runner's own argv. Swapped for a controlled parser, restored after.FalloutBuild.RootDirectory— resolved once in a static constructor, so it cannot be varied per run. It lands on this repo's root; the harness asserts the.falloutmarker is there, becauseUpdateNotificationAttributeprompts for a key press on a local build without one (guard, not hang).Out of scope: concurrent runs. The harness is sequential, and the process-wide singletons FT-2 called out (value-injection cache, tool-path resolver config) are still shared.
Full build + full spec suite green (14 projects, 0 failures;
Build.Specs164 → 169).🤖 Generated with Claude Code