Skip to content

fix: dispose IProcessInvoker in Docker/Container/UnixUtil call sites#4378

Open
herbenderbler wants to merge 1 commit intoactions:mainfrom
herbenderbler:fix/f1-undisposed-process-invoker
Open

fix: dispose IProcessInvoker in Docker/Container/UnixUtil call sites#4378
herbenderbler wants to merge 1 commit intoactions:mainfrom
herbenderbler:fix/f1-undisposed-process-invoker

Conversation

@herbenderbler
Copy link
Copy Markdown

@herbenderbler herbenderbler commented Apr 24, 2026

Summary

IProcessInvoker instances created via HostContext.CreateService<IProcessInvoker>() in a few Docker/container/unix paths were not disposed.
Because CreateService returns a new instance per call, those paths could retain process-related resources longer than needed on long-lived runners.

What Changed

  • Wrapped per-call IProcessInvoker usage in using scopes in:
    • src/Runner.Worker/Container/DockerCommandManager.cs
    • src/Runner.Worker/ContainerOperationProvider.cs
    • src/Runner.Common/Util/UnixUtil.cs
  • Kept existing output/error handler behavior and call semantics intact.
  • Added focused L0 coverage in:
    • src/Test/L0/ProcessInvokerDisposalL0.cs
    • src/Test/L0/Util/UnixUtilL0.cs

Why This Is Safer

  • Ensures invoker disposal is deterministic per invocation.
  • Reduces risk of resource retention in hot Docker/container code paths.
  • Preserves existing runtime behavior (command execution, output capture, cancellation flow).

Test Plan

  • L0 tests validate disposal on success and exception paths.
  • git diff --check and IDE lints are clean.

Copilot AI review requested due to automatic review settings April 24, 2026 23:23
@herbenderbler herbenderbler requested a review from a team as a code owner April 24, 2026 23:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a managed-memory/resource leak in runner container/unix execution paths by ensuring IProcessInvoker instances created via HostContext.CreateService<IProcessInvoker>() are properly disposed after use.

Changes:

  • Wrap IProcessInvoker creation in using blocks across Docker/container execution and UnixUtil.ExecAsync.
  • Add L0 tests to verify CreateService<IProcessInvoker>() results are disposed (including exception paths).
  • Add PR-audit documentation and adjust .gitignore.

Reviewed changes

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

Show a summary per file
File Description
src/Runner.Worker/Container/DockerCommandManager.cs Dispose per-call IProcessInvoker instances in docker command execution paths.
src/Runner.Worker/ContainerOperationProvider.cs Dispose per-call IProcessInvoker in ExecuteCommandAsync output-capture path.
src/Runner.Common/Util/UnixUtil.cs Dispose per-call IProcessInvoker in ExecAsync, keeping handler unsubscribe safety.
src/Test/L0/ProcessInvokerDisposalL0.cs Adds disposal-pattern tests for CreateService + using and exception paths.
src/Test/L0/Util/UnixUtilL0.cs Adds UnixUtil-focused disposal verification tests on non-Windows.
PR_DESCRIPTION_F1.md Adds a PR/audit write-up describing the leak and fix.
.gitignore Adjusts ignore patterns (currently impacts Rider .DotSettings.user ignores).

Comment thread PR_DESCRIPTION_F1.md Outdated
Comment thread src/Runner.Worker/Container/DockerCommandManager.cs Outdated
Comment thread src/Test/L0/ProcessInvokerDisposalL0.cs Outdated
@herbenderbler herbenderbler force-pushed the fix/f1-undisposed-process-invoker branch 5 times, most recently from e55defd to 9490e0a Compare April 25, 2026 01:46
@herbenderbler herbenderbler force-pushed the fix/f1-undisposed-process-invoker branch from 9490e0a to 5831d64 Compare April 25, 2026 10:12
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.

2 participants