diff --git a/docs/internal/validation.md b/docs/internal/validation.md index f4766e7..fe05c50 100644 --- a/docs/internal/validation.md +++ b/docs/internal/validation.md @@ -54,7 +54,8 @@ 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. +exchange. The context supplied when creating the tree governs the reader +lifetime, including shutdown and child reaping. Regular-file reads return independent caller-owned byte slices and are not retained by the tree. Symlink targets are retained as immutable owned strings by blob object ID; explicitly reading a symlink still returns independent diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 12fb28b..74d68fa 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Performance Implementation Remediation -**Status:** Required follow-up; remediation pending. +**Status:** Remediation in progress; R1 complete. **Review baseline:** df064bd, reviewed on 2026-10-09 against the [performance target](performance.md) and @@ -59,6 +59,12 @@ shutdown is still blocked, and verify completion, child cleanup, and preserved cancellation identity. Retain coverage of read cancellation, successful closure, and preservation of an earlier check failure. +**Completion:** Complete. The tree retains its operation context for the reader +lifetime. An EOF-synchronized regression now verifies shutdown cancellation, +cancellation identity, and child reaping; it failed before the fix and passes +afterward. Race tests passed for validation, Git, and process packages, including +existing read cancellation, successful closure, and primary-failure coverage. + ## R2: Attribute Shutdown Failures To A Check That Read Content **Priority:** Medium. diff --git a/internal/validation/tree.go b/internal/validation/tree.go index 5bcb7f2..b904687 100644 --- a/internal/validation/tree.go +++ b/internal/validation/tree.go @@ -13,6 +13,7 @@ import ( // CandidateTree is an inventory of one captured commit with blob contents // loaded lazily by object ID. type CandidateTree struct { + context context.Context repositoryDir string entries []git.TreeEntry byPath map[string]int @@ -24,13 +25,14 @@ type CandidateTree struct { } // ReadCandidateTree enumerates the captured commit without reading from the -// worktree. +// worktree. The supplied context also governs the scoped reader through closure. func ReadCandidateTree(ctx context.Context, repositoryDir, commit string) (*CandidateTree, error) { entries, err := git.ReadTree(ctx, repositoryDir, commit) if err != nil { return nil, err } tree := &CandidateTree{ + context: ctx, repositoryDir: repositoryDir, entries: entries, byPath: make(map[string]int, len(entries)), @@ -104,10 +106,12 @@ func (tree *CandidateTree) readBlob(ctx context.Context, entry git.TreeEntry) ([ return nil, fmt.Errorf("read committed path %q: candidate tree is closed", entry.Path) } 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) + // The operation context governs shutdown independently of each read. + operationContext := tree.context + if operationContext == nil { + operationContext = context.Background() + } + tree.blobReader = git.NewBlobReader(operationContext, tree.repositoryDir) } contents, err := tree.blobReader.Read(ctx, entry.ObjectID) if err != nil { diff --git a/internal/validation/tree_lifecycle_test.go b/internal/validation/tree_lifecycle_test.go index d470160..9647ca9 100644 --- a/internal/validation/tree_lifecycle_test.go +++ b/internal/validation/tree_lifecycle_test.go @@ -7,7 +7,9 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" + "syscall" "testing" "time" ) @@ -93,6 +95,55 @@ exec sleep 60`) } } +func TestRunPreservesCancellationDuringReaderShutdown(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(), "shutdown-ready") + 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 '%s\n' "$$" > "$RELEASEKIT_GIT_READER_READY" +exec sleep 60`) + t.Setenv("RELEASEKIT_GIT_READER_READY", ready) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + result := make(chan error, 1) + go func() { result <- Run(ctx, lifecycleRequest(repository, commit)) }() + waitForLifecycleFile(t, ready) + pidBytes, err := os.ReadFile(ready) + if err != nil { + t.Fatal(err) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(pidBytes))) + if err != nil { + t.Fatal(err) + } + child, err := os.FindProcess(pid) + if err != nil { + t.Fatal(err) + } + cancel() + select { + case err := <-result: + var failure *Failure + if !errors.As(err, &failure) || failure.Check != GoFormat || !errors.Is(err, context.Canceled) { + t.Fatalf("Run() error = %v, want GoFormat cancellation cause", err) + } + if err := child.Signal(syscall.Signal(0)); !errors.Is(err, syscall.ESRCH) && !errors.Is(err, os.ErrProcessDone) { + t.Fatalf("child signal after Run() = %v, want reaped child", err) + } + case <-time.After(5 * time.Second): + _ = child.Kill() + <-result + t.Fatal("Run() did not return after cancellation during reader shutdown") + } +} + func lifecycleRequest(repository, commit string) Request { return Request{ RepositoryDir: repository,