diff --git a/RMuseum/Controllers/GanjoorController.cs b/RMuseum/Controllers/GanjoorController.cs index fdbdbb7e..41f7259e 100644 --- a/RMuseum/Controllers/GanjoorController.cs +++ b/RMuseum/Controllers/GanjoorController.cs @@ -9,7 +9,6 @@ using RMuseum.Models.Auth.Memory; using RMuseum.Models.Auth.ViewModel; using RMuseum.Models.Ganjoor; using RMuseum.Models.Ganjoor.PublicExport; -using RMuseum.Models.Ganjoor.SemanticSearch; using RMuseum.Models.Ganjoor.ViewModels; using RMuseum.Models.GanjoorAudio.ViewModels; using RMuseum.Models.GanjoorIntegration; @@ -5023,30 +5022,6 @@ namespace RMuseum.Controllers } - /// - /// semantic ("find a poem about...") search - /// - [HttpPost("search/semantic")] - [ProducesResponseType((int)HttpStatusCode.OK)] - [ProducesResponseType((int)HttpStatusCode.BadRequest, Type = typeof(string))] - public async Task SemanticSearch([FromBody] SemanticSearchRequestDto request) - { - try - { - var result = await _semanticSearchService.SearchAsync(request.Query, request.TopK); - return Ok(result); - } - catch (ArgumentException exp) - { - return BadRequest(exp.Message); - } - catch (Exception exp) - { - return BadRequest(exp.ToString()); - } - } - - /// @@ -5117,8 +5092,6 @@ namespace RMuseum.Controllers protected IConfiguration Configuration { get; } - protected ISemanticSearchService _semanticSearchService; - /// /// constructor /// @@ -5128,15 +5101,13 @@ namespace RMuseum.Controllers /// /// /// - /// public GanjoorController( IGanjoorService ganjoorService, IAppUserService appUserService, IHttpContextAccessor httpContextAccessor, IImageFileService imageFileService, IMemoryCache memoryCache, - IConfiguration configuration, - ISemanticSearchService semanticSearchService + IConfiguration configuration ) { _ganjoorService = ganjoorService; @@ -5145,7 +5116,6 @@ namespace RMuseum.Controllers _imageFileService = imageFileService; _memoryCache = memoryCache; Configuration = configuration; - _semanticSearchService = semanticSearchService; } } } diff --git a/RMuseum/Controllers/SemanticSearchController.cs b/RMuseum/Controllers/SemanticSearchController.cs new file mode 100644 index 00000000..b3b2ea37 --- /dev/null +++ b/RMuseum/Controllers/SemanticSearchController.cs @@ -0,0 +1,65 @@ +using Microsoft.AspNetCore.Mvc; +using RMuseum.Models.Ganjoor.SemanticSearch; +using RMuseum.Services.Implementation; +using RMuseum.Utils.SemanticSearch; +using System; +using System.Net; +using System.Threading.Tasks; + +namespace RMuseum.Controllers +{ + /// + /// Semantic ("find a poem about...") search — deliberately its own controller, not a method + /// on GanjoorController, after a production incident: GanjoorController's constructor took + /// ISemanticSearchService (indirectly requiring EmbeddingIndex/QueryEmbedder to load + /// successfully), so a resource-loading failure prevented the ENTIRE controller from being + /// constructed — a 503 on every endpoint under /api/ganjoor, not just this feature. Same + /// route prefix as before (api/ganjoor), so the endpoint's URL is unchanged + /// (POST /api/ganjoor/search/semantic) — only which controller class hosts it changed. + /// A future failure in this feature's own dependencies can now only ever affect this one + /// controller/endpoint, never GanjoorController or anything else. + /// + [Produces("application/json")] + [Route("api/ganjoor")] + [ApiController] + public class SemanticSearchController : ControllerBase + { + protected readonly ISemanticSearchService _semanticSearchService; + + public SemanticSearchController(ISemanticSearchService semanticSearchService) + { + _semanticSearchService = semanticSearchService; + } + + /// + /// semantic ("find a poem about...") search + /// + [HttpPost("search/semantic")] + [ProducesResponseType((int)HttpStatusCode.OK)] + [ProducesResponseType((int)HttpStatusCode.BadRequest, Type = typeof(string))] + [ProducesResponseType((int)HttpStatusCode.ServiceUnavailable, Type = typeof(string))] + public async Task SemanticSearch([FromBody] SemanticSearchRequestDto request) + { + try + { + var result = await _semanticSearchService.SearchAsync(request.Query, request.TopK); + return Ok(result); + } + catch (SemanticSearchUnavailableException exp) + { + // distinct from a plain 400/500 - lets a client (or a person reading logs) tell + // "this feature isn't loaded/configured right now" apart from a bad query or an + // actual crash + return StatusCode((int)HttpStatusCode.ServiceUnavailable, exp.Message); + } + catch (ArgumentException exp) + { + return BadRequest(exp.Message); + } + catch (Exception exp) + { + return BadRequest(exp.ToString()); + } + } + } +} diff --git a/RMuseum/RMuseum.xml b/RMuseum/RMuseum.xml index 2db4d337..c07cd3a8 100644 --- a/RMuseum/RMuseum.xml +++ b/RMuseum/RMuseum.xml @@ -2644,11 +2644,6 @@ - - - semantic ("find a poem about...") search - - readonly mode @@ -2689,7 +2684,7 @@ Configuration - + constructor @@ -2699,7 +2694,6 @@ - @@ -3436,6 +3430,24 @@ + + + Semantic ("find a poem about...") search — deliberately its own controller, not a method + on GanjoorController, after a production incident: GanjoorController's constructor took + ISemanticSearchService (indirectly requiring EmbeddingIndex/QueryEmbedder to load + successfully), so a resource-loading failure prevented the ENTIRE controller from being + constructed — a 503 on every endpoint under /api/ganjoor, not just this feature. Same + route prefix as before (api/ganjoor), so the endpoint's URL is unchanged + (POST /api/ganjoor/search/semantic) — only which controller class hosts it changed. + A future failure in this feature's own dependencies can now only ever affect this one + controller/endpoint, never GanjoorController or anything else. + + + + + semantic ("find a poem about...") search + + add site banner (send form) with these fields: alt, url and an image attachment @@ -23077,15 +23089,22 @@ - - Deliberately NOT a GanjoorService partial, unlike everything else in this project. - EmbeddingIndex (~530MB in memory) and QueryEmbedder (a loaded ONNX model) both need to be - true singletons — constructed once at startup, never per-request — while GanjoorService - and RMuseumDbContext are scoped per-request throughout this codebase. Injecting a - singleton's dependencies into a per-request class (or vice versa) is a real DI lifetime - bug, not just an inconsistency, so this stays a separate service registered as a - singleton itself (see INTEGRATION.md for the exact registration). - + + Deliberately NOT a GanjoorService partial, unlike everything else in this project. + EmbeddingIndex (~530MB in memory) and QueryEmbedder (a loaded ONNX model) both need to be + true singletons — constructed once at startup, never per-request — while GanjoorService + and RMuseumDbContext are scoped per-request throughout this codebase. Injecting a + singleton's dependencies into a per-request class (or vice versa) is a real DI lifetime + bug, not just an inconsistency, so this stays a separate service registered as a + singleton itself (see INTEGRATION.md for the exact registration). + + Depends on LazySemanticSearchResources rather than EmbeddingIndex/QueryEmbedder directly — + deliberately, after a production incident where eager, throwing DI factories for those two + meant a load failure (wrong/missing file paths) prevented GanjoorController itself from + being constructed, taking down every endpoint under /api/ganjoor with a 503, not just + semantic search. This class must never let a resource-loading failure become an unhandled + exception that propagates past SearchAsync — see the catch below. + @@ -24451,6 +24470,46 @@ L2-normalized the same way the indexed vectors are — see QueryEmbedder. + + + Wraps EmbeddingIndex + QueryEmbedder loading so a failure (missing files, wrong paths, + corrupt data) can NEVER take down anything else in the app. + + The original design registered EmbeddingIndex/QueryEmbedder as singletons whose DI + factories called EmbeddingIndex.Load(...)/`new QueryEmbedder(...)` directly — both of + which throw on failure. Because GanjoorController's constructor (indirectly, through + ISemanticSearchService) depended on them, a load failure meant the controller itself + couldn't be constructed — taking down EVERY endpoint under /api/ganjoor, not just semantic + search, with a 503. That's exactly what happened in production. This class exists so that + can't happen again: the actual load is deferred to first real use (not app/controller + construction), attempted at most once, and a failure is caught, logged, and remembered — + SearchAsync() then reports "search unavailable" as an ordinary result, not an exception + that propagates into breaking anything else. + + Also worth knowing if this server runs multiple IIS worker processes for the same app + pool: each process gets its own instance of this (and everything it loads) — "singleton" + only means one instance per process, not per server. See the migration notes for the + memory math this implies at scale. + + + + + Attempts to load the resources on first call (subsequent calls reuse the same result, + success or failure — this never retries automatically; a fresh app start is required + to try again after a config/file fix, which is the expected deploy-and-restart flow + anyway). Returns true and populates both out parameters if available; returns false + and populates otherwise. NEVER THROWS — that guarantee is the + entire point of this class. + + + + + Thrown by SemanticSearchService when the underlying resources aren't available — the + controller catches this specifically and returns HTTP 503 with the message, distinct from + a plain 400/500, so a client (or a person checking logs) can tell "this feature isn't + configured/loaded right now" apart from "the query itself was bad" or "something crashed". + + Embeds a single user-typed search query using the SAME ONNX model + tokenizer as diff --git a/RMuseum/Services/Implementation/SemanticSearchService.cs b/RMuseum/Services/Implementation/SemanticSearchService.cs index 346a8f02..a8ef6c3b 100644 --- a/RMuseum/Services/Implementation/SemanticSearchService.cs +++ b/RMuseum/Services/Implementation/SemanticSearchService.cs @@ -22,19 +22,24 @@ namespace RMuseum.Services.Implementation /// singleton's dependencies into a per-request class (or vice versa) is a real DI lifetime /// bug, not just an inconsistency, so this stays a separate service registered as a /// singleton itself (see INTEGRATION.md for the exact registration). + /// + /// Depends on LazySemanticSearchResources rather than EmbeddingIndex/QueryEmbedder directly — + /// deliberately, after a production incident where eager, throwing DI factories for those two + /// meant a load failure (wrong/missing file paths) prevented GanjoorController itself from + /// being constructed, taking down every endpoint under /api/ganjoor with a 503, not just + /// semantic search. This class must never let a resource-loading failure become an unhandled + /// exception that propagates past SearchAsync — see the catch below. /// public class SemanticSearchService : ISemanticSearchService { private const int DefaultTopK = 10; private const int MaxTopK = 50; - private readonly EmbeddingIndex _embeddingIndex; - private readonly QueryEmbedder _queryEmbedder; + private readonly LazySemanticSearchResources _resources; - public SemanticSearchService(EmbeddingIndex embeddingIndex, QueryEmbedder queryEmbedder) + public SemanticSearchService(LazySemanticSearchResources resources) { - _embeddingIndex = embeddingIndex; - _queryEmbedder = queryEmbedder; + _resources = resources; } public async Task SearchAsync(string query, int? topK) @@ -42,12 +47,18 @@ namespace RMuseum.Services.Implementation if (string.IsNullOrWhiteSpace(query)) throw new ArgumentException("query must not be empty", nameof(query)); + if (!_resources.TryGetResources(out var embeddingIndex, out var queryEmbedder, out var error)) + { + throw new SemanticSearchUnavailableException( + "Semantic search is not available right now" + (error != null ? $": {error}" : ".")); + } + int k = topK.GetValueOrDefault(DefaultTopK); if (k <= 0 || k > MaxTopK) k = DefaultTopK; - float[] queryVector = _queryEmbedder.EmbedQuery(query); - List<(int PoemId, float Score)> topMatches = _embeddingIndex.FindTopSimilar(queryVector, k); + float[] queryVector = queryEmbedder.EmbedQuery(query); + List<(int PoemId, float Score)> topMatches = embeddingIndex.FindTopSimilar(queryVector, k); var response = new SemanticSearchResponseDto { Query = query }; diff --git a/RMuseum/Startup.cs b/RMuseum/Startup.cs index 066a1060..de3ed703 100644 --- a/RMuseum/Startup.cs +++ b/RMuseum/Startup.cs @@ -327,23 +327,14 @@ namespace RMuseum services.AddOpenAIService(); - services.AddSingleton(sp => - { - var config = sp.GetRequiredService(); - string dir = config["SemanticSearch:EmbeddingsDirectory"]; - return EmbeddingIndex.Load(dir); - }); - - services.AddSingleton(sp => - { - var config = sp.GetRequiredService(); - int dimension = int.Parse(config["SemanticSearch:Dimension"] ?? "1024"); - return new QueryEmbedder( - config["SemanticSearch:ModelPath"], - config["SemanticSearch:VocabPath"], - config["SemanticSearch:MergesPath"], - dimension); - }); + // See LazySemanticSearchResources.cs: this is deliberately NOT + // services.AddSingleton(sp => EmbeddingIndex.Load(...)) anymore. That + // eager, throwing factory is what caused a production 503 on the whole /api/ganjoor + // surface when the configured paths were wrong — GanjoorController's constructor + // (via ISemanticSearchService) couldn't be built, so nothing under that route could + // run. LazySemanticSearchResources defers the actual load to first real use and + // never throws; a failure there disables semantic search only. + services.AddSingleton(); services.AddSingleton(); diff --git a/RMuseum/Utils/SemanticSearch/LazySemanticSearchResources.cs b/RMuseum/Utils/SemanticSearch/LazySemanticSearchResources.cs new file mode 100644 index 00000000..f0dc7dbf --- /dev/null +++ b/RMuseum/Utils/SemanticSearch/LazySemanticSearchResources.cs @@ -0,0 +1,111 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; +using System; + +namespace RMuseum.Utils.SemanticSearch +{ + /// + /// Wraps EmbeddingIndex + QueryEmbedder loading so a failure (missing files, wrong paths, + /// corrupt data) can NEVER take down anything else in the app. + /// + /// The original design registered EmbeddingIndex/QueryEmbedder as singletons whose DI + /// factories called EmbeddingIndex.Load(...)/`new QueryEmbedder(...)` directly — both of + /// which throw on failure. Because GanjoorController's constructor (indirectly, through + /// ISemanticSearchService) depended on them, a load failure meant the controller itself + /// couldn't be constructed — taking down EVERY endpoint under /api/ganjoor, not just semantic + /// search, with a 503. That's exactly what happened in production. This class exists so that + /// can't happen again: the actual load is deferred to first real use (not app/controller + /// construction), attempted at most once, and a failure is caught, logged, and remembered — + /// SearchAsync() then reports "search unavailable" as an ordinary result, not an exception + /// that propagates into breaking anything else. + /// + /// Also worth knowing if this server runs multiple IIS worker processes for the same app + /// pool: each process gets its own instance of this (and everything it loads) — "singleton" + /// only means one instance per process, not per server. See the migration notes for the + /// memory math this implies at scale. + /// + public class LazySemanticSearchResources + { + private readonly object _lock = new object(); + private EmbeddingIndex _embeddingIndex; + private QueryEmbedder _queryEmbedder; + private string _loadError; + private bool _attempted; + + private readonly string _embeddingsDirectory; + private readonly string _modelPath; + private readonly string _vocabPath; + private readonly string _mergesPath; + private readonly int _dimension; + private readonly ILogger _logger; + + public LazySemanticSearchResources(IConfiguration configuration, ILogger logger) + { + _embeddingsDirectory = configuration["SemanticSearch:EmbeddingsDirectory"]; + _modelPath = configuration["SemanticSearch:ModelPath"]; + _vocabPath = configuration["SemanticSearch:VocabPath"]; + _mergesPath = configuration["SemanticSearch:MergesPath"]; + _dimension = int.TryParse(configuration["SemanticSearch:Dimension"], out var d) ? d : 1024; + _logger = logger; + } + + /// + /// Attempts to load the resources on first call (subsequent calls reuse the same result, + /// success or failure — this never retries automatically; a fresh app start is required + /// to try again after a config/file fix, which is the expected deploy-and-restart flow + /// anyway). Returns true and populates both out parameters if available; returns false + /// and populates otherwise. NEVER THROWS — that guarantee is the + /// entire point of this class. + /// + public bool TryGetResources(out EmbeddingIndex embeddingIndex, out QueryEmbedder queryEmbedder, out string error) + { + if (!_attempted) + { + lock (_lock) + { + if (!_attempted) + { + try + { + _embeddingIndex = EmbeddingIndex.Load(_embeddingsDirectory); + _queryEmbedder = new QueryEmbedder(_modelPath, _vocabPath, _mergesPath, _dimension); + _logger.LogInformation( + "Semantic search resources loaded: {Count} poems, dimension {Dimension}.", + _embeddingIndex.Metadata.Count, _embeddingIndex.Metadata.Dimension); + } + catch (Exception exp) + { + _loadError = exp.Message; + _embeddingIndex = null; + _queryEmbedder = null; + _logger.LogError(exp, + "Semantic search resources failed to load from '{EmbeddingsDirectory}' / '{ModelPath}' " + + "— semantic search will report unavailable, but this must not affect anything else.", + _embeddingsDirectory, _modelPath); + } + finally + { + _attempted = true; + } + } + } + } + + embeddingIndex = _embeddingIndex; + queryEmbedder = _queryEmbedder; + error = _loadError; + return _embeddingIndex != null && _queryEmbedder != null; + } + } + + /// + /// Thrown by SemanticSearchService when the underlying resources aren't available — the + /// controller catches this specifically and returns HTTP 503 with the message, distinct from + /// a plain 400/500, so a client (or a person checking logs) can tell "this feature isn't + /// configured/loaded right now" apart from "the query itself was bad" or "something crashed". + /// + public class SemanticSearchUnavailableException : Exception + { + public SemanticSearchUnavailableException(string message) : base(message) { } + } +}