fix(streaming): don't recycle in-flight pooled buffers mid-batch - #10264
Merged
ReubenBond merged 2 commits intoJul 15, 2026
Merged
ReubenBond merged 2 commits into
ReubenBond merged 2 commits into
Conversation
The chronological eviction strategy eagerly freed the previous pooled buffer whenever the cache was empty (PurgeObservable.IsEmpty). Because callers encode an entire batch into pooled buffers before committing it, IsEmpty stays true throughout the encode and the buffer being freed can still hold in-flight, uncommitted segments for the current batch. The freed buffer is returned to the LIFO pool and the next allocation in the same batch pops it straight back, overwriting the earlier segments from offset 0 and silently corrupting payloads (surfacing later as an ArgumentOutOfRangeException in SegmentBuilder.ReadNextString). Remove the eager-free branch; buffers are reclaimed safely at purge time by FreePurgedBuffers. This affects all three pooled caches (EventHub, Memory, Generator), which share this eviction strategy. Removing the eager-free would expose a pre-existing buffer leak for oversized messages, since it was the only thing that reclaimed a freshly allocated block when a message was too big to ever fit. The EventHub, Memory and Generator caches now dispose the freshly allocated block back to the pool and throw before registering it with the eviction strategy, so repeated oversized (poison) messages reuse a single block instead of growing the retained set without bound. Adds regression tests covering multi-buffer batch payload integrity and the oversized-message no-leak behavior. Fixes dotnet#10263 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 577af892-8411-4f35-9b87-55ad42b0d059
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a streaming buffer-corruption bug in Orleans’ pooled queue cache infrastructure by preventing in-flight pooled buffers from being recycled mid-batch, and adds regression tests to cover the corruption scenario plus an oversized-message buffer leak.
Changes:
- Removed eager buffer recycling in
ChronologicalEvictionStrategy.OnBlockAllocatedto avoid returning still-in-use buffers to the pool mid-batch. - Updated EventHub/Memory/Generator pooled caches (and the test converter) to validate segment fit before registering a new buffer with the eviction strategy, disposing the unused buffer on failure to avoid leaks.
- Added regression tests validating multi-buffer batch payload integrity from an empty cache and ensuring repeated oversized messages do not leak buffers.
Show a summary per file
| File | Description |
|---|---|
| test/Orleans.Streaming.Tests/OrleansRuntime/Streams/PooledQueueCacheTests.cs | Adds regression tests for mid-batch corruption and oversized-message non-leak; updates test converter allocation order accordingly. |
| src/Orleans.Streaming/MemoryStreams/MemoryPooledCache.cs | Disposes newly-allocated buffers on oversized-message failure before eviction registration to prevent leaks. |
| src/Orleans.Streaming/Generator/GeneratorPooledCache.cs | Same oversized-message no-leak allocation ordering fix for generator cache. |
| src/Orleans.Streaming/Common/PooledCache/ChronologicalEvictionStrategy.cs | Removes eager-free logic from OnBlockAllocated to prevent recycling in-flight buffers while cache is “empty”. |
| src/Azure/Orleans.Streaming.EventHubs/Providers/Streams/EventHub/EventHubQueueCache.cs | Same oversized-message no-leak allocation ordering fix for EventHub queue cache. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 3
Fix 'to big' -> 'too big' typo in the EventHub and Generator oversized-message exception messages, matching the Memory cache, and make the test TestQueueMessage.Data an init-only property so per-message payloads can't be mutated after construction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 577af892-8411-4f35-9b87-55ad42b0d059
This was referenced Jul 22, 2026
This was referenced Jul 29, 2026
This was referenced Jul 29, 2026
Merged
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Fixes #10263.
Pooled stream caches (EventHub, Memory, Generator) can silently corrupt message payloads, which later surfaces as an
ArgumentOutOfRangeExceptioninSegmentBuilder.ReadNextString.ChronologicalEvictionStrategy.OnBlockAllocatedeagerly freed the previous pooled buffer whenever the cache reported empty (PurgeObservable.IsEmpty). That was safe when the strategy was written, but callers now encode an entire batch into pooled buffers before committing it to the cache. During that encodeIsEmptystaystrue, so the buffer being freed can still hold in-flight, uncommitted segments for the current batch. The freed buffer is returned to the (LIFO) object pool, and the very next allocation in the same batch pops it straight back and overwrites the earlier segments from offset 0 — corrupting those messages. The three pooled caches all share this eviction strategy, so all of them are affected, not just EventHub.Solution
Remove the eager-free branch in
OnBlockAllocated. In-use buffers are already reclaimed correctly at purge time byFreePurgedBuffers, which only releases buffers once their segments are no longer referenced by committed messages.Removing the eager-free would, on its own, expose a pre-existing buffer leak: for a message too large to ever fit in a pooled buffer, the eager-free was the only thing that reclaimed the freshly allocated (and never-committed) block. To close that, the EventHub, Memory and Generator caches now dispose the freshly allocated block back to the pool and throw before registering it with the eviction strategy, so repeated oversized (poison) messages reuse a single block instead of growing the retained set without bound.
Adds regression tests for multi-buffer batch payload integrity from an empty cache and for the oversized-message no-leak behavior. Both fail without the fix and pass with it.
Microsoft Reviewers: Open in CodeFlow