fix(tests): make FakeChatClient thread-safe to fix racy compaction test - #1579
Merged
Aaronontheweb merged 3 commits intoJul 5, 2026
Merged
Conversation
CompactionIntegrationTests.Successive_compactions_preserve_prior_summary_in_observer_system_prompt
failed on ubuntu CI (run 28690821775) with:
System.InvalidOperationException : Collection was modified;
enumeration operation may not execute.
at CompactionIntegrationTests.cs:617
FakeChatClient is a single shared instance whose GetResponseAsync is
invoked concurrently from multiple actor dispatcher / ThreadPool threads
(main-model turn + compaction summarizer sidecar + fire-and-forget
memory-extraction sidecar launched one line after CompactionOutput is
emitted). It appended to three plain List<T> fields — ReceivedMessages /
ReceivedOptions / ReceivedToolNames — with no synchronization, while the
test thread enumerated ReceivedMessages via .Select().Where().ToList().
Only _callCount was Interlocked-guarded — a partial fix that left the
lists exposed. The test's ExpectMsg<CompactionOutput> barrier does not
quiesce the collection, so the enumeration raced a concurrent Add.
Fix: back the three recording collections with private lists and expose
them as IReadOnlyList<T> getters that return a snapshot copy taken under
a single _recordingGate lock; guard the appends (and the PlannedExceptions
check) under the same lock. Snapshot-on-read is load-bearing — locking the
writes alone would not stop a caller's List<T> enumeration from throwing
when an Add interleaves its MoveNext. The lock is never held across an
await. The racing writer (memory-extraction, "memory extraction assistant"
prompt) is filtered out of the assertion, and the summarizer calls the
test inspects are ordered-before their CompactionOutput, so a thread-safe
fake alone makes the test deterministic without an added actor barrier.
Verified on Linux: build clean (0 warnings), target test 25/25,
full Netclaw.Actors.Tests suite 2531 passed / 0 failed, slopwatch clean.
Aaronontheweb
enabled auto-merge (squash)
July 5, 2026 14:54
Aaronontheweb
added a commit
that referenced
this pull request
Jul 5, 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.
Problem
CompactionIntegrationTests.Successive_compactions_preserve_prior_summary_in_observer_system_promptfailed on ubuntu CI only (run 28690821775) — Windows and macOS passed, the classic racy-test signature:Root cause
FakeChatClientis a single shared instance whoseGetResponseAsyncis invoked concurrently from multiple actor dispatcher / ThreadPool threads — the main-model turn, the compaction summarizer sidecar, and a fire-and-forget memory-extraction sidecar thatLlmSessionActorlaunches one line after it emitsCompactionOutput(LlmSessionActor.cs:1273). It appended to three plainList<T>fields (ReceivedMessages/ReceivedOptions/ReceivedToolNames) with no synchronization, while the test thread enumeratedReceivedMessagesvia.Select().Where().ToList()at line 617. Only_callCountwasInterlocked-guarded — a partial fix that left the lists exposed.The test's
ExpectMsg<CompactionOutput>barrier proves the summarizer call landed but does not quiesce the collection, so the enumeration raced a concurrentAdd. Ubuntu-only reproduction is scheduling sensitivity — the bug is platform-independent.Fix
Back the three recording collections with private lists and expose them as
IReadOnlyList<T>getters that return a snapshot copy taken under a single_recordingGatelock; guard the appends (and thePlannedExceptionscheck) under the same lock. The lock is never held across anawait.Snapshot-on-read is load-bearing — locking the writes alone would not stop a caller's
List<T>enumeration from throwing when anAddinterleaves itsMoveNext. The racing writer (memory-extraction,"memory extraction assistant"prompt) is filtered out of the assertion's.Where(Contains("session summarizer"))predicate, and the two summarizer calls the test inspects are ordered-before theirCompactionOutputvia the mailbox — so a thread-safe fake alone makes the test deterministic without adding an actor-side barrier.Scope
Tightly scoped to the three recording collections +
PlannedExceptions(the one field every call path touches). The deeper queue dequeues (PlannedToolCallDecisions/PlannedUsageOverrides/PlannedResponses) share the pattern but aren't reached concurrently in the current suite (sidecars branch earlier; one session's main-model calls are actor-serialized), so they were left alone to avoid widening the blast radius.Follow-up worth filing:
CompactionOutputisn't a "compaction fully settled" barrier because memory extraction fires fire-and-forget afterward with no completion signal. Fine for this test, but any future test asserting on the extraction call itself would need a real ack.Verification (on Linux — the platform that reproduced it)
IReadOnlyListshape)Netclaw.Actors.Tests: 2531 passed, 0 failed (was 2530 / 1-failed in CI)dotnet slopwatch analyze: 0 issues🤖 Analysis by akka-net-specialist + dotnet-concurrency-specialist via
/analyze-racy-test.