Conversation
|
@ejsmith could you review this specific cache-counter behavior before we simplify With Foundatio 13.0.4, .NET 10 on macOS arm64, five isolated runs of 10,000
This reproduction uses only an in-memory cache, without application services or network access. The decompiled implementation reads and mutates the same existing Exact reproductionCreate a .NET 10 console project with using Foundatio.Caching;
using var cache = new InMemoryCacheClient();
const int incrementCount = 10000;
for (int run = 0; run < 5; run++)
{
string key = $"counter-{run}";
await cache.SetAsync(key, 0L);
await Parallel.ForEachAsync(Enumerable.Range(0, incrementCount),
new ParallelOptions { MaxDegreeOfParallelism = 16 },
async (attempt, cancellationToken) => await cache.IncrementAsync(key, 1));
long actual = (await cache.GetAsync<long>(key)).Value;
Console.WriteLine($"Run {run + 1}: expected {incrementCount}, actual {actual}");
if (actual != incrementCount)
Environment.ExitCode = 1;
}This PR remains draft because simply replacing the markers with the current in-memory counter implementation would lose concurrent failures. The dependency should provide the atomic counter behavior so authentication can stay simple. |
|
Reviewing Roughly 80% of the additions are tests, and security regression coverage is justified. The concern is the production scope and the custom throttling machinery.
For the concurrency concern, a local reproduction using the actual Basic-auth handler with delayed repository responses admitted 100 requests before completion. It evaluated 99 incorrect passwords, and the final correct guess authenticated even after new requests were blocked. Admission needs to account for password checks already underway; the existing tests establish eventual blocking after failures complete. The Foundatio counter bug is real and was also reproduced locally, so simply replacing the markers with today's counters would be incorrect. Fix and test that underlying primitive separately, then use a small limiter with explicit handling of in-flight verification. Atomic counters alone do not solve the admission race. There is necessary complexity around concurrency, successful requests, and recovery. However, requiring successful authentication to perform no cache writes or distributed locking makes the design harder. That optimization should have a measured need, and successful requests should not consume failure quota. I recommend narrowing this PR to the limiter, its two authentication callers, recovery wiring, and targeted regression tests. That should make the change substantially smaller and easier to establish as correct. |
Centralize password-login checks and normalize credential parsing, token exchange, and outbound request validation. Add regression coverage for login, password recovery, concurrent requests, and integration handling.
3cd7d98 to
f3425cb
Compare
Centralize password-login handling in a shared
AuthService, align account-state checks across authentication paths, and normalize credential parsing, token exchange, outbound address validation, and webhook request handling. Add regression coverage for login, recovery, concurrency, and existing request formats.Validation: 110 focused auth tests passed on this commit; earlier runs passed 988 broader API/contract checks and 79 auth endpoint tests with local Redis caching. Changed C# files were formatted.
Review blocker: failure tracking still uses bounded markers. Simplifying it to counters depends on resolving the reproduced Foundatio 13.0.4 in-memory counter behavior described in the review comment. Window-specific cache keys also restart transient throttle history during rollout; mixed-version nodes use separate histories.
Verification and implementation details
Focused verification:
EX_StripeApiKey= EX_ExceptionlessServerUrl=http://localhost:7110 EX_ExceptionlessApiKey= dotnet test --project tests/Exceptionless.Tests --no-restore -- --filter-class Exceptionless.Tests.Services.AuthServiceTests --filter-class Exceptionless.Tests.Api.Endpoints.AuthEndpointTests