Skip to content

fix: Resource leak in the tests#2230

Merged
maximilien-noal merged 1 commit into
masterfrom
fix/build_leaks
Jun 18, 2026
Merged

fix: Resource leak in the tests#2230
maximilien-noal merged 1 commit into
masterfrom
fix/build_leaks

Conversation

@kevinferrare

@kevinferrare kevinferrare commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Fixes leaks in tests:

  • LoggerService was creating threads for async but never disposing them
  • MCP server was never disposing its HTTP server
  • GC was not agressive in tests, was leading to all ram being used
  • Assemblies loaded for testing generated code were never unloaded

Copilot AI review requested due to automatic review settings June 17, 2026 22:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces long-running test-suite memory growth by preventing generated-override assemblies and some runtime services from permanently accumulating in the default load context, and by disabling memory-hungry runtime behaviors in the test host.

Changes:

  • Load generated override assemblies into a collectible AssemblyLoadContext and dispose/unload it after each test.
  • Disable MCP HTTP hosting during tests (McpHttpPort = 0) and only start MCP HTTP when the port is non-zero.
  • Reduce per-test allocations by using ILoggerService substitutes in several tests and forcing workstation GC settings for the test project.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/Spice86.Tests/Video/VgaRenderer256ColorTestsBase.cs Replace LoggerService allocation with ILoggerService substitute in renderer test setup.
tests/Spice86.Tests/Video/RendererTestsBase.cs Replace LoggerService allocation with ILoggerService substitute in renderer test setup.
tests/Spice86.Tests/Spice86Creator.cs Disable MCP HTTP server in tests by setting McpHttpPort = 0.
tests/Spice86.Tests/Spice86.Tests.csproj Force workstation GC (and disable concurrent GC) to reduce peak RSS during the full test run.
tests/Spice86.Tests/GeneratedOverrideCompiler.cs Cache metadata references and load compiled overrides into a collectible load context.
tests/Spice86.Tests/GeneratedCodeMachineTestRunner.cs Ensure compiled override is disposed/unloaded via using.
tests/Spice86.Tests/GeneratedCodeMachineTest.cs Ensure compiled override is disposed/unloaded via using.
tests/Spice86.Tests/CompiledGeneratedOverride.cs Wrap compiled override supplier + load context and implement IDisposable to unload.
tests/Spice86.Tests/CollectibleAssemblyLoadContext.cs New collectible ALC type to host generated override assemblies.
tests/Spice86.Tests/CfgCpu/InstructionsFeederTest.cs Use ILoggerService substitute instead of substituting LoggerService.
tests/Spice86.Tests/CfgCpu/CfgGraphReloadTest.cs Silence logs in tests by setting AreLogsSilenced = true on LoggerService.
src/Spice86/Spice86DependencyInjection.cs Start MCP HTTP transport only when McpHttpPort != 0.
src/Spice86.Shared/Interfaces/ILoggerService.cs Remove UseStderrForConsoleOutput() from the shared logging interface.
src/Spice86.Logging/LoggerService.cs Rework logger building to avoid retained configuration state and remove stderr-redirection API.
src/Spice86.Core/Emulator/Devices/Video/Renderer.cs Accept ILoggerService (interface) instead of concrete LoggerService.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Spice86/Spice86DependencyInjection.cs Outdated
Comment thread tests/Spice86.Tests/GeneratedOverrideCompiler.cs Outdated
Comment thread tests/Spice86.Tests/Spice86.Tests.csproj Outdated
@maximilien-noal maximilien-noal merged commit 6fdee16 into master Jun 18, 2026
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants