Skip to content

fix: make PagingLogger IDisposable to prevent fd leak on step fault#4379

Open
herbenderbler wants to merge 1 commit intoactions:mainfrom
herbenderbler:fix/f4-paging-logger-dispose
Open

fix: make PagingLogger IDisposable to prevent fd leak on step fault#4379
herbenderbler wants to merge 1 commit intoactions:mainfrom
herbenderbler:fix/f4-paging-logger-dispose

Conversation

@herbenderbler
Copy link
Copy Markdown

@herbenderbler herbenderbler commented Apr 24, 2026

Summary

PagingLogger can lose partial step logs and hold file handles open when a step faults before ExecutionContext.Complete() reaches _logger.End().

This PR makes logger lifetime explicit: owned PagingLogger instances are disposed when an ExecutionContext completes, while shared embedded loggers are not double-disposed.

What Changed

  • Keep PagingLogger disposable behavior and focused disposal tests in src/Test/L0/PagingLoggerL0.cs.
  • Add ownership tracking in src/Runner.Worker/ExecutionContext.cs:
    • mark loggers created via HostContext.CreateService<IPagingLogger>() as owned
    • dispose owned loggers in Complete() via finally
    • avoid disposing shared embedded loggers
  • Add ExecutionContext L0 tests in src/Test/L0/Worker/ExecutionContextL0.cs to verify:
    • owned job logger is disposed
    • owned child logger is disposed
    • shared embedded logger is not disposed
  • Trim low-signal/diagnostic-only F4 tests to keep the suite focused and maintainable.

Why This Is Safer

  • Prevents leaked file handles from owned loggers on non-happy paths.
  • Reduces risk of missing partial logs by ensuring disposal runs deterministically when contexts complete.
  • Preserves existing shared-logger behavior for embedded execution contexts.

Test Plan

  • Existing L0 PagingLogger disposal behavior tests pass in unit test environment.
  • New L0 ExecutionContext ownership tests cover the production ownership path.
  • git diff --check and IDE lints are clean.

Copilot AI review requested due to automatic review settings April 24, 2026 23:24
@herbenderbler herbenderbler requested a review from a team as a code owner April 24, 2026 23:24
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

Updates PagingLogger to be disposable so file handles are deterministically released (and partial logs are still queued) when a step faults before End() is reached, preventing fd leaks and silent log loss.

Changes:

  • Extend IPagingLogger with IDisposable and implement PagingLogger.Dispose() (best-effort, idempotent, non-throwing).
  • Add L0 tests covering the new disposal contract and expected behavior on exception-like paths.
  • Add a PR write-up document; update .gitignore.

Reviewed changes

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

File Description
src/Runner.Common/Logging.cs Makes IPagingLogger disposable and adds PagingLogger.Dispose() to flush/close and queue partial uploads safely.
src/Test/L0/PagingLoggerL0.cs Adds new L0 tests validating IDisposable contract and disposal behavior (partial write, idempotence, using-pattern, etc.).
PR_DESCRIPTION_F4.md Documents the bug, repro, and rationale for the IDisposable fix and tests.
.gitignore Modifies ignore rule related to DotSettings / leakbench artifacts.

Comment thread src/Runner.Common/Logging.cs Outdated
Comment thread src/Test/L0/PagingLoggerL0.cs Outdated
Comment thread src/Test/L0/PagingLoggerL0.cs
@herbenderbler herbenderbler force-pushed the fix/f4-paging-logger-dispose branch 7 times, most recently from 6ca0ef8 to c2304a0 Compare April 25, 2026 09:32
@herbenderbler herbenderbler force-pushed the fix/f4-paging-logger-dispose branch from c2304a0 to 2d27adb Compare April 25, 2026 10:06
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