From 79cba800eed11043d591b2ca67d08642137b0e9d Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 13 Aug 2026 03:18:55 +0000 Subject: [PATCH] Report comparison cleanup recovery state --- docs/cli.md | 10 ++-- docs/integrations/comparison-bundle.md | 5 ++ docs/internal/app-orchestration.md | 3 +- docs/internal/comparison-publication.md | 8 ++- docs/operations.md | 11 ++-- internal/app/comparison_test.go | 55 ++++++++++------- internal/cli/comparison_test.go | 8 +-- internal/cli/result.go | 15 ++++- internal/cli/result_test.go | 30 +++++++++- internal/comparison/publish.go | 56 ++++++++++++++---- internal/comparison/publish_test.go | 78 ++++++++++++++++++++++++- 11 files changed, 228 insertions(+), 51 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 995c3cb..fcb8146 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -154,16 +154,18 @@ when available. The safe error includes only a category and message: aggregate and unclassified application failures use `application`; cancellation uses `canceled`; deadlines use `deadline_exceeded`; prompt execution uses its published Promptkit category; destination failures use `destination_`; -and committed cleanup failures use `publication_cleanup`. It does not expose +and committed cleanup failures use `publication_cleanup` with a message that +states whether a complete prior bundle, partial remnants, or no prior bundle +remains, or that recovery state could not be inspected. It does not expose provider diagnostics, filesystem causes, or recovery paths. See the [comparison bundle contract](integrations/comparison-bundle.md) for durable artifact fields and failure invariants. If the bundle is published but cleanup of its replaced prior bundle fails, the summary still includes the published artifact paths and has status `failed`. -Its JSON error is `publication_cleanup` with the message `comparison published -but cleanup did not complete`; the returned command error identifies the -retained backup path for operator recovery. +Its JSON error is `publication_cleanup`; the returned command error identifies +a recovery path only when cleanup left a sibling behind. Only a reported +complete prior bundle is a rollback artifact. ## Flag Reference diff --git a/docs/integrations/comparison-bundle.md b/docs/integrations/comparison-bundle.md index ea210c5..8cb10e3 100644 --- a/docs/integrations/comparison-bundle.md +++ b/docs/integrations/comparison-bundle.md @@ -92,6 +92,11 @@ When replacing a recognized bundle, cancellation observed before the new bundle is installed preserves the prior bundle rather than committing the replacement. +Cleanup of a prior bundle occurs only after its replacement is committed and +does not affect the new bundle's compatibility. A cleanup error may identify a +complete recovery bundle, partial remnants, no remaining sibling, or an +uninspectable state; this operational state is not recorded in the manifest. + The manifest contains safe operational provenance, but `data-package.yml` and the generated Markdown can contain sensitive weather or location context. Do not assume these artifacts are safe for public distribution. Handle retention, diff --git a/docs/internal/app-orchestration.md b/docs/internal/app-orchestration.md index fff9128..d886dba 100644 --- a/docs/internal/app-orchestration.md +++ b/docs/internal/app-orchestration.md @@ -56,7 +56,8 @@ When publication has committed its new bundle, application results contain the absolute manifest, data-package, and successful report paths even if removal of the previous sibling backup then fails. That cleanup failure is still returned as an operational error rather than treating the new bundle as unpublished; -the returned error retains the recovery path and underlying filesystem cause. +the returned error identifies the observed recovery state and includes a path +only when cleanup left a sibling behind. ## Boundaries And Verification diff --git a/docs/internal/comparison-publication.md b/docs/internal/comparison-publication.md index 76179ed..b636dc7 100644 --- a/docs/internal/comparison-publication.md +++ b/docs/internal/comparison-publication.md @@ -31,9 +31,11 @@ not incorporate its basename. The new bundle is committed only after the staged directory has been installed at the target. From that point its artifact paths are authoritative: a failure to remove the retained sibling backup does not roll back the new bundle. -Publication returns an inspectable cleanup error with the absolute backup path -and underlying filesystem cause so an operator can recover or remove that -backup manually. +After a cleanup failure, publication inspects the sibling without masking the +original filesystem cause. Its inspectable cleanup result distinguishes a +complete recognized recovery bundle, partial remnants, an absent sibling, or +an uninspectable state. A recovery path is reported only when something +remains; only a complete recognized bundle is suitable for rollback recovery. The application preflights before prompt inspection and collection, then preflights again before publication. A cancellation or any failure before the diff --git a/docs/operations.md b/docs/operations.md index e647303..374e985 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -188,10 +188,13 @@ cancellation and pre-publication errors leave the prior destination unchanged. If a replacement commits but cleanup of its prior sibling backup fails, the new bundle remains valid and its artifact paths appear in the failed command -summary. The summary records a safe `publication_cleanup` error, while the -returned command error reports the retained backup path. Preserve that backup -until it has been inspected and cleaned up manually; do not remove the new -bundle to retry that cleanup. +summary. The summary records a safe `publication_cleanup` error that indicates +whether a complete prior bundle remains, only partial remnants remain, or no +prior bundle remains; it also identifies when the sibling cannot be inspected. +The returned command error includes a recovery path only when a sibling remains. +Preserve a complete recognized recovery bundle until it has been inspected and +cleaned up manually; partial remnants are not a rollback artifact. Do not +remove the new bundle to retry cleanup. Enable explicit debug capture only when content-rich Promptkit diagnostics are necessary. diff --git a/internal/app/comparison_test.go b/internal/app/comparison_test.go index 7109790..71907fd 100644 --- a/internal/app/comparison_test.go +++ b/internal/app/comparison_test.go @@ -87,27 +87,42 @@ func TestCompareDetailedPublishesPartialBundleAndReturnsAggregateError(t *testin } func TestCompareDetailedRetainsCommittedPathsWhenBackupCleanupFails(t *testing.T) { - bundle := generationBundle(t) - backupPath := filepath.Join(t.TempDir(), ".comparison-daily.backup-retained") - cleanupCause := errors.New("backup cleanup failed") - publish := func(context.Context, comparison.DestinationPlan, comparison.LogicalBundle) (comparison.PublicationResult, error) { - return comparison.PublicationResult{Committed: true, RetainedBackupPath: backupPath}, &comparison.PublicationCleanupError{RetainedBackupPath: backupPath, Err: cleanupCause} - } + for _, test := range []struct { + name string + state comparison.BackupRecoveryState + path bool + }{ + {name: "complete recovery bundle", state: comparison.BackupRecoveryComplete, path: true}, + {name: "partial remnants", state: comparison.BackupRecoveryPartial, path: true}, + {name: "absent backup", state: comparison.BackupRecoveryAbsent}, + } { + t.Run(test.name, func(t *testing.T) { + bundle := generationBundle(t) + recoveryPath := "" + if test.path { + recoveryPath = filepath.Join(t.TempDir(), ".comparison-daily.backup-recovery") + } + cleanupCause := errors.New("backup cleanup failed") + publish := func(context.Context, comparison.DestinationPlan, comparison.LogicalBundle) (comparison.PublicationResult, error) { + return comparison.PublicationResult{Committed: true, RecoveryState: test.state, RecoveryPath: recoveryPath}, &comparison.PublicationCleanupError{RecoveryState: test.state, RecoveryPath: recoveryPath, Err: cleanupCause} + } - result, err := compareDetailed(context.Background(), ComparisonRequest{ - Config: comparisonConfig(), Report: ReportDaily, ProfileIDs: []string{"weather-light", "weather-deep"}, - WorkingDir: t.TempDir(), Date: generationTime("2026-05-29T12:00:00-05:00"), - Clock: timeutil.FixedClock{Time: generationTime("2026-05-29T08:30:00-05:00")}, - Collector: &generationCollector{bundle: &bundle}, Executor: &generationExecutor{}, - }, publish) - var cleanupErr *comparison.PublicationCleanupError - if result == nil || !errors.As(err, &cleanupErr) || !errors.Is(err, cleanupCause) || cleanupErr.RetainedBackupPath != backupPath || !filepath.IsAbs(result.ManifestPath) || !filepath.IsAbs(result.DataPackagePath) { - t.Fatalf("CompareDetailed() result/error = %#v/%v", result, err) - } - for _, profile := range result.Results { - if profile.Status == comparison.StatusSucceeded && !filepath.IsAbs(profile.ReportPath) { - t.Fatalf("published profile result = %#v", profile) - } + result, err := compareDetailed(context.Background(), ComparisonRequest{ + Config: comparisonConfig(), Report: ReportDaily, ProfileIDs: []string{"weather-light", "weather-deep"}, + WorkingDir: t.TempDir(), Date: generationTime("2026-05-29T12:00:00-05:00"), + Clock: timeutil.FixedClock{Time: generationTime("2026-05-29T08:30:00-05:00")}, + Collector: &generationCollector{bundle: &bundle}, Executor: &generationExecutor{}, + }, publish) + var cleanupErr *comparison.PublicationCleanupError + if result == nil || !errors.As(err, &cleanupErr) || !errors.Is(err, cleanupCause) || cleanupErr.RecoveryState != test.state || cleanupErr.RecoveryPath != recoveryPath || !filepath.IsAbs(result.ManifestPath) || !filepath.IsAbs(result.DataPackagePath) { + t.Fatalf("CompareDetailed() result/error = %#v/%v", result, err) + } + for _, profile := range result.Results { + if profile.Status == comparison.StatusSucceeded && !filepath.IsAbs(profile.ReportPath) { + t.Fatalf("published profile result = %#v", profile) + } + } + }) } } diff --git a/internal/cli/comparison_test.go b/internal/cli/comparison_test.go index 345c51c..2b5cd79 100644 --- a/internal/cli/comparison_test.go +++ b/internal/cli/comparison_test.go @@ -223,9 +223,9 @@ func TestCompareCommandReportsCommittedBundleWhenCleanupFails(t *testing.T) { {Position: 1, ProfileID: "weather-light", ModelName: "light", Status: comparison.StatusSucceeded, ValidationStatus: promptexec.ValidationPassed, ReportPath: filepath.Join(outputDirectory, "01-weather-light.md")}, {Position: 2, ProfileID: "weather-deep", ModelName: "deep", Status: comparison.StatusSucceeded, ValidationStatus: promptexec.ValidationPassed, ReportPath: filepath.Join(outputDirectory, "02-weather-deep.md")}, }) - backupPath := filepath.Join(workingDir, ".comparison-daily.backup-retained") + recoveryPath := filepath.Join(workingDir, ".comparison-daily.backup-retained") cleanupCause := errors.New("filesystem cleanup detail") - cleanupErr := &comparison.PublicationCleanupError{RetainedBackupPath: backupPath, Err: cleanupCause} + cleanupErr := &comparison.PublicationCleanupError{RecoveryState: comparison.BackupRecoveryComplete, RecoveryPath: recoveryPath, Err: cleanupCause} runner := comparisonRunner(t, workingDir) runner.compareDetailed = func(context.Context, app.ComparisonRequest) (*app.ComparisonResult, error) { return result, cleanupErr @@ -240,10 +240,10 @@ func TestCompareCommandReportsCommittedBundleWhenCleanupFails(t *testing.T) { if err := json.Unmarshal(stdout.Bytes(), &summary); err != nil { t.Fatalf("decode summary: %v\n%s", err, stdout.String()) } - if summary.Status != summaryStatusFailed || summary.Error == nil || summary.Error.Category != "publication_cleanup" || summary.Error.Message != "comparison published but cleanup did not complete" || summary.ManifestPath != result.ManifestPath || summary.DataPackagePath != result.DataPackagePath || summary.Results[0].ReportPath == "" { + if summary.Status != summaryStatusFailed || summary.Error == nil || summary.Error.Category != "publication_cleanup" || summary.Error.Message != "comparison published but a complete prior bundle remains" || summary.ManifestPath != result.ManifestPath || summary.DataPackagePath != result.DataPackagePath || summary.Results[0].ReportPath == "" { t.Fatalf("summary = %#v", summary) } - for _, unsafe := range []string{cleanupCause.Error(), backupPath} { + for _, unsafe := range []string{cleanupCause.Error(), recoveryPath} { if strings.Contains(stdout.String(), unsafe) { t.Fatalf("summary contains unsafe recovery detail %q: %s", unsafe, stdout.String()) } diff --git a/internal/cli/result.go b/internal/cli/result.go index 1903e2b..3094662 100644 --- a/internal/cli/result.go +++ b/internal/cli/result.go @@ -225,7 +225,7 @@ func safeComparisonSummaryError(err error) *comparison.SafeError { } var cleanupErr *comparison.PublicationCleanupError if errors.As(err, &cleanupErr) { - safe := comparison.NewSafeError("publication_cleanup", "comparison published but cleanup did not complete") + safe := comparison.NewSafeError("publication_cleanup", publicationCleanupMessage(cleanupErr.RecoveryState)) return &safe } var destinationErr *comparison.DestinationError @@ -249,6 +249,19 @@ func safeComparisonSummaryError(err error) *comparison.SafeError { return &safe } +func publicationCleanupMessage(state comparison.BackupRecoveryState) string { + switch state { + case comparison.BackupRecoveryComplete: + return "comparison published but a complete prior bundle remains" + case comparison.BackupRecoveryPartial: + return "comparison published but partial cleanup remnants remain" + case comparison.BackupRecoveryAbsent: + return "comparison published but no prior bundle remains" + default: + return "comparison published but cleanup recovery state is unknown" + } +} + func comparisonAggregateErrorMessage(err error) (string, bool) { const prefix = "comparison completed with " const suffix = " failed profiles" diff --git a/internal/cli/result_test.go b/internal/cli/result_test.go index c2b7928..adf1093 100644 --- a/internal/cli/result_test.go +++ b/internal/cli/result_test.go @@ -179,12 +179,36 @@ func TestSafeComparisonSummaryErrorClassifiesWrappedFailures(t *testing.T) { message: "comparison destination preflight failed", }, { - name: "publication cleanup", + name: "complete cleanup recovery", err: fmt.Errorf("outer wrapper: %w", &comparison.PublicationCleanupError{ - RetainedBackupPath: "/tmp/" + unsafeDetail, Err: fmt.Errorf("%w: %s", context.Canceled, unsafeDetail), + RecoveryState: comparison.BackupRecoveryComplete, RecoveryPath: "/tmp/" + unsafeDetail, Err: fmt.Errorf("%w: %s", context.Canceled, unsafeDetail), }), category: "publication_cleanup", - message: "comparison published but cleanup did not complete", + message: "comparison published but a complete prior bundle remains", + }, + { + name: "partial cleanup remnants", + err: fmt.Errorf("outer wrapper: %w", &comparison.PublicationCleanupError{ + RecoveryState: comparison.BackupRecoveryPartial, RecoveryPath: "/tmp/" + unsafeDetail, Err: fmt.Errorf("%w: %s", context.Canceled, unsafeDetail), + }), + category: "publication_cleanup", + message: "comparison published but partial cleanup remnants remain", + }, + { + name: "absent cleanup recovery", + err: fmt.Errorf("outer wrapper: %w", &comparison.PublicationCleanupError{ + RecoveryState: comparison.BackupRecoveryAbsent, Err: fmt.Errorf("%w: %s", context.Canceled, unsafeDetail), + }), + category: "publication_cleanup", + message: "comparison published but no prior bundle remains", + }, + { + name: "unknown cleanup recovery", + err: fmt.Errorf("outer wrapper: %w", &comparison.PublicationCleanupError{ + RecoveryState: comparison.BackupRecoveryUnknown, Err: fmt.Errorf("%w: %s", context.Canceled, unsafeDetail), + }), + category: "publication_cleanup", + message: "comparison published but cleanup recovery state is unknown", }, { name: "unknown", diff --git a/internal/comparison/publish.go b/internal/comparison/publish.go index eab5eb8..37b7719 100644 --- a/internal/comparison/publish.go +++ b/internal/comparison/publish.go @@ -445,22 +445,44 @@ func unrecognizedBundleError(action string, err error) error { return fmt.Errorf("%w: %s: %v", ErrUnrecognizedBundle, action, err) } +// BackupRecoveryState describes what remains of a prior bundle after cleanup +// reports an error. +type BackupRecoveryState string + +const ( + BackupRecoveryComplete BackupRecoveryState = "complete" + BackupRecoveryPartial BackupRecoveryState = "partial" + BackupRecoveryAbsent BackupRecoveryState = "absent" + BackupRecoveryUnknown BackupRecoveryState = "unknown" +) + // PublicationResult describes the durable state of a publication attempt. type PublicationResult struct { - Committed bool - RetainedBackupPath string + Committed bool + RecoveryState BackupRecoveryState + RecoveryPath string } -// PublicationCleanupError reports that a committed bundle could not remove its -// prior sibling backup. The new bundle remains installed and the backup path -// is retained for operator recovery. +// PublicationCleanupError reports that a committed bundle could not completely +// remove its prior sibling backup. The new bundle remains installed; recovery +// fields describe the state observed after cleanup failed. type PublicationCleanupError struct { - RetainedBackupPath string - Err error + RecoveryState BackupRecoveryState + RecoveryPath string + Err error } func (err *PublicationCleanupError) Error() string { - return fmt.Sprintf("remove comparison backup %q: %v", err.RetainedBackupPath, err.Err) + switch err.RecoveryState { + case BackupRecoveryComplete: + return fmt.Sprintf("remove comparison backup %q: %v; complete recovery bundle remains", err.RecoveryPath, err.Err) + case BackupRecoveryPartial: + return fmt.Sprintf("remove comparison backup %q: %v; partial remnants remain", err.RecoveryPath, err.Err) + case BackupRecoveryAbsent: + return fmt.Sprintf("remove comparison backup: %v; no recovery bundle remains", err.Err) + default: + return fmt.Sprintf("remove comparison backup: %v; recovery state is unknown", err.Err) + } } func (err *PublicationCleanupError) Unwrap() error { @@ -558,12 +580,26 @@ func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, op } temporaryDirectory = "" if err := operations.removeAll(backupDirectory); err != nil { - cleanupErr := &PublicationCleanupError{RetainedBackupPath: backupDirectory, Err: err} - return PublicationResult{Committed: true, RetainedBackupPath: backupDirectory}, cleanupErr + recoveryState, recoveryPath := inspectBackupRecovery(backupDirectory) + cleanupErr := &PublicationCleanupError{RecoveryState: recoveryState, RecoveryPath: recoveryPath, Err: err} + return PublicationResult{Committed: true, RecoveryState: recoveryState, RecoveryPath: recoveryPath}, cleanupErr } return PublicationResult{Committed: true}, nil } +func inspectBackupRecovery(backupDirectory string) (BackupRecoveryState, string) { + if _, err := os.Lstat(backupDirectory); err != nil { + if errors.Is(err, os.ErrNotExist) { + return BackupRecoveryAbsent, "" + } + return BackupRecoveryUnknown, "" + } + if _, err := RecognizeBundle(backupDirectory); err == nil { + return BackupRecoveryComplete, backupDirectory + } + return BackupRecoveryPartial, backupDirectory +} + func authorizeMovedDestination(plan DestinationPlan, backupDirectory string) error { backupPlan, err := PlanDestination(plan.WorkingDirectory, backupDirectory, plan.Replace) if err != nil { diff --git a/internal/comparison/publish_test.go b/internal/comparison/publish_test.go index 2491971..511776a 100644 --- a/internal/comparison/publish_test.go +++ b/internal/comparison/publish_test.go @@ -577,7 +577,7 @@ func TestPublishReportsCommittedBundleWhenBackupCleanupFails(t *testing.T) { }, }) var cleanupErr *PublicationCleanupError - if !result.Committed || result.RetainedBackupPath != backupPath || !filepath.IsAbs(backupPath) || !errors.As(err, &cleanupErr) || cleanupErr.RetainedBackupPath != backupPath || !errors.Is(err, cleanupCause) { + if !result.Committed || result.RecoveryState != BackupRecoveryComplete || result.RecoveryPath != backupPath || !filepath.IsAbs(backupPath) || !errors.As(err, &cleanupErr) || cleanupErr.RecoveryState != BackupRecoveryComplete || cleanupErr.RecoveryPath != backupPath || !errors.Is(err, cleanupCause) { t.Fatalf("publish() result/error = %#v/%v", result, err) } if _, err := RecognizeBundle(target); err != nil { @@ -592,6 +592,82 @@ func TestPublishReportsCommittedBundleWhenBackupCleanupFails(t *testing.T) { } } +func TestPublishReportsPartialRecoveryAfterBackupCleanupFailure(t *testing.T) { + workingDirectory := t.TempDir() + target := filepath.Join(workingDirectory, "comparison-daily") + initialPlan, err := PlanDestination(workingDirectory, target, false) + if err != nil { + t.Fatal(err) + } + if _, err := Publish(context.Background(), initialPlan, testBundle()); err != nil { + t.Fatal(err) + } + plan, err := PlanDestination(workingDirectory, target, true) + if err != nil { + t.Fatal(err) + } + cleanupCause := errors.New("backup removal failed") + var backupPath string + result, err := publish(context.Background(), plan, testBundle(), publishOperations{ + rename: os.Rename, + removeAll: func(path string) error { + backupPath = path + if err := os.Remove(filepath.Join(path, ManifestFilename)); err != nil { + t.Fatal(err) + } + return cleanupCause + }, + }) + var cleanupErr *PublicationCleanupError + if !result.Committed || result.RecoveryState != BackupRecoveryPartial || result.RecoveryPath != backupPath || !errors.As(err, &cleanupErr) || cleanupErr.RecoveryState != BackupRecoveryPartial || cleanupErr.RecoveryPath != backupPath || !errors.Is(err, cleanupCause) { + t.Fatalf("publish() result/error = %#v/%v", result, err) + } + if _, err := RecognizeBundle(target); err != nil { + t.Fatalf("new bundle recognition error = %v", err) + } + if _, err := RecognizeBundle(backupPath); err == nil { + t.Fatal("partially removed backup was recognized") + } +} + +func TestPublishReportsAbsentRecoveryAfterBackupCleanupFailure(t *testing.T) { + workingDirectory := t.TempDir() + target := filepath.Join(workingDirectory, "comparison-daily") + initialPlan, err := PlanDestination(workingDirectory, target, false) + if err != nil { + t.Fatal(err) + } + if _, err := Publish(context.Background(), initialPlan, testBundle()); err != nil { + t.Fatal(err) + } + plan, err := PlanDestination(workingDirectory, target, true) + if err != nil { + t.Fatal(err) + } + cleanupCause := errors.New("backup removal failed") + var backupPath string + result, err := publish(context.Background(), plan, testBundle(), publishOperations{ + rename: os.Rename, + removeAll: func(path string) error { + backupPath = path + if err := os.RemoveAll(path); err != nil { + t.Fatal(err) + } + return cleanupCause + }, + }) + var cleanupErr *PublicationCleanupError + if !result.Committed || result.RecoveryState != BackupRecoveryAbsent || result.RecoveryPath != "" || !errors.As(err, &cleanupErr) || cleanupErr.RecoveryState != BackupRecoveryAbsent || cleanupErr.RecoveryPath != "" || !errors.Is(err, cleanupCause) { + t.Fatalf("publish() result/error = %#v/%v", result, err) + } + if _, err := RecognizeBundle(target); err != nil { + t.Fatalf("new bundle recognition error = %v", err) + } + if _, err := os.Lstat(backupPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("backup stat error = %v, want not exist", err) + } +} + func TestPublishRestoresExistingBundleAfterReplacementFailure(t *testing.T) { workingDirectory := t.TempDir() target := filepath.Join(workingDirectory, "comparison-daily")