eric/weatherreporter
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

Report committed comparison cleanup failures

Browse Source
This commit is contained in:
Eric Rakestraw
2026-08-02 13:23:18 +00:00
parent 606b4423f1
commit acb476a142
9 changed files with 206 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
internal/app/comparison.go
View File

@@ -65,6 +65,8 @@ type ComparisonProfileResult struct {
Error *comparison.SafeError
}
var publishComparison = comparison.Publish
// CompareDetailed assembles, executes, and atomically publishes a comparison
// bundle. Profile failures publish a complete partial bundle; all other
// failures leave the destination untouched.
@@ -141,13 +143,16 @@ func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonRes
if err != nil {
return result, fmt.Errorf("re-preflight comparison destination: %w", err)
}
if err := comparison.Publish(ctx, publicationPlan, bundle); err != nil {
publication, err := publishComparison(ctx, publicationPlan, bundle)
if publication.Committed {
result.OutputDirectory = publicationPlan.Target
result.ManifestPath = filepath.Join(publicationPlan.Target, comparison.ManifestFilename)
result.DataPackagePath = filepath.Join(publicationPlan.Target, comparison.DataPackageFilename)
copyComparisonOutcomes(result, executed.Outcomes, true)
}
if err != nil {
return result, fmt.Errorf("publish comparison bundle: %w", err)
}
result.OutputDirectory = publicationPlan.Target
result.ManifestPath = filepath.Join(publicationPlan.Target, comparison.ManifestFilename)
result.DataPackagePath = filepath.Join(publicationPlan.Target, comparison.DataPackageFilename)
copyComparisonOutcomes(result, executed.Outcomes, true)
if result.Failed > 0 {
return result, fmt.Errorf("comparison completed with %d failed profiles", result.Failed)

27
internal/app/comparison_test.go
View File

@@ -85,6 +85,33 @@ 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")
originalPublish := publishComparison
publishComparison = func(context.Context, comparison.DestinationPlan, comparison.LogicalBundle) (comparison.PublicationResult, error) {
return comparison.PublicationResult{Committed: true, RetainedBackupPath: backupPath}, &comparison.PublicationCleanupError{RetainedBackupPath: backupPath, Err: cleanupCause}
}
t.Cleanup(func() { publishComparison = originalPublish })
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{},
})
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)
}
}
}
func TestCompareDetailedPreflightsBeforePromptOrCollection(t *testing.T) {
invalidDestination := filepath.Join(t.TempDir(), "not-a-directory")
if err := os.WriteFile(invalidDestination, []byte("x"), 0o600); err != nil {

35
internal/cli/comparison_test.go
View File

@@ -212,6 +212,41 @@ func TestCompareCommandWritesStructuredPartialFailure(t *testing.T) {
}
}
func TestCompareCommandReportsCommittedBundleWhenCleanupFails(t *testing.T) {
workingDir := t.TempDir()
configPath := comparisonConfigPath(t, "weather_api:\n base_url: https://weather.api.example.com/\n")
outputDirectory := filepath.Join(workingDir, "comparison-daily")
result := comparisonResult(outputDirectory, []app.ComparisonProfileResult{
{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")
cleanupCause := errors.New("filesystem cleanup detail")
cleanupErr := &comparison.PublicationCleanupError{RetainedBackupPath: backupPath, Err: cleanupCause}
runner := comparisonRunner(t, workingDir)
runner.compareDetailed = func(context.Context, app.ComparisonRequest) (*app.ComparisonResult, error) {
return result, cleanupErr
}
var stdout, stderr bytes.Buffer
err := runner.Run(context.Background(), []string{"compare", "daily", "--date", "2026-05-29", "--profile", "weather-light", "--profile", "weather-deep", "--config", configPath}, &stdout, &stderr)
if !errors.Is(err, cleanupCause) || stderr.Len() != 0 {
t.Fatalf("Run() error/stderr = %v/%q", err, stderr.String())
}
var summary comparisonSummary
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.Message != "comparison did not complete" || summary.ManifestPath != result.ManifestPath || summary.DataPackagePath != result.DataPackagePath || summary.Results[0].ReportPath == "" {
t.Fatalf("summary = %#v", summary)
}
for _, unsafe := range []string{cleanupCause.Error(), backupPath} {
if strings.Contains(stdout.String(), unsafe) {
t.Fatalf("summary contains unsafe recovery detail %q: %s", unsafe, stdout.String())
}
}
}
func TestCompareCommandWritesSuccessForDefaultAndExplicitDestinations(t *testing.T) {
workingDir := t.TempDir()
configPath := comparisonConfigPath(t, "weather_api:\n base_url: https://weather.api.example.com/\noutput:\n directory: configured-reports\n")

74
internal/comparison/publish.go
View File

@@ -262,43 +262,72 @@ func unrecognizedBundleError(action string, err error) error {
return fmt.Errorf("%w: %s: %v", ErrUnrecognizedBundle, action, err)
}
// PublicationResult describes the durable state of a publication attempt.
type PublicationResult struct {
Committed bool
RetainedBackupPath 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.
type PublicationCleanupError struct {
RetainedBackupPath string
Err error
}
func (err *PublicationCleanupError) Error() string {
return fmt.Sprintf("remove comparison backup %q: %v", err.RetainedBackupPath, err.Err)
}
func (err *PublicationCleanupError) Unwrap() error {
return err.Err
}
// Publish writes a complete logical bundle through a private sibling directory
// and atomically installs it at a preflighted destination.
func Publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle) error {
return publish(ctx, plan, bundle, publishOperations{rename: os.Rename})
func Publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle) (PublicationResult, error) {
return publish(ctx, plan, bundle, publishOperations{rename: os.Rename, removeAll: os.RemoveAll})
}
type publishOperations struct {
rename func(string, string) error
removeAll func(string) error
beforeCommit func()
}
func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, operations publishOperations) error {
func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, operations publishOperations) (PublicationResult, error) {
if operations.rename == nil {
operations.rename = os.Rename
}
if operations.removeAll == nil {
operations.removeAll = os.RemoveAll
}
if err := bundle.Validate(); err != nil {
return fmt.Errorf("validate comparison bundle: %w", err)
return PublicationResult{}, fmt.Errorf("validate comparison bundle: %w", err)
}
manifestData, err := EncodeManifest(bundle.Manifest)
if err != nil {
return err
return PublicationResult{}, err
}
if err := ctx.Err(); err != nil {
return err
return PublicationResult{}, err
}
plan, err = PlanDestination(plan.WorkingDirectory, plan.Target, plan.Replace)
if err != nil {
return err
return PublicationResult{}, err
}
if err := os.MkdirAll(filepath.Dir(plan.Target), 0o755); err != nil {
return fmt.Errorf("create comparison destination parent %q: %w", filepath.Dir(plan.Target), err)
return PublicationResult{}, fmt.Errorf("create comparison destination parent %q: %w", filepath.Dir(plan.Target), err)
}
temporaryDirectory, err := os.MkdirTemp(filepath.Dir(plan.Target), "."+filepath.Base(plan.Target)+".staging-")
if err != nil {
return fmt.Errorf("create comparison staging directory: %w", err)
return PublicationResult{}, fmt.Errorf("create comparison staging directory: %w", err)
}
if err := os.Chmod(temporaryDirectory, 0o700); err != nil {
os.RemoveAll(temporaryDirectory)
return fmt.Errorf("secure comparison staging directory: %w", err)
return PublicationResult{}, fmt.Errorf("secure comparison staging directory: %w", err)
}
defer func() {
if temporaryDirectory != "" {
@@ -307,45 +336,46 @@ func publish(ctx context.Context, plan DestinationPlan, bundle LogicalBundle, op
}()
if err := writeLogicalBundle(ctx, temporaryDirectory, bundle, manifestData); err != nil {
return err
return PublicationResult{}, err
}
currentPlan, err := PlanDestination(plan.WorkingDirectory, plan.Target, plan.Replace)
if err != nil {
return err
return PublicationResult{}, err
}
if operations.beforeCommit != nil {
operations.beforeCommit()
}
if err := ctx.Err(); err != nil {
return err
return PublicationResult{}, err
}
if currentPlan.state == destinationAbsent {
if err := operations.rename(temporaryDirectory, currentPlan.Target); err != nil {
return fmt.Errorf("publish comparison bundle to %q: %w", currentPlan.Target, err)
return PublicationResult{}, fmt.Errorf("publish comparison bundle to %q: %w", currentPlan.Target, err)
}
temporaryDirectory = ""
return nil
return PublicationResult{Committed: true}, nil
}
backupDirectory, err := uniqueSiblingPath(filepath.Dir(currentPlan.Target), "."+filepath.Base(currentPlan.Target)+".backup-")
if err != nil {
return err
return PublicationResult{}, err
}
if err := operations.rename(currentPlan.Target, backupDirectory); err != nil {
return fmt.Errorf("back up comparison destination %q: %w", currentPlan.Target, err)
return PublicationResult{}, fmt.Errorf("back up comparison destination %q: %w", currentPlan.Target, err)
}
if err := authorizeMovedDestination(currentPlan, backupDirectory); err != nil {
return restoreMovedDestination(operations, backupDirectory, currentPlan.Target, err)
return PublicationResult{}, restoreMovedDestination(operations, backupDirectory, currentPlan.Target, err)
}
if err := operations.rename(temporaryDirectory, currentPlan.Target); err != nil {
return restoreMovedDestination(operations, backupDirectory, currentPlan.Target, fmt.Errorf("replace comparison destination %q: %w", currentPlan.Target, err))
return PublicationResult{}, restoreMovedDestination(operations, backupDirectory, currentPlan.Target, fmt.Errorf("replace comparison destination %q: %w", currentPlan.Target, err))
}
temporaryDirectory = ""
if err := os.RemoveAll(backupDirectory); err != nil {
return fmt.Errorf("remove comparison backup %q: %w", backupDirectory, err)
if err := operations.removeAll(backupDirectory); err != nil {
cleanupErr := &PublicationCleanupError{RetainedBackupPath: backupDirectory, Err: err}
return PublicationResult{Committed: true, RetainedBackupPath: backupDirectory}, cleanupErr
}
return nil
return PublicationResult{Committed: true}, nil
}
func authorizeMovedDestination(plan DestinationPlan, backupDirectory string) error {

71
internal/comparison/publish_test.go
View File

@@ -275,7 +275,7 @@ func TestPublishReauthorizesMovedDestination(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if err := Publish(context.Background(), plan, testBundle()); err != nil {
if _, err := Publish(context.Background(), plan, testBundle()); err != nil {
t.Fatal(err)
}
},
@@ -303,7 +303,7 @@ func TestPublishReauthorizesMovedDestination(t *testing.T) {
if err != nil {
t.Fatal(err)
}
err = publish(context.Background(), plan, testBundle(), publishOperations{
_, err = publish(context.Background(), plan, testBundle(), publishOperations{
rename: os.Rename,
beforeCommit: func() {
if err := os.RemoveAll(target); err != nil {
@@ -333,7 +333,7 @@ func TestPublishRetainsUnauthorizedMovedDestinationWhenRestoreFails(t *testing.T
}
var backupPath string
calls := 0
err = publish(context.Background(), plan, testBundle(), publishOperations{
_, err = publish(context.Background(), plan, testBundle(), publishOperations{
rename: func(oldPath, newPath string) error {
calls++
if calls == 1 {
@@ -376,7 +376,7 @@ func TestPublishRetainsMovedDestinationWhenTargetReappears(t *testing.T) {
t.Fatal(err)
}
var backupPath string
err = publish(context.Background(), plan, testBundle(), publishOperations{
_, err = publish(context.Background(), plan, testBundle(), publishOperations{
rename: func(oldPath, newPath string) error {
if backupPath != "" {
return os.Rename(oldPath, newPath)
@@ -413,7 +413,7 @@ func TestPublishWritesAndReplacesBundle(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if err := Publish(context.Background(), plan, testBundle()); err != nil {
if _, err := Publish(context.Background(), plan, testBundle()); err != nil {
t.Fatalf("Publish() error = %v", err)
}
if _, err := RecognizeBundle(target); err != nil {
@@ -433,7 +433,7 @@ func TestPublishWritesAndReplacesBundle(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if err := Publish(context.Background(), plan, next); err != nil {
if _, err := Publish(context.Background(), plan, next); err != nil {
t.Fatalf("Publish(replace) error = %v", err)
}
if _, err := os.Stat(filepath.Join(target, "01-weather-light.md")); !errors.Is(err, os.ErrNotExist) {
@@ -454,7 +454,7 @@ func TestPublishReplacesEmptyDirectory(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if err := Publish(context.Background(), plan, testBundle()); err != nil {
if _, err := Publish(context.Background(), plan, testBundle()); err != nil {
t.Fatalf("Publish() error = %v", err)
}
if _, err := RecognizeBundle(target); err != nil {
@@ -462,6 +462,49 @@ func TestPublishReplacesEmptyDirectory(t *testing.T) {
}
}
func TestPublishReportsCommittedBundleWhenBackupCleanupFails(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)
}
previous := directorySnapshot(t, target)
next := testBundle()
next.Reports[0].Markdown = []byte("# Next\n")
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, next, publishOperations{
rename: os.Rename,
removeAll: func(path string) error {
backupPath = path
return cleanupCause
},
})
var cleanupErr *PublicationCleanupError
if !result.Committed || result.RetainedBackupPath != backupPath || !filepath.IsAbs(backupPath) || !errors.As(err, &cleanupErr) || cleanupErr.RetainedBackupPath != 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 retained := directorySnapshot(t, backupPath); !equalSnapshots(previous, retained) {
t.Fatalf("retained backup = %#v, want %#v", retained, previous)
}
manifestData, err := os.ReadFile(filepath.Join(target, ManifestFilename))
if err != nil || strings.Contains(string(manifestData), backupPath) {
t.Fatalf("manifest/backup path = %q/%q, error = %v", manifestData, backupPath, err)
}
}
func TestPublishRestoresExistingBundleAfterReplacementFailure(t *testing.T) {
workingDirectory := t.TempDir()
target := filepath.Join(workingDirectory, "comparison-daily")
@@ -470,7 +513,7 @@ func TestPublishRestoresExistingBundleAfterReplacementFailure(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if err := Publish(context.Background(), initialPlan, initial); err != nil {
if _, err := Publish(context.Background(), initialPlan, initial); err != nil {
t.Fatal(err)
}
before := directorySnapshot(t, target)
@@ -482,14 +525,14 @@ func TestPublishRestoresExistingBundleAfterReplacementFailure(t *testing.T) {
t.Fatal(err)
}
calls := 0
err = publish(context.Background(), plan, next, publishOperations{rename: func(oldPath, newPath string) error {
publication, err := publish(context.Background(), plan, next, publishOperations{rename: func(oldPath, newPath string) error {
calls++
if calls == 2 {
return errors.New("replace failed")
}
return os.Rename(oldPath, newPath)
}})
if err == nil {
if publication.Committed || err == nil {
t.Fatal("publish() succeeded despite replacement failure")
}
after := directorySnapshot(t, target)
@@ -512,7 +555,7 @@ func TestPublishRetainsBackupWhenRestorationFails(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if err := Publish(context.Background(), initialPlan, testBundle()); err != nil {
if _, err := Publish(context.Background(), initialPlan, testBundle()); err != nil {
t.Fatal(err)
}
plan, err := PlanDestination(workingDirectory, target, true)
@@ -521,7 +564,7 @@ func TestPublishRetainsBackupWhenRestorationFails(t *testing.T) {
}
var backupPath string
calls := 0
err = publish(context.Background(), plan, testBundle(), publishOperations{rename: func(oldPath, newPath string) error {
_, err = publish(context.Background(), plan, testBundle(), publishOperations{rename: func(oldPath, newPath string) error {
calls++
if calls == 1 {
backupPath = newPath
@@ -547,7 +590,7 @@ func TestPublishCancellationBeforeCommitLeavesNoDestination(t *testing.T) {
t.Fatal(err)
}
ctx, cancel := context.WithCancel(context.Background())
if err := publish(ctx, plan, testBundle(), publishOperations{rename: os.Rename, beforeCommit: cancel}); !errors.Is(err, context.Canceled) {
if publication, err := publish(ctx, plan, testBundle(), publishOperations{rename: os.Rename, beforeCommit: cancel}); publication.Committed || !errors.Is(err, context.Canceled) {
t.Fatalf("Publish() error = %v, want context cancellation", err)
}
if _, err := os.Lstat(target); !errors.Is(err, os.ErrNotExist) {
@@ -572,7 +615,7 @@ func publishTestBundle(t *testing.T, bundle LogicalBundle) string {
if err != nil {
t.Fatal(err)
}
if err := Publish(context.Background(), plan, bundle); err != nil {
if _, err := Publish(context.Background(), plan, bundle); err != nil {
t.Fatal(err)
}
return target