TimeOut and Retry - #28
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #28 +/- ##
==========================================
+ Coverage 78.42% 79.90% +1.48%
==========================================
Files 2 2
Lines 190 209 +19
Branches 30 34 +4
==========================================
+ Hits 149 167 +18
Misses 21 21
- Partials 20 21 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR extends MinimalWorker with per-execution timeouts and automatic retry behavior for background workers, and updates the source generator to emit shared infrastructure once for improved incrementality.
Changes:
- Added
WithTimeout(...)andWithRetry(...)to the fluent worker builder and persisted settings onWorkerRegistration. - Implemented timeout + retry execution logic in generated worker initializers (continuous/periodic/cron).
- Split generator output into shared infrastructure (
RegisterPostInitializationOutput) vs worker-specific initializer code.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
src/MinimalWorker/BackgroundWorkerExtensions.cs |
Adds builder APIs and stores timeout/retry configuration on registrations. |
src/MinimalWorker.Generators/WorkerGenerator.cs |
Emits shared generated code once via post-init output. |
src/MinimalWorker.Generators/WorkerEmitter.cs |
Generates retry/timeout-aware execution loops and splits shared vs worker-specific emission. |
| private static void EmitParameterResolutionWithExecutionToken(StringBuilder sb, WorkerInvocationModel worker, string indent, string scopeVar) | ||
| { | ||
| foreach (var param in worker.Parameters) | ||
| { | ||
| if (param.IsCancellationToken) | ||
| { | ||
| sb.AppendLine($"{indent}var {param.Name} = executionToken;"); | ||
| } | ||
| else | ||
| { | ||
| sb.AppendLine($"{indent}var {param.Name} = {scopeVar}.ServiceProvider.GetRequiredService<{param.Type}>();"); | ||
| } | ||
| } |
There was a problem hiding this comment.
EmitParameterResolutionWithExecutionToken uses the delegate parameter name directly for the generated local (var {param.Name} = ...). This can generate non-compiling code when the delegate uses common names that already exist in the generated method scope (e.g., token), because C# forbids re-declaring a local with the same name in an inner scope. Consider generating unique local names (e.g., prefix with __mw_) and using those names when emitting the delegate invocation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/MinimalWorker.Generators/WorkerEmitter.cs:818
- Same issue as above:
catch (OperationCanceledException) when (timeout.HasValue)will translate anyOperationCanceledExceptioninto a timeout whenWithTimeout()is set. That can misclassify cancellations unrelated to timeout and suppress retries. Consider verifying the cancellation came from the per-execution timeout token before converting it toTimeoutException.
sb.AppendLine(" var tcs = new TaskCompletionSource<bool>();");
sb.AppendLine(" using var timer = timeProvider.CreateTimer(_ => tcs.TrySetResult(true), null, delay, Timeout.InfiniteTimeSpan);");
sb.AppendLine(" using var reg = token.Register(() => tcs.TrySetCanceled());");
sb.AppendLine(" try { await tcs.Task; } catch (OperationCanceledException) when (token.IsCancellationRequested) { break; }");
sb.AppendLine(" }");
| await host.StartAsync(); | ||
| // Advance time for retries (2 retries * 10s delay = 20s minimum) | ||
| await WorkerTestHelper.AdvanceTimeAsync(timeProvider, TimeSpan.FromMinutes(1), steps: 12); |
There was a problem hiding this comment.
Same issue here: the unreachable return after throw forces CS0162 suppression. Consider removing the unreachable return Task.CompletedTask; and the associated pragmas.
| { | ||
| services.AddSingleton<TimeProvider>(timeProvider); | ||
| }) | ||
| .Build(); | ||
|
|
||
| host.RunBackgroundWorker((CancellationToken token) => | ||
| { | ||
| Interlocked.Increment(ref attemptCount); | ||
| throw new InvalidOperationException("Always fails"); | ||
| #pragma warning disable CS0162 // Unreachable code detected | ||
| return Task.CompletedTask; | ||
| #pragma warning restore CS0162 | ||
| }) | ||
| .WithRetry(maxAttempts: 2, delay: TimeSpan.FromSeconds(10)) | ||
| .WithErrorHandler(ex => | ||
| { | ||
| errorHandlerCalled = true; | ||
| }); | ||
|
|
||
| // Act | ||
| await host.StartAsync(); | ||
| // Advance time for retries | ||
| await WorkerTestHelper.AdvanceTimeAsync(timeProvider, TimeSpan.FromMinutes(1), steps: 12); | ||
| await host.StopAsync(); | ||
|
|
||
| // Assert | ||
| Assert.True(attemptCount >= 2, $"Expected at least 2 attempts, got {attemptCount}"); | ||
| Assert.True(errorHandlerCalled, "Error handler should be called after all retries exhausted"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task PeriodicWorker_WithTimeoutAndRetry_Should_Not_Retry_Timeouts() | ||
| { |
There was a problem hiding this comment.
PeriodicWorker_WithTimeoutAndRetry_Should_Not_Retry_Timeouts doesn’t currently verify the “not retried” part. With a 10ms schedule the periodic worker can start multiple separate executions during the 200ms window; each will time out once, so attemptCount may be > 1 even if timeouts aren’t retried within a single execution. Consider constraining the test to a single scheduled execution (e.g., use a long interval or a fake TimeProvider and only advance enough for one tick), and then assert the delegate was invoked exactly once while maxAttempts > 1.
| /// <item><description>The error handler is invoked (if configured)</description></item> | ||
| /// <item><description>Without an error handler, the application terminates (fail-fast)</description></item> | ||
| /// <item><description><b>Periodic/Cron workers:</b> Will run again on the next scheduled interval</description></item> | ||
| /// </list> | ||
| /// <para> | ||
| /// <b>Note:</b> Retries do not occur for <see cref="OperationCanceledException"/> (graceful shutdown) or <see cref="TimeoutException"/> (if timeout is configured). |
There was a problem hiding this comment.
The WithRetry XML docs list “Periodic/Cron workers: Will run again on the next scheduled interval” under “After all retry attempts are exhausted”, but the preceding bullet says that without an error handler the application terminates (so it won’t run again). Consider clarifying that periodic/cron workers only continue to the next interval when an error handler is configured (or when fatal termination is otherwise disabled).
| /// <item><description>The error handler is invoked (if configured)</description></item> | |
| /// <item><description>Without an error handler, the application terminates (fail-fast)</description></item> | |
| /// <item><description><b>Periodic/Cron workers:</b> Will run again on the next scheduled interval</description></item> | |
| /// </list> | |
| /// <para> | |
| /// <b>Note:</b> Retries do not occur for <see cref="OperationCanceledException"/> (graceful shutdown) or <see cref="TimeoutException"/> (if timeout is configured). | |
| /// <item><description>If an error handler is configured, it is invoked and <b>Periodic/Cron workers</b> will run again on the next scheduled interval</description></item> | |
| /// <item><description>Without an error handler, the application terminates (fail-fast), so no further executions (including periodic/cron runs) occur</description></item> | |
| /// </list> | |
| /// <para> | |
| /// <b>Note:</b> Retries do not occur for <see cref="OperationCanceledException"/> (graceful shutdown) or <see cref="TimeoutException"/> (if timeout is configured). | |
| /// <b>Note:</b> Retries do not occur for <see cref="OperationCanceledException"/> (graceful shutdown) or <see cref="TimeoutException"/> (if timeout is configured). |
| { | ||
| services.AddSingleton<TimeProvider>(timeProvider); | ||
| }) |
There was a problem hiding this comment.
This test suppresses CS0162 due to an unreachable return Task.CompletedTask; after a throw. The pragma noise can be avoided by removing the unreachable return (a lambda that always throws is valid for a Func<..., Task> return type).
| var attemptCount = 0; | ||
| var errorHandlerCalled = false; | ||
| var timeProvider = WorkerTestHelper.CreateTimeProvider(); |
There was a problem hiding this comment.
Same issue here: the #pragma warning disable CS0162 is only needed because of an unreachable return after throw. Removing the unreachable return Task.CompletedTask; will eliminate the need for pragmas and keep the test easier to read.
No description provided.