Skip to content

fix: async void -> async Task and fix TotalMilliseconds in Sleep log#149

Closed
manan164 wants to merge 1 commit into
conductor-oss:mainfrom
manan164:fix/async-void-and-timespan
Closed

fix: async void -> async Task and fix TotalMilliseconds in Sleep log#149
manan164 wants to merge 1 commit into
conductor-oss:mainfrom
manan164:fix/async-void-and-timespan

Conversation

@manan164
Copy link
Copy Markdown
Collaborator

@manan164 manan164 commented May 5, 2026

Problems Fixed

Bug 2: async void fire-and-forget in WorkflowTaskExecutor

WorkOnce, ProcessTasks, and ProcessTask were all declared private async void. This means:

  • Exceptions thrown inside these methods are never caught by the Work4Ever try/catch loop.
  • The caller cannot await them, so task processing silently swallows errors.

Fix: Changed all three to private async Task. Work4Ever now calls WorkOnce(token).GetAwaiter().GetResult() to preserve the synchronous outer loop while surfacing exceptions correctly.

Bug 3: TimeSpan.Milliseconds vs TimeSpan.TotalMilliseconds in Sleep log

The Sleep() helper logged timeSpan.Milliseconds, which is the sub-second milliseconds component (range 0-999). Any sleep of 1 second or more would log an incorrect/truncated value (e.g., a 1.5s sleep logs 500ms, a 2s sleep logs 0ms).

Fix: Changed to timeSpan.TotalMilliseconds which returns the full duration as a double.

Changes

Conductor/Client/Worker/WorkflowTaskExecutor.cs

  • WorkOnce: async void -> async Task
  • ProcessTasks: async void -> async Task
  • ProcessTask: async void -> async Task
  • Work4Ever: awaits WorkOnce via .GetAwaiter().GetResult()
  • Sleep(): log uses timeSpan.TotalMilliseconds instead of timeSpan.Milliseconds

Bug 2: WorkOnce, ProcessTasks, and ProcessTask were declared as
`private async void`, which swallows exceptions — they are fire-and-forget
and callers cannot await them. Changed all three to `private async Task`
so exceptions propagate correctly. Work4Ever now calls
WorkOnce(...).GetAwaiter().GetResult() to stay synchronous at the
top-level loop.

Bug 3: The Sleep() log message used timeSpan.Milliseconds (the
subsecond component, range 0-999) instead of timeSpan.TotalMilliseconds
(the full duration as a double). Any sleep >= 1 second would log 0ms
or an incorrect truncated value. Fixed to use TotalMilliseconds.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
Conductor/Client/Worker/WorkflowTaskExecutor.cs 0.00% 2 Missing ⚠️
Flag Coverage Δ
unittests 1.05% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
Conductor/Client/Worker/WorkflowTaskExecutor.cs 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@manan164
Copy link
Copy Markdown
Collaborator Author

manan164 commented May 6, 2026

Closing — Agentspan uses TaskResourceApi directly rather than WorkflowTaskHost/WorkflowTaskExecutor, so the async void and TotalMilliseconds issues don't affect our usage.

@manan164 manan164 closed this May 6, 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.

1 participant