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..d4df44bd9 100644 --- a/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs +++ b/src/Infrastructure/BotSharp.Abstraction/Infrastructures/SharpCacheAttribute.cs @@ -2,7 +2,14 @@ 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.Reflection; +using System.Runtime.CompilerServices; +using System.Security.Cryptography; +using System.Text.Json; using Rougamo.Context; namespace BotSharp.Core.Infrastructures; @@ -10,7 +17,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 +51,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 +89,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,23 +128,36 @@ 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)}_{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)) { @@ -113,27 +180,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.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..928d10329 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,30 +33,19 @@ public override async Task OnInstructionLoaded(string template, IDictionar agents = agents.Except(planners).ToArray(); } - // Postprocess agent required fields, remove it if the states exists + // 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(); - foreach (var agent in agents) + agents = agents.Select(x => new RoutableAgent { - 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); - } - } - } + AgentId = x.AgentId, + Name = x.Name, + Description = x.Description, + Type = x.Type, + Profiles = x.Profiles, + RequiredFields = x.RequiredFields.Where(f => !states.ContainsState(f.Name)).ToList(), + OptionalFields = x.OptionalFields.Where(f => !states.ContainsState(f.Name)).ToList() + }).ToArray(); dict["routing_agents"] = 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/BotSharp.Core.UnitTests.csproj b/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj index 46f2f5e5b..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 @@ -34,6 +36,8 @@ + + diff --git a/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs new file mode 100644 index 000000000..0cc30c039 --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/Infrastructures/SharpCacheAgentMemoryTests.cs @@ -0,0 +1,366 @@ +using System.Reflection; +using System.Runtime.CompilerServices; +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)); + } + + /// + /// 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() + { + 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); + } + + /// + /// 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() + { + 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)); + } + + /// + /// 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; } + } + + 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); +} 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; + } +}