From b0bd79183a1a8226be0ba5d4f8e506baae1d2014 Mon Sep 17 00:00:00 2001 From: James Salt Date: Wed, 27 May 2026 12:54:19 +0100 Subject: [PATCH 1/4] BCH-1158: Cascade soft-delete to step evidence and workflow executions on instance deletion When deleting a WorkflowInstance, the transaction now also soft-deletes: - StepEvidence for open step executions (so orphaned evidence no longer appears in the evidence view) - All WorkflowExecution records belonging to the instance (so the empty execution link is no longer navigable) Open step executions (pending, blocked, in_progress, overdue) are removed. Closed step executions (completed, failed, skipped) and their evidence are preserved to retain the audit trail. Co-Authored-By: Claude Sonnet 4.6 --- .../workflows/workflow_instance_service.go | 55 ++++++- .../workflow_instance_service_test.go | 154 ++++++++++++++++++ 2 files changed, 207 insertions(+), 2 deletions(-) diff --git a/internal/service/relational/workflows/workflow_instance_service.go b/internal/service/relational/workflows/workflow_instance_service.go index dde639a1..938890f2 100644 --- a/internal/service/relational/workflows/workflow_instance_service.go +++ b/internal/service/relational/workflows/workflow_instance_service.go @@ -133,9 +133,60 @@ func (s *WorkflowInstanceService) Update(id *uuid.UUID, updates *WorkflowInstanc return s.base.UpdateEntity(&existing, updates, id, "workflow instance") } -// Delete soft deletes a workflow instance +// Delete soft deletes a workflow instance and cascades to its open step executions and their +// evidence. Closed step executions (completed, failed, skipped) and their evidence are preserved +// to retain the audit trail. All workflow executions belonging to the instance are also removed. func (s *WorkflowInstanceService) Delete(id *uuid.UUID) error { - return s.base.DeleteEntity(&WorkflowInstance{}, id, "workflow instance") + return s.db.Transaction(func(tx *gorm.DB) error { + openStatuses := []string{ + string(StepStatusPending), + string(StepStatusBlocked), + string(StepStatusInProgress), + string(StepStatusOverdue), + } + + execSubquery := tx.Model(&WorkflowExecution{}). + Select("id"). + Where("workflow_instance_id = ?", id) + + // Collect open step execution IDs so we can delete their evidence. + var openStepIDs []uuid.UUID + if err := tx.Model(&StepExecution{}). + Select("id"). + Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, openStatuses). + Pluck("id", &openStepIDs).Error; err != nil { + return err + } + + if len(openStepIDs) > 0 { + if err := tx. + Where("step_execution_id IN ?", openStepIDs). + Delete(&StepEvidence{}).Error; err != nil { + return err + } + } + + if err := tx. + Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, openStatuses). + Delete(&StepExecution{}).Error; err != nil { + return err + } + + if err := tx. + Where("workflow_instance_id = ?", id). + Delete(&WorkflowExecution{}).Error; err != nil { + return err + } + + result := tx.Delete(&WorkflowInstance{}, id) + if result.Error != nil { + return result.Error + } + if result.RowsAffected == 0 { + return fmt.Errorf("workflow instance with id %s not found", id.String()) + } + return nil + }) } // Activate activates a workflow instance diff --git a/internal/service/relational/workflows/workflow_instance_service_test.go b/internal/service/relational/workflows/workflow_instance_service_test.go index 2167f0bd..094d4a24 100644 --- a/internal/service/relational/workflows/workflow_instance_service_test.go +++ b/internal/service/relational/workflows/workflow_instance_service_test.go @@ -293,6 +293,160 @@ func TestWorkflowInstanceService_Delete(t *testing.T) { assert.Contains(t, err.Error(), "not found") } +// TestWorkflowInstanceService_Delete_SoftDeletesOpenStepExecutions tests that deleting a workflow +// instance also soft-deletes open step executions (BCH-1158). +// Observed: pending/blocked/in_progress/overdue step executions remain alive after instance deletion. +// Expected: open step executions are soft-deleted; completed/failed/skipped are left untouched. +func TestWorkflowInstanceService_Delete_SoftDeletesOpenStepExecutions(t *testing.T) { + db := setupTestDB(t) + service := NewWorkflowInstanceService(db) + + workflowDef := createTestWorkflowDefinition() + require.NoError(t, db.Create(workflowDef).Error) + + instance := createTestWorkflowInstance(workflowDef.ID) + require.NoError(t, db.Create(instance).Error) + + execution := createTestWorkflowExecution(instance.ID) + require.NoError(t, db.Create(execution).Error) + + openStatuses := []string{ + string(StepStatusPending), + string(StepStatusBlocked), + string(StepStatusInProgress), + string(StepStatusOverdue), + } + closedStatuses := []string{ + string(StepStatusCompleted), + string(StepStatusFailed), + string(StepStatusSkipped), + } + + openStepIDs := make([]*uuid.UUID, 0, len(openStatuses)) + for _, status := range openStatuses { + sd := createTestWorkflowStepDefinition(workflowDef.ID) + require.NoError(t, db.Create(sd).Error) + step := createTestStepExecution(execution.ID, sd.ID) + step.Status = status + require.NoError(t, db.Create(step).Error) + openStepIDs = append(openStepIDs, step.ID) + } + + closedStepIDs := make([]*uuid.UUID, 0, len(closedStatuses)) + for _, status := range closedStatuses { + sd := createTestWorkflowStepDefinition(workflowDef.ID) + require.NoError(t, db.Create(sd).Error) + step := createTestStepExecution(execution.ID, sd.ID) + step.Status = status + require.NoError(t, db.Create(step).Error) + closedStepIDs = append(closedStepIDs, step.ID) + } + + require.NoError(t, service.Delete(instance.ID)) + + // Open step executions must be soft-deleted. + for _, id := range openStepIDs { + var step StepExecution + require.NoError(t, db.Unscoped().First(&step, id).Error) + assert.NotNil(t, step.DeletedAt.Time, "open step %s should be soft-deleted", id) + assert.True(t, step.DeletedAt.Valid, "open step %s should have a valid deleted_at", id) + } + + // Closed step executions must be preserved. + for _, id := range closedStepIDs { + var step StepExecution + require.NoError(t, db.First(&step, id).Error, "closed step %s should still exist", id) + assert.False(t, step.DeletedAt.Valid, "closed step %s should not be soft-deleted", id) + } +} + +// TestWorkflowInstanceService_Delete_SoftDeletesWorkflowExecutions tests that deleting a workflow +// instance also soft-deletes all workflow executions belonging to it (BCH-1158). +func TestWorkflowInstanceService_Delete_SoftDeletesWorkflowExecutions(t *testing.T) { + db := setupTestDB(t) + service := NewWorkflowInstanceService(db) + + workflowDef := createTestWorkflowDefinition() + require.NoError(t, db.Create(workflowDef).Error) + + instance := createTestWorkflowInstance(workflowDef.ID) + require.NoError(t, db.Create(instance).Error) + + exec1 := createTestWorkflowExecution(instance.ID) + require.NoError(t, db.Create(exec1).Error) + exec2 := createTestWorkflowExecution(instance.ID) + require.NoError(t, db.Create(exec2).Error) + + require.NoError(t, service.Delete(instance.ID)) + + for _, execID := range []*uuid.UUID{exec1.ID, exec2.ID} { + var we WorkflowExecution + require.NoError(t, db.Unscoped().First(&we, execID).Error) + assert.True(t, we.DeletedAt.Valid, "workflow execution %s should be soft-deleted", execID) + } +} + +// TestWorkflowInstanceService_Delete_SoftDeletesStepEvidenceForOpenSteps tests that deleting a +// workflow instance also soft-deletes evidence submitted for open (non-closed) steps (BCH-1158). +// Closed steps and their evidence must be preserved for the audit trail. +func TestWorkflowInstanceService_Delete_SoftDeletesStepEvidenceForOpenSteps(t *testing.T) { + db := setupTestDB(t) + service := NewWorkflowInstanceService(db) + + workflowDef := createTestWorkflowDefinition() + require.NoError(t, db.Create(workflowDef).Error) + + instance := createTestWorkflowInstance(workflowDef.ID) + require.NoError(t, db.Create(instance).Error) + + execution := createTestWorkflowExecution(instance.ID) + require.NoError(t, db.Create(execution).Error) + + // Open step with evidence that should be deleted. + openStepDef := createTestWorkflowStepDefinition(workflowDef.ID) + require.NoError(t, db.Create(openStepDef).Error) + openStep := createTestStepExecution(execution.ID, openStepDef.ID) + openStep.Status = string(StepStatusInProgress) + require.NoError(t, db.Create(openStep).Error) + + openEvidenceID := uuid.New() + openEvidence := &StepEvidence{ + UUIDModel: relational.UUIDModel{ID: &openEvidenceID}, + StepExecutionID: openStep.ID, + Name: "Open step evidence", + EvidenceType: "document", + } + require.NoError(t, db.Create(openEvidence).Error) + + // Closed step with evidence that must be preserved. + closedStepDef := createTestWorkflowStepDefinition(workflowDef.ID) + require.NoError(t, db.Create(closedStepDef).Error) + closedStep := createTestStepExecution(execution.ID, closedStepDef.ID) + closedStep.Status = string(StepStatusCompleted) + require.NoError(t, db.Create(closedStep).Error) + + closedEvidenceID := uuid.New() + closedEvidence := &StepEvidence{ + UUIDModel: relational.UUIDModel{ID: &closedEvidenceID}, + StepExecutionID: closedStep.ID, + Name: "Closed step evidence", + EvidenceType: "document", + } + require.NoError(t, db.Create(closedEvidence).Error) + + require.NoError(t, service.Delete(instance.ID)) + + // Evidence for the open step must be soft-deleted. + var deletedEvidence StepEvidence + require.NoError(t, db.Unscoped().First(&deletedEvidence, &openEvidenceID).Error) + assert.True(t, deletedEvidence.DeletedAt.Valid, "evidence for open step should be soft-deleted") + + // Evidence for the closed step must be preserved. + var preservedEvidence StepEvidence + require.NoError(t, db.First(&preservedEvidence, &closedEvidenceID).Error, "evidence for closed step should still exist") + assert.False(t, preservedEvidence.DeletedAt.Valid, "evidence for closed step should not be soft-deleted") +} + // TestWorkflowInstanceService_Activate tests the Activate method func TestWorkflowInstanceService_Activate(t *testing.T) { db := setupTestDB(t) From 775e09f7f095fe0371522ad0072ed21d1ab2fe1f Mon Sep 17 00:00:00 2001 From: James Salt Date: Wed, 27 May 2026 13:42:13 +0100 Subject: [PATCH 2/4] Somebody kick the wabbit From ea97701abe6bc874e0aca3353363131a46c0df32 Mon Sep 17 00:00:00 2001 From: James Salt Date: Wed, 27 May 2026 14:01:50 +0100 Subject: [PATCH 3/4] BCH-1158: Address PR review comments on cascade delete - Extract OpenStepStatuses package var to constants.go to avoid drift across codebase queries (workflow_instance_service, step_execution_service) - Use NewBaseService(tx).DeleteEntity for instance soft-delete inside the transaction instead of inline result/RowsAffected check Co-Authored-By: Claude Sonnet 4.6 --- .../service/relational/workflows/constants.go | 9 +++++++++ .../workflows/step_execution_service.go | 2 +- .../workflows/workflow_instance_service.go | 20 +++---------------- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/internal/service/relational/workflows/constants.go b/internal/service/relational/workflows/constants.go index 8138c26d..b6ddc2fa 100644 --- a/internal/service/relational/workflows/constants.go +++ b/internal/service/relational/workflows/constants.go @@ -144,6 +144,15 @@ const ( StepStatusSkipped StepExecutionStatus = "skipped" ) +// OpenStepStatuses contains the step execution statuses that represent incomplete work. +// Use this slice for queries that target non-terminal steps (e.g. cascade deletes, assignment lookups). +var OpenStepStatuses = []string{ + string(StepStatusPending), + string(StepStatusBlocked), + string(StepStatusInProgress), + string(StepStatusOverdue), +} + // IsValid checks if the step execution status is valid func (s StepExecutionStatus) IsValid() bool { switch s { diff --git a/internal/service/relational/workflows/step_execution_service.go b/internal/service/relational/workflows/step_execution_service.go index 61b4c1a6..d7abf25f 100644 --- a/internal/service/relational/workflows/step_execution_service.go +++ b/internal/service/relational/workflows/step_execution_service.go @@ -368,7 +368,7 @@ func (s *StepExecutionService) GetCompletedSteps(executionID *uuid.UUID) ([]Step func (s *StepExecutionService) GetAssignedSteps(assignedToType, assignedToID string) ([]StepExecution, error) { var stepExecutions []StepExecution err := s.db.Where("assigned_to_type = ? AND assigned_to_id = ? AND status IN ?", - assignedToType, assignedToID, []string{"pending", "in_progress", "blocked", "overdue"}). + assignedToType, assignedToID, OpenStepStatuses). Preload("WorkflowExecution"). Preload("WorkflowExecution.WorkflowInstance"). Preload("WorkflowStepDefinition"). diff --git a/internal/service/relational/workflows/workflow_instance_service.go b/internal/service/relational/workflows/workflow_instance_service.go index 938890f2..1ee8314b 100644 --- a/internal/service/relational/workflows/workflow_instance_service.go +++ b/internal/service/relational/workflows/workflow_instance_service.go @@ -138,13 +138,6 @@ func (s *WorkflowInstanceService) Update(id *uuid.UUID, updates *WorkflowInstanc // to retain the audit trail. All workflow executions belonging to the instance are also removed. func (s *WorkflowInstanceService) Delete(id *uuid.UUID) error { return s.db.Transaction(func(tx *gorm.DB) error { - openStatuses := []string{ - string(StepStatusPending), - string(StepStatusBlocked), - string(StepStatusInProgress), - string(StepStatusOverdue), - } - execSubquery := tx.Model(&WorkflowExecution{}). Select("id"). Where("workflow_instance_id = ?", id) @@ -153,7 +146,7 @@ func (s *WorkflowInstanceService) Delete(id *uuid.UUID) error { var openStepIDs []uuid.UUID if err := tx.Model(&StepExecution{}). Select("id"). - Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, openStatuses). + Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, OpenStepStatuses). Pluck("id", &openStepIDs).Error; err != nil { return err } @@ -167,7 +160,7 @@ func (s *WorkflowInstanceService) Delete(id *uuid.UUID) error { } if err := tx. - Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, openStatuses). + Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, OpenStepStatuses). Delete(&StepExecution{}).Error; err != nil { return err } @@ -178,14 +171,7 @@ func (s *WorkflowInstanceService) Delete(id *uuid.UUID) error { return err } - result := tx.Delete(&WorkflowInstance{}, id) - if result.Error != nil { - return result.Error - } - if result.RowsAffected == 0 { - return fmt.Errorf("workflow instance with id %s not found", id.String()) - } - return nil + return NewBaseService(tx).DeleteEntity(&WorkflowInstance{}, id, "workflow instance") }) } From 7e651b0fdf953ff379f67210dffd43a86af1a180 Mon Sep 17 00:00:00 2001 From: James Salt Date: Wed, 27 May 2026 14:36:18 +0100 Subject: [PATCH 4/4] BCH-1158: Harden OpenStepStatuses against mutation Replace exported mutable slice with an unexported backing var and a copy-returning accessor function to prevent callers from accidentally mutating shared query state at runtime. Co-Authored-By: Claude Sonnet 4.6 --- internal/service/relational/workflows/constants.go | 14 +++++++++++--- .../relational/workflows/step_execution_service.go | 2 +- .../workflows/workflow_instance_service.go | 4 ++-- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/internal/service/relational/workflows/constants.go b/internal/service/relational/workflows/constants.go index b6ddc2fa..4c6a1492 100644 --- a/internal/service/relational/workflows/constants.go +++ b/internal/service/relational/workflows/constants.go @@ -144,15 +144,23 @@ const ( StepStatusSkipped StepExecutionStatus = "skipped" ) -// OpenStepStatuses contains the step execution statuses that represent incomplete work. -// Use this slice for queries that target non-terminal steps (e.g. cascade deletes, assignment lookups). -var OpenStepStatuses = []string{ +// openStepStatuses is the unexported backing slice for OpenStepStatuses. +var openStepStatuses = []string{ string(StepStatusPending), string(StepStatusBlocked), string(StepStatusInProgress), string(StepStatusOverdue), } +// OpenStepStatuses returns a copy of the step execution statuses that represent incomplete work. +// Use it for queries that target non-terminal steps (e.g. cascade deletes, assignment lookups). +// A copy is returned to prevent callers from mutating the shared backing slice. +func OpenStepStatuses() []string { + s := make([]string, len(openStepStatuses)) + copy(s, openStepStatuses) + return s +} + // IsValid checks if the step execution status is valid func (s StepExecutionStatus) IsValid() bool { switch s { diff --git a/internal/service/relational/workflows/step_execution_service.go b/internal/service/relational/workflows/step_execution_service.go index d7abf25f..deacb0e5 100644 --- a/internal/service/relational/workflows/step_execution_service.go +++ b/internal/service/relational/workflows/step_execution_service.go @@ -368,7 +368,7 @@ func (s *StepExecutionService) GetCompletedSteps(executionID *uuid.UUID) ([]Step func (s *StepExecutionService) GetAssignedSteps(assignedToType, assignedToID string) ([]StepExecution, error) { var stepExecutions []StepExecution err := s.db.Where("assigned_to_type = ? AND assigned_to_id = ? AND status IN ?", - assignedToType, assignedToID, OpenStepStatuses). + assignedToType, assignedToID, OpenStepStatuses()). Preload("WorkflowExecution"). Preload("WorkflowExecution.WorkflowInstance"). Preload("WorkflowStepDefinition"). diff --git a/internal/service/relational/workflows/workflow_instance_service.go b/internal/service/relational/workflows/workflow_instance_service.go index 1ee8314b..dfc09324 100644 --- a/internal/service/relational/workflows/workflow_instance_service.go +++ b/internal/service/relational/workflows/workflow_instance_service.go @@ -146,7 +146,7 @@ func (s *WorkflowInstanceService) Delete(id *uuid.UUID) error { var openStepIDs []uuid.UUID if err := tx.Model(&StepExecution{}). Select("id"). - Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, OpenStepStatuses). + Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, OpenStepStatuses()). Pluck("id", &openStepIDs).Error; err != nil { return err } @@ -160,7 +160,7 @@ func (s *WorkflowInstanceService) Delete(id *uuid.UUID) error { } if err := tx. - Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, OpenStepStatuses). + Where("workflow_execution_id IN (?) AND status IN ?", execSubquery, OpenStepStatuses()). Delete(&StepExecution{}).Error; err != nil { return err }