Key the check-in de-duplication window per account and stop it sliding

dotnet/EcencyApi.Tests/CheckinGateTests.cs

@@ -0,0 +1,186 @@
+using EcencyApi.Infrastructure;
+using Xunit;
+
+namespace EcencyApi.Tests;
+
+/// <summary>
+/// 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.
+/// </summary>
+public class CheckinGateTests
+{
+    /// <summary>
+    /// The web client's check-in poll interval (<c>1000 * 60 * 15 + 8</c> in
+    /// vision-next's <c>user-activity-recorder.tsx</c>). The gate has to let a
+    /// caller polling at this rate through every single time.
+    /// </summary>
+    private const long ClientPollIntervalMs = 1000 * 60 * 15 + 8;
+
+    /// <summary>
+    /// Conservative lower bound on the points backend's own per-account minimum
+    /// spacing, which is a little under 15 minutes. The exact value belongs to
+    /// that service; the gate only needs to stay below it, so that anything it
+    /// absorbs is something the backend would have refused anyway.
+    /// </summary>
+    private const long BackendMinSpacingLowerBoundMs = 870_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 ACachedEntryOutlivesItsOwnWindow()
+    {
+        // If the entry expired first, the window would end early and silently.
+        Assert.True(CheckinGate.CacheTtlSeconds * 1000 >= CheckinGate.WindowMs);
+    }
+
+    [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));
+    }
+
+    [Fact]
+    public void AbsorbedRepeatsDoNotDisplaceASteadyPoller()
+    {
+        // 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.
+        const long extraSourceOffsetMs = 420_000;
+
+        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 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 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);
+    }
+}

dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs

@@ -81,72 +81,42 @@ 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)
-            {
-                vip = ctx.Connection.RemoteIpAddress?.ToString() ?? "";
-            }
-            if (vip.Length == 0)
-            {
-                vip = ctx.Request.Headers["x-forwarded-for"].ToString();
-            }
-            var identifier = vip;
+            // 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.
+            var key = CheckinGate.CacheKey(username);
 
             string? rec = null;
             try
             {
-                rec = MemCache.Get<string>(identifier);
+                rec = MemCache.Get<string>(key);
             }
             catch (Exception e)
             {
                 Console.Error.WriteLine(e);
                 Console.Error.WriteLine("Cache get failed.");
             }
 
-            if (!string.IsNullOrEmpty(rec))
+            var decision = CheckinGate.Decide(rec, DateTimeOffset.UtcNow.ToUnixTimeMilliseconds());
+
+            if (!decision.Forward)
+            {
+                // A repeat inside the window: ack it and drop it, storing nothing.
+                // Refreshing the window here would push it past this account's next
+                // scheduled check-in, which would then be absorbed as well. It has
+                // to stay anchored to the last check-in that reached the backend.
+                await ctx.SendJson(201, new JsonObject());
+                return;
+            }
+
+            try
             {
-                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;
-                }
+                MemCache.Set(key, decision.StampToStore!, CheckinGate.CacheTtlSeconds);
             }
-            else
+            catch (Exception e)
             {
-                try
-                {
-                    MemCache.Set(identifier,
-                        DateTimeOffset.UtcNow.ToUnixTimeMilliseconds().ToString(), 901);
-                }
-                catch (Exception e)
-                {
-                    Console.Error.WriteLine(e);
-                    Console.Error.WriteLine("Cache set failed.");
-                }
+                Console.Error.WriteLine(e);
+                Console.Error.WriteLine("Cache set failed.");
             }
         }
 

dotnet/EcencyApi/Infrastructure/CheckinGate.cs

@@ -0,0 +1,96 @@
+using System.Globalization;
+
+namespace EcencyApi.Infrastructure;
+
+/// <summary>
+/// De-duplication window for check-in activities (<c>ty</c> 10) on
+/// <c>/private-api/usr-activity</c>.
+///
+/// Clients poll check-in on a fixed interval a little over 15 minutes. The
+/// points backend enforces its own minimum spacing <em>per account</em> 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:
+///
+/// <list type="bullet">
+/// <item>The window is keyed on the <em>account</em>, 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.</item>
+/// <item>The window is fixed, not sliding. Only a forwarded check-in stores a
+/// timestamp. Refreshing it on an absorbed repeat pushes the window past the
+/// caller's next scheduled check-in, which is then absorbed too, leaving a
+/// steady poller with no way out.</item>
+/// <item>The window stays comfortably below both the client poll interval and
+/// the backend's own per-account spacing. At or near the poll interval, which
+/// of two consecutive polls survives comes down to arrival jitter; below the
+/// backend's spacing, an absorbed repeat is provably one the backend would have
+/// refused, so the gate can never cost an account a check-in.</item>
+/// </list>
+/// </summary>
+public static class CheckinGate
+{
+    /// <summary>
+    /// What the gate decided for one request. <see cref="StampToStore"/> is
+    /// non-null exactly when the request is forwarded, which is what keeps
+    /// "an absorbed repeat leaves the window alone" structural rather than a
+    /// rule a caller has to remember.
+    /// </summary>
+    public readonly record struct Decision(string? StampToStore)
+    {
+        public bool Forward => StampToStore != null;
+    }
+
+    /// <summary>
+    /// Decides one check-in against the account's last forwarded one.
+    /// </summary>
+    public static Decision Decide(string? recorded, long nowMs) =>
+        IsWithinWindow(recorded, nowMs) ? new Decision(null) : new Decision(Stamp(nowMs));
+
+    /// <summary>
+    /// How long after a forwarded check-in a repeat for the same account 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.
+    /// </summary>
+    public const long WindowMs = 780_000;
+
+    /// <summary>
+    /// Derived from the window so the two cannot drift apart: an entry that
+    /// outlives its window would only be read to conclude "expired" anyway.
+    /// </summary>
+    public const double CacheTtlSeconds = WindowMs / 1000d;
+
+    /// <summary>Cache key for an account's last forwarded check-in.</summary>
+    public static string CacheKey(string username) => "checkin:" + username;
+
+    /// <summary>Serializes a timestamp for the cache; inverse of the parse in
+    /// <see cref="IsWithinWindow"/>.</summary>
+    public static string Stamp(long nowMs) => nowMs.ToString(CultureInfo.InvariantCulture);
+
+    /// <summary>
+    /// True when <paramref name="recorded"/> is a timestamp this window still
+    /// covers. Anything unreadable, absent or in the future is false: the gate
+    /// fails open, because forwarding a repeat costs one upstream call the
+    /// backend discards, while absorbing a real check-in costs the account its
+    /// check-in.
+    /// </summary>
+    public static bool IsWithinWindow(string? recorded, long nowMs)
+    {
+        if (string.IsNullOrEmpty(recorded))
+        {
+            return false;
+        }
+
+        if (!double.TryParse(recorded, NumberStyles.Float, CultureInfo.InvariantCulture, out var recMs))
+        {
+            return false;
+        }
+
+        var age = nowMs - recMs;
+        return age >= 0 && age < WindowMs;
+    }
+}