From 9a250a0a24b61b171edfc22ca7a5f84fff1493b4 Mon Sep 17 00:00:00 2001 From: feruzm Date: Sat, 15 Aug 2026 15:56:55 +0000 Subject: [PATCH 1/2] fix(checkin): give the anchor back when the check-in never reached the backend The gate claims its anchor before the upstream call, which is what makes the burst race safe to close. The cost was that a check-in which never landed still held the window: on an upstream timeout or 5xx, another attempt for that account inside the window was absorbed with a 201 even though nothing had been recorded. Same shape as the bug this gate was just fixed for, with a narrower trigger. Pipe turns a transport failure into 504/500, so a 5xx on the response is exactly the "not delivered" set. An upstream 4xx is a deliberate rejection that a retry would not change, so it keeps the window. Release names the exact anchor it claimed and removes only that one, under the same stripe lock, so a stale release arriving after the account has checked in again cannot discard the live anchor. Not awaiting the upstream result to stamp only on success instead: that means hand-rolling the Express-compatible response path Pipe owns (invariant 3), and a 2xx from the backend does not mean the check-in was credited anyway, since its verifier decides that later. Closes #70 --- dotnet/EcencyApi.Tests/CheckinGateTests.cs | 35 +++++++++++++++++++ dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs | 15 ++++++++ .../EcencyApi/Infrastructure/CheckinGate.cs | 33 +++++++++++++++++ 3 files changed, 83 insertions(+) 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..5c690193 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 @@ -119,6 +123,17 @@ public static async Task Activities(HttpContext ctx) } 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. Pipe turns a transport failure into 504/500, so a + // 5xx here is exactly the "not delivered" set: an upstream 4xx is a + // deliberate rejection that a retry would not change. + if (reservedAnchor != null && 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..45a0ea0d 100644 --- a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs +++ b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs @@ -172,6 +172,39 @@ 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 (Exception e) + { + Console.Error.WriteLine(e); + Console.Error.WriteLine("Cache release failed."); + } + } + } + private const int StripeCount = 64; private static readonly object[] Stripes = From 5a877cbce2e58fa598988c7be210663c09fd7da9 Mon Sep 17 00:00:00 2001 From: feruzm Date: Sat, 15 Aug 2026 16:10:07 +0000 Subject: [PATCH 2/2] fix(checkin): release the anchor from a finally, and cover the never-started case Review found two ways the release could be skipped, both leaving a failed check-in holding the window, which is the failure this gate is being fixed for. Pipe can throw out of the write itself, so a release that sits after the await never runs when a client disconnects mid-response. It moves into a finally. ApiRequest builds the auth headers eagerly and throws on a misconfigured deployment, so the request can fail before Pipe is entered at all. The status code cannot report that, since nothing set it, so an explicit flag marks whether the upstream call ever started. A backend answer that only failed on the way back to a client that went away still keeps the anchor: the check-in landed, and SendLikeExpress sets the upstream status before it writes, so the status still reports that in the finally. Release is now silent as well as swallowing. It runs after the response is written, so an escaping exception would raise an error the client can no longer be told about, and a request handler should not be adding logging. --- dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs | 31 ++++++++++++++----- .../EcencyApi/Infrastructure/CheckinGate.cs | 14 ++++++--- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs index 5c690193..81517701 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs @@ -122,17 +122,34 @@ 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. Pipe turns a transport failure into 504/500, so a - // 5xx here is exactly the "not delivered" set: an upstream 4xx is a - // deliberate rejection that a retry would not change. - if (reservedAnchor != null && ctx.Response.StatusCode >= 500) + // that never landed. + var upstreamStarted = false; + try { - CheckinGate.Release(username, reservedAnchor); + // 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); + } } } diff --git a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs index 45a0ea0d..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. @@ -197,10 +198,13 @@ public static void Release(string username, string stamp) MemCache.Del(key); } } - catch (Exception e) + catch { - Console.Error.WriteLine(e); - Console.Error.WriteLine("Cache release failed."); + // 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. } } }