fix(scripting): resolve Medium code-review finding (Core.Scripting-007)
In TimedScriptEvaluator.RunAsync, the catch (TimeoutException) block now checks ct.IsCancellationRequested before throwing ScriptTimeoutException, so a caller cancellation that races a timeout deterministically surfaces as OperationCanceledException regardless of which WaitAsync observes first. Regression test Caller_cancellation_wins_even_when_timeout_fires_first added. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -76,6 +76,14 @@ public sealed class TimedScriptEvaluator<TContext, TResult>
|
|||||||
// WaitAsync's synthesized timeout — the inner task may still be running
|
// WaitAsync's synthesized timeout — the inner task may still be running
|
||||||
// on its thread-pool thread (known leak documented in the class summary).
|
// on its thread-pool thread (known leak documented in the class summary).
|
||||||
// Wrap so callers can distinguish from user-written timeout logic.
|
// Wrap so callers can distinguish from user-written timeout logic.
|
||||||
|
//
|
||||||
|
// The class docs guarantee "caller-supplied cancel wins over timeout".
|
||||||
|
// When both fire at nearly the same time, WaitAsync observes them in
|
||||||
|
// non-deterministic order, so a cancel that arrives a few µs after the
|
||||||
|
// timeout still reaches here as TimeoutException. Re-check the token so
|
||||||
|
// the guarantee holds regardless of race ordering. (Core.Scripting-007.)
|
||||||
|
if (ct.IsCancellationRequested)
|
||||||
|
throw new OperationCanceledException(ct);
|
||||||
throw new ScriptTimeoutException(Timeout);
|
throw new ScriptTimeoutException(Timeout);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -132,4 +132,27 @@ public sealed class TimedScriptEvaluatorTests
|
|||||||
ex.Message.ShouldContain("ctx.Logger");
|
ex.Message.ShouldContain("ctx.Logger");
|
||||||
ex.Message.ShouldContain("widening the timeout");
|
ex.Message.ShouldContain("widening the timeout");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task Caller_cancellation_wins_even_when_timeout_fires_first()
|
||||||
|
{
|
||||||
|
// Regression for Core.Scripting-007: when the caller's CancellationToken is
|
||||||
|
// already signalled at the moment WaitAsync returns TimeoutException (a tight
|
||||||
|
// race), the implementation must re-check ct.IsCancellationRequested and throw
|
||||||
|
// OperationCanceledException rather than ScriptTimeoutException. Here we use a
|
||||||
|
// token that is cancelled before the evaluator starts — the tightest possible
|
||||||
|
// race — to verify the re-check path deterministically.
|
||||||
|
var inner = ScriptEvaluator<FakeScriptContext, int>.Compile("""return 1;""");
|
||||||
|
var timed = new TimedScriptEvaluator<FakeScriptContext, int>(
|
||||||
|
inner, TimeSpan.FromMilliseconds(1));
|
||||||
|
|
||||||
|
using var cts = new CancellationTokenSource();
|
||||||
|
cts.Cancel(); // already cancelled before RunAsync is even called
|
||||||
|
|
||||||
|
// WaitAsync sees an already-cancelled token and may surface as either
|
||||||
|
// TimeoutException or OperationCanceledException depending on ordering.
|
||||||
|
// Either way, the caller must see OperationCanceledException, not ScriptTimeoutException.
|
||||||
|
await Should.ThrowAsync<OperationCanceledException>(async () =>
|
||||||
|
await timed.RunAsync(new FakeScriptContext(), cts.Token));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user