diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ec17cc1a..3042dc1fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,27 @@ follow [Semantic Versioning](https://semver.org). Ongoing work is collected unde and obscure what really happened. Line breaks are now stripped from these values before they are logged. +- **A REST API key is now confined to its own project on the project pages too.** The confinement + above was applied everywhere except the project endpoints themselves, which still judged a key + purely by who created it. Since keys are issued by an administrator, a key made for one project + could therefore list every project in the installation, open another project's details, and read + another project's **member email addresses**. Those three endpoints now apply the same rule as the + rest of the application: a key sees its own project and nothing else. + +- **A stored password can no longer end up in the log.** Whenever a user account was written to the + operator log — directly, or as part of something that mentions one, such as an API key's owner — + the scrambled form of that user's password went with it. It is scrambled rather than readable, so + nobody could have logged in with what was written, but it is exactly the material an attacker + wants for an offline guessing attack, and it should never leave the database. It is now masked + wherever an account is written out, the same way provider keys and mail passwords already were. + +- **The login session cookie no longer risks losing its "HTTPS only" marking.** The marking is + decided from which environment the application says it is running in, and that was worked out + differently in two places. Where an installation set both of the standard environment variables to + conflicting values, the two disagreed and the cookie could be sent without the marking on an + HTTPS installation, allowing it to travel over a plaintext connection. Both places now agree, and + environment-specific settings files are honoured everywhere. + ### Changed - **Optimization proposals are now held to a stricter, correct standard of proof.** Two flaws made @@ -70,6 +91,58 @@ follow [Semantic Versioning](https://semver.org). Ongoing work is collected unde ### Fixed +- **A stalled provider no longer ties up the proxy indefinitely.** The five-minute limit on an + upstream call stopped applying as soon as the provider sent its response headers, so a provider + that answered and then went quiet mid-reply held the request, its connection and its worker open + until the calling application itself gave up — which, for a patient client, was never. Enough of + these and the proxy runs out of capacity. The limit now covers the whole reply, and a provider + that stalls is reported as a gateway timeout and recorded as one on the trace. + +- **A price-feed outage no longer makes refreshing a provider's models appear to hang.** Model + prices come from a public price list and a public exchange-rate feed, and a provider's models were + priced one at a time. If either feed was unreachable, every single model triggered its own full + retry, one after another — so a provider offering hundreds of models turned one refresh into + hundreds of doomed attempts against an already-failing address, and the refresh looked frozen. A + failed lookup is now remembered briefly for both feeds, so an outage costs one attempt instead of + hundreds, while a brief blip still recovers on the next refresh. + +- **Someone who belongs to several projects now gets a traces overview without naming one.** Asking + for the traces overview without specifying a project returned an empty overview for anyone who is + not an administrator and belongs to more than one project — the figures were only ever computed + for a single project at a time. The overview, and the evaluator sparklines beside it, are now + aggregated across every project the caller may see. The web app was never affected, because it + always names the current project. + +- **Closing a page with live updates no longer files an entry in the Error Log.** Navigating away + from — or simply closing — a view that streams live updates was recorded as an application error, + complete with an error reference that pointed at nothing an operator could act on. Ordinary + disconnects are now recognized as such and left out of the log; genuine faults are recorded exactly + as before. + +- **A test run that fails to hand off its results no longer reports itself finished twice.** If + something went wrong in the moment after a run group finished, the group announced its completion a + second time and was analysed for anomalies a second time — so a real anomaly could be flagged and + notified twice, while the log claimed the run had failed even though it is shown as completed. A + group now announces itself and queues its follow-up work exactly once, and a failure to queue one + of the two follow-up jobs no longer costs it the other. + +- **A test run that is cancelled just as it finishes no longer skips its follow-up analysis.** If a + cancellation arrived in the instant a run group was completing, the group was reported as + Completed but neither the optimizer nor anomaly detection ever looked at it — so that run silently + produced no improvement proposals and no anomaly flags, with nothing to show anything had been + missed. Once a group has finished, its follow-up work now always runs. + +- **A response that fails halfway through is no longer delivered as if it were complete.** When an + error struck after the server had already begun sending a reply, the connection was closed + tidily, so the caller received a well-formed but cut-off answer and had no way to tell it apart + from a genuinely short one — a truncated result could be consumed as real data. Such a failure now + breaks the connection, which callers and client libraries report as an error. + +- **An Azure OpenAI endpoint written in its fully-qualified form is now recognized.** A host name + ending in a dot — `resource.openai.azure.com.`, a legal way to write an absolute name — was not + detected as Azure, so model discovery used the wrong route and the Azure credential header was + left off, showing up as an empty model list and rejected calls with nothing pointing at the cause. + - **A REST API key now sees its own project's data without having to name the project.** List endpoints take an optional project filter, and leaving it out was meant to mean "everything I am allowed to see". For anyone who is not an administrator it instead meant *nothing*: the request diff --git a/Proxytrace.Api.Tests/AgentCallsControllerTests.cs b/Proxytrace.Api.Tests/AgentCallsControllerTests.cs index 655a84532..eea86a41a 100644 --- a/Proxytrace.Api.Tests/AgentCallsControllerTests.cs +++ b/Proxytrace.Api.Tests/AgentCallsControllerTests.cs @@ -210,6 +210,63 @@ public async Task GetAll_AsNonAdminInSeveralProjectsWithoutFilter_ReturnsTheUnio result.Total.Should().Be(2); } + [TestMethod] + public async Task GetOverview_AsNonAdminInSeveralProjectsWithoutFilter_AggregatesTheUnion() + { + // #483: the overview's aggregates go through StatisticsFilter, which used to carry a single + // project id (partly applied in raw SQL). A caller who may read several projects and named + // none therefore got an empty overview instead of an aggregate over their own projects. + IServiceProvider services = GetServices(); + var first = await SeedAgentInNewProjectAsync(services, "first"); + var second = await SeedAgentInNewProjectAsync(services, "second"); + var outsider = await SeedAgentInNewProjectAsync(services, "outsider"); + await SeedCallWithToolsAsync(services, first, []); + await SeedCallWithToolsAsync(services, second, []); + await SeedCallWithToolsAsync(services, outsider, []); + + var controller = ResolveController(services, ScopedGuard(first.Project.Id, second.Project.Id)); + var overview = await controller.GetOverview(cancellationToken: CancellationToken); + + overview.AgentBreakdown.Select(b => b.AgentId).Should().BeEquivalentTo([first.Id, second.Id]); + overview.Agents.Select(a => a.Id).Should().BeEquivalentTo([first.Id, second.Id]); + // The latency percentiles are the raw-SQL path in production; the third project's call must + // not be in the sample either. + overview.Latency.Sum(l => l.SampleCount).Should().Be(2); + } + + [TestMethod] + public async Task GetOverview_AsNonAdminInOneProjectWithoutFilter_AggregatesThatProjectOnly() + { + // The single-project scope (the web UI, and every REST API key — confined to one project) + // keeps going through the filter's by-one-project branch, unchanged by #483. + IServiceProvider services = GetServices(); + var mine = await SeedAgentInNewProjectAsync(services, "mine"); + var theirs = await SeedAgentInNewProjectAsync(services, "theirs"); + await SeedCallWithToolsAsync(services, mine, []); + await SeedCallWithToolsAsync(services, theirs, []); + + var controller = ResolveController(services, ScopedGuard(mine.Project.Id)); + var overview = await controller.GetOverview(cancellationToken: CancellationToken); + + overview.AgentBreakdown.Should().ContainSingle().Which.AgentId.Should().Be(mine.Id); + overview.Agents.Select(a => a.Id).Should().Equal(mine.Id); + } + + [TestMethod] + public async Task GetOverview_AsNonMember_ReturnsEmptyWithoutQuerying() + { + IServiceProvider services = GetServices(); + var agent = await SeedAgentInNewProjectAsync(services, "theirs"); + await SeedCallWithToolsAsync(services, agent, []); + + var controller = ResolveController(services, DenyingGuard()); + var overview = await controller.GetOverview(cancellationToken: CancellationToken); + + overview.Agents.Should().BeEmpty(); + overview.AgentBreakdown.Should().BeEmpty(); + overview.Latency.Should().BeEmpty(); + } + /// /// An agent in a project of its own, so a test can tell two tenants' rows apart. /// diff --git a/Proxytrace.Api.Tests/Config/HostEnvironmentNameTests.cs b/Proxytrace.Api.Tests/Config/HostEnvironmentNameTests.cs new file mode 100644 index 000000000..f293d5d74 --- /dev/null +++ b/Proxytrace.Api.Tests/Config/HostEnvironmentNameTests.cs @@ -0,0 +1,63 @@ +using AwesomeAssertions; +using Proxytrace.Api.Configuration; + +namespace Proxytrace.Api.Tests.Config; + +/// +/// Pins the environment-name resolution the container module shares with the host. The precedence +/// is the whole point: WebApplicationBuilder lets DOTNET_ENVIRONMENT win over +/// ASPNETCORE_ENVIRONMENT, and reading them the other way round defaulted the session +/// cookie's Secure attribute to false on an HTTPS install. +/// +[TestClass] +public sealed class HostEnvironmentNameTests +{ + private static Func Environment(string? dotnet = null, string? aspNetCore = null) => + name => name switch + { + "DOTNET_ENVIRONMENT" => dotnet, + "ASPNETCORE_ENVIRONMENT" => aspNetCore, + _ => null, + }; + + [TestMethod] + public void Resolve_WithNeitherVariableSet_IsProduction() => + HostEnvironmentName.Resolve(Environment()).Should().Be("Production"); + + [TestMethod] + public void Resolve_WithOnlyAspNetCoreSet_UsesIt() => + HostEnvironmentName.Resolve(Environment(aspNetCore: "Staging")).Should().Be("Staging"); + + [TestMethod] + public void Resolve_WithOnlyDotnetSet_UsesIt() => + HostEnvironmentName.Resolve(Environment(dotnet: "Staging")).Should().Be("Staging"); + + [TestMethod] + public void Resolve_WhenBothSetAndDisagree_PrefersDotnetLikeTheHost() + { + HostEnvironmentName.Resolve(Environment(dotnet: "Production", aspNetCore: "Development")) + .Should().Be("Production"); + + HostEnvironmentName.Resolve(Environment(dotnet: "Development", aspNetCore: "Production")) + .Should().Be("Development"); + } + + [TestMethod] + public void Resolve_WithBlankValues_TreatsThemAsUnset() + { + HostEnvironmentName.Resolve(Environment(dotnet: "", aspNetCore: "Development")) + .Should().Be("Development"); + + HostEnvironmentName.Resolve(Environment(dotnet: " ", aspNetCore: " ")) + .Should().Be("Production"); + } + + [TestMethod] + public void IsDevelopment_MatchesCaseInsensitively() + { + HostEnvironmentName.IsDevelopment("development").Should().BeTrue(); + HostEnvironmentName.IsDevelopment("Development").Should().BeTrue(); + HostEnvironmentName.IsDevelopment("Production").Should().BeFalse(); + HostEnvironmentName.IsDevelopment("Staging").Should().BeFalse(); + } +} diff --git a/Proxytrace.Api.Tests/Middleware/ExceptionHandlingMiddlewareTests.cs b/Proxytrace.Api.Tests/Middleware/ExceptionHandlingMiddlewareTests.cs index d992db3fc..2906992c6 100644 --- a/Proxytrace.Api.Tests/Middleware/ExceptionHandlingMiddlewareTests.cs +++ b/Proxytrace.Api.Tests/Middleware/ExceptionHandlingMiddlewareTests.cs @@ -3,7 +3,9 @@ using AwesomeAssertions; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using Proxytrace.Api.Middleware; @@ -18,7 +20,10 @@ namespace Proxytrace.Api.Tests.Middleware; [TestClass] public sealed class ExceptionHandlingMiddlewareTests { - private static ExceptionHandlingMiddleware Create(RequestDelegate next, bool isDevelopment = false) + private static ExceptionHandlingMiddleware Create( + RequestDelegate next, + bool isDevelopment = false, + ILogger? logger = null) { var env = Substitute.For(); env.EnvironmentName.Returns(isDevelopment ? "Development" : "Production"); @@ -35,7 +40,7 @@ private static ExceptionHandlingMiddleware Create(RequestDelegate next, bool isD ]; return new ExceptionHandlingMiddleware( - next, NullLogger.Instance, mappers, env); + next, logger ?? NullLogger.Instance, mappers, env); } private static async Task<(int Status, string Body)> InvokeAsync(ExceptionHandlingMiddleware middleware) @@ -50,6 +55,32 @@ private static ExceptionHandlingMiddleware Create(RequestDelegate next, bool isD return (ctx.Response.StatusCode, await reader.ReadToEndAsync()); } + /// + /// Runs the middleware against a context whose response has already started, and hands back the + /// lifetime feature so the test can observe whether the connection was reset, plus the recorded + /// log levels so it can observe whether the fault was captured into the Error Log. + /// + private static async Task<(IHttpRequestLifetimeFeature Lifetime, RecordingLogger Log)> + InvokeAfterResponseStartedAsync(Exception thrown, bool clientDisconnected = false) + { + var response = Substitute.For(); + response.HasStarted.Returns(true); + response.Headers.Returns(new HeaderDictionary()); + + var lifetime = Substitute.For(); + lifetime.RequestAborted.Returns( + clientDisconnected ? new CancellationToken(canceled: true) : CancellationToken.None); + + var ctx = new DefaultHttpContext(); + ctx.Features.Set(response); + ctx.Features.Set(lifetime); + ctx.Request.Path = "/api/agent-calls/stream"; + + var log = new RecordingLogger(); + await Create(_ => throw thrown, logger: log).InvokeAsync(ctx); + return (lifetime, log); + } + private static JsonElement Error(string body) { using var doc = JsonDocument.Parse(body); @@ -225,6 +256,93 @@ await FluentActions .Should().ThrowAsync(); } + [TestMethod] + public async Task InvokeAsync_ExceptionAfterResponseStarted_AbortsTheConnection() + { + // Returning normally would let the framework close the response cleanly, so the client would + // read a truncated body as a complete one. The reset is the only remaining failure signal. + var (lifetime, _) = await InvokeAfterResponseStartedAsync(new InvalidOperationException("boom")); + + lifetime.Received(1).Abort(); + } + + [TestMethod] + public async Task InvokeAsync_ExceptionAfterResponseStarted_CapturesAnError() + { + // A genuine mid-stream fault on a live connection keeps its Error-level capture, so the + // Error Log row (and the errorId that deep-links to it) still exists for an operator. + var (_, log) = await InvokeAfterResponseStartedAsync(new InvalidOperationException("boom")); + + log.Levels.Should().Contain(LogLevel.Error); + } + + [TestMethod] + public async Task InvokeAsync_ExceptionAfterResponseStarted_DoesNotRethrow() + { + await FluentActions + .Invoking(() => InvokeAfterResponseStartedAsync(new InvalidOperationException("boom"))) + .Should().NotThrowAsync(); + } + + [TestMethod] + public async Task InvokeAsync_ExceptionAfterClientDisconnected_DoesNotAbort() + { + // The connection is already gone — resetting it again would only add noise. + var (lifetime, _) = await InvokeAfterResponseStartedAsync( + new IOException("connection reset"), clientDisconnected: true); + + lifetime.DidNotReceive().Abort(); + } + + [TestMethod] + public async Task InvokeAsync_ExceptionAfterClientDisconnected_DoesNotCaptureAnError() + { + // Every Error/Critical entry becomes an ApplicationError row, so an Error-level log here + // would put one Error Log entry in front of operators per closed browser tab — for a + // routine disconnect nobody can act on. + var (_, log) = await InvokeAfterResponseStartedAsync( + new IOException("connection reset"), clientDisconnected: true); + + log.Levels.Should().NotContain(LogLevel.Error); + } + + [TestMethod] + public async Task InvokeAsync_ExceptionAfterClientDisconnected_LogsOnceAtDebug() + { + var (_, log) = await InvokeAfterResponseStartedAsync( + new IOException("connection reset"), clientDisconnected: true); + + log.Levels.Should().ContainSingle().Which.Should().Be(LogLevel.Debug); + } + + /// + /// Records the level of every entry the middleware writes. Only Error/Critical entries are + /// picked up by the error-log capture pipeline, so the recorded levels tell a test whether a + /// fault was captured as an ApplicationError row. + /// + private sealed class RecordingLogger : ILogger + { + public List Levels { get; } = []; + + public IDisposable BeginScope(TState state) where TState : notnull => NullScope.Instance; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + => Levels.Add(logLevel); + + private sealed class NullScope : IDisposable + { + public static readonly NullScope Instance = new(); + + public void Dispose() + { + } + } + } + private sealed class FakeDbException : DbException { private readonly string sqlState; diff --git a/Proxytrace.Api.Tests/ProjectsControllerTests.cs b/Proxytrace.Api.Tests/ProjectsControllerTests.cs index a7599583a..ba23fe83a 100644 --- a/Proxytrace.Api.Tests/ProjectsControllerTests.cs +++ b/Proxytrace.Api.Tests/ProjectsControllerTests.cs @@ -1,11 +1,11 @@ using Proxytrace.Domain.AuditLog; -using System.Security.Claims; -using Autofac; using AwesomeAssertions; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.DependencyInjection; using NSubstitute; +using Proxytrace.Api.Auth; +using Proxytrace.Api.Auth.Rest; using Proxytrace.Api.Controllers; using Proxytrace.Api.Dto.Projects; using Proxytrace.Application.Auth; @@ -124,17 +124,12 @@ public async Task Update_RenamesProject_ButLeavesMembershipUnchanged() [TestMethod] public async Task GetAll_AsNonAdmin_ReturnsOnlyMemberProjects() { - ICurrentUserAccessor accessor = null!; - IServiceProvider services = GetServices(builder => accessor = RegisterAccessor(builder)); - var endpoint = await services.GetRequiredService>().GetOrCreateAsync(CancellationToken); - var user = await services.GetRequiredService>().CreateAsync(CancellationToken); - var createNew = services.GetRequiredService(); - var repo = services.GetRequiredService(); - var mine = await repo.AddAsync(createNew("Mine", endpoint, [user]), CancellationToken); - await repo.AddAsync(createNew("Theirs", endpoint, []), CancellationToken); - accessor.GetCurrentUserAsync(Arg.Any()).Returns(user); + IServiceProvider services = GetServices(); + var user = await CreateUserAsync(services, UserRole.Member); + var mine = await ProjectWithMembersAsync(services, user); + await ProjectWithMembersAsync(services); // someone else's project - var controller = ResolveController(services, ContextWithRoles()); + var controller = ResolveController(services, NewGuard(services, user)); var result = await controller.GetAll(cancellationToken: CancellationToken); result.Items.Should().ContainSingle().Which.Id.Should().Be(mine.Id); @@ -144,61 +139,113 @@ public async Task GetAll_AsNonAdmin_ReturnsOnlyMemberProjects() public async Task GetAll_AsAdmin_ReturnsAllProjects() { IServiceProvider services = GetServices(); - var endpoint = await services.GetRequiredService>().GetOrCreateAsync(CancellationToken); - var createNew = services.GetRequiredService(); - var repo = services.GetRequiredService(); - await repo.AddAsync(createNew("A", endpoint, []), CancellationToken); - await repo.AddAsync(createNew("B", endpoint, []), CancellationToken); + var admin = await CreateUserAsync(services, UserRole.Admin); + await ProjectWithMembersAsync(services); + await ProjectWithMembersAsync(services); - var controller = ResolveController(services, ContextWithRoles(nameof(UserRole.Admin))); + var controller = ResolveController(services, NewGuard(services, admin)); var result = await controller.GetAll(cancellationToken: CancellationToken); result.Items.Should().HaveCount(2); } + [TestMethod] + public async Task GetAll_WithApiKeyScopedToOneProject_ReturnsOnlyThatProject() + { + // #474: the listing used to be driven by an inline role/membership check that never saw the + // key's project, so a key minted for A listed every project its (admin) owner could reach. + IServiceProvider services = GetServices(); + var owner = await CreateUserAsync(services, UserRole.Admin); + var projectA = await ProjectWithMembersAsync(services); + var projectB = await ProjectWithMembersAsync(services); + + var controller = ResolveController(services, NewGuard(services, owner, apiKeyProjectId: projectA.Id)); + var result = await controller.GetAll(cancellationToken: CancellationToken); + + result.Items.Should().ContainSingle().Which.Id.Should().Be(projectA.Id); + result.Total.Should().Be(1); + result.Items.Should().NotContain(p => p.Id == projectB.Id); + } + [TestMethod] public async Task Get_AsNonMember_ReturnsNotFound() { - ICurrentUserAccessor accessor = null!; - IServiceProvider services = GetServices(builder => accessor = RegisterAccessor(builder)); - var outsider = await services.GetRequiredService>().CreateAsync(CancellationToken); - var project = await services.GetRequiredService>().CreateAsync(CancellationToken); - accessor.GetCurrentUserAsync(Arg.Any()).Returns(outsider); + IServiceProvider services = GetServices(); + var outsider = await CreateUserAsync(services, UserRole.Member); + var project = await ProjectWithMembersAsync(services); - var controller = ResolveController(services, ContextWithRoles()); + var controller = ResolveController(services, NewGuard(services, outsider)); var result = await controller.Get(project.Id, CancellationToken); result.Result.Should().BeOfType(); } + [TestMethod] + public async Task Get_WithApiKeyScopedToAnotherProject_ReturnsNotFound() + { + // A key minted for A must not read B's detail, even though its admin owner could. + IServiceProvider services = GetServices(); + var owner = await CreateUserAsync(services, UserRole.Admin); + var projectA = await ProjectWithMembersAsync(services); + var projectB = await ProjectWithMembersAsync(services); + + var controller = ResolveController(services, NewGuard(services, owner, apiKeyProjectId: projectA.Id)); + var result = await controller.Get(projectB.Id, CancellationToken); + + result.Result.Should().BeOfType(); + } + + [TestMethod] + public async Task Get_WithApiKeyScopedToThatProject_ReturnsDto() + { + IServiceProvider services = GetServices(); + var owner = await CreateUserAsync(services, UserRole.Admin); + var projectA = await ProjectWithMembersAsync(services); + + var controller = ResolveController(services, NewGuard(services, owner, apiKeyProjectId: projectA.Id)); + var result = await controller.Get(projectA.Id, CancellationToken); + + result.Value.Should().NotBeNull(); + result.Value.Id.Should().Be(projectA.Id); + } + [TestMethod] public async Task GetMembers_AsNonMember_ReturnsNotFound() { - ICurrentUserAccessor accessor = null!; - IServiceProvider services = GetServices(builder => accessor = RegisterAccessor(builder)); - var outsider = await services.GetRequiredService>().CreateAsync(CancellationToken); - var project = await services.GetRequiredService>().CreateAsync(CancellationToken); - accessor.GetCurrentUserAsync(Arg.Any()).Returns(outsider); + IServiceProvider services = GetServices(); + var outsider = await CreateUserAsync(services, UserRole.Member); + var project = await ProjectWithMembersAsync(services); - var controller = ResolveController(services, ContextWithRoles()); + var controller = ResolveController(services, NewGuard(services, outsider)); var result = await controller.GetMembers(project.Id, CancellationToken); result.Result.Should().BeOfType(); } + [TestMethod] + public async Task GetMembers_WithApiKeyScopedToAnotherProject_ReturnsNotFound() + { + // Member emails are PII: a key minted for A must not enumerate B's members. + IServiceProvider services = GetServices(); + var owner = await CreateUserAsync(services, UserRole.Admin); + var member = await CreateUserAsync(services, UserRole.Member); + var projectA = await ProjectWithMembersAsync(services); + var projectB = await ProjectWithMembersAsync(services, member); + + var controller = ResolveController(services, NewGuard(services, owner, apiKeyProjectId: projectA.Id)); + var result = await controller.GetMembers(projectB.Id, CancellationToken); + + result.Result.Should().BeOfType(); + } + [TestMethod] public async Task GetMembers_AsMember_ReturnsMembers() { - ICurrentUserAccessor accessor = null!; - IServiceProvider services = GetServices(builder => accessor = RegisterAccessor(builder)); - var endpoint = await services.GetRequiredService>().GetOrCreateAsync(CancellationToken); - var user = await services.GetRequiredService>().CreateAsync(CancellationToken); - var createNew = services.GetRequiredService(); - var repo = services.GetRequiredService(); - var project = await repo.AddAsync(createNew("Mine", endpoint, [user]), CancellationToken); - accessor.GetCurrentUserAsync(Arg.Any()).Returns(user); + IServiceProvider services = GetServices(); + var user = await CreateUserAsync(services, UserRole.Member); + var project = await ProjectWithMembersAsync(services, user); - var controller = ResolveController(services, ContextWithRoles()); + var controller = ResolveController(services, NewGuard(services, user)); var result = await controller.GetMembers(project.Id, CancellationToken); result.Value.Should().ContainSingle(m => m.Id == user.Id); @@ -248,10 +295,15 @@ public async Task Delete_UnknownProject_ReturnsNotFound() result.Should().BeOfType(); } - private static ProjectsController ResolveController(IServiceProvider services, ControllerContext? context = null) - { - var controller = new ProjectsController( - services.GetRequiredService(), + /// + /// Builds the controller. Without an explicit the permissive + /// stub from the test module is used, so tests that do not care about tenant scoping stay + /// unaffected; the access tests pass a real guard built by . + /// + private static ProjectsController ResolveController( + IServiceProvider services, + IProjectAccessGuard? accessGuard = null) => + new(services.GetRequiredService(), services.GetRequiredService>(), services.GetRequiredService>(), services.GetRequiredService(), @@ -259,25 +311,45 @@ private static ProjectsController ResolveController(IServiceProvider services, C services.GetRequiredService(), services.GetRequiredService(), services.GetRequiredService(), - services.GetRequiredService(), + accessGuard ?? services.GetRequiredService(), Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance); - if (context is not null) - controller.ControllerContext = context; - return controller; - } - private static ICurrentUserAccessor RegisterAccessor(ContainerBuilder builder) + /// + /// The real guard, wired to a given caller and — optionally — to a REST API key confined to one + /// project, exactly as ApiKeyAuthenticationHandler marks the request. + /// + private static ProjectAccessGuard NewGuard( + IServiceProvider services, + IUser? currentUser, + Guid? apiKeyProjectId = null) { var accessor = Substitute.For(); - builder.RegisterInstance(accessor).As(); - return accessor; + accessor.GetCurrentUserAsync(Arg.Any()).Returns(currentUser); + + var httpContextAccessor = new HttpContextAccessor { HttpContext = new DefaultHttpContext() }; + if (apiKeyProjectId is { } projectId) + httpContextAccessor.HttpContext.Items[ApiKeyAuthenticationHandler.ProjectIdItemKey] = projectId; + + return new ProjectAccessGuard( + accessor, + services.GetRequiredService(), + httpContextAccessor); } - private static ControllerContext ContextWithRoles(params string[] roles) + private async Task CreateUserAsync(IServiceProvider services, UserRole role) { - var identity = new ClaimsIdentity(roles.Select(r => new Claim(ClaimTypes.Role, r)), "test"); - var http = new DefaultHttpContext { User = new ClaimsPrincipal(identity) }; - return new ControllerContext { HttpContext = http }; + // The IUser generator picks a random role, which would make a role-sensitive test flaky. + var create = services.GetRequiredService(); + var user = create($"{Guid.NewGuid():N}@example.test", externalSubject: null, passwordHash: "hash", role); + return await services.GetRequiredService>().AddAsync(user, CancellationToken); + } + + private async Task ProjectWithMembersAsync(IServiceProvider services, params IUser[] members) + { + var endpoint = await services.GetRequiredService>().GetOrCreateAsync(CancellationToken); + var createNew = services.GetRequiredService(); + var project = createNew($"P-{Guid.NewGuid():N}", endpoint, members); + return await services.GetRequiredService().AddAsync(project, CancellationToken); } private async Task<(IProject project, IUser user)> SeedProjectAndUserAsync(IServiceProvider services) diff --git a/Proxytrace.Api/Configuration/HostEnvironmentName.cs b/Proxytrace.Api/Configuration/HostEnvironmentName.cs new file mode 100644 index 000000000..c9b59b325 --- /dev/null +++ b/Proxytrace.Api/Configuration/HostEnvironmentName.cs @@ -0,0 +1,66 @@ +namespace Proxytrace.Api.Configuration; + +/// +/// Resolves the environment name the host is running under, the way the host itself resolves +/// it. +/// +/// +/// builds its own view (the host's +/// does not read appsettings.local.json, which holds the generated signing key — see +/// Program.cs), and that second view has to agree with the host about which environment this +/// is: it decides which appsettings.{Environment}.json is layered in, and the session +/// cookie's Secure default is derived from it. +/// +/// +/// +/// The order matters and is not the intuitive one: WebApplicationBuilder lets +/// DOTNET_ENVIRONMENT win over ASPNETCORE_ENVIRONMENT when both are set and disagree +/// (verified on .NET 10). Reading them the other way round meant the host ran Production while the +/// module computed Development, defaulting the 7-day session cookie's Secure attribute to +/// false on an HTTPS install. +/// +/// +/// +/// Read from the environment rather than from configuration: the host bootstraps its environment +/// from environment variables (and the command line) before any appsettings*.json is layered +/// in, so an ASPNETCORE_ENVIRONMENT key inside a JSON file never moves the host and must not +/// move this either. +/// +/// +internal static class HostEnvironmentName +{ + public const string Production = "Production"; + public const string Development = "Development"; + + /// + /// The environment name from the ambient process environment, defaulting to + /// . + /// + public static string Resolve() => Resolve(Environment.GetEnvironmentVariable); + + /// + /// Testable overload: stands in for + /// . + /// + /// + /// A variable set to an empty or whitespace value counts as unset. A container orchestrator + /// that passes ASPNETCORE_ENVIRONMENT= would otherwise name a nonexistent + /// appsettings..json; the host reaches the same conclusion for the only decision that + /// depends on it here, since an empty name is not either way. + /// + public static string Resolve(Func readEnvironmentVariable) + { + ArgumentNullException.ThrowIfNull(readEnvironmentVariable); + + return NullIfBlank(readEnvironmentVariable("DOTNET_ENVIRONMENT")) + ?? NullIfBlank(readEnvironmentVariable("ASPNETCORE_ENVIRONMENT")) + ?? Production; + } + + /// True when names the Development environment. + public static bool IsDevelopment(string environmentName) => + string.Equals(environmentName, Development, StringComparison.OrdinalIgnoreCase); + + private static string? NullIfBlank(string? value) => + string.IsNullOrWhiteSpace(value) ? null : value; +} diff --git a/Proxytrace.Api/Controllers/AgentCallsController.cs b/Proxytrace.Api/Controllers/AgentCallsController.cs index 14b36f3cf..38272ef82 100644 --- a/Proxytrace.Api/Controllers/AgentCallsController.cs +++ b/Proxytrace.Api/Controllers/AgentCallsController.cs @@ -82,6 +82,22 @@ public AgentCallsController( return agent is not null && scope.Contains(agent.Project.Id) ? scope : []; } + // The agents the overview lists: one project's when the scope names one (the indexed load), + // the union of the caller's projects when it spans several, and every agent for an unrestricted + // admin. Mirrors EvaluatorsController.ListScopedAsync — the agents table is small and bounded by + // the licensed agent limit, so narrowing a multi-project scope in memory is cheap. + private async Task> ScopedAgentsAsync( + IReadOnlyCollection? scope, + CancellationToken cancellationToken) + { + if (scope.IsEmpty()) + return []; + if (scope.SingleProject() is { } projectId) + return await agentRepository.GetByProjectAsync(projectId, cancellationToken); + var all = await agentRepository.GetAllAsync(cancellationToken); + return scope is null ? all : all.Where(a => scope.Contains(a.Project.Id)).ToArray(); + } + // Truncate a caller-supplied session key and pair it with its derived id, so the seed endpoint // carries both as one value and never has to re-check the key for null. private static (Guid Id, string Key) DeriveSession(Guid projectId, string sessionKey) @@ -210,23 +226,15 @@ public async Task GetOverview( if (scope.IsEmpty()) return new TracesOverviewDto([], [], []); - // Unlike the trace lists, the aggregates below go through StatisticsFilter, which filters by - // a single project (partly in raw SQL). A scope spanning several projects therefore has - // nothing it can safely aggregate over — running unscoped would mix in other tenants' rows — - // so those callers still get an empty overview. Every single-project caller, including an - // unfiltered REST API key, is served. Tracked in #483. - var scopedProjectId = scope.SingleProject(); - if (scope is not null && scopedProjectId is null) - return new TracesOverviewDto([], [], []); - - var latencyFilter = new StatisticsFilter(from, null, scopedProjectId, agentId); - var breakdownFilter = new StatisticsFilter(from, null, scopedProjectId); + // A scope naming exactly one project keeps the single-project filter, so the common case — + // the web UI, which always sends a projectId, and a REST API key, confined to one project — + // runs the unchanged indexed by-one-project aggregate. A caller who may read several and + // named none aggregates over that set instead of getting an empty overview (#483). + var (scopedProjectId, scopedProjectIds) = scope.ToFilterScope(); + var latencyFilter = new StatisticsFilter(from, null, scopedProjectId, agentId, ProjectIds: scopedProjectIds); + var breakdownFilter = new StatisticsFilter(from, null, scopedProjectId, ProjectIds: scopedProjectIds); - // Scope the agent load to the project when filtered, instead of loading every agent and - // discarding the rest in memory. - Task> agentsTask = scopedProjectId.HasValue - ? agentRepository.GetByProjectAsync(scopedProjectId.Value, cancellationToken) - : agentRepository.GetAllAsync(cancellationToken); + Task> agentsTask = ScopedAgentsAsync(scope, cancellationToken); Task> lastCallTask = repository.GetLastCallTimesAsync(cancellationToken); Task> breakdownTask = statistics.GetAgentBreakdownAsync(breakdownFilter, cancellationToken); Task> latencyTask = statistics.GetLatencyAsync(latencyFilter, cancellationToken); diff --git a/Proxytrace.Api/Controllers/EvaluatorsController.cs b/Proxytrace.Api/Controllers/EvaluatorsController.cs index 5c0f1c48f..9e070f343 100644 --- a/Proxytrace.Api/Controllers/EvaluatorsController.cs +++ b/Proxytrace.Api/Controllers/EvaluatorsController.cs @@ -146,13 +146,15 @@ public async Task GetOverview( if (scope.IsEmpty()) return new EvaluatorsOverviewDto([], [], []); - // The sparkline query is per-project, so it runs whenever the scope narrows to exactly one - // — which now includes an unfiltered REST API key, confined to its own project. - var singleProject = scope.SingleProject(); + // The sparkline query is project-scoped, so it runs for every restricted scope — one + // project's when the request named one (or the caller is a REST API key, confined to its + // own), and the caller's whole membership otherwise (#483). An unrestricted admin scope + // (null) still gets no sparklines: there is no project set to key them on, and the + // instance-wide series is not what that view shows. Task> evaluatorsTask = ListScopedAsync(scope, cancellationToken); Task> suitesTask = ListScopedSuitesAsync(scope, cancellationToken); - Task> sparklinesTask = singleProject is { } sparklineProject && from.HasValue && to.HasValue - ? evaluatorStats.GetSparklinesAsync(sparklineProject, from.Value, to.Value, bucket, cancellationToken) + Task> sparklinesTask = scope is { Count: > 0 } sparklineProjects && from.HasValue && to.HasValue + ? evaluatorStats.GetSparklinesAsync(sparklineProjects, from.Value, to.Value, bucket, cancellationToken) : Task.FromResult>([]); await Task.WhenAll(evaluatorsTask, suitesTask, sparklinesTask); diff --git a/Proxytrace.Api/Controllers/ProjectsController.cs b/Proxytrace.Api/Controllers/ProjectsController.cs index 29ca71e3b..13c479180 100644 --- a/Proxytrace.Api/Controllers/ProjectsController.cs +++ b/Proxytrace.Api/Controllers/ProjectsController.cs @@ -3,8 +3,8 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; +using Proxytrace.Api.Auth; using Proxytrace.Api.Dto.Projects; -using Proxytrace.Application.Auth; using Proxytrace.Application.Evaluator; using Proxytrace.Application.Tracey; using Proxytrace.Domain; @@ -30,7 +30,7 @@ public class ProjectsController : ControllerBase private readonly IProject.CreateExisting createExisting; private readonly ITraceyAgentProvisioner traceyProvisioner; private readonly IDefaultEvaluatorProvisioner defaultEvaluatorProvisioner; - private readonly ICurrentUserAccessor currentUser; + private readonly IProjectAccessGuard accessGuard; private readonly ILogger audit; public ProjectsController( @@ -42,7 +42,7 @@ public ProjectsController( IProject.CreateExisting createExisting, ITraceyAgentProvisioner traceyProvisioner, IDefaultEvaluatorProvisioner defaultEvaluatorProvisioner, - ICurrentUserAccessor currentUser, + IProjectAccessGuard accessGuard, ILogger audit) { this.repository = repository; @@ -53,7 +53,7 @@ public ProjectsController( this.createExisting = createExisting; this.traceyProvisioner = traceyProvisioner; this.defaultEvaluatorProvisioner = defaultEvaluatorProvisioner; - this.currentUser = currentUser; + this.accessGuard = accessGuard; this.audit = audit; } @@ -63,30 +63,37 @@ public async Task> GetAll( [FromQuery] int pageSize = 50, CancellationToken cancellationToken = default) { - // Clamp before either branch: the admin path clamps inside GetPagedAsync, but the in-memory - // member path below did not, so pageSize=int.MaxValue returned every member project in one - // response and echoed the unclamped size back in the PagedResult. + // Clamp before either branch: the unscoped path clamps inside GetPagedAsync, but the + // in-memory scoped path below does not, so pageSize=int.MaxValue would return every + // accessible project in one response and echo the unclamped size back in the PagedResult. (page, pageSize) = Paging.Clamp(page, pageSize); - // Admins see every project; non-admins (e.g. the sidebar project switcher) see only the - // projects they belong to — never the full cross-tenant list. - if (User.IsInRole(nameof(UserRole.Admin))) + // The guard is the single authority on who may see which project: an admin sees every + // project (null scope), everyone else only the projects they belong to, and a REST API key + // only the project it was minted for — the confinement this endpoint used to miss (#474), + // because an inline User.IsInRole/membership check cannot see the key's project. + // There is no projectId filter to resolve here (the listed resource *is* the project), so + // the scope is simply the caller's own reach. + var scope = await accessGuard.ResolveListScopeAsync(requestedProjectId: null, cancellationToken); + if (scope.IsEmpty()) + return new PagedResult([], 0, page, pageSize); + + if (scope is null) { var paged = await repository.GetPagedAsync(page, pageSize, cancellationToken); return paged.Map(ProjectDtoMapper.ToListItemDto); } - var user = await currentUser.GetCurrentUserAsync(cancellationToken); - if (user is null) - return new PagedResult([], 0, page, pageSize); - - var memberProjects = await repository.GetByMemberAsync(user.Id, cancellationToken); - var items = memberProjects + // Tolerate an id that vanished between resolving the scope and loading it (a project + // deleted concurrently) rather than failing the whole listing. + var accessible = await repository.GetManyAsync(scope, cancellationToken, ignoreMissing: true); + var items = accessible + .OrderByDescending(p => p.CreatedAt) .Skip(Paging.Offset(page, pageSize)) .Take(pageSize) .Select(ProjectDtoMapper.ToListItemDto) .ToArray(); - return new PagedResult(items, memberProjects.Count, page, pageSize); + return new PagedResult(items, accessible.Count, page, pageSize); } [HttpGet("{id:guid}")] @@ -96,7 +103,7 @@ public async Task> Get(Guid id, CancellationToken cance if (project is null) return NotFound(); // Hide projects the caller cannot access behind a 404 so existence does not leak. - if (!await CanAccessAsync(project, cancellationToken)) + if (!await accessGuard.CanAccessProjectAsync(project.Id, cancellationToken)) return NotFound(); return ToDto(project); } @@ -199,8 +206,9 @@ public async Task>> GetMembers( var project = await repository.FindAsync(id, cancellationToken); if (project is null) return NotFound(); - // Members' emails are PII — only an admin or a member of the project may list them. - if (!await CanAccessAsync(project, cancellationToken)) + // Members' emails are PII — only a caller who may reach the project may list them (an + // admin, a member, or a REST API key minted for exactly this project). + if (!await accessGuard.CanAccessProjectAsync(project.Id, cancellationToken)) return NotFound(); return project.Members.Select(ProjectDtoMapper.ToMemberDto).ToArray(); } @@ -251,16 +259,6 @@ public async Task> RemoveMember( return ToDto(saved); } - // Admins can access any project; everyone else only the projects they belong to. The project is - // already loaded with its Members, so membership is checked in memory without an extra query. - private async Task CanAccessAsync(IProject project, CancellationToken cancellationToken) - { - if (User.IsInRole(nameof(UserRole.Admin))) - return true; - var user = await currentUser.GetCurrentUserAsync(cancellationToken); - return user is not null && project.Members.Any(m => m.Id == user.Id); - } - private async Task?> ResolveMembersAsync( IReadOnlyList? memberIds, CancellationToken cancellationToken) diff --git a/Proxytrace.Api/Middleware/ExceptionHandlingMiddleware.cs b/Proxytrace.Api/Middleware/ExceptionHandlingMiddleware.cs index d17f9775b..2e605aa7c 100644 --- a/Proxytrace.Api/Middleware/ExceptionHandlingMiddleware.cs +++ b/Proxytrace.Api/Middleware/ExceptionHandlingMiddleware.cs @@ -32,6 +32,22 @@ public async Task InvokeAsync(HttpContext context) } catch (Exception ex) when (ex is not OperationCanceledException) { + // Classify a client that hung up mid-stream *first*, before anything is logged at Error + // level. Such a disconnect surfaces as a failed write (typically an IOException) rather + // than an OperationCanceledException, so it reaches this catch like a genuine fault — + // and capturing it would persist an ApplicationError row, carrying an errorId nobody can + // act on, for every browser tab closed on a page with an open SSE stream. There is no + // connection left to reset either, and no error body can be written into a response that + // has already started, so a single Debug line is the whole handling. + if (context.Response.HasStarted && context.RequestAborted.IsCancellationRequested) + { + logger.LogDebug( + ex, + "Request aborted by the client after the response had already started for {Path}", + context.Request.Path.Value.ToSingleLogLine()); + return; + } + // Pre-assign the captured error's id so it can be returned to the client (for an // admin deep-link into the Error Log). Only meaningful when we actually capture, i.e. // the Error/Critical branch below — a not-implemented stub is logged at Information. @@ -59,14 +75,23 @@ public async Task InvokeAsync(HttpContext context) // Once the response has started (every SSE stream writes its headers and first frames // long before its work can fail) the status and headers are read-only: assigning them // throws InvalidOperationException, that secondary exception escapes this catch, and the - // original fault is replaced by a bare connection abort. Nothing useful can be sent at - // this point, so log the real cause and let the abort happen on its own. + // original fault is replaced by a bare connection abort. An error body is equally + // useless — it would be appended to a payload the client is already consuming. + // Reaching here with a started response means the client is still connected (the + // disconnect case returned above), so this is a genuine fault on a live stream. if (context.Response.HasStarted) { logger.LogWarning( ex, - "Unhandled exception after the response had already started for {Path}; cannot write an error body", + "Unhandled exception after the response had already started for {Path}; resetting the connection", context.Request.Path.Value.ToSingleLogLine()); + + // Returning without aborting would signal *success* to the framework: it finishes the + // response cleanly (chunked terminator / HTTP/2 END_STREAM) and the client reads a + // well-formed but truncated payload as a complete one. Aborting resets the connection + // so the truncation surfaces as a transport error the caller cannot mistake for a + // short-but-valid result. + context.Abort(); return; } diff --git a/Proxytrace.Api/Module.cs b/Proxytrace.Api/Module.cs index 3525c4397..9ec6a9eeb 100644 --- a/Proxytrace.Api/Module.cs +++ b/Proxytrace.Api/Module.cs @@ -47,9 +47,18 @@ protected override void Load(ContainerBuilder builder) builder.RegisterModule(); + // The container owns a second view of configuration because the host's does not read + // appsettings.local.json (the machine-local override holding the generated signing key — + // see Program.cs). It must otherwise mirror the host's view: the environment-specific + // appsettings.{Environment}.json is layered in exactly where the host layers it, between + // the base file and the more specific local override, so an operator who adds + // appsettings.Production.json gets it honoured here too. See docs/security.md. + var environmentName = HostEnvironmentName.Resolve(); + ConfigurationBuilder configurationBuilder = new ConfigurationBuilder(); IConfiguration configuration = configurationBuilder .AddJsonFile("appsettings.json", optional: false, reloadOnChange: false) + .AddJsonFile($"appsettings.{environmentName}.json", optional: true, reloadOnChange: false) .AddJsonFile("appsettings.local.json", optional: true, reloadOnChange: false) .AddEnvironmentVariables() .Build(); @@ -289,7 +298,7 @@ protected override void Load(ContainerBuilder builder) .WithToolsFromAssembly(typeof(Module).Assembly) .WithPromptsFromAssembly(typeof(Module).Assembly)); - ConfigureAuth(builder, configuration, kiosk); + ConfigureAuth(builder, configuration, kiosk, environmentName); SearchConfiguration searchConfiguration = configuration.GetSection("Search").Get() ?? new SearchConfiguration(); @@ -319,7 +328,11 @@ protected override void Load(ContainerBuilder builder) builder.RegisterInstance(new DashboardCacheOptions { TtlSeconds = statisticsOptions.DashboardCacheTtlSeconds }); } - private void ConfigureAuth(ContainerBuilder builder, IConfiguration configuration, KioskOptions kiosk) + private void ConfigureAuth( + ContainerBuilder builder, + IConfiguration configuration, + KioskOptions kiosk, + string environmentName) { var authOptions = configuration.GetSection("Authentication").Get() ?? new AuthOptions(); builder.RegisterInstance(authOptions); @@ -331,10 +344,12 @@ private void ConfigureAuth(ContainerBuilder builder, IConfiguration configuratio // http:// — browsers do accept Secure cookies on localhost, but a dev host that is not // localhost would otherwise silently fail to log in). Override with // Authentication:SessionCookie:Secure for a deliberate plain-HTTP deployment. See docs/security.md. - var environmentName = configuration["ASPNETCORE_ENVIRONMENT"] - ?? configuration["DOTNET_ENVIRONMENT"] - ?? "Production"; - var isDevelopment = string.Equals(environmentName, "Development", StringComparison.OrdinalIgnoreCase); + // + // environmentName comes from HostEnvironmentName, which resolves it the way the host does — + // DOTNET_ENVIRONMENT ahead of ASPNETCORE_ENVIRONMENT. Reading it here in the opposite order + // made a Production host compute Development when both were set and disagreed, silently + // dropping Secure from the 7-day session cookie on an HTTPS install. + var isDevelopment = HostEnvironmentName.IsDevelopment(environmentName); builder .RegisterInstance(new SessionCookieOptions { diff --git a/Proxytrace.Application.Tests/Statistics/DashboardStatisticsTests.cs b/Proxytrace.Application.Tests/Statistics/DashboardStatisticsTests.cs index 364ac1f1f..8020cbc70 100644 --- a/Proxytrace.Application.Tests/Statistics/DashboardStatisticsTests.cs +++ b/Proxytrace.Application.Tests/Statistics/DashboardStatisticsTests.cs @@ -130,6 +130,43 @@ await runStats.Received(1).GetPassTotalsAsync( Arg.Any()); } + [TestMethod] + public async Task GetSummaryAsync_WithSeveralProjects_ResolvesAgentsOfEveryScopedProject() + { + // #483: a caller who may read several projects and named none. The agent set behind the + // pass-rate totals must be the union of those projects' agents — not empty (the old + // single-project-only branch) and not every agent in the install. + var svc = Build(out var runStats, out var callStats, out var agents); + callStats.GetSummaryAsync(Arg.Any(), Arg.Any()) + .Returns(new StatisticsSummary(0, 0, 0, 0, 0, 0)); + runStats.GetPassTotalsAsync(Arg.Any(), Arg.Any()) + .Returns(new TestRunPassTotals(0, 0)); + + var firstProject = Guid.NewGuid(); + var secondProject = Guid.NewGuid(); + IAgent first = AgentIn(firstProject); + IAgent second = AgentIn(secondProject); + IAgent outsider = AgentIn(Guid.NewGuid()); + agents.GetAllAsync(Arg.Any()).Returns([first, second, outsider]); + + await svc.GetSummaryAsync( + new StatisticsFilter(ProjectIds: [firstProject, secondProject]), CancellationToken); + + await runStats.Received(1).GetPassTotalsAsync( + Arg.Is(f => + f != null && f.AgentIds != null && f.AgentIds.Count == 2 + && f.AgentIds.Contains(first.Id) && f.AgentIds.Contains(second.Id)), + Arg.Any()); + } + + private static IAgent AgentIn(Guid projectId) + { + var agent = Substitute.For(); + agent.Id.Returns(Guid.NewGuid()); + agent.Project.Id.Returns(projectId); + return agent; + } + [TestMethod] public async Task GetDashboardTrendsAsync_CapsSparklineToRecentCohorts() { diff --git a/Proxytrace.Application.Tests/TestRunnerServiceTests.cs b/Proxytrace.Application.Tests/TestRunnerServiceTests.cs index aec3d21e2..edcfca2d2 100644 --- a/Proxytrace.Application.Tests/TestRunnerServiceTests.cs +++ b/Proxytrace.Application.Tests/TestRunnerServiceTests.cs @@ -4,6 +4,9 @@ using AwesomeAssertions; using Microsoft.Extensions.DependencyInjection; using NSubstitute; +using NSubstitute.ExceptionExtensions; +using Proxytrace.Application.Anomaly; +using Proxytrace.Application.Optimization; using Proxytrace.Application.Streaming; using Proxytrace.Application.TestRun; using Proxytrace.Domain; @@ -272,6 +275,179 @@ public async Task CancelAsync_WhileGroupIsRunning_SettlesGroupCancelledWithoutAS .Which.GroupStatus.Should().Be(TestRunStatus.Cancelled); } + [TestMethod] + public async Task RunInForeground_WhenCancelLandsAfterTheGroupCompleted_StillEnqueuesOptimizerAndAnomalyDetection() + { + // Regression for #476. The group transitions to Completed *inside* the per-group lock, but + // the optimizer / anomaly-detection enqueues ran outside it on the linked (cancellable) + // token. A cancel landing in that window tripped the token, both enqueues were skipped, + // SettleCancelledAsync then saw an already-terminal group and no-op'd — leaving the group + // reporting Completed forever with no optimization and no anomaly detection ever run, and + // nothing surfaced to say so. + // + // The window is opened deterministically from PublishGroupComplete, which the runner calls + // inside the lock immediately after SetCompleted. Cancelling the *caller's* token trips the + // very same linked token CancelAsync would, and is the only safe way to do it from here: + // CancelAsync re-enters the group lock this callback runs under. + var expectedOutput = new AssistantMessage([Content.FromText(MatchingText)], []); + + using var cts = new CancellationTokenSource(); + + var broadcaster = Substitute.For(); + broadcaster.When(b => b.PublishGroupComplete(Arg.Any())) + .Do(ci => + { + var groupEvent = ci.Arg(); + ArgumentNullException.ThrowIfNull(groupEvent); + if (groupEvent.GroupStatus == TestRunStatus.Completed) + cts.Cancel(); + }); + + // Both fakes honour the token they are handed, exactly as the real implementations do — + // each enqueue is a ChannelWriter.WriteAsync, which faults immediately on an already + // cancelled token rather than queueing the group. + CancellationToken optimizerToken = default; + CancellationToken anomalyToken = default; + + var optimizer = Substitute.For(); + optimizer.EnqueueAsync(Arg.Any(), Arg.Any()) + .Returns(call => + { + optimizerToken = call.Arg(); + optimizerToken.ThrowIfCancellationRequested(); + return Task.CompletedTask; + }); + + var anomalyDetection = Substitute.For(); + anomalyDetection.EnqueueAsync(Arg.Any(), Arg.Any()) + .Returns(call => + { + anomalyToken = call.Arg(); + anomalyToken.ThrowIfCancellationRequested(); + return Task.CompletedTask; + }); + + var services = GetServices(config => + { + RegisterFakeModelClient(config, expectedOutput); + config.RegisterInstance(broadcaster).As(); + config.RegisterInstance(optimizer).As(); + config.RegisterInstance(anomalyDetection).As(); + }); + + var suite = await BuildSuiteAsync(services, expectedOutput, CancellationToken); + var endpoint = (await CreateEndpoints(services, 1))[0]; + var runner = services.GetRequiredService(); + + // isSystemTestRun stays false — internal A/B runs deliberately skip both pipelines. + var group = await runner.RunInForegroundAsync(suite, [endpoint], cancellationToken: cts.Token); + + cts.IsCancellationRequested.Should() + .BeTrue("the test must have cancelled inside the completion window for this to prove anything"); + group.Status.Should().Be(TestRunStatus.Completed); + + await optimizer.Received(1).EnqueueAsync(Arg.Any(), Arg.Any()); + await anomalyDetection.Received(1).EnqueueAsync(Arg.Any(), Arg.Any()); + optimizerToken.IsCancellationRequested.Should() + .BeFalse("a durably Completed group owes its downstream jobs — a racing cancel must not skip them"); + anomalyToken.IsCancellationRequested.Should().BeFalse(); + + var stored = await services.GetRequiredService() + .GetAsync(group.Id, CancellationToken); + stored.Status.Should().Be(TestRunStatus.Completed); + } + + [TestMethod] + public async Task RunInForeground_WhenAnEnqueueFaultsAfterTheGroupCompleted_DoesNotRepeatTheTerminalEventOrDetection() + { + // Regression for #486. The generic catch assumed it had been reached from a *non-terminal* + // group: PublishGroupComplete sat outside the `if (!group.Status.IsTerminal())` guard and the + // anomaly enqueue followed unconditionally. But the tail of the try runs after the group is + // already durably Completed, so a fault raised there re-published the terminal event — + // carrying Completed a second time, not Failed — and detected the same group twice, which + // flags and notifies a genuine anomaly twice. + // + // The fault is injected into the anomaly enqueue itself because it is the *last* statement of + // the success path, so both duplicates are observable from one run: the success-path enqueue + // has already happened, making a second one distinguishable from the first. Each enqueue now + // also absorbs its own failure (see the sibling test below), so this pins the outcome from + // both directions: whichever layer catches the fault, the group settles exactly once. + var expectedOutput = new AssistantMessage([Content.FromText(MatchingText)], []); + + var broadcaster = Substitute.For(); + var groupEvents = new ConcurrentQueue(); + broadcaster.When(b => b.PublishGroupComplete(Arg.Any())) + .Do(ci => + { + var groupEvent = ci.Arg(); + ArgumentNullException.ThrowIfNull(groupEvent); + groupEvents.Enqueue(groupEvent); + }); + + // The real enqueue is a ChannelWriter.WriteAsync; a completed or disposed writer faults it. + var anomalyDetection = Substitute.For(); + anomalyDetection.EnqueueAsync(Arg.Any(), Arg.Any()) + .Throws(new InvalidOperationException("the anomaly queue is gone")); + + var services = GetServices(config => + { + RegisterFakeModelClient(config, expectedOutput); + config.RegisterInstance(broadcaster).As(); + config.RegisterInstance(anomalyDetection).As(); + }); + + var suite = await BuildSuiteAsync(services, expectedOutput, CancellationToken); + var endpoint = (await CreateEndpoints(services, 1))[0]; + var runner = services.GetRequiredService(); + + // isSystemTestRun stays false, so the enqueues — and with them the fault — actually run. + var group = await runner.RunInForegroundAsync(suite, [endpoint], cancellationToken: CancellationToken); + + // A fault landing after the group settled cannot un-settle it, and the failure path must not + // pretend otherwise: no second terminal event, and no second pass of anomaly detection. + group.Status.Should().Be(TestRunStatus.Completed); + var stored = await services.GetRequiredService() + .GetAsync(group.Id, CancellationToken); + stored.Status.Should().Be(TestRunStatus.Completed); + + groupEvents.Should().ContainSingle("a group has exactly one terminal event") + .Which.GroupStatus.Should().Be(TestRunStatus.Completed); + await anomalyDetection.Received(1).EnqueueAsync(Arg.Any(), Arg.Any()); + } + + [TestMethod] + public async Task RunInForeground_WhenTheOptimizerEnqueueFaults_StillEnqueuesAnomalyDetection() + { + // The two downstream jobs a completed group owes are unrelated, so failing to hand it to one + // must not cost it the other. They used to be bare sequential awaits: an optimizer fault + // skipped the anomaly enqueue and fell into the generic catch, which happened to enqueue it + // there instead. That accidental compensation is gone now that the catch only settles a group + // it actually transitioned (#486), so each enqueue owns its own failure. + var expectedOutput = new AssistantMessage([Content.FromText(MatchingText)], []); + + var optimizer = Substitute.For(); + optimizer.EnqueueAsync(Arg.Any(), Arg.Any()) + .Throws(new InvalidOperationException("the optimizer queue is gone")); + + var anomalyDetection = Substitute.For(); + + var services = GetServices(config => + { + RegisterFakeModelClient(config, expectedOutput); + config.RegisterInstance(optimizer).As(); + config.RegisterInstance(anomalyDetection).As(); + }); + + var suite = await BuildSuiteAsync(services, expectedOutput, CancellationToken); + var endpoint = (await CreateEndpoints(services, 1))[0]; + var runner = services.GetRequiredService(); + + var group = await runner.RunInForegroundAsync(suite, [endpoint], cancellationToken: CancellationToken); + + group.Status.Should().Be(TestRunStatus.Completed); + await anomalyDetection.Received(1).EnqueueAsync(Arg.Any(), Arg.Any()); + } + [TestMethod] public async Task RunInForeground_WhenCancelledMidRun_MarksGroupCancelled() { diff --git a/Proxytrace.Application/Statistics/Internal/DashboardStatistics.cs b/Proxytrace.Application/Statistics/Internal/DashboardStatistics.cs index 810640a6c..c96dd262f 100644 --- a/Proxytrace.Application/Statistics/Internal/DashboardStatistics.cs +++ b/Proxytrace.Application/Statistics/Internal/DashboardStatistics.cs @@ -164,16 +164,20 @@ private async Task ComputeDashboardViewAsync(StatisticsFilter fil Task tokenBucketTask = Task.Run(() => ResolveTokenBucketAsync(filter, cancellationToken), cancellationToken); Task> tokenUsageTask = Task.Run(async () => await GetTokenUsageAsync(filter, await tokenBucketTask, cancellationToken), cancellationToken); Task> tokenByAgentTask = Task.Run(async () => await GetTokenUsageByAgentAsync(filter, await tokenBucketTask, cancellationToken), cancellationToken); + // The recent-traces list carries the same project scope as the aggregates — including a + // multi-project one (#483), which the trace filter applies as a set (#482). Task<(IReadOnlyList Items, int Total)> recentTask = Task.Run(() => agentCalls.GetFilteredAsync( - new AgentCallFilter(ProjectId: filter.ProjectId, From: filter.From, IncludeSystemAgents: !filter.ExcludeSystemAgents), + new AgentCallFilter( + ProjectId: filter.ProjectId, + From: filter.From, + IncludeSystemAgents: !filter.ExcludeSystemAgents, + ProjectIds: filter.ProjectIds), page: 1, pageSize: recentTraceCount, cancellationToken), cancellationToken); - // Scope the agent load to the project when filtered, instead of loading every agent and + // Scope the agent load to the filter's projects, instead of loading every agent and // discarding the rest in memory. The unfiltered (global) dashboard still needs all agents. - Task> agentsTask = Task.Run(() => filter.ProjectId is { } projectId - ? agents.GetByProjectAsync(projectId, cancellationToken) - : agents.GetAllAsync(cancellationToken), cancellationToken); + Task> agentsTask = Task.Run(() => GetScopedAgentsAsync(filter, cancellationToken), cancellationToken); Task> lastCallTimesTask = Task.Run(() => agentCalls.GetLastCallTimesAsync(cancellationToken), cancellationToken); Task> pulseTask = Task.Run(() => GetPulseAsync(filter, cancellationToken), cancellationToken); @@ -292,13 +296,36 @@ internal async Task GetDashboardTrendsAsync(StatisticsFilter fi return new DashboardTrends(trends.Traces, trends.LatencyMs, trends.Throughput, passRate); } + /// + /// The agents the filter's project scope covers: one project's agents when it names a project, + /// the union of the scope's projects when it spans several (#483), and every agent when it is + /// unscoped (the admin-only global dashboard). + /// + /// + /// The multi-project case loads all agents and narrows in memory, mirroring how the agent and + /// evaluator listings scope themselves — the agents table is small and bounded by the licensed + /// agent limit, unlike the trace table its aggregates run over. + /// + private async Task> GetScopedAgentsAsync(StatisticsFilter filter, CancellationToken cancellationToken) + { + if (filter.ProjectId is { } projectId) + { + return await agents.GetByProjectAsync(projectId, cancellationToken); + } + + IReadOnlyList all = await agents.GetAllAsync(cancellationToken); + return filter.ProjectIds is { Count: > 0 } projectIds + ? all.Where(a => projectIds.Contains(a.Project.Id)).ToArray() + : all; + } + private async Task ToRunFilterAsync(StatisticsFilter filter, CancellationToken cancellationToken) { IReadOnlyCollection? agentIds = null; - if (filter.ProjectId is { } projectId) + if (filter.ProjectId is not null || filter.ProjectIds is { Count: > 0 }) { - IReadOnlyList projectAgents = await agents.GetByProjectAsync(projectId, cancellationToken); - agentIds = projectAgents.Select(a => a.Id).ToArray(); + IReadOnlyList scopedAgents = await GetScopedAgentsAsync(filter, cancellationToken); + agentIds = scopedAgents.Select(a => a.Id).ToArray(); } return new TestRunStats.Filter( diff --git a/Proxytrace.Application/TestRun/Internal/TestRunnerService.cs b/Proxytrace.Application/TestRun/Internal/TestRunnerService.cs index 5dde7b1cd..3c2027464 100644 --- a/Proxytrace.Application/TestRun/Internal/TestRunnerService.cs +++ b/Proxytrace.Application/TestRun/Internal/TestRunnerService.cs @@ -234,6 +234,23 @@ private async Task SettleCancelledAsync(ITestRunGroup group, Canc return group; } + /// + /// Hands a durably completed group to one downstream job, absorbing a failure to enqueue it. + /// The group is already terminal and already broadcast, so nothing here may unwind that — and the + /// jobs are independent, so one failing must not cost the group the other. + /// + private async Task EnqueueCompletedGroupAsync(string job, Func enqueue, Guid groupId) + { + try + { + await enqueue(); + } + catch (Exception ex) + { + logger.LogError(ex, "Failed to enqueue {Job} for completed test run group {GroupId}", job, groupId); + } + } + private async Task ExecuteGroupAsync( ITestRunGroup group, IAgent? customAgent = null, @@ -284,8 +301,24 @@ await Parallel.ForEachAsync( if (!isSystemTestRun) { - await optimizer.EnqueueAsync(group, cancellationToken); - await anomalyDetection.EnqueueAsync(group, cancellationToken); + // CancellationToken.None on purpose, exactly like the failure path below. The group + // is already durably Completed and its completion already broadcast, so a CancelAsync + // landing in this window has nothing left to cancel: SettleCancelledAsync sees a + // terminal group and no-ops. Handing the linked (cancellable) token to the enqueues + // let that race skip both jobs silently — the group read Completed forever with no + // optimization and no anomaly detection ever run, and no error surfaced. Once the + // group is Completed the downstream work is owed, so it must not be cancellable. + // + // Independently, too: the two jobs are unrelated, so a failure to enqueue one must not + // cost the group the other. They used to be bare sequential awaits, which meant an + // optimizer failure skipped anomaly detection and fell into the generic catch — where + // an unconditional enqueue happened to run it anyway. That compensation is gone now + // that the catch only settles a group it actually transitioned (#486), so each enqueue + // owns its own failure here. + await EnqueueCompletedGroupAsync( + "optimization", () => optimizer.EnqueueAsync(group, CancellationToken.None), group.Id); + await EnqueueCompletedGroupAsync( + "anomaly detection", () => anomalyDetection.EnqueueAsync(group, CancellationToken.None), group.Id); } return group; } @@ -311,21 +344,32 @@ await Parallel.ForEachAsync( logger.LogError(ex, "Test run group {GroupId} failed", group.Id); try { + bool transitionedToFailed = false; + // Same check-then-act as the cancel path: a user cancelling while the group is - // failing must not collide with this transition or its broadcast. + // failing must not collide with this transition or its broadcast. The broadcast + // belongs *inside* the guard, not after it: this handler is also reachable with the + // group already terminal — settled Cancelled by that racing CancelAsync, or settled + // Completed above when the fault came from the tail of the try (an enqueue). Whoever + // settled it already published its one terminal event, so re-publishing here sent a + // second one, carrying the state it was already in rather than Failed (#486). using (IDisposable sync = await asyncLock.LockAsync(group.Id, CancellationToken.None)) { group = await group.ReloadAsync(CancellationToken.None); if (!group.Status.IsTerminal()) { group = await group.SetFailed(CancellationToken.None); + broadcaster.PublishGroupComplete(GroupRunCompleteEvent.Create(group)); + transitionedToFailed = true; } - broadcaster.PublishGroupComplete(GroupRunCompleteEvent.Create(group)); } - // A failed group is the most important anomaly. The success-path enqueue above is - // skipped when we land here, so detect from the failure path too. - if (!isSystemTestRun) + // A failed group is the most important anomaly, and the success-path enqueue never + // ran for one. Only *this* call's Failed transition owes that work: a group that was + // already terminal was settled elsewhere, which enqueued detection itself (the + // success path) or deliberately did not (a cancel) — either way, enqueueing again + // here would detect the same group twice and notify on it twice. + if (transitionedToFailed && !isSystemTestRun) { await anomalyDetection.EnqueueAsync(group, CancellationToken.None); } diff --git a/Proxytrace.Domain.Tests/ProviderEndpointsTests.cs b/Proxytrace.Domain.Tests/ProviderEndpointsTests.cs index 21db30b07..572062cad 100644 --- a/Proxytrace.Domain.Tests/ProviderEndpointsTests.cs +++ b/Proxytrace.Domain.Tests/ProviderEndpointsTests.cs @@ -9,6 +9,7 @@ public sealed class ProviderEndpointsTests [TestMethod] [DataRow("https://my-resource.openai.azure.com/", true)] [DataRow("https://eastus.api.cognitive.microsoft.azure.com/", true)] + [DataRow("https://azure.com/v1", true)] [DataRow("https://api.openai.com/v1", false)] [DataRow("https://api.anthropic.com/v1", false)] // Lookalike hosts must not match: the suffix is a domain boundary, not a substring. @@ -18,4 +19,16 @@ public void IsAzure_DetectsByHost(string endpoint, bool expected) { ProviderEndpoints.IsAzure(new Uri(endpoint)).Should().Be(expected); } + + [TestMethod] + // Uri.Host preserves the DNS root dot, so a fully-qualified hostname must still classify. + [DataRow("https://resource.openai.azure.com./", true)] + [DataRow("https://azure.com./v1", true)] + // …without weakening the domain-boundary check for lookalikes. + [DataRow("https://my-azure.com.example.net./v1", false)] + [DataRow("https://notazure.com./v1", false)] + public void IsAzure_WithTrailingRootDot_DetectsByHost(string endpoint, bool expected) + { + ProviderEndpoints.IsAzure(new Uri(endpoint)).Should().Be(expected); + } } diff --git a/Proxytrace.Domain.Tests/SecretRedactionToStringTests.cs b/Proxytrace.Domain.Tests/SecretRedactionToStringTests.cs index 37b3aecd6..259beb2aa 100644 --- a/Proxytrace.Domain.Tests/SecretRedactionToStringTests.cs +++ b/Proxytrace.Domain.Tests/SecretRedactionToStringTests.cs @@ -4,6 +4,7 @@ using Proxytrace.Domain.ModelProvider; using Proxytrace.Domain.Notification; using Proxytrace.Domain.Notifications; +using Proxytrace.Domain.User; using Proxytrace.Domain.UserTotpEnrollment; using Proxytrace.Testing; @@ -90,10 +91,32 @@ public async Task UserTotpEnrollment_ToString_RedactsSharedSecret() text.Should().Contain("Secret = ***"); } + [TestMethod] + public void User_ToString_RedactsPasswordHash() + { + IServiceProvider services = GetServices(); + var create = services.GetRequiredService(); + + var user = create( + email: "operator@example.com", + externalSubject: "https://issuer.example.com|subject-1234", + passwordHash: "AQAAAAIAAYagAAAAEsuper-secret-password-hash", + role: UserRole.Admin); + + var text = user.ToString(); + + text.Should().NotBeNull(); + text.Should().NotContain("AQAAAAIAAYagAAAAEsuper-secret-password-hash"); + text.Should().Contain("PasswordHash = ***"); + // Identifiers are not secrets: they stay readable so a log line still says who this is. + text.Should().Contain("operator@example.com") + .And.Contain("https://issuer.example.com|subject-1234"); + } + [TestMethod] public async Task ModelProvider_ToString_RedactsApiKey() { - // Pins the reference implementation the four peers above mirror. + // Pins the reference implementation the five peers above mirror. IServiceProvider services = GetServices(); var provider = await services .GetRequiredService>() diff --git a/Proxytrace.Domain/ModelProvider/ProviderEndpoints.cs b/Proxytrace.Domain/ModelProvider/ProviderEndpoints.cs index 17115a76e..a5d000fb2 100644 --- a/Proxytrace.Domain/ModelProvider/ProviderEndpoints.cs +++ b/Proxytrace.Domain/ModelProvider/ProviderEndpoints.cs @@ -13,11 +13,21 @@ public static class ProviderEndpoints /// True when the endpoint host indicates an Azure OpenAI resource. Matched as a domain suffix, /// not a substring: a substring test also accepts lookalike hosts such as /// my-azure.com.example.net, and misclassifying one as Azure makes the proxy attach the - /// provider credential in a second api-key header on top of the bearer. + /// provider credential in a second api-key header on top of the bearer. A single trailing + /// DNS root dot is normalized away first — (like + /// and ) preserves it, so the fully-qualified + /// resource.openai.azure.com. would otherwise fail the suffix match. Exactly one dot is + /// trimmed: resource.azure.com.. is not a legal hostname and stays unmatched. /// - public static bool IsAzure(Uri endpoint) => - endpoint.Host.Equals(AzureDomain, StringComparison.OrdinalIgnoreCase) - || endpoint.Host.EndsWith($".{AzureDomain}", StringComparison.OrdinalIgnoreCase); + public static bool IsAzure(Uri endpoint) + { + string host = endpoint.Host; + if (host.EndsWith('.')) + host = host[..^1]; + + return host.Equals(AzureDomain, StringComparison.OrdinalIgnoreCase) + || host.EndsWith($".{AzureDomain}", StringComparison.OrdinalIgnoreCase); + } private const string AzureDomain = "azure.com"; diff --git a/Proxytrace.Domain/Statistics/IEvaluatorStatsReader.cs b/Proxytrace.Domain/Statistics/IEvaluatorStatsReader.cs index 2e798bced..b3ed7c959 100644 --- a/Proxytrace.Domain/Statistics/IEvaluatorStatsReader.cs +++ b/Proxytrace.Domain/Statistics/IEvaluatorStatsReader.cs @@ -12,8 +12,14 @@ Task GetOverviewAsync( StatisticsBucket bucket, CancellationToken cancellationToken = default); + /// + /// One pass-rate sparkline per evaluator owned by . Takes a set + /// rather than a single project so the evaluators overview can be scoped to everything the + /// caller may read — a non-admin who belongs to several projects and filtered by none would + /// otherwise see evaluators with no sparklines beside them (#483). + /// Task> GetSparklinesAsync( - Guid projectId, + IReadOnlyCollection projectIds, DateTimeOffset from, DateTimeOffset to, StatisticsBucket bucket, diff --git a/Proxytrace.Domain/Statistics/StatisticsRecords.cs b/Proxytrace.Domain/Statistics/StatisticsRecords.cs index 91d0a8367..0324935d2 100644 --- a/Proxytrace.Domain/Statistics/StatisticsRecords.cs +++ b/Proxytrace.Domain/Statistics/StatisticsRecords.cs @@ -13,7 +13,18 @@ public record StatisticsFilter( // When true, drops calls attributed to system agents (the Tracey assistant, evaluators) from // every aggregate. Default false keeps project-wide totals. Used by the Tracey dashboard tool so // its usage figures are about the user's own agents, not the platform's own activity. - bool ExcludeSystemAgents = false); + bool ExcludeSystemAgents = false, + // The set of projects the aggregate is restricted to, for a caller who may read several and + // named none (#483). Mirrors AgentCallFilter.ProjectIds in shape and semantics: ProjectId above + // stays the single-project filter and a scope naming exactly one project keeps using it — so + // the common, indexed by-one-project path is unchanged — while a genuinely multi-project scope + // comes through here. Null or empty means "not restricted by a set"; both the LINQ chokepoint + // and the raw-SQL percentile paths must honour it. + // + // Note it participates in this record's value equality by identity (collections compare by + // reference), so a multi-project filter misses the dashboard view cache rather than aliasing + // another caller's entry — a miss, never a false hit. + IReadOnlyCollection? ProjectIds = null); public record StatisticsSummary( long TotalCalls, diff --git a/Proxytrace.Domain/User/Internal/User.cs b/Proxytrace.Domain/User/Internal/User.cs index 9795db99a..a1e74912c 100644 --- a/Proxytrace.Domain/User/Internal/User.cs +++ b/Proxytrace.Domain/User/Internal/User.cs @@ -1,4 +1,5 @@ using System.ComponentModel.DataAnnotations; +using System.Text; using Proxytrace.Common.Validation; using Proxytrace.Domain.Internal; using Proxytrace.Domain.Notification; @@ -77,6 +78,31 @@ public Task ChangeEmailNotificationPreferences(bool emailNotificationsEna ? Task.FromResult(this) : ApplyAsync(this with { EmailNotificationsEnabled = emailNotificationsEnabled, EmailNotificationMinSeverity = emailNotificationMinSeverity }, cancellationToken); + // Redact the stored password hash from the record's generated ToString()/PrintMembers so it never + // leaks into a log line, exception message or debugger string — a salted, slow hash lifted from + // the operator Error Log or a support bundle is an offline-cracking target. It also travels + // transitively: any record holding an IUser (UserTotpEnrollment, ApiKey.Owner) prints it. The + // same treatment ModelProvider gives its upstream API key. PasswordHash stays a public member + // (the login path reads it; equality keeps it) — only its textual rendering is masked. + // ExternalSubject is deliberately left visible: it is an identifier, not a credential, and + // holding it grants nothing (like EmailSettings.Username, which also stays). + protected override bool PrintMembers(StringBuilder builder) + { + if (base.PrintMembers(builder)) + { + builder.Append(", "); + } + + builder.Append("Email = ").Append(Email) + .Append(", ExternalSubject = ").Append(ExternalSubject) + .Append(", PasswordHash = ***") + .Append(", Role = ").Append(Role) + .Append(", Language = ").Append(Language) + .Append(", EmailNotificationsEnabled = ").Append(EmailNotificationsEnabled) + .Append(", EmailNotificationMinSeverity = ").Append(EmailNotificationMinSeverity); + return true; + } + public override IEnumerable Validate(ValidationContext validationContext) { foreach (var result in base.Validate(validationContext)) diff --git a/Proxytrace.Infrastructure.Tests/FrankfurterFxRateProviderTests.cs b/Proxytrace.Infrastructure.Tests/FrankfurterFxRateProviderTests.cs index 828a5191f..1988a4cd0 100644 --- a/Proxytrace.Infrastructure.Tests/FrankfurterFxRateProviderTests.cs +++ b/Proxytrace.Infrastructure.Tests/FrankfurterFxRateProviderTests.cs @@ -11,12 +11,21 @@ public sealed class FrankfurterFxRateProviderTests { public required TestContext TestContext { get; init; } + private const string RateBody = + """{"amount":1.0,"base":"USD","date":"2026-06-09","rates":{"EUR":0.92}}"""; + + /// + /// Comfortably longer than the provider's (private) FailedFetchRetryInterval — advancing the test + /// clock by this expires the negative cache a failed fetch armed, while staying inside the same + /// calendar day so the positive cache is unaffected. + /// + private static readonly TimeSpan PastNegativeCache = TimeSpan.FromMinutes(5); + [TestMethod] public async Task GetUsdToEur_ParsesRate() { - var handler = new StubHandler(HttpStatusCode.OK, - """{"amount":1.0,"base":"USD","date":"2026-06-09","rates":{"EUR":0.92}}"""); - var sut = new FrankfurterFxRateProvider(new HttpClient(handler), new PricingOptions(), new NoOpAsyncLock()); + var handler = new StubHandler(HttpStatusCode.OK, RateBody); + var sut = new FrankfurterFxRateProvider(new HttpClient(handler), new PricingOptions(), new NoOpAsyncLock(), new MutableClock()); var rate = await sut.GetUsdToEurAsync(TestContext.CancellationToken); @@ -27,11 +36,97 @@ public async Task GetUsdToEur_ParsesRate() public async Task GetUsdToEur_OnFailure_ReturnsNull() { var handler = new StubHandler(HttpStatusCode.InternalServerError, "boom"); - var sut = new FrankfurterFxRateProvider(new HttpClient(handler), new PricingOptions(), new NoOpAsyncLock()); + var sut = new FrankfurterFxRateProvider(new HttpClient(handler), new PricingOptions(), new NoOpAsyncLock(), new MutableClock()); + + (await sut.GetUsdToEurAsync(TestContext.CancellationToken)).Should().BeNull(); + } + + [TestMethod] + public async Task GetUsdToEur_DuringFxOutage_AttemptsOnlyOneFetchForRepeatedCalls() + { + var handler = new SequencedHandler(_ => throw new HttpRequestException("fx feed is down")); + var sut = new FrankfurterFxRateProvider(new HttpClient(handler), new PricingOptions(), new NoOpAsyncLock(), new MutableClock()); + + // A provider exposing many models resolves a price per model and every price needs the FX + // rate; the outage must not turn that into one outbound fetch attempt per model (#487). + for (int i = 0; i < 10; i++) + { + decimal? rate = await sut.GetUsdToEurAsync(TestContext.CancellationToken); + rate.Should().BeNull(); + } + + handler.CallCount.Should().Be(1); + } + + [TestMethod] + public async Task GetUsdToEur_AfterNegativeCacheExpires_RetriesTheFetch() + { + var clock = new MutableClock(); + var handler = new SequencedHandler(_ => throw new HttpRequestException("fx feed is down")); + var sut = new FrankfurterFxRateProvider(new HttpClient(handler), new PricingOptions(), new NoOpAsyncLock(), clock); + + await sut.GetUsdToEurAsync(TestContext.CancellationToken); + await sut.GetUsdToEurAsync(TestContext.CancellationToken); + handler.CallCount.Should().Be(1, "the second call is still inside the negative-cache window"); + + clock.Advance(PastNegativeCache); + await sut.GetUsdToEurAsync(TestContext.CancellationToken); + + handler.CallCount.Should().Be(2); + } + + [TestMethod] + public async Task GetUsdToEur_WhenCancelled_DoesNotArmTheNegativeCache() + { + using var cts = new CancellationTokenSource(); + var handler = new SequencedHandler( + ct => + { + cts.Cancel(); + ct.ThrowIfCancellationRequested(); + throw new InvalidOperationException("unreachable"); + }, + _ => Ok()); + var sut = new FrankfurterFxRateProvider(new HttpClient(handler), new PricingOptions(), new NoOpAsyncLock(), new MutableClock()); + + await FluentActions + .Invoking(() => sut.GetUsdToEurAsync(cts.Token)) + .Should().ThrowAsync(); + + // Caller-initiated cancellation is not an FX failure, so the very next call must fetch again + // rather than sit out the negative-cache window. + decimal? rate = await sut.GetUsdToEurAsync(TestContext.CancellationToken); + + rate.Should().Be(0.92m); + handler.CallCount.Should().Be(2); + } + + [TestMethod] + public async Task GetUsdToEur_SuccessfulFetchAfterFailure_CachesTheRate() + { + var clock = new MutableClock(); + var handler = new SequencedHandler( + _ => throw new HttpRequestException("transient"), + _ => Ok(), + _ => throw new InvalidOperationException("the rate must not be fetched after it was cached")); + var sut = new FrankfurterFxRateProvider(new HttpClient(handler), new PricingOptions(), new NoOpAsyncLock(), clock); (await sut.GetUsdToEurAsync(TestContext.CancellationToken)).Should().BeNull(); + clock.Advance(PastNegativeCache); + (await sut.GetUsdToEurAsync(TestContext.CancellationToken)).Should().Be(0.92m); + + // The recovered rate is cached for the rest of the calendar day — a later call neither + // re-fetches nor is affected by the earlier failure. + clock.Advance(PastNegativeCache); + decimal? third = await sut.GetUsdToEurAsync(TestContext.CancellationToken); + + third.Should().Be(0.92m); + handler.CallCount.Should().Be(2); } + private static HttpResponseMessage Ok() => + new(HttpStatusCode.OK) { Content = new StringContent(RateBody, Encoding.UTF8, "application/json") }; + private sealed class NoOpAsyncLock : IAsyncLock { public IDisposable Lock(object key) => new Handle(); @@ -52,4 +147,23 @@ protected override Task SendAsync(HttpRequestMessage reques Content = new StringContent(body, Encoding.UTF8, "application/json"), }); } + + /// + /// Responds with a different scripted outcome per call, so a test can assert what happens across + /// repeated fetches (retry after failure, or no second fetch at all). + /// + private sealed class SequencedHandler(params Func[] responses) + : HttpMessageHandler + { + private int calls; + + public int CallCount => calls; + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken ct) + { + int index = Interlocked.Increment(ref calls) - 1; + Func response = responses[Math.Min(index, responses.Length - 1)]; + return Task.FromResult(response(ct)); + } + } } diff --git a/Proxytrace.Infrastructure.Tests/LiteLlmCatalogResolverTests.cs b/Proxytrace.Infrastructure.Tests/LiteLlmCatalogResolverTests.cs index 05677b7f3..1ff9b2d7f 100644 --- a/Proxytrace.Infrastructure.Tests/LiteLlmCatalogResolverTests.cs +++ b/Proxytrace.Infrastructure.Tests/LiteLlmCatalogResolverTests.cs @@ -22,12 +22,18 @@ public sealed class LiteLlmCatalogResolverTests } """; + /// + /// Comfortably longer than the resolver's (private) FailedFetchRetryInterval — advancing the test + /// clock by this expires the negative cache a failed fetch armed. + /// + private static readonly TimeSpan PastNegativeCache = TimeSpan.FromMinutes(5); + [TestMethod] public async Task Resolve_KnownModel_ConvertsUsdPerTokenToEurPer1M() { var fx = Substitute.For(); fx.GetUsdToEurAsync(Arg.Any()).Returns(0.9m); - var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For()); + var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For(), new MutableClock()); var price = await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); @@ -42,7 +48,7 @@ public async Task Resolve_ModelWithoutCachedPrice_LeavesCachedNull() { var fx = Substitute.For(); fx.GetUsdToEurAsync(Arg.Any()).Returns(1.0m); - var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For()); + var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For(), new MutableClock()); // azure/gpt-4o has input/output but no cache_read_input_token_cost. var price = await sut.ResolveAsync(["azure/gpt-4o"], TestContext.CancellationToken); @@ -56,7 +62,7 @@ public async Task Resolve_TriesCandidatesInOrder_FirstMatchWins() { var fx = Substitute.For(); fx.GetUsdToEurAsync(Arg.Any()).Returns(1.0m); - var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For()); + var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For(), new MutableClock()); // azure/gpt-4o (0.000003) precedes gpt-4o (0.0000025) → azure entry wins. var price = await sut.ResolveAsync(["azure/gpt-4o", "gpt-4o"], TestContext.CancellationToken); @@ -69,7 +75,7 @@ public async Task Resolve_FallsBackToLaterCandidate_WhenEarlierMissing() { var fx = Substitute.For(); fx.GetUsdToEurAsync(Arg.Any()).Returns(1.0m); - var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For()); + var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For(), new MutableClock()); // azure/gpt-5 is absent → falls back to gpt-4o. var price = await sut.ResolveAsync(["azure/gpt-5", "gpt-4o"], TestContext.CancellationToken); @@ -82,7 +88,7 @@ public async Task Resolve_UnknownModel_ReturnsUnknown() { var fx = Substitute.For(); fx.GetUsdToEurAsync(Arg.Any()).Returns(0.9m); - var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For()); + var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For(), new MutableClock()); var price = await sut.ResolveAsync(["does-not-exist"], TestContext.CancellationToken); @@ -94,7 +100,7 @@ public async Task Resolve_NoFxRate_ReturnsUnknown() { var fx = Substitute.For(); fx.GetUsdToEurAsync(Arg.Any()).Returns((decimal?)null); - var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For()); + var sut = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(HttpStatusCode.OK, Catalog)), new PricingOptions(), fx, Substitute.For(), new MutableClock()); var price = await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); @@ -106,21 +112,112 @@ public async Task Resolve_AfterFailedFetch_RetriesInsteadOfCachingTheEmptyCatalo { var fx = Substitute.For(); fx.GetUsdToEurAsync(Arg.Any()).Returns(1.0m); + var clock = new MutableClock(); var handler = new SequencedHandler( _ => throw new HttpRequestException("transient"), - _ => new HttpResponseMessage(HttpStatusCode.OK) - { Content = new StringContent(Catalog, Encoding.UTF8, "application/json") }); - var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For()); + _ => Ok()); + var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For(), clock); // First fetch fails: fail-soft to Unknown, but the empty result must not be cached... ModelPrice first = await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); - // ...so the next call re-fetches and picks the catalog up. + // ...so once the short negative cache expires the next call re-fetches and picks the catalog up. + clock.Advance(PastNegativeCache); ModelPrice second = await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); first.Should().Be(ModelPrice.Unknown); second.InputTokenCost.Should().Be(2.5m); } + [TestMethod] + public async Task Resolve_DuringCatalogOutage_AttemptsOnlyOneFetchForRepeatedCalls() + { + var fx = Substitute.For(); + fx.GetUsdToEurAsync(Arg.Any()).Returns(1.0m); + var handler = new SequencedHandler(_ => throw new HttpRequestException("catalog is down")); + var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For(), new MutableClock()); + + // A provider exposing many models resolves a price per model; the outage must not turn that + // into one outbound fetch attempt per model (#478). + for (int i = 0; i < 10; i++) + { + ModelPrice price = await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); + price.Should().Be(ModelPrice.Unknown); + } + + handler.CallCount.Should().Be(1); + } + + [TestMethod] + public async Task Resolve_AfterNegativeCacheExpires_RetriesTheFetch() + { + var fx = Substitute.For(); + fx.GetUsdToEurAsync(Arg.Any()).Returns(1.0m); + var clock = new MutableClock(); + var handler = new SequencedHandler(_ => throw new HttpRequestException("catalog is down")); + var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For(), clock); + + await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); + await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); + handler.CallCount.Should().Be(1, "the second call is still inside the negative-cache window"); + + clock.Advance(PastNegativeCache); + await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); + + handler.CallCount.Should().Be(2); + } + + [TestMethod] + public async Task Resolve_WhenCancelled_DoesNotArmTheNegativeCache() + { + var fx = Substitute.For(); + fx.GetUsdToEurAsync(Arg.Any()).Returns(1.0m); + using var cts = new CancellationTokenSource(); + var handler = new SequencedHandler( + ct => + { + cts.Cancel(); + ct.ThrowIfCancellationRequested(); + throw new InvalidOperationException("unreachable"); + }, + _ => Ok()); + var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For(), new MutableClock()); + + await FluentActions + .Invoking(() => sut.ResolveAsync(["gpt-4o"], cts.Token)) + .Should().ThrowAsync(); + + // Caller-initiated cancellation is not a catalog failure, so the very next call must fetch + // again rather than sit out the negative-cache window. + ModelPrice price = await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); + + price.InputTokenCost.Should().Be(2.5m); + handler.CallCount.Should().Be(2); + } + + [TestMethod] + public async Task Resolve_SuccessfulFetchAfterFailure_CachesTheCatalog() + { + var fx = Substitute.For(); + fx.GetUsdToEurAsync(Arg.Any()).Returns(1.0m); + var clock = new MutableClock(); + var handler = new SequencedHandler( + _ => throw new HttpRequestException("transient"), + _ => Ok(), + _ => throw new InvalidOperationException("catalog must not be fetched after it was cached")); + var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For(), clock); + + await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); + clock.Advance(PastNegativeCache); + await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); + // The recovered catalog is cached for good — a later call neither re-fetches nor is affected + // by the earlier failure. + clock.Advance(PastNegativeCache); + ModelPrice third = await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); + + third.InputTokenCost.Should().Be(2.5m); + handler.CallCount.Should().Be(2); + } + [TestMethod] public async Task Resolve_SuccessfulFetch_IsCachedAndNotRefetched() { @@ -130,7 +227,7 @@ public async Task Resolve_SuccessfulFetch_IsCachedAndNotRefetched() _ => new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent(Catalog, Encoding.UTF8, "application/json") }, _ => throw new InvalidOperationException("catalog must not be fetched twice")); - var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For()); + var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For(), new MutableClock()); await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); ModelPrice second = await sut.ResolveAsync(["gpt-4o"], TestContext.CancellationToken); @@ -151,13 +248,16 @@ public async Task Resolve_WhenCancelled_PropagatesCancellationInsteadOfEmptyCata ct.ThrowIfCancellationRequested(); throw new InvalidOperationException("unreachable"); }); - var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For()); + var sut = new LiteLlmCatalogResolver(new HttpClient(handler), new PricingOptions(), fx, Substitute.For(), new MutableClock()); await FluentActions .Invoking(() => sut.ResolveAsync(["gpt-4o"], cts.Token)) .Should().ThrowAsync(); } + private static HttpResponseMessage Ok() => + new(HttpStatusCode.OK) { Content = new StringContent(Catalog, Encoding.UTF8, "application/json") }; + private sealed class StubHandler(HttpStatusCode status, string body) : HttpMessageHandler { protected override Task SendAsync(HttpRequestMessage request, CancellationToken ct) => diff --git a/Proxytrace.Infrastructure.Tests/MutableClock.cs b/Proxytrace.Infrastructure.Tests/MutableClock.cs new file mode 100644 index 000000000..44d5a9ee2 --- /dev/null +++ b/Proxytrace.Infrastructure.Tests/MutableClock.cs @@ -0,0 +1,21 @@ +using Proxytrace.Common.Time; + +namespace Proxytrace.Infrastructure.Tests; + +/// +/// A test clock whose time can be advanced deterministically, so expiry-based behaviour can be +/// exercised without sleeping. Each test constructs its own instance — never share one. +/// +internal sealed class MutableClock : IClock +{ + public MutableClock() + : this(new DateTimeOffset(2026, 1, 1, 0, 0, 0, TimeSpan.Zero)) + { + } + + public MutableClock(DateTimeOffset start) => UtcNow = start; + + public DateTimeOffset UtcNow { get; set; } + + public void Advance(TimeSpan delta) => UtcNow += delta; +} diff --git a/Proxytrace.Infrastructure.Tests/PricingServiceTests.cs b/Proxytrace.Infrastructure.Tests/PricingServiceTests.cs index 518136b74..0889462b7 100644 --- a/Proxytrace.Infrastructure.Tests/PricingServiceTests.cs +++ b/Proxytrace.Infrastructure.Tests/PricingServiceTests.cs @@ -74,7 +74,7 @@ private static PricingService BuildService(decimal fx, string catalog = Catalog) { var fxProvider = Substitute.For(); fxProvider.GetUsdToEurAsync(Arg.Any()).Returns(fx); - var liteLlm = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(catalog)), new PricingOptions(), fxProvider, Substitute.For()); + var liteLlm = new LiteLlmCatalogResolver(new HttpClient(new StubHandler(catalog)), new PricingOptions(), fxProvider, Substitute.For(), new MutableClock()); return new PricingService(liteLlm); } diff --git a/Proxytrace.Infrastructure/Internal/FrankfurterFxRateProvider.cs b/Proxytrace.Infrastructure/Internal/FrankfurterFxRateProvider.cs index ec5de50cb..50a69ba06 100644 --- a/Proxytrace.Infrastructure/Internal/FrankfurterFxRateProvider.cs +++ b/Proxytrace.Infrastructure/Internal/FrankfurterFxRateProvider.cs @@ -1,47 +1,95 @@ using System.Text.Json; using Proxytrace.Common.Async; +using Proxytrace.Common.Time; using Proxytrace.Domain.ModelProvider; namespace Proxytrace.Infrastructure.Internal; -/// USD→EUR via the free, no-key Frankfurter (ECB) API. Cached for the calendar day. +/// +/// USD→EUR via the free, no-key Frankfurter (ECB) API. A successful rate is cached for the calendar +/// day; a failed fetch is not cached as a rate but suppresses further attempts for +/// . +/// internal sealed class FrankfurterFxRateProvider : IFxRateProvider { private const string CacheGateKey = "fx-rate:usd-eur"; + // How long a failed FX fetch suppresses further fetch attempts. Sized to be longer than a single + // model refresh — which resolves a price per discovered model, and every resolved price needs the + // FX rate, so during an outage it would otherwise queue one fetch attempt per model — while still + // short enough that an operator who retries after a blip sees real prices rather than a stale + // outage state (#487, mirroring #478 one layer up). + private static readonly TimeSpan FailedFetchRetryInterval = TimeSpan.FromSeconds(30); + private readonly HttpClient http; private readonly PricingOptions options; private readonly IAsyncLock asyncLock; - private decimal? cachedRate; - private DateOnly cachedOn; + private readonly IClock clock; + + // volatile: the double-checked fast path below reads this outside the lock; the write happens + // under the gate. Rate and day live in one immutable record so a lock-free reader can never see a + // fresh rate paired with a stale day — a decimal and a DateOnly are two non-atomic writes, a + // single reference is one. + private volatile CachedRate? cached; - public FrankfurterFxRateProvider(HttpClient http, PricingOptions options, IAsyncLock asyncLock) + // UTC ticks before which no fetch is attempted (0 = no failure recorded). Read on the lock-free + // fast path and written under the gate; a 64-bit field cannot be volatile, so it is accessed with + // Interlocked to keep the read atomic on 32-bit runtimes. + private long retryNotBeforeTicks; + + public FrankfurterFxRateProvider(HttpClient http, PricingOptions options, IAsyncLock asyncLock, IClock clock) { this.http = http; this.options = options; this.asyncLock = asyncLock; + this.clock = clock; } public async Task GetUsdToEurAsync(CancellationToken cancellationToken = default) { - DateOnly today = DateOnly.FromDateTime(DateTimeOffset.UtcNow.UtcDateTime); - if (cachedRate is not null && cachedOn == today) - return cachedRate; + DateOnly today = DateOnly.FromDateTime(clock.UtcNow.UtcDateTime); + + CachedRate? hit = cached; + if (hit is not null && hit.Day == today) + return hit.Rate; + + // Outage fast path: bail out before queueing on the gate, so a refresh that resolves a price + // per discovered model does not serialize one fetch attempt per model behind the gate. + if (IsRetrySuppressed()) + return null; using IDisposable sync = await asyncLock.LockAsync(CacheGateKey, cancellationToken); - if (cachedRate is not null && cachedOn == today) - return cachedRate; + hit = cached; + if (hit is not null && hit.Day == today) + return hit.Rate; + if (IsRetrySuppressed()) + return null; decimal? rate = await FetchAsync(cancellationToken); - if (rate is not null) + + // Only a fetch that actually produced a rate is cached for the day. A failed fetch instead + // arms a short *negative* cache: for the next FailedFetchRetryInterval callers get null + // without a fetch, then exactly one caller retries. That keeps the recover-after-a-blip + // property while collapsing a refresh of a provider's N models into a single outbound attempt + // per interval (#487). A caller-cancelled fetch never reaches here — FetchAsync rethrows — so + // cancellation does not arm the negative cache. + if (rate is null) { - cachedRate = rate; - cachedOn = today; + Interlocked.Exchange(ref retryNotBeforeTicks, clock.UtcNow.Add(FailedFetchRetryInterval).UtcTicks); + return null; } + + cached = new CachedRate(rate.Value, today); return rate; } + private bool IsRetrySuppressed() + { + long notBefore = Interlocked.Read(ref retryNotBeforeTicks); + return notBefore > 0 && clock.UtcNow.UtcTicks < notBefore; + } + private async Task FetchAsync(CancellationToken cancellationToken) { try @@ -61,9 +109,23 @@ public FrankfurterFxRateProvider(HttpClient http, PricingOptions options, IAsync } return null; } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + // Caller-initiated cancellation is not an FX failure — never swallow it into a null rate + // that would arm the negative cache and sit out the retry window for everyone else. + throw; + } catch { + // fail-soft: no rate → callers get ModelPrice.Unknown. GetUsdToEurAsync never caches it as + // the day's rate; it arms a short negative cache and retries once that expires. return null; } } + + /// + /// The rate cached for a given calendar day, held as one immutable reference so the lock-free + /// fast path always observes a consistent (rate, day) pair. + /// + private sealed record CachedRate(decimal Rate, DateOnly Day); } diff --git a/Proxytrace.Infrastructure/Internal/LiteLlmCatalogResolver.cs b/Proxytrace.Infrastructure/Internal/LiteLlmCatalogResolver.cs index 8662c7994..5dab5a4f3 100644 --- a/Proxytrace.Infrastructure/Internal/LiteLlmCatalogResolver.cs +++ b/Proxytrace.Infrastructure/Internal/LiteLlmCatalogResolver.cs @@ -1,16 +1,25 @@ +using System.Collections.ObjectModel; using System.Text.Json; using Proxytrace.Common.Async; +using Proxytrace.Common.Time; using Proxytrace.Domain.ModelProvider; namespace Proxytrace.Infrastructure.Internal; /// /// Resolves prices from the LiteLLM catalog (USD per token), converting to EUR / 1M tokens via the -/// FX provider. The catalog is fetched once and cached in memory. Used for all provider kinds — -/// Azure providers pass an azure/<model> candidate ahead of the bare model name. +/// FX provider. The catalog is fetched once and cached in memory; a failed fetch is not cached but +/// suppresses further attempts for . Used for all provider +/// kinds — Azure providers pass an azure/<model> candidate ahead of the bare model name. /// internal sealed class LiteLlmCatalogResolver { + // How long a failed (or empty) catalog fetch suppresses further fetch attempts. Sized to be + // longer than a single model refresh — which resolves a price per discovered model, so during an + // outage it would otherwise queue one fetch attempt per model — while still short enough that an + // operator who retries after a blip sees real prices rather than a stale outage state (#478). + private static readonly TimeSpan FailedFetchRetryInterval = TimeSpan.FromSeconds(30); + // Instance-scoped lock key (the resolver is a singleton). A per-instance Guid is not a constant, // so it must not be a static field; it only needs to be stable for the lifetime of this resolver // to serialize the one-shot catalog fetch. @@ -20,21 +29,29 @@ internal sealed class LiteLlmCatalogResolver private readonly PricingOptions options; private readonly IFxRateProvider fxRateProvider; private readonly IAsyncLock gate; + private readonly IClock clock; // volatile: the double-checked fast path below reads this outside the lock; the write happens // under the gate. volatile guarantees the freshly-fetched catalog is visible to those lock-free // reads without tearing. private volatile IReadOnlyDictionary? cache; + // UTC ticks before which no fetch is attempted (0 = no failure recorded). Read on the lock-free + // fast path and written under the gate; a 64-bit field cannot be volatile, so it is accessed with + // Interlocked to keep the read atomic on 32-bit runtimes. + private long retryNotBeforeTicks; + public LiteLlmCatalogResolver( HttpClient http, PricingOptions options, IFxRateProvider fxRateProvider, - IAsyncLock gate) + IAsyncLock gate, + IClock clock) { this.http = http; this.options = options; this.fxRateProvider = fxRateProvider; this.gate = gate; + this.clock = clock; } /// @@ -80,21 +97,45 @@ public async Task ResolveAsync( if (cache is not null) return cache; + // Outage fast path: bail out before queueing on the gate, so a refresh that resolves a price + // per discovered model does not serialize one fetch attempt per model behind the gate. + if (IsRetrySuppressed()) + return ReadOnlyDictionary.Empty; + using var _ = await gate.LockAsync(lockKey, cancellationToken); if (cache is not null) return cache; + if (IsRetrySuppressed()) + return ReadOnlyDictionary.Empty; IReadOnlyDictionary fetched = await FetchAsync(cancellationToken); - // Only a fetch that actually produced entries is cached. A failed or empty fetch is left - // uncached so the next caller retries — caching it would pin every model price to - // ModelPrice.Unknown for the rest of the process lifetime after a single network blip. - if (fetched.Count > 0) - cache = fetched; + // Only a fetch that actually produced entries is cached. A failed or empty fetch is never + // cached as the catalog — that would pin every model price to ModelPrice.Unknown for the rest + // of the process lifetime after a single network blip. It instead arms a short *negative* + // cache: for the next FailedFetchRetryInterval callers get ModelPrice.Unknown without a + // fetch, then exactly one caller retries. That keeps the recover-after-a-blip property while + // collapsing a refresh of a provider's N models into a single outbound attempt per interval + // (#478). A caller-cancelled fetch never reaches here — FetchAsync rethrows — so cancellation + // does not arm the negative cache. + if (fetched.Count == 0) + { + Interlocked.Exchange(ref retryNotBeforeTicks, clock.UtcNow.Add(FailedFetchRetryInterval).UtcTicks); + return fetched; + } + // Once the catalog is cached both checks above short-circuit, so a stale suppression + // timestamp can never be read again. + cache = fetched; return fetched; } + private bool IsRetrySuppressed() + { + long notBefore = Interlocked.Read(ref retryNotBeforeTicks); + return notBefore > 0 && clock.UtcNow.UtcTicks < notBefore; + } + private async Task> FetchAsync( CancellationToken cancellationToken) { @@ -125,8 +166,8 @@ public async Task ResolveAsync( } catch { - // fail-soft: empty catalog → callers get ModelPrice.Unknown, and GetCatalogAsync - // deliberately does not cache it, so the next call retries. + // fail-soft: empty catalog → callers get ModelPrice.Unknown. GetCatalogAsync never caches + // it as the catalog; it arms a short negative cache and retries once that expires. return new Dictionary(StringComparer.OrdinalIgnoreCase); } diff --git a/Proxytrace.Proxy.Tests/FakeHttpClients.cs b/Proxytrace.Proxy.Tests/FakeHttpClients.cs index 148cf2516..3ba0f5fe1 100644 --- a/Proxytrace.Proxy.Tests/FakeHttpClients.cs +++ b/Proxytrace.Proxy.Tests/FakeHttpClients.cs @@ -214,8 +214,9 @@ public override void Flush() { } } /// -/// A response stream that records everything written to it plus the size of the largest single write, -/// so a test can assert the proxy forwarded a body in bounded pieces instead of materializing it whole. +/// A response stream that records everything written to it, the size of the largest single write and +/// how many writes there were — so a test can assert the proxy forwarded a body in bounded pieces +/// instead of materializing it whole, and that it forwarded event by event instead of in one batch. /// internal sealed class RecordingResponseStream : Stream { @@ -223,6 +224,9 @@ internal sealed class RecordingResponseStream : Stream public int LargestWriteBytes { get; private set; } + /// Number of individual writes — one per forwarded SSE segment on the streaming path. + public int WriteCount { get; private set; } + public byte[] Written => written.ToArray(); public override bool CanRead => false; @@ -234,12 +238,14 @@ internal sealed class RecordingResponseStream : Stream public override async ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) { LargestWriteBytes = Math.Max(LargestWriteBytes, buffer.Length); + WriteCount++; await written.WriteAsync(buffer, cancellationToken); } public override void Write(byte[] buffer, int offset, int count) { LargestWriteBytes = Math.Max(LargestWriteBytes, count); + WriteCount++; written.Write(buffer, offset, count); } @@ -291,6 +297,93 @@ public override void Flush() { } public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); } +/// +/// Serves a response whose headers arrive immediately but whose body never produces a byte: every read +/// blocks until the caller's token trips. Stands in for the slow-loris upstream of #475 — the case +/// stops covering once ResponseHeadersRead has returned. The +/// client's timeout is settable so a test can drive the proxy's own body bound in milliseconds. +/// +internal sealed class StallingBodyHttpClientFactory : IHttpClientFactory +{ + private readonly TimeSpan timeout; + private readonly CancellationTokenSource? cancelOnFirstRead; + + // `timeout` is the client timeout — which is also the budget the proxy applies to the body copy. + // `cancelOnFirstRead`, when given, is cancelled the instant the proxy asks for the first body byte: + // that models a client disconnecting while the proxy waits on the upstream, with no sleep and no race. + public StallingBodyHttpClientFactory(TimeSpan timeout, CancellationTokenSource? cancelOnFirstRead = null) + { + this.timeout = timeout; + this.cancelOnFirstRead = cancelOnFirstRead; + } + + public HttpClient CreateClient(string name) => new(new Handler(cancelOnFirstRead)) + { + BaseAddress = new Uri("http://fake-upstream/"), + Timeout = timeout, + }; + + private sealed class Handler : HttpMessageHandler + { + private readonly CancellationTokenSource? cancelOnFirstRead; + + public Handler(CancellationTokenSource? cancelOnFirstRead) => this.cancelOnFirstRead = cancelOnFirstRead; + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + => Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StreamContent(new StallingStream(cancelOnFirstRead)), + }); + } +} + +/// A read-only stream whose reads never complete until the token they were given is cancelled. +internal sealed class StallingStream : Stream +{ + private readonly CancellationTokenSource? cancelOnFirstRead; + private bool cancelled; + + public StallingStream(CancellationTokenSource? cancelOnFirstRead = null) + => this.cancelOnFirstRead = cancelOnFirstRead; + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + public override long Position { get => 0; set => throw new NotSupportedException(); } + + public override async ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + { + SignalFirstRead(); + await Task.Delay(Timeout.Infinite, cancellationToken); + return 0; + } + + public override async Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + SignalFirstRead(); + await Task.Delay(Timeout.Infinite, cancellationToken); + return 0; + } + + private void SignalFirstRead() + { + if (cancelOnFirstRead is null || cancelled) + { + return; + } + + cancelled = true; + cancelOnFirstRead.Cancel(); + } + + public override void Flush() { } + public override int Read(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); +} + /// A response stream that fails every write — simulates a client that disconnected. internal sealed class ThrowOnWriteStream : Stream { diff --git a/Proxytrace.Proxy.Tests/OpenAiProxyControllerTests.cs b/Proxytrace.Proxy.Tests/OpenAiProxyControllerTests.cs index be549d355..c08b1777e 100644 --- a/Proxytrace.Proxy.Tests/OpenAiProxyControllerTests.cs +++ b/Proxytrace.Proxy.Tests/OpenAiProxyControllerTests.cs @@ -505,6 +505,128 @@ public async Task Proxy_StreamingSseWithCrlf_NormalizesToLf_AndCapturesTranscrip captured?.ResponseBody.Should().Be(expected, "the captured transcript mirrors what was forwarded"); } + [TestMethod] + public async Task Proxy_StreamingSseWithCrOnlyLineEndings_ForwardsEachEventAsItArrives() + { + // Regression for #480: the chunk splitter that replaced ReadLineAsync split only on '\n'. SSE + // also permits a lone '\r' as a line terminator — ReadLineAsync honoured it — so a CR-only + // event stream was held in `pending` until the 256 KiB flush threshold or EOF and reached the + // client in one batch instead of event by event, defeating streaming. One byte per read, so + // every terminator also lands on a chunk boundary. + const string upstreamBody = "data: {\"a\":1}\r\rdata: [DONE]\r"; + const string expected = "data: {\"a\":1}\n\ndata: [DONE]\n"; + + IngestMessage? captured = null; + var stream = Substitute.For(); + stream.PublishAsync(Arg.Do(m => captured = m), Arg.Any()) + .Returns(Task.CompletedTask); + + var responseBody = new RecordingResponseStream(); + var controller = BuildController( + stream, + ResolverFor(ApiKey()), + new ChunkedRawHttpClientFactory(Encoding.UTF8.GetBytes(upstreamBody), maxBytesPerRead: 1)); + controller.ControllerContext = BuildContext( + "Bearer valid", body: """{"model":"gpt-4o","stream":true,"messages":[]}"""); + controller.ControllerContext.HttpContext.Response.Body = responseBody; + + await controller.Proxy("chat/completions", project: null, CancellationToken.None); + + Encoding.UTF8.GetString(responseBody.Written).Should().Be( + expected, "a lone CR terminates a line and is normalized to LF, exactly as ReadLine did"); + responseBody.WriteCount.Should().Be( + 3, "each CR-terminated event is forwarded as it arrives, not batched up at EOF"); + captured.Should().NotBeNull(); + captured?.ResponseBody.Should().Be(expected, "the captured transcript mirrors what was forwarded"); + } + + [TestMethod] + public async Task Proxy_StreamingSseCrlfSplitAcrossChunkBoundary_CountsTheTerminatorOnce() + { + // The subtle half of #480: with '\r' now terminating a line, a CRLF whose '\r' ends one read + // and whose '\n' starts the next must still be ONE terminator. Counting it twice would inject + // a spurious empty line into every event of a CRLF stream. One byte per read puts the seam + // between the CR and the LF deterministically. + const string upstreamBody = "data: a\r\ndata: b\r\n"; + const string expected = "data: a\ndata: b\n"; + + var responseBody = new RecordingResponseStream(); + var controller = BuildController( + Substitute.For(), + ResolverFor(ApiKey()), + new ChunkedRawHttpClientFactory(Encoding.UTF8.GetBytes(upstreamBody), maxBytesPerRead: 1)); + controller.ControllerContext = BuildContext( + "Bearer valid", body: """{"model":"gpt-4o","stream":true,"messages":[]}"""); + controller.ControllerContext.HttpContext.Response.Body = responseBody; + + await controller.Proxy("chat/completions", project: null, CancellationToken.None); + + Encoding.UTF8.GetString(responseBody.Written).Should().Be(expected); + responseBody.WriteCount.Should().Be( + 2, "a CRLF split across two reads is one line terminator, not two"); + } + + [TestMethod] + public async Task Proxy_BufferedUpstreamStallsAfterHeaders_AbortsAtTheClientTimeout_AndRecords504() + { + // Regression for #475: the buffered branch reads with ResponseHeadersRead, and + // HttpClient.Timeout stops applying the moment the headers are in — so an upstream that sends + // headers and then stalls held the request, a socket and a thread-pool continuation open until + // the *client* gave up. The copy loop now carries the bound itself, sourced from the same + // HttpClient timeout; this test proves the wiring by shortening that timeout — a hardcoded + // five minutes in the controller would hang here instead. + IngestMessage? captured = null; + var stream = Substitute.For(); + stream.PublishAsync(Arg.Do(m => captured = m), Arg.Any()) + .Returns(Task.CompletedTask); + + var controller = BuildController( + stream, + ResolverFor(ApiKey()), + new StallingBodyHttpClientFactory(TimeSpan.FromSeconds(1))); + controller.ControllerContext = BuildContext("Bearer valid", body: """{"model":"gpt-4o","messages":[]}"""); + + await controller.Proxy("chat/completions", project: null, CancellationToken.None); + + controller.Response.StatusCode.Should().Be( + StatusCodes.Status504GatewayTimeout, "a stalled upstream body is a gateway timeout, not a hang"); + captured.Should().NotBeNull(); + captured?.HttpStatus.Should().Be( + StatusCodes.Status504GatewayTimeout, "the trace records the timeout rather than upstream's 200"); + } + + [TestMethod] + public async Task Proxy_BufferedUpstreamStalls_WhenClientDisconnects_PropagatesCancellation_NotATimeout() + { + // The other side of #475: the body bound must stay distinguishable from a client abort. With a + // long upstream budget and the *request* token tripping, the cancellation propagates exactly as + // it did before (there is nobody left to answer) instead of being reported as a 504. + IngestMessage? captured = null; + var stream = Substitute.For(); + stream.PublishAsync(Arg.Do(m => captured = m), Arg.Any()) + .Returns(Task.CompletedTask); + + // Cancelled the moment the proxy asks for the first body byte — a client that disconnects while + // the proxy waits on a stalled upstream, with the five-minute body budget nowhere near tripping. + using var clientGoneAway = new CancellationTokenSource(); + + var controller = BuildController( + stream, + ResolverFor(ApiKey()), + new StallingBodyHttpClientFactory(TimeSpan.FromMinutes(5), clientGoneAway)); + controller.ControllerContext = BuildContext("Bearer valid", body: """{"model":"gpt-4o","messages":[]}"""); + + await FluentActions + .Awaiting(() => controller.Proxy("chat/completions", project: null, clientGoneAway.Token)) + .Should().ThrowAsync(); + + controller.Response.StatusCode.Should().Be( + (int)HttpStatusCode.OK, "a client abort must not be rewritten into an upstream timeout"); + captured.Should().NotBeNull(); + captured?.HttpStatus.Should().Be( + (int)HttpStatusCode.OK, "the partial capture keeps upstream's own status on a client abort"); + } + private static OpenAiProxyController BuildController( IIngestionStream stream, IApiKeyResolver resolver, diff --git a/Proxytrace.Proxy/Controllers/OpenAiProxyController.cs b/Proxytrace.Proxy/Controllers/OpenAiProxyController.cs index fa0242689..92a1ff7d4 100644 --- a/Proxytrace.Proxy/Controllers/OpenAiProxyController.cs +++ b/Proxytrace.Proxy/Controllers/OpenAiProxyController.cs @@ -92,7 +92,7 @@ public class OpenAiProxyController : ControllerBase // Response side of the same transparency rule: every upstream header is relayed to the client // EXCEPT hop-by-hop headers and the framing headers Kestrel must own. Content-Length is dropped - // because the streaming path normalizes CRLF line endings to LF, so the relayed byte count can + // because the streaming path normalizes CRLF/CR line endings to LF, so the relayed byte count can // legitimately differ from upstream's. private static readonly IReadOnlyCollection StrippedResponseHeaders = new HashSet( [ @@ -212,6 +212,11 @@ await RejectBlockedRequestAsync( // (bounded only by HttpClient.MaxResponseContentBufferSize, ~2 GB by default) — which is // exactly the unbounded residency ProxyBufferedResponseAsync's chunked copy exists to // avoid. Both paths read the body as a stream, so neither needs the eager buffer. + // + // The price is that HttpClient.Timeout stops applying the moment the headers land, so it + // no longer bounds the body download (#475). ProxyBufferedResponseAsync restores that + // bound at its copy loop, using this client's own configured timeout — read off the + // instance rather than re-declared here so the two can never drift apart. upstreamResponse = await client.SendAsync(upstream, HttpCompletionOption.ResponseHeadersRead, cancellationToken); } catch (Exception ex) @@ -233,7 +238,7 @@ await RejectBlockedRequestAsync( } else { - await ProxyBufferedResponseAsync(resolved.Provider, resolved.Project, requestBody, upstreamResponse, sw, sessionId, conversationId, agentName, cancellationToken); + await ProxyBufferedResponseAsync(resolved.Provider, resolved.Project, requestBody, upstreamResponse, sw, sessionId, conversationId, agentName, client.Timeout, cancellationToken); } } } @@ -585,6 +590,7 @@ private async Task ProxyBufferedResponseAsync( string? sessionId, string? conversationId, string? agentName, + TimeSpan upstreamBodyTimeout, CancellationToken cancellationToken) { // Stream the upstream body straight through to the client instead of materializing it as a @@ -599,14 +605,28 @@ private async Task ProxyBufferedResponseAsync( var buffer = ArrayPool.Shared.Rent(CopyChunkBytes); var chars = ArrayPool.Shared.Rent(Encoding.UTF8.GetMaxCharCount(buffer.Length)); + // #475: HttpClient.Timeout covers only up to the response headers, and the SendAsync above + // deliberately returns at that point (ResponseHeadersRead), so nothing upstream of here bounds + // the body download — SocketsHttpHandler has no read-timeout knob either. Without this an + // upstream that sends headers and then stalls pins a proxy request, a socket and a thread-pool + // continuation until the *client* gives up, because `cancellationToken` is RequestAborted. + // The budget is the "openai" client's own timeout (Proxytrace.Proxy/Module.cs), passed in + // rather than re-declared, so the header phase and the body phase can never drift apart. + using var bodyTimeout = new CancellationTokenSource(upstreamBodyTimeout); + using var linkedTokens = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, bodyTimeout.Token); + var bodyToken = linkedTokens.Token; + + // What gets recorded for the call: upstream's own status, unless we cut the body short. + var capturedStatus = upstreamResponse.StatusCode; + try { - await using var upstreamStream = await upstreamResponse.Content.ReadAsStreamAsync(cancellationToken); + await using var upstreamStream = await upstreamResponse.Content.ReadAsStreamAsync(bodyToken); int read; - while ((read = await upstreamStream.ReadAsync(buffer.AsMemory(), cancellationToken)) > 0) + while ((read = await upstreamStream.ReadAsync(buffer.AsMemory(), bodyToken)) > 0) { - await Response.Body.WriteAsync(buffer.AsMemory(0, read), cancellationToken); + await Response.Body.WriteAsync(buffer.AsMemory(0, read), bodyToken); // Bound the captured copy: decode this chunk and append only while under the cap. The // Decoder carries a multi-byte UTF-8 sequence split across a chunk boundary between @@ -618,6 +638,26 @@ private async Task ProxyBufferedResponseAsync( } } } + // Our timeout only, never a client abort: the filter requires that the *body* budget tripped + // and that the request token did not, so a disconnecting client still propagates its + // cancellation exactly as before (there is nobody left to answer) while a stalled upstream is + // reported — to the client and to the trace — as the gateway timeout it is. + catch (OperationCanceledException) when (bodyTimeout.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + { + capturedStatus = HttpStatusCode.GatewayTimeout; + logger.LogWarning( + "Upstream stalled while sending the response body; aborted after {TimeoutSeconds:F0}s", + upstreamBodyTimeout.TotalSeconds); + + // The status is only ours to set while nothing has reached the wire. Once the first bytes + // are out, upstream's status stands and the client gets a truncated body — but the trace + // published below still records the timeout either way. + if (!Response.HasStarted) + { + Response.Clear(); + Response.StatusCode = StatusCodes.Status504GatewayTimeout; + } + } finally { ArrayPool.Shared.Return(chars); @@ -627,7 +667,7 @@ private async Task ProxyBufferedResponseAsync( // Capture is decoupled from the client request lifetime: the upstream call has already // completed, so a client disconnect/timeout here must not drop the captured call. // Publish with CancellationToken.None rather than the request-aborted token. - await EnqueueSafeAsync(provider, project, requestBody, captured.ToString(), sw.Elapsed, upstreamResponse.StatusCode, sessionId, conversationId, agentName, CancellationToken.None); + await EnqueueSafeAsync(provider, project, requestBody, captured.ToString(), sw.Elapsed, capturedStatus, sessionId, conversationId, agentName, CancellationToken.None); } } @@ -649,34 +689,68 @@ private async Task ProxyStreamingResponseAsync( await using var upstreamStream = await upstreamResponse.Content.ReadAsStreamAsync(cancellationToken); using var reader = new StreamReader(upstreamStream, Encoding.UTF8, leaveOpen: true); - // Read fixed-size chunks and split on '\n' ourselves rather than calling ReadLineAsync, which - // grows an unbounded internal buffer until it finds a newline. Whether the response is really - // an event stream is decided by the *request* (`"stream": true`), never by the upstream, so a - // provider that ignores the flag — or a WAF/error page in front of it — can answer with one - // multi-megabyte single-line body, which ReadLineAsync would materialize whole as a string and - // WriteSseSegmentAsync would then rent 3x its length on top of. A line that grows past - // MaxForwardedLineChars is flushed as a raw, unterminated segment instead: the client still - // receives every byte, but nothing unbounded is ever resident. + // Read fixed-size chunks and split on the line terminator ourselves rather than calling + // ReadLineAsync, which grows an unbounded internal buffer until it finds one. Whether the + // response is really an event stream is decided by the *request* (`"stream": true`), never by + // the upstream, so a provider that ignores the flag — or a WAF/error page in front of it — can + // answer with one multi-megabyte single-line body, which ReadLineAsync would materialize whole + // as a string and WriteSseSegmentAsync would then rent 3x its length on top of. A line that + // grows past MaxForwardedLineChars is flushed as a raw, unterminated segment instead: the + // client still receives every byte, but nothing unbounded is ever resident. var chunk = ArrayPool.Shared.Rent(StreamReadChunkChars); var pending = new StringBuilder(); + // Carries one bit of splitter state across a chunk boundary: the previous chunk ended on a + // '\r' that terminated a line, so a '\n' opening this chunk is the second half of that same + // CRLF and must be swallowed rather than read as a second, empty line (#480). + var pendingCarriageReturn = false; + try { int read; while ((read = await reader.ReadAsync(chunk.AsMemory(0, StreamReadChunkChars), cancellationToken)) > 0) { var start = 0; + if (pendingCarriageReturn) + { + pendingCarriageReturn = false; + if (chunk[0] == '\n') + { + start = 1; + } + } + while (start < read) { - var newline = Array.IndexOf(chunk, '\n', start, read - start); - if (newline < 0) + // SSE terminates a line with LF, CRLF *or* a lone CR, and the ReadLineAsync this + // loop replaced honoured all three — so split on either character (#480). A + // CR-only stream that is only split on '\n' is held back until the + // MaxForwardedLineChars flush or EOF, which defeats incremental delivery. + var offset = chunk.AsSpan(start, read - start).IndexOfAny('\r', '\n'); + if (offset < 0) { pending.Append(chunk, start, read - start); break; } - pending.Append(chunk, start, newline - start); - start = newline + 1; + var terminator = start + offset; + pending.Append(chunk, start, terminator - start); + start = terminator + 1; + + // CRLF is one terminator, not two. The '\n' may be the first character of the + // *next* chunk, in which case the flag above carries the state across the seam. + if (chunk[terminator] == '\r') + { + if (start >= read) + { + pendingCarriageReturn = true; + } + else if (chunk[start] == '\n') + { + start++; + } + } + await EmitSseSegmentAsync(pending, accumulated, terminated: true, cancellationToken); } @@ -708,21 +782,17 @@ private async Task ProxyStreamingResponseAsync( // ── Helpers ─────────────────────────────────────────────────────────────── // Drains the pending segment: appends it to the bounded capture buffer and forwards it to the - // client. `terminated` distinguishes a real line (a '\n' was seen upstream, so one is written and - // a trailing '\r' is dropped — the CRLF→LF normalization StreamReader.ReadLine used to do) from a - // length-forced flush of an over-long line, which is written raw so the forwarded bytes stay - // faithful and no newline is invented. + // client. `terminated` distinguishes a real line (a terminator was seen upstream — LF, CRLF or a + // lone CR, all written back as the single '\n' StreamReader.ReadLine used to normalize them to) + // from a length-forced flush of an over-long line, which is written raw so the forwarded bytes + // stay faithful and no newline is invented. The splitter consumes the terminator characters + // themselves, so `pending` never carries one into here. private async Task EmitSseSegmentAsync( StringBuilder pending, StringBuilder accumulated, bool terminated, CancellationToken cancellationToken) { - if (terminated && pending.Length > 0 && pending[^1] == '\r') - { - pending.Length--; - } - var segment = pending.ToString(); pending.Clear(); diff --git a/Proxytrace.Proxy/Module.cs b/Proxytrace.Proxy/Module.cs index 0552c8eab..ff4bee30b 100644 --- a/Proxytrace.Proxy/Module.cs +++ b/Proxytrace.Proxy/Module.cs @@ -58,7 +58,11 @@ protected override void Load(ContainerBuilder builder) { services.AddMemoryCache(); // The upstream target is per-request (provider endpoint), so only the timeout matters - // here. Auto-redirect is off for the same reason as the pass-through client below: a + // here. That timeout is also the budget OpenAiProxyController applies to the *body* + // download: it reads with ResponseHeadersRead (to avoid buffering the whole reply), and + // HttpClient.Timeout stops applying once the headers land, so the controller re-arms this + // same value on its copy loop (#475). Changing it here changes both phases. + // Auto-redirect is off for the same reason as the pass-through client below: a // transparent proxy relays a 3xx to the client rather than chasing it server-side. The // BCL's redirect handler only clears the typed Authorization header on a cross-origin // hop — the Azure `api-key` header (BuildUpstreamRequest) and every blanket-forwarded diff --git a/Proxytrace.Storage.Tests/AgentCallStatsQueriesTests.cs b/Proxytrace.Storage.Tests/AgentCallStatsQueriesTests.cs index 0278d7836..e41201f58 100644 --- a/Proxytrace.Storage.Tests/AgentCallStatsQueriesTests.cs +++ b/Proxytrace.Storage.Tests/AgentCallStatsQueriesTests.cs @@ -6,8 +6,11 @@ using Proxytrace.Domain.Agent; using Proxytrace.Domain.AgentCall; using Proxytrace.Domain.Completion; +using Proxytrace.Domain.Inference; using Proxytrace.Domain.Message; using Proxytrace.Domain.ModelEndpoint; +using Proxytrace.Domain.Project; +using Proxytrace.Domain.Prompt; using Proxytrace.Domain.Usage; using Proxytrace.Testing; @@ -235,6 +238,106 @@ private async Task SeedSystemAgentCall(IServiceProvider services) return systemAgent.Id; } + // ── multi-project scope (#483) ───────────────────────────────────────────── + // + // The LINQ chokepoint Query() is what these exercise: on the in-memory provider the latency + // percentiles fall back to it too, so the raw-SQL twin is covered separately by + // StatisticsFilterWhereTests (its WHERE fragment) and StatisticsFilterParityTests (that both + // paths know every filter member). + + [TestMethod] + public async Task GetSummary_ScopedToSeveralProjects_CountsOnlyThoseProjectsCalls() + { + IServiceProvider services = GetServices(); + var reader = services.GetRequiredService(); + var first = await SeedAgentInNewProjectAsync(services, "first"); + var second = await SeedAgentInNewProjectAsync(services, "second"); + var outsider = await SeedAgentInNewProjectAsync(services, "outsider"); + await SeedPlainCallAsync(services, first); + await SeedPlainCallAsync(services, second); + await SeedPlainCallAsync(services, outsider); + + var summary = await reader.GetSummaryAsync( + new StatisticsFilter(ProjectIds: [first.Project.Id, second.Project.Id]), CancellationToken); + + summary.TotalCalls.Should().Be(2); + } + + [TestMethod] + public async Task GetAgentBreakdown_ScopedToSeveralProjects_ListsOnlyThoseProjectsAgents() + { + IServiceProvider services = GetServices(); + var reader = services.GetRequiredService(); + var first = await SeedAgentInNewProjectAsync(services, "first"); + var second = await SeedAgentInNewProjectAsync(services, "second"); + var outsider = await SeedAgentInNewProjectAsync(services, "outsider"); + await SeedPlainCallAsync(services, first); + await SeedPlainCallAsync(services, second); + await SeedPlainCallAsync(services, outsider); + + var rows = await reader.GetAgentBreakdownAsync( + new StatisticsFilter(ProjectIds: [first.Project.Id, second.Project.Id]), CancellationToken); + + rows.Select(r => r.AgentId).Should().BeEquivalentTo(new[] { first.Id, second.Id }); + } + + [TestMethod] + public async Task GetLatency_ScopedToSeveralProjects_SamplesOnlyThoseProjectsCalls() + { + IServiceProvider services = GetServices(); + var reader = services.GetRequiredService(); + var first = await SeedAgentInNewProjectAsync(services, "first"); + var second = await SeedAgentInNewProjectAsync(services, "second"); + var outsider = await SeedAgentInNewProjectAsync(services, "outsider"); + await SeedPlainCallAsync(services, first); + await SeedPlainCallAsync(services, second); + await SeedPlainCallAsync(services, outsider); + + var rows = await reader.GetLatencyAsync( + new StatisticsFilter(ProjectIds: [first.Project.Id, second.Project.Id]), CancellationToken); + + rows.Sum(r => r.SampleCount).Should().Be(2); + } + + [TestMethod] + public async Task GetSummary_ScopedToAnEmptyProjectSet_IsNotRestricted() + { + // An empty set means "no set filter", not "no rows" — endpoints short-circuit an empty scope + // before they ever build a filter (ProjectListScope.IsEmpty), and both filter paths mirror + // AgentCallFilter in ignoring a zero-length set. + IServiceProvider services = GetServices(); + var reader = services.GetRequiredService(); + await services.GetRequiredService>().CreateAsync(CancellationToken); + + var summary = await reader.GetSummaryAsync(new StatisticsFilter(ProjectIds: []), CancellationToken); + + summary.TotalCalls.Should().Be(1); + } + + /// An agent in a project of its own, so a test can tell two tenants' rows apart. + private async Task SeedAgentInNewProjectAsync(IServiceProvider services, string name) + { + var endpoint = await services.GetRequiredService>() + .GetOrCreateAsync(CancellationToken); + var project = await services.GetRequiredService().AddAsync( + services.GetRequiredService()($"P-{name}-{Guid.NewGuid():N}", endpoint, []), + CancellationToken); + var template = services.GetRequiredService()($"T-{name}", "You are a test agent."); + var parameters = services.GetRequiredService()(null, null, null, null, null); + + return await services.GetRequiredService().AddAsync( + services.GetRequiredService()($"A-{name}", template, [], endpoint, project, parameters), + CancellationToken); + } + + private async Task SeedPlainCallAsync(IServiceProvider services, IAgent agent) + { + AssistantMessage sample = (await services.GetRequiredService>() + .CreateAsync(CancellationToken)).Response; + return await SeedCallAsync(services, agent, agent.Endpoint, sample, conversationId: null, + new TokenUsage(100, 50, 0), latencyMs: 120, toolCount: 0, HttpStatusCode.OK); + } + [TestMethod] public async Task GetSummary_FilteredByUnknownAgent_ReturnsZeros() { diff --git a/Proxytrace.Storage.Tests/EvaluatorStatsQueriesTests.cs b/Proxytrace.Storage.Tests/EvaluatorStatsQueriesTests.cs index fdcd0d018..6748d505e 100644 --- a/Proxytrace.Storage.Tests/EvaluatorStatsQueriesTests.cs +++ b/Proxytrace.Storage.Tests/EvaluatorStatsQueriesTests.cs @@ -6,6 +6,7 @@ using Proxytrace.Domain.Completion; using Proxytrace.Domain.Evaluation; using Proxytrace.Domain.Evaluator; +using Proxytrace.Domain.Project; using Proxytrace.Domain.TestCase; using Proxytrace.Domain.TestResult; using Proxytrace.Domain.Usage; @@ -40,7 +41,7 @@ public async Task GetSparklines_NoEvaluatorsInProject_ReturnsEmpty() IServiceProvider services = GetServices(); var reader = services.GetRequiredService(); - var result = await reader.GetSparklinesAsync(Guid.NewGuid(), From, To, StatisticsBucket.Daily, CancellationToken); + var result = await reader.GetSparklinesAsync([Guid.NewGuid()], From, To, StatisticsBucket.Daily, CancellationToken); result.Should().BeEmpty(); } @@ -89,13 +90,45 @@ public async Task GetSparklines_WithEvaluations_ReturnsPassRatePointsForProjectE .FirstAsync(CancellationToken); var (from, to) = NowWindow(); - var result = await reader.GetSparklinesAsync(projectId, from, to, StatisticsBucket.Daily, CancellationToken); + var result = await reader.GetSparklinesAsync([projectId], from, to, StatisticsBucket.Daily, CancellationToken); var sparkline = result.Should().ContainSingle(s => s.EvaluatorId == evaluator.Id).Subject; sparkline.Points.Sum(p => p.Total).Should().Be(2); sparkline.Points.Sum(p => p.Passed).Should().Be(1); } + [TestMethod] + public async Task GetSparklines_WithSeveralProjects_CoversEveryScopedProjectAndNothingElse() + { + IServiceProvider services = GetServices(); + var reader = services.GetRequiredService(); + + // A caller who may read two projects and filtered by none (#483): before the reader took a + // set, the overview could only sparkline one project, so this caller saw none at all. + var (firstProject, first) = await CreateEvaluatorInNewProject(services); + var (secondProject, second) = await CreateEvaluatorInNewProject(services); + var (_, outsider) = await CreateEvaluatorInNewProject(services); + + await PersistResult(services, first, EvaluationScore.Good); + await PersistResult(services, second, EvaluationScore.Bad); + await PersistResult(services, outsider, EvaluationScore.Excellent); + + var (from, to) = NowWindow(); + var result = await reader.GetSparklinesAsync( + [firstProject, secondProject], from, to, StatisticsBucket.Daily, CancellationToken); + + result.Select(s => s.EvaluatorId).Should().BeEquivalentTo(new[] { first.Id, second.Id }); + } + + private async Task<(Guid ProjectId, IEvaluator Evaluator)> CreateEvaluatorInNewProject(IServiceProvider services) + { + var project = await services.GetRequiredService>().CreateAsync(CancellationToken); + var evaluator = await services.GetRequiredService>().AddAsync( + services.GetRequiredService()(project), + CancellationToken); + return (project.Id, evaluator); + } + [TestMethod] public async Task AddResult_PopulatesEvaluationStatProjection() { diff --git a/Proxytrace.Storage.Tests/StatisticsFilterParityTests.cs b/Proxytrace.Storage.Tests/StatisticsFilterParityTests.cs index 8228742c0..95d62d6a0 100644 --- a/Proxytrace.Storage.Tests/StatisticsFilterParityTests.cs +++ b/Proxytrace.Storage.Tests/StatisticsFilterParityTests.cs @@ -34,6 +34,10 @@ public sealed class StatisticsFilterParityTests : BaseTest nameof(StatisticsFilter.AgentId), nameof(StatisticsFilter.EndpointId), nameof(StatisticsFilter.ExcludeSystemAgents), + // #483: the multi-project scope. Query() applies it as a semi-join against + // AgentVersion(Project); BuildLatencyWhere() as "= ANY(@projectIds)" over one uuid[] + // parameter — see StatisticsFilterWhereTests. + nameof(StatisticsFilter.ProjectIds), ]; [TestMethod] diff --git a/Proxytrace.Storage.Tests/StatisticsFilterWhereTests.cs b/Proxytrace.Storage.Tests/StatisticsFilterWhereTests.cs new file mode 100644 index 000000000..065b46747 --- /dev/null +++ b/Proxytrace.Storage.Tests/StatisticsFilterWhereTests.cs @@ -0,0 +1,70 @@ +using AwesomeAssertions; +using Microsoft.Extensions.DependencyInjection; +using Proxytrace.Domain.Statistics; +using Proxytrace.Storage.Internal.Statistics; +using Proxytrace.Testing; + +namespace Proxytrace.Storage.Tests; + +/// +/// Covers the raw-SQL half of translation — the WHERE fragment +/// the latency/percentile paths build by hand. Those paths only run on a relational provider, so the +/// behavioural tests (in-memory) never reach them; this asserts the generated SQL and its parameters +/// directly instead. +/// +[TestClass] +public sealed class StatisticsFilterWhereTests : BaseTest +{ + [TestMethod] + public void BuildLatencyWhere_WithOneProject_KeepsTheEqualityPredicate() + { + IServiceProvider services = GetServices(); + StorageDbContext context = services.GetRequiredService>()(); + Guid projectId = Guid.NewGuid(); + + var (where, parameters) = AgentCallStatsQueries.BuildLatencyWhere( + context, new StatisticsFilter(ProjectId: projectId)); + + // The common case must keep its equality predicate rather than degrade into a + // single-element array comparison, which the planner can cost differently. + where.Should().Contain("\"Project\" = @projectId").And.NotContain("ANY"); + var parameter = parameters.Should().ContainSingle().Subject; + parameter.Name.Should().Be("@projectId"); + parameter.Value.Should().Be(projectId); + } + + [TestMethod] + public void BuildLatencyWhere_WithSeveralProjects_ComparesAgainstOneArrayParameter() + { + IServiceProvider services = GetServices(); + StorageDbContext context = services.GetRequiredService>()(); + Guid first = Guid.NewGuid(); + Guid second = Guid.NewGuid(); + + var (where, parameters) = AgentCallStatsQueries.BuildLatencyWhere( + context, new StatisticsFilter(ProjectIds: [first, second])); + + // = ANY over a single uuid[] parameter: the ids never reach the statement text (so the SQL + // stays parameterised and its text constant regardless of how many projects there are). + where.Should().Contain("\"Project\" = ANY(@projectIds)"); + where.Should().NotContain(first.ToString()).And.NotContain(second.ToString()); + var parameter = parameters.Should().ContainSingle().Subject; + parameter.Name.Should().Be("@projectIds"); + parameter.Value.Should().BeOfType().Which.Should().Equal(first, second); + } + + [TestMethod] + public void BuildLatencyWhere_WithAnEmptyProjectSet_AddsNoClause() + { + IServiceProvider services = GetServices(); + StorageDbContext context = services.GetRequiredService>()(); + + var (where, parameters) = AgentCallStatsQueries.BuildLatencyWhere( + context, new StatisticsFilter(ProjectIds: [])); + + // Mirrors Query(): an empty set is "not restricted by a set", never "match nothing" — an + // endpoint short-circuits an empty scope before it builds a filter at all. + where.Should().BeEmpty(); + parameters.Should().BeEmpty(); + } +} diff --git a/Proxytrace.Storage.Tests/StatsQueryTranslationTests.cs b/Proxytrace.Storage.Tests/StatsQueryTranslationTests.cs index e52b4f1c9..5794c0ce8 100644 --- a/Proxytrace.Storage.Tests/StatsQueryTranslationTests.cs +++ b/Proxytrace.Storage.Tests/StatsQueryTranslationTests.cs @@ -198,6 +198,33 @@ public void SessionsRecent_OrdersByLastActivity_TranslatesToServerSidePagedQuery sql.Should().Contain("LIMIT"); } + [TestMethod] + public void MultiProjectScope_SemiJoinAgainstAgentVersion_TranslatesToServerSideFilter() + { + using IContainer container = BuildPostgresContainer(); + var context = container.Resolve(); + + // The AgentCallStatsQueries.Query() ProjectIds branch (#483): the aggregate is restricted to + // the versions of a SET of projects. Same shape as the single-project branch with IN instead + // of =, so it must stay a server-side semi-join — client-evaluating it would materialize the + // whole trace table at 1M+ rows, the exact failure docs/performance-testing.md exists to catch. + IReadOnlyCollection projectIds = [Guid.NewGuid(), Guid.NewGuid()]; + IQueryable versionIdsForProjects = context.Set() + .AsNoTracking() + .Where(v => projectIds.Contains(v.Project)) + .Select(v => v.Id); + string sql = context.Set() + .AsNoTracking() + .Where(c => versionIdsForProjects.Contains(c.AgentVersionId)) + .GroupBy(_ => 1) + .Select(g => new { Count = g.Count() }) + .ToQueryString(); + + // The project set is compared in SQL (Npgsql renders a parameterised collection as + // "= ANY (@ids)", older shapes as an IN list) — not pulled back and filtered in memory. + sql.Should().MatchRegex("\"Project\"\\s*(=\\s*ANY|IN)"); + } + [TestMethod] public void PulseAggregate_PerMinuteCountBuckets_TranslatesToServerSideGroupBy() { diff --git a/Proxytrace.Storage/Internal/Statistics/AgentCallStatsQueries.cs b/Proxytrace.Storage/Internal/Statistics/AgentCallStatsQueries.cs index 6d9e310da..b7197d54c 100644 --- a/Proxytrace.Storage/Internal/Statistics/AgentCallStatsQueries.cs +++ b/Proxytrace.Storage/Internal/Statistics/AgentCallStatsQueries.cs @@ -215,7 +215,12 @@ ORDER BY "EndpointId" /// for the raw-SQL percentile paths. Kept in lockstep with the table/column /// names in AgentCallConfig/AgentVersionConfig/AgentConfig. /// - private static (string Where, IReadOnlyList<(string Name, object Value)> Parameters) BuildLatencyWhere( + /// + /// internal rather than private so StatisticsFilterWhereTests can assert the + /// generated fragment directly: the behavioural tests run on the in-memory provider, where the + /// percentile paths fall back to LINQ, so this SQL is otherwise only exercised in production. + /// + internal static (string Where, IReadOnlyList<(string Name, object Value)> Parameters) BuildLatencyWhere( StorageDbContext context, StatisticsFilter filter) { var clauses = new List(); @@ -231,6 +236,16 @@ private static (string Where, IReadOnlyList<(string Name, object Value)> Paramet clauses.Add("\"AgentVersionId\" IN (SELECT \"Id\" FROM \"AgentVersionEntity\" WHERE \"Project\" = @projectId)"); parameters.Add(("@projectId", projectId)); } + if (filter.ProjectIds is { Count: > 0 } projectIds) + { + // Multi-project scope (#483). The ids go over as ONE uuid[] parameter compared with + // = ANY — never interpolated into the statement, and never a variable-length IN list, + // so the SQL text is constant regardless of how many projects the caller belongs to + // (Npgsql maps Guid[] to uuid[]). Separate from the single-project clause above so a + // one-project scope keeps its equality predicate rather than a single-element array. + clauses.Add("\"AgentVersionId\" IN (SELECT \"Id\" FROM \"AgentVersionEntity\" WHERE \"Project\" = ANY(@projectIds))"); + parameters.Add(("@projectIds", projectIds.ToArray())); + } if (filter.EndpointId is { } endpointId) { clauses.Add("\"EndpointId\" = @endpointId"); @@ -878,6 +893,19 @@ private static IQueryable Query(StorageDbContext context, Stati .Select(v => v.Id); query = query.Where(c => versionIdsForProject.Contains(c.AgentVersionId)); } + // Multi-project scope (#483): an unfiltered aggregate from a caller who may read several + // projects. Same shape as the single-project branch above — a semi-join against + // AgentVersion(Project), with IN instead of = — so it stays server-side rather than + // degenerating into a client-side filter over every trace row. Kept separate from that + // branch so a scope naming exactly one project keeps its equality predicate and its plan. + if (filter.ProjectIds is { Count: > 0 } projectIds) + { + IQueryable versionIdsForProjects = context.Set() + .AsNoTracking() + .Where(v => projectIds.Contains(v.Project)) + .Select(v => v.Id); + query = query.Where(c => versionIdsForProjects.Contains(c.AgentVersionId)); + } if (filter.From is { } from) { query = query.Where(c => c.CreatedAt >= from); diff --git a/Proxytrace.Storage/Internal/Statistics/EvaluatorStatsQueries.cs b/Proxytrace.Storage/Internal/Statistics/EvaluatorStatsQueries.cs index f063128df..fb25f6842 100644 --- a/Proxytrace.Storage/Internal/Statistics/EvaluatorStatsQueries.cs +++ b/Proxytrace.Storage/Internal/Statistics/EvaluatorStatsQueries.cs @@ -61,17 +61,24 @@ public async Task GetOverviewAsync( } public async Task> GetSparklinesAsync( - Guid projectId, + IReadOnlyCollection projectIds, DateTimeOffset from, DateTimeOffset to, StatisticsBucket bucket, CancellationToken cancellationToken = default) { + if (projectIds.Count == 0) + { + return []; + } + StorageDbContext context = contextFactory(); + // The owning-project filter runs in SQL for one project or several alike (#483); the + // evaluations query below is unchanged — it was already keyed on the resulting evaluator ids. Guid[] evaluatorIds = await context.Set() .AsNoTracking() - .Where(e => e.Project == projectId) + .Where(e => projectIds.Contains(e.Project)) .Select(e => e.Id) .ToArrayAsync(cancellationToken); diff --git a/docs/architecture.md b/docs/architecture.md index a871354ba..b338df4ac 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -34,7 +34,7 @@ Proxytrace.Api → Proxytrace.Application → Proxytrace.Domain → Proxyt - **Proxytrace.Serialization** — JSON serializers and output formats (`ISerializer`, `IOutputFormat`, `ObjectToInferredTypesConverter`). - **Proxytrace.Storage** — EF Core entities, configurations, mappers, migrations. Provider auto-detected (SQLite / PostgreSQL / SQL Server). - **Proxytrace.Common** — Shared utilities: validation helpers, async/type extensions, DI extensions, randomness. -- **Proxytrace.Proxy** — **Shared pipeline library** (classlib) for the OpenAI-compatible proxy route. Contains the MVC controller (`OpenAiProxyController`), the API-key resolver (`IApiKeyResolver`/`ApiKeyResolver` — **deliberately uncached**, straight from storage on every request so provider-key rotation/revocation takes effect on the next request and the proxy fails closed when the database is unreachable rather than serving stale credentials — #407), the blocking-rule provider (`IBlockingRuleProvider`/`CachedBlockingRuleProvider`), and the request blocker (`IRequestBlocker`/`RequestBlocker`). Its `Module` registers only the pipeline types and their supporting services (IMemoryCache, HTTP clients). References Domain + Infrastructure + Messaging + Storage **+ Licensing** (it does **not** reference Api **or Application** — directly or transitively). The host composition root is responsible for wiring storage, messaging, infrastructure, and licensing; the host also adds the library as an MVC application part so its controller is discovered (the controller assembly is **not** auto-discovered, so a host that does not add the part has no proxy route). This design allows both the standalone `Proxytrace.Proxy.Api` host and, in kiosk mode, `Proxytrace.Api` to mount the proxy route. **In-process kiosk mount:** `Proxytrace.Api` registers `Proxytrace.Proxy.Module` and adds the controller's application part **only when `Kiosk:Enabled` AND a live `Kiosk:Endpoint` is configured** — in production or kiosk-without-endpoint the pipeline services are absent and the `openai/v1/{**path}` routes never resolve. In that single-process kiosk, the controller publishes captured calls to the same in-process `IIngestionStream` (`Messaging__Provider=InProcess`, a shared singleton) that the app's `AgentCallIngestionWorker` consumes — no Redis, no separate container. The controller's kiosk guard refuses (503) only when kiosk has **no** live endpoint; with a live endpoint it serves so a sample client's OpenAI SDK `baseURL` can point at the kiosk API. Kiosk seeding also mints a fixed, config-known demo ingestion key (`Kiosk:DemoApiKey`, default `pk-kiosk-demo`) for the "Showcase Project", attached to the live provider and stored hashed like any operator-minted key (`DemoApiKeySeedScenario`). +- **Proxytrace.Proxy** — **Shared pipeline library** (classlib) for the OpenAI-compatible proxy route. Contains the MVC controller (`OpenAiProxyController`), the API-key resolver (`IApiKeyResolver`/`ApiKeyResolver` — **deliberately uncached**, straight from storage on every request so provider-key rotation/revocation takes effect on the next request and the proxy fails closed when the database is unreachable rather than serving stale credentials — #407), the blocking-rule provider (`IBlockingRuleProvider`/`CachedBlockingRuleProvider`), and the request blocker (`IRequestBlocker`/`RequestBlocker`). Its `Module` registers only the pipeline types and their supporting services (IMemoryCache, HTTP clients). References Domain + Infrastructure + Messaging + Storage **+ Licensing** (it does **not** reference Api **or Application** — directly or transitively). The host composition root is responsible for wiring storage, messaging, infrastructure, and licensing; the host also adds the library as an MVC application part so its controller is discovered (the controller assembly is **not** auto-discovered, so a host that does not add the part has no proxy route). This design allows both the standalone `Proxytrace.Proxy.Api` host and, in kiosk mode, `Proxytrace.Api` to mount the proxy route. **In-process kiosk mount:** `Proxytrace.Api` registers `Proxytrace.Proxy.Module` and adds the controller's application part **only when `Kiosk:Enabled` AND a live `Kiosk:Endpoint` is configured** — in production or kiosk-without-endpoint the pipeline services are absent and the `openai/v1/{**path}` routes never resolve. In that single-process kiosk, the controller publishes captured calls to the same in-process `IIngestionStream` (`Messaging__Provider=InProcess`, a shared singleton) that the app's `AgentCallIngestionWorker` consumes — no Redis, no separate container. The controller's kiosk guard refuses (503) only when kiosk has **no** live endpoint; with a live endpoint it serves so a sample client's OpenAI SDK `baseURL` can point at the kiosk API. Kiosk seeding also mints a fixed, config-known demo ingestion key (`Kiosk:DemoApiKey`, default `pk-kiosk-demo`) for the "Showcase Project", attached to the live provider and stored hashed like any operator-minted key (`DemoApiKeySeedScenario`). **Upstream response handling:** both branches send with `HttpCompletionOption.ResponseHeadersRead` and copy the body through in bounded chunks (never `ReadAsStringAsync`), forwarding every byte untruncated while capturing at most `MaxCapturedResponseChars` (16 MiB) for ingestion. Because `HttpClient.Timeout` stops applying the moment the response headers arrive, the buffered branch re-arms that **same** configured timeout (5 min — `Proxytrace.Proxy/Module.cs`, passed in rather than duplicated) as a linked `CancellationTokenSource` around its copy loop, so an upstream that sends headers and then stalls is cut off with a **504** and recorded as such instead of pinning a request, a socket and a thread-pool continuation until the client gives up ([#475](https://github.com/SyntaktikEU/Proxytrace/issues/475)); a client abort stays distinguishable from that timeout and still propagates. The streaming branch splits SSE lines itself (bounded by `MaxForwardedLineChars`, 256 KiB) on **LF, CRLF or a lone CR** — all normalized to LF on the wire, as the `ReadLineAsync` it replaced did ([#480](https://github.com/SyntaktikEU/Proxytrace/issues/480)). - **Proxytrace.Proxy.Api** — **Standalone** deployable host for the proxy pipeline (own `Program`/`Dockerfile`/`Module`). Loads `Proxytrace.Proxy.Module` for the shared pipeline and adds the host-lifecycle services: it deliberately constructs `Storage.Module` with `registerApplicationServices: false` and never registers `Application.Module`, so **no Application service runs in the proxy** (test runner, optimizer, ingestion worker, search indexing, demo seeder, …). It registers `Proxytrace.Infrastructure.Security.SecretProtectionModule` directly (needed to decrypt upstream provider keys — see docs/security.md), plus small local stubs for the factory delegates the storage model-building graph expects. The licensing module is registered with `ServerCheckEnabled = false` in **both** build flavors — the main app owns the license-server heartbeat and the offline-grace cache file in the shared data dir; the proxy only *consumes* the snapshot for use-time gating and keeps the DB-stored license fresh via a polling `ProxyStoredLicenseService` (host-only, lives in `Proxytrace.Proxy.Api.Internal`). - **Proxytrace.Messaging** — Ingestion transport between the proxy (producer) and the app's ingestion worker (consumer), via `IIngestionStream`. Backed by **Redis Streams** in production (`StackExchange.Redis`); backed by an in-memory channel in tests and single-process/kiosk runs. - **Proxytrace.Licensing** — License resolution and feature/limit gating via `ILicenseService`. Tiers, `LicenseFeature`/`LicenseLimit`, JWT public-key verification. See [`licensing.md`](licensing.md). @@ -105,6 +105,21 @@ repository method (`GetByProjectsPagedAsync`) rather than merging per-project pa splits a scope into the `(ProjectId, ProjectIds)` pair `AgentCallFilter` takes, so the hot traces query keeps its equality predicate whenever the scope names a single project. -Aggregates built on `StatisticsFilter` still take a single project. `StatisticsController` refuses an -unscoped non-admin request with `403` (an explicit contract, not a silent empty page); the traces -overview returns empty for a multi-project scope rather than aggregating across tenants (#483). +Aggregates built on `StatisticsFilter` take the same `(ProjectId, ProjectIds)` pair (#483), so the +traces overview aggregates over a multi-project scope like the lists beside it. `ToFilterScope()` +feeds both filters, so a scope naming one project keeps the single-project predicate in both. The set +must be applied in **both** of the filter's translation paths — the LINQ chokepoint +`AgentCallStatsQueries.Query()` and the hand-built raw-SQL `BuildLatencyWhere()` behind the latency +percentiles, where it goes over as one `uuid[]` parameter (`= ANY(@projectIds)`), never interpolated. +`StatisticsFilterParityTests` fails if a new filter member reaches only one of them. +`StatisticsController`'s dashboard is unchanged: it refuses an unscoped non-admin request with `403` +(an explicit contract, not a silent empty page). + +**No controller re-implements the check.** `ProjectsController` — where the listed resource *is* the +project, so there is no `projectId` filter — used to do its own `User.IsInRole` + membership test. +That is exactly what the guard exists to prevent: a role claim says nothing about the REST API key +the request came in on, so a key minted for project A could read project B's detail and its members +(PII) whenever the key's *owner* was a member of B (#474). It now calls +`ResolveListScopeAsync(requestedProjectId: null)` for `GET /api/projects` and `CanAccessProjectAsync` +for the by-id reads, like every other controller. If you need a role or membership decision in a +controller, extend the guard — never inline it. diff --git a/docs/database.md b/docs/database.md index 1474d7de8..61bf2a7e1 100644 --- a/docs/database.md +++ b/docs/database.md @@ -81,6 +81,17 @@ Set the connection string in: database needed), and `StatisticsFilterParityTests` locks the `StatisticsFilter` member list so the LINQ chokepoint (`Query()`) and the raw-SQL percentile `WHERE` builder (`BuildLatencyWhere`) cannot silently diverge when the filter gains a member. +- **Project scope is one id *or* a set, and both are applied in SQL.** `StatisticsFilter` carries + `ProjectId` **and** `ProjectIds`, mirroring `AgentCallFilter` (#482/#483): a scope naming exactly + one project keeps the equality predicate against `AgentVersion(Project)` — the common, indexed + case, so its plan is untouched — while a caller who may read several and named none filters on the + set. Both translation paths must honour it: `Query()` as a semi-join with `IN`, and + `BuildLatencyWhere()` as `"Project" = ANY(@projectIds)` with the ids passed as a **single `Guid[]` + parameter** (Npgsql maps it to `uuid[]`). Never interpolate ids into that SQL — the statement text + must stay constant regardless of how many projects the caller belongs to. An empty set means "not + restricted by a set", not "match nothing"; endpoints short-circuit an empty scope before building + a filter at all (`ProjectListScope.IsEmpty`). `StatisticsFilterWhereTests` covers the raw-SQL + fragment, which the in-memory provider never reaches. - **The dashboard composite is served from a short-TTL, single-flight, in-process cache** (`DashboardStatistics`). The dashboard fans out ~12 statistics queries per request and every open viewer polls it every 30 s, three of those queries being full-window scans — without sharing, N diff --git a/docs/optimization-loop.md b/docs/optimization-loop.md index eaf05f35d..681c77023 100644 --- a/docs/optimization-loop.md +++ b/docs/optimization-loop.md @@ -141,13 +141,24 @@ A/B-test card both go through it, so the surfaces can't drift). A wall-clock `Co measure would fold in the run's queue wait, the evaluator passes, and the parallel-execution overlap between cases, so it is deliberately **not** used for latency. -When a group completes, `TestRunnerService` calls `IOptimizerService.EnqueueAsync(group)` -(`TestRunnerService.cs:178`). `TestRunGroupsController` can also enqueue on demand. +When a group completes, `TestRunnerService.ExecuteGroupAsync` calls +`IOptimizerService.EnqueueAsync(group)`. `TestRunGroupsController` can also enqueue on demand. The same completion point also feeds **anomaly detection** (a parallel, independent pipeline — see below): `TestRunnerService` calls `IAnomalyDetectionService.EnqueueAsync(group)` on both the success and the failure path (a failed group is itself the most important anomaly). Anomaly detection raises user notifications rather than theories; it does not participate in the theory→proposal loop. + +**Each path only settles a group it actually settled.** Both the success path and the generic +failure handler check `group.Status.IsTerminal()` under the per-group lock and do their terminal +work — the `SetCompleted`/`SetFailed` transition, the `PublishGroupComplete` broadcast, *and* the +enqueues — only when that check let them through. A group is settled exactly once, by exactly one +of them, so it gets exactly **one** terminal SSE event and **one** anomaly-detection pass. This +matters because the failure handler is reachable with the group already terminal: a racing +`CancelAsync` may have settled it `Cancelled`, or the fault may have come from the *tail* of the +success path (an enqueue) with the group already `Completed`. Broadcasting or enqueueing from there +re-sent the terminal event carrying the state the group was already in rather than `Failed`, and +re-ran detection, so one genuine anomaly was flagged and notified twice (#486). `Proxytrace.Application/Anomaly/` holds `IAnomalyDetectionService` (a hosted background queue, a structural copy of `OptimizerService`), the pure `IAnomalyDetector` rule engine (run failed / endpoint unavailable, pass-rate drop or latency increase vs a rolling baseline computed from @@ -156,6 +167,15 @@ baseline — shared with the kiosk demo seeder, which runs the same rule engine incident groups), and `AnomalyDetectionConfiguration` (thresholds + baseline window). Detected anomalies are delivered through `INotificationService` → the dashboard notification channel. +**Both enqueues are handed `CancellationToken.None`, deliberately.** The group's transition to a +terminal state happens *inside* the per-group `IAsyncLock`, but the two `EnqueueAsync` calls run +outside it. Passing the run's linked (cancellable) token there opened a silent hole: a `CancelAsync` +landing in that window tripped the token, both enqueues were skipped, and `SettleCancelledAsync` then +found an already-terminal group and no-op'd — so the group read `Completed` forever with neither +optimization nor anomaly detection ever run, and no error surfaced (#476). Once a group is durably +terminal the downstream jobs are **owed**, so they must not be cancellable by a racing cancel. Keep +it that way on both the success and the failure path. + **Sampling and the loop — cohort aggregation.** The per-run `TestRunStats` projection is left **unchanged** (one row per `TestRun`); the loop aggregates at read time so N samples never produce N near-identical anomalies or bias a proposal toward "sample 0": diff --git a/docs/performance-testing.md b/docs/performance-testing.md index a4b3f304a..aa7abb6f0 100644 --- a/docs/performance-testing.md +++ b/docs/performance-testing.md @@ -83,6 +83,30 @@ scan — only the narrow scalar columns are read, never the request/response JSO dev seed (2026-07) was 276.9ms / 170.6ms; budgets sit ~45% above. A jump toward seconds means either the per-endpoint fold started round-tripping or the planner lost its statistics (see #246 below). +### Multi-project scope (`agentCallsListByProjects`, `statsAgentBreakdownByProjects`, `statsLatencyPercentilesByProjects`) + +A caller who may read several projects and names none is scoped to the **set** of their projects +(#482 for the lists, #483 for the aggregates). The thing to protect is that the set is applied *in +the database*: it is the same semi-join against `AgentVersion(Project)` as the single-project branch +with `IN` instead of `=`, so each metric must stay in the same class as its single-project twin. +Filtering a scope client-side would materialize every row — the exact failure this suite exists to +catch — and it only bites at scale, because a correctness test on a handful of in-memory rows passes +either way. + +The two aggregate metrics are measured as a **pair** because `StatisticsFilter` has two translation +paths that can regress independently: `AgentCallStatsQueries.Query()` (LINQ; backs +`statsAgentBreakdownByProjects`) and the hand-built raw-SQL `BuildLatencyWhere()` behind the latency +percentiles (backs `statsLatencyPercentilesByProjects`), where the ids travel as **one `uuid[]` +parameter** compared with `= ANY(@projectIds)` — never interpolated, so the statement text stays +constant however many projects the caller belongs to. + +The **single-project path is deliberately preserved**: a scope naming exactly one project still +filters by `ProjectId` (`ProjectListScope.ToFilterScope()`), because `= ANY` over a one-element array +can plan differently from `=`. The common case — the web UI, which always sends a `projectId`, and +every REST API key, confined to one — is therefore bit-for-bit the query it was. All three set +budgets are their single-project twins plus headroom for the wider subquery and are **uncalibrated +placeholders** until a full 1M run lands. + ### Retention's session reconciliation (`sessionRemovalDeltas`) Trace retention has to give the denormalized session counters back what the traces it deletes diff --git a/docs/security.md b/docs/security.md index 9544085b7..9f4b8e5f1 100644 --- a/docs/security.md +++ b/docs/security.md @@ -136,10 +136,19 @@ record equality; only its textual rendering is masked. Current overrides: `ModelProvider` (`ApiKey`, the reference implementation), `EmailSettings` (`Password`), `UserTotpEnrollment` (`Secret`), `KioskEndpointOptions` and `ResolvedKioskEndpoint` -(`ApiKey`). Note the accessibility differs: a sealed record deriving from `object` must declare -`private bool PrintMembers(StringBuilder)`, one deriving from another record +(`ApiKey`), `User` (`PasswordHash`). Note the accessibility differs: a sealed record deriving from +`object` must declare `private bool PrintMembers(StringBuilder)`, one deriving from another record `protected override bool PrintMembers(StringBuilder)` (and must chain to `base.PrintMembers`). -`Proxytrace.Domain.Tests/SecretRedactionToStringTests` pins all five. +`Proxytrace.Domain.Tests/SecretRedactionToStringTests` pins all six. + +Only **credentials** are masked — identifiers stay readable, so a redacted record is still useful for +telling *who* or *what* a log line is about. Hence `User.Email` and `User.ExternalSubject` (the OIDC +subject: a stable identifier, not something you can authenticate with) render in full, the way +`EmailSettings.Username` does. `User` matters more than its own logging sites suggest: it is printed +transitively by every record holding an `IUser` (`UserTotpEnrollment.User`, `ApiKey.Owner`). Its +`PasswordHash` is a salted, slow `IPasswordService` hash rather than a plaintext, so masking it is +defence in depth — but a hash in the operator Error Log or a support bundle is an offline-cracking +target. ## Backfill of pre-existing rows @@ -293,6 +302,19 @@ be induced to make. a secure context and accept `Secure` cookies there, so the local Docker/e2e/kiosk stacks (`http://localhost:5101`, `:5103`) work with the default. +This setting is the mirror image of `ForwardedHeaders` / `RateLimiting` above: it is read from the +**container's** configuration view (`Proxytrace.Api/Module.cs`), the one that also sees +`appsettings.local.json`. That view must agree with the host about which environment this is, so the +environment name comes from `HostEnvironmentName` (`Proxytrace.Api/Configuration/`), which resolves +it exactly as `WebApplicationBuilder` does — **`DOTNET_ENVIRONMENT` ahead of +`ASPNETCORE_ENVIRONMENT`**, from the process environment rather than from a JSON file — and the +module layers `appsettings.{Environment}.json` in between `appsettings.json` and +`appsettings.local.json`, where the host layers it. Both divergences were real: the reversed +precedence made a Production host compute `Development` (and drop `Secure`) when the two variables +were set and disagreed, and the missing environment file meant an operator's +`appsettings.Production.json` was silently ignored here while being honoured everywhere the host +config is read. + ## In-process auth/MFA/rate-limit state is single-instance by design Several auth defenses keep their state **in process memory**, not in a shared store: diff --git a/docs/sse-events.md b/docs/sse-events.md index f05c50250..8b4f9abce 100644 --- a/docs/sse-events.md +++ b/docs/sse-events.md @@ -25,7 +25,12 @@ hooks in `frontend/src/api/event-stream.ts`. `*` = **terminal** event. The client closes the `EventSource` on the terminal event and the run/group views are **pure-SSE (no polling)**; on terminal they invalidate the relevant TanStack queries to -heal any dropped events. Do **not** reintroduce `refetchInterval` on these views. +heal any dropped events. Do **not** reintroduce `refetchInterval` on these views. A terminal event is +therefore **emitted exactly once** per run/group: the publisher must sit inside the same +`IsTerminal()`-guarded critical section as the transition that settles the entity, so a second path +reaching an already-settled entity publishes nothing (see +[`docs/optimization-loop.md`](optimization-loop.md) — `TestRunnerService`'s success, cancel and +failure paths all do this; #486). **Cross-tenant scoping (every stream).** Streams are tenant-scoped via `IProjectAccessGuard` (`Proxytrace.Api/Auth/IProjectAccessGuard.cs`; admins bypass). Per-resource streams @@ -122,6 +127,23 @@ that write fails, the action unwinds, `RequestAborted` fires, and the broadcaste registration removes the subscription. Mirror this exactly when adding a stream — emit the heartbeat on the `null` tick, real events otherwise. +**Mid-stream faults (any started response).** Once the first frame is out, headers and status are +read-only, so `ExceptionHandlingMiddleware` cannot turn a later fault into an error body — writing +one would only append junk to a payload the client is already consuming. It therefore logs the real +cause and calls `HttpContext.Abort()`. The reset matters: simply returning is a *success* signal to +the framework, which then closes the response cleanly (chunked terminator / HTTP/2 `END_STREAM`), and +a client reads the truncated payload as a complete one. SSE consumers just reconnect, but any other +chunked response would silently hand back half a document. + +**Client disconnects are not faults.** A mid-stream hang-up surfaces as a *failed write* (typically +an `IOException`), not an `OperationCanceledException`, so it reaches the same catch block as a real +fault. The middleware therefore classifies it **first**, before anything is logged: a started +response plus a cancelled `RequestAborted` is logged once at `Debug` and returns — no Error-level +log, so no captured `ApplicationError` row and no `errorId`, and no abort (there is nothing left to +reset). Without that ordering every closed browser tab with an open stream would add an Error Log +entry no operator can act on. Genuine faults on a live connection keep the full treatment: Error +capture with an `errorId`, a warning, and the reset. + **Subscriber cap (every broadcaster).** Each broadcaster bounds total live subscriptions at `MaxSubscribers = 2000` so an authenticated client can't exhaust memory/sockets by opening unbounded streams; past the cap `Subscribe` returns an immediately-completed reader (the SSE request closes diff --git a/manual/admin/error-log.md b/manual/admin/error-log.md index 0ec0ded8f..ee1f4c21b 100644 --- a/manual/admin/error-log.md +++ b/manual/admin/error-log.md @@ -18,7 +18,10 @@ on. This is broader than just failed HTTP requests: errors that never surface as still recorded. Expected cancellations (client disconnect or shutdown, i.e. `OperationCanceledException` / -`TaskCanceledException`) are **not** recorded — they are normal, not faults. +`TaskCanceledException`) are **not** recorded — they are normal, not faults. The same holds for a +client that hangs up in the middle of a streaming response (closing a browser tab that had live +updates open, for example): the failed write is recognised as a disconnect and skipped, so normal +navigation never fills this page with entries. Each entry stores: diff --git a/manual/admin/providers-and-api-keys.md b/manual/admin/providers-and-api-keys.md index bade9a703..3d47be733 100644 --- a/manual/admin/providers-and-api-keys.md +++ b/manual/admin/providers-and-api-keys.md @@ -90,6 +90,13 @@ loads without a price (shown as `—`). Every stored price is normalised to **EUR per 1M tokens** and is refreshed from the catalogue on each reload. +If either feed is unreachable — the catalogue **or** the exchange-rate feed — models still load, they +simply load without prices. Proxytrace makes **one** attempt against the failing feed and then pauses +for about half a minute before trying it again, instead of retrying once per discovered model. A +reload during a feed outage therefore finishes promptly rather than appearing to hang; run it again +once the feed is back to fill the prices in. A successfully fetched exchange rate is reused for the +rest of the day, so a healthy day needs a single rate lookup. + Operators can point the pricing feeds at different sources via the `Pricing` section of `appsettings` (see [Configuration](/admin/configuration)): `Pricing:LiteLlmFeedUrl` and `Pricing:FxApiUrl`. @@ -121,7 +128,9 @@ Each key also carries explicit **capabilities** (least privilege), chosen when y A REST key is confined to its own project, so list endpoints that take an optional `projectId` return that project's rows whether or not you pass one — there is no need to repeat the project on every -call, and a key can never widen its reach by omitting it. +call, and a key can never widen its reach by omitting it. The confinement holds for the projects +themselves: `GET /api/projects` lists only the key's project, and reading any other project (or its +members) answers `404`, regardless of what the key's owner could see when signed in. A key works only on the surfaces it was granted: an ingestion-only key cannot drive MCP or the REST API, an MCP-only key cannot proxy LLM traffic or drive REST, and a REST key cannot drive MCP. Keys issued diff --git a/perf/Proxytrace.PerfHarness/Scenarios/QueryLatencyScenario.cs b/perf/Proxytrace.PerfHarness/Scenarios/QueryLatencyScenario.cs index b25b53a12..0d300912a 100644 --- a/perf/Proxytrace.PerfHarness/Scenarios/QueryLatencyScenario.cs +++ b/perf/Proxytrace.PerfHarness/Scenarios/QueryLatencyScenario.cs @@ -172,6 +172,21 @@ await Measure("statsPulse", await Measure("anomalyTimeline", () => statsReader.GetAnomalyCountsByAgentAsync(filter, StatisticsBucket.Daily, cancellationToken)); + // Multi-project scope (#483): the traces overview as a caller who may read several projects + // and named none. Both aggregates that overview runs are measured because they translate the + // scope through DIFFERENT paths — the agent breakdown through the LINQ chokepoint (a + // semi-join against AgentVersion(Project), IN instead of =), the latency percentiles through + // the raw-SQL "= ANY(@projectIds)". Each must stay in the same class as its single-project + // twin; a climb toward the unfiltered full-scan band means the set stopped being applied in + // the database. Measured against a two-element scope (one real project plus one absent id) + // so the set genuinely has to be evaluated. + var projectsFilter = new StatisticsFilter( + From: from, To: now, ProjectIds: [projectId ?? Guid.Empty, Guid.NewGuid()]); + await Measure("statsAgentBreakdownByProjects", + () => statsReader.GetAgentBreakdownAsync(projectsFilter, cancellationToken)); + await Measure("statsLatencyPercentilesByProjects", + () => statsReader.GetLatencyAsync(projectsFilter, cancellationToken)); + // Per-agent overview page. await Measure("agentOverview", () => agentStats.GetAgentOverviewAsync(agentId, from, now, StatisticsBucket.Daily, cancellationToken)); diff --git a/perf/perf-budgets.json b/perf/perf-budgets.json index eb2962cb3..473d3ca43 100644 --- a/perf/perf-budgets.json +++ b/perf/perf-budgets.json @@ -3,6 +3,8 @@ "_comment_listByProjects": "agentCallsListByProjects covers the multi-project scope added for #482: a list request from a caller who may read several projects (an unfiltered non-admin) filters AgentVersionId against the versions of a SET of projects rather than one. It is the same semi-join as the single-project branch with IN instead of =, so it belongs in the same class as agentCallsList; the budget is agentCallsList's plus headroom for the wider subquery, NOT a new class of cost. Regression signature: a climb toward the unfiltered-scan aggregates means the IN stopped translating and the scope is being applied client-side over every row — which at 1M is the exact failure mode the perf rule exists to catch. Measured against a two-element scope (one real project plus one absent id) so the set genuinely has to be evaluated. RECALIBRATE on your CI hardware.", + "_comment_statsByProjects": "statsAgentBreakdownByProjects/statsLatencyPercentilesByProjects cover the multi-project scope added for #483: the traces overview from a caller who may read several projects and named none, which restricts the aggregate to a SET of projects rather than one. They are measured as a PAIR because the two paths translate the set differently and can regress independently — the breakdown through the LINQ chokepoint (Query(): a semi-join against AgentVersion(Project), IN instead of =), the percentiles through the hand-built raw-SQL WHERE ('AgentVersionId' IN (SELECT 'Id' FROM 'AgentVersionEntity' WHERE 'Project' = ANY(@projectIds)), one uuid[] parameter, never interpolated). Both are the same shape as their single-project twins plus a wider subquery, so they belong in the same class as statsAgentBreakdown (500) and statsLatencyPercentiles (1200), NOT a new class of cost; the budgets are those plus ~25% for the wider inner predicate. Note the single-project path is deliberately PRESERVED (a scope naming exactly one project still filters by ProjectId), because '= ANY' over a one-element array can plan differently from '=' — so a regression here never affects the common indexed case. Measured against a two-element scope (one real project plus one absent id) so the set genuinely has to be evaluated. Both budgets are UNCALIBRATED PLACEHOLDERS derived from their single-project twins — RECALIBRATE ~20-30% above observed p95 on a full 1M run. Regression signature: a climb toward the unfiltered full-scan band means the set stopped being applied in the database and the scope is being filtered client-side over every row.", + "_comment_summary": "agentCallsSummary/agentCallsSummaryByTimeRange back the traces KPI band. The trace list scrolls rather than pages, so the band describes the WHOLE filtered set — these are unpaged aggregates by design, and the unfiltered case is a full-table scan at any size. The query GROUPs BY EndpointId (cost is priced per endpoint via CalculateCost and cannot be summed in SQL), so what crosses the wire is O(endpoints), never O(rows): EXPLAIN shows a HashAggregate over the scan emitting one row per endpoint, with width=42 on the scan — only the narrow scalar columns are read, never the request/response JSON. Budgets are measured p95 on a 1M dev seed (2026-07: 276.9ms / 170.6ms) plus ~45% headroom. Like the other scan-bound aggregates below, no index helps the unfiltered full-window case; the time-ranged variant rides the CreatedAt index and is the state the UI actually opens in. Regression signature: a jump toward seconds means either the cost fold started round-tripping per endpoint or the planner lost its statistics. RECALIBRATE on your CI hardware.", "_comment_sessionRemovalDeltas": "sessionRemovalDeltas times the aggregate trace retention reads BEFORE it deletes: the per-session totals of the doomed calls, which the sweep hands back to the session counters so a session header cannot keep claiming traces its timeline no longer holds (#436). A GROUP BY SessionId over the same indexed CreatedAt range the delete uses, null-session rows excluded — what crosses the wire is O(sessions in the window), never O(rows). The probe passes a cutoff covering the WHOLE seed, which is the worst case by design; the nightly sweep only ever sees the tail beyond the retention window. Budget is the measured p95 on a 1M dev seed (2026-07: 464.4ms) plus ~40% headroom. Like the other scan-bound aggregates no index helps the full-window case. Regression signature: a climb toward seconds means the grouping stopped translating and started materialising the doomed rows client-side. RECALIBRATE on your CI hardware.", @@ -35,6 +37,8 @@ "statsLatencyPercentiles": 1200, "statsTokenUsage": 1200, "statsAgentBreakdown": 500, + "statsAgentBreakdownByProjects": 650, + "statsLatencyPercentilesByProjects": 1500, "statsModelBreakdown": 600, "statsCostEstimate": 600, "statsCallTrends": 1100,