diff --git a/docs/components/Transport.md b/docs/components/Transport.md index 6104ad73..615a18b7 100644 --- a/docs/components/Transport.md +++ b/docs/components/Transport.md @@ -138,7 +138,7 @@ The two levels interact in a specific order, and it is load-bearing (register ro - **It fails OPEN.** A store error logs a Warning naming the client and the cap, and *permits* the attempt. This is a best-effort rate cap layered over a surface that is already authenticated and Administrator-gated, so the availability of legitimate imports outranks the cap — a database blip must not make the import wizard unusable. Falling back to the old process-local window instead was considered and rejected: it would still block a legitimate operator's 11th import during an outage, which is precisely the outcome fail-open exists to prevent. The Warning is the operator's signal that the fleet cap is not currently being enforced. - **Accuracy is deliberately approximate.** The count-then-insert pair is not atomic across the two nodes, so genuinely simultaneous attempts can overshoot the cap by up to the number in flight. A conditional single-statement insert would close that at the cost of provider-specific raw SQL; the overshoot is bounded and self-corrects within the window. -- **Rows are pruned opportunistically, not by a background service.** Transport owns no central scheduler, so the prune rides the unlock path: claimed by CAS at most once per 15 minutes per process, bounded to 1000 rows per pass, retaining `2 ×` the window so a prune racing a count can never delete a row the count still needs, and wrapped in a try/catch that logs and swallows — retention maintenance must never fail the operator's actual unlock. This is the same shape `DeploymentService` uses for terminal deployment records. +- **Rows are pruned by two drivers sharing one cadence claim (register row 65).** The prune is claimed by CAS at most once per 15 minutes per process, bounded to 1000 rows per pass, retaining `2 ×` the window so a prune racing a count can never delete a row the count still needs, and wrapped in a try/catch that logs and swallows — retention maintenance must never fail the operator's actual unlock. This is the same shape `DeploymentService` uses for terminal deployment records. It originally rode the **unlock path only**, on the reasoning that Transport owned no central scheduler; the consequence was that a cluster which stopped importing entirely left its last rows in place indefinitely, since only a node actively serving unlocks ever pruned. Transport has since gained exactly such a scheduler — `BundleSessionEvictionService`, the central-only 1-minute session sweep — so it now also ticks `IBundleUnlockAttemptWindow.PruneAsync`, and the table converges whether or not anyone is importing. Because both drivers take the **same** claim, ticking on the 1-minute sweep costs at most one bounded DELETE per quarter hour and the two can never prune concurrently. `PruneAsync` is contractually non-throwing (unlike `TryRegisterAttemptAsync`, which throws and lets the limiter own the fail-open decision) — a background sweep has no caller to report to and must survive a database blip. Behaviour the operator sees is otherwise unchanged: same config key, same semantics ("attempts, not failures"), same ordering relative to the per-bundle lockout, same `` bucket. diff --git a/docs/requirements/Component-Transport.md b/docs/requirements/Component-Transport.md index f8bb5f8b..37130617 100644 --- a/docs/requirements/Component-Transport.md +++ b/docs/requirements/Component-Transport.md @@ -338,7 +338,7 @@ Imports are all-or-nothing per bundle. A bundle either applies fully or not at a - **Passphrase never persisted.** It lives only inside the export/import service call path and is discarded after use. No environment variable, no log line. - **Failed-unlock rate limit:** per-bundle 3-strike lockout (keyed by `ContentHash`, server-side); per-IP-per-hour cap (`MaxUnlockAttemptsPerIpPerHour`, default 10, configurable) to deter brute force against a stolen bundle. Each failed attempt produces a `BundleImportUnlockFailed` audit row. The two are evaluated in order — bundle lockout, then per-IP throttle, then decrypt — so hammering an already-locked bundle spends no hourly budget, and a throttle denial (`BundleUnlockThrottledException`, fail-closed with a curated message on both the UI and CLI surfaces) burns **no** per-bundle strike, since it never reaches the decryptor. Both typed exceptions live in `Commons/Types/Transport/` — `ManagementService` references Commons but not Transport, so a type declared beside the importer cannot be curated by `MapFault` and reaches the CLI as a generic internal-error message (register rows 44 and 51). Successful unlocks count against the hourly budget too: it caps attempts, not failures. The client key is the caller's remote IP — from `CircuitClientAddressProvider` (snapshotted at Blazor circuit establishment, since `HttpContext` is unavailable mid-circuit) for the Central UI, and from `ManagementEnvelope.ClientIp` for the CLI; a caller with no attributable address shares a single stricter bucket rather than bypassing the throttle. Per-client isolation is only real behind a ForwardedHeaders-trusted proxy, the same caveat the login throttle carries in `Component-Security.md`. (Register row 44: the limiter existed, was configured and unit-tested, but was never invoked until this was wired.) -**The per-IP window is fleet-wide and restart-durable (register row 52).** It is persisted in the central MS SQL `BundleUnlockAttempt` table — the HA store both central nodes already share — rather than in each node's process memory, so the documented cap is a fleet cap and not N × the cap, and a node restart no longer clears it. `BundleUnlockRateLimiter` still owns the policy; `SqlBundleUnlockAttemptWindow` is the store behind the `IBundleUnlockAttemptWindow` seam, and `IBundleUnlockAttemptRepository` (Commons interface, ConfigurationDatabase implementation) is the persistence. Three deliberate trades: **the throttle fails OPEN** — a store error logs a Warning and permits the attempt, because this is a best-effort rate cap over an already-authenticated Administrator-gated surface and the availability of legitimate imports outranks the cap; **accuracy is approximate** — count-then-insert is not atomic across nodes, so simultaneous attempts can overshoot by the number in flight, which is bounded and self-corrects within the window; and **rows are pruned opportunistically on the unlock path** (CAS-claimed at most every 15 minutes per process, bounded to 1000 rows, retaining twice the window, best-effort) rather than by a new background service, since Transport owns no central scheduler. +**The per-IP window is fleet-wide and restart-durable (register row 52).** It is persisted in the central MS SQL `BundleUnlockAttempt` table — the HA store both central nodes already share — rather than in each node's process memory, so the documented cap is a fleet cap and not N × the cap, and a node restart no longer clears it. `BundleUnlockRateLimiter` still owns the policy; `SqlBundleUnlockAttemptWindow` is the store behind the `IBundleUnlockAttemptWindow` seam, and `IBundleUnlockAttemptRepository` (Commons interface, ConfigurationDatabase implementation) is the persistence. Three deliberate trades: **the throttle fails OPEN** — a store error logs a Warning and permits the attempt, because this is a best-effort rate cap over an already-authenticated Administrator-gated surface and the availability of legitimate imports outranks the cap; **accuracy is approximate** — count-then-insert is not atomic across nodes, so simultaneous attempts can overshoot by the number in flight, which is bounded and self-corrects within the window; and **rows are pruned best-effort** (CAS-claimed at most every 15 minutes per process, bounded to 1000 rows, retaining twice the window). That prune originally rode the unlock path alone, since Transport owned no central scheduler — which meant a cluster that stopped importing entirely left its last rows in place indefinitely. **Register row 65 closed that**: Transport's central-only `BundleSessionEvictionService` sweep now also ticks `IBundleUnlockAttemptWindow.PruneAsync`, sharing the one 15-minute cadence claim with the unlock path, so the table converges with no import traffic at all. `PruneAsync` never throws (the fail-open policy stays with the limiter on the attempt path). - **Bundle size cap** on upload (default 100 MB, configurable) to bound memory. - **In-transit:** existing HTTPS to the Central UI; no new channel. - **Audit trail is the chain of custody.** Every export, every import (including aborted ones at validation), and every unlock failure is audit-logged with source env, content hash, encrypted yes/no, and artifact summary. diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs index 2034c50e..b6915ef1 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs @@ -291,7 +291,22 @@ public class ArtifactDeploymentService site.Id, globals, cancellationToken, deploymentId); } - // Deploy to each site in parallel with per-site timeout + // Deploy to each site in parallel with per-site timeout. + // + // Deferred-work register row 60 — what makes this fan-out safe, stated precisely + // because the invariant is easy to break from a distance: + // * `globals` is NOT EF state. FetchGlobalArtifactsAsync projects the entities + // into pure Commons message DTOs and drops the entities, so the snapshot the + // lambda closes over cannot touch a DbContext at all. + // * `site` IS a tracked EF entity, and it is the one piece of EF state that + // genuinely crosses into the parallel phase. Only already-materialised scalar + // properties are read below (Id, SiteIdentifier, Name) and NO navigation + // property is traversed — with lazy loading off (pinned by + // ConfigurationDatabase.Tests.AddConfigurationDatabase_DoesNotEnableLazyLoading) + // those are plain field reads that never re-enter the shared context. + // So: do not traverse a navigation property on `site` in here, and do not move + // per-site command building (which DOES query) inside this lambda — either one + // puts concurrent load on one non-thread-safe DbContext. var tasks = sites.Select(async site => { try diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/SiteCallAuditActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/SiteCallAuditActor.cs index ad2a7078..a8970ebf 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/SiteCallAuditActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/SiteCallAuditActor.cs @@ -522,6 +522,20 @@ public class SiteCallAuditActor : ReceiveActor /// pass (the failing site's cursor is left at its previous value so the next /// tick retries the same window). /// + /// + /// SERIAL BY DESIGN — do NOT fan this loop out (register row 59). The + /// is a single EF Core DbContext-backed instance + /// resolved once per tick by , and a DbContext + /// is not thread-safe. The `await`-in-`foreach` below is the ONLY thing keeping this + /// tick's scope — which spans every site x up to + /// pages of upserts — to one concurrent + /// user. Replacing it with Task.WhenAll to overlap the per-site network pulls + /// would reintroduce register row 45 (concurrent use of one scoped DbContext), which + /// surfaces as sporadic + /// "A second operation was started on this context instance" failures rather than a + /// clean break. If the serial pass ever becomes too slow, the fix is a scope PER SITE + /// (resolve a repository inside the loop body), not a shared scope under fan-out. + /// private async Task ReconcileSitesAsync( IReadOnlyList sites, IPullSiteCallsClient client, ISiteCallAuditRepository repository) { diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionEvictionService.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionEvictionService.cs index cae75a32..030f18e7 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionEvictionService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionEvictionService.cs @@ -14,12 +14,22 @@ namespace ZB.MOM.WW.ScadaBridge.Transport.Import; /// design contract is "bundles are not retained server-side after ApplyAsync /// commits". This service keeps abandoned / failed sessions from pinning that /// plaintext for the full 30-minute TTL when no other traffic flows. +/// +/// Register row 65: it also ticks the bundle-unlock attempt prune. That retention +/// sweep used to ride the unlock path alone, on the reasoning that Transport owned no +/// central scheduler — this service is that scheduler, so a cluster that stops importing +/// entirely no longer leaves its last BundleUnlockAttempt rows in place +/// indefinitely. The window keeps its own 15-minute cadence claim, so ticking it on the +/// 1-minute session sweep costs at most one bounded DELETE per quarter hour, and the two +/// drivers can never prune concurrently. +/// /// internal sealed class BundleSessionEvictionService : BackgroundService { private static readonly TimeSpan SweepInterval = TimeSpan.FromMinutes(1); private readonly IBundleSessionStore _sessionStore; + private readonly IBundleUnlockAttemptWindow _unlockAttemptWindow; private readonly ILogger _logger; /// @@ -35,12 +45,15 @@ internal sealed class BundleSessionEvictionService : BackgroundService /// Initializes a new instance of . /// /// The bundle session store to sweep for expired sessions. + /// Shared unlock-attempt window whose retention prune this sweep also ticks (register row 65). /// Logger for sweep diagnostics. public BundleSessionEvictionService( IBundleSessionStore sessionStore, + IBundleUnlockAttemptWindow unlockAttemptWindow, ILogger logger) { _sessionStore = sessionStore ?? throw new ArgumentNullException(nameof(sessionStore)); + _unlockAttemptWindow = unlockAttemptWindow ?? throw new ArgumentNullException(nameof(unlockAttemptWindow)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _openSessionCountProvider = () => _sessionStore.OpenSessionCount; } @@ -95,6 +108,26 @@ internal sealed class BundleSessionEvictionService : BackgroundService { _logger.LogWarning(ex, "Bundle session sweep failed; will retry on next interval."); } + + // Register row 65: retention tick for the fleet-wide unlock-attempt window. + // PruneAsync is contractually non-throwing and self-limits to its own + // 15-minute cadence, so this is a cheap no-op on most sweeps. Kept separate + // from the session sweep's try so neither maintenance duty can mask the other. + try + { + await _unlockAttemptWindow + .PruneAsync(BundleUnlockRateLimiter.DefaultWindow, stoppingToken) + .ConfigureAwait(false); + } + catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) + { + return; + } + catch (Exception ex) + { + _logger.LogWarning( + ex, "Bundle-unlock attempt prune tick failed; will retry on next interval."); + } } } } diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/IBundleUnlockAttemptWindow.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/IBundleUnlockAttemptWindow.cs index 8e0340ba..39b0549b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/IBundleUnlockAttemptWindow.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/IBundleUnlockAttemptWindow.cs @@ -25,4 +25,23 @@ public interface IBundleUnlockAttemptWindow /// true when the attempt was recorded (within budget); false when the bucket is exhausted. Task TryRegisterAttemptAsync( string clientKey, int maxAttemptsPerWindow, TimeSpan window, CancellationToken ct = default); + + /// + /// Register row 65: drives one retention sweep of aged-out attempt rows independently + /// of the unlock path. The prune used to ride + /// only, so a cluster that stopped importing entirely left its last rows in place + /// indefinitely; BundleSessionEvictionService — Transport's periodic + /// central-only sweep — now also ticks this, which closes that. + /// + /// Unlike , implementations must NOT throw on + /// store failure: this is pure retention maintenance with no caller to report to, and + /// the sweep that drives it must never be taken down by a database blip. Implementations + /// are also expected to keep their own cadence claim, so calling this more often than + /// the intended prune interval is harmless. + /// + /// + /// Trailing window the attempts are counted over (one hour in production); rows are retained for a multiple of it. + /// Cancellation token. + /// A task that completes when the prune attempt has finished (or been skipped for cadence). + Task PruneAsync(TimeSpan window, CancellationToken ct = default); } diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/SqlBundleUnlockAttemptWindow.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/SqlBundleUnlockAttemptWindow.cs index fb59e22d..c9ea21cf 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/SqlBundleUnlockAttemptWindow.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/SqlBundleUnlockAttemptWindow.cs @@ -27,11 +27,12 @@ namespace ZB.MOM.WW.ScadaBridge.Transport.Import; public sealed class SqlBundleUnlockAttemptWindow : IBundleUnlockAttemptWindow { /// - /// Minimum wall-clock gap between opportunistic prunes. The prune runs on the unlock - /// path (Transport owns no central scheduler, and the design constraint was "no new - /// background service"), so it is claimed at most this often per process — the same - /// rate-limited opportunistic-sweep shape DeploymentService uses for terminal - /// deployment records. + /// Minimum wall-clock gap between prunes, claimed at most this often per process — the + /// same rate-limited sweep shape DeploymentService uses for terminal deployment + /// records. Both prune drivers share the one claim: the opportunistic sweep on the + /// unlock path, and (register row 65) BundleSessionEvictionService's periodic + /// tick, which is what keeps the table converging on a cluster that has stopped + /// importing altogether. /// internal static readonly TimeSpan PruneMinInterval = TimeSpan.FromMinutes(15); @@ -116,6 +117,39 @@ public sealed class SqlBundleUnlockAttemptWindow : IBundleUnlockAttemptWindow return true; } + /// + public async Task PruneAsync(TimeSpan window, CancellationToken ct = default) + { + if (window <= TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException(nameof(window), "Window must be positive."); + } + + // Register row 65: the same rate-limited, bounded, best-effort sweep the unlock + // path runs — deliberately sharing the ONE prune claim, so a background tick and a + // concurrent unlock cannot both prune, and the effective cadence stays + // PruneMinInterval no matter how often this is called. + try + { + using var scope = _scopeFactory.CreateScope(); + var repo = scope.ServiceProvider.GetRequiredService(); + await TryPruneAsync(repo, _timeProvider.GetUtcNow().UtcDateTime, window, ct) + .ConfigureAwait(false); + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } + catch (Exception ex) + { + // The interface contract is "must not throw": there is no caller to report to, + // and the periodic sweep driving this must survive a database blip. Resolving + // the scope/repository is outside TryPruneAsync's own try, so it is caught here. + _logger.LogWarning( + ex, "Scheduled bundle-unlock attempt prune failed; will retry on the next sweep."); + } + } + /// /// Opportunistic, rate-limited, bounded prune of aged-out attempt rows. Best-effort: /// any failure is logged and swallowed, because retention maintenance must never fail diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DeploymentsPushUpdateTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DeploymentsPushUpdateTests.cs index ea6dfac3..d04ecdfd 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DeploymentsPushUpdateTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DeploymentsPushUpdateTests.cs @@ -112,6 +112,118 @@ public class DeploymentsPushUpdateTests : BunitContext Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } + /// + /// Deferred-work register row 58: the notifier is a process SINGLETON that retains + /// delegates targeting circuit-scoped components, so an un-detached handler leaks the + /// whole component graph (and its circuit) for the process lifetime. + /// + /// The sibling pins the + /// observable behaviour, but it is satisfied by the _disposed guard ALONE — + /// delete the -= from Dispose and it still passes while the leak is + /// wide open. This test pins the retention property structurally instead: the + /// notifier's invocation list must actually shrink back to empty. + /// + /// + [Fact] + public void Deployments_Dispose_ActuallyDetachesTheDelegate_NotJustGuardsIt() + { + RegisterServices(); + var cut = Render(); + + Assert.Equal(1, SubscriberCount(_notifier)); + + cut.Instance.Dispose(); + + Assert.Equal(0, SubscriberCount(_notifier)); + } + + /// + /// Reads the compiler-generated backing field of the field-like + /// StatusChanged event and counts its invocation list. Reflection is the only + /// way to observe retention — the notifier deliberately exposes no subscriber count, + /// and a behavioural probe cannot distinguish "detached" from "attached but guarded". + /// + private static int SubscriberCount(DeploymentStatusNotifier notifier) + { + var field = typeof(DeploymentStatusNotifier).GetField( + "StatusChanged", BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(field); + + var handlers = (Delegate?)field!.GetValue(notifier); + return handlers?.GetInvocationList().Length ?? 0; + } + + /// + /// Deferred-work register row 58's TRIGGER, pinned: "a second page subscribes, or any + /// subscriber lands without a matching Dispose unsubscribe". Row 58 is + /// acceptable only while every subscriber detaches itself; a new page that subscribes + /// and forgets is a silent circuit leak that no existing test would catch, because + /// every other pin is written against the one known subscriber. + /// + /// A source scan rather than reflection: the subscription is a += operator + /// inside a method body, which is not visible on the type's reflected surface. + /// Mirrors the source-scan convention in Commons.Tests/ArchitecturalConstraintTests. + /// + /// + [Fact] + public void EveryCentralUiSubscriberToTheNotifier_UnsubscribesAndIsDisposable() + { + var sourceDir = GetCentralUiSourceDirectory(); + var sourceFiles = Directory + .GetFiles(sourceDir, "*.razor", SearchOption.AllDirectories) + .Concat(Directory.GetFiles(sourceDir, "*.cs", SearchOption.AllDirectories)) + .Where(f => !f.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}", + StringComparison.Ordinal) + && !f.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}", + StringComparison.Ordinal)); + + var subscribers = new List(); + foreach (var file in sourceFiles) + { + var content = File.ReadAllText(file); + if (!content.Contains("StatusChanged +=", StringComparison.Ordinal)) + { + continue; + } + + subscribers.Add(file); + + Assert.True( + content.Contains("StatusChanged -=", StringComparison.Ordinal), + $"{Path.GetFileName(file)} subscribes to IDeploymentStatusNotifier.StatusChanged but never " + + "unsubscribes. The notifier is a singleton, so the delegate pins this circuit-scoped " + + "component for the process lifetime (deferred-work register row 58)."); + + Assert.True( + content.Contains("IDisposable", StringComparison.Ordinal) + || content.Contains("IAsyncDisposable", StringComparison.Ordinal), + $"{Path.GetFileName(file)} subscribes to IDeploymentStatusNotifier.StatusChanged but does not " + + "implement IDisposable/IAsyncDisposable, so its unsubscribe can never run."); + } + + // Guard the guard: if the scan silently stops matching (file moved, API renamed) + // the loop above would vacuously pass and the trigger would go unwatched. + Assert.NotEmpty(subscribers); + } + + /// Walks up from the test output directory to the CentralUI project source root. + private static string GetCentralUiSourceDirectory() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir != null) + { + var candidate = Path.Combine(dir.FullName, "src", "ZB.MOM.WW.ScadaBridge.CentralUI"); + if (Directory.Exists(candidate)) + { + return candidate; + } + + dir = dir.Parent; + } + + throw new InvalidOperationException("Could not locate the ZB.MOM.WW.ScadaBridge.CentralUI source directory."); + } + /// /// Regression test for CentralUI-022. The notifier is a process singleton: /// it can read its subscriber list and begin invoking diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/ServiceCollectionExtensionsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/ServiceCollectionExtensionsTests.cs index 88bc9826..27d102ee 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/ServiceCollectionExtensionsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/ServiceCollectionExtensionsTests.cs @@ -48,6 +48,77 @@ public class ServiceCollectionExtensionsTests #pragma warning restore EF1001 } + /// + /// Deferred-work register row 60: ArtifactDeploymentService builds its per-site + /// deploy commands serially ("DbContext is not thread-safe") and then fans the SEND out + /// with Task.WhenAll, with EF-materialised Site entities captured by the + /// parallel lambda. That is safe for exactly one reason: with lazy loading off, reading + /// a property on a materialised entity is a plain field read that never re-enters the + /// DbContext. Turn lazy-loading proxies on and every one of those reads becomes a + /// database round-trip on the shared context from N threads at once — the row's named + /// trigger, and a failure mode that surfaces as sporadic "A second operation was + /// started on this context instance" rather than a clean break. + /// + /// Pinned here rather than at the call site because the hazard is a DI/configuration + /// change made somewhere else entirely, by someone who will never read + /// ArtifactDeploymentService. + /// + /// + [Fact] + public void AddConfigurationDatabase_DoesNotEnableLazyLoading() + { + var services = new ServiceCollection(); + services.AddConfigurationDatabase("Server=unused;Database=unused;"); + using var sp = services.BuildServiceProvider(); + + var options = sp.GetRequiredService>(); + + Assert.DoesNotContain( + options.Extensions, + e => e.GetType().Name.Contains("Proxies", StringComparison.Ordinal)); + + // Belt and braces, and the structurally stronger half: UseLazyLoadingProxies() is + // not even a compilable call without the Proxies package, so asserting the package + // is absent from the whole repo closes the hazard at the source rather than at one + // registration. (Mirrors Csproj_ShouldNotHaveForbiddenPackageReferences in + // Commons.Tests/ArchitecturalConstraintTests.) + var repoRoot = GetRepositoryRoot(); + var projectFiles = Directory + .GetFiles(repoRoot, "*.csproj", SearchOption.AllDirectories) + .Concat(Directory.GetFiles(repoRoot, "Directory.Packages.props", SearchOption.AllDirectories)) + .Where(f => !f.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}", + StringComparison.Ordinal) + && !f.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}", + StringComparison.Ordinal)) + .ToList(); + + Assert.NotEmpty(projectFiles); + foreach (var file in projectFiles) + { + Assert.DoesNotContain( + "EntityFrameworkCore.Proxies", + File.ReadAllText(file), + StringComparison.Ordinal); + } + } + + /// Walks up from the test output directory to the repository root. + private static string GetRepositoryRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir != null) + { + if (File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.ScadaBridge.slnx"))) + { + return dir.FullName; + } + + dir = dir.Parent; + } + + throw new InvalidOperationException("Could not locate the repository root (ZB.MOM.WW.ScadaBridge.slnx)."); + } + // The no-arg overload is [Obsolete(error: true)], so it cannot be referenced directly // from source — that is the compile-time guard. Invoke it via reflection to verify the // runtime defence-in-depth behaviour. diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/FleetWideUnlockThrottleTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/FleetWideUnlockThrottleTests.cs index ff2bcf60..ddd8dd31 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/FleetWideUnlockThrottleTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/FleetWideUnlockThrottleTests.cs @@ -252,6 +252,70 @@ public sealed class FleetWideUnlockThrottleTests : IDisposable Assert.Equal(2, await ctx.BundleUnlockAttempts.AsNoTracking().CountAsync()); } + [Fact] + public async Task ScheduledPrune_ClearsAgedRows_WithNoUnlockTrafficAtAll() + { + // Register row 65: the prune used to ride the unlock path ONLY, so a cluster that + // stopped importing entirely left its last rows in place indefinitely. The + // periodic sweep now drives PruneAsync directly — no second unlock attempt is made + // anywhere in this test, which is the whole point. + var clock = new TestClock(new DateTimeOffset(2026, 6, 1, 12, 0, 0, TimeSpan.Zero)); + var window = new SqlBundleUnlockAttemptWindow( + _provider.GetRequiredService(), clock); + + Assert.True(await window.TryRegisterAttemptAsync( + "10.0.0.1", 10, BundleUnlockRateLimiter.DefaultWindow)); + + // Past both the 2-hour retention horizon and the 15-minute prune cadence. + clock.Advance(TimeSpan.FromHours(3)); + await window.PruneAsync(BundleUnlockRateLimiter.DefaultWindow); + + using var scope = _provider.CreateScope(); + var ctx = scope.ServiceProvider.GetRequiredService(); + Assert.Empty(await ctx.BundleUnlockAttempts.AsNoTracking().ToListAsync()); + } + + [Fact] + public async Task ScheduledPrune_RespectsTheRetentionHorizonAndTheCadenceClaim() + { + // Two negative controls in one: a row inside the retention horizon survives, and + // the shared cadence claim means a burst of ticks cannot prune more often than + // PruneMinInterval (so ticking on the 1-minute session sweep is cheap). + var clock = new TestClock(new DateTimeOffset(2026, 6, 1, 12, 0, 0, TimeSpan.Zero)); + var window = new SqlBundleUnlockAttemptWindow( + _provider.GetRequiredService(), clock); + + Assert.True(await window.TryRegisterAttemptAsync( + "10.0.0.1", 10, BundleUnlockRateLimiter.DefaultWindow)); + + // Recent row, past the cadence: pruning runs but must delete nothing. + clock.Advance(TimeSpan.FromMinutes(30)); + for (var i = 0; i < 5; i++) + { + await window.PruneAsync(BundleUnlockRateLimiter.DefaultWindow); + } + + using var scope = _provider.CreateScope(); + var ctx = scope.ServiceProvider.GetRequiredService(); + Assert.Equal(1, await ctx.BundleUnlockAttempts.AsNoTracking().CountAsync()); + } + + [Fact] + public async Task ScheduledPrune_DoesNotThrow_WhenTheStoreIsUnavailable() + { + // The sweep that drives this has no caller to report to, so PruneAsync's contract + // is "never throw" — a database blip must not kill the background service loop. + var services = new ServiceCollection(); + services.AddScoped( + _ => throw new InvalidOperationException("database unavailable")); + using var broken = services.BuildServiceProvider(); + + var window = new SqlBundleUnlockAttemptWindow( + broken.GetRequiredService(), TimeProvider.System); + + await window.PruneAsync(BundleUnlockRateLimiter.DefaultWindow); + } + /// Minimal mutable-clock for deterministic windows. private sealed class TestClock : TimeProvider { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs index 1c85e604..88b24938 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionObservabilityTests.cs @@ -219,7 +219,7 @@ public sealed class BundleSessionObservabilityTests // against this test's own store (register row 74) — see ReadGaugeDelta. var clock = new TestClock(DateTimeOffset.UtcNow); var store = new BundleSessionStore(Options.Create(new TransportOptions()), clock, NullLogger.Instance); - var service = new BundleSessionEvictionService(store, NullLogger.Instance); + var service = new BundleSessionEvictionService(store, InertUnlockWindow.Instance, NullLogger.Instance); await service.StartAsync(CancellationToken.None); try @@ -259,8 +259,8 @@ public sealed class BundleSessionObservabilityTests var clock = new TestClock(DateTimeOffset.UtcNow); var oldStore = new BundleSessionStore(Options.Create(new TransportOptions()), clock, NullLogger.Instance); var newStore = new BundleSessionStore(Options.Create(new TransportOptions()), clock, NullLogger.Instance); - var oldService = new BundleSessionEvictionService(oldStore, NullLogger.Instance); - var newService = new BundleSessionEvictionService(newStore, NullLogger.Instance); + var oldService = new BundleSessionEvictionService(oldStore, InertUnlockWindow.Instance, NullLogger.Instance); + var newService = new BundleSessionEvictionService(newStore, InertUnlockWindow.Instance, NullLogger.Instance); await oldService.StartAsync(CancellationToken.None); await newService.StartAsync(CancellationToken.None); @@ -423,6 +423,24 @@ public sealed class BundleSessionObservabilityTests public void Advance(TimeSpan by) => _now = _now.Add(by); } + /// + /// Do-nothing for the eviction-service tests. + /// The service also ticks the unlock-attempt retention prune (register row 65), but + /// these tests exercise only its gauge lifecycle — never ExecuteAsync — so the + /// window is present purely to satisfy the constructor. The prune tick itself is + /// covered by FleetWideUnlockThrottleTests against the real SQL-backed window. + /// + private sealed class InertUnlockWindow : IBundleUnlockAttemptWindow + { + public static readonly InertUnlockWindow Instance = new(); + + public Task TryRegisterAttemptAsync( + string clientKey, int maxAttemptsPerWindow, TimeSpan window, CancellationToken ct = default) + => Task.FromResult(true); + + public Task PruneAsync(TimeSpan window, CancellationToken ct = default) => Task.CompletedTask; + } + /// Captures formatted log entries so the cap-rejection Warning can be asserted. private sealed class CapturingLogger : ILogger {