From 7541a38762c66beaa2792802281e22110114974c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 08:29:09 -0400 Subject: [PATCH] fix(site-runtime): reuse exited detached worker slots in script scheduler (review 2026-10-21 01/SR-M1) --- .../Scripts/ScriptExecutionScheduler.cs | 62 +++++++++++++++---- .../Scripts/ScriptExecutionSchedulerTests.cs | 40 ++++++++++++ 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptExecutionScheduler.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptExecutionScheduler.cs index 4b2ed75e..fcfaab1e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptExecutionScheduler.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptExecutionScheduler.cs @@ -307,17 +307,43 @@ public sealed class ScriptExecutionScheduler : TaskScheduler, IDisposable } } - /// Appends new worker slots + threads. Caller holds . + /// + /// Adds workers. Slots vacated by an exited detached worker are + /// reused first (review 2026-10-21 01/SR-M1: otherwise a script that wedges on every run + /// grows and starts a fresh slot per detach for the process lifetime); + /// only the remainder appends new slots. Caller holds . + /// private void GrowTo(int count) { var existing = _slots; - var grown = new WorkerSlot[existing.Length + count]; - Array.Copy(existing, grown, existing.Length); + var reuse = new List(); + for (var i = 0; i < existing.Length && reuse.Count < count; i++) + if (Volatile.Read(ref existing[i].Free) != 0) reuse.Add(i); - for (var i = 0; i < count; i++) + var appended = count - reuse.Count; + var grown = existing; + if (appended > 0) { - var index = existing.Length + i; - grown[index] = new WorkerSlot(); + grown = new WorkerSlot[existing.Length + appended]; + Array.Copy(existing, grown, existing.Length); + for (var i = 0; i < appended; i++) + grown[existing.Length + i] = new WorkerSlot(); + } + + var indices = new List(count); + indices.AddRange(reuse); + for (var i = 0; i < appended; i++) + indices.Add(existing.Length + i); + + // Reset reused slots BEFORE the thread starts; the exited worker never touches its slot + // again after marking it Free. + foreach (var index in reuse) + { + var slot = grown[index]; + Volatile.Write(ref slot.Detached, 0); + Volatile.Write(ref slot.RunStamp, 0); + Volatile.Write(ref slot.BusySinceTicks, 0); + Volatile.Write(ref slot.Free, 0); } // Publish the array BEFORE starting the threads: a worker's very first action is to @@ -325,19 +351,22 @@ public sealed class ScriptExecutionScheduler : TaskScheduler, IDisposable _slots = grown; _configuredCount += count; - for (var i = 0; i < count; i++) + foreach (var index in indices) { - var index = existing.Length + i; - var thread = new Thread(() => WorkerLoop(index)) + var i = index; + var thread = new Thread(() => WorkerLoop(i)) { IsBackground = true, - Name = ThreadNamePrefix + index + Name = ThreadNamePrefix + i }; - grown[index].Thread = thread; + grown[i].Thread = thread; thread.Start(); } } + /// Total worker slots (live + detached-but-unexited + free). Diagnostic/test seam. + internal int SlotCount => _slots.Length; + [ThreadStatic] private static ScriptExecutionScheduler? CurrentScheduler; @@ -368,6 +397,14 @@ public sealed class ScriptExecutionScheduler : TaskScheduler, IDisposable if (Volatile.Read(ref slot.Detached) != 0) { Interlocked.Decrement(ref _detachedLive); + // Hand the slot back for reuse by the next GrowTo. Under the lock so a + // concurrent GrowTo sees either "not free" or a fully vacated slot; the + // thread touches no slot state after this. + lock (_growLock) + { + slot.Thread = null; + Volatile.Write(ref slot.Free, 1); + } return; } } @@ -435,6 +472,9 @@ public sealed class ScriptExecutionScheduler : TaskScheduler, IDisposable /// Non-zero once the worker has been detached; it exits after its current task. public int Detached; + /// Non-zero once a detached worker has exited and the slot may be reused by . + public int Free; + /// The worker thread, for to join. public Thread? Thread; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptExecutionSchedulerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptExecutionSchedulerTests.cs index d9265248..b6266d6a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptExecutionSchedulerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptExecutionSchedulerTests.cs @@ -118,4 +118,44 @@ public class ScriptExecutionSchedulerTests await Task.Delay(50); Assert.True(condition(), "condition not met within timeout"); } + + /// + /// Repeatedly wedging + detaching a worker must reuse the vacated slot rather than grow the + /// slot table without bound (review 2026-10-21 01/SR-M1). + /// + [Fact] + public async Task DetachedWorkerSlot_IsReused_SlotTableStaysBounded() + { + using var scheduler = new ScriptExecutionScheduler(1); + + for (var round = 0; round < 6; round++) + { + var gate = new ManualResetEventSlim(false); + var started = new TaskCompletionSource<(int Slot, long Stamp)>(); + var wedged = Task.Factory.StartNew(() => + { + var slot = ScriptExecutionScheduler.CurrentWorkerSlot!.Value; + started.SetResult((slot, scheduler.CurrentRunStamp(slot))); + gate.Wait(); + }, CancellationToken.None, TaskCreationOptions.None, scheduler); + + var (s, stamp) = await started.Task; + Assert.Equal(WorkerDetachOutcome.Detached, scheduler.TryDetachWorker(s, stamp)); + gate.Set(); + await wedged; + + // Wait for the detached worker to exit and hand its slot back. + var deadline = DateTime.UtcNow.AddSeconds(5); + while (scheduler.DetachedThreadCount != 0 && DateTime.UtcNow < deadline) + await Task.Delay(10); + Assert.Equal(0, scheduler.DetachedThreadCount); + await Task.Delay(50); + } + + // 1 live + at most 1 free/in-flight detached slot, never 1 + 6. + Assert.True(scheduler.SlotCount <= 2, $"slot table grew to {scheduler.SlotCount}"); + // The pool still works afterwards. + var ran = await Task.Factory.StartNew(() => 42, CancellationToken.None, TaskCreationOptions.None, scheduler); + Assert.Equal(42, ran); + } }