diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs index 2b883bde..598c3bbc 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs @@ -159,14 +159,20 @@ public static class TemplateCommands private static Command BuildList(Option urlOption, Option formatOption, Option usernameOption, Option passwordOption) { + // The server projects this listing in the database and returns + // TemplateSummary rows (counts, no child collections), so --detail cannot + // produce full definitions here — it only skips the CLI-side column + // projection and renders the summary payload verbatim. Full definitions + // come from `template get --id`. var detailOption = new Option("--detail") { - Description = "Include full template definitions (all attributes/alarms/scripts) in table output. " - + "Without it, table output is a compact summary (counts only). JSON output is always full." + Description = "Render the raw list payload in table output instead of the compact column projection. " + + "The server returns summary rows (member counts only) for a listing, so this does NOT include " + + "attribute/alarm/script definitions — use 'template get --id ' for those. No effect on JSON output." }; var skipOption = new Option("--skip") { Description = "Offset paging: number of items to skip (default 0)" }; var takeOption = new Option("--take") { Description = "Offset paging: max items to return (1..1000; omit for unlimited)" }; - var cmd = new Command("list") { Description = "List all templates (compact table summary; use --detail for the full dump)" }; + var cmd = new Command("list") { Description = "List all templates (compact table summary with member counts; definitions via 'template get')" }; cmd.Add(detailOption); cmd.Add(skipOption); cmd.Add(takeOption); diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateTableProjection.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateTableProjection.cs index 107ec22a..e5c46aaa 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateTableProjection.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateTableProjection.cs @@ -5,12 +5,30 @@ namespace ZB.MOM.WW.ScadaBridge.CLI.Commands; /// /// Compact table projection for template list / template get. -/// The management API returns full Template entities — every attribute, alarm, +/// +/// +/// template get returns a full Template entity — every attribute, alarm, /// script, and composition inline — which the generic table renderer dumps as one giant -/// cell per template (~171 KB for a real catalogue, unusable in a terminal). This -/// projector reduces each template to id / name / description / parent / derived plus -/// member counts, leaving JSON output untouched (callers pass this only on the -/// table path) and the full dump available via the command's --detail flag. +/// cell per template (~171 KB for a real catalogue, unusable in a terminal). +/// template list no longer returns entities at all: the management +/// ListTemplates handler projects in the DATABASE and returns +/// TemplateSummary rows whose children are already reduced to +/// attributeCount/alarmCount/… scalars. +/// +/// +/// +/// This projector handles BOTH shapes: it prefers the pre-computed *Count +/// scalar and falls back to the length of the matching child array, so the same +/// column set renders for a summary row and for a full entity. Reading only the +/// arrays (as it originally did) made template list print a wall of zeros +/// once the server switched to summaries. +/// +/// +/// +/// JSON output is left untouched (callers pass this only on the table path), and +/// the command's --detail flag skips the projection to render the raw +/// payload as-is. +/// /// internal static class TemplateTableProjection { @@ -67,11 +85,11 @@ internal static class TemplateTableProjection ["description"] = Str(element, "description"), ["parentTemplateId"] = Int(element, "parentTemplateId"), ["isDerived"] = Bool(element, "isDerived"), - ["#attrs"] = Count(element, "attributes"), - ["#alarms"] = Count(element, "alarms"), - ["#scripts"] = Count(element, "scripts"), - ["#comps"] = Count(element, "compositions"), - ["#nativeAlarms"] = Count(element, "nativeAlarmSources"), + ["#attrs"] = Count(element, "attributeCount", "attributes"), + ["#alarms"] = Count(element, "alarmCount", "alarms"), + ["#scripts"] = Count(element, "scriptCount", "scripts"), + ["#comps"] = Count(element, "compositionCount", "compositions"), + ["#nativeAlarms"] = Count(element, "nativeAlarmSourceCount", "nativeAlarmSources"), }; } @@ -104,9 +122,28 @@ internal static class TemplateTableProjection ? JsonValue.Create(v.GetBoolean()) : null; - private static JsonNode Count(JsonElement obj, string name) - => JsonValue.Create( - TryGetPropertyCI(obj, name, out var v) && v.ValueKind == JsonValueKind.Array + /// + /// Member count for a column, preferring the summary payload's pre-computed + /// scalar () and falling back to the length of the + /// full entity's child array (). Zero when neither + /// is present. + /// + /// The template object being projected. + /// Scalar count property on a TemplateSummary row. + /// Child-collection property on a full Template entity. + /// The member count as a JSON number. + private static JsonNode Count(JsonElement obj, string countName, string arrayName) + { + if (TryGetPropertyCI(obj, countName, out var scalar) + && scalar.ValueKind == JsonValueKind.Number + && scalar.TryGetInt32(out var n)) + { + return JsonValue.Create(n); + } + + return JsonValue.Create( + TryGetPropertyCI(obj, arrayName, out var v) && v.ValueKind == JsonValueKind.Array ? v.GetArrayLength() : 0); + } } diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs index 18304a41..a65f5f09 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/ManagementHttpClient.cs @@ -9,33 +9,52 @@ public class ManagementHttpClient : IDisposable private readonly HttpClient _httpClient; /// - /// WP2.6e (arch-review misc — CLI HttpClient timeout): default overall - /// for the shared client construction (30 s). This - /// bounds a hung/black-holed connection — before this, the public constructor left - /// at its framework default (100 s), silently longer - /// than most CLI callers' own per-request TimeSpan timeout argument - /// (// - /// already bound each call via their own , but a - /// connection attempt that never completes at all — no response headers, ever — is - /// bounded by instead, since that governs the whole - /// request/response including connect). Config-overridable via the - /// SCADABRIDGE_HTTP_TIMEOUT_SECONDS environment variable, consistent with how - /// every other CLI setting is overridden (see ) — kept - /// self-contained here (no /command-file plumbing) since CLI - /// commands are owned by a separate work package this phase. + /// Default bound on the CONNECT phase only (30 s) — how long a TCP/TLS + /// connection attempt to a black-holed management endpoint may hang before the + /// call fails. + /// + /// + /// It is deliberately NOT an overall request timeout. + /// caps the whole request/response, so setting + /// it to any fixed value silently truncates every caller whose own per-call + /// TimeSpan timeout argument is longer: the effective deadline becomes + /// min(HttpClient.Timeout, caller timeout). That is exactly what a 30 s + /// client timeout did to deploy site's 5-minute bulk deploy and to the + /// five-minute bundle export/preview/import calls — each printed a fake + /// 504 Request timed out at 30 s while the server carried on working. + /// is therefore + /// and the per-call + /// in + /// // + /// is the SINGLE overall deadline — it bounds connect too, since the token is + /// passed into the send itself. + /// + /// + /// + /// The connect bound lives on + /// instead, which is connect-scoped and so cannot truncate a long-running + /// request that has already reached the server. Overridable via the + /// SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS environment variable, + /// consistent with how every other CLI setting is overridden (see + /// ) — kept self-contained here (no + /// /command-file plumbing) since CLI commands are owned + /// by a separate work package this phase. + /// /// - public static readonly TimeSpan DefaultTimeout = TimeSpan.FromSeconds(30); + public static readonly TimeSpan DefaultConnectTimeout = TimeSpan.FromSeconds(30); - /// Test seam (WP2.6e) — the effective this instance was constructed with. + /// Test seam — the effective this instance was constructed with. internal TimeSpan EffectiveTimeout { get; } /// - /// Resolves the effective default timeout: the SCADABRIDGE_HTTP_TIMEOUT_SECONDS - /// environment variable when set to a positive integer, otherwise . + /// Resolves the effective connect timeout: the + /// SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS environment variable when set + /// to a positive integer, otherwise . /// - private static TimeSpan ResolveDefaultTimeout() + /// The connect timeout to apply to the socket handler. + internal static TimeSpan ResolveConnectTimeout() { - var env = Environment.GetEnvironmentVariable("SCADABRIDGE_HTTP_TIMEOUT_SECONDS"); + var env = Environment.GetEnvironmentVariable("SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS"); if (!string.IsNullOrWhiteSpace(env) && int.TryParse(env, out var seconds) && seconds > 0) @@ -43,19 +62,26 @@ public class ManagementHttpClient : IDisposable return TimeSpan.FromSeconds(seconds); } - return DefaultTimeout; + return DefaultConnectTimeout; } /// - /// Initializes a new instance of the class, with - /// set to - /// (30 s, or the SCADABRIDGE_HTTP_TIMEOUT_SECONDS override). + /// Initializes a new instance of the class with + /// an INFINITE (each call supplies its own + /// deadline) over a whose + /// is + /// . /// /// The base URL for the management API. /// The username for HTTP Basic authentication. /// The password for HTTP Basic authentication. public ManagementHttpClient(string baseUrl, string username, string password) - : this(new HttpClient { Timeout = ResolveDefaultTimeout() }, baseUrl, username, password) + : this( + new HttpClient(new SocketsHttpHandler { ConnectTimeout = ResolveConnectTimeout() }) + { + Timeout = Timeout.InfiniteTimeSpan + }, + baseUrl, username, password) { } diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index e737dd63..c4d7f802 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -86,28 +86,33 @@ Exit codes: #### `template list` -List all templates. **Table** output (`--format table`) shows a compact summary — id, -name, description, parent, and member **counts** (`#attrs`, `#alarms`, `#scripts`, -`#comps`, `#nativeAlarms`) — so it stays readable in a terminal. Add `--detail` to dump -the full attribute/alarm/script/composition definitions in the table. **JSON** output -(`--format json`) is always the full, unmodified payload regardless of `--detail`. +List all templates. The server projects this listing in the database and returns +**summary rows** — no child collections, member counts only. **Table** output +(`--format table`) renders them as id, name, description, parent, and member **counts** +(`#attrs`, `#alarms`, `#scripts`, `#comps`, `#nativeAlarms`), so it stays readable in a +terminal. `--detail` skips that column projection and renders the summary payload +verbatim; it does **not** add attribute/alarm/script definitions — a listing never +carries them. Use `template get --id ` for a template's full definition. **JSON** +output (`--format json`) is always the unmodified payload regardless of `--detail`. ```sh -scadabridge --url template list # compact table -scadabridge --url --format table template list --detail # full table dump -scadabridge --url --format json template list # full JSON (always) +scadabridge --url template list # compact table +scadabridge --url --format table template list --detail # raw summary rows +scadabridge --url --format json template list # unmodified JSON (always) ``` | Option | Required | Description | |--------|----------|-------------| -| `--detail` | no | Include full definitions in table output (no effect on JSON) | +| `--detail` | no | Render the raw summary payload instead of the compact column projection (no effect on JSON; does not add definitions) | | `--skip` | no | Offset paging: number of items to skip (default 0) | | `--take` | no | Offset paging: max items to return (clamped 1..1000; omit for unlimited) | #### `template get` -Get a single template by ID. Like `template list`, table output is a compact summary -unless `--detail` is supplied; JSON output is always full. +Get a single template by ID. Unlike `template list`, this returns the **full** template +entity with every attribute, alarm, script and composition inline. Table output is a +compact summary (the same counts as `template list`) unless `--detail` is supplied, which +dumps the full definitions; JSON output is always full. ```sh scadabridge --url template get --id [--detail] diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SiteRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SiteRepository.cs index b9bcfde9..10de8662 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SiteRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SiteRepository.cs @@ -2,6 +2,7 @@ using Microsoft.EntityFrameworkCore; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories; @@ -11,14 +12,23 @@ namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories; public class SiteRepository : ISiteRepository { private readonly ScadaBridgeDbContext _dbContext; + private readonly ITemplateGraphWatermark? _watermark; /// /// Initializes a new instance of the SiteRepository. /// /// The database context. - public SiteRepository(ScadaBridgeDbContext dbContext) + /// + /// Graph version watermark bumped when a committed change alters a flattening + /// input this repository owns (see ). Optional so + /// existing single-argument construction (tests, ad-hoc tooling) keeps + /// compiling; when absent the caches simply never get the invalidation signal + /// from this repository, which is the pre-existing behaviour. + /// + public SiteRepository(ScadaBridgeDbContext dbContext, ITemplateGraphWatermark? watermark = null) { _dbContext = dbContext ?? throw new ArgumentNullException(nameof(dbContext)); + _watermark = watermark; } // --- Sites --- @@ -166,8 +176,58 @@ public class SiteRepository : ISiteRepository } /// + /// + /// Bumps the after a successful commit + /// that touched a . + /// + /// + /// A data connection's Protocol, primary/backup configuration and + /// failover retry count are flattening inputs — FlatteningService + /// packages them into the flattened config's Connections map and + /// RevisionHashService folds them into the revision hash. Without a bump, + /// the process-wide StaleInstanceProbe memo and the flatten-session + /// caches keep reporting the pre-edit hash for every instance bound to the + /// connection, so an instance whose deployed config genuinely drifted reads as + /// up to date. + /// + /// + /// + /// The bump is rather than a + /// per-template one because a connection has no owning template: the affected + /// set is "every instance at the site bound to it", which is exactly what the + /// unattributed fallback is for. Applied only AFTER a successful commit, so a + /// failed save does not invalidate caches for a change that never landed. + /// Sites, areas and instances saved here are left alone — only the connection + /// rows feed the flattener. + /// + /// public async Task SaveChangesAsync(CancellationToken cancellationToken = default) { - return await _dbContext.SaveChangesAsync(cancellationToken); + // Inspect BEFORE committing: afterwards every entry is Unchanged and the + // attribution is lost. + var connectionsChanged = _watermark is not null && HasPendingDataConnectionChange(); + + var written = await _dbContext.SaveChangesAsync(cancellationToken); + + if (connectionsChanged) + _watermark!.BumpAll(); + + return written; + } + + /// + /// True when the change tracker holds an added, modified or deleted + /// . + /// + /// Whether a data-connection mutation is about to be committed. + private bool HasPendingDataConnectionChange() + { + foreach (var entry in _dbContext.ChangeTracker.Entries()) + { + if (entry.State is EntityState.Added or EntityState.Modified or EntityState.Deleted) + return true; + } + + return false; } } diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs index 83e7d017..99a70640 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/TemplateEngineRepository.cs @@ -865,7 +865,18 @@ public class TemplateEngineRepository : ITemplateEngineRepository unattributed = true; break; default: - // Areas, folders and anything else do not feed the flattener. + // Anything this switch does not recognise is invalidated + // conservatively, as the doc comment above promises. The + // previous "areas, folders and anything else do not feed the + // flattener" no-op was wrong in at least one shipped case: + // DataConnection rows land here, and their Protocol / + // Primary+Backup configuration / FailoverRetryCount ARE hash + // inputs (FlatteningService packages them as ConnectionConfig, + // RevisionHashService hashes them as HashableConnection). A + // silent no-op on an unrecognised entity is the one failure + // mode this watermark must not have: it produces STALE work, + // whereas an over-broad bump only ever produces extra work. + unattributed = true; break; } } diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs index 3fef99e7..f88a6111 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs @@ -193,7 +193,8 @@ public class DeploymentService using (prepared.LockHandle) { - var outcome = await SendDeploymentAsync(prepared, cancellationToken); + var outcome = await StageAndSendDeploymentAsync( + prepared, user, dbGate: null, cancellationToken); return await FinalizeDeploymentAsync(prepared, outcome, user, cancellationToken); } } @@ -201,9 +202,18 @@ public class DeploymentService /// /// Phase 1 of a deployment (serial, database-bound): validate the state /// transition, take the per-instance operation lock, mint the deployment id, - /// flatten + validate, run query-before-redeploy reconciliation, stage the - /// PendingDeployment row and insert the InProgress - /// . + /// flatten + validate, run query-before-redeploy reconciliation and insert the + /// InProgress . + /// + /// + /// Note what is NOT here: the PendingDeployment row. Its fetch token + /// expires after it is + /// STAGED, so staging in phase 1 started the clock on every instance in a bulk + /// deploy at once while phase 2 reached them one batch at a time — with enough + /// instances the tail's tokens expired before their command was ever sent, and + /// the site's fetch 404'd. Staging therefore moved to + /// , immediately before each send. + /// /// /// /// Everything here touches the scoped, non-thread-safe DbContext, so it @@ -367,48 +377,135 @@ public class DeploymentService try { - // Notify-and-fetch: instead of shipping the (potentially oversized, - // silently-dropped >128 KB) flattened config inline in a - // DeployInstanceCommand, stage it in a PendingDeployment row and send - // a small RefreshDeploymentCommand. The site fetches the config from - // CentralFetchBaseUrl over HTTP using the per-deployment fetch token. + // Site routing is resolved here (a repository read) so phase 2 needs + // nothing but the staging write itself. var siteId = await ResolveSiteIdentifierAsync(instance.SiteId, cancellationToken); - var token = DeploymentFetchToken.Generate(); - var stagedAt = DateTimeOffset.UtcNow; - await _repository.AddPendingDeploymentAsync(new PendingDeployment( - deploymentId, instanceId, revisionHash, configJson, token, - stagedAt, stagedAt + _commOptions.PendingDeploymentTtl), cancellationToken); - await _repository.SaveChangesAsync(cancellationToken); - - var command = new RefreshDeploymentCommand( - deploymentId, instance.UniqueName, revisionHash, user, stagedAt, - _commOptions.CentralFetchBaseUrl, token); - - // Cleanup of the staged PendingDeployment is TTL-based ONLY — the row - // is deliberately NOT deleted on success or on failure. On a - // central-side Ask timeout the site may have applied AND told the - // standby node to fetch; deleting now would 404 that in-flight - // standby fetch and break failover. Supersession bounds pending rows - // to ≤1 per instance and the fetch endpoint enforces the TTL, so - // leaving rows for TTL purge is safe. Expired rows are swept by the - // central PendingDeploymentPurgeActor singleton on its - // CommunicationOptions.PendingDeploymentPurgeInterval cadence. return new PreparedDeployment( instance, deploymentId, record, revisionHash, configJson, - siteId, command, lockHandle, EarlyResult: null); + siteId, lockHandle, EarlyResult: null); } catch (Exception ex) { - // Staging failed before anything was sent. Record the failure exactly - // as the post-send path does (never leave the record InProgress) and - // release the lock — there is no phase 2 or 3 to run. + // Preparation failed before anything was staged or sent. Record the + // failure exactly as the post-send path does (never leave the record + // InProgress) and release the lock — there is no phase 2 or 3 to run. await MarkDeploymentFailedAsync(record, instance, deploymentId, user, ex); lockHandle.Dispose(); return PreparedDeployment.Resolved(FailureResultFor(ex)); } } + /// + /// Stages the deployment's PendingDeployment row and immediately sends + /// its RefreshDeploymentCommand — phase 2. + /// + /// + /// Notify-and-fetch: instead of shipping the (potentially oversized, + /// silently-dropped >128 KB) flattened config inline in a + /// DeployInstanceCommand, the config is staged in a + /// PendingDeployment row and a small RefreshDeploymentCommand is + /// sent; the site then fetches the config from CentralFetchBaseUrl over + /// HTTP using the per-deployment fetch token. + /// + /// + /// + /// Staging is immediately before the send, on purpose. The row's + /// ExpiresAt is stagedAt + PendingDeploymentTtl (5 min by + /// default) and the fetch endpoint enforces it, so the gap between staging and + /// sending is dead time burned off the token's life. Staging the whole batch up + /// front in phase 1 made that gap grow with batch size — for a large site the + /// last instances' tokens could expire before their command was even sent. + /// + /// + /// + /// Serialisation. Staging is the ONE repository touch in this otherwise + /// network-only phase, and the scoped DbContext is not thread-safe, so a + /// bulk deploy passes — a 1-permit semaphore that lets + /// exactly one in-flight send do its staging write at a time. The sends + /// themselves stay fully concurrent (they touch no repository), so this costs no + /// parallelism, and the single-instance path passes + /// because there is nothing to serialise against. + /// + /// + /// + /// A staging failure is returned as the phase-2 fault rather than thrown, so + /// phase 3 marks the record Failed on exactly the same path as a send fault — + /// the record must never be left InProgress. + /// + /// + /// The prepared deployment to stage and send. + /// User attributed on the refresh command. + /// Serialises the staging write across a concurrent batch; for a batch of one. + /// Cancellation token, already carrying any per-instance deadline. + /// The site's response, or the exception that prevented one. + private async Task StageAndSendDeploymentAsync( + PreparedDeployment prepared, + string user, + SemaphoreSlim? dbGate, + CancellationToken cancellationToken) + { + RefreshDeploymentCommand command; + try + { + if (dbGate is not null) + await dbGate.WaitAsync(cancellationToken); + try + { + command = await StagePendingDeploymentAsync(prepared, user, cancellationToken); + } + finally + { + dbGate?.Release(); + } + } + catch (Exception ex) + { + return new SendOutcome(null, ex); + } + + return await SendDeploymentAsync(prepared, command, cancellationToken); + } + + /// + /// Writes the PendingDeployment row for a prepared deployment and builds + /// the matching RefreshDeploymentCommand, stamping stagedAt at the + /// moment of the write. + /// + /// + /// Cleanup of the staged row is TTL-based ONLY — it is deliberately NOT deleted + /// on success or on failure. On a central-side Ask timeout the site may have + /// applied AND told the standby node to fetch; deleting now would 404 that + /// in-flight standby fetch and break failover. Supersession bounds pending rows + /// to ≤1 per instance and the fetch endpoint enforces the TTL, so leaving rows + /// for TTL purge is safe. Expired rows are swept by the central + /// PendingDeploymentPurgeActor singleton on its + /// cadence. + /// + /// + /// The prepared deployment being staged. + /// User attributed on the refresh command. + /// Cancellation token. + /// The refresh command to send to the site. + private async Task StagePendingDeploymentAsync( + PreparedDeployment prepared, + string user, + CancellationToken cancellationToken) + { + var token = DeploymentFetchToken.Generate(); + var stagedAt = DateTimeOffset.UtcNow; + + await _repository.AddPendingDeploymentAsync(new PendingDeployment( + prepared.DeploymentId, prepared.Instance.Id, prepared.RevisionHash, + prepared.ConfigJson, token, + stagedAt, stagedAt + _commOptions.PendingDeploymentTtl), cancellationToken); + await _repository.SaveChangesAsync(cancellationToken); + + return new RefreshDeploymentCommand( + prepared.DeploymentId, prepared.Instance.UniqueName, prepared.RevisionHash, + user, stagedAt, _commOptions.CentralFetchBaseUrl, token); + } + /// /// Phase 2 of a deployment (parallelisable, network-bound): the /// RefreshDeploymentCommand round-trip to the site. Touches no @@ -416,10 +513,12 @@ public class DeploymentService /// out across a batch while phases 1 and 3 stay serial. /// /// The prepared deployment to send. + /// The refresh command built by the staging step immediately before this call. /// Cancellation token, already carrying any per-instance deadline. /// The site's response, or the exception that prevented one. private async Task SendDeploymentAsync( PreparedDeployment prepared, + RefreshDeploymentCommand command, CancellationToken cancellationToken) { try @@ -429,7 +528,7 @@ public class DeploymentService prepared.DeploymentId, prepared.Instance.UniqueName, prepared.SiteIdentifier); var response = await _communicationService.RefreshDeploymentAsync( - prepared.SiteIdentifier, prepared.Command, cancellationToken); + prepared.SiteIdentifier, command, cancellationToken); return new SendOutcome(response, null); } catch (Exception ex) @@ -630,7 +729,6 @@ public class DeploymentService string RevisionHash, string ConfigJson, string SiteIdentifier, - RefreshDeploymentCommand Command, IDisposable? LockHandle, Result? EarlyResult) { @@ -638,7 +736,7 @@ public class DeploymentService /// The result to hand back to the caller. /// A prepared deployment whose is set. public static PreparedDeployment Resolved(Result result) => - new(null!, string.Empty, null!, string.Empty, string.Empty, string.Empty, null!, null, result); + new(null!, string.Empty, null!, string.Empty, string.Empty, string.Empty, null, result); } /// @@ -655,21 +753,23 @@ public class DeploymentService /// /// /// - /// Prepare (serial). Every instance is flattened, validated, staged - /// and given an InProgress record on the caller's single scoped + /// Prepare (serial). Every instance is flattened, validated and given + /// an InProgress record on the caller's single scoped /// DbContext. All instances share ONE , /// so a template chain common to N instances is walked once, and the /// session-global queries (shared scripts, schema library, the site's data /// connections) run once for the whole batch instead of once per instance. /// /// - /// Send (bounded parallel). Site round-trips run concurrently up to - /// , + /// Stage + send (bounded parallel). Site round-trips run concurrently + /// up to , /// each under its own /// - /// deadline, so one wedged instance cannot stall the batch. This phase - /// touches no repository — that is precisely why it is the only phase that - /// may run in parallel against a non-thread-safe DbContext. + /// deadline, so one wedged instance cannot stall the batch. Each instance's + /// PendingDeployment row is staged here, immediately before its own + /// send, so a fetch token's TTL is not burned waiting for the batch ahead of + /// it; that single repository touch is serialised behind a 1-permit + /// semaphore, since the DbContext is not thread-safe. /// /// /// Finalize (serial). Terminal statuses, post-success side effects @@ -710,54 +810,81 @@ public class DeploymentService var prepared = new List(instances.Count); var results = new List(instances.Count); - foreach (var instance in instances) + // Index of the first prepared deployment phase 3 has NOT yet finalised. + // Everything from here to the end of `prepared` still owns an operation + // lock and an InProgress record, so the cleanup handler below knows exactly + // what it must unwind if the operation escapes early. + var finalizedUpTo = 0; + try { - cancellationToken.ThrowIfCancellationRequested(); + foreach (var instance in instances) + { + cancellationToken.ThrowIfCancellationRequested(); - PreparedDeployment step; - try - { - step = await PrepareDeploymentAsync(instance.Id, user, session, cancellationToken); - } - catch (Exception ex) - { - // A prepare fault (most commonly a TimeoutException from the - // operation lock because another operation holds this instance) - // fails only this instance. Recording it and moving on is what - // makes a bulk deploy usable while individual instances are busy. - _logger.LogWarning(ex, - "Preparing instance {Instance} for bulk deployment of site {SiteId} failed", - instance.UniqueName, site.SiteIdentifier); - results.Add(new InstanceDeploymentResult( - instance.Id, instance.UniqueName, null, false, ex.Message)); - continue; + PreparedDeployment step; + try + { + step = await PrepareDeploymentAsync(instance.Id, user, session, cancellationToken); + } + catch (Exception ex) + { + // A prepare fault (most commonly a TimeoutException from the + // operation lock because another operation holds this instance) + // fails only this instance. Recording it and moving on is what + // makes a bulk deploy usable while individual instances are busy. + _logger.LogWarning(ex, + "Preparing instance {Instance} for bulk deployment of site {SiteId} failed", + instance.UniqueName, site.SiteIdentifier); + results.Add(new InstanceDeploymentResult( + instance.Id, instance.UniqueName, null, false, ex.Message)); + continue; + } + + if (step.EarlyResult is { } early) + { + results.Add(ToInstanceResult(instance.Id, instance.UniqueName, early)); + continue; + } + + prepared.Add(step); } - if (step.EarlyResult is { } early) - { - results.Add(ToInstanceResult(instance.Id, instance.UniqueName, early)); - continue; - } + // ---- Phase 2: stage + bounded-parallel site round-trips. ---- + // Never throws: faults (cancellation included) come back as per-instance + // outcomes so phase 3 still runs for every prepared deployment. + var outcomes = await SendPreparedAsync(prepared, user, cancellationToken); - prepared.Add(step); + // ---- Phase 3: finalize, serially, releasing each lock as we go. ---- + for (; finalizedUpTo < prepared.Count; finalizedUpTo++) + { + var step = prepared[finalizedUpTo]; + try + { + var result = await FinalizeDeploymentAsync(step, outcomes[finalizedUpTo], user, cancellationToken); + results.Add(ToInstanceResult(step.Instance.Id, step.Instance.UniqueName, result, step.DeploymentId)); + } + finally + { + step.LockHandle?.Dispose(); + } + } } - - // ---- Phase 2: bounded-parallel site round-trips. ---- - var outcomes = await SendPreparedAsync(prepared, cancellationToken); - - // ---- Phase 3: finalize, serially, releasing each lock as we go. ---- - for (var i = 0; i < prepared.Count; i++) + catch (Exception ex) { - var step = prepared[i]; - try - { - var result = await FinalizeDeploymentAsync(step, outcomes[i], user, cancellationToken); - results.Add(ToInstanceResult(step.Instance.Id, step.Instance.UniqueName, result, step.DeploymentId)); - } - finally - { - step.LockHandle?.Dispose(); - } + // Nothing above may escape while a prepared deployment still holds its + // per-instance operation lock: OperationLockManager hands out real + // semaphores, so an undisposed handle wedges that instance against + // EVERY future mutating command for the life of the process — a + // permanent, restart-only outage for one instance. The two live escape + // routes are phase 1's ThrowIfCancellationRequested and a fault from + // phase 3's own persistence; both land here. + // + // Records are finalised as Failed for the same reason the send/persist + // paths do it: a deployment must never be left InProgress. Uses + // CancellationToken.None inside (see MarkDeploymentFailedAsync) so the + // already-cancelled token cannot abort the cleanup writes themselves. + await ReleasePreparedAsync(prepared, finalizedUpTo, user, ex); + throw; } var summary = new SiteDeploymentSummary( @@ -776,15 +903,76 @@ public class DeploymentService } /// - /// Runs phase 2 for a whole batch: every prepared deployment's site round-trip, - /// concurrent up to - /// and each bounded by + /// Unwinds the prepared deployments a bulk deploy never finalised, after the + /// operation escaped early (cancellation, or a fault in phase 3's own + /// persistence). For each one it writes the terminal Failed status and releases + /// the per-instance operation lock. + /// + /// + /// Best-effort throughout: a fault while unwinding one entry must not stop the + /// remaining locks from being released, and must not mask the original + /// exception the caller is about to rethrow. The lock release in particular is + /// in a finally, because leaking it is the unrecoverable outcome — the + /// Failed status can be reconciled by an operator, a wedged semaphore cannot. + /// + /// + /// The batch's prepared deployments. + /// Index of the first entry phase 3 did not finalise. + /// User attributed on the failure audit rows. + /// The exception that aborted the operation. + /// A task that completes once every outstanding entry has been unwound. + private async Task ReleasePreparedAsync( + IReadOnlyList prepared, + int from, + string user, + Exception cause) + { + for (var i = from; i < prepared.Count; i++) + { + var step = prepared[i]; + try + { + await MarkDeploymentFailedAsync( + step.Record, step.Instance, step.DeploymentId, user, cause); + } + catch (Exception ex) + { + _logger.LogError(ex, + "Failed to finalize deployment {DeploymentId} for instance {Instance} while unwinding " + + "an aborted bulk deployment", + step.DeploymentId, step.Instance.UniqueName); + } + finally + { + step.LockHandle?.Dispose(); + } + } + } + + /// + /// Runs phase 2 for a whole batch: every prepared deployment's staging write + /// plus site round-trip, concurrent up to + /// and each + /// bounded by /// . /// Outcomes are returned positionally so phase 3 can pair them back with their /// prepared deployment. + /// + /// + /// Never throws on cancellation: the whole point of returning outcomes + /// positionally is that phase 3 must run for EVERY prepared deployment, whatever + /// happened in phase 2 — otherwise records stay InProgress and their operation + /// locks stay held. A cancelled instance is recorded as its own + /// fault and finalised as Failed like any other. + /// /// + /// The prepared deployments to stage and send. + /// User attributed on each refresh command. + /// Cancellation token for the batch. + /// One outcome per prepared deployment, positionally aligned. private async Task SendPreparedAsync( IReadOnlyList prepared, + string user, CancellationToken cancellationToken) { var outcomes = new SendOutcome[prepared.Count]; @@ -792,15 +980,39 @@ public class DeploymentService return outcomes; using var gate = new SemaphoreSlim(_options.SiteDeploymentMaxParallelism); + // Serialises the ONE repository touch left in this phase (the staging + // write) across the concurrent sends — the scoped DbContext is not + // thread-safe. The sends themselves stay fully concurrent. + using var dbGate = new SemaphoreSlim(1, 1); var sends = prepared.Select(async (step, index) => { - await gate.WaitAsync(cancellationToken); + // Cancellation must not escape as an exception: it would abandon phase 3 + // for every instance, leaking their operation locks (a wedged + // per-instance semaphore is permanent for the life of the process) and + // stranding their records InProgress. + try + { + await gate.WaitAsync(cancellationToken); + } + catch (OperationCanceledException ex) + { + outcomes[index] = new SendOutcome(null, ex); + return; + } + try { using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); cts.CancelAfter(_options.SiteDeploymentTimeoutPerInstance); - outcomes[index] = await SendDeploymentAsync(step, cts.Token); + outcomes[index] = await StageAndSendDeploymentAsync(step, user, dbGate, cts.Token); + } + catch (Exception ex) + { + // StageAndSendDeploymentAsync already converts its own faults into + // outcomes; this is the belt-and-braces net so no send task can + // fault Task.WhenAll and skip phase 3. + outcomes[index] = new SendOutcome(null, ex); } finally { diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs index 973c5b73..4663653a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs @@ -49,7 +49,12 @@ public static class ScriptCompileVerdictCache /// /// Upper bound on entries in the hot generation. The cache holds at most /// 2 × SegmentCapacity entries in total (hot + cold), preserving the - /// previous 4096-entry ceiling. + /// previous 4096-entry ceiling. Enforced on EVERY insert into hot — fresh + /// verdicts and cold-generation promotions alike, both of which go through + /// . (Hot can momentarily exceed the bound by the number of + /// threads that pass the pre-rotation check concurrently; the bound is a + /// memory ceiling, not a hard invariant, and that overshoot is bounded by + /// concurrency rather than by working-set size.) /// private const int SegmentCapacity = 2048; @@ -61,8 +66,13 @@ public static class ScriptCompileVerdictCache /// private static readonly object RotateGate = new(); - private static ConcurrentDictionary _hot = new(); - private static ConcurrentDictionary _cold = new(); + // Volatile: both fields are REPLACED wholesale by a rotation under + // RotateGate, while readers on GetOrAdd/Store run lock-free. Without the + // volatile read a reader is free to cache the reference (or observe the + // rotation's two writes out of order) and keep serving — or worse, keep + // inserting into — a generation the rotation has already retired. + private static volatile ConcurrentDictionary _hot = new(); + private static volatile ConcurrentDictionary _cold = new(); private static long _hits; private static long _evictions; @@ -110,11 +120,18 @@ public static class ScriptCompileVerdictCache if (cold.TryGetValue(key, out verdict)) { // Promote: the entry is in active use, so it must survive the next - // rotation. Writing to the CURRENT hot generation (re-read, in case a - // rotation happened since the snapshot) is what makes this an LRU - // rather than a fixed-lifetime cache. + // rotation. Goes through Store (rather than a direct `_hot[key] =`) + // so a promotion obeys SegmentCapacity like any other insert — a + // direct write let hot grow past the segment bound whenever a working + // set larger than SegmentCapacity was being re-read, taking the true + // ceiling to 3 × SegmentCapacity against a documented 2 ×. Store + // writes to the CURRENT hot generation (re-read inside, in case a + // rotation happened since the snapshot above), which is what makes + // this an LRU rather than a fixed-lifetime cache; a rotation + // triggered BY this promotion still keeps the entry, because the + // insert lands in the fresh hot generation afterwards. Interlocked.Increment(ref _hits); - _hot[key] = verdict; + Store(key, verdict); return verdict; } diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index 17a912a2..ef5a9932 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -87,6 +87,12 @@ public sealed class BundleImporter : IBundleImporter // then best-effort empty (informational only, never gates the import). private readonly IStaleInstanceProbe? _staleInstanceProbe; private readonly IScriptArtifactChangeBus? _scriptArtifactChangeBus; + // Optional. Bumped once per apply ATTEMPT (see ApplyAsync) because this + // importer commits through the raw DbContext, bypassing + // TemplateEngineRepository.SaveChangesAsync — the only place that otherwise + // derives watermark bumps from the change tracker. Null in hosts that don't + // register it; the caches then simply never get this invalidation signal. + private readonly ITemplateGraphWatermark? _watermark; private readonly ILogger? _logger; /// @@ -112,6 +118,7 @@ public sealed class BundleImporter : IBundleImporter /// Optional: recomputes a deployed instance's current revision hash so the importer can enumerate instances stale-ed by a template overwrite. Null when no flattening pipeline is registered (Transport-without-DeploymentManager hosts) — staleness is then skipped. /// Optional: node-local bus notified AFTER a successful commit that script-bearing artifacts (ApiMethod / SharedScript / Template) were overwritten or renamed, so compiled-handler caches can invalidate. Null on hosts that don't register it (staleness/invalidation then relies on the consumer's own self-heal). /// Optional logger. + /// Optional: template-graph version watermark, bumped once per apply attempt so the flatten-session caches and the process-wide staleness memo cannot serve pre-import (or rolled-back) readings. Null on hosts that don't register it. public BundleImporter( BundleSerializer bundleSerializer, ManifestValidator manifestValidator, @@ -132,7 +139,8 @@ public sealed class BundleImporter : IBundleImporter SemanticValidator semanticValidator, IStaleInstanceProbe? staleInstanceProbe = null, IScriptArtifactChangeBus? scriptArtifactChangeBus = null, - ILogger? logger = null) + ILogger? logger = null, + ITemplateGraphWatermark? watermark = null) { _bundleSerializer = bundleSerializer ?? throw new ArgumentNullException(nameof(bundleSerializer)); _manifestValidator = manifestValidator ?? throw new ArgumentNullException(nameof(manifestValidator)); @@ -153,6 +161,7 @@ public sealed class BundleImporter : IBundleImporter _semanticValidator = semanticValidator ?? throw new ArgumentNullException(nameof(semanticValidator)); _staleInstanceProbe = staleInstanceProbe; _scriptArtifactChangeBus = scriptArtifactChangeBus; + _watermark = watermark; _logger = logger; } @@ -1327,6 +1336,7 @@ public sealed class BundleImporter : IBundleImporter content, resolutions, nameMap, session, bundleImportId, user, ct).ConfigureAwait(false); await tx.CommitAsync(ct).ConfigureAwait(false); await tx.DisposeAsync().ConfigureAwait(false); + BumpGraphWatermark(); return applied; } catch @@ -1351,6 +1361,14 @@ public sealed class BundleImporter : IBundleImporter // persist on the next SaveChangesAsync (a retry or the // failure-row write below). _dbContext.ChangeTracker.Clear(); + + // Bump on the ROLLBACK path too. ComputeStaleInstanceIdsAsync + // runs PRE-commit (it reads the staged change tracker), so a + // rolled-back attempt has already written StaleInstanceProbe + // memos describing state that never landed. Leaving the + // watermark untouched would keep those memos "current" + // forever — including for the strategy's own next retry. + BumpGraphWatermark(); throw; } }).ConfigureAwait(false); @@ -1651,6 +1669,38 @@ public sealed class BundleImporter : IBundleImporter Warnings: validationWarnings); } + /// + /// Invalidates every template-graph cache keyed on the + /// , called once per apply ATTEMPT — after + /// the commit on the success path, and after the rollback on the failure path. + /// + /// + /// Why the importer must do this itself. Watermark bumps are normally + /// derived from the change tracker by + /// TemplateEngineRepository.SaveChangesAsync, but this importer commits + /// through the raw ScadaBridgeDbContext (a single deferred + /// SaveChangesAsync plus a handful of intra-transaction flushes), so not + /// one of its writes went through that path. The observable bug: a second + /// import overwriting the same template could be OMITTED from + /// ImportResult.StaleInstanceIds, because StaleInstanceProbe's + /// process-static memo — validated purely against watermark readings that never + /// moved — served the FIRST import's revision hash. + /// + /// + /// + /// Why both paths, and why . + /// The probe runs pre-commit, so a rolled-back attempt leaves memos for state + /// that never landed; bumping on rollback is what makes the memo non-transactional + /// yet still sound (the simplest correct shape — a transactional memo would mean + /// threading commit/rollback awareness through a process-static cache). And an + /// import touches an arbitrary, unattributed set of templates, instances and data + /// connections, so the unattributed global bump is both the correct granularity + /// and cheap: an import is a rare, heavyweight operation, and the watermark can + /// only ever cause EXTRA flattening work, never stale work. + /// + /// + private void BumpGraphWatermark() => _watermark?.BumpAll(); + /// /// T-007: zeros the session's /// buffer in place so any caller still holding a reference observes the diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/ManagementHttpClientTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/ManagementHttpClientTests.cs index e35da643..389d18d1 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/ManagementHttpClientTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/ManagementHttpClientTests.cs @@ -1,4 +1,6 @@ +using System.Diagnostics; using System.Net; +using System.Net.Sockets; using System.Text; using ZB.MOM.WW.ScadaBridge.CLI; @@ -106,32 +108,41 @@ public class ManagementHttpClientTests } /// -/// WP2.6e (arch-review misc — CLI HttpClient timeout): the public -/// constructor must bound its underlying -/// explicitly (30 s default) rather than leaving the -/// framework's 100 s default in place, and must honor the -/// SCADABRIDGE_HTTP_TIMEOUT_SECONDS override — consistent with how every other -/// CLI setting is environment-overridable (). Runs in the shared -/// "Environment" collection (see ) so it never races another -/// test mutating process-wide environment variables. +/// The public constructor must leave +/// INFINITE so the per-call +/// is the single overall deadline — a fixed +/// client timeout silently truncated every caller with a longer per-call timeout +/// (deploy site's 5-minute bulk deploy, the 5-minute bundle calls), +/// which then printed a fake 504 while the server kept working. The connect phase +/// is bounded separately on , honoring +/// the SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS override — consistent with how +/// every other CLI setting is environment-overridable (). Runs +/// in the shared "Environment" collection (see ) so it +/// never races another test mutating process-wide environment variables. /// [Collection("Environment")] public class ManagementHttpClientTimeoutTests { - private const string EnvVar = "SCADABRIDGE_HTTP_TIMEOUT_SECONDS"; + private const string EnvVar = "SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS"; [Fact] - public void DefaultConstructor_SetsThirtySecondTimeout_WhenEnvVarUnset() + public void DefaultConstructor_LeavesClientTimeoutInfinite() + { + using var client = new ManagementHttpClient("http://localhost:9001", "user", "pass"); + + Assert.Equal(Timeout.InfiniteTimeSpan, client.EffectiveTimeout); + } + + [Fact] + public void ConnectTimeout_DefaultsToThirtySeconds_WhenEnvVarUnset() { var original = Environment.GetEnvironmentVariable(EnvVar); try { Environment.SetEnvironmentVariable(EnvVar, null); - using var client = new ManagementHttpClient("http://localhost:9001", "user", "pass"); - - Assert.Equal(TimeSpan.FromSeconds(30), ManagementHttpClient.DefaultTimeout); - Assert.Equal(TimeSpan.FromSeconds(30), client.EffectiveTimeout); + Assert.Equal(TimeSpan.FromSeconds(30), ManagementHttpClient.DefaultConnectTimeout); + Assert.Equal(TimeSpan.FromSeconds(30), ManagementHttpClient.ResolveConnectTimeout()); } finally { @@ -144,16 +155,14 @@ public class ManagementHttpClientTimeoutTests [InlineData("-5")] [InlineData("not-a-number")] [InlineData("")] - public void InvalidOrNonPositiveEnvValue_FallsBackToDefault(string value) + public void InvalidOrNonPositiveEnvValue_FallsBackToDefaultConnectTimeout(string value) { var original = Environment.GetEnvironmentVariable(EnvVar); try { Environment.SetEnvironmentVariable(EnvVar, value); - using var client = new ManagementHttpClient("http://localhost:9001", "user", "pass"); - - Assert.Equal(TimeSpan.FromSeconds(30), client.EffectiveTimeout); + Assert.Equal(TimeSpan.FromSeconds(30), ManagementHttpClient.ResolveConnectTimeout()); } finally { @@ -162,20 +171,79 @@ public class ManagementHttpClientTimeoutTests } [Fact] - public void PositiveEnvValue_OverridesDefaultTimeout() + public void PositiveEnvValue_OverridesDefaultConnectTimeout() { var original = Environment.GetEnvironmentVariable(EnvVar); try { Environment.SetEnvironmentVariable(EnvVar, "5"); - using var client = new ManagementHttpClient("http://localhost:9001", "user", "pass"); - - Assert.Equal(TimeSpan.FromSeconds(5), client.EffectiveTimeout); + Assert.Equal(TimeSpan.FromSeconds(5), ManagementHttpClient.ResolveConnectTimeout()); } finally { Environment.SetEnvironmentVariable(EnvVar, original); } } + + /// + /// The regression that matters: a per-call timeout LONGER than the old 30 s + /// client cap must actually be honored. Two calls against the same hanging + /// local listener — one with a short deadline, one with a longer one — must + /// time out in that order and at their own deadlines, which is only possible + /// if is not silently capping both. Uses a + /// real socket (not the stub handler) so the connect + send path is exercised + /// end to end, and sub-second deadlines so the test stays fast. + /// + [Fact] + public async Task PerCallTimeoutLongerThanTheOldClientCap_IsHonored() + { + // A listener that accepts connections and then never answers: every + // request hangs until the caller's own deadline fires. + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + var port = ((IPEndPoint)listener.LocalEndpoint).Port; + var accepted = new List(); + var acceptLoop = Task.Run(async () => + { + try + { + while (true) + accepted.Add(await listener.AcceptTcpClientAsync()); + } + catch (ObjectDisposedException) { /* listener stopped — expected */ } + catch (SocketException) { /* listener stopped — expected */ } + }); + + try + { + using var client = new ManagementHttpClient($"http://127.0.0.1:{port}", "user", "pass"); + + var shortSw = Stopwatch.StartNew(); + var shortResponse = await client.SendCommandAsync("ListSites", new { }, TimeSpan.FromMilliseconds(300)); + shortSw.Stop(); + + var longSw = Stopwatch.StartNew(); + var longResponse = await client.SendCommandAsync("ListSites", new { }, TimeSpan.FromMilliseconds(1500)); + longSw.Stop(); + + Assert.Equal("TIMEOUT", shortResponse.ErrorCode); + Assert.Equal("TIMEOUT", longResponse.ErrorCode); + + // The longer deadline must genuinely outlast the shorter one rather + // than both being clipped to a single client-wide cap. + Assert.True( + longSw.Elapsed > TimeSpan.FromMilliseconds(1000), + $"1.5 s per-call timeout returned after only {longSw.ElapsedMilliseconds} ms — the client cap truncated it."); + Assert.True( + shortSw.Elapsed < TimeSpan.FromMilliseconds(1000), + $"300 ms per-call timeout took {shortSw.ElapsedMilliseconds} ms."); + } + finally + { + listener.Stop(); + foreach (var c in accepted) c.Dispose(); + await acceptLoop; + } + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/TemplateTableProjectionTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/TemplateTableProjectionTests.cs index 0a59e0a0..7a18e19d 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/TemplateTableProjectionTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/TemplateTableProjectionTests.cs @@ -1,5 +1,6 @@ using System.Text.Json; using ZB.MOM.WW.ScadaBridge.CLI.Commands; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Templates; namespace ZB.MOM.WW.ScadaBridge.CLI.Tests; @@ -7,6 +8,14 @@ namespace ZB.MOM.WW.ScadaBridge.CLI.Tests; /// Tests for the compact template list/get table projection (followup #6): /// the full per-template attribute/alarm/script dumps are collapsed to counts so table /// output is usable in a terminal, while the array/object shape is preserved. +/// +/// +/// Two server shapes must both project correctly: template get still returns a +/// full Template entity with child ARRAYS, while template list returns +/// database-projected rows carrying pre-computed COUNT +/// scalars and no arrays at all. Reading only the arrays made every listed template +/// render as zeros. +/// /// public class TemplateTableProjectionTests { @@ -92,6 +101,72 @@ public class TemplateTableProjectionTests Assert.False(root.TryGetProperty("attributes", out _)); } + /// + /// The template list shape. Serialised from the REAL + /// record rather than hand-written JSON, so a + /// rename of one of its count properties fails this test instead of silently + /// putting zeros back on every row. + /// + [Fact] + public void ProjectSummary_SummaryRows_ReadsCountScalars() + { + var rows = new[] + { + new TemplateSummary( + Id: 3, Name: "MESReceiver", Description: "base", ParentTemplateId: null, + FolderId: null, IsDerived: false, OwnerCompositionId: null, + AttributeCount: 3, AlarmCount: 1, ScriptCount: 2, + CompositionCount: 0, NativeAlarmSourceCount: 0), + new TemplateSummary( + Id: 5, Name: "LeftMESReceiver", Description: null, ParentTemplateId: 3, + FolderId: 2, IsDerived: false, OwnerCompositionId: null, + AttributeCount: 1, AlarmCount: 0, ScriptCount: 0, + CompositionCount: 1, NativeAlarmSourceCount: 1), + }; + var json = JsonSerializer.Serialize(rows, + new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }); + + var compact = TemplateTableProjection.ProjectSummary(json); + + using var doc = JsonDocument.Parse(compact); + var root = doc.RootElement; + Assert.Equal(2, root.GetArrayLength()); + + var first = root[0]; + Assert.Equal(3, first.GetProperty("id").GetInt32()); + Assert.Equal("MESReceiver", first.GetProperty("name").GetString()); + Assert.Equal(3, first.GetProperty("#attrs").GetInt32()); + Assert.Equal(1, first.GetProperty("#alarms").GetInt32()); + Assert.Equal(2, first.GetProperty("#scripts").GetInt32()); + Assert.Equal(0, first.GetProperty("#comps").GetInt32()); + Assert.Equal(0, first.GetProperty("#nativeAlarms").GetInt32()); + + var second = root[1]; + Assert.Equal(5, second.GetProperty("id").GetInt32()); + Assert.Equal(3, second.GetProperty("parentTemplateId").GetInt32()); + Assert.Equal(1, second.GetProperty("#attrs").GetInt32()); + Assert.Equal(1, second.GetProperty("#comps").GetInt32()); + Assert.Equal(1, second.GetProperty("#nativeAlarms").GetInt32()); + } + + /// + /// A count scalar wins over a child array when (hypothetically) both are + /// present, so a payload that gains summary fields never regresses to array + /// counting. + /// + [Fact] + public void ProjectSummary_PrefersCountScalarOverArray() + { + const string bothJson = """ + { "id": 1, "name": "T", "attributeCount": 42, "attributes": [ {"id":1} ] } + """; + + var compact = TemplateTableProjection.ProjectSummary(bothJson); + + using var doc = JsonDocument.Parse(compact); + Assert.Equal(42, doc.RootElement.GetProperty("#attrs").GetInt32()); + } + [Fact] public void ProjectSummary_NonJson_ReturnedVerbatim() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/RepositoryCoverageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/RepositoryCoverageTests.cs index f24ecdeb..6073b641 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/RepositoryCoverageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/RepositoryCoverageTests.cs @@ -7,6 +7,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; using ZB.MOM.WW.ScadaBridge.Commons.Types.Notifications; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Templates; using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase; using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories; using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Services; @@ -906,6 +907,71 @@ public class SiteRepositoryTests : IDisposable { Assert.Throws(() => new SiteRepository(null!)); } + + /// + /// A data connection's protocol and primary/backup configuration are flattening + /// inputs — FlatteningService packages them into the flattened config's + /// Connections map and RevisionHashService folds them into the + /// revision hash. Without a watermark bump on save, the process-wide + /// StaleInstanceProbe memo and the flatten-session caches keep serving the + /// pre-edit hash, so an instance whose deployed config genuinely drifted reads as + /// up to date. (The bump is unattributed — a connection has no owning template — + /// so both the global and structure counters must move.) + /// + [Fact] + public async Task SaveChanges_DataConnectionEdit_BumpsWatermark() + { + var watermark = new TemplateGraphWatermark(); + var repository = new SiteRepository(_context, watermark); + + var site = new Site("Site1", "S-001"); + await repository.AddSiteAsync(site); + await repository.SaveChangesAsync(); + + var afterSiteOnly = (watermark.Global, watermark.StructureVersion); + + var conn = new DataConnection("Conn1", "OpcUa", site.Id); + await repository.AddDataConnectionAsync(conn); + await repository.SaveChangesAsync(); + + Assert.True(watermark.Global > afterSiteOnly.Global, "adding a data connection did not bump the global watermark"); + Assert.True(watermark.StructureVersion > afterSiteOnly.StructureVersion, + "adding a data connection did not bump the structure watermark"); + + var afterAdd = (watermark.Global, watermark.StructureVersion); + + conn.Protocol = "MxGateway"; + await repository.UpdateDataConnectionAsync(conn); + await repository.SaveChangesAsync(); + + Assert.True(watermark.Global > afterAdd.Global, "editing a data connection did not bump the watermark"); + + var afterUpdate = watermark.Global; + + await repository.DeleteDataConnectionAsync(conn.Id); + await repository.SaveChangesAsync(); + + Assert.True(watermark.Global > afterUpdate, "deleting a data connection did not bump the watermark"); + } + + /// + /// The bump is scoped to the entity that actually feeds the flattener: a save + /// that touches no data connection must leave the watermark alone, or every + /// site/area edit would needlessly invalidate the whole flatten cache. + /// + [Fact] + public async Task SaveChanges_WithoutDataConnectionChange_DoesNotBumpWatermark() + { + var watermark = new TemplateGraphWatermark(); + var repository = new SiteRepository(_context, watermark); + + var site = new Site("Site1", "S-001"); + await repository.AddSiteAsync(site); + await repository.SaveChangesAsync(); + + Assert.Equal(0, watermark.Global); + Assert.Equal(0, watermark.StructureVersion); + } } public class DeploymentManagerRepositoryTests : IDisposable diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs index 26a33aed..0a07338b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploySiteAsyncTests.cs @@ -1,3 +1,5 @@ +using System.Collections.Concurrent; +using System.Diagnostics; using Akka.Actor; using Akka.TestKit.Xunit2; using Microsoft.Extensions.Logging.Abstractions; @@ -209,6 +211,124 @@ public class DeploySiteAsyncTests : TestKit Assert.Contains("not found", result.Error); } + /// + /// A PendingDeployment's fetch token expires + /// PendingDeploymentTtl after the row is STAGED, so any delay between + /// staging and sending is dead time burned off the token's life. Staging the + /// whole batch up front in phase 1 made that delay grow with batch size — the + /// tail instances' tokens could expire before their command was ever sent, and + /// the site's fetch then 404s. + /// + /// + /// This pins the fix: every instance's RefreshDeploymentCommand must + /// arrive at the site carrying a FRESH Timestamp (the staging instant), + /// regardless of how long the batch ahead of it took. Run serially with a slow + /// site so the batch takes far longer than the freshness bound being asserted — + /// under the old shape the last instance's token would already be ~2 s old. + /// + /// + [Fact] + public async Task DeploySiteAsync_StagesEachTokenImmediatelyBeforeItsOwnSend() + { + const int instanceCount = 20; + var perSendDelay = TimeSpan.FromMilliseconds(100); + ArrangeInstances(instanceCount); + + var ages = new ConcurrentBag(); + var commActor = Sys.ActorOf(Props.Create(() => new TokenAgeRecordingSiteActor(ages, perSendDelay))); + + // Parallelism 1 makes the batch strictly serial, so the accumulated lag a + // front-loaded staging phase would produce is at its largest. + var service = CreateService(commActor, maxParallelism: 1); + + var sw = Stopwatch.StartNew(); + var result = await service.DeploySiteAsync(SiteId, "admin"); + sw.Stop(); + + Assert.True(result.IsSuccess); + Assert.Equal(instanceCount, result.Value.SuccessCount); + Assert.Equal(instanceCount, ages.Count); + + // The batch really did take a long time — otherwise the freshness assertion + // below would pass vacuously. + Assert.True(sw.Elapsed > TimeSpan.FromMilliseconds(1500), + $"the batch completed in {sw.ElapsedMilliseconds} ms; too fast to prove token freshness"); + + // ...yet no token was stale when its command reached the site. + var oldest = ages.Max(); + Assert.True(oldest < TimeSpan.FromMilliseconds(500), + $"a fetch token was already {oldest.TotalMilliseconds:F0} ms old on arrival — " + + "staging is not tracking the send."); + } + + /// + /// Cancelling mid-batch must not leak operation locks. Phase 1's + /// ThrowIfCancellationRequested escapes DeploySiteAsync while every + /// already-prepared deployment still holds its per-instance lock — and + /// OperationLockManager hands out real semaphores, so an undisposed handle + /// wedges that instance against every future mutating command for the life of + /// the process. Their records must also be finalised as Failed rather than left + /// InProgress. + /// + [Fact] + public async Task DeploySiteAsync_CancelledMidPrepare_ReleasesEveryLock_AndFailsPreparedRecords() + { + ArrangeInstances(6); + + using var cts = new CancellationTokenSource(); + + // Cancel while preparing the third instance: the first two are fully + // prepared and holding locks when the loop's next cancellation check throws. + _pipeline + .FlattenAndValidateAsync(3, Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(_ => + { + cts.Cancel(); + var config = new FlattenedConfiguration { InstanceUniqueName = "Inst-03" }; + return Result.Success( + new FlatteningPipelineResult(config, "sha256:3", ValidationResult.Success())); + }); + + var commActor = Sys.ActorOf(Props.Create(() => + new ThrottledSiteActor(new ConcurrencyTracker(), slowInstanceName: null, slowDelay: TimeSpan.Zero))); + + var service = CreateService(commActor, maxParallelism: 2); + + await Assert.ThrowsAnyAsync( + () => service.DeploySiteAsync(SiteId, "admin", cts.Token)); + + Assert.Equal(0, _lockManager.TrackedLockCount); + + // No prepared deployment may be left InProgress. + await _repo.Received().UpdateDeploymentRecordAsync( + Arg.Is(r => r.Status == DeploymentStatus.Failed), + Arg.Any()); + } + + /// + /// Answers every RefreshDeploymentCommand after a fixed delay, recording + /// how old each command's staging Timestamp already was on arrival. + /// + private sealed class TokenAgeRecordingSiteActor : ReceiveActor + { + public TokenAgeRecordingSiteActor(ConcurrentBag ages, TimeSpan delay) + { + Receive(env => + { + if (env.Message is not RefreshDeploymentCommand cmd) + return; + + ages.Add(DateTimeOffset.UtcNow - cmd.Timestamp); + + var replyTo = Sender; + Context.System.Scheduler.Advanced.ScheduleOnce(delay, () => + replyTo.Tell(new DeploymentStatusResponse( + cmd.DeploymentId, cmd.InstanceUniqueName, + DeploymentStatus.Success, null, DateTimeOffset.UtcNow))); + }); + } + } + /// Records the peak number of simultaneously in-flight site round-trips. private sealed class ConcurrencyTracker { diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs new file mode 100644 index 00000000..fed5a45c --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/StaleInstanceProbeTests.cs @@ -0,0 +1,99 @@ +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Types; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Templates; +using ZB.MOM.WW.ScadaBridge.TemplateEngine.Flattening; + +namespace ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests; + +/// +/// The staleness fast path: memoises a computed +/// revision hash against the readings it was +/// computed under, and the memo is PROCESS-STATIC. That makes the watermark the +/// only thing standing between a cached hash and a wrong answer — so these tests +/// pin that the memo is served only while the watermark has not moved, and is +/// dropped the moment it has. +/// +/// +/// The failure this guards against is a real one: a data-connection edit, or a +/// bundle import committing through the raw DbContext, changes the flattened +/// output without going through the repository path that derives bumps from the +/// change tracker. If that write does not bump, the memo below keeps answering with +/// the old hash forever. +/// +/// +public class StaleInstanceProbeTests +{ + private const int InstanceId = 1; + + private readonly TemplateGraphWatermark _watermark = new(); + private readonly IFlatteningPipeline _pipeline = Substitute.For(); + private string _currentHash = "sha256:first"; + private int _flattenCount; + + public StaleInstanceProbeTests() + { + // Process-static memo: start every test from a clean slate. + StaleInstanceProbe.ClearMemos(); + + _pipeline.CreateSession().Returns(_ => new FlattenSession(_watermark)); + _pipeline + .FlattenAndValidateAsync(InstanceId, Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(_ => + { + _flattenCount++; + var config = new FlattenedConfiguration { InstanceUniqueName = "Inst-01" }; + return Result.Success( + new FlatteningPipelineResult(config, _currentHash, ValidationResult.Success())); + }); + } + + private StaleInstanceProbe CreateProbe() => new(_pipeline, _watermark); + + [Fact] + public async Task UnchangedWatermark_ServesMemoisedHash_WithoutReflattening() + { + var probe = CreateProbe(); + + Assert.Equal("sha256:first", await probe.GetCurrentRevisionHashAsync(InstanceId)); + Assert.Equal(1, _flattenCount); + + Assert.Equal("sha256:first", await probe.GetCurrentRevisionHashAsync(InstanceId)); + Assert.Equal(1, _flattenCount); + } + + /// + /// BumpAll is the unattributed fallback used by everything that changes a + /// flattening input without an owning template id — data-connection saves and + /// bundle imports both rely on it. It must invalidate the memo. + /// + [Fact] + public async Task BumpAll_InvalidatesTheMemo() + { + var probe = CreateProbe(); + await probe.GetCurrentRevisionHashAsync(InstanceId); + + // The underlying config drifted (e.g. a data connection was repointed). + _currentHash = "sha256:second"; + + // Without a bump the stale hash would still be served... + Assert.Equal("sha256:first", await probe.GetCurrentRevisionHashAsync(InstanceId)); + + _watermark.BumpAll(); + + Assert.Equal("sha256:second", await probe.GetCurrentRevisionHashAsync(InstanceId)); + Assert.Equal(2, _flattenCount); + } + + [Fact] + public async Task BumpInstance_InvalidatesTheMemoForThatInstance() + { + var probe = CreateProbe(); + await probe.GetCurrentRevisionHashAsync(InstanceId); + + _currentHash = "sha256:second"; + _watermark.BumpInstance(InstanceId); + + Assert.Equal("sha256:second", await probe.GetCurrentRevisionHashAsync(InstanceId)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompileVerdictCacheEvictionTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompileVerdictCacheEvictionTests.cs index aa46cb6e..ea021251 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompileVerdictCacheEvictionTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompileVerdictCacheEvictionTests.cs @@ -105,6 +105,36 @@ public class ScriptCompileVerdictCacheEvictionTests $"cache grew to {ScriptCompileVerdictCache.Count} entries, exceeding its two-generation bound"); } + /// + /// The bound must hold under a PROMOTION-heavy workload too, not just a + /// pure-insert one. Promotion used to write straight into hot + /// (_hot[key] = verdict) with no capacity check, so re-reading a working + /// set bigger than one segment pulled the whole cold generation back into hot + /// on top of what hot already held — hot alone reached 2 × SegmentCapacity and + /// the total 3 ×, against a documented 2 ×. Pure-insert overflow never hits + /// that path because every key is distinct. + /// + [Fact] + public void PromotionOverflow_KeepsCacheBounded() + { + ScriptCompileVerdictCache.Clear(); + + // Fill past one rotation so a large working set is sitting in cold with + // hot already partly full. + const int WorkingSet = 3000; + for (var i = 0; i < WorkingSet; i++) + Lookup($"// promo {i}", static () => (true, null)); + + Assert.True(ScriptCompileVerdictCache.Evictions > 0); + + // Re-read the whole working set: every entry still in cold promotes. + for (var i = 0; i < WorkingSet; i++) + Lookup($"// promo {i}", static () => (true, null)); + + Assert.True(ScriptCompileVerdictCache.Count <= 4096, + $"cache grew to {ScriptCompileVerdictCache.Count} entries under promotion, exceeding its two-generation bound"); + } + [Fact] public void SurfaceIsPartOfTheKey_AcrossEviction() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs index d9adecf0..66a0a35b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs @@ -1335,6 +1335,122 @@ public sealed class BundleImporterApplyTests : IDisposable Assert.DoesNotContain(notDeployedInstanceId, result.StaleInstanceIds); } + /// + /// Two consecutive imports overwriting the SAME template must both report the + /// deployed instance as stale. + /// + /// + /// StaleInstanceProbe memoises its computed revision hash against + /// ITemplateGraphWatermark readings, in a PROCESS-STATIC dictionary. The + /// importer commits through the raw ScadaBridgeDbContext, bypassing + /// TemplateEngineRepository.SaveChangesAsync — the only place that + /// otherwise derives watermark bumps from the change tracker — so before the + /// fix nothing an import wrote ever moved the watermark. The second import then + /// re-served the FIRST import's hash and the instance silently dropped out of + /// ImportResult.StaleInstanceIds: the operator is told nothing needs + /// redeploying while the site runs a config that has drifted twice. + /// + /// + /// + /// The snapshot is refreshed between the two imports (simulating the redeploy + /// the first stale report prompts), so the second import's staleness verdict + /// depends entirely on a FRESH hash rather than on the leftover drift. + /// + /// + [Fact] + public async Task ApplyAsync_second_overwrite_of_same_template_still_marks_instance_stale() + { + // The memo is process-static and keyed by instance id alone; start clean so + // this test does not inherit another class's entry. + StaleInstanceProbe.ClearMemos(); + + await SeedSiteAsync(); + + // Shape A → bundle A. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var t = new Template("Pump") { Description = "shape-a" }; + t.Attributes.Add(new TemplateAttribute("Flow") { DataType = DataType.Float, Value = "1.0" }); + ctx.Templates.Add(t); + await ctx.SaveChangesAsync(); + } + var sessionA = await ExportAndLoadAsync(); + + // Shape B → bundle B (a DIFFERENT flattened output from shape A). + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var t = await ctx.Templates.Include(x => x.Attributes).SingleAsync(x => x.Name == "Pump"); + t.Description = "shape-b"; + t.Attributes.Single(a => a.Name == "Flow").Value = "2.0"; + await ctx.SaveChangesAsync(); + } + var sessionB = await ExportAndLoadAsync(); + + // Target starts on a third shape, and the deployed snapshot captures it. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var t = await ctx.Templates.Include(x => x.Attributes).SingleAsync(x => x.Name == "Pump"); + t.Description = "shape-target"; + t.Attributes.Single(a => a.Name == "Flow").Value = "0.0"; + await ctx.SaveChangesAsync(); + } + var instanceId = await SeedDeployedInstanceWithRealSnapshotAsync("Pump", "Pump-Deployed"); + + // Import #1 — Overwrite to shape A. Drifts the instance off its snapshot. + ImportResult first; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + first = await importer.ApplyAsync(sessionA, + new List { new("Template", "Pump", ResolutionAction.Overwrite, null) }, + user: "bob"); + } + Assert.Contains(instanceId, first.StaleInstanceIds); + + // The operator redeploys: the snapshot now matches the post-import-#1 config, + // so the instance is genuinely up to date going into import #2. + await RefreshDeployedSnapshotAsync(instanceId); + + // Import #2 — Overwrite the SAME template to shape B. The instance drifts + // again and must be reported again. + ImportResult second; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + second = await importer.ApplyAsync(sessionB, + new List { new("Template", "Pump", ResolutionAction.Overwrite, null) }, + user: "bob"); + } + + Assert.Equal(1, second.Overwritten); + Assert.Contains(instanceId, second.StaleInstanceIds); + } + + /// + /// Re-points the instance's at its CURRENT + /// flattened revision hash — the state a successful redeploy would leave behind. + /// + /// Instance whose snapshot is refreshed. + /// A task that completes once the snapshot is updated. + private async Task RefreshDeployedSnapshotAsync(int instanceId) + { + await using var scope = _provider.CreateAsyncScope(); + var ctx = scope.ServiceProvider.GetRequiredService(); + var pipeline = scope.ServiceProvider.GetRequiredService(); + + var flattened = await pipeline.FlattenAndValidateAsync(instanceId); + Assert.True(flattened.IsSuccess, + $"Snapshot refresh flatten failed: {(flattened.IsFailure ? flattened.Error : "(success)")}"); + + var snapshot = await ctx.DeployedConfigSnapshots.SingleAsync(s => s.InstanceId == instanceId); + snapshot.RevisionHash = flattened.Value.RevisionHash; + snapshot.ConfigurationJson = System.Text.Json.JsonSerializer.Serialize(flattened.Value.Configuration); + await ctx.SaveChangesAsync(); + } + // ============ #05-T14: post-commit ScriptArtifactsChanged publish ============ [Fact]