Skip to content

Conversation

@thomhurst
Copy link
Owner

Summary

  • Convert TestStateManager methods from Task-returning to void since they are purely synchronous in-memory operations
  • Make InProgress message fire-and-forget in TestCoordinator since it's informational and doesn't need to block test execution

Changes

Old Method New Method
MarkRunningAsync MarkRunning
MarkCompletedAsync MarkCompleted
MarkFailedAsync MarkFailed
MarkSkippedAsync MarkSkipped
MarkCircularDependencyFailedAsync MarkCircularDependencyFailed
MarkDependencyResolutionFailedAsync MarkDependencyResolutionFailed

Benefits

  • Eliminates async state machine overhead at call sites
  • Zero allocation for synchronous operations (previously used Task.CompletedTask)
  • Cleaner API that accurately represents the synchronous nature of these operations

Test plan

  • All 1013 profile tests pass
  • No performance regression (~251-263ms same as baseline)
  • Build succeeds for all target frameworks

🤖 Generated with Claude Code

- Convert TestStateManager methods from Task-returning to void since they
  are purely synchronous in-memory operations:
  - MarkRunningAsync → MarkRunning
  - MarkCompletedAsync → MarkCompleted
  - MarkFailedAsync → MarkFailed
  - MarkSkippedAsync → MarkSkipped
  - MarkCircularDependencyFailedAsync → MarkCircularDependencyFailed
  - MarkDependencyResolutionFailedAsync → MarkDependencyResolutionFailed

- Make InProgress message fire-and-forget in TestCoordinator since it's
  informational and doesn't need to block test execution

Benefits:
- Eliminates async state machine overhead at call sites
- Zero allocation for synchronous operations (no Task.CompletedTask)
- Cleaner API that accurately represents synchronous nature of operations

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@thomhurst
Copy link
Owner Author

Summary

Converts synchronous TestStateManager methods from Task-returning to void to eliminate async overhead.

Critical Issues

Fire-and-forget pattern introduces potential issues (TestCoordinator.cs:65)

The change to fire-and-forget for InProgress messages introduces several problems:

  1. Silent exception swallowing: If InProgress throws, the exception will be unobserved
  2. Race conditions: InProgress message may not complete before subsequent state changes, causing out-of-order test state reporting
  3. Inconsistent pattern: All other message bus calls are awaited

Recommendation: Keep the await for InProgress. The performance gain is negligible compared to the reliability risk.

Suggestions

The TestStateManager void conversion itself is good - these are genuinely synchronous operations.

Verdict

REQUEST CHANGES - Fire-and-forget pattern needs to be addressed

@thomhurst thomhurst merged commit 377459c into main Jan 12, 2026
12 of 13 checks passed
@thomhurst thomhurst deleted the perf/sync-state-manager-methods branch January 12, 2026 00:54
This was referenced Jan 12, 2026
This was referenced Jan 13, 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.

2 participants