Run Cleanup on a best-effort basis when a later trial stage fails

internal/orchestrator/orchestrator.go

@@ -3,6 +3,7 @@ package orchestrator
 import (
 	"context"
 	"fmt"
+	"log"
 	"time"
 
 	"github.com/urunc-dev/evaluation_suite/internal/plan"
@@ -147,19 +148,52 @@ func (o *Orchestrator) runTrial(
 	}
 
 	for i := 0; i < repetitions; i++ {
+		// prepared tracks whether the Prepare stage has succeeded, i.e.
+		// whether the adapter may own real resources (containers, tasks,
+		// netns, etc.) that Cleanup needs to tear down. cleanedUp tracks
+		// whether Cleanup has already run (as the last regular stage) so we
+		// don't invoke it a second time on the happy path.
+		prepared := false
+		cleanedUp := false
 
 		for _, stage := range stages {
 			// if the stage's experiment does not match the trial's experiment, skip it
 			if runtimeTC.Trial.ExperimentName != stage.experiment {
 				continue
 			}
 			stageResult, err := stage.fn(ctx, runtimeTC)
+			result.RuntimeStages = append(result.RuntimeStages, stageResult)
+
+			if stage.name == harnessruntime.StageCleanup {
+				cleanedUp = err == nil
+			}
+
 			if err != nil {
-				result.RuntimeStages = append(result.RuntimeStages, stageResult)
+				// A later stage failed after resources were created in
+				// Prepare. Run cleanup on a best-effort basis so a failed
+				// trial doesn't leak containers/VMs/netns on the host. A
+				// cleanup failure here is logged, not fatal: it must not
+				// mask the original stage error being returned below.
+				if prepared && !cleanedUp && stage.name != harnessruntime.StageCleanup {
+					for _, adapter := range adapters {
+						if adapter.ExperimentName() != stage.experiment {
+							continue
+						}
+						if _, cleanupErr := adapter.Cleanup(ctx, runtimeTC); cleanupErr != nil {
+							log.Printf(
+								"trial %s: cleanup after %s stage failure also failed: %v",
+								trial.ID, stage.name, cleanupErr,
+							)
+						}
+						break
+					}
+				}
 				return failTrial(result, stage.name, err)
 			}
 
-			result.RuntimeStages = append(result.RuntimeStages, stageResult)
+			if stage.name == harnessruntime.StagePrepare {
+				prepared = true
+			}
 		}
 	}
 

internal/orchestrator/orchestrator_test.go

@@ -0,0 +1,213 @@
+package orchestrator
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/urunc-dev/evaluation_suite/internal/plan"
+	harnessruntime "github.com/urunc-dev/evaluation_suite/internal/runtime"
+)
+
+// fakeAdapter is a minimal harnessruntime.Adapter used to drive runTrial
+// without needing a real containerd daemon. Each stage method records that
+// it ran and can be configured to fail.
+type fakeAdapter struct {
+	experiment string
+
+	failStage harnessruntime.Stage
+	failErr   error
+
+	cleanupErr error
+
+	calls []harnessruntime.Stage
+}
+
+func (f *fakeAdapter) ExperimentName() string { return f.experiment }
+
+func (f *fakeAdapter) stage(stage harnessruntime.Stage) (harnessruntime.StageResult, error) {
+	f.calls = append(f.calls, stage)
+	if f.failStage == stage {
+		return harnessruntime.StageResult{Stage: stage}, f.failErr
+	}
+	return harnessruntime.StageResult{Stage: stage}, nil
+}
+
+func (f *fakeAdapter) Prepare(ctx context.Context, tc harnessruntime.TrialContext) (harnessruntime.StageResult, error) {
+	return f.stage(harnessruntime.StagePrepare)
+}
+
+func (f *fakeAdapter) CreateTask(ctx context.Context, tc harnessruntime.TrialContext) (harnessruntime.StageResult, error) {
+	return f.stage(harnessruntime.StageCreate)
+}
+
+func (f *fakeAdapter) StartTask(ctx context.Context, tc harnessruntime.TrialContext) (harnessruntime.StageResult, error) {
+	return f.stage(harnessruntime.StageStart)
+}
+
+func (f *fakeAdapter) WaitReady(ctx context.Context, tc harnessruntime.TrialContext) (harnessruntime.StageResult, error) {
+	return f.stage(harnessruntime.StageWaitReady)
+}
+
+func (f *fakeAdapter) Stop(ctx context.Context, tc harnessruntime.TrialContext) (harnessruntime.StageResult, error) {
+	return f.stage(harnessruntime.StageStop)
+}
+
+func (f *fakeAdapter) DeleteTask(ctx context.Context, tc harnessruntime.TrialContext) (harnessruntime.StageResult, error) {
+	return f.stage(harnessruntime.StageDelete)
+}
+
+func (f *fakeAdapter) Cleanup(ctx context.Context, tc harnessruntime.TrialContext) (harnessruntime.StageResult, error) {
+	f.calls = append(f.calls, harnessruntime.StageCleanup)
+	if f.failStage == harnessruntime.StageCleanup {
+		return harnessruntime.StageResult{Stage: harnessruntime.StageCleanup}, f.failErr
+	}
+	if f.cleanupErr != nil {
+		return harnessruntime.StageResult{Stage: harnessruntime.StageCleanup}, f.cleanupErr
+	}
+	return harnessruntime.StageResult{Stage: harnessruntime.StageCleanup}, nil
+}
+
+func (f *fakeAdapter) GenerateResult(ctx context.Context, tc harnessruntime.TrialContext, results []harnessruntime.StageResult) (any, error) {
+	return nil, nil
+}
+
+func countStage(calls []harnessruntime.Stage, stage harnessruntime.Stage) int {
+	n := 0
+	for _, c := range calls {
+		if c == stage {
+			n++
+		}
+	}
+	return n
+}
+
+func trial(experiment string) plan.Trial {
+	return plan.Trial{
+		ID:             "trial-1",
+		ExperimentName: experiment,
+		RuntimeName:    "urunc",
+	}
+}
+
+// TestRunTrialCleansUpAfterStageFailure verifies that when a stage fails
+// after Prepare has already created resources, the orchestrator still calls
+// Cleanup on the adapter before returning the (unmasked) original error.
+// This is the regression test for issue #9: a failed trial used to leak
+// containers/VMs/netns because Cleanup was only reachable via the normal
+// (all-stages-succeed) path.
+func TestRunTrialCleansUpAfterStageFailure(t *testing.T) {
+	stagesToFail := []harnessruntime.Stage{
+		harnessruntime.StageCreate,
+		harnessruntime.StageStart,
+		harnessruntime.StageWaitReady,
+		harnessruntime.StageStop,
+		harnessruntime.StageDelete,
+	}
+
+	for _, failStage := range stagesToFail {
+		failStage := failStage
+		t.Run(string(failStage), func(t *testing.T) {
+			wantErr := errors.New("boom")
+			adapter := &fakeAdapter{experiment: "lifecycle", failStage: failStage, failErr: wantErr}
+
+			o := New(func(plan.Trial) (harnessruntime.Adapter, error) { return adapter, nil })
+
+			result := o.runTrial(context.Background(), trial("lifecycle"), Options{})
+
+			if result.Status != TrialStatusFailed {
+				t.Fatalf("status = %s, want %s", result.Status, TrialStatusFailed)
+			}
+			if result.FailedStage != failStage {
+				t.Fatalf("failedStage = %s, want %s", result.FailedStage, failStage)
+			}
+			if result.Error != wantErr.Error() {
+				t.Fatalf("error = %q, want %q", result.Error, wantErr.Error())
+			}
+			if countStage(adapter.calls, harnessruntime.StageCleanup) != 1 {
+				t.Fatalf("Cleanup calls = %d, want 1 (calls: %v)", countStage(adapter.calls, harnessruntime.StageCleanup), adapter.calls)
+			}
+		})
+	}
+}
+
+// TestRunTrialSkipsCleanupWhenPrepareFails verifies that Cleanup is not
+// invoked when Prepare itself fails, since no resources were created yet
+// (calling Cleanup in that state can be unsafe for adapters that assume
+// Prepare has already initialized adapter state).
+func TestRunTrialSkipsCleanupWhenPrepareFails(t *testing.T) {
+	wantErr := errors.New("prepare failed")
+	adapter := &fakeAdapter{experiment: "lifecycle", failStage: harnessruntime.StagePrepare, failErr: wantErr}
+
+	o := New(func(plan.Trial) (harnessruntime.Adapter, error) { return adapter, nil })
+
+	result := o.runTrial(context.Background(), trial("lifecycle"), Options{})
+
+	if result.Status != TrialStatusFailed {
+		t.Fatalf("status = %s, want %s", result.Status, TrialStatusFailed)
+	}
+	if countStage(adapter.calls, harnessruntime.StageCleanup) != 0 {
+		t.Fatalf("Cleanup calls = %d, want 0 (calls: %v)", countStage(adapter.calls, harnessruntime.StageCleanup), adapter.calls)
+	}
+}
+
+// TestRunTrialCleanupFailureDoesNotMaskOriginalError verifies that a
+// Cleanup failure is surfaced only via logging: the trial's reported error
+// and failed stage must still reflect the original stage failure.
+func TestRunTrialCleanupFailureDoesNotMaskOriginalError(t *testing.T) {
+	wantErr := errors.New("create failed")
+	adapter := &fakeAdapter{
+		experiment: "lifecycle",
+		failStage:  harnessruntime.StageCreate,
+		failErr:    wantErr,
+		cleanupErr: errors.New("cleanup also failed"),
+	}
+
+	o := New(func(plan.Trial) (harnessruntime.Adapter, error) { return adapter, nil })
+
+	result := o.runTrial(context.Background(), trial("lifecycle"), Options{})
+
+	if result.FailedStage != harnessruntime.StageCreate {
+		t.Fatalf("failedStage = %s, want %s", result.FailedStage, harnessruntime.StageCreate)
+	}
+	if result.Error != wantErr.Error() {
+		t.Fatalf("error = %q, want original stage error %q (must not be masked by cleanup failure)", result.Error, wantErr.Error())
+	}
+	if countStage(adapter.calls, harnessruntime.StageCleanup) != 1 {
+		t.Fatalf("Cleanup calls = %d, want 1", countStage(adapter.calls, harnessruntime.StageCleanup))
+	}
+}
+
+// TestRunTrialCallsCleanupExactlyOnceOnSuccess verifies the happy path is
+// unchanged: Cleanup runs once, as the final regular stage.
+func TestRunTrialCallsCleanupExactlyOnceOnSuccess(t *testing.T) {
+	adapter := &fakeAdapter{experiment: "lifecycle"}
+
+	o := New(func(plan.Trial) (harnessruntime.Adapter, error) { return adapter, nil })
+
+	result := o.runTrial(context.Background(), trial("lifecycle"), Options{})
+
+	if result.Status != TrialStatusSuccess {
+		t.Fatalf("status = %s, want %s", result.Status, TrialStatusSuccess)
+	}
+	if countStage(adapter.calls, harnessruntime.StageCleanup) != 1 {
+		t.Fatalf("Cleanup calls = %d, want 1 (calls: %v)", countStage(adapter.calls, harnessruntime.StageCleanup), adapter.calls)
+	}
+}
+
+// TestRunTrialSkipsAdapterForOtherExperiments verifies adapters whose
+// ExperimentName doesn't match the trial are never invoked.
+func TestRunTrialSkipsAdapterForOtherExperiments(t *testing.T) {
+	adapter := &fakeAdapter{experiment: "storage"}
+
+	o := New(func(plan.Trial) (harnessruntime.Adapter, error) { return adapter, nil })
+
+	result := o.runTrial(context.Background(), trial("lifecycle"), Options{})
+
+	if result.Status != TrialStatusSuccess {
+		t.Fatalf("status = %s, want %s", result.Status, TrialStatusSuccess)
+	}
+	if len(adapter.calls) != 0 {
+		t.Fatalf("calls = %v, want none", adapter.calls)
+	}
+}