Skip to content

Fix StreamingTextNode animation callback race with content mutations#225

Merged
Aaronontheweb merged 1 commit into
devfrom
fix/streaming-textnode-animation-race
May 24, 2026
Merged

Fix StreamingTextNode animation callback race with content mutations#225
Aaronontheweb merged 1 commit into
devfrom
fix/streaming-textnode-animation-race

Conversation

@Aaronontheweb

Copy link
Copy Markdown
Owner

Summary

Follow-up to #224 (which fixed a logic bug). The branch name fix/rebuildbuffer-race always implied a concurrency fix; #224 only fixed the post-Clear() newline logic. This PR fixes the actual race.

Animation subscription callbacks at AppendTracked and Replace previously invoked RebuildBuffer() from the segment's own scheduler (e.g. the R3 timer thread for SpinnerSegment) without acquiring _contentLock. That raced with Append/AppendTracked/Remove/Replace/Clear/Dispose, which all hold the lock while mutating _content and _buffer. Symptoms ranged from InvalidOperationException ("Collection was modified") on the foreach inside RebuildBuffer to torn buffer state, and ObjectDisposedException if Dispose() ran while a callback was queued behind the lock.

  • Funnel both callback sites through a single OnAnimationInvalidated() helper that takes _contentLock, checks a new volatile _disposed flag, then calls RebuildBuffer + OnNext under the lock.
  • Set _disposed = true first inside Dispose() so in-flight callbacks bail before touching the about-to-be-disposed _invalidated Subject.
  • Make Dispose() idempotent.

Performance

Adds one uncontended Monitor.Enter/Exit per animation frame (~10-20ns) and one volatile bool read. At typical spinner cadences (12.5/sec) this is well under one ppm of frame time. No allocations added. Render() and scroll-read paths are unchanged.

Tests

  • Converted AppendTracked_BlockWithAnimatedSegment_UpdatesInPlace to FakeTimeProvider (per CLAUDE.md convention), dropping real-timer + async + Thread.Sleep-style waits.
  • Added StreamingTextNodeThreadSafetyTests with:
    • Stress tests for concurrent invalidation against Append/Remove, Clear, and Replace
    • Post-Dispose() late-invalidation safety
    • Double-Dispose() idempotency

All 1124 project tests pass locally.

Known follow-ups

A self-review surfaced two test-quality issues worth a follow-up commit (not blockers):

  1. The Replace stress test orders Interlocked.Exchange after node.Replace, so the ticker mostly hits an already-disposed segment rather than racing a live one. Swap the order to actually exercise the live race.
  2. AnimationInvalidation_AfterDispose_DoesNotThrow passes for the wrong reason — Dispose() removes the subscription before the late fire, so OnAnimationInvalidated is never invoked and the _disposed guard isn't exercised. Restructure to capture and invoke the lambda directly.

Also flagged: public mutators (Append/Clear/Scroll*) still call NotifyChanged() without a _disposed check. Pre-existing latent race, narrow window, not introduced by this PR; worth a separate hardening pass.

Test plan

  • dotnet build succeeds with 0 warnings
  • dotnet test — full suite (1124 tests) passes
  • Streaming test subset (56 tests including 5 new thread-safety tests) passes
  • CI green on PR

Animation subscription callbacks at AppendTracked and Replace previously
invoked RebuildBuffer() from the segment's own scheduler (e.g. the R3 timer
thread for SpinnerSegment) without acquiring _contentLock. That raced with
Append/AppendTracked/Remove/Replace/Clear/Dispose, which all hold the lock
while mutating _content and _buffer. Symptoms ranged from InvalidOperationException
("Collection was modified") on the foreach inside RebuildBuffer to torn buffer
state, and ObjectDisposedException if Dispose() ran while a callback was queued
behind the lock.

Funnel both callback sites through a single OnAnimationInvalidated() helper
that takes _contentLock, checks a new volatile _disposed flag (set first inside
Dispose so in-flight callbacks bail before touching _invalidated), then calls
RebuildBuffer + OnNext under the lock.

