diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index 22a2c49f66..e103b86e5f 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -137,7 +137,12 @@ func writeCacheAtomic(path string, data []byte) error { // baseURL points at the repo hosting the constraint artifacts (see // RepoConstraintBaseURL); it is empty when no source is configured, which is // reported below as a fetch-phase error. -func FetchConstraints(ctx context.Context, baseURL, envKey, cacheDir string) (*Constraints, error) { +// +// writeCache controls whether a successful live fetch populates the on-disk +// cache. Callers pass false for a dry run (--check), which must not mutate +// disk; an existing cache is still read for offline fallback, since reading is +// not a mutation. +func FetchConstraints(ctx context.Context, baseURL, envKey, cacheDir string, writeCache bool) (*Constraints, error) { if baseURL == "" { // No constraint host is configured. This is reported at the fetch phase (as // E_FETCH) rather than aborting earlier, so the failure flows through the @@ -158,9 +163,12 @@ func FetchConstraints(ctx context.Context, baseURL, envKey, cacheDir string) (*C return nil, fmt.Errorf("parse constraints for %s: %w", envKey, err) } // Write the cache copy (creating cacheDir if needed, atomically); non-fatal - // so a read-only cacheDir doesn't break the command. - if err := writeCacheAtomic(cachePath, data); err != nil { - log.Debugf(ctx, "failed to write constraint cache %s: %v", filepath.ToSlash(cachePath), err) + // so a read-only cacheDir doesn't break the command. Skipped under a dry + // run so --check performs no disk writes at all. + if writeCache { + if err := writeCacheAtomic(cachePath, data); err != nil { + log.Debugf(ctx, "failed to write constraint cache %s: %v", filepath.ToSlash(cachePath), err) + } } return &Constraints{ EnvKey: envKey, diff --git a/libs/localenv/constraints_test.go b/libs/localenv/constraints_test.go index 8df363d943..c330c4bba2 100644 --- a/libs/localenv/constraints_test.go +++ b/libs/localenv/constraints_test.go @@ -30,7 +30,7 @@ func TestRepoConstraintBaseURL(t *testing.T) { func TestFetchConstraintsNoSourceConfigured(t *testing.T) { // An empty base URL means no constraint host is configured; it must classify as // E_FETCH (surfaced at the fetch phase) and name the env var to set. - _, err := FetchConstraints(t.Context(), "", "serverless/serverless-v4", t.TempDir()) + _, err := FetchConstraints(t.Context(), "", "serverless/serverless-v4", t.TempDir(), true) var pe *PipelineError require.ErrorAs(t, err, &pe) assert.Equal(t, ErrFetch, pe.Code) @@ -116,7 +116,7 @@ func TestFetchConstraintsCreatesCacheDir(t *testing.T) { })) defer srv.Close() - _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", cacheDir) + _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", cacheDir, true) require.NoError(t, err) // The cache file was written into the freshly created directory. written, err := os.ReadFile(filepath.Join(cacheDir, cacheFileName("serverless/serverless-v4"))) @@ -124,6 +124,23 @@ func TestFetchConstraintsCreatesCacheDir(t *testing.T) { assert.Equal(t, sampleToml, string(written)) } +func TestFetchConstraintsSkipsCacheWriteWhenDisabled(t *testing.T) { + // With writeCache=false (the --check dry-run path), a successful live fetch + // must not write anything to cacheDir. + cacheDir := t.TempDir() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(sampleToml)) + })) + defer srv.Close() + + c, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", cacheDir, false) + require.NoError(t, err) + assert.False(t, c.FromCache) + entries, err := os.ReadDir(cacheDir) + require.NoError(t, err) + assert.Empty(t, entries, "no cache file should be written under --check") +} + func TestCacheFileNameInjective(t *testing.T) { // Distinct env keys that flatten to the same slug must not collide, so a // cache hit can never serve another environment's constraints. @@ -140,7 +157,7 @@ func TestFetchConstraintsHTTP(t *testing.T) { })) defer srv.Close() - c, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir()) + c, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir(), true) require.NoError(t, err) assert.False(t, c.FromCache) assert.Equal(t, "databricks-connect~=17.2.0", c.DatabricksConnect) @@ -155,7 +172,7 @@ func TestFetchConstraintsEnvKeyNotFound(t *testing.T) { })) defer srv.Close() - _, err := FetchConstraints(t.Context(), srv.URL, "dbr/99.9.x-scala2.12", t.TempDir()) + _, err := FetchConstraints(t.Context(), srv.URL, "dbr/99.9.x-scala2.12", t.TempDir(), true) var pe *PipelineError require.ErrorAs(t, err, &pe) assert.Equal(t, ErrEnvUnsupported, pe.Code) @@ -167,7 +184,7 @@ func TestFetchConstraintsTransportFailureNoCache(t *testing.T) { url := down.URL down.Close() - _, err := FetchConstraints(t.Context(), url, "serverless/serverless-v4", t.TempDir()) + _, err := FetchConstraints(t.Context(), url, "serverless/serverless-v4", t.TempDir(), true) var pe *PipelineError require.ErrorAs(t, err, &pe) assert.Equal(t, ErrFetch, pe.Code) @@ -182,7 +199,7 @@ func TestFetchConstraintsRejectsOversizedBody(t *testing.T) { })) defer srv.Close() - _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir()) + _, err := FetchConstraints(t.Context(), srv.URL, "serverless/serverless-v4", t.TempDir(), true) var pe *PipelineError require.ErrorAs(t, err, &pe) assert.Equal(t, ErrFetch, pe.Code) @@ -194,12 +211,12 @@ func TestFetchConstraintsFallsBackToCache(t *testing.T) { good := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(sampleToml)) })) - _, err := FetchConstraints(t.Context(), good.URL, "serverless/serverless-v4", cacheDir) + _, err := FetchConstraints(t.Context(), good.URL, "serverless/serverless-v4", cacheDir, true) require.NoError(t, err) good.Close() // Now the server is down; fetch must serve the cache. - c, err := FetchConstraints(t.Context(), good.URL, "serverless/serverless-v4", cacheDir) + c, err := FetchConstraints(t.Context(), good.URL, "serverless/serverless-v4", cacheDir, true) require.NoError(t, err) assert.True(t, c.FromCache) } diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index e3f152d730..3fa0737a91 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -191,8 +191,10 @@ func (p *Pipeline) resolve(ctx context.Context) (*TargetInfo, error) { } // fetch fetches constraints for the resolved target and records the fetch phase. +// Under --check the cache is not populated, so a dry run performs no disk writes +// (an existing cache is still read for offline fallback). func (p *Pipeline) fetch(ctx context.Context, target *TargetInfo) (*Constraints, error) { - c, err := FetchConstraints(ctx, p.ConstraintBaseURL, target.EnvKey, p.CacheDir) + c, err := FetchConstraints(ctx, p.ConstraintBaseURL, target.EnvKey, p.CacheDir, !p.Check) if err != nil { // FetchConstraints classifies the cause: E_ENV_UNSUPPORTED for a missing // key (404) versus E_FETCH for transport failure with no cache. Both are diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index 1a318cd187..8d6286c47b 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -81,12 +81,13 @@ func newTestServer(t *testing.T) *httptest.Server { func TestPipelineCheckMutatesNothing(t *testing.T) { dir := writeProject(t) before, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + cacheDir := t.TempDir() srv := newTestServer(t) defer srv.Close() p := &Pipeline{ Mode: ModeDefault, Check: true, ProjectDir: dir, - ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + ConstraintBaseURL: srv.URL, CacheDir: cacheDir, Flags: TargetFlags{Serverless: "v4"}, Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, } @@ -98,6 +99,11 @@ func TestPipelineCheckMutatesNothing(t *testing.T) { assert.Contains(t, res.Plan.Diff, "==3.12.*") after, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) assert.Equal(t, string(before), string(after)) // unchanged + + // --check must not populate the constraint cache either (no disk writes). + entries, err := os.ReadDir(cacheDir) + require.NoError(t, err) + assert.Empty(t, entries) } func TestPipelineCheckReRunPlanMatchesRealRun(t *testing.T) {