Race in JSRuntimeInvocationHandlerBase.OnTimeoutElapsed crashes the test host: "Nullable object must have a value"

[calebcwells]

Summary

JSRuntimeInvocationHandlerBase<TResult>.OnTimeoutElapsed has a time-of-check/time-of-use race on the currentInvocation field. When it loses the race, it throws InvalidOperationException: Nullable object must have a value on a thread-pool thread. Timer callbacks have no ambient exception handler, so the exception is unhandled and terminates the test host process, aborting every test still queued in that assembly.

Observed on a 2-core CI runner: 581 of 975 component tests ran, then the host died. An immediate re-run of the identical commit was green, so it presents as an unattributable flake.

Mechanism

src/bunit/JSInterop/InvocationHandlers/JSRuntimeInvocationHandlerBase{TResult}.cs (v2.10.3):

private Timer? timeoutTimer;                     // line 10
private JSRuntimeInvocation? currentInvocation;  // line 11

private void ClearTimeoutTimer()                 // lines 138-143
{
    timeoutTimer?.Dispose();
    timeoutTimer = null;
    currentInvocation = null;
}

private void OnTimeoutElapsed(object? state)     // lines 145-154
{
    if (!completionSource.Task.IsCompleted && currentInvocation.HasValue)
    {
        var exception = new JSRuntimeInvocationNotSetException(currentInvocation.Value);
        completionSource.TrySetException(exception);
    }

    ClearTimeoutTimer();
}

Neither field is volatile, and neither path takes a lock. OnTimeoutElapsed reads currentInvocation twice: once for HasValue (line 147) and again for .Value (line 149). ClearTimeoutTimer writes null to it from a different thread, reachable from SetResultBase (line 71), SetExceptionBase (line 57), and Dispose (line 125).

The timer is armed in exactly one place, HandleAsync (line 100), for an invocation matching a Setup/SetupVoid handler whose result has not been set yet. That makes disposal the routine loser: a test that arms the timer and finishes roughly DefaultWaitTimeout later tears its context down at the same instant the timer fires. On a loaded runner "roughly a second later" is common.

The window is small but it is a process kill, not a test failure, so a single hit costs the rest of the assembly.

Stack trace

From the failing run (GitHub Actions, ubuntu-latest, 2 cores, .NET 10, Release):

The active test run was aborted. Reason: Test host process crashed : Unhandled exception. System.InvalidOperationException: Nullable object must have a value.
   at System.Nullable`1.get_Value()
   at Bunit.JSRuntimeInvocationHandlerBase`1.OnTimeoutElapsed(Object state) in /_/src/bunit/JSInterop/InvocationHandlers/JSRuntimeInvocationHandlerBase{TResult}.cs:line 149
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.TimerQueueTimer.Fire(Boolean isThreadPool)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()
   at System.Threading.Thread.StartCallback()

Test Run Aborted.
Passed!  - Failed:     0, Passed:   581, Skipped:     0, Total:   581, Duration: 5 s

Versions affected

Reproduced against 2.10.3. The three methods above are byte-identical in 2.7.2, so a version bump is not a workaround. Line numbers cited are 2.10.3.

Proposed fix

Read the field into a local once, so the null check and the dereference see the same value:

private void OnTimeoutElapsed(object? state)
{
    var invocation = currentInvocation;
    if (!completionSource.Task.IsCompleted && invocation.HasValue)
    {
        completionSource.TrySetException(new JSRuntimeInvocationNotSetException(invocation.Value));
    }

    ClearTimeoutTimer();
}

That closes the observed crash. If the intent is that a completing handler reliably suppresses an in-flight timer callback, a lock (or Interlocked.Exchange) shared by StartTimeoutTimer, ClearTimeoutTimer and OnTimeoutElapsed would be the stronger form, since timeoutTimer is torn down unsynchronised on the same paths.

Either way, wrapping the callback body in a try/catch would be worth it independently: any throw from this callback is currently fatal to the host rather than to a test.

Workaround for users

Complete every planned invocation at setup, so the timer never arms:

JSInterop.SetupVoid("someInterop.doThing", _ => true).SetVoidResult();

Setting BunitContext.DefaultWaitTimeout to zero makes an uncompleted handler throw JSRuntimeInvocationNotSetException synchronously and name the identifier, which turns the audit into a single test run.

Possibly related

#1892 (BunitHtmlParser.Dispose() races Parse() on another thread) is a different class but the same shape: a teardown path racing a concurrent reader.