refactor(siteruntime): ScriptExecutionScheduler is an un-substitutable process-wide static with no disposal guard #18

New Issue

2026-07-17T02:58:47-04:00

dohertj2 commented

2026-07-17 02:58:47 -04:00

Summary

ScriptExecutionScheduler is a process-wide static singleton with no reset, no disposal guard, and no injection seam. It works in the Host (one scheduler per process is the intent) but it is un-substitutable and un-resettable, which makes it a shared mutable global for anything that runs more than one logical site in a process — most visibly the test assembly, where it is the mechanism behind #16.

Split out of #16, which is about the specific flake. This issue is about the seam itself.

Detail

src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptExecutionScheduler.cs:31-53:

private static volatile ScriptExecutionScheduler? _shared;
private static readonly object SharedLock = new();

public static ScriptExecutionScheduler Shared(SiteRuntimeOptions options)
{
    if (_shared != null) return _shared;
    lock (SharedLock) { return _shared ??= new ScriptExecutionScheduler(options.ScriptExecutionThreadCount); }
}

Consumers call the static directly — ScriptExecutionActor.cs:115, AlarmExecutionActor.cs:92, AlarmActor.cs:557 — so there is no way to hand them a different instance.

Three properties follow:

1. First caller wins, so the pool size is set by whoever gets there first. ScriptExecutionThreadCount (default 8, SiteRuntimeOptions.cs:46) is read only on the first call; every later caller silently gets the first caller's sizing regardless of its own options. In a process hosting one site that is fine and intended. In the test assembly the winner depends on test execution order, so the pool size is not deterministic across runs.

2. The class is IDisposable but the static is never cleared. Nothing disposes it today (verified). If anything ever did — a test teardown, a future multi-site host, a shutdown path — _shared would keep handing out the disposed instance for the remainder of the process, and every subsequent script and alarm execution would fail with ObjectDisposedException. There is no guard: no disposed check in Shared(), no reset. The failure would be total and confusing, and the codebase already knows this shape (ScriptActor.ExpressionEvalFailed explicitly cites "ObjectDisposedException from a disposed ScriptExecutionScheduler" — see AlarmActor.cs:822).

3. No seam for tests. A test cannot give an actor its own scheduler, so every script-executing test class in SiteRuntime.Tests shares one fixed pool of dedicated threads while xUnit runs those classes in parallel. #16 is the consequence: one test parks workers on an unbounded semaphore wait and starves unrelated tests (DeploymentManagerActorTests.RouteInboundApiCall_* time out waiting for a routed script), with one run failing 22 tests.

Suggested fix

Give the scheduler an injection seam and let Shared() stay as the Host's default rather than the only option:

Take the scheduler from DI / actor Props instead of calling the static inside the actors. The Host registers the process-wide instance exactly as today; tests pass their own. This removes the shared-pool coupling for every test, not just #16's.
Failing that, at minimum guard the static: track disposal and either throw a clear error or recreate on next Shared(), so a disposed singleton cannot silently poison the rest of the process.

Option 1 also makes the "first caller wins" sizing explicit rather than incidental.

Notes

No product bug is known to result from this today — the Host creates one scheduler and never disposes it. This is about removing a latent global and unblocking #16's structural fix.
#16 can be fixed on its own (bound the test's semaphore wait) without this; this is the deeper remedy.

References

ScriptExecutionScheduler.cs:31-53 (the static), consumers at ScriptExecutionActor.cs:115, AlarmExecutionActor.cs:92, AlarmActor.cs:557.
#16 — the flake this enables.

## Summary `ScriptExecutionScheduler` is a process-wide static singleton with no reset, no disposal guard, and no injection seam. It works in the Host (one scheduler per process is the intent) but it is un-substitutable and un-resettable, which makes it a shared mutable global for anything that runs more than one logical site in a process — most visibly the test assembly, where it is the mechanism behind #16. Split out of #16, which is about the specific flake. This issue is about the seam itself. ## Detail `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptExecutionScheduler.cs:31-53`: ```csharp private static volatile ScriptExecutionScheduler? _shared; private static readonly object SharedLock = new(); public static ScriptExecutionScheduler Shared(SiteRuntimeOptions options) { if (_shared != null) return _shared; lock (SharedLock) { return _shared ??= new ScriptExecutionScheduler(options.ScriptExecutionThreadCount); } } ``` Consumers call the static directly — `ScriptExecutionActor.cs:115`, `AlarmExecutionActor.cs:92`, `AlarmActor.cs:557` — so there is no way to hand them a different instance. Three properties follow: **1. First caller wins, so the pool size is set by whoever gets there first.** `ScriptExecutionThreadCount` (default 8, `SiteRuntimeOptions.cs:46`) is read only on the first call; every later caller silently gets the first caller's sizing regardless of its own options. In a process hosting one site that is fine and intended. In the test assembly the winner depends on test execution order, so the pool size is not deterministic across runs. **2. The class is `IDisposable` but the static is never cleared.** Nothing disposes it today (verified). If anything ever did — a test teardown, a future multi-site host, a shutdown path — `_shared` would keep handing out the **disposed** instance for the remainder of the process, and every subsequent script and alarm execution would fail with `ObjectDisposedException`. There is no guard: no disposed check in `Shared()`, no reset. The failure would be total and confusing, and the codebase already knows this shape (`ScriptActor.ExpressionEvalFailed` explicitly cites "ObjectDisposedException from a disposed ScriptExecutionScheduler" — see `AlarmActor.cs:822`). **3. No seam for tests.** A test cannot give an actor its own scheduler, so every script-executing test class in `SiteRuntime.Tests` shares one fixed pool of dedicated threads while xUnit runs those classes in parallel. #16 is the consequence: one test parks workers on an unbounded semaphore wait and starves unrelated tests (`DeploymentManagerActorTests.RouteInboundApiCall_*` time out waiting for a routed script), with one run failing 22 tests. ## Suggested fix Give the scheduler an injection seam and let `Shared()` stay as the Host's default rather than the only option: 1. Take the scheduler from DI / actor `Props` instead of calling the static inside the actors. The Host registers the process-wide instance exactly as today; tests pass their own. This removes the shared-pool coupling for every test, not just #16's. 2. Failing that, at minimum guard the static: track disposal and either throw a clear error or recreate on next `Shared()`, so a disposed singleton cannot silently poison the rest of the process. Option 1 also makes the "first caller wins" sizing explicit rather than incidental. ## Notes - No product bug is known to result from this today — the Host creates one scheduler and never disposes it. This is about removing a latent global and unblocking #16's structural fix. - #16 can be fixed on its own (bound the test's semaphore wait) without this; this is the deeper remedy. ## References - `ScriptExecutionScheduler.cs:31-53` (the static), consumers at `ScriptExecutionActor.cs:115`, `AlarmExecutionActor.cs:92`, `AlarmActor.cs:557`. - #16 — the flake this enables.

dohertj2 referenced this issue from a commit

2026-07-17 16:07:16 -04:00

fix(siteruntime): injectable ScriptExecutionScheduler + un-leakable expression-fault test

dohertj2 referenced this issue

2026-07-17 16:07:26 -04:00

fix(siteruntime): injectable ScriptExecutionScheduler + un-leakable expression-fault test (#16, #18) #21

dohertj2 closed this issue

2026-07-17 16:07:41 -04:00

dohertj2 referenced this issue from a commit

2026-08-01 20:16:02 -04:00

docs(register): track 4 untracked deferrals, split the failover/perf row, close #18

Sign in to join this conversation.