[RangeIndex] Replace busy-spin snapshot claim with SemaphoreSlim to avoid CPU spikes

libs/common/Testing/ExceptionInjectionHelper.cs

@@ -149,6 +149,16 @@ public static void WaitOnClear(ExceptionInjectionType exceptionType)
                 Thread.Yield();
         }
 
+        /// <summary>
+        /// Blocks until <paramref name="exceptionType"/> is disabled.
+        /// </summary>
+        /// <param name="exceptionType"></param>
+        public static void WaitOnClearWithThreadSleep(ExceptionInjectionType exceptionType)
+        {
+            while (IsEnabled(exceptionType))
+                Thread.Sleep(1);
+        }
+
         /// <summary>
         /// Wait on clear condition
         /// </summary>

libs/common/Testing/ExceptionInjectionType.cs

@@ -112,5 +112,11 @@ public enum ExceptionInjectionType
         /// in-flight ProcessRecord.
         /// </summary>
         RangeIndex_Migration_Receive_Pause_In_ProcessRecord,
+        /// <summary>
+        /// RangeIndex CPR snapshot: while enabled, the snapshot holds the per-tree snapshot lock
+        /// (blocking, no CPU) to emulate a slow <c>cpr_snapshot</c>, so a test can observe how
+        /// concurrent snapshots on the same tree (e.g. migration) behave under contention.
+        /// </summary>
+        RangeIndex_Snapshot_Inject_Latency,
     }
 }

libs/server/Resp/RangeIndex/RangeIndexManager.cs

@@ -66,6 +66,14 @@ public sealed partial class RangeIndexManager : IDisposable
         /// <summary>Gets the number of live (registered) BfTree indexes (activated + pending).</summary>
         internal int LiveIndexCount => liveIndexes.Count;
 
+        /// <summary>
+        /// Test-only: returns the live <see cref="TreeEntry"/> for the given key, or <c>null</c>
+        /// if no live (activated) tree exists for it. Used by tests that need to drive the
+        /// per-tree snapshot claim directly.
+        /// </summary>
+        internal TreeEntry GetLiveTreeEntryForTest(ReadOnlySpan<byte> keyBytes)
+            => liveIndexes.TryGetValue(KeyId(keyBytes), out var entry) && entry?.Tree != null ? entry : null;
+
         /// <summary>Gets the log-tied root directory for RI files. Used by the cluster replication layer
         /// to determine the target directory for received flush files on the replica side.</summary>
         public string RiLogRoot => riLogRoot;
@@ -149,45 +157,31 @@ internal sealed class TreeEntry
             public int SnapshotPending;
 
             /// <summary>
-            /// Per-tree snapshot serialization atomic. 0 = idle; 1 = a snapshot is in flight.
-            /// Both <see cref="GarnetRecordTriggers.OnFlush"/> and
-            /// <see cref="SnapshotAllTreesForCheckpoint"/> claim this before calling
-            /// <c>cpr_snapshot</c> so the two callers do not race for bftree's internal
-            /// <c>snapshot_in_progress</c> flag (which would no-op one of them silently).
+            /// Serializes CPR snapshots of this tree; concurrent snapshots are not allowed.
             /// </summary>
-            public int SnapshotInProgress;
+            private readonly SemaphoreSlim snapshotLock = new(1, 1);
 
-            /// <summary>Try to claim the per-tree snapshot atomic. Returns true if claimed.</summary>
-            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            public bool TryClaimSnapshot()
-                => Interlocked.CompareExchange(ref SnapshotInProgress, 1, 0) == 0;
-
-            /// <summary>Release the per-tree snapshot atomic.</summary>
-            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            public void ReleaseSnapshot()
-                => Volatile.Write(ref SnapshotInProgress, 0);
+            /// <summary>
+            /// Test-only: <c>true</c> while a snapshot holds <see cref="snapshotLock"/>. Lets a test
+            /// detect that the holder has entered the critical section.
+            /// </summary>
+            internal bool IsSnapshotInProgressForTest => snapshotLock.CurrentCount == 0;
 
             /// <summary>
-            /// Spin to claim the per-tree snapshot atomic, take a CPR snapshot of the live tree
-            /// directly into <paramref name="destinationPath"/>, then release the claim. The
-            /// claim serializes against concurrent flush / checkpoint / migration snapshots on
-            /// the same tree — bftree silently no-ops a <c>cpr_snapshot</c> that races its
-            /// internal <c>snapshot_in_progress</c> flag, so each caller must hold the claim
-            /// while it snapshots. bftree takes the destination path as a <c>cpr_snapshot</c>
-            /// argument and writes a fresh self-contained file, so we snapshot straight to
-            /// <paramref name="destinationPath"/>.
+            /// Under the per-tree snapshot lock (which serializes concurrent snapshots), take a CPR
+            /// snapshot of the live tree straight into <paramref name="destinationPath"/>.
             /// </summary>
             public void SnapshotUnderClaim(string destinationPath)
             {
-                while (!TryClaimSnapshot())
-                    Thread.Yield();
+                snapshotLock.Wait();
                 try
                 {
+                    ExceptionInjectionHelper.WaitOnClearWithThreadSleep(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency);
                     BfTreeService.CprSnapshotByPtr(Tree.NativePtr, destinationPath);
                 }
                 finally
                 {
-                    ReleaseSnapshot();
+                    snapshotLock.Release();
                 }
             }
 
@@ -648,7 +642,7 @@ internal void LogOnFlushInvariantViolation(string hashPrefix, long logicalAddres
         ///
         /// <para><b>Live case</b> (<c>stub.TreeHandle != 0</c>): take a CPR snapshot via the
         /// native handle. CPR is concurrent-safe with workers (no per-key X-lock needed).
-        /// Per-tree atomic <see cref="TreeEntry.SnapshotInProgress"/> serializes against
+        /// Per-tree lock <see cref="TreeEntry.SnapshotUnderClaim"/> serializes against
         /// concurrent <see cref="SnapshotAllTreesForCheckpoint"/> for the same tree (otherwise
         /// bftree's internal <c>snapshot_in_progress</c> would no-op one of them).</para>
         ///
@@ -784,7 +778,7 @@ internal void ClearCheckpointBarrier()
         /// to snapshot from; data.bftree was pre-staged by PreStage).</item>
         /// </list></para>
         ///
-        /// <para>Uses the per-tree atomic <see cref="TreeEntry.SnapshotInProgress"/> to serialize
+        /// <para>Uses the per-tree lock <see cref="TreeEntry.SnapshotUnderClaim"/> to serialize
         /// against concurrent <see cref="SnapshotTreeForFlush"/> for the same tree. Per-key
         /// X-lock is NOT taken here — that lock would deadlock if any deferred OnFlush fired
         /// on the checkpoint thread while it held S-locks on hot-path readers' shards.</para>

test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs

@@ -6,6 +6,9 @@
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
+#if DEBUG
+using Garnet.common;
+#endif
 using Garnet.server;
 using NUnit.Framework;
 using NUnit.Framework.Legacy;
@@ -2491,5 +2494,60 @@ public void RangeIndexManagerKeyExistsTest()
             ClassicAssert.IsTrue(rangeIndexManager.KeyExists(System.Text.Encoding.ASCII.GetBytes("str-key"), ref ctx));
             ClassicAssert.IsFalse(rangeIndexManager.KeyExists(System.Text.Encoding.ASCII.GetBytes("nope"), ref ctx));
         }
+
+#if DEBUG
+        /// <summary>
+        /// Concurrent CPR snapshots of the same tree serialize by <b>blocking</b> on the per-tree
+        /// <c>SemaphoreSlim</c>, not busy-spinning. A holder takes the snapshot lock with injected
+        /// latency; concurrent snapshots (the migration path: <c>SnapshotForMigration</c> →
+        /// <c>TreeEntry.SnapshotUnderClaim</c>) park instead of burning a core each.
+        ///
+        /// <para>Deterministic: no waiter completes while the holder holds the lock; all complete
+        /// once the latency clears.</para>
+        /// </summary>
+        [Test]
+        public void RIConcurrentSnapshotsTest()
+        {
+            const int waiterCount = 10;
+
+            using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig());
+            var db = redis.GetDatabase(0);
+
+            // DISK-backed RI mirrors migration and writes a real snapshot file to riLogRoot.
+            db.Execute("RI.CREATE", "snapidx", "DISK", "CACHESIZE", "65536", "MINRECORD", "8");
+            db.Execute("RI.SET", "snapidx", "field1", "value1");
+
+            var manager = server.Provider.StoreWrapper.rangeIndexManager;
+            var entry = manager.GetLiveTreeEntryForTest("snapidx"u8);
+            ClassicAssert.IsNotNull(entry, "expected a live BfTree for 'snapidx'");
+
+            string SnapPath(string name) => Path.Combine(manager.RiLogRoot, $"{name}.snapshot.bftree");
+
+            // Arm the latency injection so the holder parks (negligible CPU) while holding the lock.
+            ExceptionInjectionHelper.EnableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency);
+            try
+            {
+                var holder = Task.Run(() => entry.SnapshotUnderClaim(SnapPath("holder")));
+                ClassicAssert.IsTrue(SpinWait.SpinUntil(() => entry.IsSnapshotInProgressForTest, TimeSpan.FromSeconds(10)), "holder failed to acquire the snapshot lock");
+
+                // Waiters contend on the SAME tree; they block on the semaphore (no busy-spin).
+                var waiters = Enumerable.Range(0, waiterCount).Select(i => Task.Run(() => entry.SnapshotUnderClaim(SnapPath($"waiter{i}")))).ToArray();
+
+                // #1: while the holder holds the lock, no waiter can complete.
+                Thread.Sleep(500);
+                ClassicAssert.IsFalse(waiters.Any(t => t.IsCompletedSuccessfully), "a waiter completed while the holder held the snapshot lock");
+
+                // Release the latency; holder + waiters each run a real (~3s) snapshot serially.
+                ExceptionInjectionHelper.DisableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency);
+
+                // #2: everything completes (and any fault surfaces) once the lock is released.
+                ClassicAssert.IsTrue(Task.WaitAll(waiters.Append(holder).ToArray(), TimeSpan.FromSeconds(90)), "snapshots did not all complete after the lock was released");
+            }
+            finally
+            {
+                ExceptionInjectionHelper.DisableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency);
+            }
+        }
+#endif
     }
 }

website/docs/commands/range-index.md

@@ -44,6 +44,41 @@ Range Index is fully integrated with Garnet's checkpoint and AOF mechanisms:
 - **Eviction and lazy restore:** When memory pressure causes the stub to be evicted from the log,
   the BfTree data file is preserved. On next access, the tree is lazily restored from its snapshot.
 
+#### Snapshot performance
+
+A BfTree checkpoint takes a CPR (Concurrent Prefix Recovery) snapshot, which is synchronous and
+non-blocking to concurrent readers/writers. It has a **fixed floor of ~3 seconds** (the snapshot
+state machine advances through several phases, each with a short barrier) plus a component that
+grows roughly **linearly with the tree's data size**. The snapshot is dominated by total data
+**size**, not the number of keys.
+
+Measured on a single disk-backed tree (local NVMe, 1 KB values, cache large enough to keep the
+tree resident; times are approximate and hardware-dependent):
+
+| Tree data | Keys | Snapshot file | Snapshot time |
+|-----------|------|---------------|---------------|
+| 32 MB     | 32 K | 39 MB         | ~3.0 s        |
+| 64 MB     | 65 K | 78 MB         | ~3.1 s        |
+| 128 MB    | 129 K| 145 MB        | ~3.2 s        |
+| 512 MB    | 516 K| 594 MB        | ~4.0 s        |
+| 1 GB      | 1.0 M| 1.16 GB       | ~5.1 s        |
+| 2 GB      | 2.1 M| 2.44 GB       | ~6.3 s        |
+| 4 GB      | 4.1 M| 4.90 GB       | ~8.6 s        |
+
+Holding the data size roughly constant (~500 MB) while varying the value size — and therefore the
+key count — shows snapshot time is **independent of key count**:
+
+| Value size | Keys  | Snapshot time |
+|------------|-------|---------------|
+| 64 B       | 6.7 M | ~4.2 s        |
+| 256 B      | 2.0 M | ~4.0 s        |
+| 1 KB       | 516 K | ~4.1 s        |
+| 4 KB       | 131 K | ~4.0 s        |
+
+> **Note:** Each live tree is snapshotted independently during a checkpoint, and snapshots of the
+> same tree are serialized. A checkpoint of many large indexes therefore scales with the sum of
+> their sizes.
+
 ---
 
 ## Lifecycle Commands