Repository navigation
Fix object-log write buffer leaks in ObjectAllocator read-only and recovery flushes - #2046
Merged
Merged
Conversation
…covery flushes ObjectAllocator flushes rent pooled object-log write buffers via CircularDiskWriteBuffer, which are returned to the pool only on Dispose(). The read-only flush and snapshot-region recovery copy paths never disposed theirs, leaking a buffer set per flush range / per recovered page. - Dispose the shared buffers after the read-only flush loop. - Dispose each recovery page's buffer after its WriteAsync. - Skip renting the buffer for inline-only read-only pages (objectIdMap empty), routing them through WriteInlinePageAsync. - Harden CircularDiskWriteBuffer.Dispose() memory ordering, since both paths now dispose while device writes may still be in flight. Co-authored-by: Copilot App <[email protected]> Copilot-Session: f45128a9-8d17-4a2e-b84d-6da1afd3cc73
Ted Hart (TedHartMS)
requested review from
Badrish Chandramouli (badrishc)
and
a lite review from Copilot
August 7, 2026 23:44
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes pooled object-log write-buffer leaks in Tsavorite’s ObjectAllocator flush paths by ensuring CircularDiskWriteBuffer instances are disposed (returning rented buffers to the pool), skipping object-log buffer usage for inline-only read-only pages, and hardening Dispose()/callback memory ordering to avoid a dispose-vs-callback race.
Changes:
- Add a full memory fence in
CircularDiskWriteBuffer.Dispose()to prevent a race where neitherDispose()nor the IO callback returns pooled buffers. - Dispose shared flush buffers after issuing all read-only page writes, and dispose per-page flush buffers in recovery flushes.
- Avoid renting object-log write buffers for read-only pages with
objectIdMap.Count == 0, routing them through the inline-only page write path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libs/storage/Tsavorite/cs/src/core/Allocator/ObjectSerialization/CircularDiskWriteBuffer.cs | Strengthens Dispose() ordering so buffer returns can’t be skipped when disposing with writes in flight. |
| libs/storage/Tsavorite/cs/src/core/Allocator/ObjectAllocatorImpl.cs | Disposes shared flush buffers after read-only flush issuance and skips object-log buffer usage for inline-only read-only pages. |
| libs/storage/Tsavorite/cs/src/core/Allocator/AllocatorBase.cs | Disposes per-page flush buffers in recovery flushes (snapshot-object copy path). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Badrish Chandramouli (badrishc)
approved these changes
Aug 7, 2026
Wrap the read-only flush loop and the per-page recovery WriteAsync in try/finally so the rented CircularDiskWriteBuffer is returned to the pool even if a page write throws. Co-authored-by: Copilot App <[email protected]> Copilot-Session: f45128a9-8d17-4a2e-b84d-6da1afd3cc73
Badrish Chandramouli (badrishc)
approved these changes
Aug 8, 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
ObjectAllocatorflushes rent pooled object-log write buffers through aCircularDiskWriteBuffer, which are returned to the pool only viaDispose(). Two flushpaths never disposed theirs, so the buffers leaked:
AsyncFlushPagesForReadOnly) — one buffer set per flush range.AsyncFlushPagesForRecovery) — one per recovered page.The read-only path also rented the buffer even for inline-only pages that have nothing to
write to the object log.
Fix
WriteAsync.objectIdMap.Count == 0),routing them through
WriteInlinePageAsync. Restricted toReadOnly; Recovery andSnapshot can't use this shortcut.
CircularDiskWriteBuffer.Dispose()memory ordering: both paths now dispose whilewrites may be in flight, so add a barrier so
Dispose()andFlushToDeviceCallbackcan't race and both skip the return-to-pool.
The Snapshot checkpoint path already disposes via
RecoveryInfo.Dispose()and is unchanged.Testing
Tsavorite.test.recordops: 58/58 passed.Tsavorite.test.recovery: 31/31 passed (object recovery, disk-delete, large-object,snapshot-eviction).