From c8eb64adec66e9a662a0fb0b687a10d34e17848e Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 10 Oct 2026 14:11:48 +0000 Subject: [PATCH] Rediscover Go settings only when Go work needs them --- docs/internal/overview.md | 4 ++- docs/roadmap/remediation.md | 10 +++++++- internal/candidate/check.go | 8 ------ internal/candidate/check_test.go | 22 +++++++++++++++++ internal/gotool/gotool_test.go | 24 +++++++++++++----- internal/gotool/owner.go | 42 +++++++++++--------------------- internal/publish/publish.go | 4 --- publish_test.go | 38 +++++++++++++++++++++++++++++ 8 files changed, 104 insertions(+), 48 deletions(-) diff --git a/docs/internal/overview.md b/docs/internal/overview.md index 178053e..17a89c8 100644 --- a/docs/internal/overview.md +++ b/docs/internal/overview.md @@ -64,7 +64,9 @@ Candidate checking captures and validates one clean committed source revision in `internal/candidate`. Its validation bridge calls `internal/validation` for the selected module's default offline Go suite, then runs product-specific checks once. Validation and its post-validation build callback share one local -Go settings owner; the candidate package owns those builds and local-state +Go settings owner. Each Go acquisition checks inherited and persistent inputs; +settings are rediscovered when subsequent Go work needs them. An unavailable +persistent-settings snapshot disables reuse. The candidate package owns builds and local-state rechecks; publication owns endpoint guards, tag creation, push, and recovery around that same candidate validation path. `internal/validation.CandidateTree` reads committed blobs lazily and resolves local destinations from committed entries; diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index e4ab731..925e931 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -157,7 +157,15 @@ a genuinely different repository or module must still fail before hosted work. ## Issue 5: Keep Go environment reuse lazy and proportionate (P2) -Status: open. Category: additional validation gates and unrequested restrictions. +Status: complete. Category: additional validation gates and unrequested restrictions. + +**Completion:** Go discovery happens on acquisition for actual Go work. Completed +callbacks trigger no additional discovery; settings-file snapshot failures disable +reuse while preserving the Go tool's accepted configurations. Regression tests +cover check/publication completion after Go leaves PATH, subsequent builds needing +rediscovery, the final callback, unreadable GOENV snapshots, normal environment +and persistent-file changes, and cancellation. Focused public cases and complete +candidate and Go-tool suites pass offline. **Evidence:** [candidate coordination](../../internal/candidate/check.go) and [publication](../../internal/publish/publish.go) eagerly call diff --git a/internal/candidate/check.go b/internal/candidate/check.go index ccd27ff..4f6141e 100644 --- a/internal/candidate/check.go +++ b/internal/candidate/check.go @@ -53,13 +53,9 @@ func Check(ctx context.Context, request Request) (Candidate, error) { } if request.AfterValidate != nil { callbackErr := request.AfterValidate(ctx, captured, request.ToolOwner) - refreshErr := request.ToolOwner.Refresh(ctx) if callbackErr != nil { return captured, fmt.Errorf("after candidate validation: %w", callbackErr) } - if refreshErr != nil { - return captured, fmt.Errorf("refresh Go settings after candidate callback: %w", refreshErr) - } } if err := Recheck(ctx, captured, ""); err != nil { return captured, err @@ -143,13 +139,9 @@ func Validate(ctx context.Context, request Request, captured Candidate) error { } if request.AdditionalChecks != nil { callbackErr := request.AdditionalChecks(ctx, captured) - refreshErr := owner.Refresh(ctx) if callbackErr != nil { return fmt.Errorf("validate candidate %s: %w", captured.Commit, callbackErr) } - if refreshErr != nil { - return fmt.Errorf("refresh Go settings after additional checks: %w", refreshErr) - } } return nil } diff --git a/internal/candidate/check_test.go b/internal/candidate/check_test.go index bdeec32..f6195e0 100644 --- a/internal/candidate/check_test.go +++ b/internal/candidate/check_test.go @@ -304,6 +304,28 @@ func TestCheckRefreshesGoSettingsAfterAdditionalChecks(t *testing.T) { } } +func TestCheckNeedsNoGoDiscoveryAfterFinalCallback(t *testing.T) { + repository := newRepository(t, "# Example Product v1.2.3\n\nRelease note.\n") + gitPath, err := exec.LookPath("git") + if err != nil { + t.Fatal(err) + } + gitOnly := t.TempDir() + if err := os.Symlink(gitPath, filepath.Join(gitOnly, "git")); err != nil { + t.Fatal(err) + } + _, err = Check(context.Background(), Request{ + RepositoryDir: repository, Product: "Example Product", Version: testVersion, + AfterValidate: func(context.Context, Candidate, *gotool.Owner) error { + t.Setenv("PATH", gitOnly) + return nil + }, + }) + if err != nil { + t.Fatalf("completed Go work was rediscovered after final callback: %v", err) + } +} + func TestCheckSharesGoDiscoveryWithAfterValidateCallback(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("the controlled Go command fixture uses a POSIX shell") diff --git a/internal/gotool/gotool_test.go b/internal/gotool/gotool_test.go index 616eb93..7abf78c 100644 --- a/internal/gotool/gotool_test.go +++ b/internal/gotool/gotool_test.go @@ -320,9 +320,6 @@ func TestOwnerReusesSettingsUntilInheritedEnvironmentChanges(t *testing.T) { cache := filepath.Join(t.TempDir(), "changed-cache") t.Setenv("GOCACHE", cache) - if err := owner.Refresh(context.Background()); err != nil { - t.Fatal(err) - } third, err := owner.Acquire(context.Background()) if err != nil { t.Fatal(err) @@ -332,6 +329,24 @@ func TestOwnerReusesSettingsUntilInheritedEnvironmentChanges(t *testing.T) { } } +func TestOwnerAcceptsGoConfigurationWithoutReadableSnapshot(t *testing.T) { + clearGoSettings(t) + t.Setenv("GOENV", t.TempDir()) + directory := t.TempDir() + if _, err := New(context.Background(), directory); err != nil { + t.Fatalf("underlying Go discovery rejected fixture: %v", err) + } + owner := NewOwner(directory) + first, err := owner.Acquire(context.Background()) + if err != nil { + t.Fatalf("owner added a snapshot prerequisite: %v", err) + } + second, err := owner.Acquire(context.Background()) + if err != nil || first == second { + t.Fatalf("unavailable snapshot should rediscover on demand: first=%p second=%p error=%v", first, second, err) + } +} + func TestOwnerRefreshesWhenPersistentSettingsChange(t *testing.T) { clearGoSettings(t) firstCache := filepath.Join(t.TempDir(), "first-cache") @@ -355,9 +370,6 @@ func TestOwnerRefreshesWhenPersistentSettingsChange(t *testing.T) { t.Fatalf("initial GOCACHE = %q, want %q", first.settings["GOCACHE"], firstCache) } writeSettings(secondCache) - if err := owner.Refresh(context.Background()); err != nil { - t.Fatal(err) - } second, err := owner.Acquire(context.Background()) if err != nil { t.Fatal(err) diff --git a/internal/gotool/owner.go b/internal/gotool/owner.go index 380d688..47a290d 100644 --- a/internal/gotool/owner.go +++ b/internal/gotool/owner.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "errors" - "fmt" "os" "path/filepath" "strings" @@ -13,12 +12,13 @@ import ( // Owner lazily discovers and reuses one Go tool while its inherited // environment and persistent settings file remain unchanged. type Owner struct { - directory string - tool *Tool - env map[string]string - file string - contents []byte - fileFound bool + directory string + tool *Tool + env map[string]string + file string + contents []byte + fileFound bool + snapshotAvailable bool } // NewOwner creates an operation-local Go tool owner without running Go. @@ -29,7 +29,7 @@ func NewOwner(directory string) *Owner { // Acquire returns the operation's Go tool, rediscovering settings when their // process or persistent inputs have changed. func (owner *Owner) Acquire(ctx context.Context) (*Tool, error) { - if owner.tool != nil { + if owner.tool != nil && owner.snapshotAvailable { changed, err := owner.inputsChanged() if err == nil && !changed { return owner.tool, nil @@ -41,38 +41,24 @@ func (owner *Owner) Acquire(ctx context.Context) (*Tool, error) { return owner.tool, nil } -// Refresh checks the owner's inputs after a consumer callback and rediscovers -// settings when the callback changed the environment or persistent file. -func (owner *Owner) Refresh(ctx context.Context) error { - if owner.tool == nil { - return nil - } - changed, err := owner.inputsChanged() - if err == nil && !changed { - return nil - } - return owner.discover(ctx) -} - func (owner *Owner) discover(ctx context.Context) error { environment := inheritedEnvironment() tool, err := New(ctx, owner.directory) if err != nil { return err } - settingsFile, err := resolveSettingsFile(owner.directory, tool.settingsFile) - if err != nil { - return err - } - contents, found, err := readSettingsFile(settingsFile) - if err != nil { - return fmt.Errorf("read Go settings file %q: %w", settingsFile, err) + settingsFile, snapshotErr := resolveSettingsFile(owner.directory, tool.settingsFile) + var contents []byte + var found bool + if snapshotErr == nil { + contents, found, snapshotErr = readSettingsFile(settingsFile) } owner.tool = tool owner.env = environment owner.file = settingsFile owner.contents = contents owner.fileFound = found + owner.snapshotAvailable = snapshotErr == nil return nil } diff --git a/internal/publish/publish.go b/internal/publish/publish.go index c44f254..7e497a1 100644 --- a/internal/publish/publish.go +++ b/internal/publish/publish.go @@ -127,13 +127,9 @@ func Publish(ctx context.Context, request Request) (Result, error) { } if request.AfterValidate != nil { callbackErr := request.AfterValidate(ctx, captured, checks.ToolOwner) - refreshErr := checks.ToolOwner.Refresh(ctx) if callbackErr != nil { return result, fmt.Errorf("after candidate validation: %w", callbackErr) } - if refreshErr != nil { - return result, fmt.Errorf("refresh Go settings after candidate callback: %w", refreshErr) - } } if err := candidate.Recheck(ctx, captured, ""); err != nil { return result, fmt.Errorf("post-validation local guard: %w", err) diff --git a/publish_test.go b/publish_test.go index ea8a8fe..bb506b2 100644 --- a/publish_test.go +++ b/publish_test.go @@ -18,6 +18,44 @@ import ( const releaseNote = "# Sample App v1.2.3\n\nA source release.\n\nUpgrade notes stay intact.\n" +func TestCandidateOperationsDiscoverGoOnlyWhenLaterWorkNeedsIt(t *testing.T) { + gitPath := mustLookPath(t, "git") + gitOnly := t.TempDir() + if err := os.Symlink(gitPath, filepath.Join(gitOnly, "git")); err != nil { + t.Fatal(err) + } + for _, operation := range []string{"check", "publish"} { + for _, withBuild := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/build=%t", operation, withBuild), func(t *testing.T) { + repo := newPublishFixture(t) + request := releasekit.ReleaseRequest{ + RepositoryDir: repo.work, Product: "Sample App", Version: "v1.2.3", + AdditionalChecks: func(context.Context, releasekit.Candidate) error { + t.Setenv("PATH", gitOnly) + return nil + }, + } + if withBuild { + request.Build = &releasekit.BuildSpec{MainPackage: ".", BinaryName: "fixture", Targets: []releasekit.Target{{OS: "linux", Arch: "amd64"}}} + } + var err error + if operation == "check" { + _, err = releasekit.Check(context.Background(), request) + } else { + _, err = releasekit.Publish(context.Background(), request) + } + if withBuild { + if !errors.Is(err, exec.ErrNotFound) { + t.Fatalf("later build did not rediscover changed Go inputs: %v", err) + } + } else if err != nil { + t.Fatalf("finished Go work added a discovery gate: %v", err) + } + }) + } + } +} + func TestPublishPushesOnlyTheCapturedAnnotatedTag(t *testing.T) { repo := newPublishFixture(t) wrapper := installGitWrapper(t, repo, "pass")