diff --git a/docs/internal/validation.md b/docs/internal/validation.md index 9e46f19..166db71 100644 --- a/docs/internal/validation.md +++ b/docs/internal/validation.md @@ -47,7 +47,13 @@ and checks supported anchors; outside-checkout and submodule observations are written to the shared output writer. A nil writer keeps library use quiet. `CandidateTree` gets metadata and bytes from Git objects, reads blob bytes lazily, and resolves destinations without consulting the worktree or -neighboring files. +neighboring files. Each tree owns one lazy Git blob reader; direct tree users +must close it when finished. The shared validation executor closes its tree on +every return before product checks or build callbacks run. A metadata-only +selection never starts the reader. Per-read contexts cancel an in-flight blob +exchange without tying the reader lifetime to a short-lived caller context. +Until formatting and parsed-result reuse are in place, regular-file bytes stay +cached by object ID for the lifetime of the tree. The internal Markdown parser uses Goldmark to record inline, reference, and autolinks, images, titles, and their source use sites, and to build heading diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 2b89ec8..c500f93 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,6 +1,6 @@ # Computational Efficiency Implementation Plan -**Status:** Stages 1–4 complete; Stages 5–11 pending. +**Status:** Stages 1–5 complete; Stages 6–11 pending. ## Purpose And Baseline @@ -316,6 +316,22 @@ preserve workspace/file-size metadata behavior. Re-run content benchmarks with reader startup and closure included in the content-operation timing. Update [validation mechanics](../internal/validation.md) for the landed lifecycle. +**Completion:** Complete. `CandidateTree` owns a lazy batch reader and exposes +idempotent cleanup. The shared executor closes it on every post-load return, +before product checks or build callbacks; metadata-only runs leave it unstarted. +Earlier check failures and cancellation remain primary, while an unexpected +reader exit after successful content checks becomes a contextual failure for +the last content-reading check. Direct tree users close their readers. The +temporary object-ID byte cache remains until Stage 6. Validation, candidate, +and root facade tests passed. Content benchmark timings now include reader +startup and close (Go 1.26.8, Git 2.53.0, Linux x86_64 Intel Core i5-7260U, +three repetitions): unique reads took 59.8–77.9 ms loose and 22.8–43.0 ms +packed; unique-file formatting took 114–188 ms loose and 33.7–55.7 ms packed; +repeated-ID reads took 2.18–6.45 ms, repeated-ID formatting took 4.75–6.62 ms, +and the Markdown workload took 15.0–16.9 ms. Compared with the Stage 1 timings, +these measurements reflect the shared Git process and include its lifecycle; +timing varies on the shared machine. + ## Stage 6: Release One-Use Source Bytes And Reuse Formatting Results **Owners:** `internal/validation/tree.go`, `validation.go`, and destination tests. diff --git a/internal/validation/destination_test.go b/internal/validation/destination_test.go index 39ef147..3ba89bb 100644 --- a/internal/validation/destination_test.go +++ b/internal/validation/destination_test.go @@ -43,6 +43,7 @@ func TestResolveDestinationClassifiesCommittedPaths(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) if err := os.Remove(filepath.Join(repository, "docs", "inside")); err != nil { t.Fatal(err) } @@ -102,6 +103,7 @@ func TestResolveDestinationHandlesExpandingCyclesAndRepeatedLinks(t *testing.T) if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) for _, test := range []struct { destination string kind DestinationKind @@ -155,6 +157,7 @@ func TestResolveDestinationIgnoresNeighboringFiles(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) gotWithoutNeighbor, err := tree.ResolveDestination(context.Background(), "docs/page.md", "outside") if err != nil { t.Fatal(err) @@ -195,6 +198,7 @@ func TestResolveDestinationPreservesParentTraversalOrder(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) for _, test := range []struct { destination string kind DestinationKind @@ -253,6 +257,7 @@ func TestResolveDestinationMapsAbsolutePathsIntoCandidate(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) if err := os.Remove(filepath.Join(repository, "README.md")); err != nil { t.Fatal(err) } diff --git a/internal/validation/file_size_test.go b/internal/validation/file_size_test.go index 4414d31..6141df9 100644 --- a/internal/validation/file_size_test.go +++ b/internal/validation/file_size_test.go @@ -135,6 +135,7 @@ func TestCheckFileSizeUsesCommittedBlobSizes(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) err = checkFileSize(tree, &FileSizeSettings{LimitBytes: int64Pointer(3), Exclusions: []string{"link"}}) if err != nil { @@ -170,6 +171,7 @@ func TestCheckFileSizeUsesOnlyCandidateFilesIndividually(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) if err := checkFileSize(tree, &FileSizeSettings{LimitBytes: int64Pointer(10)}); err != nil { t.Fatalf("checkFileSize() error = %v, want current individual files to pass", err) } diff --git a/internal/validation/format_test.go b/internal/validation/format_test.go index 899fb53..c4563c7 100644 --- a/internal/validation/format_test.go +++ b/internal/validation/format_test.go @@ -36,6 +36,7 @@ func TestCheckGoFormatUsesCapturedRegularGoBlobs(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) err = checkGoFormat(context.Background(), tree) var failure *Failure if !errors.As(err, &failure) || failure.Check != GoFormat { @@ -70,6 +71,7 @@ func TestCheckGoFormatPreservesParserAndInspectionCauses(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) err = checkGoFormat(context.Background(), tree) var failure *Failure if !errors.As(err, &failure) || failure.Check != GoFormat || failure.Cause == nil { @@ -90,6 +92,7 @@ func TestCheckGoFormatPreservesParserAndInspectionCauses(t *testing.T) { byPath: map[string]int{"missing.go": 0}, blobs: make(map[string][]byte), } + cleanupCandidateTree(t, tree) err := checkGoFormat(context.Background(), tree) var failure *Failure if !errors.As(err, &failure) || failure.Check != GoFormat || failure.Cause == nil { diff --git a/internal/validation/local_replacements_test.go b/internal/validation/local_replacements_test.go index 80901ff..c609aa2 100644 --- a/internal/validation/local_replacements_test.go +++ b/internal/validation/local_replacements_test.go @@ -21,6 +21,7 @@ func TestCheckLocalReplacementsParsesCommittedModuleDeclarations(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) err = checkLocalReplacements(context.Background(), tree) failure, ok := err.(*Failure) @@ -61,6 +62,7 @@ func TestCheckLocalReplacementsFollowsCommittedModuleSymlinks(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) err = checkLocalReplacements(context.Background(), tree) failure, ok := err.(*Failure) if !ok || failure.Check != LocalReplacements || len(failure.Findings) != 1 { @@ -82,6 +84,7 @@ func TestCheckLocalReplacementsReportsUnavailableSymlinkContents(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) err = checkLocalReplacements(context.Background(), tree) failure, ok := err.(*Failure) if !ok || failure.Check != LocalReplacements || failure.Cause == nil { @@ -100,6 +103,7 @@ func TestCheckLocalReplacementsPreservesParseCause(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) err = checkLocalReplacements(context.Background(), tree) failure, ok := err.(*Failure) if !ok || failure.Check != LocalReplacements || failure.Cause == nil || !strings.Contains(failure.Cause.Error(), "parse committed go.mod") { diff --git a/internal/validation/markdown_check_test.go b/internal/validation/markdown_check_test.go index 02aeb0a..d04d592 100644 --- a/internal/validation/markdown_check_test.go +++ b/internal/validation/markdown_check_test.go @@ -289,6 +289,7 @@ func candidateTreeAtHead(t *testing.T, repository string) *CandidateTree { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) return tree } diff --git a/internal/validation/markdown_test.go b/internal/validation/markdown_test.go index 171b938..4ae7d83 100644 --- a/internal/validation/markdown_test.go +++ b/internal/validation/markdown_test.go @@ -99,6 +99,7 @@ func TestMarkdownCacheKeepsParsedDocumentsPerCandidatePath(t *testing.T) { if err != nil { t.Fatal(err) } + cleanupCandidateTree(t, tree) cache := newMarkdownCache(tree) first, err := cache.document(context.Background(), "guide.md") if err != nil { diff --git a/internal/validation/performance_test.go b/internal/validation/performance_test.go index f8f3413..d09bdec 100644 --- a/internal/validation/performance_test.go +++ b/internal/validation/performance_test.go @@ -54,6 +54,9 @@ func BenchmarkPerformanceValidation(b *testing.B) { if len(tree.Entries()) != len(fixture.paths) { b.Fatalf("tree entries = %d, want %d", len(tree.Entries()), len(fixture.paths)) } + if err := tree.Close(); err != nil { + b.Fatal(err) + } runtime.KeepAlive(tree) } }) @@ -77,6 +80,9 @@ func BenchmarkPerformanceValidation(b *testing.B) { if bytesRead != fixture.totalBytes { b.Fatalf("read %d bytes, want %d", bytesRead, fixture.totalBytes) } + if err := tree.Close(); err != nil { + b.Fatal(err) + } runtime.KeepAlive(tree) } runtime.KeepAlive(trees) @@ -92,6 +98,9 @@ func BenchmarkPerformanceValidation(b *testing.B) { if err := checkGoFormat(context.Background(), tree); err != nil { b.Fatal(err) } + if err := tree.Close(); err != nil { + b.Fatal(err) + } runtime.KeepAlive(tree) } runtime.KeepAlive(trees) @@ -109,6 +118,9 @@ func BenchmarkPerformanceValidation(b *testing.B) { if err := checkMarkdownLinks(context.Background(), tree, false, io.Discard); err != nil { b.Fatal(err) } + if err := tree.Close(); err != nil { + b.Fatal(err) + } runtime.KeepAlive(tree) } runtime.KeepAlive(trees) @@ -121,6 +133,7 @@ func BenchmarkPerformanceRetainedContent(b *testing.B) { if err != nil { b.Fatal(err) } + b.Cleanup(func() { _ = tree.Close() }) cache := newMarkdownCache(tree) var markdownPaths []string for _, entry := range tree.Entries() { @@ -149,6 +162,9 @@ func BenchmarkPerformanceRetainedContent(b *testing.B) { } } b.StopTimer() + if err := tree.Close(); err != nil { + b.Fatal(err) + } runtime.GC() var after runtime.MemStats runtime.ReadMemStats(&after) @@ -209,12 +225,16 @@ func freshPerformanceTrees(tb testing.TB, fixture performanceTree, count int) [] if err != nil { tb.Fatal(err) } + cleanupCandidateTree(tb, base) trees := make([]*CandidateTree, count) for index := range trees { tree := *base tree.blobs = make(map[string][]byte) trees[index] = &tree } + if err := base.Close(); err != nil { + tb.Fatal(err) + } runtime.KeepAlive(base) return trees } @@ -270,6 +290,9 @@ func verifyPerformanceTree(tb testing.TB, fixture performanceTree, repeated, pac tb.Fatal("Markdown link fixture does not contain the expected repeated targets") } } + if err := tree.Close(); err != nil { + tb.Fatal(err) + } objects := gitTree(tb, fixture.repository, "count-objects", "-v") loose, packedCount := performanceObjectCounts(tb, objects) diff --git a/internal/validation/tree.go b/internal/validation/tree.go index 0fe791c..54a56c1 100644 --- a/internal/validation/tree.go +++ b/internal/validation/tree.go @@ -18,6 +18,9 @@ type CandidateTree struct { byPath map[string]int directories map[string]struct{} blobs map[string][]byte + blobReader *git.BlobReader + closed bool + closeErr error } // ReadCandidateTree enumerates the captured commit without reading from the @@ -52,6 +55,9 @@ func (tree *CandidateTree) Entries() []git.TreeEntry { // ReadFile returns the committed blob bytes for path. Contents are cached by // object ID for later reads from this tree. func (tree *CandidateTree) ReadFile(ctx context.Context, path string) ([]byte, error) { + if tree.closed { + return nil, fmt.Errorf("read committed path %q: candidate tree is closed", path) + } index, ok := tree.byPath[path] if !ok { return nil, fmt.Errorf("committed path %q does not exist", path) @@ -63,14 +69,36 @@ func (tree *CandidateTree) ReadFile(ctx context.Context, path string) ([]byte, e if contents, ok := tree.blobs[entry.ObjectID]; ok { return append([]byte(nil), contents...), nil } - contents, err := git.ReadBlob(ctx, tree.repositoryDir, entry.ObjectID) + if tree.blobReader == nil { + // The reader belongs to the tree and can outlive an individual read + // context. Each Read call still applies its own cancellation to the + // in-flight exchange. + tree.blobReader = git.NewBlobReader(context.Background(), tree.repositoryDir) + } + contents, err := tree.blobReader.Read(ctx, entry.ObjectID) if err != nil { return nil, fmt.Errorf("read committed path %q: %w", path, err) } + if tree.blobs == nil { + tree.blobs = make(map[string][]byte) + } tree.blobs[entry.ObjectID] = append([]byte(nil), contents...) return contents, nil } +// Close closes the scoped local Git blob reader, if content was requested. +// Metadata-only trees have no child process to close. +func (tree *CandidateTree) Close() error { + if tree.closed { + return tree.closeErr + } + tree.closed = true + if tree.blobReader != nil { + tree.closeErr = tree.blobReader.Close() + } + return tree.closeErr +} + // Entry returns the committed metadata for path when present. func (tree *CandidateTree) Entry(path string) (git.TreeEntry, bool) { index, ok := tree.byPath[path] diff --git a/internal/validation/tree_lifecycle_test.go b/internal/validation/tree_lifecycle_test.go new file mode 100644 index 0000000..d470160 --- /dev/null +++ b/internal/validation/tree_lifecycle_test.go @@ -0,0 +1,133 @@ +package validation + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestRunAttributesUnexpectedReaderCloseFailureToLastContentCheck(t *testing.T) { + repository := newTreeRepository(t) + writeTreeFile(t, repository, "go.mod", []byte("module fixture.example/app\n\ngo 1.25.0\n")) + writeTreeFile(t, repository, "main.go", []byte("package app\n\nconst value = 1\n")) + commit := commitCandidateTree(t, repository) + installLifecycleTools(t, `while IFS= read -r object; do + size=$("$RELEASEKIT_REAL_GIT" -C "$PWD" cat-file -s "$object") + printf '%s blob %s\n' "$object" "$size" + "$RELEASEKIT_REAL_GIT" -C "$PWD" cat-file blob "$object" + printf '\n' +done +printf 'batch process failed during cleanup\n' >&2 +exit 23`) + + err := Run(context.Background(), lifecycleRequest(repository, commit)) + var failure *Failure + if !errors.As(err, &failure) || failure.Check != GoFormat { + t.Fatalf("Run() error = %v, want cleanup failure attributed to GoFormat", err) + } + var exitErr *exec.ExitError + if !errors.As(failure.Cause, &exitErr) || exitErr.ExitCode() != 23 { + t.Fatalf("cleanup cause = %v, want Git child exit code 23", failure.Cause) + } + if !strings.Contains(failure.Cause.Error(), "batch process failed during cleanup") { + t.Fatalf("cleanup cause = %v, want captured Git stderr", failure.Cause) + } +} + +func TestRunPreservesCheckFailureDuringReaderCleanup(t *testing.T) { + repository := newTreeRepository(t) + writeTreeFile(t, repository, "go.mod", []byte("module fixture.example/app\n\ngo 1.25.0\n")) + writeTreeFile(t, repository, "broken.go", []byte("package app\nfunc ( {\n")) + commit := commitCandidateTree(t, repository) + installLifecycleTools(t, `while IFS= read -r object; do + size=$("$RELEASEKIT_REAL_GIT" -C "$PWD" cat-file -s "$object") + printf '%s blob %s\n' "$object" "$size" + "$RELEASEKIT_REAL_GIT" -C "$PWD" cat-file blob "$object" + printf '\n' +done +printf 'secondary cleanup failure\n' >&2 +exit 24`) + + err := Run(context.Background(), lifecycleRequest(repository, commit)) + var failure *Failure + if !errors.As(err, &failure) || failure.Check != GoFormat || failure.Cause == nil { + t.Fatalf("Run() error = %v, want original GoFormat check failure", err) + } + if len(failure.Findings) != 1 || failure.Findings[0].Path != "broken.go" { + t.Fatalf("Run() findings = %#v, want original broken.go finding", failure.Findings) + } + if strings.Contains(failure.Cause.Error(), "secondary cleanup failure") { + t.Fatalf("Run() replaced the check cause with cleanup output: %v", failure.Cause) + } +} + +func TestRunPreservesCancellationDuringReaderCleanup(t *testing.T) { + repository := newTreeRepository(t) + writeTreeFile(t, repository, "go.mod", []byte("module fixture.example/app\n\ngo 1.25.0\n")) + writeTreeFile(t, repository, "main.go", []byte("package app\n\nconst value = 1\n")) + commit := commitCandidateTree(t, repository) + ready := filepath.Join(t.TempDir(), "reader-ready") + installLifecycleTools(t, `IFS= read -r object +: > "$RELEASEKIT_GIT_READER_READY" +exec sleep 60`) + t.Setenv("RELEASEKIT_GIT_READER_READY", ready) + ctx, cancel := context.WithCancel(t.Context()) + result := make(chan error, 1) + go func() { result <- Run(ctx, lifecycleRequest(repository, commit)) }() + waitForLifecycleFile(t, ready) + cancel() + select { + case err := <-result: + var failure *Failure + if !errors.As(err, &failure) || failure.Check != GoFormat || !errors.Is(failure.Cause, context.Canceled) { + t.Fatalf("Run() error = %v, want GoFormat cancellation cause", err) + } + case <-time.After(5 * time.Second): + t.Fatal("Run() did not return after candidate reader cancellation") + } +} + +func lifecycleRequest(repository, commit string) Request { + return Request{ + RepositoryDir: repository, + Commit: commit, + Settings: Settings{GoFormat: true}, + } +} + +func installLifecycleTools(t *testing.T, batchScript string) { + t.Helper() + realGit, err := exec.LookPath("git") + if err != nil { + t.Fatal(err) + } + installGoShim(t, "", false) + t.Setenv("RELEASEKIT_REAL_GIT", realGit) + directory := t.TempDir() + gitWrapper := filepath.Join(directory, "git") + script := "#!/bin/sh\nset -eu\nif [ \"${1-}\" = cat-file ] && [ \"${2-}\" = --batch ]; then\n" + batchScript + "\nfi\nexec \"$RELEASEKIT_REAL_GIT\" \"$@\"\n" + if err := os.WriteFile(gitWrapper, []byte(script), 0o700); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +func waitForLifecycleFile(t *testing.T, path string) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(path); err == nil { + return + } else if !errors.Is(err, os.ErrNotExist) { + t.Fatal(fmt.Errorf("inspect reader readiness signal: %w", err)) + } + time.Sleep(5 * time.Millisecond) + } + t.Fatal("candidate blob reader did not reach its blocking exchange") +} diff --git a/internal/validation/tree_test.go b/internal/validation/tree_test.go index f01211d..747ba86 100644 --- a/internal/validation/tree_test.go +++ b/internal/validation/tree_test.go @@ -38,6 +38,7 @@ func TestReadCandidateTreeUsesCapturedCommitAndEnumeratesGitEntries(t *testing.T if err != nil { t.Fatalf("ReadCandidateTree() error = %v", err) } + cleanupCandidateTree(t, tree) entries := make(map[string]gitTreeEntry, len(tree.Entries())) for _, entry := range tree.Entries() { entries[entry.Path] = gitTreeEntry{mode: entry.Mode, kind: entry.Type, size: entry.Size, hasSize: entry.HasSize} @@ -77,6 +78,11 @@ func TestReadCandidateTreeUsesCapturedCommitAndEnumeratesGitEntries(t *testing.T } } +func cleanupCandidateTree(tb testing.TB, tree *CandidateTree) { + tb.Helper() + tb.Cleanup(func() { _ = tree.Close() }) +} + func TestReadCandidateTreePreservesGitProcessError(t *testing.T) { repository := newTreeRepository(t) _, err := ReadCandidateTree(context.Background(), repository, strings.Repeat("f", 40)) diff --git a/internal/validation/validation.go b/internal/validation/validation.go index 3328934..f25cb8e 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -63,7 +63,7 @@ func (request Request) ReadCandidateTree(ctx context.Context) (*CandidateTree, e } // Run executes shared validation for the captured candidate. -func Run(ctx context.Context, request Request) error { +func Run(ctx context.Context, request Request) (result error) { if request.Settings.FileSize != nil && request.Settings.FileSize.LimitBytes != nil && *request.Settings.FileSize.LimitBytes < 0 { return &Failure{ Check: FileSize, @@ -136,7 +136,17 @@ func Run(ctx context.Context, request Request) error { } return &Failure{Check: check, Cause: err} } + lastContentCheck := Check("") + defer func() { + if closeErr := tree.Close(); closeErr != nil && result == nil && lastContentCheck != "" { + result = &Failure{ + Check: lastContentCheck, + Cause: fmt.Errorf("close candidate Git blob reader: %w", closeErr), + } + } + }() if request.Settings.GoFormat { + lastContentCheck = GoFormat if err := checkGoFormat(ctx, tree); err != nil { return err } @@ -147,6 +157,7 @@ func Run(ctx context.Context, request Request) error { } } if request.Settings.ModuleDistribution.LocalReplacements { + lastContentCheck = LocalReplacements if err := checkLocalReplacements(ctx, tree); err != nil { return err } @@ -157,6 +168,7 @@ func Run(ctx context.Context, request Request) error { } } if request.Settings.Markdown != nil { + lastContentCheck = MarkdownLinks if err := checkMarkdownLinks(ctx, tree, request.Settings.Markdown.EnforceCheckoutBoundary, request.Output); err != nil { return err }