Skip to content

Comments

fix: race condition and timer leak in IdeStreamingSink (#4857)#4917

Merged
thomhurst merged 1 commit intomainfrom
fix/ide-streaming-race
Feb 19, 2026
Merged

fix: race condition and timer leak in IdeStreamingSink (#4857)#4917
thomhurst merged 1 commit intomainfrom
fix/ide-streaming-race

Conversation

@thomhurst
Copy link
Owner

Summary

Closes #4857

  • Race condition fix: In OnTimerTick and SendOutputUpdateWithFollowUpHeartbeatAsync, the double-check pattern called TryMarkCompleted() without inspecting its return value. This allowed multiple timer threads to pass the completion checks concurrently and send updates after a test had already been marked complete. Now, IsCompleted is checked first as a fast path, and when Result becomes non-null, only the thread that atomically wins TryMarkCompleted() (returns true) performs cleanup, preventing duplicate cleanup and post-completion updates.
  • Timer leak fix: In CreateStreamingState, if the Timer constructor throws, the TestStreamingState object was never disposed. Wrapped timer creation in try-catch to dispose the state on failure before re-throwing.

Test plan

  • Verify dotnet build TUnit.Engine/TUnit.Engine.csproj succeeds with no warnings
  • Run IDE streaming tests in Rider/VS to confirm real-time output still works
  • Verify no post-completion InProgressTestNodeStateProperty messages are sent under concurrent load

Fix two concurrency issues in IdeStreamingSink:

1. Race condition: The double-check pattern in OnTimerTick and
   SendOutputUpdateWithFollowUpHeartbeatAsync called TryMarkCompleted()
   without checking its return value, allowing multiple threads to
   proceed past completion checks. Now only the thread that wins the
   atomic TryMarkCompleted gate performs cleanup.

2. Timer leak: If the Timer constructor in CreateStreamingState threw,
   the TestStreamingState was never disposed. Wrap timer creation in
   try-catch to dispose the state on failure.
@claude
Copy link
Contributor

claude bot commented Feb 19, 2026

Code Review

No issues found. Checked for bugs and CLAUDE.md compliance.

Summary

This PR correctly addresses two bugs in IdeStreamingSink.cs:

Race condition fix — The original code called TryMarkCompleted() without checking its return value, allowing multiple concurrent timer threads to each enter the cleanup path. The fix gates CleanupTest() behind the atomic TryMarkCompleted() return value in OnTimerTick, ensuring only the winning thread performs cleanup. The split of the combined if (state.IsCompleted || testContext.Result is not null) check in SendOutputUpdateWithFollowUpHeartbeatAsync into two separate checks is semantically correct — cleanup there is intentionally deferred to the next timer tick, which is a valid design given the timer is still running.

Timer leak fix — The try/catch around the Timer constructor with state.Dispose() in the catch block is a correct defensive pattern. Without it, a TestStreamingState could be allocated and never freed if Timer construction throws.

The atomic operations (Interlocked.Exchange in TryMarkCompleted(), Interlocked.CompareExchange in IsCompleted) are correctly implemented. All CLAUDE.md rules pass — no VSTest APIs, no new allocations in hot paths, no blocking async calls, no unannotated reflection.

@thomhurst thomhurst merged commit 506b1ca into main Feb 19, 2026
14 checks passed
@thomhurst thomhurst deleted the fix/ide-streaming-race branch February 19, 2026 06:56
This was referenced Feb 22, 2026
This was referenced Feb 23, 2026
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.

fix: race condition in IdeStreamingSink

1 participant