diff --git a/docs/plans/2026-11-09-central-cache-staleness-audit.md b/docs/plans/2026-11-09-central-cache-staleness-audit.md new file mode 100644 index 00000000..66ca434d --- /dev/null +++ b/docs/plans/2026-11-09-central-cache-staleness-audit.md @@ -0,0 +1,75 @@ +# Central per-node cache staleness audit (Gitea #85) + +**Date:** 2026-11-09. **Scope:** every in-process cache on a **central** node that holds +configuration-database (or secret-store) state, and how stale it can be on the **peer** central +node after a change is made through the other one. Same defect class as #83 +(`ExternalDefinitionLookupCache`). + +## Context that sets the risk + +- Traefik routes UI, `/management` and `/api/*` to the **active** central node only + (`/health/active`). An edit and a use on different nodes therefore only happen when someone + talks to a node directly (CLI pointed at a node's own port) or across a **failover**. +- Every TTL below is measured from when the entry was loaded, so an entry a node cached while it + was previously active is still bounded by its TTL after it becomes active again. +- No cache below replicates its invalidation to the peer. Central already has one cross-node + channel: the `DistributedPubSub` topic `CentralCommunicationActor.HealthReportTopic` (peer + health replication). A config-invalidation broadcast, if an owner wants one, should be a second + topic on the same mediator, published from `AfterConfigurationCommit`. + +## Inventory + +| Cache | Holds | Local invalidation | Peer staleness bound | Verdict | +|---|---|---|---|---| +| `InboundScriptExecutor` compiled handlers (InboundAPI) | Compiled method delegates | `InvalidateMethod` on delete; change bus on import | **None** — every request revision-checks the cached handler against the resolved `ApiMethod.Script` and recompiles on mismatch | OK | +| `ApiMethodCache` (InboundAPI) | `ApiMethod` rows, incl. not-found | Change bus (bundle import); **management create/update/delete — added by #85** | `InboundApi:ApiMethodCacheTtl`, default **5 s** | OK (local gap fixed, see below) | +| `ExternalDefinitionLookupCache` (ESG, central only) | External systems / methods / DB connections (found only) | 9 ManagementActor handlers (#65); bundle import (#83) | `ExternalSystemGateway:DefinitionCacheTtl`, **10 s** on central | Owner decision — #83 | +| `RoleMappingCache` (Security) — the cache at `ManagementActor` `InvalidateRoleMappingCacheAfterCommit` | LDAP group → role map + Deployer site scopes | Mapping / scope-rule commands, post-commit | `Security:RoleMappingCacheSeconds`, default **30 s** (`0` disables) | Owner decision — see D1 | +| `PendingUpdatesCache` (DeploymentManager, #91) | Fleet pending-updates snapshot | Deployment finished on this node | `DeploymentManager:PendingUpdatesCacheTtl`, default **60 s** | OK — display only | +| `CentralCommunicationActor` site-address cache (+ `GrpcSiteTransport` channel pairs) | Site gRPC endpoints | Site create/update/delete → `RefreshSiteAddresses` | Periodic DB reload, **60 s** (hard-coded) | OK — rare edit, self-heals | +| `SiteStreamGrpcClientFactory` clients | Per (site, endpoint) stream clients | Site delete → `RemoveSiteAsync` (local only) | Not stale data: a removed site's client on the peer is reclaimed by idle eviction, `Communication:GrpcClientIdleTtl` (**30 min**) × 1.5 after last use | OK — resource, not correctness | +| `SitePskProvider` (Host) | Per-site gRPC PSK from config / secret store | Site removal (via either gRPC client path, on every node within the 60 s reload) | **Unbounded** for an in-place key rotation — on **both** nodes, not only the peer | Owner decision — see D2 | +| `DefaultSecretResolver` (shared `ZB.MOM.WW.Secrets`) | Decrypted secret values | Lib's own invalidator (UI writes, sync service) | `Secrets:ResolveCacheTtl`, **30 s** | OK — owned by the shared lib | +| `KpiSnapshotCache` (CentralUI) | KPI query results (operational data) | — | **8 s** | OK — not config | +| `SharedAlarmSummaryService` (CentralUI) | Per-site alarm summary fan-out | — | **12 s** cold-cache window | OK — not config | +| `IDeploymentStatusNotifier` (DeploymentManager) | Push of deployment status to UI circuits | In-process event | Peer UI never sees the push, but the UI is only served by the active node and the pages also poll | OK | +| `ITemplateGraphWatermark` / `FlattenSession` | Template-chain memo keyed by in-process version | Repository bumps | Session lives for one deploy/validate operation; the watermark is process-local by design (peer starts from 0 → extra work, never stale work) | OK | +| `ScriptCompileVerdictCache`, `TrustVerdictCache`, `AuditRegexCache`, `ScriptAnalysisService` `IMemoryCache`, `OAuth2TokenService` | Pure functions of their key (code hash + surface, pattern, credential hash) | — | Not applicable — a changed input is a different key | OK | +| `ZB.MOM.WW.Auth.ApiKeys.SqlServer` (inbound API keys) | — | — | No in-process cache in the shared store (checked in the 0.3.0 source) — revocation is immediate on both nodes | OK | + +## Fixed in #85 + +**`ApiMethodCache` was not invalidated by the management API even locally.** Only the compiled +handler was dropped, so for up to the 5 s TTL on the node that made the change: + +- a just-**deleted** method stayed callable — the executor recompiled it from the cached row; +- a just-**created** method answered 404 if a not-found had been cached. + +`ManagementActor` now drops the method's cache entry post-commit on create, update and delete +(`InvalidateApiMethodCacheAfterCommit`), the same post-commit pattern as the role-mapping cache. +The peer is still bounded by the 5 s TTL. Tests: `ApiMethodCacheInvalidationTests` (3 of 4 fail +without the fix; the fourth is the negative control). + +No peer-staleness case was **clearly** a bug: every config cache with peer staleness is +TTL-bounded (5–60 s), its bound is documented at the cache, and the active-only routing means the +window is only hit via a direct-to-node call or a failover inside the TTL. So no cross-node +broadcast was added. + +## Needs an owner decision + +- **D1 — `RoleMappingCache` revocation lag on the peer (30 s).** Removing a mapping or narrowing a + Deployer's site scope takes effect at once on the node that made the change and within 30 s on + the other. It is a security-relevant window (revoked access keeps working briefly), but only on + the node the change did *not* go through. Options: accept and document (current state); lower + `RoleMappingCacheSeconds`; or add the `DistributedPubSub` invalidation topic described above. + Recommendation: accept unless the operators' revocation SLA is tighter than 30 s. +- **D2 — `SitePskProvider` caches a successful PSK resolve forever.** Its own remarks say + `Invalidate` covers "a key rotated in place", but nothing calls it on rotation — only on site + removal. Rotating `SB-GRPC-PSK-{site}` in the secret store (with the site pair restarted on the + new key) leaves **both** central nodes dialling with the old key until they restart. Options: make + central restart part of the rotation runbook; give the cache a TTL; or invalidate on a + `PermissionDenied` from the site. Not peer-specific, so it is out of #85's fix scope — file it + separately. +- **D3 — `ExternalDefinitionLookupCache` peer bound (10 s).** Already tracked by #83; the + recommendation there (accept and document) still stands. If D1 gets a broadcast topic, this cache + and `ApiMethodCache` should join it rather than add a second mechanism. diff --git a/docs/requirements/Component-InboundAPI.md b/docs/requirements/Component-InboundAPI.md index 95a8d531..80fae17e 100644 --- a/docs/requirements/Component-InboundAPI.md +++ b/docs/requirements/Component-InboundAPI.md @@ -175,7 +175,7 @@ The `sbk__` design already supports zero-downtime rotation — a ## Script Compilation & Hot-Reload -API method scripts are pre-compiled in the **background after startup** by `InboundMethodWarmupService` (a hosted service registered after `AkkaHostedService`, so it never delays the cluster join or `/health/*`): it waits for `ApplicationStarted`, then — only once this node is the active central node (`IActiveNodeGate`; a standby pays nothing until a failover promotes it) — loads all method definitions and compiles them into in-memory delegates. Lazy compile + the per-request revision check guarantee correctness regardless. Compiles are **single-flight per method name + script revision**: N concurrent first callers share one compile (and one non-collectible loaded assembly). `InboundScriptExecutor.CompileAndRegister(ApiMethod, out IReadOnlyList errors)` compiles once and returns the diagnostics, so a save needs no separate validate pass; the management Create/Update path uses its sibling `CompileForPublish(method, out errors, out publish)`, which compiles once for the `compileWarning` and hands back a `publish` action queued as the post-commit registration — one compile (one pinned assembly) per save even inside a configuration transaction (review 2026-10-21 05/M1). `ApiMethodCache` prunes expired entries on a miss once large (scopes are exact method names — pinned by test — so growth is bounded). +API method scripts are pre-compiled in the **background after startup** by `InboundMethodWarmupService` (a hosted service registered after `AkkaHostedService`, so it never delays the cluster join or `/health/*`): it waits for `ApplicationStarted`, then — only once this node is the active central node (`IActiveNodeGate`; a standby pays nothing until a failover promotes it) — loads all method definitions and compiles them into in-memory delegates. Lazy compile + the per-request revision check guarantee correctness regardless. Compiles are **single-flight per method name + script revision**: N concurrent first callers share one compile (and one non-collectible loaded assembly). `InboundScriptExecutor.CompileAndRegister(ApiMethod, out IReadOnlyList errors)` compiles once and returns the diagnostics, so a save needs no separate validate pass; the management Create/Update path uses its sibling `CompileForPublish(method, out errors, out publish)`, which compiles once for the `compileWarning` and hands back a `publish` action queued as the post-commit registration — one compile (one pinned assembly) per save even inside a configuration transaction (review 2026-10-21 05/M1). `ApiMethodCache` prunes expired entries on a miss once large (scopes are exact method names — pinned by test — so growth is bounded). A Management API/UI create, update or delete drops that method's `ApiMethodCache` entry post-commit on the node that made the change; the peer central node converges within `ApiMethodCacheTtl` (default 5 s) — Gitea #85, audit `docs/plans/2026-11-09-central-cache-staleness-audit.md`. ### Update Workflow diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ApiMethodCache.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ApiMethodCache.cs index dab26a3e..aad1c74e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ApiMethodCache.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ApiMethodCache.cs @@ -14,13 +14,15 @@ namespace ZB.MOM.WW.ScadaBridge.InboundAPI; /// /// Fast path. calls /// for every name in an ApiMethod -/// ScriptArtifactsChanged notification (bundle-import changes today). +/// ScriptArtifactsChanged notification (bundle-import changes), and the +/// ManagementActor API-method create/update/delete handlers drop the changed name +/// post-commit (Gitea #85). Both are node-local. /// Fallback. Every entry — including a negative (not-found) result — /// expires after regardless of whether an -/// invalidation notification ever arrives. This is what keeps an operator edit via the -/// Management API/UI (which does not publish to the change bus) correct within a bounded -/// window, and is also the correctness backstop for the bus's at-least-once/in-process-only -/// contract — a missed or duplicate notification is harmless. +/// invalidation notification ever arrives. This is what bounds staleness on the PEER central +/// node (no invalidation crosses nodes), and is also the correctness backstop for the bus's +/// at-least-once/in-process-only contract — a missed or duplicate notification is +/// harmless. /// /// /// A negative (method not found) result is cached too — bounded by the same short TTL — so a diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiOptions.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiOptions.cs index 18ead25a..332dd8c6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiOptions.cs @@ -41,9 +41,9 @@ public class InboundApiOptions /// before the next request re-fetches it from the repository. The /// ScriptArtifactChangeSubscriber/IScriptArtifactChangeBus pipeline /// invalidates a specific method's entry immediately on a bundle-import change (the fast - /// path); this TTL is the self-healing fallback for changes the bus does not cover (e.g. - /// an operator edit via the Management API/UI, which does not publish a bus notification), - /// bounding staleness without requiring every mutation path to remember to invalidate. + /// path), and a Management API/UI create/update/delete drops it post-commit on the node that + /// made the change (Gitea #85). Both are node-local, so this TTL is the bound on the PEER + /// central node (and the self-healing fallback for any path that forgets to invalidate). /// Short by design — the whole point is to absorb repeated-request-storm SQL, not to /// serve minutes-old method definitions. /// diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index d8c20085..4d265dc2 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -3811,6 +3811,7 @@ public class ManagementActor : ReceiveActor }; await repo.AddApiMethodAsync(method); await repo.SaveChangesAsync(); + await InvalidateApiMethodCacheAfterCommit(sp, method.Name); // The script is persisted regardless; surface a non-fatal warning when it does // not compile so the caller is not misled into thinking a broken method is live // (a never-compiled method returns "Script compilation failed" on first call). @@ -3832,6 +3833,7 @@ public class ManagementActor : ReceiveActor method.TraceAllCalls = traceAllCalls; await repo.UpdateApiMethodAsync(method); await repo.SaveChangesAsync(); + await InvalidateApiMethodCacheAfterCommit(sp, method.Name); // The persisted DB row is authoritative: on a compile failure the method returns // a compilation error on every request (ExecuteAsync revision-checks the cached // handler against the fresh row), so the saved-but-broken script does NOT keep a @@ -3849,11 +3851,32 @@ public class ManagementActor : ReceiveActor await repo.DeleteApiMethodAsync(cmd.ApiMethodId); await repo.SaveChangesAsync(); if (method != null) + { await AfterConfigurationCommit(sp, () => { sp.GetService()?.InvalidateMethod(method.Name); return Task.CompletedTask; }); + await InvalidateApiMethodCacheAfterCommit(sp, method.Name); + } await AuditAsync(sp, user, "Delete", "ApiMethod", cmd.ApiMethodId.ToString(), method?.Name ?? cmd.ApiMethodId.ToString(), null); return true; } + /// + /// Drops this node's entry for + /// once an API-method create/update/delete has committed (Gitea #85). Without it the + /// node that made the change kept serving the pre-change row for up to + /// ApiMethodCacheTtl: a just-deleted method stayed callable (the executor recompiled + /// it from the cached row), and a cached not-found answered 404 for a just-created one. + /// Queued post-commit, like the role-mapping invalidation, so a concurrent request cannot + /// re-cache the pre-change row. The peer central node still converges within the TTL + /// (default 5 s) — see docs/plans/2026-11-09-central-cache-staleness-audit.md. + /// + private static Task InvalidateApiMethodCacheAfterCommit(IServiceProvider sp, string name) + { + var cache = sp.GetService(); + return cache is null + ? Task.CompletedTask + : AfterConfigurationCommit(sp, () => { cache.Invalidate(name); return Task.CompletedTask; }); + } + /// /// Recompiles and re-registers an inbound API method's script after a Create/Update, /// returning a non-null human-readable warning when the script did NOT compile. The diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiMethodCacheInvalidationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiMethodCacheInvalidationTests.cs new file mode 100644 index 00000000..f1f3845b --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ApiMethodCacheInvalidationTests.cs @@ -0,0 +1,114 @@ +using Akka.Actor; +using Akka.TestKit.Xunit2; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; +using ZB.MOM.WW.ScadaBridge.InboundAPI; + +namespace ZB.MOM.WW.ScadaBridge.ManagementService.Tests; + +/// +/// Gitea #85: an API-method create / update / delete through the management API must drop this +/// node's entry once the change commits. Before, only the compiled +/// handler was invalidated, so the node kept resolving the pre-change row for up to the cache +/// TTL — a deleted method stayed callable and a cached not-found hid a newly created one. +/// +public class ApiMethodCacheInvalidationTests : TestKit +{ + private readonly ServiceCollection _services = new ServiceCollection().AddTestConfigurationTransaction(); + private readonly IInboundApiRepository _apiRepo = Substitute.For(); + + // A TTL far longer than the test, so only an explicit invalidation can clear an entry. + private readonly ApiMethodCache _cache = new(TimeSpan.FromHours(1)); + + /// Initializes the harness with a long-TTL method cache. + public ApiMethodCacheInvalidationTests() + { + _services.AddScoped(_ => _apiRepo); + _services.AddScoped(_ => Substitute.For()); + _services.AddSingleton(_cache); + _apiRepo.SaveChangesAsync(Arg.Any()).Returns(1); + } + + private IActorRef CreateActor() => + Sys.ActorOf(Props.Create(() => new ManagementActor( + _services.BuildServiceProvider(), NullLogger.Instance))); + + private static ManagementEnvelope Envelope(object command) => + new(new AuthenticatedUser("designer", "Designer", new[] { "Designer" }, Array.Empty()), + command, Guid.NewGuid().ToString("N")); + + private Task Seed(string name, ApiMethod? row) => + _cache.GetOrFetchAsync(name, _ => Task.FromResult(row), CancellationToken.None); + + /// Returns true when the cache re-fetches (the entry was dropped). + private async Task Refetches(string name) + { + var fetched = false; + await _cache.GetOrFetchAsync(name, _ => { fetched = true; return Task.FromResult(null); }, CancellationToken.None); + return fetched; + } + + /// A cached not-found must not hide a method created through the management API. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Create_DropsCachedNotFound() + { + await Seed("Fresh", null); + Assert.False(await Refetches("Fresh")); // positive control: the negative entry is served + + CreateActor().Tell(Envelope(new CreateApiMethodCommand("Fresh", "return 1;", 30, null, null))); + ExpectMsg(TimeSpan.FromSeconds(30)); + + Assert.True(await Refetches("Fresh")); + } + + /// An update drops the cached pre-change row. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Update_DropsCachedRow() + { + var existing = new ApiMethod("Edited", "return 1;") { Id = 7, TimeoutSeconds = 10 }; + _apiRepo.GetApiMethodByIdAsync(7, Arg.Any()).Returns(existing); + await Seed("Edited", existing); + + CreateActor().Tell(Envelope(new UpdateApiMethodCommand(7, "return 2;", 10, null, null))); + ExpectMsg(TimeSpan.FromSeconds(30)); + + Assert.True(await Refetches("Edited")); + } + + /// A delete drops the cached row so the deleted method is no longer resolved. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Delete_DropsCachedRow() + { + var existing = new ApiMethod("Gone", "return 1;") { Id = 9, TimeoutSeconds = 10 }; + _apiRepo.GetApiMethodByIdAsync(9, Arg.Any()).Returns(existing); + await Seed("Gone", existing); + + CreateActor().Tell(Envelope(new DeleteApiMethodCommand(9))); + ExpectMsg(TimeSpan.FromSeconds(30)); + + Assert.True(await Refetches("Gone")); + } + + /// A change to one method leaves other methods' entries cached. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Delete_LeavesOtherMethodsCached() + { + var existing = new ApiMethod("Gone2", "return 1;") { Id = 10, TimeoutSeconds = 10 }; + _apiRepo.GetApiMethodByIdAsync(10, Arg.Any()).Returns(existing); + await Seed("Other", new ApiMethod("Other", "return 3;")); + + CreateActor().Tell(Envelope(new DeleteApiMethodCommand(10))); + ExpectMsg(TimeSpan.FromSeconds(30)); + + Assert.False(await Refetches("Other")); + } +}