fix(workflows): stop instance designer refresh timers when the circuit disconnects

[sfmskywalker]

Purpose

Stop Elsa Studio from crashing when a Blazor Server circuit disconnects while a workflow instance is open, for example when the tab showing the instance is closed.

Scope

• Bug fix (behavior change)

Description

Problem

WorkflowInstanceDesigner drives a periodic refresh timer and a one-second elapsed-time timer. After POST /_blazor/disconnect the circuit's scoped services and JS interop are disposed, but the timers kept firing on thread-pool threads. The refresh path then threw ObjectDisposedException or Microsoft.JSInterop.JSDisconnectedException from RefreshActivityStatePeriodically, and because the timer callback is async void the exception escaped and took the host down.

Solution

• The component tracks disposal. DisposeAsync marks it disposed first, then stops and disposes both timers and the observer, so no timer body runs after disposal, including the documented case where a System.Threading.Timer callback fires once more after Dispose.
• Both timer ticks run through one shared guard: return immediately when disposed; on ObjectDisposedException, JSDisconnectedException, or OperationCanceledException treat the circuit as gone, stop that timer, and return; let every other exception propagate exactly as before, so real failures are not hidden.
• Stopping a timer detaches it atomically from its field before disposing it, and the refresh rearm tolerates a timer that disposal just disposed, so a tick racing DisposeAsync cannot throw out of the callback.
• Timer creation and observer creation both re-check disposal after their asynchronous or racy step: a timer published after disposal is detached and disposed at once, and an observer whose factory completed after teardown started is disposed instead of being subscribed, so a lifecycle continuation cannot leave a live timer or observer rooting the disposed component.
• Live-circuit behavior is unchanged: the refresh cadence, StateHasChanged, the details-tab refresh, and observer subscription are as they were.
• The tick bodies are exposed as internal seams (the assembly already grants the test project internals access), plus an internal virtual state-notification seam so a test can make the render notification throw. No public surface changed.

This builds on the approach in the draft PR #770, which caught only ObjectDisposedException on one path.

Tests

WorkflowInstanceDesignerDisconnectRefreshTests (bunit, 18 tests) renders a lightweight subclass of the component and drives the ticks directly: after disposal neither tick does anything (the activity execution service is not called and no state notification is issued); when the service or the render notification throws any of the three circuit-gone exceptions, nothing propagates and the corresponding timer is stopped so later ticks make no further calls; a live tick still notifies once; a rearm against a timer disposed concurrently does not throw; two genuinely overlapping disposals (one blocked on an in-flight callback) leave both timer fields null without throwing; timer creators are no-ops after disposal and install exactly one timer on a live component; disposal stops the refresh timer even when observer disposal throws; and an observer created by a factory that completes after disposal is disposed and never published, while a normal creation still publishes and subscribes. The disposed and stop-on-disconnect assertions were checked to fail with the guards removed.

Verification

• dotnet build Elsa.Studio.sln: success.
• dotnet test src/modules/Elsa.Studio.Workflows.Tests/...: 204 passed.

Fixes #743

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Concurrent timer disposal and rearming can still produce an unhandled exception from the async-void refresh callback.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Prevents Blazor Server disconnects from leaving workflow-instance timers running against disposed circuit services.

Changes:

• Adds disposal-aware guards for refresh and elapsed timers.
• Handles circuit-disconnection exceptions and stops affected timers.
• Adds nine bUnit tests covering disposal and disconnect behavior.

File summaries

File	Description
WorkflowInstanceDesigner.razor.cs	Adds guarded timer ticks and disposal handling.
WorkflowInstanceDesignerDisconnectRefreshTests.cs	Tests timer behavior during disposal and disconnects.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The disposal flag lacks cross-thread visibility, and the concurrency test does not currently create overlapping disposal calls.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

A lifecycle continuation can still create a periodic elapsed timer after component disposal, leaking the component.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Timer exceptions can be hidden, and refresh cleanup can be skipped when observer disposal fails.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

An in-flight observer creation can publish a live observer after component disposal completes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Both timers can fire before publication, bypassing their new stop and rearm guarantees.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/modules/Elsa.Studio.Workflows/Components/WorkflowInstanceViewer/Components/WorkflowInstanceDesigner.razor.cs:472

• The one-shot refresh timer is also armed before publication. If the creating thread is delayed long enough for the tick to run first, the tick cannot find this timer to stop or rearm; afterward PublishRefreshTimer stores an already-fired timer, so refresh silently stops and the timer remains retained until teardown. Publish a disabled timer before arming it.

        // Ownership of the timer created here transfers to _refreshTimer via PublishRefreshTimer; it is
        // disposed by the stop path (StopRefreshActivityStatePeriodically) or by DisposeAsync.
        PublishRefreshTimer(new Timer(Callback, null, TimeSpan.FromSeconds(1), Timeout.InfiniteTimeSpan));

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Timer self-disposal can deadlock, and observer publication still races with teardown.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/modules/Elsa.Studio.Workflows/Components/WorkflowInstanceViewer/Components/WorkflowInstanceDesigner.razor.cs:247

• The disposal check and observer subscription/publication are not atomic with DisposeAsync. Disposal can set _disposed, exchange a null observer, and complete; this continuation can then subscribe the newly created observer before the final check removes it. During that window a SignalR update can invoke component/JS work on the disconnected circuit. Coordinate the disposed transition and observer publication with the same synchronization primitive so no observer can be subscribed after teardown has won the race.

        observer.ActivityExecutionLogUpdated += OnActivityExecutionLogUpdated;

        var previousObserver = Interlocked.Exchange(ref _workflowInstanceObserver, observer);

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Blanket handling of cancellation exceptions can permanently stop refreshes on healthy circuits after transient API cancellation.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/modules/Elsa.Studio.Workflows/Components/WorkflowInstanceViewer/Components/WorkflowInstanceDesigner.razor.cs:602

• Treating every OperationCanceledException as proof that the circuit is gone also catches cancellations from the refresh's backend calls. RefreshSelectedItemAsync calls RemoteActivityExecutionService, which forwards the default token to HTTP API calls; a request timeout can therefore surface as TaskCanceledException while the circuit is still healthy. This branch then silently removes the refresh timer permanently, contrary to the stated unchanged live-circuit behavior and the intent to propagate real failures. Please suppress cancellation only when it is tied to component/circuit teardown (or narrow this handling to the renderer invocation), rather than classifying all operation cancellation as a disconnect.
  src/modules/Elsa.Studio.Workflows.Tests/WorkflowInstanceDesignerDisconnectRefreshTests.cs:228
• This explanation contradicts the asserted behavior: with the previous check/await/clear implementation, the first disposal remains suspended before clearing _refreshTimer, so the Assert.Null immediately below would fail rather than “still hold.” Please state that the field would still be non-null so the regression mechanism is documented accurately.

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced