fix(acl): a stale-generation straggler can no longer roll the cache stamp backward and thrash ClearAll

2026-08-17 05:51:24 -04:00
parent f797df5187
commit 98d6a1908e
2 changed files with 74 additions and 2 deletions
@@ -252,9 +252,16 @@ public sealed class SessionAclDecisionCache
            // before, and it is accepted — the loss window is a single compare-and-swap wide, and it is
            // paid only by threads that overflowed the same shard simultaneously, against a refill cost
            // paid by every eviction.
+            //
+            // A shard from a PREVIOUS generation contributes no hint: the fill level it reached belongs to a
+            // set of grants that no longer exists, and it is about to be displaced by the generation check
+            // below in any case. Sizing this generation's storage from the last one's crawl would be a guess
+            // dressed as evidence.
            shard = Displace(
                sessionKey, shard, generation,
-                capacityHint: Math.Clamp(Volatile.Read(ref shard.ApproxCount), 0, MaxEntriesPerSession));
+                capacityHint: shard.Generation == generation
+                    ? Math.Clamp(Volatile.Read(ref shard.ApproxCount), 0, MaxEntriesPerSession)
+                    : 0);
        }

        // The shard must belong to THIS generation before anything is filed in it. Reached in two ways: a
@@ -420,10 +427,33 @@ public sealed class SessionAclDecisionCache
    internal void AdvanceGenerationForTest(long generation) =>
        Interlocked.Exchange(ref _generation, generation);

+    /// <summary>
+    ///     Advances the cache-wide stamp when this call presents a newer generation, and purges on the way.
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         ⚠️ <b>The comparison is <c>&lt;=</c>, not <c>!=</c>, and that is a straggler guard.</b> The
+    ///         generation only ever increases (<c>DataPlaneAuthorityState</c> counts it up rather than
+    ///         deriving it from the revision hash, precisely so it cannot go backwards), but a <i>request</i>
+    ///         can still present an older one: it captured its authority snapshot before a deploy and then
+    ///         lost the scheduling race to a request that captured after. An inequality test treats that
+    ///         straggler as a deploy, rolls the stamp <i>backward</i>, and purges the new generation's
+    ///         freshly-populated shards — so the post-deploy window, when the cache is coldest and doing the
+    ///         most work, is exactly when it thrashes. Verdicts are safe either way now that each shard
+    ///         carries its own generation; what is at stake here is only whether the cache is any use.
+    ///     </para>
+    ///     <para>
+    ///         The straggler's own lookup is unaffected by returning early: its shard-generation check misses
+    ///         against the newer shards regardless, so it recomputes, which is the correct outcome for a
+    ///         request holding a superseded snapshot. <c>_generation</c> starts at <c>-1</c>, below any real
+    ///         generation, so the first one still passes.
+    ///     </para>
+    /// </remarks>
+    /// <param name="generation">The generation the calling request is being decided under.</param>
    private void InvalidateIfStale(long generation)
    {
        var seen = Interlocked.Read(ref _generation);
-        if (seen == generation) return;
+        if (generation <= seen) return;

        // Only the thread that actually moves the stamp clears, so a burst of concurrent requests arriving
        // just after a deploy repopulates once rather than each clearing the others' fresh entries.