Tests:
- Convert AppendTracked_BlockWithAnimatedSegment_UpdatesInPlace to use
  FakeTimeProvider (per CLAUDE.md), dropping the real-timer / async / Thread.Sleep
  style.
- Add StreamingTextNodeThreadSafetyTests covering concurrent invalidation with
  Append/Remove, Clear, and Replace, plus post-Dispose invalidation and
  double-Dispose idempotency.
@Aaronontheweb Aaronontheweb merged commit 8a7a220 into dev May 24, 2026
13 checks passed
@Aaronontheweb Aaronontheweb deleted the fix/streaming-textnode-animation-race branch May 24, 2026 19:09
Aaronontheweb added a commit that referenced this pull request May 24, 2026
Follow-up to #225. A self-review surfaced two test-quality issues in the
new race regression suite:

1. AnimationInvalidation_ConcurrentWithReplace_DoesNotThrow ordered
   Interlocked.Exchange AFTER node.Replace, so the ticker mostly read a
   reference to an already-disposed segment rather than racing a live one.
   The test mostly validated "TriggerInvalidation tolerates disposed
   segment" rather than the advertised live-vs-Replace race.

   Restructure: a permanently-tracked spinner is the ticker's target;
   Replace operates on a SEPARATE tracked element. The ticker is always
   firing on a live subscription, so OnAnimationInvalidated competes with
   Replace for _contentLock — which is the race the test claims to cover.

2. AnimationInvalidation_AfterDispose_DoesNotThrow passed for the wrong
   reason: node.Dispose() disposes the R3 subscription before the late
   TriggerInvalidation runs, so OnAnimationInvalidated is never invoked
   and the _disposed flag's belt-and-suspenders role is not exercised.

   Keep the original test (rebranded — it does validate subscription
   teardown, a legitimate concern) and add a new reflection-based test
   that invokes OnAnimationInvalidated directly after Dispose. Verified
   the new test FAILS with ObjectDisposedException from R3.Subject.OnNext
   when the `_disposed` guard is removed, and PASSES when the guard is
   present — so it actually catches the regression.
Aaronontheweb added a commit that referenced this pull request May 24, 2026
Final follow-up from the PR #225 self-review. Pre-existing latent race:
public mutators (Append, AppendLine, AppendTracked, Remove, Replace, Clear,
ScrollUp/Down/ToBottom) all end with NotifyChanged() → _invalidated.OnNext.
The mutator releases _contentLock before NotifyChanged, so Dispose() can run
in the gap, complete _invalidated.OnCompleted()/Dispose() (which are called
outside the lock), and the racing mutator's OnNext then throws
ObjectDisposedException from R3.Subject<T>.OnNext on a disposed Subject.
Same shape applies to a deliberate use-after-dispose call.

Fix:
- NotifyChanged() reads the volatile _disposed flag and bails before touching
  _invalidated. A try/catch (ObjectDisposedException) handles the narrow
  window where the flag reads false but Dispose() completes between the read
  and OnNext — harmless because a disposed node has no live observers.
- Route OnAnimationInvalidated's notification through NotifyChanged instead
  of inlining _invalidated.OnNext, so the animation path inherits the same
  hardening and the cross-cutting NotifyChanged point isn't bypassed (any
  future instrumentation hook there now covers animation frames too).

Tests:
- 12 parameterized cases covering every public mutator after Dispose().
  Verified the suite FAILS without the guard (11 of 12 cases throw
  ObjectDisposedException; the 2 outliers — Remove/Replace with a missing
  id — return false before reaching NotifyChanged) and PASSES with the
  guard restored.
- Append_ConcurrentWithDispose_DoesNotThrow: stress test where Append fires
  on a background thread while the main thread disposes the node mid-flight,
  bounded to 250ms wall clock.

1138/1138 tests pass.
@Aaronontheweb Aaronontheweb mentioned this pull request May 24, 2026
4 tasks
Aaronontheweb added a commit that referenced this pull request May 24, 2026
- Update VersionPrefix to 0.10.1
- Refresh PackageReleaseNotes with the StreamingTextNode thread-safety
  fixes (PRs #224, #225, #226, #227)
- Prepend 0.10.1 section to RELEASE_NOTES.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant