diff --git a/dotnet/EcencyApi.Tests/CheckinGateTests.cs b/dotnet/EcencyApi.Tests/CheckinGateTests.cs index 1402dadf..28376d80 100644 --- a/dotnet/EcencyApi.Tests/CheckinGateTests.cs +++ b/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() { diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs index 2697299a..81517701 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs +++ b/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(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) diff --git a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs index 98626e93..b832bddb 100644 --- a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs +++ b/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) } } + /// + /// 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 is removed, so this + /// can never discard one a later check-in established. + /// + public static void Release(string username, string stamp) + { + var key = CacheKey(username); + + lock (StripeFor(key)) + { + try + { + if (MemCache.Get(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 =