From a7f5afb4b69021679b615c3dd3904a47824efc9d Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Fri, 9 Oct 2026 11:30:05 +0800 Subject: [PATCH 1/5] GTR-13556 SharpCache: content-based keys, per-instance entries on the instance SharpCache keyed reference-type arguments by GetHashCode(), i.e. object identity. RuleEngine.Triggered builds a new AgentFilter on every call, so every trigger missed and parked another full copy of all agents in the static MemoryCache for 10 minutes (OneBrain pods saw-toothing at 80-88% of their 3 GiB limit). perInstanceCache entries were parked there too, one dead copy per DI scope. - Build argument keys from content: strings (length-prefixed, SHA-256 when long), primitives, ICacheKey, otherwise a SHA-256 of the JSON. An argument without a stable key bypasses the cache instead of falling back to identity. - AgentFilter implements ICacheKey. - perInstanceCache entries live on the instance (ConditionalWeakTable) and are collected with it; ClearCacheAsync still clears them. - Callers that mutated now-shared cached results work on copies: RoutingAgentHook (routable agent fields) and AgentController.GetAgent (inherit). - GetAgent no longer flags the shared AgentSettings.LlmConfig as inherited. Co-Authored-By: Claude Opus 5.5 --- .../Agents/Models/Agent.cs | 6 + .../Infrastructures/SharpCacheAttribute.cs | 139 +++++++-- .../Repositories/Filters/AgentFilter.cs | 17 +- .../Agents/Services/AgentService.GetAgents.cs | 3 +- .../Routing/Hooks/RoutingAgentHook.cs | 14 + .../Controllers/Agent/AgentController.cs | 11 +- .../SharpCacheAgentMemoryTests.cs | 268 ++++++++++++++++++ 7 files changed, 434 insertions(+), 24 deletions(-) create mode 100644 tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs diff --git a/src/Infrastructure/BotSharp.Abstraction/Agents/Models/Agent.cs b/src/Infrastructure/BotSharp.Abstraction/Agents/Models/Agent.cs index 4fc3b10d8..0b422dceb 100644 --- a/src/Infrastructure/BotSharp.Abstraction/Agents/Models/Agent.cs +++ b/src/Infrastructure/BotSharp.Abstraction/Agents/Models/Agent.cs @@ -165,6 +165,12 @@ public override string ToString() => $"{Name} {Id}"; + /// + /// Copies every member, including the ones leaves out. Lists and nested objects + /// are still shared with the source: replace any of them before changing it. + /// + public Agent ShallowCopy() => (Agent)MemberwiseClone(); + public static Agent Clone(Agent agent) { return new Agent diff --git a/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs b/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs index c358e0693..d46cd3112 100644 --- a/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs +++ b/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs @@ -2,7 +2,13 @@ using BotSharp.Abstraction.MultiTenancy; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Rougamo; +using System.Collections.Concurrent; +using System.Globalization; +using System.Runtime.CompilerServices; +using System.Security.Cryptography; +using System.Text.Json; using Rougamo.Context; namespace BotSharp.Core.Infrastructures; @@ -10,7 +16,18 @@ namespace BotSharp.Core.Infrastructures; public class SharpCacheAttribute : AsyncMoAttribute { public static IServiceProvider Services { get; set; } = null!; - private static readonly object NullMarker = new { __is_null = "$_is_null" }; + private const string NullKey = "$_is_null"; + private const int MaxRawStringKeyLength = 128; + private static readonly JsonSerializerOptions KeyJsonOptions = new() { WriteIndented = false }; + + /// + /// Entries for perInstanceCache, attached to the instance they belong to. Such an entry can only + /// ever be hit through that instance, so it is collected together with it. In the shared cache it stayed + /// for the full expiry after a scoped service's scope had ended, one dead copy per scope. + /// + private static readonly ConditionalWeakTable> InstanceCaches = new(); + + private sealed record InstanceCacheEntry(object Value, DateTime ExpiresAt); private readonly int _minutes; private readonly bool _perInstanceCache; @@ -33,7 +50,14 @@ public override async ValueTask OnEntryAsync(MethodContext context) } var key = GetCacheKey(context); - var value = await _cache.GetAsync(key, context.TaskReturnType); + if (key == null) + { + return; + } + + var value = IsInstanceScoped(context) + ? GetInstanceValue(context.Target!, key) + : await _cache.GetAsync(key, context.TaskReturnType); if (value != null) { // check if the cache is out of date @@ -64,8 +88,37 @@ public override async ValueTask OnSuccessAsync(MethodContext context) if (context.ReturnValue != null) { var key = GetCacheKey(context); - await _cache.SetAsync(key, context.ReturnValue, new TimeSpan(0, _minutes, 0)); + if (key == null) + { + return; + } + + var expiry = new TimeSpan(0, _minutes, 0); + if (IsInstanceScoped(context)) + { + InstanceCaches.GetOrCreateValue(context.Target!)[key] = new InstanceCacheEntry(context.ReturnValue, DateTime.UtcNow + expiry); + } + else + { + await _cache.SetAsync(key, context.ReturnValue, expiry); + } + } + } + + private bool IsInstanceScoped(MethodContext context) + => _perInstanceCache && context.Target != null; + + private static object? GetInstanceValue(object target, string key) + { + if (InstanceCaches.TryGetValue(target, out var entries) && entries.TryGetValue(key, out var entry)) + { + if (entry.ExpiresAt > DateTime.UtcNow) + { + return entry.Value; + } + entries.TryRemove(key, out _); } + return null; } public virtual Task IsOutOfDate(MethodContext context, object value) @@ -74,19 +127,27 @@ public virtual Task IsOutOfDate(MethodContext context, object value) } - private string GetCacheKey(MethodContext context) + /// + /// Returns null when an argument cannot be turned into a stable key; the call then bypasses the cache + /// rather than risk a key that never hits or that collides with another caller's entry. + /// + private string? GetCacheKey(MethodContext context) { - var prefixKey = GetPrefixKey(context.Method.Name); - var argsKey = string.Join("_", context.Arguments.Select(arg => GetCacheKeyByArg(arg))); - - if (_perInstanceCache && context.Target != null) + var argKeys = new List(context.Arguments.Length); + foreach (var arg in context.Arguments) { - return $"{prefixKey}-{context.Target.GetHashCode()}_{argsKey}"; - } - else - { - return $"{prefixKey}_{argsKey}"; + if (!TryGetCacheKeyByArg(arg, out var argKey)) + { + Services.GetService>()?.LogDebug( + "SharpCache bypassed for {Method}: argument of type {ArgType} has no stable cache key.", + context.Method.Name, arg?.GetType().FullName); + return null; + } + argKeys.Add(argKey); } + + // Instance-scoped entries live on the instance itself, so the key needs no instance part. + return $"{GetPrefixKey(context.Method.Name)}_{string.Join("_", argKeys)}"; } private string GetPrefixKey(string name) @@ -113,27 +174,65 @@ private string GetPrefixKey(string name) } } - private string GetCacheKeyByArg(object? arg) + /// + /// Builds the key from the argument's content. Reference-type arguments used to be keyed by + /// GetHashCode(), i.e. object identity: a freshly built filter or list never hit the cache, + /// so every call parked another copy of the result, and a 32-bit hash collision could hand one + /// caller another caller's entry. + /// + private static bool TryGetCacheKeyByArg(object? arg, out string key) { - if (arg is null) + switch (arg) { - return NullMarker.GetHashCode().ToString(); + case null: + key = NullKey; + return true; + case ICacheKey withCacheKey: + key = withCacheKey.GetCacheKey(); + return true; + case string text: + // Length-prefixed so "a_b" + "c" and "a" + "b_c" cannot join into the same key. + key = text.Length <= MaxRawStringKeyLength ? $"s{text.Length}:{text}" : $"h:{Sha256(text)}"; + return true; + case DateTime or DateTimeOffset: + key = ((IFormattable)arg).ToString("o", CultureInfo.InvariantCulture); + return true; } - else if (arg is ICacheKey withCacheKey) + + var type = arg.GetType(); + if (type.IsPrimitive || type.IsEnum || arg is decimal or Guid or TimeSpan) + { + key = Convert.ToString(arg, CultureInfo.InvariantCulture) ?? string.Empty; + return true; + } + + try { - return withCacheKey.GetCacheKey(); + var json = JsonSerializer.Serialize(arg, type, KeyJsonOptions); + key = $"j:{Sha256(type.FullName + json)}"; + return true; } - else + catch (Exception ex) when (ex is NotSupportedException or JsonException or InvalidOperationException) { - return arg.GetHashCode().ToString(); + key = string.Empty; + return false; } } + private static string Sha256(string value) + => Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(value))); + public async Task ClearCacheAsync() { var tenantId = GetTenantId(); var prefix = string.IsNullOrWhiteSpace(tenantId) ? _settings.Prefix : $"{_settings.Prefix}:{tenantId}"; await _cache.ClearCacheAsync(prefix); + + // Instance-scoped entries were cleared along with the shared cache before they moved out of it. + foreach (var (_, entries) in InstanceCaches) + { + entries.Clear(); + } } } diff --git a/src/Infrastructure/BotSharp.Abstraction/Repositories/Filters/AgentFilter.cs b/src/Infrastructure/BotSharp.Abstraction/Repositories/Filters/AgentFilter.cs index ea42ecd70..b2c40afa5 100644 --- a/src/Infrastructure/BotSharp.Abstraction/Repositories/Filters/AgentFilter.cs +++ b/src/Infrastructure/BotSharp.Abstraction/Repositories/Filters/AgentFilter.cs @@ -1,6 +1,8 @@ +using BotSharp.Abstraction.Infrastructures; + namespace BotSharp.Abstraction.Repositories.Filters; -public class AgentFilter +public class AgentFilter : ICacheKey { public Pagination Pager { get; set; } = new Pagination(); public List? AgentIds { get; set; } @@ -16,4 +18,17 @@ public static AgentFilter Empty() { return new AgentFilter(); } + + /// + /// Content-based, so equal filters share one cache entry. RuleEngine builds a new filter on every + /// trigger; keyed by identity, each trigger cached another full copy of every agent. + /// Every filter field must be part of the key, or two different filters would share a result. + /// + public string GetCacheKey() + => $"{nameof(AgentFilter)}_{Pager?.GetCacheKey()}_{JoinKey(AgentIds)}_{JoinKey(AgentNames)}_{SimilarName}" + + $"_{Disabled}_{Installed}_{JoinKey(Types)}_{JoinKey(Labels)}_{IsPublic}"; + + // Joined with the ASCII unit separator, which is not expected in agent ids, names, types or labels. + private static string JoinKey(List? values) + => values == null ? "null" : $"[{string.Join('\u001f', values)}]"; } diff --git a/src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.GetAgents.cs b/src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.GetAgents.cs index 035553d99..a8aa1b5a7 100644 --- a/src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.GetAgents.cs +++ b/src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.GetAgents.cs @@ -61,7 +61,8 @@ public async Task GetAgent(string id) var agentSetting = _services.GetRequiredService(); if (profile.LlmConfig == null) { - profile.LlmConfig = agentSetting.LlmConfig; + // A copy: flagging IsInherit on the shared settings instance marked the default config itself as inherited. + profile.LlmConfig = agentSetting.LlmConfig.DeepClone() ?? new AgentLlmConfig(); profile.LlmConfig.IsInherit = true; } else if (string.IsNullOrEmpty(profile.LlmConfig?.Provider) || string.IsNullOrEmpty(profile.LlmConfig?.Model)) diff --git a/src/Infrastructure/BotSharp.Core/Routing/Hooks/RoutingAgentHook.cs b/src/Infrastructure/BotSharp.Core/Routing/Hooks/RoutingAgentHook.cs index 923911a28..82c7a2cc1 100644 --- a/src/Infrastructure/BotSharp.Core/Routing/Hooks/RoutingAgentHook.cs +++ b/src/Infrastructure/BotSharp.Core/Routing/Hooks/RoutingAgentHook.cs @@ -1,3 +1,4 @@ +using BotSharp.Abstraction.Routing.Models; using BotSharp.Abstraction.Routing.Settings; namespace BotSharp.Core.Routing.Hooks; @@ -32,6 +33,19 @@ public override async Task OnInstructionLoaded(string template, IDictionar agents = agents.Except(planners).ToArray(); } + // GetRoutableAgents is cached and shared by every conversation, so the fields below are removed from + // per-conversation copies; removing them in place stripped them for every other conversation too. + agents = agents.Select(x => new RoutableAgent + { + AgentId = x.AgentId, + Name = x.Name, + Description = x.Description, + Type = x.Type, + Profiles = x.Profiles, + RequiredFields = [.. x.RequiredFields], + OptionalFields = [.. x.OptionalFields] + }).ToArray(); + // Postprocess agent required fields, remove it if the states exists var states = _services.GetRequiredService(); foreach (var agent in agents) diff --git a/src/Infrastructure/BotSharp.OpenAPI/Controllers/Agent/AgentController.cs b/src/Infrastructure/BotSharp.OpenAPI/Controllers/Agent/AgentController.cs index 1c894ca3f..e6db27b10 100644 --- a/src/Infrastructure/BotSharp.OpenAPI/Controllers/Agent/AgentController.cs +++ b/src/Infrastructure/BotSharp.OpenAPI/Controllers/Agent/AgentController.cs @@ -40,8 +40,15 @@ public AgentSettings GetSettings() AgentIds = [id] }); - var foundAgent = pagedAgents.Items.FirstOrDefault(); - if (foundAgent == null) return null; + var cachedAgent = pagedAgents.Items.FirstOrDefault(); + if (cachedAgent == null) return null; + + // GetAgents is cached and shared with every other caller. Inherit into a copy with its own lists, + // and give the view its own routing rules, because RedirectToAgentName is written to them below. + var foundAgent = cachedAgent.ShallowCopy(); + foundAgent.Templates = cachedAgent.Templates?.ToList() ?? []; + foundAgent.Functions = cachedAgent.Functions?.ToList() ?? []; + foundAgent.RoutingRules = cachedAgent.RoutingRules?.Select(x => x.Clone()).ToList() ?? []; await _agentService.InheritAgent(foundAgent); var targetAgent = AgentViewModel.FromAgent(foundAgent); diff --git a/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs new file mode 100644 index 000000000..098c75271 --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs @@ -0,0 +1,268 @@ +using System.Reflection; +using BotSharp.Abstraction.Agents; +using BotSharp.Abstraction.Agents.Models; +using BotSharp.Abstraction.Agents.Settings; +using BotSharp.Abstraction.Infrastructures; +using BotSharp.Abstraction.Options; +using BotSharp.Abstraction.Plugins; +using BotSharp.Abstraction.Plugins.Models; +using BotSharp.Abstraction.Repositories; +using BotSharp.Abstraction.Repositories.Filters; +using BotSharp.Abstraction.Routing.Settings; +using BotSharp.Abstraction.Rules; +using BotSharp.Abstraction.Utilities; +using BotSharp.Core.Agents.Services; +using BotSharp.Core.Infrastructures; +using BotSharp.Core.Plugins; +using BotSharp.Core.Rules.Engines; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; +using Xunit.Abstractions; + +namespace BotSharp.Core.UnitTests.Infrastructures; + +/// +/// Guards against the OneBrain memory saw-tooth (1.8-2.8 GB against a 3 GiB limit, GTR-13556). +/// +/// used to key reference-type arguments by GetHashCode(), i.e. object +/// identity. builds a new on every call, so each +/// trigger missed and parked another full copy of every agent in the static MemoryCache for 10 minutes. +/// perInstanceCache entries were also parked there, one dead copy per DI scope. +/// +/// Uses the real, Rougamo-woven ; only the repository is mocked, returning fresh +/// objects on every call the way the Mongo driver deserializes them. +/// +public class SharpCacheAgentMemoryTests : IDisposable +{ + // Roughly the shape of OneBrain's agent collection: ~60 agents, ~2 MB of JSON on disk. + private const int AgentCount = 60; + private const int InstructionChars = 8_000; + private const int TemplateChars = 4_000; + private const int Calls = 50; + + private readonly ITestOutputHelper _output; + + public SharpCacheAgentMemoryTests(ITestOutputHelper output) + { + _output = output; + ClearSharpCache(); + } + + public void Dispose() => ClearSharpCache(); + + [Fact] + public async Task GetAgents_with_equal_new_filters_reads_once_and_caches_one_copy() + { + var (provider, db) = Build(); + var agentService = provider.GetRequiredService(); + + var entriesBefore = SharpCacheEntryCount(); + var bytesBefore = RetainedBytes(); + + for (var i = 0; i < Calls; i++) + { + // Exactly what RuleEngine.Triggered does on every call. + await agentService.GetAgents(new AgentFilter { Pager = new Pagination { Size = 1000 } }); + } + + var retained = RetainedBytes() - bytesBefore; + _output.WriteLine($"[new filter per call] calls={Calls}, repo reads={ReadCount(db)}, cache entries added={SharpCacheEntryCount() - entriesBefore}, retained={retained / 1024 / 1024} MB"); + + Assert.Equal(1, ReadCount(db)); + Assert.Equal(1, SharpCacheEntryCount() - entriesBefore); + Assert.True(retained < 3 * EstimatedBytesPerCopy(), $"Expected about one cached copy, got {retained} bytes retained."); + } + + [Fact] + public async Task GetAgents_with_different_filters_does_not_share_an_entry() + { + var (provider, db) = Build(); + var agentService = provider.GetRequiredService(); + + await agentService.GetAgents(new AgentFilter { AgentIds = ["agent-1"] }); + await agentService.GetAgents(new AgentFilter { AgentIds = ["agent-2"] }); + await agentService.GetAgents(new AgentFilter { AgentIds = ["agent-1"] }); + + Assert.Equal(2, ReadCount(db)); + } + + [Fact] + public async Task RuleEngine_Triggered_once_per_work_order_reads_the_agents_once() + { + var (provider, db) = Build(); + var engine = new RuleEngine(provider, NullLogger.Instance); + + var entriesBefore = SharpCacheEntryCount(); + for (var wo = 0; wo < Calls; wo++) + { + await engine.Triggered(new StubTrigger { EntityId = wo.ToString() }, $"Work order {wo} has been on site for over 6 hours."); + } + + _output.WriteLine($"[RuleEngine x {Calls} work orders] repo reads={ReadCount(db)}, cache entries added={SharpCacheEntryCount() - entriesBefore}"); + Assert.Equal(1, ReadCount(db)); + Assert.Equal(1, SharpCacheEntryCount() - entriesBefore); + } + + /// + /// GetAgent is perInstanceCache: callers mutate the agent it returns, so each scope keeps its own + /// copy. That copy now lives on the scoped AgentService instead of in the shared cache. + /// + [Fact] + public async Task GetAgent_caches_per_scope_without_adding_shared_cache_entries() + { + var (provider, db) = Build(); + const int Scopes = 20; + + var entriesBefore = SharpCacheEntryCount(); + for (var i = 0; i < Scopes; i++) + { + using var scope = provider.CreateScope(); + var agentService = scope.ServiceProvider.GetRequiredService(); + var first = await agentService.GetAgent("agent-0"); + var second = await agentService.GetAgent("agent-0"); + Assert.Same(first, second); + } + + Assert.Equal(Scopes, db.Invocations.Count(x => x.Method.Name == nameof(IBotSharpRepository.GetAgent))); + Assert.Equal(0, SharpCacheEntryCount() - entriesBefore); + } + + [Fact] + public async Task GetAgent_does_not_mark_the_shared_default_llm_config_as_inherited() + { + var (provider, db) = Build(); + db.Setup(x => x.GetAgent(It.IsAny(), It.IsAny())) + .ReturnsAsync((string id, bool _) => { var agent = BuildAgent(id); agent.LlmConfig = null!; return agent; }); + + using var scope = provider.CreateScope(); + var agent = await scope.ServiceProvider.GetRequiredService().GetAgent("agent-0"); + + var settings = provider.GetRequiredService(); + Assert.True(agent.LlmConfig.IsInherit); + Assert.False(settings.LlmConfig.IsInherit); + Assert.NotSame(settings.LlmConfig, agent.LlmConfig); + } + + [Fact] + public void Equal_lists_produce_the_same_key() + => Assert.Equal(KeyOf(new List { 1, 2, 3 }), KeyOf(new List { 1, 2, 3 })); + + [Fact] + public void Different_lists_produce_different_keys() + => Assert.NotEqual(KeyOf(new List { 1, 2, 3 }), KeyOf(new List { 1, 2, 4 })); + + [Fact] + public void Strings_are_length_prefixed_so_joined_arguments_cannot_collide() + => Assert.NotEqual(KeyOf("a_b") + "_" + KeyOf("c"), KeyOf("a") + "_" + KeyOf("b_c")); + + [Fact] + public void Long_strings_are_hashed_into_a_bounded_key() + { + var key = KeyOf(new string('x', 10_000)); + Assert.True(key!.Length < 100); + Assert.NotEqual(key, KeyOf(new string('x', 9_999) + "y")); + } + + [Fact] + public void An_argument_that_cannot_be_serialized_has_no_key() + { + var cyclic = new Node(); + cyclic.Next = cyclic; + Assert.Null(KeyOf(cyclic)); + } + + private sealed class Node + { + public Node? Next { get; set; } + } + + private sealed class StubTrigger : IRuleTrigger + { + public string EntityType { get; set; } = "client_WOReactive"; + public string EntityId { get; set; } = "0"; + public string Name => "OnSiteOverstayRuleTrigger"; + public string Channel => "crontab"; + } + + /// The key built for one argument, or null when the call would bypass the cache. + private static string? KeyOf(object? arg) + { + var method = typeof(SharpCacheAttribute).GetMethod("TryGetCacheKeyByArg", BindingFlags.NonPublic | BindingFlags.Static)!; + var args = new object?[] { arg, null }; + return (bool)method.Invoke(null, args)! ? (string)args[1]! : null; + } + + private static (ServiceProvider provider, Mock db) Build() + { + var db = new Mock(); + // A fresh list per call, as the Mongo driver would deserialize it. + db.Setup(x => x.GetAgents(It.IsAny())).ReturnsAsync(() => BuildAgents()); + db.Setup(x => x.GetAgent(It.IsAny(), It.IsAny())) + .ReturnsAsync((string id, bool _) => BuildAgent(id)); + db.Setup(x => x.GetPluginConfig()).ReturnsAsync(new PluginConfig()); + + var services = new ServiceCollection(); + services.AddSingleton(db.Object); + services.AddSingleton(new SharpCacheSettings { Enabled = true, Prefix = "test" }); + services.AddSingleton(); + services.AddSingleton(); + services.AddSingleton(new AgentSettings()); + services.AddSingleton(new RoutingSettings()); + services.AddSingleton(new BotSharpOptions()); + services.AddSingleton(new PluginLoader(new ServiceCollection(), new ConfigurationBuilder().Build(), new PluginSettings())); + services.AddSingleton(); + services.AddSingleton(typeof(ILogger<>), typeof(NullLogger<>)); + // Scoped, as in BotSharp's own registration - this is what makes perInstanceCache per scope. + services.AddScoped(); + + var provider = services.BuildServiceProvider(); + SharpCacheAttribute.Services = provider; + return (provider, db); + } + + private static List BuildAgents() + => Enumerable.Range(0, AgentCount).Select(i => BuildAgent($"agent-{i}")).ToList(); + + private static Agent BuildAgent(string id) => new() + { + Id = id, + Name = id, + Disabled = false, + Instruction = new string('i', InstructionChars), + Templates = + [ + new AgentTemplate("t1", new string('a', TemplateChars)), + new AgentTemplate("t2", new string('b', TemplateChars)) + ], + Rules = [] + }; + + // UTF-16 strings dominate; object overhead ignored, so this is a lower bound. + private static long EstimatedBytesPerCopy() => (long)AgentCount * (InstructionChars + 2 * TemplateChars) * sizeof(char); + + private static int ReadCount(Mock db) + => db.Invocations.Count(x => x.Method.Name == nameof(IBotSharpRepository.GetAgents)); + + private static long RetainedBytes() + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + return GC.GetTotalMemory(forceFullCollection: true); + } + + private static MemoryCache SharpCache() + => (MemoryCache)typeof(MemoryCacheService) + .GetField("_cache", BindingFlags.NonPublic | BindingFlags.Static)! + .GetValue(null)!; + + private static int SharpCacheEntryCount() => SharpCache().Count; + + private static void ClearSharpCache() => SharpCache().Compact(1.0); +} From fd55c23e817a753a718c941986789ecbe9f41358 Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Fri, 9 Oct 2026 13:51:59 +0800 Subject: [PATCH 2/5] GTR-13556 Tests for callers of now-shared cached results - RoutingAgentHook: one conversation's filled fields are removed from a copy, not from the cached routable agents the next conversation gets. - AgentController.GetAgent: inheriting and resolving redirect names leave the cached agent untouched. - perInstanceCache entries are collected together with their scope, and Utilities.ClearCache still clears them. Co-Authored-By: Claude Opus 5.5 --- .../BotSharp.Core.UnitTests.csproj | 2 + .../SharpCacheAgentMemoryTests.cs | 44 ++++++ .../SharpCacheSharedResultTests.cs | 131 ++++++++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheSharedResultTests.cs diff --git a/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj b/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj index 46f2f5e5b..7c97ed97f 100644 --- a/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj +++ b/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj @@ -34,6 +34,8 @@ + + diff --git a/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs index 098c75271..f58bead61 100644 --- a/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs +++ b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs @@ -1,4 +1,5 @@ using System.Reflection; +using System.Runtime.CompilerServices; using BotSharp.Abstraction.Agents; using BotSharp.Abstraction.Agents.Models; using BotSharp.Abstraction.Agents.Settings; @@ -133,6 +134,49 @@ public async Task GetAgent_caches_per_scope_without_adding_shared_cache_entries( Assert.Equal(0, SharpCacheEntryCount() - entriesBefore); } + /// + /// The point of moving per-instance entries out of the shared cache: once the scope is gone, nothing + /// holds its cached agent any more, instead of it lingering for the full 10 minutes. + /// + [Fact] + public async Task GetAgent_entry_is_collected_with_its_scope() + { + var (provider, db) = Build(); + + var (serviceRef, agentRef) = await LoadInDisposedScope(provider); + // Moq records every return value; drop those records so they do not keep the agent alive. + db.Invocations.Clear(); + RetainedBytes(); + + Assert.False(serviceRef.IsAlive, "The scoped AgentService should be collectable once its scope is disposed."); + Assert.False(agentRef.IsAlive, "The per-instance cache entry should be collected together with its instance."); + } + + [Fact] + public async Task ClearCache_also_clears_per_instance_entries() + { + var (provider, db) = Build(); + using var scope = provider.CreateScope(); + var agentService = scope.ServiceProvider.GetRequiredService(); + + await agentService.GetAgent("agent-0"); + // What CreateAgent/UpdateAgent call after changing an agent. + Utilities.ClearCache(); + await agentService.GetAgent("agent-0"); + + Assert.Equal(2, db.Invocations.Count(x => x.Method.Name == nameof(IBotSharpRepository.GetAgent))); + } + + // Kept out of the test method so no local in the caller's frame roots the scope or the agent. + [MethodImpl(MethodImplOptions.NoInlining)] + private static async Task<(WeakReference service, WeakReference agent)> LoadInDisposedScope(IServiceProvider provider) + { + using var scope = provider.CreateScope(); + var agentService = scope.ServiceProvider.GetRequiredService(); + var agent = await agentService.GetAgent("agent-0"); + return (new WeakReference(agentService), new WeakReference(agent)); + } + [Fact] public async Task GetAgent_does_not_mark_the_shared_default_llm_config_as_inherited() { diff --git a/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheSharedResultTests.cs b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheSharedResultTests.cs new file mode 100644 index 000000000..864968d0c --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheSharedResultTests.cs @@ -0,0 +1,131 @@ +using BotSharp.Abstraction.Agents; +using BotSharp.Abstraction.Agents.Enums; +using BotSharp.Abstraction.Agents.Models; +using BotSharp.Abstraction.Agents.Settings; +using BotSharp.Abstraction.Conversations; +using BotSharp.Abstraction.Functions.Models; +using BotSharp.Abstraction.Repositories.Filters; +using BotSharp.Abstraction.Routing; +using BotSharp.Abstraction.Routing.Models; +using BotSharp.Abstraction.Routing.Settings; +using BotSharp.Abstraction.Tasks; +using BotSharp.Abstraction.Users; +using BotSharp.Abstraction.Users.Models; +using BotSharp.Abstraction.Utilities; +using BotSharp.Core.Routing.Hooks; +using BotSharp.OpenAPI.Controllers; +using Microsoft.Extensions.DependencyInjection; +using Moq; +using Xunit; + +namespace BotSharp.Core.UnitTests.Infrastructures; + +/// +/// Once SharpCache keys are content-based (GTR-13556), GetRoutableAgents and GetAgents hand the +/// same cached objects to every caller. These pin that the two callers which used to modify what they got +/// back now work on copies, so one conversation or request cannot change what the next one sees. +/// +public class SharpCacheSharedResultTests +{ + [Fact] + public async Task RoutingAgentHook_removes_filled_fields_from_a_copy_not_from_the_cached_routable_agents() + { + // What the cache returns: the same array instance on every call. + var cached = new[] + { + new RoutableAgent + { + AgentId = "task-1", + Name = "task-1", + Type = AgentType.Task, + RequiredFields = [new ParameterPropertyDef("wo_id", "work order id"), new ParameterPropertyDef("client_id", "client id")], + OptionalFields = [new ParameterPropertyDef("note", "note")] + } + }; + + // Conversation A already has every field filled; conversation B has none of them. + var dictA = await RunRoutingHook(cached, filledStates: ["wo_id", "client_id", "note"]); + var dictB = await RunRoutingHook(cached, filledStates: []); + + var agentA = ((RoutableAgent[])dictA["routing_agents"]).Single(); + var agentB = ((RoutableAgent[])dictB["routing_agents"]).Single(); + + Assert.Empty(agentA.RequiredFields); + Assert.Empty(agentA.OptionalFields); + Assert.Equal(["wo_id", "client_id"], agentB.RequiredFields.Select(x => x.Name)); + Assert.Equal(["note"], agentB.OptionalFields.Select(x => x.Name)); + + // The cached entry itself is untouched. + Assert.Equal(2, cached[0].RequiredFields.Count); + Assert.Single(cached[0].OptionalFields); + } + + [Fact] + public async Task AgentController_GetAgent_does_not_modify_the_cached_agent() + { + var cachedAgent = new Agent + { + Id = "child", + Name = "child", + InheritAgentId = "parent", + Templates = [new AgentTemplate("own", "own template")], + Functions = [new FunctionDef { Name = "own_fn" }], + RoutingRules = [new RoutingRule { Field = "wo_id", RedirectTo = "redirect" }] + }; + var redirectAgent = new Agent { Id = "redirect", Name = "Redirect Agent" }; + + var agentService = new Mock(); + agentService.Setup(x => x.GetAgents(It.Is(f => f.AgentIds!.Contains("child")))) + .ReturnsAsync(new PagedItems { Items = [cachedAgent], Count = 1 }); + agentService.Setup(x => x.GetAgents(It.Is(f => f.AgentIds!.Contains("redirect")))) + .ReturnsAsync(new PagedItems { Items = [redirectAgent], Count = 1 }); + // Mirrors the real InheritAgent: appends the parent's templates and functions. + agentService.Setup(x => x.InheritAgent(It.IsAny())) + .Callback((Agent agent) => + { + agent.Templates.Add(new AgentTemplate("inherited", "parent template")); + agent.Functions.Add(new FunctionDef { Name = "inherited_fn" }); + }) + .Returns(Task.CompletedTask); + + var userService = new Mock(); + userService.Setup(x => x.GetUserAuthorizations(It.IsAny?>())).ReturnsAsync(new UserAuthorization()); + + var services = new ServiceCollection(); + services.AddSingleton(new AgentSettings()); + services.AddSingleton(userService.Object); + var controller = new AgentController(agentService.Object, Mock.Of(), Mock.Of(), services.BuildServiceProvider()); + + var view = await controller.GetAgent("child"); + + // The response carries the inherited items and the resolved redirect name... + Assert.Equal(["own", "inherited"], view!.Templates.Select(x => x.Name)); + Assert.Equal(["own_fn", "inherited_fn"], view.Functions.Select(x => x.Name)); + Assert.Equal("Redirect Agent", view.RoutingRules.Single().RedirectToAgentName); + + // ...but none of it leaked into the agent other callers get from the cache. + Assert.Equal(["own"], cachedAgent.Templates.Select(x => x.Name)); + Assert.Equal(["own_fn"], cachedAgent.Functions.Select(x => x.Name)); + Assert.Null(cachedAgent.RoutingRules.Single().RedirectToAgentName); + } + + private static async Task> RunRoutingHook(RoutableAgent[] cached, string[] filledStates) + { + var routing = new Mock(); + routing.Setup(x => x.GetRoutableAgents(It.IsAny>())).ReturnsAsync(cached); + + var states = new Mock(); + states.Setup(x => x.ContainsState(It.IsAny())).Returns((string name) => filledStates.Contains(name)); + + var services = new ServiceCollection(); + services.AddSingleton(routing.Object); + services.AddSingleton(states.Object); + + var hook = new RoutingAgentHook(services.BuildServiceProvider(), new AgentSettings(), new RoutingSettings()); + hook.SetAgent(new Agent { Id = "router", Name = "router", Type = AgentType.Routing }); + + var dict = new Dictionary(); + await hook.OnInstructionLoaded("template", dict); + return dict; + } +} From 3409b7fbc9458ae9d8ff763670eec2fd4a1378fb Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Fri, 9 Oct 2026 14:01:41 +0800 Subject: [PATCH 3/5] GTR-13556 AgentFilter: rely on the content-hash cache key Drop the hand-written AgentFilter.GetCacheKey. The generic SharpCache key already hashes the filter's JSON, which picks up any field added later; a hand-written key silently serves wrong results when a new field is left out, and its field-by-field "_" join let text shift between adjacent free-text fields into the same key. Co-Authored-By: Claude Opus 5.5 --- .../Repositories/Filters/AgentFilter.cs | 17 +---------------- .../SharpCacheAgentMemoryTests.cs | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/Infrastructure/BotSharp.Abstraction/Repositories/Filters/AgentFilter.cs b/src/Infrastructure/BotSharp.Abstraction/Repositories/Filters/AgentFilter.cs index b2c40afa5..ea42ecd70 100644 --- a/src/Infrastructure/BotSharp.Abstraction/Repositories/Filters/AgentFilter.cs +++ b/src/Infrastructure/BotSharp.Abstraction/Repositories/Filters/AgentFilter.cs @@ -1,8 +1,6 @@ -using BotSharp.Abstraction.Infrastructures; - namespace BotSharp.Abstraction.Repositories.Filters; -public class AgentFilter : ICacheKey +public class AgentFilter { public Pagination Pager { get; set; } = new Pagination(); public List? AgentIds { get; set; } @@ -18,17 +16,4 @@ public static AgentFilter Empty() { return new AgentFilter(); } - - /// - /// Content-based, so equal filters share one cache entry. RuleEngine builds a new filter on every - /// trigger; keyed by identity, each trigger cached another full copy of every agent. - /// Every filter field must be part of the key, or two different filters would share a result. - /// - public string GetCacheKey() - => $"{nameof(AgentFilter)}_{Pager?.GetCacheKey()}_{JoinKey(AgentIds)}_{JoinKey(AgentNames)}_{SimilarName}" + - $"_{Disabled}_{Installed}_{JoinKey(Types)}_{JoinKey(Labels)}_{IsPublic}"; - - // Joined with the ASCII unit separator, which is not expected in agent ids, names, types or labels. - private static string JoinKey(List? values) - => values == null ? "null" : $"[{string.Join('\u001f', values)}]"; } diff --git a/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs index f58bead61..5f5edaaa2 100644 --- a/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs +++ b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs @@ -93,6 +93,22 @@ public async Task GetAgents_with_different_filters_does_not_share_an_entry() Assert.Equal(2, ReadCount(db)); } + /// + /// Joined field by field with "_", these two read the same ("[a]_[b]_[c]_z"), because text can move + /// between adjacent free-text fields. The content hash keeps them apart. + /// + [Fact] + public async Task GetAgents_filters_whose_fields_would_splice_into_the_same_text_stay_separate() + { + var (provider, db) = Build(); + var agentService = provider.GetRequiredService(); + + await agentService.GetAgents(new AgentFilter { AgentIds = ["a"], AgentNames = ["b"], SimilarName = "[c]_z" }); + await agentService.GetAgents(new AgentFilter { AgentIds = ["a]_[b"], AgentNames = ["c"], SimilarName = "z" }); + + Assert.Equal(2, ReadCount(db)); + } + [Fact] public async Task RuleEngine_Triggered_once_per_work_order_reads_the_agents_once() { From 6fd185734dbb50574050c099f2b27bcbcd10f787 Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Fri, 9 Oct 2026 14:09:10 +0800 Subject: [PATCH 4/5] GTR-13556 RoutingAgentHook: filter filled fields while copying Build each conversation's routable agents with the already-filled fields left out, instead of copying the cached agents and then removing fields from the copies. Same result, one pass, no list mutation. Co-Authored-By: Claude Opus 5.5 --- .../Routing/Hooks/RoutingAgentHook.cs | 34 +++---------------- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/src/Infrastructure/BotSharp.Core/Routing/Hooks/RoutingAgentHook.cs b/src/Infrastructure/BotSharp.Core/Routing/Hooks/RoutingAgentHook.cs index 82c7a2cc1..928d10329 100644 --- a/src/Infrastructure/BotSharp.Core/Routing/Hooks/RoutingAgentHook.cs +++ b/src/Infrastructure/BotSharp.Core/Routing/Hooks/RoutingAgentHook.cs @@ -33,8 +33,9 @@ public override async Task OnInstructionLoaded(string template, IDictionar agents = agents.Except(planners).ToArray(); } - // GetRoutableAgents is cached and shared by every conversation, so the fields below are removed from - // per-conversation copies; removing them in place stripped them for every other conversation too. + // GetRoutableAgents is cached and shared by every conversation: build this conversation's view + // without the fields it already has, instead of removing them from the cached agents. + var states = _services.GetRequiredService(); agents = agents.Select(x => new RoutableAgent { AgentId = x.AgentId, @@ -42,35 +43,10 @@ public override async Task OnInstructionLoaded(string template, IDictionar Description = x.Description, Type = x.Type, Profiles = x.Profiles, - RequiredFields = [.. x.RequiredFields], - OptionalFields = [.. x.OptionalFields] + RequiredFields = x.RequiredFields.Where(f => !states.ContainsState(f.Name)).ToList(), + OptionalFields = x.OptionalFields.Where(f => !states.ContainsState(f.Name)).ToList() }).ToArray(); - // Postprocess agent required fields, remove it if the states exists - var states = _services.GetRequiredService(); - foreach (var agent in agents) - { - var fields = agent.RequiredFields.ToArray(); - foreach (var field in fields) - { - if (states.ContainsState(field.Name)) - { - var requiredField = agent.RequiredFields.First(x => x.Name == field.Name); - agent.RequiredFields.Remove(requiredField); - } - } - - fields = agent.OptionalFields.ToArray(); - foreach (var field in fields) - { - if (states.ContainsState(field.Name)) - { - var optionalField = agent.OptionalFields.First(x => x.Name == field.Name); - agent.OptionalFields.Remove(optionalField); - } - } - } - dict["routing_agents"] = agents; return await base.OnInstructionLoaded(template, dict); From 716f902d4d17cd2f523977d471a5ac8da4b8f721 Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Fri, 9 Oct 2026 14:30:04 +0800 Subject: [PATCH 5/5] GTR-13556 SharpCache: include the declaring type in the cache key The key prefix was the method name only, so same-named methods on different classes with equal arguments shared one entry. OneBrain loads both CsvNERDataLoader and OneDbEntityDataLoader, whose argument-less, cached LoadVocabularyAsync() therefore served whichever vocabulary loaded first to both for 60 minutes. The test project now references Rougamo.Fody so tests can weave [SharpCache] onto their own classes; FodyWeavers.xml is generated on build, as for the src projects. Co-Authored-By: Claude Opus 5.5 --- .../Infrastructures/SharpCacheAttribute.cs | 10 ++++- .../BotSharp.Core.UnitTests.csproj | 2 + .../SharpCacheAgentMemoryTests.cs | 38 +++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs b/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs index d46cd3112..d4df44bd9 100644 --- a/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs +++ b/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs @@ -6,6 +6,7 @@ using Rougamo; using System.Collections.Concurrent; using System.Globalization; +using System.Reflection; using System.Runtime.CompilerServices; using System.Security.Cryptography; using System.Text.Json; @@ -147,11 +148,16 @@ public virtual Task IsOutOfDate(MethodContext context, object value) } // Instance-scoped entries live on the instance itself, so the key needs no instance part. - return $"{GetPrefixKey(context.Method.Name)}_{string.Join("_", argKeys)}"; + return $"{GetPrefixKey(context.Method)}_{string.Join("_", argKeys)}"; } - private string GetPrefixKey(string name) + /// + /// Includes the declaring type: keyed by method name alone, two classes with a same-named method and equal + /// arguments shared one entry, e.g. two IEntityDataLoader implementations' LoadVocabularyAsync(). + /// + private string GetPrefixKey(MethodBase method) { + var name = $"{method.DeclaringType?.FullName ?? method.DeclaringType?.Name}.{method.Name}"; var tenantId = GetTenantId(); if (!string.IsNullOrWhiteSpace(tenantId)) { diff --git a/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj b/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj index 7c97ed97f..f8b3c619c 100644 --- a/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj +++ b/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj @@ -11,6 +11,8 @@ + + runtime; build; native; contentfiles; analyzers; buildtransitive diff --git a/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs index 5f5edaaa2..0cc30c039 100644 --- a/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs +++ b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs @@ -237,6 +237,44 @@ public void An_argument_that_cannot_be_serialized_has_no_key() Assert.Null(KeyOf(cyclic)); } + /// + /// OneBrain loads both CsvNERDataLoader and OneDbEntityDataLoader, each with a cached, argument-less + /// LoadVocabularyAsync(). Keyed by method name alone, whichever ran first served its vocabulary to both. + /// + [Fact] + public async Task Same_named_methods_on_different_classes_do_not_share_an_entry() + { + Build(); + CsvVocabularyLoader.Loads = 0; + + var first = await new CsvVocabularyLoader().LoadVocabularyAsync(); + var second = await new DbVocabularyLoader().LoadVocabularyAsync(); + await new CsvVocabularyLoader().LoadVocabularyAsync(); + + Assert.Equal("csv", first); + Assert.Equal("db", second); + // Proves the test classes are actually woven: without SharpCache the second csv call would load again. + Assert.Equal(1, CsvVocabularyLoader.Loads); + } + + public class CsvVocabularyLoader + { + public static int Loads; + + [SharpCache(60)] + public async Task LoadVocabularyAsync() + { + Interlocked.Increment(ref Loads); + return await Task.FromResult("csv"); + } + } + + public class DbVocabularyLoader + { + [SharpCache(60)] + public async Task LoadVocabularyAsync() => await Task.FromResult("db"); + } + private sealed class Node { public Node? Next { get; set; }