From d6a9a79c277d33c90f972eae4dd7070df039a26f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:20:44 -0400 Subject: [PATCH] fix(deployment): purge SystemArtifactDeploymentRecords in the terminal-record retention sweep (review 2026-10-21 08/G2) --- .../Component-DeploymentManager.md | 5 ++ .../IDeploymentManagerRepository.cs | 11 ++++ .../DeploymentManagerRepository.cs | 23 ++++++++ .../DeploymentManagerOptions.cs | 2 + .../DeploymentService.cs | 23 ++++++-- .../SystemArtifactDeploymentPurgeTests.cs | 52 +++++++++++++++++++ .../DeploymentServiceTests.cs | 26 ++++++++++ 7 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SystemArtifactDeploymentPurgeTests.cs diff --git a/docs/requirements/Component-DeploymentManager.md b/docs/requirements/Component-DeploymentManager.md index 2d919c98..8f559bc5 100644 --- a/docs/requirements/Component-DeploymentManager.md +++ b/docs/requirements/Component-DeploymentManager.md @@ -221,6 +221,11 @@ never purged regardless of age: it is precisely the row the query-before-redeplo reconciliation reads to decide whether a prior deploy actually landed at the site, and expiring it by age would silently disable that idempotency guard. +The same window also bounds **`SystemArtifactDeploymentRecords`** — one row per +*Deploy Artifacts* run, written after the sweep and never read back — which the +sweep deletes by `DeployedAt` in the same bounded batches. Before 2026-10 that table +had no purge path at all. + ## Diff View Before deploying, the Deployment Manager can request a diff from the Template Engine showing: diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs index 09551385..51f67081 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs @@ -194,6 +194,17 @@ public interface IDeploymentManagerRepository /// A cancellation token that can be used to cancel the operation. /// A task representing the asynchronous operation. Task DeleteSystemArtifactDeploymentAsync(int id, CancellationToken cancellationToken = default); + /// + /// Deletes system artifact deployment records deployed before , + /// in one bounded batch. One row is written per Deploy Artifacts run and nothing + /// reads them back, so they share the terminal deployment-record retention window + /// instead of growing without bound. + /// + /// Records deployed before this instant are eligible. + /// Maximum rows deleted in this call. + /// A cancellation token that can be used to cancel the operation. + /// The number of rows deleted; fewer than means the sweep is complete. + Task PurgeSystemArtifactDeploymentRecordsAsync(DateTimeOffset cutoffUtc, int batchSize, CancellationToken cancellationToken = default); // DeployedConfigSnapshot /// diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs index 84efacc4..ef99c45a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs @@ -285,6 +285,29 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository return Task.CompletedTask; } + /// + public async Task PurgeSystemArtifactDeploymentRecordsAsync( + DateTimeOffset cutoffUtc, + int batchSize, + CancellationToken cancellationToken = default) + { + // Same bounded-batch shape as PurgeTerminalDeploymentRecordsAsync: the caller + // re-invokes until fewer than batchSize rows come back. Every row is terminal by + // construction (it is written once, after the artifact sweep finished). + var stale = await _dbContext.SystemArtifactDeploymentRecords + .Where(d => d.DeployedAt < cutoffUtc) + .OrderBy(d => d.Id) + .Take(batchSize) + .ToListAsync(cancellationToken); + + if (stale.Count == 0) + return 0; + + _dbContext.SystemArtifactDeploymentRecords.RemoveRange(stale); + await _dbContext.SaveChangesAsync(cancellationToken); + return stale.Count; + } + // --- DeployedConfigSnapshot --- /// diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptions.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptions.cs index a75b6e77..a875cab3 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentManagerOptions.cs @@ -72,6 +72,8 @@ public class DeploymentManagerOptions /// InProgress or Pending row is never purged regardless of age, /// because it is exactly the row the query-before-redeploy reconciliation /// path needs to find. Set to to disable purging. + /// The same window bounds system artifact deployment records (one per + /// Deploy Artifacts run), purged by their deploy time. /// /// public TimeSpan TerminalDeploymentRecordRetention { get; set; } = TimeSpan.FromDays(365); diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs index 01908aab..76c81cef 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs @@ -1330,7 +1330,8 @@ public class DeploymentService instanceId, uniqueName, deploymentId, false, result.Error); /// - /// Opportunistic retention sweep for TERMINAL deployment records, rate-limited + /// Opportunistic retention sweep for TERMINAL deployment records and system + /// artifact deployment records, rate-limited /// to once every per process and bounded /// to batches per call. /// @@ -1379,12 +1380,26 @@ public class DeploymentService break; } + // System artifact deployment records (one per Deploy Artifacts run) share the + // same retention window — nothing reads them back, so without this sweep the + // table only ever grows. + var artifactPurged = 0; + for (var batch = 0; batch < TerminalPurgeBatchesPerSweep; batch++) + { + var deleted = await _repository.PurgeSystemArtifactDeploymentRecordsAsync( + cutoff, TerminalPurgeBatchSize, cancellationToken); + artifactPurged += deleted; + if (deleted < TerminalPurgeBatchSize) + break; + } + + purged += artifactPurged; if (purged > 0) { _logger.LogInformation( - "Purged {Count} terminal deployment record(s) completed before {Cutoff} " + - "(retention {Retention}).", - purged, cutoff, retention); + "Purged {Count} terminal deployment record(s) ({ArtifactCount} system artifact " + + "record(s)) completed before {Cutoff} (retention {Retention}).", + purged, artifactPurged, cutoff, retention); } } catch (Exception ex) diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SystemArtifactDeploymentPurgeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SystemArtifactDeploymentPurgeTests.cs new file mode 100644 index 00000000..7f91a83f --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SystemArtifactDeploymentPurgeTests.cs @@ -0,0 +1,52 @@ +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Deployment; +using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories; + +namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests; + +/// +/// Retention purge for rows (review 2026-10-21 +/// 08/G2): one row is written per Deploy Artifacts run and nothing reads them back, so they +/// must share the terminal deployment-record retention window. +/// +public sealed class SystemArtifactDeploymentPurgeTests : IDisposable +{ + private readonly ScadaBridgeDbContext _context = SqliteTestHelper.CreateInMemoryContext(); + private readonly DeploymentManagerRepository _repository; + + /// Initializes the repository over a fresh in-memory SQLite context. + public SystemArtifactDeploymentPurgeTests() + { + _repository = new DeploymentManagerRepository(_context); + } + + /// Releases the in-memory database. + public void Dispose() => _context.Dispose(); + + private async Task SeedAsync(DateTimeOffset deployedAt) + { + await _repository.AddSystemArtifactDeploymentAsync( + new SystemArtifactDeploymentRecord("Artifacts", "admin") { DeployedAt = deployedAt }); + await _repository.SaveChangesAsync(); + } + + /// Only rows older than the cutoff are deleted, in bounded batches. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Purge_DeletesOnlyRowsOlderThanCutoff_InBoundedBatches() + { + var now = DateTimeOffset.UtcNow; + await SeedAsync(now.AddDays(-400)); + await SeedAsync(now.AddDays(-380)); + await SeedAsync(now.AddDays(-370)); + await SeedAsync(now.AddDays(-1)); + var cutoff = now.AddDays(-365); + + Assert.Equal(2, await _repository.PurgeSystemArtifactDeploymentRecordsAsync(cutoff, batchSize: 2)); + Assert.Equal(1, await _repository.PurgeSystemArtifactDeploymentRecordsAsync(cutoff, batchSize: 2)); + Assert.Equal(0, await _repository.PurgeSystemArtifactDeploymentRecordsAsync(cutoff, batchSize: 2)); + + var remaining = await _repository.GetAllSystemArtifactDeploymentsAsync(); + var survivor = Assert.Single(remaining); + Assert.True(survivor.DeployedAt > cutoff); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs index 2d956bf3..617f46df 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs @@ -87,6 +87,32 @@ public class DeploymentServiceTests : TestKit PendingDeploymentTtl = TestPendingTtl }); + // ── Terminal-record retention (review 2026-10-21 08/G2) ── + + /// + /// The retention sweep also purges system artifact deployment records — one per Deploy + /// Artifacts run, never read back — with the same cutoff as terminal deployment records. + /// NOTE: the sweep is rate-limited by a process-wide static, so this is the only test in + /// the assembly that may drive . + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task TryPurgeTerminalDeploymentRecords_AlsoPurgesSystemArtifactRecords() + { + _repo.PurgeTerminalDeploymentRecordsAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(1); + _repo.PurgeSystemArtifactDeploymentRecordsAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(3); + + var purged = await _service.TryPurgeTerminalDeploymentRecordsAsync(); + + Assert.Equal(4, purged); + var expectedCutoff = DateTimeOffset.UtcNow - new DeploymentManagerOptions().TerminalDeploymentRecordRetention; + await _repo.Received(1).PurgeSystemArtifactDeploymentRecordsAsync( + Arg.Is(c => (c - expectedCutoff).Duration() < TimeSpan.FromMinutes(1)), + Arg.Any(), Arg.Any()); + } + // ── Deployment flow ── /// Verifies that deploy instance async instance not found returns failure.