fix(deployment): purge SystemArtifactDeploymentRecords in the terminal-record retention sweep (review 2026-10-21 08/G2)

This commit is contained in:
Joseph Doherty
2026-10-22 06:20:44 -04:00
parent da5a049077
commit d6a9a79c27
7 changed files with 138 additions and 4 deletions
@@ -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, reconciliation reads to decide whether a prior deploy actually landed at the site,
and expiring it by age would silently disable that idempotency guard. 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 ## Diff View
Before deploying, the Deployment Manager can request a diff from the Template Engine showing: Before deploying, the Deployment Manager can request a diff from the Template Engine showing:
@@ -194,6 +194,17 @@ public interface IDeploymentManagerRepository
/// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param> /// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param>
/// <returns>A task representing the asynchronous operation.</returns> /// <returns>A task representing the asynchronous operation.</returns>
Task DeleteSystemArtifactDeploymentAsync(int id, CancellationToken cancellationToken = default); Task DeleteSystemArtifactDeploymentAsync(int id, CancellationToken cancellationToken = default);
/// <summary>
/// Deletes system artifact deployment records deployed before <paramref name="cutoffUtc"/>,
/// in one bounded batch. One row is written per <i>Deploy Artifacts</i> run and nothing
/// reads them back, so they share the terminal deployment-record retention window
/// instead of growing without bound.
/// </summary>
/// <param name="cutoffUtc">Records deployed before this instant are eligible.</param>
/// <param name="batchSize">Maximum rows deleted in this call.</param>
/// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param>
/// <returns>The number of rows deleted; fewer than <paramref name="batchSize"/> means the sweep is complete.</returns>
Task<int> PurgeSystemArtifactDeploymentRecordsAsync(DateTimeOffset cutoffUtc, int batchSize, CancellationToken cancellationToken = default);
// DeployedConfigSnapshot // DeployedConfigSnapshot
/// <summary> /// <summary>
@@ -285,6 +285,29 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository
return Task.CompletedTask; return Task.CompletedTask;
} }
/// <inheritdoc />
public async Task<int> 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 --- // --- DeployedConfigSnapshot ---
/// <inheritdoc /> /// <inheritdoc />
@@ -72,6 +72,8 @@ public class DeploymentManagerOptions
/// <c>InProgress</c> or <c>Pending</c> row is never purged regardless of age, /// <c>InProgress</c> or <c>Pending</c> row is never purged regardless of age,
/// because it is exactly the row the query-before-redeploy reconciliation /// because it is exactly the row the query-before-redeploy reconciliation
/// path needs to find. Set to <see cref="TimeSpan.Zero"/> to disable purging. /// path needs to find. Set to <see cref="TimeSpan.Zero"/> to disable purging.
/// The same window bounds system artifact deployment records (one per
/// <i>Deploy Artifacts</i> run), purged by their deploy time.
/// </para> /// </para>
/// </summary> /// </summary>
public TimeSpan TerminalDeploymentRecordRetention { get; set; } = TimeSpan.FromDays(365); public TimeSpan TerminalDeploymentRecordRetention { get; set; } = TimeSpan.FromDays(365);
@@ -1330,7 +1330,8 @@ public class DeploymentService
instanceId, uniqueName, deploymentId, false, result.Error); instanceId, uniqueName, deploymentId, false, result.Error);
/// <summary> /// <summary>
/// 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 <see cref="TerminalPurgeMinInterval"/> per process and bounded /// to once every <see cref="TerminalPurgeMinInterval"/> per process and bounded
/// to <see cref="TerminalPurgeBatchesPerSweep"/> batches per call. /// to <see cref="TerminalPurgeBatchesPerSweep"/> batches per call.
/// ///
@@ -1379,12 +1380,26 @@ public class DeploymentService
break; 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) if (purged > 0)
{ {
_logger.LogInformation( _logger.LogInformation(
"Purged {Count} terminal deployment record(s) completed before {Cutoff} " + "Purged {Count} terminal deployment record(s) ({ArtifactCount} system artifact " +
"(retention {Retention}).", "record(s)) completed before {Cutoff} (retention {Retention}).",
purged, cutoff, retention); purged, artifactPurged, cutoff, retention);
} }
} }
catch (Exception ex) catch (Exception ex)
@@ -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;
/// <summary>
/// Retention purge for <see cref="SystemArtifactDeploymentRecord"/> 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.
/// </summary>
public sealed class SystemArtifactDeploymentPurgeTests : IDisposable
{
private readonly ScadaBridgeDbContext _context = SqliteTestHelper.CreateInMemoryContext();
private readonly DeploymentManagerRepository _repository;
/// <summary>Initializes the repository over a fresh in-memory SQLite context.</summary>
public SystemArtifactDeploymentPurgeTests()
{
_repository = new DeploymentManagerRepository(_context);
}
/// <summary>Releases the in-memory database.</summary>
public void Dispose() => _context.Dispose();
private async Task SeedAsync(DateTimeOffset deployedAt)
{
await _repository.AddSystemArtifactDeploymentAsync(
new SystemArtifactDeploymentRecord("Artifacts", "admin") { DeployedAt = deployedAt });
await _repository.SaveChangesAsync();
}
/// <summary>Only rows older than the cutoff are deleted, in bounded batches.</summary>
/// <returns>A task that represents the asynchronous operation.</returns>
[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);
}
}
@@ -87,6 +87,32 @@ public class DeploymentServiceTests : TestKit
PendingDeploymentTtl = TestPendingTtl PendingDeploymentTtl = TestPendingTtl
}); });
// ── Terminal-record retention (review 2026-10-21 08/G2) ──
/// <summary>
/// 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 <see cref="DeploymentService.TryPurgeTerminalDeploymentRecordsAsync"/>.
/// </summary>
/// <returns>A task that represents the asynchronous operation.</returns>
[Fact]
public async Task TryPurgeTerminalDeploymentRecords_AlsoPurgesSystemArtifactRecords()
{
_repo.PurgeTerminalDeploymentRecordsAsync(Arg.Any<DateTimeOffset>(), Arg.Any<int>(), Arg.Any<CancellationToken>())
.Returns(1);
_repo.PurgeSystemArtifactDeploymentRecordsAsync(Arg.Any<DateTimeOffset>(), Arg.Any<int>(), Arg.Any<CancellationToken>())
.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<DateTimeOffset>(c => (c - expectedCutoff).Duration() < TimeSpan.FromMinutes(1)),
Arg.Any<int>(), Arg.Any<CancellationToken>());
}
// ── Deployment flow ── // ── Deployment flow ──
/// <summary>Verifies that deploy instance async instance not found returns failure.</summary> /// <summary>Verifies that deploy instance async instance not found returns failure.</summary>