diff --git a/internal/detonators/simrun_detonator.go b/internal/detonators/simrun_detonator.go index 7a1bdc8..a6d3f8b 100644 --- a/internal/detonators/simrun_detonator.go +++ b/internal/detonators/simrun_detonator.go @@ -99,12 +99,17 @@ func (d *SimrunDetonator) Detonate() (map[string]string, error) { return nil, fmt.Errorf("failed to generate execution ID: %w", err) } + // Carry execution_id out on every failure path past this point so a + // partially-completed detonation (e.g. terraform applied, then the + // detonation timed out) stays correlatable in the persisted result. + failure := map[string]string{"execution_id": executionID} + d.setupLoggingAndTerraform(executionID) // Get manifest and find simulation simulation, err := d.resolveSimulation(ctx) if err != nil { - return nil, err + return failure, err } d.logSimulationInfo(simulation) @@ -115,7 +120,7 @@ func (d *SimrunDetonator) Detonate() (map[string]string, error) { } terraformOutputs, tfWorkDir, err := d.setupTerraform(ctx, executionID, simulation) if err != nil { - return nil, err + return failure, err } if tfWorkDir != "" { defer d.cleanupTerraform(ctx, executionID, tfWorkDir) @@ -125,7 +130,7 @@ func (d *SimrunDetonator) Detonate() (map[string]string, error) { d.reportPhase("detonating") detonateOutput, err := d.executeSimulation(ctx, executionID, terraformOutputs) if err != nil { - return nil, err + return failure, err } // Run custom cleanup if needed @@ -217,6 +222,38 @@ func (d *SimrunDetonator) terraformEnvVars(executionID string) map[string]string return vars } +// detonationEnvVars returns the environment for the pack runner. It augments +// the run env with AWS_REGION/AWS_DEFAULT_REGION derived from the resolved +// aws_region parameter. Terraform pins the AWS provider to var.aws_region, but +// the detonation resolves region via the AWS SDK default chain (ambient +// AWS_REGION). Without aligning them, terraform creates resources in +// var.aws_region (default us-east-1) while the detonation acts on the +// container's ambient region — producing NotFound errors. envutil merges this +// map over the process env, so the value here wins over any ambient AWS_REGION. +func (d *SimrunDetonator) detonationEnvVars() map[string]string { + env := make(map[string]string, len(d.envVars)+2) + for k, v := range d.envVars { + env[k] = v + } + region := d.resolveAWSRegion() + env["AWS_REGION"] = region + env["AWS_DEFAULT_REGION"] = region + return env +} + +// resolveAWSRegion resolves the AWS region the detonation should use, mirroring +// the precedence terraformEnvVars applies to TF_VAR_aws_region: per-sim +// scenario param > pack-level param > built-in default. +func (d *SimrunDetonator) resolveAWSRegion() string { + if r, ok := d.params["aws_region"].(string); ok && r != "" { + return r + } + if r, ok := d.packConfig.Parameters["aws_region"].(string); ok && r != "" { + return r + } + return pack.DefaultAWSRegion +} + // formatTFVar renders a parameter value into the string form Terraform // accepts for TF_VAR_*. Map and slice values are JSON-encoded so map/list // typed Terraform variables receive a parseable HCL/JSON literal. @@ -398,7 +435,7 @@ func (d *SimrunDetonator) logSimulationInfo(simulation *pack.SimulationManifest) // createExecutor creates a pack runner and executor for simulation execution func (d *SimrunDetonator) createExecutor(ctx context.Context, executionID string) (*executor.Executor, runner.PackRunner, error) { - packRunner, err := d.runnerFactory.CreateRunner(ctx, d.packConfig, d.envVars) + packRunner, err := d.runnerFactory.CreateRunner(ctx, d.packConfig, d.detonationEnvVars()) if err != nil { return nil, nil, fmt.Errorf("failed to create pack runner: %w", err) } diff --git a/internal/detonators/simrun_detonator_test.go b/internal/detonators/simrun_detonator_test.go index 619029f..a244419 100644 --- a/internal/detonators/simrun_detonator_test.go +++ b/internal/detonators/simrun_detonator_test.go @@ -4,8 +4,64 @@ import ( "testing" "github.com/IBM/simrun/internal/config" + "github.com/IBM/simrun/pack" ) +// TestDetonationEnvVars_AWSRegion verifies the detonation env pins +// AWS_REGION/AWS_DEFAULT_REGION to the same aws_region value terraform's +// provider uses. Without this, terraform creates resources in var.aws_region +// (default us-east-1) while the detonation SDK resolves region from the +// container's ambient AWS_REGION, so deletes/lookups hit the wrong region and +// fail with NotFound. Precedence mirrors terraformEnvVars: per-sim > pack-level +// > built-in default. +func TestDetonationEnvVars_AWSRegion(t *testing.T) { + tests := []struct { + name string + envVars map[string]string + packParams map[string]any + simParams map[string]any + want string + }{ + { + name: "unset falls back to built-in default", + want: pack.DefaultAWSRegion, + }, + { + name: "pack-level value used", + packParams: map[string]any{"aws_region": "eu-west-1"}, + want: "eu-west-1", + }, + { + name: "per-sim overrides pack-level", + packParams: map[string]any{"aws_region": "eu-west-1"}, + simParams: map[string]any{"aws_region": "ap-south-1"}, + want: "ap-south-1", + }, + { + name: "param overrides ambient run-env region", + envVars: map[string]string{"AWS_REGION": "us-west-2"}, + packParams: map[string]any{"aws_region": "eu-west-1"}, + want: "eu-west-1", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &SimrunDetonator{ + packConfig: config.PackConfig{Parameters: tt.packParams}, + params: tt.simParams, + envVars: tt.envVars, + } + env := d.detonationEnvVars() + if env["AWS_REGION"] != tt.want { + t.Errorf("AWS_REGION = %q, want %q", env["AWS_REGION"], tt.want) + } + if env["AWS_DEFAULT_REGION"] != tt.want { + t.Errorf("AWS_DEFAULT_REGION = %q, want %q", env["AWS_DEFAULT_REGION"], tt.want) + } + }) + } +} + func TestTerraformEnvVars_PerSimOverridesPackLevel(t *testing.T) { d := &SimrunDetonator{ packConfig: config.PackConfig{ diff --git a/internal/runner/runner.go b/internal/runner/runner.go index 91e232a..18af6fd 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -81,7 +81,10 @@ func (m *Runner) runScenario(scenario *Scenario) (string, float64, error) { executionOutput, logger, err := m.executeScenario(scenario) if err != nil { - return "", 0, err + // Preserve the execution_id even on failure so a partially-completed + // detonation (e.g. terraform applied, detonation timed out) stays + // correlatable. Indexing a nil map yields "". + return executionOutput["execution_id"], 0, err } indicators := m.buildIndicatorsList(scenario, executionOutput) diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 20323e3..c6c46c1 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -103,7 +103,7 @@ func TestRunnerErrorHandling(t *testing.T) { mockDetonator.On("SetStatusCallback", mock.AnythingOfType("func(string)")).Return() mockFailingDetonator := &detonatorMocks.MockDetonator{} - mockFailingDetonator.On("Detonate").Return(map[string]string{}, errors.New("foo")) + mockFailingDetonator.On("Detonate").Return(map[string]string{"execution_id": "failed-uid"}, errors.New("foo")) mockFailingDetonator.On("String").Return("mock-failing-detonator") mockFailingDetonator.On("SimulationId").Return("failing-simulation") mockFailingDetonator.On("PackName").Return("") @@ -155,6 +155,15 @@ func TestRunnerErrorHandling(t *testing.T) { assert.Equal(t, 2, successCount) assert.Equal(t, 1, failedCount) + // A failed scenario must still carry its execution_id so the result can be + // correlated with the partial detonation (e.g. terraform that did apply). + for _, result := range results { + if !result.Success { + assert.Equal(t, "failed-uid", result.ExecutionId, + "failed scenario should retain the execution_id from the detonation output") + } + } + // All scenarios should have been detonated, even if one returned an error mockDetonator.AssertNumberOfCalls(t, "Detonate", 2) mockFailingDetonator.AssertNumberOfCalls(t, "Detonate", 1) diff --git a/internal/web/retention_test.go b/internal/web/retention_test.go index de5d183..8052353 100644 --- a/internal/web/retention_test.go +++ b/internal/web/retention_test.go @@ -122,3 +122,96 @@ func TestSweepAssessments_DisabledIsNoOp(t *testing.T) { assert.FileExists(t, jsonl) assert.FileExists(t, ndjson) } + +// seedTerraformDir creates /terraform// with nested state and +// plugin contents, mirroring a real Terraform working directory so removal must +// recurse (os.RemoveAll), not a flat os.Remove. +func seedTerraformDir(t *testing.T, dataDir, execID string) string { + t.Helper() + dir := filepath.Join(dataDir, "terraform", execID) + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".terraform"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "terraform.tfstate"), []byte("{}"), 0o644)) + return dir +} + +// makeAgedRunWithExecutions creates an aged completed run whose scenario results +// carry the given execution IDs (one result per ID), returning the run ID. +func makeAgedRunWithExecutions(t *testing.T, ctx context.Context, store *fakes.RunStore, dataDir string, execIDs ...string) uuid.UUID { + t.Helper() + id := uuid.New() + created := time.Now().AddDate(0, 0, -40) + require.NoError(t, store.Create(ctx, &db.Run{ + ID: id, + Status: "completed", + StartTime: created, + CreatedAt: created, + })) + writeLog(t, dataDir, id.String(), 40) + for i, execID := range execIDs { + require.NoError(t, store.AddScenarioResult(ctx, id, &db.ScenarioResult{ + Name: "s" + uuid.NewString()[:4] + "-" + string(rune('a'+i)), + ExecutionID: execID, + })) + } + return id +} + +// Deleting a run reclaims the per-execution Terraform working directory for each +// of its scenario results — the disk space these dirs hold is the whole point. +func TestSweepAssessments_RemovesTerraformDirs(t *testing.T) { + dataDir := t.TempDir() + store := fakes.New().Run + ctx := context.Background() + + e1, e2 := uuid.NewString(), uuid.NewString() + id := makeAgedRunWithExecutions(t, ctx, store, dataDir, e1, e2) + dir1 := seedTerraformDir(t, dataDir, e1) + dir2 := seedTerraformDir(t, dataDir, e2) + + web.SweepAssessments(ctx, store, dataDir, true, 30) + + _, err := store.Get(ctx, id) + assert.Error(t, err, "aged run row should be deleted") + assert.NoDirExists(t, dir1, "Terraform dir for E1 should be removed") + assert.NoDirExists(t, dir2, "Terraform dir for E2 should be removed") +} + +// An unsafe execution_id must never cause cleanup to escape or wipe the +// terraform/ base directory — a blank id would otherwise RemoveAll the base. +func TestSweepAssessments_SkipsUnsafeExecutionID(t *testing.T) { + for _, execID := range []string{"", " ", "../escape", "nested/id", ".", ".."} { + t.Run("id="+execID, func(t *testing.T) { + dataDir := t.TempDir() + store := fakes.New().Run + ctx := context.Background() + + // A sibling dir keyed on a safe id proves only the unsafe id is skipped. + safe := uuid.NewString() + id := makeAgedRunWithExecutions(t, ctx, store, dataDir, execID, safe) + safeDir := seedTerraformDir(t, dataDir, safe) + base := filepath.Join(dataDir, "terraform") + + web.SweepAssessments(ctx, store, dataDir, true, 30) + + _, err := store.Get(ctx, id) + assert.Error(t, err, "aged run row should still be deleted") + assert.DirExists(t, base, "terraform base dir must survive an unsafe id") + assert.NoDirExists(t, safeDir, "the safe sibling id should still be cleaned up") + }) + } +} + +// A missing Terraform dir must not fail the delete — removal is best-effort. +func TestSweepAssessments_MissingTerraformDirIsBestEffort(t *testing.T) { + dataDir := t.TempDir() + store := fakes.New().Run + ctx := context.Background() + + // No terraform dir is seeded for this execution id. + id := makeAgedRunWithExecutions(t, ctx, store, dataDir, uuid.NewString()) + + web.SweepAssessments(ctx, store, dataDir, true, 30) + + _, err := store.Get(ctx, id) + assert.Error(t, err, "delete should succeed even though the Terraform dir was missing") +} diff --git a/internal/web/run_deletion.go b/internal/web/run_deletion.go index 9ad0bdb..af5b680 100644 --- a/internal/web/run_deletion.go +++ b/internal/web/run_deletion.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strings" "time" "github.com/IBM/simrun/internal/db" @@ -13,19 +14,22 @@ import ( // deleteRunWithArtifacts removes a run's database row (cascading to its // scenario_results via FK) and best-effort removes all of its on-disk -// artifacts: the per-run JSONL log file and every collected .ndjson file -// referenced by scenario_results.collected_log_path. +// artifacts: the per-run JSONL log file, every collected .ndjson file +// referenced by scenario_results.collected_log_path, and each scenario +// result's Terraform working directory at /terraform//. // -// Collected paths are read before the row is deleted, since the cascade -// removes the scenario_results that hold them. On-disk removal is best-effort: -// a missing or unremovable file logs a warning but does not fail the delete, -// so a leftover artifact never blocks reclaiming the database row. +// Collected paths and execution IDs are read before the row is deleted, since +// the cascade removes the scenario_results that hold them. On-disk removal is +// best-effort: a missing or unremovable file logs a warning but does not fail +// the delete, so a leftover artifact never blocks reclaiming the database row. // // Shared by manual delete (HandleDeleteRun) and the assessment-retention // sweeper so both reclaim the large collected .ndjson artifacts identically. func deleteRunWithArtifacts(ctx context.Context, runStore db.RunStore, dataDir string, runID uuid.UUID) error { - // Collect the .ndjson paths before deleting, while the rows still exist. + // Collect the .ndjson paths and execution IDs before deleting, while the + // rows still exist. var collected []string + var executionIDs []string results, err := runStore.GetScenarioResults(ctx, runID) if err != nil { // A failed lookup must not strand the row: log and continue to delete. @@ -36,6 +40,7 @@ func deleteRunWithArtifacts(ctx context.Context, runStore db.RunStore, dataDir s if res.CollectedLogPath != nil && *res.CollectedLogPath != "" { collected = append(collected, *res.CollectedLogPath) } + executionIDs = append(executionIDs, res.ExecutionID) } } @@ -62,6 +67,23 @@ func deleteRunWithArtifacts(ctx context.Context, runStore db.RunStore, dataDir s } } + // Best-effort: remove each scenario result's Terraform working directory at + // /terraform//. + base := filepath.Join(dataDir, "terraform") + for _, id := range executionIDs { + dir := filepath.Join(base, id) + // Skip blank/whitespace ids and any id that doesn't resolve to a direct + // child of terraform/. filepath.Join cleans the result, so separators, + // "." and ".." would otherwise let cleanup escape or wipe the base dir. + if strings.TrimSpace(id) == "" || filepath.Dir(dir) != base { + continue + } + if err := os.RemoveAll(dir); err != nil && !os.IsNotExist(err) { + logrus.WithError(err).WithField("path", dir).WithField("run_id", runID). + Warn("failed to remove Terraform working directory") + } + } + return nil } diff --git a/openspec/changes/archive/2026-06-22-purge-terraform-dirs/.openspec.yaml b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/.openspec.yaml new file mode 100644 index 0000000..38f7628 --- /dev/null +++ b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-22 diff --git a/openspec/changes/archive/2026-06-22-purge-terraform-dirs/design.md b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/design.md new file mode 100644 index 0000000..6882da7 --- /dev/null +++ b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/design.md @@ -0,0 +1,79 @@ +## Context + +`deleteRunWithArtifacts` (`internal/web/run_deletion.go:26`) is the single shared +deletion path used by both the manual delete handler (`HandleDeleteRun`) and the +assessment-retention sweeper (`SweepAssessments`). Today it loads the run's +scenario results, deletes the `runs` row (cascading to `scenario_results`), then +best-effort removes the JSONL run log and each collected `.ndjson` file. It never +references `/terraform/`. + +Each detonation creates a Terraform working directory at +`/terraform/` — see `terraform.Manager.Setup` building +`filepath.Join(m.baseDir, executionID)` (`internal/packs/terraform/manager.go:106`) +with `baseDir = /terraform`. The `executionID` is persisted per scenario +result as `scenario_results.execution_id` (`db.ScenarioResult.ExecutionID`, +`internal/db/runs.go:73`). The deletion function already loads these results into +`results`, so the execution IDs are in hand at exactly the right point — before the +cascade removes the rows. + +## Goals / Non-Goals + +**Goals:** +- Reclaim `/terraform//` for every scenario result of a + deleted run, via the shared deletion path so both manual delete and the sweeper + benefit with no duplicated logic. +- Keep removal best-effort and non-fatal, matching the existing JSONL/`.ndjson` + cleanup contract. +- Make the removal path-safe so it can never escape or wipe the `terraform/` base. + +**Non-Goals:** +- No change to how/where Terraform dirs are created during detonation. +- No backfill/orphan-sweep of pre-existing Terraform dirs whose runs are already + gone (those have no `execution_id` to key on anymore). Out of scope. +- No edits to the unarchived `assessment-retention` change's spec (flagged in the + proposal for sync at archive time, not modified here). + +## Decisions + +**Key the cleanup on `execution_id` from the already-loaded results.** +The function already calls `GetScenarioResults` to gather collected paths before +the cascade. Reuse that same loop to collect `/terraform/` +targets. No second query, no signature change — `dataDir` is already a parameter. +*Alternative considered:* glob `/terraform/` and match against run state. +Rejected — slower, racy with concurrent runs, and the rows are about to be deleted. + +**Use `os.RemoveAll`, guarded against unsafe `execution_id`.** +The work dir is a directory tree (state, `.terraform/` plugins), so `os.Remove` +is insufficient; `os.RemoveAll` is required. Before removing, skip any +`execution_id` that is empty/whitespace (`strings.TrimSpace`) or contains a path +separator (`strings.ContainsRune(id, os.PathSeparator)` / `filepath.Separator`, +also reject `/`). This prevents both `RemoveAll("/terraform/")` (which a +blank id would produce) and traversal. Execution IDs are UUIDs in practice, so the +guard rejects only malformed data. +*Alternative considered:* trust the value since it is server-generated. Rejected — +`fail loud / defense in depth`; a blank id wiping the whole base dir is too costly +to leave unguarded. + +**Best-effort, log-and-continue on error.** +Mirror the existing `.ndjson` block: on `os.RemoveAll` error that is not +`os.IsNotExist`, log a warning with `run_id` and the path, and continue. A leftover +or unremovable dir must never block reclaiming the DB row. + +## Risks / Trade-offs + +- [A scenario result carries an `execution_id` that points outside `terraform/`] + → The path-separator/blank guard rejects it; only `/terraform/` with + a clean single-segment id is ever removed. +- [Concurrent run reuses the same dir while deletion runs] → Execution IDs are + unique UUIDs per detonation, so a deleted run's dir is never an in-flight run's + dir; no locking needed. +- [Pre-existing orphaned dirs from before this change] → Not reclaimed by this + change (no row/id to key on). Acceptable; explicitly a non-goal. Operators can + remove them manually if desired. + +## Migration Plan + +Pure code change, no schema migration. Deploys with the binary. Rollback is +reverting the commit — no persisted state depends on the new behavior. After +deploy, deleting a run (or a sweeper tick) removes the Terraform dirs going +forward. diff --git a/openspec/changes/archive/2026-06-22-purge-terraform-dirs/proposal.md b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/proposal.md new file mode 100644 index 0000000..51d2f3a --- /dev/null +++ b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/proposal.md @@ -0,0 +1,48 @@ +## Why + +Deleting a run — whether manually via `DELETE /api/runs/{runId}` or automatically +via the assessment-retention sweeper — leaves the run's Terraform working +directories on disk forever. The shared `deleteRunWithArtifacts` path removes only +the DB row, the JSONL run log, and the collected `.ndjson` files; it never touches +`/terraform//`. These directories accumulate unbounded (one +per detonation, each holding providers, state, and plugins). The capability to +reclaim that disk simply does not exist in the code today. + +## What Changes + +- Extend the shared `deleteRunWithArtifacts` path so that, for every scenario + result of the run, it best-effort removes the run's Terraform working directory + at `/terraform//`, keyed by `scenario_results.execution_id`. +- Removal is best-effort and bounded: a missing or unremovable directory logs a + warning and never fails the delete (matching how JSONL/`.ndjson` cleanup behaves + today). A blank/whitespace `execution_id` or one containing a path separator is + skipped so the cleanup can never escape or wipe the `terraform/` base directory. +- Because both the manual delete handler and the retention sweeper call + `deleteRunWithArtifacts`, both reclaim Terraform directories identically with no + separate code path. + +## Capabilities + +### New Capabilities + + +### Modified Capabilities +- `runs`: The "Delete Run Cascades to Results and Log File" requirement gains + best-effort removal of the run's per-execution Terraform working directories, so + the enumerated artifact set removed on delete now includes + `/terraform//`. + +## Impact + +- **Code**: `internal/web/run_deletion.go` (`deleteRunWithArtifacts`) — adds the + Terraform-dir removal step using `dataDir` and each result's `ExecutionID` + (already loaded via `GetScenarioResults`). No new dependencies, no signature + change; the sweeper (`SweepAssessments`) and manual delete (`HandleDeleteRun`) + inherit the behavior automatically. +- **On-disk layout**: Relies on the detonator/terraform-manager convention that + the work dir is `/terraform/` (`internal/packs/terraform/manager.go`). +- **Flag for follow-up (not in scope here)**: the pending `assessment-retention` + change's "Assessment-Retention Sweeper" requirement enumerates the artifacts the + sweeper removes (row, JSONL, `.ndjson`) and should be kept in sync to mention + Terraform directories when that change is archived. Surfaced here rather than + edited, to avoid blending with an unarchived change's deltas. diff --git a/openspec/changes/archive/2026-06-22-purge-terraform-dirs/specs/runs/spec.md b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/specs/runs/spec.md new file mode 100644 index 0000000..4fa8d48 --- /dev/null +++ b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/specs/runs/spec.md @@ -0,0 +1,30 @@ +## MODIFIED Requirements + +### Requirement: Delete Run Cascades to Results and Log File +The system SHALL delete the `runs` row on `DELETE /api/runs/{runId}`, +cascade-delete all `scenario_results` rows via the FK, and best-effort +remove the run's on-disk artifacts: the run's JSONL log file, every collected +`.ndjson` file referenced by the run's `scenario_results.collected_log_path`, +and, for each scenario result with a non-empty `execution_id`, the run's +Terraform working directory at `/terraform//`. The +system SHALL skip Terraform-directory removal for any `execution_id` that is +blank/whitespace or contains a path separator, so cleanup can never escape or +remove the `/terraform/` base directory. Failure to remove any on-disk +artifact (log file, collected `.ndjson`, or Terraform directory) SHALL be logged +and SHALL NOT fail the request. + +#### Scenario: Successful delete +- **WHEN** a client deletes a run with 3 results +- **THEN** the `runs` row, all 3 `scenario_results` rows, and the JSONL log file are removed + +#### Scenario: Terraform directories removed +- **WHEN** a client deletes a run whose scenario results have execution IDs `E1` and `E2` +- **THEN** the directories `/terraform/E1/` and `/terraform/E2/` are removed along with the row and log file + +#### Scenario: Missing Terraform directory does not fail delete +- **WHEN** a run is deleted but its `/terraform//` directory is already gone +- **THEN** the delete succeeds and the missing directory is ignored + +#### Scenario: Unsafe execution id skipped +- **WHEN** a scenario result has a blank `execution_id` or one containing a path separator +- **THEN** no Terraform directory is removed for that result and the `/terraform/` base directory is left intact diff --git a/openspec/changes/archive/2026-06-22-purge-terraform-dirs/tasks.md b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/tasks.md new file mode 100644 index 0000000..1f5e2f2 --- /dev/null +++ b/openspec/changes/archive/2026-06-22-purge-terraform-dirs/tasks.md @@ -0,0 +1,16 @@ +## 1. Implementation + +- [x] 1.1 In `internal/web/run_deletion.go`, in the existing `GetScenarioResults` loop of `deleteRunWithArtifacts`, collect each result's `ExecutionID` alongside the collected `.ndjson` paths. +- [x] 1.2 After the collected-`.ndjson` removal block, add a best-effort Terraform-dir removal step: for each collected `execution_id`, skip it when `strings.TrimSpace(id) == ""` or it contains a path separator (`/` or `filepath.Separator`); otherwise `os.RemoveAll(filepath.Join(dataDir, "terraform", id))`, logging a warning with `run_id` and path on any error that is not `os.IsNotExist`. +- [x] 1.3 Update the `deleteRunWithArtifacts` doc comment to include the Terraform working directories in the enumerated artifact set it removes. + +## 2. Tests + +- [x] 2.1 Add a test (table-driven where it fits the existing style) verifying that deleting a run removes `/terraform//` for each scenario result's execution id, using a temp `dataDir` with seeded dirs. +- [x] 2.2 Add a test asserting a blank or path-separator-containing `execution_id` is skipped and the `/terraform/` base directory is left intact. +- [x] 2.3 Add a test asserting an already-missing Terraform directory does not fail the delete (best-effort contract). + +## 3. Verify + +- [x] 3.1 Run `go test ./internal/web/...` and confirm the new and existing deletion tests pass. +- [x] 3.2 Run `mise run lint` and confirm no new findings in `run_deletion.go`. diff --git a/openspec/specs/runs/spec.md b/openspec/specs/runs/spec.md index 8b3a584..8cfd630 100644 --- a/openspec/specs/runs/spec.md +++ b/openspec/specs/runs/spec.md @@ -77,13 +77,33 @@ as a known scaling gap. ### Requirement: Delete Run Cascades to Results and Log File The system SHALL delete the `runs` row on `DELETE /api/runs/{runId}`, cascade-delete all `scenario_results` rows via the FK, and best-effort -remove the run's JSONL log file. Failure to delete the log file SHALL -NOT fail the request. +remove the run's on-disk artifacts: the run's JSONL log file, every collected +`.ndjson` file referenced by the run's `scenario_results.collected_log_path`, +and, for each scenario result with a non-empty `execution_id`, the run's +Terraform working directory at `/terraform//`. The +system SHALL skip Terraform-directory removal for any `execution_id` that does +not resolve to a direct child of `/terraform/` — including blank/whitespace +ids, ids containing a path separator, and `.`/`..` — so cleanup can never escape or +remove the `/terraform/` base directory. Failure to remove any on-disk +artifact (log file, collected `.ndjson`, or Terraform directory) SHALL be logged +and SHALL NOT fail the request. #### Scenario: Successful delete - **WHEN** a client deletes a run with 3 results - **THEN** the `runs` row, all 3 `scenario_results` rows, and the JSONL log file are removed +#### Scenario: Terraform directories removed +- **WHEN** a client deletes a run whose scenario results have execution IDs `E1` and `E2` +- **THEN** the directories `/terraform/E1/` and `/terraform/E2/` are removed along with the row and log file + +#### Scenario: Missing Terraform directory does not fail delete +- **WHEN** a run is deleted but its `/terraform//` directory is already gone +- **THEN** the delete succeeds and the missing directory is ignored + +#### Scenario: Unsafe execution id skipped +- **WHEN** a scenario result has a blank `execution_id` or one containing a path separator +- **THEN** no Terraform directory is removed for that result and the `/terraform/` base directory is left intact + ### Requirement: Run Logs Are JSONL on Disk The system SHALL write run log entries as one JSON object per line to `/run-logs/.jsonl`. Each entry SHALL contain `ts`, `level`, diff --git a/pack/builtins.go b/pack/builtins.go index d804d27..10e275b 100644 --- a/pack/builtins.go +++ b/pack/builtins.go @@ -22,6 +22,13 @@ type builtinParam struct { rewrite func(simulationID string, defaultTagsValue map[string]string, file *hclwrite.File) bool } +// DefaultAWSRegion is the built-in default for the aws_region pack +// parameter. The terraform AWS provider is rewritten to use var.aws_region +// (see rewriteAWSRegion); the detonator pins the detonation's AWS_REGION to +// the same value so resources are acted on in the region terraform created +// them in. +const DefaultAWSRegion = "us-east-1" + // awsRegions is the canonical set of AWS regions exposed in the // aws_region enum. var awsRegions = []string{ @@ -81,7 +88,7 @@ var builtinParams = []builtinParam{ Name: "aws_region", Type: PackParamTypeString, Description: "AWS region used by the pack's AWS provider.", - Default: "us-east-1", + Default: DefaultAWSRegion, Enum: awsRegions, }, hclType: "string", diff --git a/web/frontend/src/lib/components/PackCard.svelte b/web/frontend/src/lib/components/PackCard.svelte index 8dc388d..30f6ab6 100644 --- a/web/frontend/src/lib/components/PackCard.svelte +++ b/web/frontend/src/lib/components/PackCard.svelte @@ -1,5 +1,6 @@ - + {#if $page.url.pathname === '/login'} {@render children()} diff --git a/web/frontend/src/routes/assessments/+page.svelte b/web/frontend/src/routes/assessments/+page.svelte index 71194b7..05623d1 100644 --- a/web/frontend/src/routes/assessments/+page.svelte +++ b/web/frontend/src/routes/assessments/+page.svelte @@ -1,6 +1,7 @@