Skip to content

fix: Handle ObjectDisposedException when disposing linked CancellationTokenSource#1714

Merged
thomhurst merged 1 commit into
mainfrom
fix/print-progress-dispose-race
Jan 1, 2026
Merged

fix: Handle ObjectDisposedException when disposing linked CancellationTokenSource#1714
thomhurst merged 1 commit into
mainfrom
fix/print-progress-dispose-race

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • Fixes race condition where CancelAfter is called on an already-disposed linked CancellationTokenSource
  • The linked CTS can be disposed when the parent engine token is cancelled before DisposeAsync is called
  • Wraps the CancelAfter call in a try-catch to gracefully handle this case

Context

This bug was exposed by the logging improvements in PR #1711. The error manifests as:

System.ObjectDisposedException: The CancellationTokenSource has been disposed.
   at System.Threading.CancellationTokenSource.ThrowObjectDisposedException()
   at System.Threading.CancellationTokenSource.CancelAfter(Int32 millisecondsDelay)
   at ModularPipelines.Engine.Executors.PrintProgressExecutor.DisposeAsync() in PrintProgressExecutor.cs:line 49

Test plan

  • Build succeeds
  • CI pipeline passes
  • No ObjectDisposedException in logs

🤖 Generated with Claude Code

…nTokenSource

When the engine cancellation token is cancelled, the linked
CancellationTokenSource in PrintProgressExecutor may already be
disposed by the time DisposeAsync is called. This causes an
ObjectDisposedException when CancelAfter is called.

Wrap the CancelAfter call in a try-catch to gracefully handle
this race condition.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 1, 2026 16:18
@thomhurst
Copy link
Copy Markdown
Owner Author

Summary

Fixes a race condition where CancelAfter is called on an already-disposed linked CancellationTokenSource in the progress executor.

Critical Issues

None found ✅

Suggestions

The fix looks good and follows existing patterns in the codebase (see src/ModularPipelines/Context/Command.cs:282 for a similar pattern). The addition of the explanatory comment is actually an improvement over the existing pattern.

One minor observation: The comment could be slightly more precise. Consider:

// The linked CancellationTokenSource may already be disposed if the parent engine token was cancelled

vs. the current:

// Linked CancellationTokenSource may already be disposed if the engine token was cancelled

Both are fine - just a matter of consistency with the rest of the codebase.

Verdict

APPROVE - No critical issues

The fix correctly handles the race condition between the parent token cancellation (which disposes the linked token source) and the explicit disposal in DisposeAsync. This is a legitimate defensive coding pattern for async disposal scenarios.

Copy link
Copy Markdown

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

This PR fixes a race condition in PrintProgressExecutor where calling CancelAfter on an already-disposed linked CancellationTokenSource throws ObjectDisposedException. The race occurs when the parent engine token is cancelled before DisposeAsync is invoked, causing the linked CTS to be disposed.

Key changes:

  • Added try-catch block around CancelAfter call to handle ObjectDisposedException
  • Added explanatory comment describing when this race condition occurs

@thomhurst thomhurst merged commit b78a2ea into main Jan 1, 2026
17 of 18 checks passed
@thomhurst thomhurst deleted the fix/print-progress-dispose-race branch January 1, 2026 16:28
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