Fix StreamingTextNode animation callback race with content mutations#225
Merged
Merged
Conversation
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.
5 tasks
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.
4 tasks
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
added a commit
that referenced
this pull request
May 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #224 (which fixed a logic bug). The branch name
fix/rebuildbuffer-racealways implied a concurrency fix; #224 only fixed the post-Clear()newline logic. This PR fixes the actual race.Animation subscription callbacks at
AppendTrackedandReplacepreviously invokedRebuildBuffer()from the segment's own scheduler (e.g. the R3 timer thread forSpinnerSegment) without acquiring_contentLock. That raced withAppend/AppendTracked/Remove/Replace/Clear/Dispose, which all hold the lock while mutating_contentand_buffer. Symptoms ranged fromInvalidOperationException("Collection was modified") on theforeachinsideRebuildBufferto torn buffer state, andObjectDisposedExceptionifDispose()ran while a callback was queued behind the lock.OnAnimationInvalidated()helper that takes_contentLock, checks a new volatile_disposedflag, then callsRebuildBuffer+OnNextunder the lock._disposed = truefirst insideDispose()so in-flight callbacks bail before touching the about-to-be-disposed_invalidatedSubject.Dispose()idempotent.Performance
Adds one uncontended
Monitor.Enter/Exitper 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
AppendTracked_BlockWithAnimatedSegment_UpdatesInPlacetoFakeTimeProvider(per CLAUDE.md convention), dropping real-timer + async +Thread.Sleep-style waits.StreamingTextNodeThreadSafetyTestswith:Append/Remove,Clear, andReplaceDispose()late-invalidation safetyDispose()idempotencyAll 1124 project tests pass locally.
Known follow-ups
A self-review surfaced two test-quality issues worth a follow-up commit (not blockers):
Interlocked.Exchangeafternode.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.AnimationInvalidation_AfterDispose_DoesNotThrowpasses for the wrong reason —Dispose()removes the subscription before the late fire, soOnAnimationInvalidatedis never invoked and the_disposedguard isn't exercised. Restructure to capture and invoke the lambda directly.Also flagged: public mutators (
Append/Clear/Scroll*) still callNotifyChanged()without a_disposedcheck. Pre-existing latent race, narrow window, not introduced by this PR; worth a separate hardening pass.Test plan
dotnet buildsucceeds with 0 warningsdotnet test— full suite (1124 tests) passes