From ef473565410fa183f804fa075cc43e700008b3e2 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 2 Nov 2026 03:24:57 +0000 Subject: [PATCH] Report temporary metadata cleanup failures --- cmd/releasekit/internal/cli/receipt.go | 10 ++- cmd/releasekit/internal/cli/receipt_test.go | 56 +++++++++++++++++ docs/roadmap/implementation.md | 2 +- internal/git/source_test.go | 25 ++++++++ internal/git/tag.go | 11 +++- internal/tempdir/tempdir.go | 49 ++++++++------- internal/tempdir/tempdir_test.go | 69 +++++++++++++++++++++ 7 files changed, 197 insertions(+), 25 deletions(-) diff --git a/cmd/releasekit/internal/cli/receipt.go b/cmd/releasekit/internal/cli/receipt.go index b8c26ab..60ae494 100644 --- a/cmd/releasekit/internal/cli/receipt.go +++ b/cmd/releasekit/internal/cli/receipt.go @@ -49,6 +49,10 @@ type BuildReceipt struct { } func WriteBuildReceipt(outputDir string, receipt BuildReceipt) (string, error) { + return writeBuildReceipt(outputDir, receipt, os.Remove) +} + +func writeBuildReceipt(outputDir string, receipt BuildReceipt, remove func(string) error) (resultPath string, resultErr error) { resolvedDir, err := filepath.Abs(outputDir) if err != nil { return "", fmt.Errorf("resolve build receipt directory: %w", err) @@ -67,7 +71,11 @@ func WriteBuildReceipt(outputDir string, receipt BuildReceipt) (string, error) { return "", fmt.Errorf("create temporary build receipt: %w", err) } temporaryPath := temporary.Name() - defer os.Remove(temporaryPath) + defer func() { + if err := remove(temporaryPath); err != nil && !errors.Is(err, os.ErrNotExist) { + resultErr = errors.Join(resultErr, fmt.Errorf("remove temporary build receipt %q: %w", temporaryPath, err)) + } + }() if _, err := temporary.Write(contents); err != nil { _ = temporary.Close() return "", fmt.Errorf("write build receipt: %w", err) diff --git a/cmd/releasekit/internal/cli/receipt_test.go b/cmd/releasekit/internal/cli/receipt_test.go index 2a595a8..914d92a 100644 --- a/cmd/releasekit/internal/cli/receipt_test.go +++ b/cmd/releasekit/internal/cli/receipt_test.go @@ -4,12 +4,68 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "os" "path/filepath" "strings" "testing" ) +func TestWriteBuildReceiptReportsTemporaryCleanup(t *testing.T) { + for _, mode := range []string{"renamed", "cleanup failure", "install and cleanup failure"} { + t.Run(mode, func(t *testing.T) { + repository := t.TempDir() + output := t.TempDir() + name := "app-v1.2.3-linux-amd64" + receipt := BuildReceipt{ + SchemaVersion: BuildReceiptSchemaVersion, RepositoryDir: repository, ModuleDir: repository, + ModulePath: "example.com/receipt", Tag: "v1.2.3", CandidateCommit: strings.Repeat("a", 40), + Version: "v1.2.3", MainPackage: ".", BinaryName: "app", Compiler: "go fixture", + Targets: []ReceiptTarget{{OS: "linux", Arch: "amd64"}}, + Artifacts: []ReceiptArtifact{{Path: filepath.Join(output, name), Filename: name, Target: ReceiptTarget{OS: "linux", Arch: "amd64"}, SHA256: strings.Repeat("a", 64)}}, + } + if mode == "install and cleanup failure" { + blocked := filepath.Join(output, BuildReceiptName) + if err := os.Mkdir(blocked, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(blocked, "keep"), nil, 0o600); err != nil { + t.Fatal(err) + } + } + cleanupErr := errors.New("cleanup failed") + var temporaryPath string + remove := func(path string) error { + temporaryPath = path + err := os.Remove(path) + if mode == "renamed" { + return err + } + return cleanupErr + } + path, err := writeBuildReceipt(output, receipt, remove) + if mode == "renamed" { + if err != nil || path != filepath.Join(output, BuildReceiptName) { + t.Fatalf("renamed receipt = %q, %v", path, err) + } + return + } + if !errors.Is(err, cleanupErr) || temporaryPath == "" || !strings.Contains(err.Error(), temporaryPath) { + t.Fatalf("cleanup path = %q, error = %v", temporaryPath, err) + } + if mode == "cleanup failure" && path != filepath.Join(output, BuildReceiptName) { + t.Fatalf("installed receipt path = %q", path) + } + if mode == "install and cleanup failure" { + var pathErr *os.PathError + if !errors.As(err, &pathErr) { + t.Fatalf("install cause missing: %v", err) + } + } + }) + } +} + func TestBuildReceiptRoundTripBindsRootAndNestedModuleIdentity(t *testing.T) { for _, test := range []struct { name string diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 9a2caaf..5c5610a 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -45,7 +45,7 @@ the agent policy; an intermediate compilation dependency is recorded accurately. | 7 | A05 | Complete — real-repository inspection failures preserve the local ref, available object ID, and Git cause without a push. | | 8 | A06, A14 | Complete — public build and prebuilt-delivery checks and CLI receipt/partial-artifact regressions passed. | | 9 | A16 | Complete — shared declaration, receipt, delivery, prebuilt, and source-only regressions passed. | -| 10 | A07: allocation and temporary metadata | Pending | +| 10 | A07: allocation and temporary metadata | Complete — allocation resolution, joined cleanup causes, tag evidence, and receipt installation checks passed. | | 11 | A07: build and delivery outcomes | Pending | | 12 | A08 | Pending | | 13 | A09 | Pending | diff --git a/internal/git/source_test.go b/internal/git/source_test.go index a08bd35..10881fd 100644 --- a/internal/git/source_test.go +++ b/internal/git/source_test.go @@ -54,6 +54,31 @@ func TestTagOperationsAcceptFullSlashContainingRefs(t *testing.T) { } } +func TestCreateAnnotatedTagReportsTemporaryCleanupFailure(t *testing.T) { + fixture := newRemoteFixture(t) + cleanupErr := errors.New("cleanup failed") + var removed string + remove := func(path string) error { + removed = path + if err := os.Remove(path); err != nil { + return err + } + return cleanupErr + } + object, err := createAnnotatedTag(t.Context(), fixture.work, "v1.2.3", fixture.commit, []byte("note\n"), remove) + if !errors.Is(err, cleanupErr) || object == "" || removed == "" || !strings.Contains(err.Error(), removed) { + t.Fatalf("tag object = %q, cleanup path = %q, error = %v", object, removed, err) + } + if got := gitText(t, fixture.work, "rev-parse", "refs/tags/v1.2.3"); got != object { + t.Fatalf("local tag = %q, want %q", got, object) + } + _, err = createAnnotatedTag(t.Context(), fixture.work, "v1.2.3", fixture.commit, []byte("note\n"), remove) + var exit *exec.ExitError + if !errors.As(err, &exit) || !errors.Is(err, cleanupErr) { + t.Fatalf("tag creation and cleanup error = %v", err) + } +} + func TestTagReferenceValidationUsesGitRules(t *testing.T) { fixture := newRemoteFixture(t) for _, tag := range []string{"", "v1..2.3", "v1.2.3.lock", "v1.2.3/", "v1.2.3\\bad"} { diff --git a/internal/git/tag.go b/internal/git/tag.go index f87faa7..cf443db 100644 --- a/internal/git/tag.go +++ b/internal/git/tag.go @@ -3,6 +3,7 @@ package git import ( "bytes" "context" + "errors" "fmt" "os" "strings" @@ -12,6 +13,10 @@ import ( // on the captured commit. Once its type and ID are established, the ID is // returned even if a later verification fails. func CreateAnnotatedTag(ctx context.Context, repositoryDir, tag, commit string, note []byte) (string, error) { + return createAnnotatedTag(ctx, repositoryDir, tag, commit, note, os.Remove) +} + +func createAnnotatedTag(ctx context.Context, repositoryDir, tag, commit string, note []byte, remove func(string) error) (objectID string, resultErr error) { if err := CheckTagRef(ctx, repositoryDir, tag); err != nil { return "", err } @@ -23,7 +28,11 @@ func CreateAnnotatedTag(ctx context.Context, repositoryDir, tag, commit string, return "", fmt.Errorf("create temporary release note: %w", err) } messagePath := message.Name() - defer os.Remove(messagePath) + defer func() { + if err := remove(messagePath); err != nil { + resultErr = errors.Join(resultErr, fmt.Errorf("remove temporary release note %q: %w", messagePath, err)) + } + }() if _, err := message.Write(note); err != nil { _ = message.Close() return "", fmt.Errorf("write temporary release note: %w", err) diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go index 5b01361..6614baa 100644 --- a/internal/tempdir/tempdir.go +++ b/internal/tempdir/tempdir.go @@ -2,6 +2,7 @@ package tempdir import ( + "errors" "fmt" "os" "path/filepath" @@ -11,11 +12,15 @@ import ( // Outside uses system temporary storage and falls back to the repository's // parent when TMPDIR resolves inside the worktree. Cleanup belongs to the caller. func Outside(repositoryDir, pattern string) (string, error) { - repository, err := filepath.Abs(repositoryDir) + return outside(repositoryDir, pattern, filepath.Abs, filepath.EvalSymlinks, os.RemoveAll) +} + +func outside(repositoryDir, pattern string, abs, resolve func(string) (string, error), remove func(string) error) (string, error) { + repository, err := abs(repositoryDir) if err != nil { return "", err } - repository, err = filepath.EvalSymlinks(repository) + repository, err = resolve(repository) if err != nil { return "", err } @@ -24,54 +29,54 @@ func Outside(repositoryDir, pattern string) (string, error) { return "", err } created := directory - directory, err = filepath.Abs(created) + directory, err = abs(created) if err != nil { - os.RemoveAll(created) - return "", err + return "", cleanupError(created, err, remove) } - directory, err = filepath.EvalSymlinks(directory) + directory, err = resolve(directory) if err != nil { - os.RemoveAll(directory) - return "", err + return "", cleanupError(created, err, remove) } outside, err := isOutside(repository, directory) if err != nil { - os.RemoveAll(directory) - return "", err + return "", cleanupError(created, err, remove) } if outside { return directory, nil } - if err := os.RemoveAll(directory); err != nil { - return "", fmt.Errorf("remove temporary storage inside repository %q: %w", repository, err) + if err := remove(created); err != nil { + return "", fmt.Errorf("remove temporary storage %q: %w", created, err) } directory, err = os.MkdirTemp(filepath.Dir(repository), pattern) if err != nil { return "", fmt.Errorf("create temporary storage outside repository %q: %w", repository, err) } created = directory - directory, err = filepath.Abs(created) + directory, err = abs(created) if err != nil { - os.RemoveAll(created) - return "", err + return "", cleanupError(created, err, remove) } - directory, err = filepath.EvalSymlinks(directory) + directory, err = resolve(directory) if err != nil { - os.RemoveAll(directory) - return "", err + return "", cleanupError(created, err, remove) } outside, err = isOutside(repository, directory) if err != nil { - os.RemoveAll(directory) - return "", err + return "", cleanupError(created, err, remove) } if !outside { - os.RemoveAll(directory) - return "", fmt.Errorf("temporary storage %q is inside repository %q", directory, repository) + return "", cleanupError(created, fmt.Errorf("temporary storage %q is inside repository %q", directory, repository), remove) } return directory, nil } +func cleanupError(path string, cause error, remove func(string) error) error { + if err := remove(path); err != nil { + return errors.Join(cause, fmt.Errorf("remove temporary storage %q: %w", path, err)) + } + return cause +} + func isOutside(repository, directory string) (bool, error) { if !strings.EqualFold(filepath.VolumeName(repository), filepath.VolumeName(directory)) { return true, nil diff --git a/internal/tempdir/tempdir_test.go b/internal/tempdir/tempdir_test.go index 6815fab..fd97512 100644 --- a/internal/tempdir/tempdir_test.go +++ b/internal/tempdir/tempdir_test.go @@ -1,12 +1,81 @@ package tempdir import ( + "errors" "os" "path/filepath" "runtime" + "strings" "testing" ) +func TestOutsideRemovesAllocatedPathWhenResolutionFails(t *testing.T) { + for _, fallback := range []bool{false, true} { + for _, operation := range []string{"absolute", "symlinks"} { + name := operation + if fallback { + name += "-fallback" + } + t.Run(name, func(t *testing.T) { + parent := t.TempDir() + repository := filepath.Join(parent, "repository") + local := filepath.Join(repository, "tmp") + if err := os.MkdirAll(local, 0o700); err != nil { + t.Fatal(err) + } + if fallback { + setTemporaryDirectory(t, local) + } else { + setTemporaryDirectory(t, parent) + } + resolutionErr := errors.New("resolution failed") + removalErr := errors.New("removal failed") + calls := 0 + failed := false + var removed string + failAt := 2 + if fallback { + failAt = 3 + } + abs := func(path string) (string, error) { + if operation == "absolute" { + calls++ + if calls == failAt { + failed = true + return "", resolutionErr + } + } + return filepath.Abs(path) + } + resolve := func(path string) (string, error) { + if operation == "symlinks" { + calls++ + if calls == failAt { + failed = true + return "", resolutionErr + } + } + return filepath.EvalSymlinks(path) + } + remove := func(path string) error { + if err := os.RemoveAll(path); err != nil { + return err + } + if failed { + removed = path + return removalErr + } + return nil + } + _, err := outside(repository, "owned-", abs, resolve, remove) + if !errors.Is(err, resolutionErr) || !errors.Is(err, removalErr) || removed == "" || !strings.Contains(err.Error(), removed) || !strings.HasPrefix(filepath.Base(removed), "owned-") { + t.Fatalf("error = %v, removed = %q", err, removed) + } + }) + } + } +} + func TestOutsideHonorsRelativeTemporaryDirectory(t *testing.T) { parent := t.TempDir() repository := t.TempDir()