fix(deploy+cli): review findings — honest CLI timeouts, watermark-complete staleness, phase-2 staging, lock-safe cancellation

Six adversarial-review findings, each verified against the code first. F1 (HIGH) CLI HttpClient capped every call at min(30s, caller timeout), silently truncating deploy site's 5-minute BulkDeployTimeout and the 5-minute bundle export/preview/import calls — which printed a fake "504 Request timed out" while the server kept working. HttpClient.Timeout is now Timeout.InfiniteTimeSpan (the per-call CTS is the single overall deadline, connect included) with the connect phase bounded separately on SocketsHttpHandler.ConnectTimeout. The env override is renamed to SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS to match its new meaning. F2 (HIGH) StaleInstanceProbe's process-static memo served stale hashes because nothing bumped the watermark on three paths: (a) BundleImporter commits through the raw DbContext, so no import ever moved the watermark — a second import overwriting the same template could be OMITTED from ImportResult.StaleInstanceIds. It now bumps once per apply ATTEMPT: after the commit, and after the rollback too (the probe runs pre-commit, so a rolled-back attempt leaves memos for state that never landed; bumping on both paths is the simplest correct shape, versus threading transaction awareness through a process-static cache). (b) CollectWatermarkBumps' default: arm silently no-op'd, contradicting its own doc. It now sets unattributed=true — an over-broad bump costs extra work, a missed one produces stale work. (c) DataConnection edits route through SiteRepository.SaveChangesAsync, which had no watermark at all, yet Protocol/Primary+Backup config/ FailoverRetryCount are revision-hash inputs. It now bumps (BumpAll — a connection has no owning template) after a commit that touched one. F3 (MED) CLI TemplateTableProjection read child ARRAYS, but ListTemplates now returns database-projected TemplateSummary rows, so template list printed all zeros. It now prefers the *Count scalars and falls back to array length (template get still returns full entities). --detail help text and README corrected: a listing cannot yield definitions, so --detail renders the raw summary payload and template get --id is the full dump. F4 (MED) DeploySiteAsync staged every PendingDeployment in phase 1 against a 5-min TTL while phase 2 reached them one batch at a time, so tail instances' fetch tokens could expire before their command was sent. Staging moved into phase 2, immediately before each send; prepare keeps its flatten/validate/record work. The staging write is the phase's only repository touch and is serialised behind a 1-permit semaphore, so the non-thread-safe DbContext constraint holds and the sends stay concurrent. F5 (MED, latent) DeploySiteAsync leaked every held operation lock if cancelled — a wedged per-instance semaphore is permanent for the process. Phase 2 no longer throws (cancellation is recorded as a per-instance outcome so phase 3 still runs), and an escape from phase 1 or 3 now unwinds every unfinalised entry: Failed status + lock release. F6 (LOW) ScriptCompileVerdictCache's promotion wrote hot directly, bypassing SegmentCapacity (true ceiling 3x against a documented 2x). Promotion now goes through Store, keeping generational semantics; _hot and _cold are volatile. Tests: CLI 396, DeploymentManager 133, ManagementService 494, TemplateEngine 478, ScriptAnalysis 60, Transport 157, Transport integration 106, ConfigurationDatabase 366 — all green, 0 build warnings. The F2/F4/F5 regression tests were each confirmed to FAIL with their fix reverted.
2026-08-14 23:51:04 -04:00
parent b1de9dfdd4
commit e0e4b24679
16 changed files with 1172 additions and 174 deletions
@@ -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;

    /// <summary>
    /// Initializes a new instance of the SiteRepository.
    /// </summary>
    /// <param name="dbContext">The database context.</param>
-    public SiteRepository(ScadaBridgeDbContext dbContext)
+    /// <param name="watermark">
+    /// Graph version watermark bumped when a committed change alters a flattening
+    /// input this repository owns (see <see cref="SaveChangesAsync"/>). 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.
+    /// </param>
+    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
    }

    /// <inheritdoc />
+    /// <remarks>
+    /// Bumps the <see cref="ITemplateGraphWatermark"/> after a successful commit
+    /// that touched a <see cref="DataConnection"/>.
+    ///
+    /// <para>
+    /// A data connection's <c>Protocol</c>, primary/backup configuration and
+    /// failover retry count are flattening inputs — <c>FlatteningService</c>
+    /// packages them into the flattened config's <c>Connections</c> map and
+    /// <c>RevisionHashService</c> folds them into the revision hash. Without a bump,
+    /// the process-wide <c>StaleInstanceProbe</c> 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.
+    /// </para>
+    ///
+    /// <para>
+    /// The bump is <see cref="ITemplateGraphWatermark.BumpAll"/> 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.
+    /// </para>
+    /// </remarks>
    public async Task<int> 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;
+    }
+
+    /// <summary>
+    /// True when the change tracker holds an added, modified or deleted
+    /// <see cref="DataConnection"/>.
+    /// </summary>
+    /// <returns>Whether a data-connection mutation is about to be committed.</returns>
+    private bool HasPendingDataConnectionChange()
+    {
+        foreach (var entry in _dbContext.ChangeTracker.Entries<DataConnection>())
+        {
+            if (entry.State is EntityState.Added or EntityState.Modified or EntityState.Deleted)
+                return true;
+        }
+
+        return false;
    }
 }
@@ -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;
            }
        }