From da5a049077f5c798abeff1d709cf1a87998338c5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:18:32 -0400 Subject: [PATCH] fix(deployment): pending-deployment supersede/purge/stage deletes never read ConfigurationJson; drop stale deferred-TODO comment (review 2026-10-21 08/P6, 08/C2) --- .../DeploymentManagerRepository.cs | 97 ++++++++++++++----- .../PendingDeploymentRepositoryTests.cs | 74 +++++++++++++- .../SqlReaderCommandRecorder.cs | 46 +++++++++ 3 files changed, 192 insertions(+), 25 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SqlReaderCommandRecorder.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs index 1877510d..84efacc4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs @@ -339,12 +339,19 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository // Supersession: a newer deploy for the same instance replaces any prior pending // row. Safe because the per-instance operation lock serializes same-instance // deploys. SaveChanges is deferred to the caller, matching the other Add methods. - var prior = await _dbContext.Set() + // + // Only the prior rows' KEYS are read: each row carries the whole flattened + // ConfigurationJson (tens of KB), and materialising it just to delete it was the + // dominant cost of the supersede. The delete itself stays a tracked Remove (not + // ExecuteDelete) so it still commits atomically with the new row in the caller's + // SaveChanges. + var priorIds = await _dbContext.Set() .Where(p => p.InstanceId == pending.InstanceId) + .Select(p => p.Id) .ToListAsync(cancellationToken); - if (prior.Count > 0) + foreach (var id in priorIds) { - _dbContext.Set().RemoveRange(prior); + RemovePendingByKey(id); } await _dbContext.Set().AddAsync(pending, cancellationToken); } @@ -395,16 +402,61 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository public async Task PurgeExpiredPendingDeploymentsAsync(DateTimeOffset nowUtc, CancellationToken cancellationToken = default) { // Self-contained TTL maintenance op: commits its own delete and returns the count. - var expired = await _dbContext.Set() - .Where(p => p.ExpiresAtUtc <= nowUtc) - .ToListAsync(cancellationToken); - if (expired.Count == 0) + // A set-based DELETE — the expired rows' ConfigurationJson is never read. + return await DeletePendingWhereAsync(p => p.ExpiresAtUtc <= nowUtc, cancellationToken); + } + + /// + /// Deletes every pending row matching immediately, without + /// materialising the rows (each carries its full flattened ConfigurationJson). + /// A relational provider issues one set-based DELETE; a non-relational provider + /// (the EF in-memory provider some host-level tests run on) cannot translate + /// ExecuteDelete, so it falls back to load-and-remove with its own save. + /// + /// Rows to delete. + /// Cancellation token. + /// The number of rows deleted. + private async Task DeletePendingWhereAsync( + System.Linq.Expressions.Expression> predicate, + CancellationToken cancellationToken) + { + var query = _dbContext.Set().Where(predicate); + if (_dbContext.Database.IsRelational()) + { + return await query.ExecuteDeleteAsync(cancellationToken); + } + + var rows = await query.ToListAsync(cancellationToken); + if (rows.Count == 0) { return 0; } - _dbContext.Set().RemoveRange(expired); + _dbContext.Set().RemoveRange(rows); await _dbContext.SaveChangesAsync(cancellationToken); - return expired.Count; + return rows.Count; + } + + /// + /// Marks the pending row with primary key for deletion on the next + /// SaveChanges without loading it: an already-tracked instance is removed in place, + /// otherwise a key-only stub is attached and removed (EF deletes by key). + /// + /// Primary key of the pending row. + private void RemovePendingByKey(int id) + { + var set = _dbContext.Set(); + var tracked = set.Local.FirstOrDefault(p => p.Id == id); + if (tracked != null) + { + set.Remove(tracked); + return; + } + + var stub = new PendingDeployment( + string.Empty, 0, string.Empty, string.Empty, string.Empty, + default, default) { Id = id }; + set.Attach(stub); + set.Remove(stub); } /// @@ -467,18 +519,15 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository CancellationToken cancellationToken = default) { // Treat createdAtUtc as "now". FIRST remove any EXPIRED pending rows for this instance - // (ExpiresAtUtc <= now). Pending rows are only TTL-purged (the periodic purge is still a - // deferred TODO), so an EXPIRED-but-unpurged row would otherwise (a) read as "a deploy is - // in flight" and block staging — handing the node an expired token (HTTP 404) and leaving - // it unhealed — and (b) collide on the DeploymentId UNIQUE index when a reconcile re-stages - // the snapshot's own DeploymentId. Dropping expired rows first fixes both. - var expired = await _dbContext.Set() - .Where(p => p.InstanceId == instanceId && p.ExpiresAtUtc <= createdAtUtc) - .ToListAsync(cancellationToken); - if (expired.Count > 0) - { - _dbContext.Set().RemoveRange(expired); - } + // (ExpiresAtUtc <= now). Pending rows are only TTL-purged — PendingDeploymentPurgeActor + // sweeps them on its PendingDeploymentPurgeInterval cadence — so between sweeps an + // EXPIRED-but-unpurged row would otherwise (a) read as "a deploy is in flight" and block + // staging — handing the node an expired token (HTTP 404) and leaving it unhealed — and + // (b) collide on the DeploymentId UNIQUE index when a reconcile re-stages the snapshot's + // own DeploymentId. Dropping expired rows first fixes both. The delete is set-based and + // immediate (an expired row is dead either way), so its JSON is never read. + await DeletePendingWhereAsync( + p => p.InstanceId == instanceId && p.ExpiresAtUtc <= createdAtUtc, cancellationToken); // THEN insert-if-absent against still-LIVE rows only. A live pending row means a genuine // in-flight deploy (or a concurrent reconcile) already owns the slot — do NOT supersede it; @@ -494,9 +543,9 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository await _dbContext.Set().AddAsync(pending, cancellationToken); } - // Self-contained: one SaveChanges flushes the expired-row cleanup and, when staged, the new - // row together (EF orders the delete before the same-DeploymentId insert to satisfy the - // unique index). Returns true only when a fresh row was staged. + // Self-contained: the expired-row cleanup has already run, so the same-DeploymentId insert + // below cannot collide with it on the unique index. Returns true only when a fresh row was + // staged. await _dbContext.SaveChangesAsync(cancellationToken); return !liveExists; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/PendingDeploymentRepositoryTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/PendingDeploymentRepositoryTests.cs index d6765140..e97d8a02 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/PendingDeploymentRepositoryTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/PendingDeploymentRepositoryTests.cs @@ -18,11 +18,12 @@ public class PendingDeploymentRepositoryTests : IDisposable { private readonly ScadaBridgeDbContext _context; private readonly DeploymentManagerRepository _repository; + private readonly SqlReaderCommandRecorder _recorder = new(); /// Initializes the test class with an in-memory context and repository. public PendingDeploymentRepositoryTests() { - _context = SqliteTestHelper.CreateInMemoryContext(); + _context = SqliteTestHelper.CreateInMemoryContext(_recorder); _repository = new DeploymentManagerRepository(_context); } @@ -344,4 +345,75 @@ public class PendingDeploymentRepositoryTests : IDisposable Assert.Null(await _repository.GetPendingDeploymentByIdAsync("stale-b")); Assert.NotNull(await _repository.GetPendingDeploymentByIdAsync("dep3")); } + + // ── review 2026-10-21 08/P6: pending deletes never materialise ConfigurationJson ── + + /// True when the change tracker holds a pending row whose JSON is . + private bool TracksPendingJson(string json) => + _context.ChangeTracker.Entries().Any(e => e.Entity.ConfigurationJson == json); + + /// + /// Supersession must delete the prior row by key, without loading its (tens-of-KB) + /// flattened config into the change tracker. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task AddPendingDeployment_Supersede_DoesNotMaterialisePriorConfigJson() + { + var instanceId = await SeedInstanceAsync("InstP6a"); + await _repository.AddPendingDeploymentAsync(NewPending("old", instanceId, "{\"prior\":true}")); + await _repository.SaveChangesAsync(); + _context.ChangeTracker.Clear(); + _recorder.Reset(); + + await _repository.AddPendingDeploymentAsync(NewPending("new", instanceId, "{\"v\":2}")); + + Assert.False(TracksPendingJson("{\"prior\":true}")); + Assert.False(_recorder.AnySelectReads("ConfigurationJson")); + await _repository.SaveChangesAsync(); + Assert.Null(await _repository.GetPendingDeploymentByIdAsync("old")); + Assert.NotNull(await _repository.GetPendingDeploymentByIdAsync("new")); + } + + /// The TTL purge is a set-based delete: nothing is loaded into the tracker. + /// A task that represents the asynchronous operation. + [Fact] + public async Task PurgeExpiredPendingDeployments_DoesNotMaterialiseRows() + { + var instanceId = await SeedInstanceAsync("InstP6b"); + var now = DateTimeOffset.UtcNow; + await _repository.AddPendingDeploymentAsync(new PendingDeployment( + "exp", instanceId, "rev", "{\"expired\":true}", "tok", now.AddMinutes(-20), now.AddMinutes(-10))); + await _repository.SaveChangesAsync(); + _context.ChangeTracker.Clear(); + _recorder.Reset(); + + var purged = await _repository.PurgeExpiredPendingDeploymentsAsync(now); + + Assert.Equal(1, purged); + Assert.False(_recorder.AnySelectReads("ConfigurationJson")); + Assert.Null(await _repository.GetPendingDeploymentByIdAsync("exp")); + } + + /// StagePendingIfAbsent drops expired rows without reading their config. + /// A task that represents the asynchronous operation. + [Fact] + public async Task StagePendingIfAbsent_ExpiredCleanup_DoesNotMaterialiseExpiredConfigJson() + { + var instanceId = await SeedInstanceAsync("InstP6c"); + var now = DateTimeOffset.UtcNow; + await _repository.AddPendingDeploymentAsync(new PendingDeployment( + "dep-x", instanceId, "rev", "{\"expired\":true}", "tok", now.AddMinutes(-20), now.AddMinutes(-10))); + await _repository.SaveChangesAsync(); + _context.ChangeTracker.Clear(); + _recorder.Reset(); + + var staged = await _repository.StagePendingIfAbsentAsync( + instanceId, "dep-x", "rev2", "{\"fresh\":true}", "tok2", now, now.AddMinutes(5)); + + Assert.True(staged); + Assert.False(_recorder.AnySelectReads("ConfigurationJson")); + var row = await _repository.GetPendingDeploymentByIdAsync("dep-x"); + Assert.Equal("{\"fresh\":true}", row!.ConfigurationJson); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SqlReaderCommandRecorder.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SqlReaderCommandRecorder.cs new file mode 100644 index 00000000..576091a2 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SqlReaderCommandRecorder.cs @@ -0,0 +1,46 @@ +using System.Data.Common; +using Microsoft.EntityFrameworkCore.Diagnostics; + +namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests; + +/// +/// Records the text of every reader command a context executes, so a query-shape test can +/// assert what a repository method actually reads (e.g. that a delete never SELECTs a fat +/// JSON column). +/// +public sealed class SqlReaderCommandRecorder : DbCommandInterceptor +{ + private readonly List _commands = new(); + + /// The text of every reader command recorded since the last . + public IReadOnlyList Commands => _commands; + + /// Clears all recorded commands. + public void Reset() => _commands.Clear(); + + /// + /// True when any recorded reader command is a SELECT that projects . + /// + /// Column name to look for. + /// Whether a recorded SELECT read the column. + public bool AnySelectReads(string column) => + _commands.Any(c => c.TrimStart().StartsWith("SELECT", StringComparison.OrdinalIgnoreCase) + && c.Contains(column, StringComparison.Ordinal)); + + /// + public override InterceptionResult ReaderExecuting( + DbCommand command, CommandEventData eventData, InterceptionResult result) + { + _commands.Add(command.CommandText); + return base.ReaderExecuting(command, eventData, result); + } + + /// + public override ValueTask> ReaderExecutingAsync( + DbCommand command, CommandEventData eventData, InterceptionResult result, + CancellationToken cancellationToken = default) + { + _commands.Add(command.CommandText); + return base.ReaderExecutingAsync(command, eventData, result, cancellationToken); + } +}