Repository navigation
Fix flaky RecoveryRollback test: GC-movable key memory in async tests - #2137
Merged
Merged
Conversation
RecoveryRollback is an async method, so its locals can be relocated by the GC. It passed SpanByte.FromPinnedVariable(ref key) into TestSpanByteKey.FromPinnedSpan, which retains only a raw pointer and no GC reference, so a collection that moved the local left the store reading stale memory. Reads then returned NotFound or another key's value. Use an array-backed key, matching the pattern already used by the sibling RecoveryCheck tests in this file.
SpanByte.FromPinnedVariable captures a raw pointer and requires non-movable storage, but these async tests passed locals, which the GC may relocate. TestSpanByteKey.FromPinnedSpan keeps only that pointer and no GC reference, so a collection could leave an operation reading stale memory. Back the affected keys and values with stable storage: GC-tracked arrays where the raw pointer is no longer needed, and GC.AllocateArray(pinned: true) where FromPinnedVariable is retained. The one site already using a pinned array is unchanged.
Ted Hart (TedHartMS)
requested review from
Badrish Chandramouli (badrishc)
and
a balanced review from Copilot
September 15, 2026 21:31
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently eliminate unsafe pointers to movable async state without introducing unresolved correctness issues.
Pull request overview
Fixes flaky Tsavorite async tests by preventing raw pointers from referencing GC-movable memory.
Changes:
- Uses array-backed keys where pointer retention is unnecessary.
- Uses pinned arrays where
SpanByte.FromPinnedVariableremains necessary. - Aligns
RecoveryRollbackwith existing recovery-test patterns.
File summaries
| File | Description |
|---|---|
ReadAddressTests.cs |
Pins values used through raw pointers. |
UnsafeContextTests.cs |
Pins values used by unsafe upserts. |
TransactionalUnsafeContextTests.cs |
Pins transactional keys and values. |
SimpleRecoveryTest.cs |
Uses array-backed value spans. |
RecoveryCheckTests.cs |
Makes rollback keys GC-safe. |
CheckpointManagerTests.cs |
Uses array-backed keys and values. |
StateMachineDriverTests.cs |
Adds reusable array-backed key construction. |
NamespaceTests.cs |
Pins the upsert value. |
MiscTests.cs |
Pins values used during recovery testing. |
CompletePendingTests.cs |
Pins values before pending operations. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 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
Sep 22, 2026
x@01 (x-at-01)
added a commit
to webc-fork/garnet
that referenced
this pull request
Oct 4, 2026
…soft#2156), path composition normalize (microsoft#2147), refuse serve w/o HLog checkpoint (microsoft#2153), retire outdated checkpoints (microsoft#2144), checkpoint serialize race (microsoft#2136), RecoveryRollback flaky (microsoft#2137)
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
RecoveryCheck2Tests.RecoveryRollbackfails intermittently — ~38% of fullTsavorite.test.recoveryruns, but never in isolation. BothSnapshotandFoldOvervariants fail, with reads returningNotFoundor another key'svalue (e.g.
output = 7, key = 691).Root cause
A test bug, not a product bug.
SpanByte.FromPinnedVariablecaptures a raw pointer viaUnsafe.AsPointerandrequires non-movable storage.
TestSpanByteKey.FromPinnedSpanthen retains onlythat
void*witharr = null— no GC reference.RecoveryRollbackpassedasync-method locals, so a GC relocation left the store reading stale memory.The sibling
RecoveryCheck1/2/3/4/5tests in the same file already use anarray-backed key, with the comment "Local variables in an async function can be
moved, so we must use an array for the key."
RecoveryRollbacknever got it.Changes
RecoveryRollback: use an array-backed key, matching its sibling tests.asynctest methodsacross 9 files — GC-tracked arrays where the raw pointer isn't needed,
GC.AllocateArray(pinned: true)whereFromPinnedVariableis retained.Tests only; no product code changes. The one site already using a pinned array
is correct and unchanged.
Verification
RecoveryRollbackwith[Repeat(40)]Also green:
VersionSwitch24,session.context142,ReadAddress33,CompletePending/Misc/Namespace49.dotnet format --verify-no-changesclean on
Tsavorite.slnx.