diff --git a/dotnet/EcencyApi.Tests/CheckinGateTests.cs b/dotnet/EcencyApi.Tests/CheckinGateTests.cs new file mode 100644 index 00000000..1402dadf --- /dev/null +++ b/dotnet/EcencyApi.Tests/CheckinGateTests.cs @@ -0,0 +1,283 @@ +using EcencyApi.Infrastructure; +using Xunit; + +namespace EcencyApi.Tests; + +/// +/// The check-in gate is the one place in this service that can silently decide a +/// user action never happened: it answers 201 and drops the request. Every rule +/// it depends on is pinned here, because the failure mode is invisible: the +/// client is told the check-in landed. +/// +public class CheckinGateTests +{ + /// + /// The web client's check-in poll interval (1000 * 60 * 15 + 8 in + /// vision-next's user-activity-recorder.tsx). The gate has to let a + /// caller polling at this rate through every single time. + /// + private const long ClientPollIntervalMs = 1000 * 60 * 15 + 8; + + /// + /// Bounds on the points backend's own per-account minimum spacing, which is a + /// little under 15 minutes. The exact value belongs to that service, so the + /// gate is pinned against the bounds rather than the number: it must absorb + /// only inside the lower bound, then anchor only outside the upper one. + /// + private const long BackendMinSpacingLowerBoundMs = 870_000; + + private const long BackendMinSpacingUpperBoundMs = 900_000; + + [Fact] + public void TheWindowClosesWellBeforeAClientPollsAgain() + { + // The regression this guards: the window used to sit 8 ms below the poll + // interval, so whether a legitimate check-in survived came down to whether + // its arrival delay happened to be longer than the previous one's. + Assert.True(CheckinGate.WindowMs < ClientPollIntervalMs); + Assert.True(ClientPollIntervalMs - CheckinGate.WindowMs >= 60_000, + "the gap between the window and the poll interval must be far larger than arrival jitter"); + } + + [Fact] + public void TheWindowNeverOutlastsTheBackendsOwnSpacing() + { + // Keeps the gate strictly weaker than the rule it fronts, so absorbing a + // request can never cost an account a check-in it would otherwise have got. + Assert.True(CheckinGate.WindowMs <= BackendMinSpacingLowerBoundMs); + } + + [Fact] + public void TheAnchorOnlyMovesOnceTheBackendWouldCredit() + { + // The other half of the same rule. Anchoring on an attempt the backend + // refuses moves the window under the caller's own schedule, which costs it + // the next check-in just as surely as absorbing one would. + Assert.True(CheckinGate.AnchorAfterMs >= BackendMinSpacingUpperBoundMs); + Assert.True(CheckinGate.AnchorAfterMs > CheckinGate.WindowMs); + } + + [Fact] + public void ACachedEntryOutlivesItsOwnWindow() + { + // If the entry expired first, the window would end early and silently. + Assert.True(CheckinGate.CacheTtlSeconds * 1000 >= CheckinGate.AnchorAfterMs); + } + + [Fact] + public void EachAccountGetsItsOwnNamespacedWindow() + { + // Accounts sharing one network address must not share a check-in slot. + // CacheKey takes nothing but the username, which is what makes that true; + // the namespace keeps it clear of the other users of this cache. + Assert.NotEqual(CheckinGate.CacheKey("alice"), CheckinGate.CacheKey("bob")); + Assert.StartsWith("checkin:", CheckinGate.CacheKey("alice")); + } + + [Fact] + public void AFirstCheckinIsAlwaysForwarded() + { + Assert.False(CheckinGate.IsWithinWindow(null, 1_000_000)); + Assert.False(CheckinGate.IsWithinWindow("", 1_000_000)); + } + + [Fact] + public void ARepeatInsideTheWindowIsAbsorbed() + { + var stamp = CheckinGate.Stamp(1_000_000); + + Assert.True(CheckinGate.IsWithinWindow(stamp, 1_000_000)); + Assert.True(CheckinGate.IsWithinWindow(stamp, 1_000_000 + CheckinGate.WindowMs - 1)); + } + + [Fact] + public void TheWindowEndsExactlyWhereItSays() + { + var stamp = CheckinGate.Stamp(1_000_000); + + Assert.False(CheckinGate.IsWithinWindow(stamp, 1_000_000 + CheckinGate.WindowMs)); + Assert.False(CheckinGate.IsWithinWindow(stamp, 1_000_000 + CheckinGate.WindowMs + 1)); + } + + [Theory] + [InlineData("not-a-number")] + [InlineData("NaN")] + [InlineData(" ")] + public void AnUnreadableStampFailsOpen(string stored) + { + // Forwarding a repeat costs one upstream call the backend discards. + // Absorbing a real check-in costs the account its check-in and its streak. + Assert.False(CheckinGate.IsWithinWindow(stored, 1_000_000)); + } + + [Fact] + public void AStampFromTheFutureFailsOpen() + { + var stamp = CheckinGate.Stamp(2_000_000); + + Assert.False(CheckinGate.IsWithinWindow(stamp, 1_000_000)); + } + + [Theory] + [InlineData(60_000)] // absorbed outright + [InlineData(420_000)] // absorbed outright + [InlineData(800_000)] // forwarded, refused by the backend, must not anchor + [InlineData(880_000)] // same, right up against the anchor threshold + public void ASecondSourceNeverDisplacesASteadyPoller(long extraSourceOffsetMs) + { + // Mirrors the handler loop: a forwarded check-in stores its timestamp, an + // absorbed one stores nothing. A second check-in source for the same + // account sits between the polls, either a second tab or the ping a page + // load fires on mount. + // + // While an absorbed request also refreshed the window, that extra source + // moved the window mid-cycle, the next scheduled poll landed inside it and + // was absorbed, that absorption moved the window again, so the account + // never got another check-in through until its page reloaded. Anchoring the + // window to the last *forwarded* check-in is what breaks that loop. + string? stored = null; + var pollsForwarded = 0; + + for (var poll = 0; poll < 20; poll++) + { + var pollAt = poll * ClientPollIntervalMs; + + var pollDecision = CheckinGate.Decide(stored, pollAt); + stored = pollDecision.StampToStore ?? stored; + if (pollDecision.Forward) + { + pollsForwarded++; + } + + var extraDecision = CheckinGate.Decide(stored, pollAt + extraSourceOffsetMs); + stored = extraDecision.StampToStore ?? stored; + } + + Assert.Equal(20, pollsForwarded); + } + + [Fact] + public void AnAttemptTheBackendWillRefuseIsForwardedButDoesNotAnchor() + { + // A second source landing between the two thresholds: too far out for the + // gate to absorb, too close for the backend to credit. It has to go + // upstream. It also has to leave the anchor alone, or the caller's own + // poll a moment later lands inside a window that moved out from under it. + var anchor = CheckinGate.Stamp(0); + var tooEarly = (CheckinGate.WindowMs + CheckinGate.AnchorAfterMs) / 2; + + var extra = CheckinGate.Decide(anchor, tooEarly); + + Assert.True(extra.Forward); + Assert.Null(extra.StampToStore); + + // The anchor is untouched, so the scheduled poll is still due. + var poll = CheckinGate.Decide(anchor, ClientPollIntervalMs); + + Assert.True(poll.Forward); + Assert.NotNull(poll.StampToStore); + } + + [Fact] + public void AStampIsOnlyEverHandedOutForAForwardedCheckin() + { + // An absorbed request never reaches upstream, so a stamp for one would + // anchor the window on a check-in that never happened. + var anchor = CheckinGate.Stamp(0); + + for (var at = 0L; at <= CheckinGate.AnchorAfterMs * 2; at += 10_000) + { + var decision = CheckinGate.Decide(anchor, at); + if (decision.StampToStore != null) + { + Assert.True(decision.Forward); + } + } + } + + [Fact] + public void AnAbsorbedRepeatStoresNothing() + { + // The structural half of the rule above: the gate cannot hand a caller a + // timestamp to store for a request it just absorbed. + var stamp = CheckinGate.Stamp(1_000_000); + var decision = CheckinGate.Decide(stamp, (1_000_000 + CheckinGate.WindowMs) - 1); + + Assert.False(decision.Forward); + Assert.Null(decision.StampToStore); + } + + [Fact] + public void AForwardedCheckinStoresItsOwnArrival() + { + var decision = CheckinGate.Decide(null, 1_000_000); + + Assert.True(decision.Forward); + Assert.Equal(CheckinGate.Stamp(1_000_000), decision.StampToStore); + } + + [Fact] + public async Task ConcurrentCheckinsForOneAccountCollapseToOneForward() + { + // Read, decide and reserve used to be three steps, so simultaneous + // check-ins for one account could all read an empty anchor and all go + // upstream. Absorbing the losers is right: they are milliseconds behind + // the winner, which the backend refuses anyway. + var username = "burst-" + Guid.NewGuid().ToString("n"); + var nowMs = 1_700_000_000_000; + + var decisions = await Task.WhenAll(Enumerable.Range(0, 32) + .Select(_ => Task.Run(() => CheckinGate.DecideAndReserve(username, nowMs)))); + + Assert.Equal(1, decisions.Count(d => d.Forward)); + Assert.Equal(1, decisions.Count(d => d.StampToStore != null)); + } + + [Fact] + public void ConcurrencyControlDoesNotMakeAccountsWaitOnEachOther() + { + // Two accounts checking in at the same moment are unrelated events; each + // gets its own anchor and both go upstream. + var nowMs = 1_700_000_000_000; + + var first = CheckinGate.DecideAndReserve("solo-" + Guid.NewGuid().ToString("n"), nowMs); + var second = CheckinGate.DecideAndReserve("solo-" + Guid.NewGuid().ToString("n"), nowMs); + + Assert.True(first.Forward); + Assert.True(second.Forward); + } + + [Fact] + public void AReservedAnchorAbsorbsTheNextCheckinAndThenReleases() + { + // The reservation is the anchor, so it has to behave like one: absorb + // inside the window, forward once the account is due again. + var username = "anchor-" + Guid.NewGuid().ToString("n"); + var nowMs = 1_700_000_000_000; + + Assert.True(CheckinGate.DecideAndReserve(username, nowMs).Forward); + Assert.False(CheckinGate.DecideAndReserve(username, nowMs + CheckinGate.WindowMs - 1).Forward); + Assert.True(CheckinGate.DecideAndReserve(username, nowMs + ClientPollIntervalMs).Forward); + } + + [Fact] + public void ABurstFromOneAccountStillCollapsesToOneUpstreamCall() + { + // The gate still has to do its job: repeated check-ins inside one window + // must cost exactly one upstream call. + string? stored = null; + var forwarded = 0; + + for (var i = 0; i < 10; i++) + { + var decision = CheckinGate.Decide(stored, i * 30_000L); + stored = decision.StampToStore ?? stored; + if (decision.Forward) + { + forwarded++; + } + } + + Assert.Equal(1, forwarded); + } +} diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs index 66363e08..2697299a 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs @@ -81,72 +81,20 @@ public static async Task Activities(HttpContext ctx) if (tyIsTen) { - // req.headers['x-real-ip'] || req.connection.remoteAddress || req.headers['x-forwarded-for'] || '' - var vip = ctx.Request.Headers["x-real-ip"].ToString(); - if (vip.Length == 0) + // Keyed on the account, not the caller's address: see CheckinGate for why + // an address-keyed window makes accounts behind one address compete for a + // single check-in slot, plus why the read, the decision and the write + // have to be one step. + var decision = CheckinGate.DecideAndReserve( + username, DateTimeOffset.UtcNow.ToUnixTimeMilliseconds()); + + if (!decision.Forward) { - vip = ctx.Connection.RemoteIpAddress?.ToString() ?? ""; - } - if (vip.Length == 0) - { - vip = ctx.Request.Headers["x-forwarded-for"].ToString(); - } - var identifier = vip; - - string? rec = null; - try - { - rec = MemCache.Get(identifier); - } - catch (Exception e) - { - Console.Error.WriteLine(e); - Console.Error.WriteLine("Cache get failed."); - } - - if (!string.IsNullOrEmpty(rec)) - { - var nowMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); - var withinWindow = double.TryParse(rec, System.Globalization.NumberStyles.Float, - System.Globalization.CultureInfo.InvariantCulture, out var recMs) - && nowMs - recMs < 900000; - - if (withinWindow) - { - await ctx.SendJson(201, new JsonObject()); - } - try - { - MemCache.Set(identifier, - DateTimeOffset.UtcNow.ToUnixTimeMilliseconds().ToString(), 901); - } - catch (Exception e) - { - Console.Error.WriteLine(e); - Console.Error.WriteLine("Cache set failed."); - } - if (withinWindow) - { - // The Node implementation was missing this return: it acked the - // rate-limited checkin with 201 but still forwarded the duplicate - // event upstream (pipe then skipped the second response, logging - // "headers already sent" on every occurrence). Short-circuit after - // refreshing the sliding window, as the branch always intended. - return; - } - } - else - { - try - { - MemCache.Set(identifier, - DateTimeOffset.UtcNow.ToUnixTimeMilliseconds().ToString(), 901); - } - catch (Exception e) - { - Console.Error.WriteLine(e); - Console.Error.WriteLine("Cache set failed."); - } + // A repeat inside the window: ack it and drop it. The anchor stays + // where it is; moving it here would push it past this account's next + // scheduled check-in, which would then be absorbed as well. + await ctx.SendJson(201, new JsonObject()); + return; } } diff --git a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs new file mode 100644 index 00000000..98626e93 --- /dev/null +++ b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs @@ -0,0 +1,182 @@ +using System.Globalization; + +namespace EcencyApi.Infrastructure; + +/// +/// De-duplication window for check-in activities (ty 10) on +/// /private-api/usr-activity. +/// +/// Clients poll check-in on a fixed interval a little over 15 minutes. The +/// points backend enforces its own minimum spacing per account and +/// refuses anything closer. This gate exists only to save the upstream call for +/// a repeat the backend would refuse anyway; it is not an authorization check +/// and it is not a rate limiter. Every rule below follows from that. Each one +/// has been wrong here before: +/// +/// +/// The window is keyed on the account, never on the caller's +/// address. Several accounts routinely share one address (NAT, carrier-grade +/// NAT, a household). An address-keyed window makes them compete for a single +/// check-in slot. Keying on the address also buys nothing: a check-in carries a +/// signed code, so a caller can only check in as an account it controls. +/// The window is anchored, not sliding. An absorbed repeat stores +/// nothing. Refreshing the anchor on one pushes the window past the caller's +/// next scheduled check-in, which is then absorbed too, leaving a steady poller +/// with no way out. +/// Only a check-in the backend will actually credit becomes the new +/// anchor. Between and a +/// check-in is forwarded but leaves the anchor alone, because the backend is +/// going to refuse it as too early. Anchoring on a refused attempt would move +/// the window under the caller's own schedule and cost it the next check-in, +/// which is the same failure in a different disguise. +/// The window stays comfortably below the client poll interval. At or +/// near it, which of two consecutive polls survives comes down to arrival +/// jitter. +/// +/// +/// The two thresholds exist because two different clocks matter: the client +/// decides how often a check-in arrives, the backend decides how often one +/// counts. Every boundary case resolves toward forwarding. A needless forward +/// costs one upstream call that the backend discards; a needless absorb costs +/// an account its check-in and its streak. The caller cannot even tell, +/// because the gate answers 201. +/// +public static class CheckinGate +{ + /// + /// What the gate decided for one request. is + /// non-null only when the request is forwarded and is far enough + /// from the last anchor for the backend to credit it. + /// + public readonly record struct Decision(bool Forward, string? StampToStore); + + /// + /// A repeat closer than this to the anchor is absorbed. Deliberately well + /// under the client poll interval, so a steady poller is never a coin flip. + /// Also under the backend's per-account spacing, so anything absorbed here + /// would have been refused there. + /// + public const long WindowMs = 780_000; + + /// + /// A forwarded check-in this far from the anchor moves it. Must be at least + /// the backend's per-account spacing: too low and the gate anchors on an + /// attempt the backend refused, too high and it merely forwards once more + /// than it had to, which is the harmless direction. + /// + public const long AnchorAfterMs = 900_000; + + /// + /// Derived from the thresholds so they cannot drift apart: an entry that + /// outlives its own usefulness would only be read to conclude "expired". + /// + public const double CacheTtlSeconds = AnchorAfterMs / 1000d; + + /// Cache key for an account's anchor. + public static string CacheKey(string username) => "checkin:" + username; + + /// Serializes a timestamp for the cache; inverse of the parse in + /// . + public static string Stamp(long nowMs) => nowMs.ToString(CultureInfo.InvariantCulture); + + /// + /// How long ago was, or null when there is no + /// usable anchor. Absent, unreadable and future timestamps all read as no + /// anchor, which forwards the check-in and replaces the bad entry. + /// + public static double? AgeMs(string? recorded, long nowMs) + { + if (string.IsNullOrEmpty(recorded)) + { + return null; + } + + if (!double.TryParse(recorded, NumberStyles.Float, CultureInfo.InvariantCulture, out var recMs)) + { + return null; + } + + var age = nowMs - recMs; + return age >= 0 ? age : null; + } + + /// True when the anchor still covers , so a + /// check-in arriving then is absorbed. + public static bool IsWithinWindow(string? recorded, long nowMs) => + AgeMs(recorded, nowMs) is { } age && age < WindowMs; + + /// Decides one check-in against the account's current anchor. + public static Decision Decide(string? recorded, long nowMs) + { + var age = AgeMs(recorded, nowMs); + + if (age is { } covered && covered < WindowMs) + { + return new Decision(false, null); + } + + var anchors = age is not { } gap || gap >= AnchorAfterMs; + return new Decision(true, anchors ? Stamp(nowMs) : null); + } + + /// + /// Reads the anchor, decides against it, then stores the new one as a single + /// step. + /// + /// The three have to happen together. Two check-ins for one account can + /// arrive in the same instant (two tabs opening at once both fire the ping + /// their page load schedules). If both read the anchor before either writes, + /// both are forwarded, which is the burst this gate exists to collapse. Serializing them makes a concurrent duplicate behave exactly + /// like a sequential one: the second reads the anchor the first just wrote + /// and is absorbed. That stays correct in the direction this gate cares + /// about, because a check-in milliseconds behind another is one the backend + /// refuses regardless. + /// + /// Striped rather than one global lock so unrelated accounts never queue + /// behind each other. The lock covers in-memory work only. It is never held + /// across an await. + /// + public static Decision DecideAndReserve(string username, long nowMs) + { + var key = CacheKey(username); + + lock (StripeFor(key)) + { + string? recorded = null; + try + { + recorded = MemCache.Get(key); + } + catch (Exception e) + { + Console.Error.WriteLine(e); + Console.Error.WriteLine("Cache get failed."); + } + + var decision = Decide(recorded, nowMs); + + if (decision.StampToStore != null) + { + try + { + MemCache.Set(key, decision.StampToStore, CacheTtlSeconds); + } + catch (Exception e) + { + Console.Error.WriteLine(e); + Console.Error.WriteLine("Cache set failed."); + } + } + + return decision; + } + } + + private const int StripeCount = 64; + + private static readonly object[] Stripes = + Enumerable.Range(0, StripeCount).Select(_ => new object()).ToArray(); + + private static object StripeFor(string key) => + Stripes[(uint)StringComparer.Ordinal.GetHashCode(key) % StripeCount]; +}