fix(SEC-31,SEC-32): identify a probe-slot reservation by version, not by timestamp

ReleaseProbe recognised its own reservation by comparing NextProbeAtTicks to
now + _probeIntervalTicks. RecordInto's rearm-on-trip writes that identical
expression, so a concurrent RecordFailure on the same WindowState whose `now`
lands on the claimer's tick — routine at ~1 ms clock resolution under load — was
mistaken for the caller's own claim. The release then stomped the legitimate
fresh re-arm back to the stale previousProbeAtTicks, which is already due, handing
the next arrival a free probe the re-arm had just closed.

WindowState gains a monotonic ProbeVersion bumped by every writer of
NextProbeAtTicks (TryConsumeProbe's claim and RecordInto's re-arm alike).
TryConsumeProbe returns the stamp it set as part of a ProbeClaim; ReleaseProbe
restores the previous value only while the state's version still equals that
stamp, checking and restoring in one lock(state) section and bumping the version
again on restore so no other stale release can match either.

Test: ProbeSlotRestore_DoesNotStompConcurrentRearmAtSameTick, with the clock held
still so the claim and the interleaved failure necessarily share a tick. Making it
deterministic needed a seam — the claim-to-release window is a few nanoseconds and
racing threads do not hit it (an earlier thread-based attempt passed against the
defective guard three runs out of three, and its end state was ordering-dependent
rather than correctness-dependent, so it was dropped rather than shipped as
theatre). The seam is an internal ProbeReleaseInterleaveHook, null in production,
costing one null check on the already-refused path. Verified as a genuine red
against the timestamp guard: Expected ThrottledByPeer, Actual ProbeAdmitted.
This commit is contained in:
Joseph Doherty
2026-08-07 06:10:14 -04:00
parent acebe18773
commit 5b681ee59b
4 changed files with 102 additions and 13 deletions
@@ -219,6 +219,54 @@ public sealed class ApiKeyFailureLimiterTests
Assert.Equal(ApiKeyThrottleDecision.ProbeAdmitted, limiter.Check(holder));
}
/// <summary>
/// A compensating release must never undo a re-arm written by a concurrent failure on the same
/// partition. Both writers store the identical <c>now + interval</c> value when they share a
/// clock tick, so identifying the caller's own reservation by timestamp would let the release
/// stomp a fresh re-arm back to an already-due value and reopen the probe slot early. The clock
/// is deliberately held still here, which forces exactly that collision.
/// </summary>
[Fact]
public void ProbeSlotRestore_DoesNotStompConcurrentRearmAtSameTick()
{
ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch);
ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 5);
ApiKeyThrottlePartition holder = new("ipv4:10.0.0.1:1", "victim");
ApiKeyThrottlePartition other = new("ipv4:10.0.0.2:1", "victim");
// Trip both layers, then push the aggregate's slot one interval past the partition's, so
// every Check below claims the partition's slot and is then refused by the aggregate — the
// claim-and-compensate path under test.
RecordFailures(limiter, holder, 5);
clock.Advance(TimeSpan.FromSeconds(3));
limiter.RecordFailure(other);
clock.Advance(TimeSpan.FromSeconds(2));
// Land a failure on the same partition inside the claim-to-release window — the interleaving
// a concurrent RecordFailure produces, forced here so the assertion is deterministic. It
// shares the frozen clock tick with the claim, so both write the identical slot value.
int interleaved = 0;
limiter.ProbeReleaseInterleaveHook = () =>
{
if (Interlocked.Exchange(ref interleaved, 1) == 0)
{
limiter.RecordFailure(holder);
}
};
Assert.Equal(ApiKeyThrottleDecision.ThrottledByAggregate, limiter.Check(holder));
Assert.Equal(1, interleaved);
limiter.ProbeReleaseInterleaveHook = null;
// Drop the aggregate so the next decision reflects the composite partition alone.
limiter.Reset(other);
// The interleaved failure pushed the slot one interval past the (still unadvanced) clock, so
// no probe may be due. Restoring over it would leave the already-due earlier value and hand
// the next arrival a free probe.
Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(holder));
}
/// <summary>A zero probe interval restores absolute blocking (documented as not recommended).</summary>
[Fact]
public void ProbeIntervalZero_BlocksAbsolutely()