From 1d5465f31c15a76a099cc75292a0a3487cef21f0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 15 May 2026 12:05:13 -0400 Subject: [PATCH] fix(deployment): instance delete fully removes the record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting an instance only undeployed it from the site and set the state to NotDeployed, leaving an orphan record that could never be removed — the state-transition matrix rejected delete from NotDeployed. Delete now removes the instance record entirely (deployment history, snapshot, attribute/alarm overrides, and connection bindings go with it), and is permitted from any state. --- .../IDeploymentManagerRepository.cs | 7 +++++++ .../DeploymentManagerRepository.cs | 21 +++++++++++++++++++ .../DeploymentService.cs | 14 ++++++------- .../StateTransitionValidator.cs | 5 +++-- .../DeploymentServiceTests.cs | 7 +++---- .../StateTransitionValidatorTests.cs | 8 +++---- 6 files changed, 45 insertions(+), 17 deletions(-) diff --git a/src/ScadaLink.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs b/src/ScadaLink.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs index ba3314e..4e775e4 100644 --- a/src/ScadaLink.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs +++ b/src/ScadaLink.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs @@ -34,5 +34,12 @@ public interface IDeploymentManagerRepository Task GetInstanceByUniqueNameAsync(string uniqueName, CancellationToken cancellationToken = default); Task UpdateInstanceAsync(Instance instance, CancellationToken cancellationToken = default); + /// + /// Removes an instance and everything that depends on it: deployment + /// records, deployed config snapshot, attribute/alarm overrides, and + /// connection bindings. + /// + Task DeleteInstanceAsync(int instanceId, CancellationToken cancellationToken = default); + Task SaveChangesAsync(CancellationToken cancellationToken = default); } diff --git a/src/ScadaLink.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs b/src/ScadaLink.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs index 5dbf2f9..de77c8b 100644 --- a/src/ScadaLink.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs +++ b/src/ScadaLink.ConfigurationDatabase/Repositories/DeploymentManagerRepository.cs @@ -189,6 +189,27 @@ public class DeploymentManagerRepository : IDeploymentManagerRepository return Task.CompletedTask; } + public async Task DeleteInstanceAsync(int instanceId, CancellationToken cancellationToken = default) + { + // DeploymentRecords have a Restrict FK to Instance — remove them + // explicitly first. The snapshot, overrides, and connection bindings + // are configured with cascade delete and go with the instance. + var records = await _dbContext.DeploymentRecords + .Where(d => d.InstanceId == instanceId) + .ToListAsync(cancellationToken); + if (records.Count > 0) + { + _dbContext.DeploymentRecords.RemoveRange(records); + } + + var instance = await _dbContext.Set() + .FirstOrDefaultAsync(i => i.Id == instanceId, cancellationToken); + if (instance != null) + { + _dbContext.Set().Remove(instance); + } + } + public async Task SaveChangesAsync(CancellationToken cancellationToken = default) { return await _dbContext.SaveChangesAsync(cancellationToken); diff --git a/src/ScadaLink.DeploymentManager/DeploymentService.cs b/src/ScadaLink.DeploymentManager/DeploymentService.cs index 5f29951..aaa2bde 100644 --- a/src/ScadaLink.DeploymentManager/DeploymentService.cs +++ b/src/ScadaLink.DeploymentManager/DeploymentService.cs @@ -282,7 +282,9 @@ public class DeploymentService } /// - /// WP-6: Delete an instance. Stops actor, removes config. S&F NOT cleared. + /// WP-6: Delete an instance. Stops the site actor, removes site config, and + /// removes the central instance record (deployment history, snapshot, + /// overrides, and connection bindings go with it). S&F NOT cleared. /// Delete fails if site unreachable (30s timeout via CommunicationOptions). /// public async Task> DeleteInstanceAsync( @@ -309,12 +311,10 @@ public class DeploymentService if (response.Success) { - // Remove deployed snapshot - await _repository.DeleteDeployedSnapshotAsync(instanceId, cancellationToken); - - // Set state to NotDeployed (or the instance record could be deleted entirely by higher layers) - instance.State = InstanceState.NotDeployed; - await _repository.UpdateInstanceAsync(instance, cancellationToken); + // Delete means delete: remove the instance record entirely. + // Deployment records, snapshot, overrides, and connection bindings + // are removed with it (see repository implementation). + await _repository.DeleteInstanceAsync(instanceId, cancellationToken); await _repository.SaveChangesAsync(cancellationToken); } diff --git a/src/ScadaLink.DeploymentManager/StateTransitionValidator.cs b/src/ScadaLink.DeploymentManager/StateTransitionValidator.cs index 7780266..c2f49c7 100644 --- a/src/ScadaLink.DeploymentManager/StateTransitionValidator.cs +++ b/src/ScadaLink.DeploymentManager/StateTransitionValidator.cs @@ -7,11 +7,12 @@ namespace ScadaLink.DeploymentManager; /// /// State | Deploy | Disable | Enable | Delete /// ----------|--------|---------|--------|------- -/// NotDeploy | OK | NO | NO | NO +/// NotDeploy | OK | NO | NO | OK /// Enabled | OK | OK | NO | OK /// Disabled | OK* | NO | OK | OK /// /// * Deploy on a Disabled instance also enables it. +/// Delete removes the instance record entirely; it is valid from any state. /// public static class StateTransitionValidator { @@ -25,7 +26,7 @@ public static class StateTransitionValidator currentState == InstanceState.Disabled; public static bool CanDelete(InstanceState currentState) => - currentState is InstanceState.Enabled or InstanceState.Disabled; + currentState is InstanceState.NotDeployed or InstanceState.Enabled or InstanceState.Disabled; /// /// Returns a human-readable error message if the transition is invalid, or null if valid. diff --git a/tests/ScadaLink.DeploymentManager.Tests/DeploymentServiceTests.cs b/tests/ScadaLink.DeploymentManager.Tests/DeploymentServiceTests.cs index c9ef160..4b6401e 100644 --- a/tests/ScadaLink.DeploymentManager.Tests/DeploymentServiceTests.cs +++ b/tests/ScadaLink.DeploymentManager.Tests/DeploymentServiceTests.cs @@ -188,15 +188,14 @@ public class DeploymentServiceTests } [Fact] - public async Task DeleteInstanceAsync_WhenNotDeployed_ReturnsTransitionError() + public async Task DeleteInstanceAsync_InstanceNotFound_ReturnsFailure() { - var instance = new Instance("TestInst") { Id = 1, SiteId = 1, State = InstanceState.NotDeployed }; - _repo.GetInstanceByIdAsync(1).Returns(instance); + _repo.GetInstanceByIdAsync(1).Returns((Instance?)null); var result = await _service.DeleteInstanceAsync(1, "admin"); Assert.True(result.IsFailure); - Assert.Contains("not allowed", result.Error); + Assert.Contains("not found", result.Error); } // ── WP-8: Deployment comparison ── diff --git a/tests/ScadaLink.DeploymentManager.Tests/StateTransitionValidatorTests.cs b/tests/ScadaLink.DeploymentManager.Tests/StateTransitionValidatorTests.cs index 374ebbb..9bb30df 100644 --- a/tests/ScadaLink.DeploymentManager.Tests/StateTransitionValidatorTests.cs +++ b/tests/ScadaLink.DeploymentManager.Tests/StateTransitionValidatorTests.cs @@ -73,9 +73,9 @@ public class StateTransitionValidatorTests } [Fact] - public void CanDelete_WhenNotDeployed_ReturnsFalse() + public void CanDelete_WhenNotDeployed_ReturnsTrue() { - Assert.False(StateTransitionValidator.CanDelete(InstanceState.NotDeployed)); + Assert.True(StateTransitionValidator.CanDelete(InstanceState.NotDeployed)); } // ── ValidateTransition ── @@ -103,10 +103,10 @@ public class StateTransitionValidatorTests } [Fact] - public void ValidateTransition_InvalidDeleteOnNotDeployed_ReturnsError() + public void ValidateTransition_ValidDeleteOnNotDeployed_ReturnsNull() { var error = StateTransitionValidator.ValidateTransition(InstanceState.NotDeployed, "delete"); - Assert.NotNull(error); + Assert.Null(error); } [Fact]