From 236e9d9c3731c487e6907d5877a3a10fa900d00e Mon Sep 17 00:00:00 2001 From: feruzm Date: Sat, 15 Aug 2026 15:37:40 +0000 Subject: [PATCH 1/3] fix(checkin): key the de-duplication window per account, anchored to the last forward The ty 10 gate on usr-activity was dropping legitimate check-ins, not only repeats, and invisibly so: it acks with 201, so the client is told the check-in landed. Two causes. The window was keyed on the caller's network address rather than the account, so accounts sharing one address competed for a single check-in slot. The address buys nothing here: a check-in carries a signed code, so a caller can only check in as an account it controls. The window was also refreshed on absorbed requests while its threshold equalled the client poll interval, leaving 8 ms of headroom. Which of two consecutive polls survived came down to arrival jitter, and once a caller was absorbed the window moved past its next scheduled check-in, so it stayed absorbed until its page reloaded. The window now keys on the account resolved from the signed code. It stays anchored to the last forwarded check-in and sits well below both the client poll interval and the backend's own per-account spacing, so it can only ever absorb a repeat the backend would have refused. The decision moves into CheckinGate, which makes "an absorbed repeat stores nothing" structural rather than a rule the handler has to remember, and makes the window semantics testable. Both defects are covered by tests that fail against the previous behaviour. Closes #68 --- dotnet/EcencyApi.Tests/CheckinGateTests.cs | 186 ++++++++++++++++++ dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs | 74 +++---- .../EcencyApi/Infrastructure/CheckinGate.cs | 96 +++++++++ 3 files changed, 304 insertions(+), 52 deletions(-) create mode 100644 dotnet/EcencyApi.Tests/CheckinGateTests.cs create mode 100644 dotnet/EcencyApi/Infrastructure/CheckinGate.cs diff --git a/dotnet/EcencyApi.Tests/CheckinGateTests.cs b/dotnet/EcencyApi.Tests/CheckinGateTests.cs new file mode 100644 index 00000000..a1752929 --- /dev/null +++ b/dotnet/EcencyApi.Tests/CheckinGateTests.cs @@ -0,0 +1,186 @@ +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; + + /// + /// 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. + /// + 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); + } +} diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs index 66363e08..2487bdcc 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs @@ -81,22 +81,15 @@ 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(identifier); + rec = MemCache.Get(key); } catch (Exception e) { @@ -104,49 +97,26 @@ public static async Task Activities(HttpContext ctx) 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."); } } diff --git a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs new file mode 100644 index 00000000..3ee7cfbe --- /dev/null +++ b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs @@ -0,0 +1,96 @@ +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 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. +/// 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. +/// +/// +public static class CheckinGate +{ + /// + /// What the gate decided for one request. 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. + /// + public readonly record struct Decision(string? StampToStore) + { + public bool Forward => StampToStore != null; + } + + /// + /// Decides one check-in against the account's last forwarded one. + /// + public static Decision Decide(string? recorded, long nowMs) => + IsWithinWindow(recorded, nowMs) ? new Decision(null) : new Decision(Stamp(nowMs)); + + /// + /// 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. + /// + public const long WindowMs = 780_000; + + /// + /// Derived from the window so the two cannot drift apart: an entry that + /// outlives its window would only be read to conclude "expired" anyway. + /// + public const double CacheTtlSeconds = WindowMs / 1000d; + + /// Cache key for an account's last forwarded check-in. + 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); + + /// + /// True when 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. + /// + 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; + } +} From deaf2f0a30cf8ee5941a27e844f9b5948d9dfe11 Mon Sep 17 00:00:00 2001 From: feruzm Date: Sat, 15 Aug 2026 15:45:41 +0000 Subject: [PATCH 2/3] fix(checkin): only anchor the window on a check-in the backend will credit Review found that the first commit still had a way to displace a steady poller. The window absorbed below 780s but re-anchored on every forward, so a second source arriving in the gap between that threshold and the backend's own per-account spacing was forwarded, refused upstream as too early, and still became the anchor. The caller's own poll ~100s later then landed inside a window that had moved out from under it and was dropped with a false 201. That is the original symptom in a different disguise. The gate now has two thresholds because two clocks matter: the client decides how often a check-in arrives, the backend decides how often one counts. Below WindowMs a repeat is absorbed. Between WindowMs and AnchorAfterMs it is forwarded but leaves the anchor alone. At or above AnchorAfterMs it is forwarded and becomes the new anchor. AnchorAfterMs has to be at least the backend's spacing; setting it too high only forwards once more than needed, which is the harmless direction. Decision carries Forward and StampToStore separately now that a forward does not always anchor. The steady-poller test runs over second-source offsets on both sides of the gap, and fails at 1 of 20 polls against the previous behaviour. --- dotnet/EcencyApi.Tests/CheckinGateTests.cs | 73 ++++++++++-- dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs | 24 ++-- .../EcencyApi/Infrastructure/CheckinGate.cs | 109 +++++++++++------- 3 files changed, 144 insertions(+), 62 deletions(-) diff --git a/dotnet/EcencyApi.Tests/CheckinGateTests.cs b/dotnet/EcencyApi.Tests/CheckinGateTests.cs index a1752929..b65f440b 100644 --- a/dotnet/EcencyApi.Tests/CheckinGateTests.cs +++ b/dotnet/EcencyApi.Tests/CheckinGateTests.cs @@ -19,13 +19,15 @@ public class CheckinGateTests private const long ClientPollIntervalMs = 1000 * 60 * 15 + 8; /// - /// 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. + /// 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() { @@ -45,11 +47,21 @@ public void TheWindowNeverOutlastsTheBackendsOwnSpacing() 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.WindowMs); + Assert.True(CheckinGate.CacheTtlSeconds * 1000 >= CheckinGate.AnchorAfterMs); } [Fact] @@ -106,8 +118,12 @@ public void AStampFromTheFutureFailsOpen() Assert.False(CheckinGate.IsWithinWindow(stamp, 1_000_000)); } - [Fact] - public void AbsorbedRepeatsDoNotDisplaceASteadyPoller() + [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 @@ -119,8 +135,6 @@ public void AbsorbedRepeatsDoNotDisplaceASteadyPoller() // 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; @@ -142,13 +156,52 @@ public void AbsorbedRepeatsDoNotDisplaceASteadyPoller() 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); + var decision = CheckinGate.Decide(stamp, (1_000_000 + CheckinGate.WindowMs) - 1); Assert.False(decision.Forward); Assert.Null(decision.StampToStore); diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs index 2487bdcc..eb48b766 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs @@ -102,21 +102,25 @@ public static async Task Activities(HttpContext ctx) 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. + // Moving the anchor 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; } - try + // Only a check-in the backend will credit becomes the new anchor; see + // CheckinGate for why a forwarded-but-refused attempt must not move it. + if (decision.StampToStore != null) { - MemCache.Set(key, decision.StampToStore!, CheckinGate.CacheTtlSeconds); - } - catch (Exception e) - { - Console.Error.WriteLine(e); - Console.Error.WriteLine("Cache set failed."); + try + { + MemCache.Set(key, decision.StampToStore, CheckinGate.CacheTtlSeconds); + } + catch (Exception e) + { + Console.Error.WriteLine(e); + Console.Error.WriteLine("Cache set failed."); + } } } diff --git a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs index 3ee7cfbe..644d4b93 100644 --- a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs +++ b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs @@ -16,81 +16,106 @@ namespace EcencyApi.Infrastructure; /// /// 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 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. -/// 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. +/// 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 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. + /// 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(string? StampToStore) - { - public bool Forward => StampToStore != null; - } + public readonly record struct Decision(bool Forward, string? StampToStore); /// - /// Decides one check-in against the account's last forwarded one. + /// 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 static Decision Decide(string? recorded, long nowMs) => - IsWithinWindow(recorded, nowMs) ? new Decision(null) : new Decision(Stamp(nowMs)); + public const long WindowMs = 780_000; /// - /// 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. + /// 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 WindowMs = 780_000; + public const long AnchorAfterMs = 900_000; /// - /// Derived from the window so the two cannot drift apart: an entry that - /// outlives its window would only be read to conclude "expired" anyway. + /// 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 = WindowMs / 1000d; + public const double CacheTtlSeconds = AnchorAfterMs / 1000d; - /// Cache key for an account's last forwarded check-in. + /// 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); /// - /// True when 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. + /// 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 bool IsWithinWindow(string? recorded, long nowMs) + public static double? AgeMs(string? recorded, long nowMs) { if (string.IsNullOrEmpty(recorded)) { - return false; + return null; } if (!double.TryParse(recorded, NumberStyles.Float, CultureInfo.InvariantCulture, out var recMs)) { - return false; + return null; } var age = nowMs - recMs; - return age >= 0 && age < WindowMs; + 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); } } From 9de68b7f3a2623ecfbb92e916230ac75d8fd92ad Mon Sep 17 00:00:00 2001 From: feruzm Date: Sat, 15 Aug 2026 15:49:23 +0000 Subject: [PATCH 3/3] fix(checkin): make the read, decision and reservation one step Review found the remaining hole: the gate read the anchor, decided, then wrote it back as three separate steps. The backing store is concurrent but the sequence was not, so check-ins arriving for one account in the same instant could all read an empty anchor and all go upstream, which is the burst the gate exists to collapse. Two tabs opening together is enough to hit it: both fire the ping their page load schedules. DecideAndReserve now does all three under a striped lock, so a concurrent duplicate behaves exactly like a sequential one. The loser reads the anchor the winner just wrote and is absorbed, which stays correct in the direction this gate cares about: a check-in milliseconds behind another is one the backend refuses regardless. Striped rather than one global lock so unrelated accounts do not queue behind each other, and nothing is held across an await. Verified by removing the lock: 32 simultaneous check-ins for one account forward 8 to 13 times instead of once. --- dotnet/EcencyApi.Tests/CheckinGateTests.cs | 44 +++++++++++++ dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs | 38 ++---------- .../EcencyApi/Infrastructure/CheckinGate.cs | 61 +++++++++++++++++++ 3 files changed, 111 insertions(+), 32 deletions(-) diff --git a/dotnet/EcencyApi.Tests/CheckinGateTests.cs b/dotnet/EcencyApi.Tests/CheckinGateTests.cs index b65f440b..1402dadf 100644 --- a/dotnet/EcencyApi.Tests/CheckinGateTests.cs +++ b/dotnet/EcencyApi.Tests/CheckinGateTests.cs @@ -216,6 +216,50 @@ public void AForwardedCheckinStoresItsOwnArrival() 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() { diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs index eb48b766..2697299a 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.Misc.cs @@ -83,45 +83,19 @@ public static async Task Activities(HttpContext ctx) { // 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(key); - } - catch (Exception e) - { - Console.Error.WriteLine(e); - Console.Error.WriteLine("Cache get failed."); - } - - var decision = CheckinGate.Decide(rec, DateTimeOffset.UtcNow.ToUnixTimeMilliseconds()); + // 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) { - // A repeat inside the window: ack it and drop it, storing nothing. - // Moving the anchor here would push it past this account's next + // 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; } - - // Only a check-in the backend will credit becomes the new anchor; see - // CheckinGate for why a forwarded-but-refused attempt must not move it. - if (decision.StampToStore != null) - { - try - { - MemCache.Set(key, decision.StampToStore, CheckinGate.CacheTtlSeconds); - } - catch (Exception e) - { - Console.Error.WriteLine(e); - Console.Error.WriteLine("Cache set failed."); - } - } } var pipeJson = new JsonObject diff --git a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs index 644d4b93..98626e93 100644 --- a/dotnet/EcencyApi/Infrastructure/CheckinGate.cs +++ b/dotnet/EcencyApi/Infrastructure/CheckinGate.cs @@ -118,4 +118,65 @@ public static Decision Decide(string? recorded, long nowMs) 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]; }