diff --git a/docs/architecture/technical-debt.md b/docs/architecture/technical-debt.md index fb85f444..f57062a2 100644 --- a/docs/architecture/technical-debt.md +++ b/docs/architecture/technical-debt.md @@ -43,25 +43,21 @@ The entry had already been symptomless for months: the 2026-08-09 resync withdre ## [TD-003] Prepare Configuration System for Redis Migration -- **Status**: Proposed -- **Severity**: Low -- **Component**: `Ums.Infrastructure/Configuration/`, `Ums.Application/Configuration/` -- **Description**: The parameterization system already runs on an in-memory cache abstraction. The remaining debt is to migrate the cache implementation to Redis without changing the business-facing `IConfigurationProvider` or the typed `ConfigurationValues` consumers. -- **Rationale**: Initial implementation uses in-memory storage for simplicity and fast iteration. Redis will be needed when UMS scales to multiple API instances requiring shared configuration state and shared cache invalidation. -- **Impact**: - - Current in-memory implementation may show stale data if multiple API instances have different cache states. - - No distributed cache invalidation across pods. - - Parameter changes still depend on the current reload path rather than a distributed cache event. -- **Mitigation**: - - Introduce `IConfigurationCache` abstraction from day one. - - Implement `InMemoryConfigurationCache` as the initial concrete implementation. - - Design `ConfigurationProvider` to depend on `IConfigurationCache`, not on concrete implementation. - - Use `ConfigurationValues` for strongly-typed consumers so the migration does not leak cache concerns into handlers or validators. - - Document the abstraction interface and future migration steps. -- **Target Resolution**: Phase 2 (when scaling to multiple API instances or when Redis infrastructure is available). -- **Related Documents**: - - [Parameterization System Specification](../governance/construction/ddd-design/parameterization-system-spec.md) - - [TODO-003](../governance/project/TODO.md#todo-003-implement-parameterization-system-with-loader-and-provider) +- **Status**: **Closed — already implemented** (verified 2026-08-10) +- **Severity**: was Low +- **Description**: The entry described the remaining work as "migrate the cache implementation to Redis without changing the business-facing `IConfigurationProvider`". That migration exists and is wired. + +### What was found on inspection + +- `Ums.Application/Configuration/Services/IConfigurationCache.cs` — the abstraction the entry proposed introducing. +- `Ums.Infrastructure/Configuration/RedisConfigurationCache.cs` — 291 lines, Redis-backed with Pub/Sub invalidation across replicas, no `NotImplementedException`. +- `Ums.Infrastructure/Configuration/InMemoryConfigurationCache.cs` — the in-process fallback. +- `DependencyInjection.cs` selects between them on `Redis:Connection` (or `REDIS_CONNECTION`, the Kubernetes form — reading only the former is a bug that was already found and fixed, see `CadenaDeRedis.Normalizar`). +- `Ums.Application.Test/Configuration/AvisoDeConfiguracionEntreReplicasTests.cs` covers cross-replica invalidation. + +Every mitigation the entry listed as future work is in place, and the "impact" it describes — stale data across instances, no distributed invalidation — does not apply when Redis is configured. What remains is **provisioning Redis in an environment**, which is infrastructure, not debt in this repository. + +- **Why it stayed open**: nobody re-read it after the work landed. A debt register is only useful if entries are closed when the debt is paid; carrying a phantom is the failure mode TD-002 also showed. --- @@ -226,13 +222,19 @@ Fixed here rather than filed separately, because otherwise TD-008's verification ## [TD-011] The Integration Suite Shares One Host Across 31 Test Classes -- **Status**: Confirmed +- **Status**: **Resolved** (2026-08-10) - **Severity**: High - **Component**: `src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs` -- **Description**: 31 test classes take `IClassFixture` over a shared seeded store. Any test that **writes** mutates state every other class reads. There is no per-test isolation and no cleanup. -- **How it surfaced (2026-08-10)**: a single new test that wrote branding to the first seeded tenant and read it back took **133 of 316 tests down in CI**. It passed locally, on both Debug and Release — execution order happened to hide it. One added write, 133 failures, and a green local run: that is the failure mode this design produces. -- **Not the first time**: the factory's own comments record an earlier incident of roughly 80 failures caused by a static constructor leaking `Persistence__Provider` between hosts. The mechanism differs; the shape does not. -- **Impact**: the suite cannot cover any write path end to end without risking the rest of it, so write coverage is pushed down to handler tests. That is a reasonable place for it, but it should be a choice, not a constraint. It also means a genuine regression can hide behind an unrelated test's pollution, and that failures do not reproduce locally — the most expensive kind. -- **Suggested resolution**: give each test class its own store (a per-class database name is the cheapest step), or add a reset between classes. The e2e suite already solved the equivalent problem for the shared cluster with `tests/helpers/limpieza.ts` and a per-run marker; the same reasoning applies here. -- **Interim measure**: `TenantBrandingRestQueryTests` covers only the read path. Read-after-write for branding is deliberately absent, with the reason written in the file so nobody re-adds it and repeats the 133. +- **Description**: 31 test classes take `IClassFixture`. The factory registered its three EF Core InMemory stores under **fixed literal names** — `"TestDb"`, `"TestProjectionDb"`, `"TestReadModelDb"` — and the InMemory provider keys a store by name **per process**. Every class therefore shared the same three stores no matter how many factory instances existed. Any write in one class was visible to all the others. +- **How it surfaced**: a single new test that wrote branding to the first seeded tenant and read it back took **133 of 316 tests down in CI**. It passed locally on both Debug and Release — execution order hid it. One added write, 133 failures, and a green local run. +- **Not the first time**: the factory's own comments record an earlier incident of roughly 80 failures from a static constructor leaking `Persistence__Provider` between hosts (G-014). Different mechanism, same shape. + +### Resolution + +Each factory instance now suffixes its three store names with a per-instance GUID, so every test class gets its own store, seeded from the same baseline and unable to reach any other. Cost: ~5% wall-clock (4m30s → 4m46s), from seeding per class instead of once. + +The write test that caused the 133 is **kept, deliberately, as a sentinel**: it exercises read-after-write against the shared-by-default seeded tenant, so if anyone reinstates a shared store it fails again immediately rather than silently corrupting a hundred unrelated assertions. + +- **Verified**: 314/316 (2 skipped) in Release with the sentinel in place. +- **Not covered by this fix**: `ContractTestWebApplicationFactory` and `PostgreSqlWebApplicationFactory` use the same literal-name pattern. Neither showed a symptom — the contract suite is a single collection and the PostgreSQL one is skipped without Docker — but the hazard is identical and worth the same treatment when either grows. diff --git a/src/apps/ums.api/Ums.Presentation.IntegrationTest/Identity/TenantBrandingRestQueryTests.cs b/src/apps/ums.api/Ums.Presentation.IntegrationTest/Identity/TenantBrandingRestQueryTests.cs index 3bac5009..00c5a853 100644 --- a/src/apps/ums.api/Ums.Presentation.IntegrationTest/Identity/TenantBrandingRestQueryTests.cs +++ b/src/apps/ums.api/Ums.Presentation.IntegrationTest/Identity/TenantBrandingRestQueryTests.cs @@ -12,12 +12,11 @@ namespace Ums.Presentation.IntegrationTest.Identity; /// Se fijan los dos desenlaces que el handler distingue en LECTURA, porque son los que el /// panel de identidad visual necesita separar y los que un 404 indiscriminado borraría. /// -/// NO se prueba aquí «leer lo que el POST acaba de escribir». Se intentó y hundió 133 -/// pruebas en CI: las 31 clases de esta suite comparten `UmsApiWebApplicationFactory`, así que -/// escribir sobre el inquilino sembrado contamina a todas las demás. En local pasaba —el orden de -/// ejecución lo escondía—, que es la peor forma de que un fallo así se manifieste. El camino de -/// escritura ya está cubierto en `Ums.Application.Test/Tenants/Branding`, a nivel de handler y sin -/// estado compartido, que es donde corresponde mientras esta suite no aísle. +/// La tercera prueba —leer lo que el POST acaba de escribir— tumbó 133 pruebas en CI la +/// primera vez, porque las 31 clases de esta suite compartían el mismo almacén InMemory y escribir +/// aquí contaminaba a todas. Con el aislamiento por instancia de factoría (TD-011) vuelve a ser +/// segura, y se conserva precisamente como centinela: si alguien reintroduce el almacén +/// compartido, esta prueba lo delata. /// public sealed class TenantBrandingRestQueryTests : IClassFixture { @@ -67,4 +66,40 @@ public async Task GetBranding_InquilinoInexistente_Responde404() response.StatusCode.Should().Be(HttpStatusCode.NotFound); } + + [Fact] + public async Task GetBranding_DevuelveLoQueElPostAcabaDeEscribir() + { + var tenantId = await PrimerInquilinoSembradoAsync(); + + var cuerpo = new + { + logo = "https://cdn.example.com/logo.png", + // Nombres EXACTOS del enum de dominio (LogoFormat, BackgroundStyle): el handler los + // resuelve con DomainEnumerationParser.FromName, que distingue mayúsculas. 'svg' o + // 'solid' devuelven 400. + logoFormat = "Svg", + primaryColor = "#123456", + backgroundStyle = "SolidColor", + headlineText = "Portal de pruebas", + secondaryText = "Subtítulo de pruebas", + primaryButtonLabel = "Entrar", + footerText = "Pie de pruebas", + customDomain = (string?)null, + magicLinkFallbackEnabled = false, + }; + + var escritura = await _client.PostAsJsonAsync( + $"/api/v1/tenants/{tenantId}/branding", cuerpo, TestContext.Current.CancellationToken); + escritura.StatusCode.Should().Be(HttpStatusCode.Created); + + var lectura = await _client.GetAsync( + $"/api/v1/tenants/{tenantId}/branding", TestContext.Current.CancellationToken); + + lectura.StatusCode.Should().Be(HttpStatusCode.OK); + using var payload = JsonDocument.Parse( + await lectura.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + payload.RootElement.GetProperty("primaryColor").GetString().Should().Be("#123456"); + payload.RootElement.GetProperty("headlineText").GetString().Should().Be("Portal de pruebas"); + } } diff --git a/src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs b/src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs index bed3b63d..dc62c002 100644 --- a/src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs +++ b/src/apps/ums.api/Ums.Presentation.IntegrationTest/Infrastructure/UmsApiWebApplicationFactory.cs @@ -23,6 +23,22 @@ public sealed class UmsApiWebApplicationFactory : WebApplicationFactory // InMemory`) que se filtraban a los hosts de PostgreSqlWebApplicationFactory y // dejaban su UmsPlatformDbContext sin registrar — la causa raíz de ~80 fallos // de integración (G-014). + + /// + /// Sufijo único por INSTANCIA de factoría (TD-011). + /// + /// El proveedor InMemory de EF Core indexa cada almacén por su NOMBRE dentro del proceso. + /// Con los literales fijos que había antes —«TestDb», «TestProjectionDb», «TestReadModelDb»— + /// las 31 clases de esta suite, cada una con su propia factoría, compartían exactamente los + /// mismos tres almacenes. Escribir en una clase se veía desde todas las demás. + /// + /// Lo que costó averiguarlo: una sola prueba que escribía identidad visual sobre el + /// inquilino sembrado tumbó 133 de 316 en CI, y pasaba en local en Debug y en Release porque + /// el orden de ejecución la escondía. Con el nombre por instancia, cada clase arranca de la + /// misma semilla y no puede contaminar a ninguna otra. + /// + private readonly string _almacenAislado = Guid.NewGuid().ToString("N"); + protected override void ConfigureWebHost(IWebHostBuilder builder) { builder.UseEnvironment("Development"); @@ -75,8 +91,8 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) // —opciones genéricas, la configuración de opciones específica del proveedor (EF Core 9+) y // el propio contexto— y se re-registran AMBOS sobre InMemory, de modo que NO quede ningún // servicio del proveedor Npgsql en el contenedor cuando el host es InMemory. - ReplaceDbContextWithInMemory(services, "TestDb"); - ReplaceDbContextWithInMemory(services, "TestProjectionDb"); + ReplaceDbContextWithInMemory(services, $"TestDb-{_almacenAislado}"); + ReplaceDbContextWithInMemory(services, $"TestProjectionDb-{_almacenAislado}"); // G-014 (causa raíz del CUELGUE en CI): el `ReadModelDbContext` (proyecciones de fase 1) // seguía registrado con Npgsql apuntando a `127.0.0.1:5433` (appsettings.Development). En // el host InMemory, cualquier proyección (p. ej. `PermissionTemplatePublishedEvent`) hacía @@ -84,7 +100,7 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) // fallo se enmascaraba, pero en un entorno limpio (runner de CI, contenedor) da «Connection // refused» y, con `EnableRetryOnFailure(3)`, se cuelga en reintentos. Se re-registra también // sobre InMemory para que el host InMemory NO toque PostgreSQL en absoluto. - ReplaceDbContextWithInMemory(services, "TestReadModelDb"); + ReplaceDbContextWithInMemory(services, $"TestReadModelDb-{_almacenAislado}"); services.RemoveAll(); services.RemoveAll();