Release the check-in anchor when the check-in never reached the backend

dotnet/EcencyApi.Tests/CheckinGateTests.cs

@@ -260,6 +260,41 @@ public void AReservedAnchorAbsorbsTheNextCheckinAndThenReleases()
         Assert.True(CheckinGate.DecideAndReserve(username, nowMs + ClientPollIntervalMs).Forward);
     }
 
+    [Fact]
+    public void AnUndeliveredCheckinGivesTheAnchorBack()
+    {
+        // The anchor is claimed before the upstream call. If the check-in never
+        // landed, holding it would absorb the account's next attempt on the
+        // strength of one that never happened.
+        var username = "release-" + Guid.NewGuid().ToString("n");
+        var nowMs = 1_700_000_000_000;
+
+        var reserved = CheckinGate.DecideAndReserve(username, nowMs);
+        Assert.NotNull(reserved.StampToStore);
+
+        CheckinGate.Release(username, reserved.StampToStore!);
+
+        Assert.True(CheckinGate.DecideAndReserve(username, nowMs + 1).Forward);
+    }
+
+    [Fact]
+    public void AReleaseCannotDiscardALaterAccountsAnchor()
+    {
+        // A release names the exact anchor it claimed, so a stale one arriving
+        // after the account has checked in again is a no-op.
+        var username = "stale-" + Guid.NewGuid().ToString("n");
+        var nowMs = 1_700_000_000_000;
+
+        var stale = CheckinGate.DecideAndReserve(username, nowMs).StampToStore!;
+        var current = CheckinGate.DecideAndReserve(username, nowMs + ClientPollIntervalMs).StampToStore!;
+        Assert.NotEqual(stale, current);
+
+        CheckinGate.Release(username, stale);
+
+        // The live anchor survives, so the window it opened still holds.
+        Assert.False(CheckinGate.DecideAndReserve(username, nowMs + ClientPollIntervalMs + 1).Forward);
+    }
+
     [Fact]
     public void ABurstFromOneAccountStillCollapsesToOneUpstreamCall()
     {

dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs

@@ -79,6 +79,8 @@ public static async Task Activities(HttpContext ctx)
         // ty === 10 (strict: JSON number equal to 10)
         var tyIsTen = ty is JsonValue tyVal && tyVal.TryGetValue<double>(out var tyNum) && tyNum == 10;
 
+        string? reservedAnchor = null;
+
         if (tyIsTen)
         {
             // Keyed on the account, not the caller's address: see CheckinGate for why
@@ -96,6 +98,8 @@ public static async Task Activities(HttpContext ctx)
                 await ctx.SendJson(201, new JsonObject());
                 return;
             }
+
+            reservedAnchor = decision.StampToStore;
         }
 
         var pipeJson = new JsonObject
@@ -118,7 +122,35 @@ public static async Task Activities(HttpContext ctx)
             pipeJson["tx"] = tx!.DeepClone();
         }
 
-        await Upstream.Pipe(ApiClient.ApiRequest("usr-activity", HttpMethod.Post, null, pipeJson), ctx);
+        // The anchor is claimed before the call, which is what closes the burst
+        // race. If the check-in then never reached the backend, give it back
+        // rather than absorb this account's next attempt on the strength of one
+        // that never landed.
+        var upstreamStarted = false;
+        try
+        {
+            // ApiRequest builds the auth headers eagerly and throws on a
+            // misconfigured deployment, so the request can fail before Pipe is
+            // ever entered. That is a check-in the backend never saw.
+            var upstream = ApiClient.ApiRequest("usr-activity", HttpMethod.Post, null, pipeJson);
+            upstreamStarted = true;
+            await Upstream.Pipe(upstream, ctx);
+        }
+        finally
+        {
+            // Pipe maps a transport failure to 504/500, so a 5xx is the "never
+            // reached the backend" set, as is a request that never started. An
+            // upstream 4xx is a deliberate rejection that a retry would not
+            // change, so it keeps the anchor. So does a backend answer that only
+            // failed on the way back to a client that went away: the check-in
+            // landed, and SendLikeExpress sets the upstream status before it
+            // writes, so the status still reports that here. The release has to
+            // sit in a finally, because Pipe can throw out of the write itself.
+            if (reservedAnchor != null && (!upstreamStarted || ctx.Response.StatusCode >= 500))
+            {
+                CheckinGate.Release(username, reservedAnchor);
+            }
+        }
     }
 
     public static async Task SubscribeNewsletter(HttpContext ctx)

dotnet/EcencyApi/Infrastructure/CheckinGate.cs

@@ -126,8 +126,9 @@ public static Decision Decide(string? recorded, long nowMs)
     /// 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
+    /// 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.
@@ -172,6 +173,42 @@ public static Decision DecideAndReserve(string username, long nowMs)
         }
     }
 
+    /// <summary>
+    /// Gives up an anchor whose check-in never reached the backend.
+    ///
+    /// The anchor is reserved before the upstream call, because that is what
+    /// closes the burst race. If the call then fails to deliver, holding the
+    /// anchor would absorb the account's next attempt on the strength of a
+    /// check-in that never happened, which is the failure this whole gate is
+    /// being fixed for. Releasing puts the account back where it started.
+    ///
+    /// Only an anchor still holding <paramref name="stamp"/> is removed, so this
+    /// can never discard one a later check-in established.
+    /// </summary>
+    public static void Release(string username, string stamp)
+    {
+        var key = CacheKey(username);
+
+        lock (StripeFor(key))
+        {
+            try
+            {
+                if (MemCache.Get<string>(key) == stamp)
+                {
+                    MemCache.Del(key);
+                }
+            }
+            catch
+            {
+                // Deliberately silent as well as swallowed. This runs after the
+                // response has been written, so letting it escape would raise an
+                // error the client can no longer be told about. A cache throwing
+                // here is already saying so from the read and the write on the way
+                // in. A request handler should not be adding logging of its own.
+            }
+        }
+    }
+
     private const int StripeCount = 64;
 
     private static readonly object[] Stripes =