diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 39d13bb1b4..c50e09160c 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -24,6 +24,10 @@ const ( venvDir = ".venv" ) +// backupTimestampLayout is the UTC, second-resolution stamp in a timestamped +// backup name (pyproject.toml..bak); shared with tests that pin the name. +const backupTimestampLayout = "20060102T150405Z" + // artifactSource values reported in --json resolved.artifactSource (spec §6). const ( artifactNetwork = "network" @@ -60,6 +64,18 @@ type Pipeline struct { // res accumulates phase statuses and result fields as the run progresses. res *Result + + // nowFn supplies the time for backup filenames; nil means time.Now. Injected + // in tests so backup names are deterministic. + nowFn func() time.Time +} + +// clock returns the current time, using nowFn when injected. +func (p *Pipeline) clock() time.Time { + if p.nowFn != nil { + return p.nowFn() + } + return time.Now() } // Run executes all pipeline phases in order and returns a fully populated Result. @@ -271,11 +287,60 @@ func (p *Pipeline) pyprojectPath() string { return filepath.Join(p.ProjectDir, pyprojectFile) } -// backupPath returns the path to the pyproject.toml backup file. +// backupPath returns the path to the canonical pyproject.toml backup file. func (p *Pipeline) backupPath() string { return filepath.Join(p.ProjectDir, backupFile) } +// timestampedBackupBase is the /pyproject.toml. stem a +// non-first backup name is built from. +func (p *Pipeline) timestampedBackupBase() string { + return filepath.Join(p.ProjectDir, pyprojectFile+"."+p.clock().UTC().Format(backupTimestampLayout)) +} + +// backupCurrent writes content to a backup of pyproject.toml and returns its +// path, never overwriting an existing backup (invariant 2). The canonical +// pyproject.toml.bak is written once and kept as the pristine original; later +// backups are pyproject.toml..bak. mode is preserved onto the file. +func (p *Pipeline) backupCurrent(content []byte, mode os.FileMode) (string, error) { + canonical := p.backupPath() + switch err := writeNew(canonical, content, mode); { + case err == nil: + return canonical, nil + case !errors.Is(err, os.ErrExist): + return "", err + } + + base := p.timestampedBackupBase() + candidate := base + ".bak" + for i := 1; ; i++ { + // Only a name collision advances the suffix, so the loop terminates. + switch err := writeNew(candidate, content, mode); { + case err == nil: + return candidate, nil + case !errors.Is(err, os.ErrExist): + return "", err + } + candidate = fmt.Sprintf("%s-%d.bak", base, i) + } +} + +// plannedBackupName previews the backup name a real run would create — canonical +// .bak if none exists yet, else a timestamped name — for --dry-run only. Best-effort: +// an unstattable .bak returns an error the caller treats as "nothing to report". +func (p *Pipeline) plannedBackupName() (string, error) { + canonical := p.backupPath() + _, statErr := os.Stat(canonical) + switch { + case errors.Is(statErr, os.ErrNotExist): + return canonical, nil + case statErr == nil: + return p.timestampedBackupBase() + ".bak", nil + default: + return "", statErr + } +} + // mergePlan computes the merged pyproject.toml bytes (without writing to disk), // decides greenfield vs. existing, and builds the Plan (populated only under // --dry-run). dbcPin is the databricks-connect pin to inject, or "" in @@ -283,7 +348,6 @@ func (p *Pipeline) backupPath() string { // write into [tool.databricks.environment], or "" for a cluster target. func (p *Pipeline) mergePlan(_ context.Context, pyMinor string, c *Constraints, dbcPin, envVersion string) (merged []byte, greenfield bool, err error) { pyproject := p.pyprojectPath() - backup := p.backupPath() // The merge base is the live pyproject.toml, not the backup. MergeManaged // rewrites only the three managed regions and preserves every other byte, and @@ -367,13 +431,11 @@ func (p *Pipeline) mergePlan(_ context.Context, pyMinor string, c *Constraints, ChangedRegions: changedRegions, WouldInstallPython: pyMinor, } - // Report a backup only when a real run would actually write one: for an - // existing project with no .bak yet. On a re-run the .bak already exists and - // applyMerge keeps it (does not re-write), so claiming a backup here would - // describe a write that won't happen. - if !greenfield { - if _, statErr := os.Stat(backup); errors.Is(statErr, os.ErrNotExist) { - plan.WouldBackup = filepath.ToSlash(backup) + // Report a backup only when the run would actually write one (i.e. it changes + // the file); a no-op re-run writes none. + if !greenfield && !bytes.Equal(merged, baseBytes) { + if backupName, statErr := p.plannedBackupName(); statErr == nil { + plan.WouldBackup = filepath.ToSlash(backupName) } } p.res.Plan = plan @@ -381,44 +443,40 @@ func (p *Pipeline) mergePlan(_ context.Context, pyMinor string, c *Constraints, return merged, greenfield, nil } -// applyMerge writes the merged bytes to disk, backing up an existing -// pyproject.toml first. From this point on, disk has been mutated. +// applyMerge writes the merged bytes to disk, backing up the current +// pyproject.toml first. From the backup copy onward, disk has been mutated. func (p *Pipeline) applyMerge(_ context.Context, mergedBytes []byte, greenfield bool) error { pyproject := p.pyprojectPath() - backup := p.backupPath() if !greenfield { - // Back up before modifying so the user's original is recoverable - // (invariant 2). Only create the backup when one does not already exist: - // on a re-run the existing .bak is the canonical original unmanaged state - // (mergePlan used it as the base), so overwriting it with the already-merged - // pyproject.toml would destroy that baseline. - _, statErr := os.Stat(backup) - switch { - case statErr == nil: - // Backup already exists — keep it as the canonical baseline. - case errors.Is(statErr, os.ErrNotExist): - // copyFile creates/truncates the backup path, so a failure mid-copy may - // leave a partial .bak: report disk as mutated. - if err := copyFile(pyproject, backup); err != nil { - return p.fail(PhaseMerge, true, NewError(ErrMerge, err, "backup pyproject.toml failed")) - } - default: - // An existing-but-unstattable backup must not be overwritten (that would - // destroy the recoverable original); fail before any write instead. - return p.fail(PhaseMerge, false, NewError(ErrMerge, statErr, "cannot stat backup %s", filepath.ToSlash(backup))) + // Stat+read up front: mode is preserved onto the backup, content is the + // no-op base and the backup source. Fail before any write (no mutation yet) + // rather than swallow a stat/read error on an existing pyproject.toml. + info, statErr := os.Stat(pyproject) + if statErr != nil { + return p.fail(PhaseMerge, false, NewError(ErrMerge, statErr, "stat pyproject.toml %s failed", filepath.ToSlash(pyproject))) + } + current, readErr := os.ReadFile(pyproject) + if readErr != nil { + return p.fail(PhaseMerge, false, NewError(ErrMerge, readErr, "read pyproject.toml %s failed", filepath.ToSlash(pyproject))) } - p.res.BackupPath = filepath.ToSlash(backup) - // Skip the write when the merged output already matches what is on disk. - // On an idempotent re-run mergePlan reproduces the current file byte for - // byte, so rewriting it would only advance the mtime — spuriously - // invalidating file watchers and uv.lock freshness checks — without - // changing content. The backup above is untouched (the existing .bak is - // kept), so this leaves disk exactly as it was. - if current, readErr := os.ReadFile(pyproject); readErr == nil && bytes.Equal(current, mergedBytes) { + // No-op: the merged output already matches disk. On an idempotent re-run + // mergePlan reproduces the current file byte for byte, so rewriting it would + // only advance the mtime — spuriously invalidating file watchers and uv.lock + // freshness checks — without changing content. Skip both the backup and the + // write, leaving disk (and every existing backup) exactly as it was. + if bytes.Equal(current, mergedBytes) { return nil } + + // Back up before overwriting (invariant 2). A partial backup is possible + // mid-write, so report disk as mutated on error. + backup, backupErr := p.backupCurrent(current, info.Mode().Perm()) + if backupErr != nil { + return p.fail(PhaseMerge, true, NewError(ErrMerge, backupErr, "backup pyproject.toml failed")) + } + p.res.BackupPath = filepath.ToSlash(backup) } if err := os.WriteFile(pyproject, mergedBytes, 0o644); err != nil { @@ -707,22 +765,21 @@ func sanitizeProjectName(name string) string { return out } -// copyFile copies src to dst, creating or overwriting dst. dst is created with -// src's permission bits: the backup preserves a locked-down pyproject.toml -// (e.g. 0o600 because it carries a private index URL) rather than widening it to -// a hardcoded 0o644. os.WriteFile only applies the mode when it creates the -// file, which is always the case for the freshly-created .bak. -func copyFile(src, dst string) error { - info, err := os.Stat(src) +// writeNew creates path with content, failing (os.ErrExist) rather than +// overwriting an existing file. This no-clobber guarantee (O_EXCL) is why a +// backup can never destroy an earlier one. mode sets the new file's permission +// bits, so a backup keeps a locked-down pyproject.toml's permissions. +func writeNew(path string, content []byte, mode os.FileMode) error { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode) if err != nil { - return fmt.Errorf("stat %s: %w", src, err) - } - data, err := os.ReadFile(src) - if err != nil { - return fmt.Errorf("read %s: %w", src, err) + return err } - if err := os.WriteFile(dst, data, info.Mode().Perm()); err != nil { - return fmt.Errorf("write %s: %w", dst, err) + _, werr := f.Write(content) + if err := errors.Join(werr, f.Close()); err != nil { + // Drop the partial file so it can't pose as a complete backup and, being + // O_EXCL-occupied, block a later run from reclaiming the name. Best-effort. + _ = os.Remove(path) + return err } return nil } diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index c4748a3198..a6782b5c43 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -577,7 +577,24 @@ func TestPipelineReRunDoesNotRewriteUnchangedPyproject(t *testing.T) { assert.Equal(t, firstInfo.ModTime(), secondInfo.ModTime(), "unchanged pyproject.toml must not be rewritten") } -func TestCopyFilePreservesMode(t *testing.T) { +func TestWriteNewRefusesToOverwrite(t *testing.T) { + // writeNew is the no-clobber primitive backups rely on: it creates a file but + // must fail rather than overwrite an existing one, so an earlier backup is never + // destroyed (invariant 2) even if two runs pick the same name. + dir := t.TempDir() + dst := filepath.Join(dir, "pyproject.toml.bak") + + require.NoError(t, writeNew(dst, []byte("first\n"), 0o644)) + got, _ := os.ReadFile(dst) + require.Equal(t, "first\n", string(got)) + + err := writeNew(dst, []byte("second\n"), 0o644) + require.ErrorIs(t, err, os.ErrExist) + got, _ = os.ReadFile(dst) + assert.Equal(t, "first\n", string(got), "writeNew must never overwrite an existing file") +} + +func TestWriteNewPreservesMode(t *testing.T) { if runtime.GOOS == "windows" { // Windows does not honor Unix permission bits. t.Skip("permission-bit preservation is Unix-only") @@ -585,10 +602,8 @@ func TestCopyFilePreservesMode(t *testing.T) { // A locked-down pyproject.toml (e.g. 0o600 because it carries a private index // URL) must not be widened when copied to the backup. dir := t.TempDir() - src := filepath.Join(dir, "pyproject.toml") - require.NoError(t, os.WriteFile(src, []byte("[project]\n"), 0o600)) dst := filepath.Join(dir, "pyproject.toml.bak") - require.NoError(t, copyFile(src, dst)) + require.NoError(t, writeNew(dst, []byte("[project]\n"), 0o600)) info, err := os.Stat(dst) require.NoError(t, err) assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) @@ -646,15 +661,15 @@ func TestPipelineUvMissingFailsAtPreflight(t *testing.T) { assertPreflightFailure(t, res, err, ErrUvMissing) } -func TestApplyMergeFailsOnUnstattableBackupWithoutOverwrite(t *testing.T) { +func TestApplyMergeFailsOnUnreadableDirWithoutOverwritingBackup(t *testing.T) { if runtime.GOOS == "windows" || os.Getuid() == 0 { // chmod-based stat blocking does not apply for root or on Windows. t.Skip("stat-permission enforcement not available") } - // Both pyproject.toml and its .bak live in a project dir that is made - // unsearchable, so os.Stat of the backup fails with a permission error rather - // than not-exist — isolating applyMerge's "can't stat" branch. applyMerge is - // called directly, bypassing the writability preflight. + // The project dir is made unsearchable, so applyMerge's up-front stat of + // pyproject.toml fails with a permission error: the run must abort before any + // write, no disk mutation claimed, and the existing backup left untouched. + // applyMerge is called directly, bypassing the writability preflight. dir := filepath.Join(t.TempDir(), "proj") require.NoError(t, os.Mkdir(dir, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), []byte("[project]\n"), 0o644)) @@ -669,7 +684,7 @@ func TestApplyMergeFailsOnUnstattableBackupWithoutOverwrite(t *testing.T) { var pe *PipelineError require.ErrorAs(t, err, &pe) assert.Equal(t, PhaseMerge, pe.FailurePhase) - assert.False(t, pe.DiskMutated, "no write should have happened before the stat check") + assert.False(t, pe.DiskMutated, "no write should have happened before the up-front stat") // The original backup must be intact. require.NoError(t, os.Chmod(dir, 0o755)) @@ -779,6 +794,162 @@ dev = ["databricks-connect~=17.2.0"] assert.Equal(t, string(original), string(bak)) } +// listBackups returns the basenames of every pyproject.toml backup in dir — the +// canonical .bak plus any timestamped ones — for count/identity assertions. +func listBackups(t *testing.T, dir string) []string { + t.Helper() + matches, err := filepath.Glob(filepath.Join(dir, "pyproject.toml.*.bak")) + require.NoError(t, err) + // The canonical pyproject.toml.bak does not match the ".*." glob, so add it + // explicitly when present. + if _, statErr := os.Stat(filepath.Join(dir, "pyproject.toml.bak")); statErr == nil { + matches = append(matches, filepath.Join(dir, "pyproject.toml.bak")) + } + names := make([]string, 0, len(matches)) + for _, m := range matches { + names = append(names, filepath.Base(m)) + } + return names +} + +func TestPipelineReRunWritesTimestampedBackupKeepingOriginal(t *testing.T) { + dir := t.TempDir() + // The canonical .bak already holds the pristine pre-first-sync original. + original := []byte(`[project] +name = "demo" +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.0.0"] +`) + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml.bak"), original, 0o644)) + // The live file is a prior sync's output the developer then edited: they reset a + // managed region (requires-python), so the coming merge will rewrite it — + // making this a real change, not a no-op. + live := []byte(`[project] +name = "demo" +requires-python = ">=3.9" +dependencies = ["rich"] + +[dependency-groups] +dev = ["databricks-connect~=17.2.0"] +`) + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), live, 0o644)) + + srv := newTestServer(t) + defer srv.Close() + + fixed := time.Date(2024, 1, 1, 15, 30, 0, 0, time.UTC) + p := &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + nowFn: func() time.Time { return fixed }, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + require.True(t, res.OK) + + // The canonical .bak is untouched — still the pristine original. + bak, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml.bak")) + assert.Equal(t, string(original), string(bak), "the pristine .bak must not be overwritten on a re-run") + + // A timestamped backup captured the pre-run live content the merge overwrote. + wantName := "pyproject.toml." + fixed.Format(backupTimestampLayout) + ".bak" + assert.Equal(t, wantName, filepath.Base(res.BackupPath)) + tsContent, err := os.ReadFile(res.BackupPath) + require.NoError(t, err) + assert.Equal(t, string(live), string(tsContent), "the timestamped backup must hold the pre-run content") + + // Both backups coexist; the managed region was applied to the live file. + assert.ElementsMatch(t, []string{"pyproject.toml.bak", wantName}, listBackups(t, dir)) + merged, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + assert.Contains(t, string(merged), `requires-python = "==3.12.*"`) +} + +func TestPipelineNoOpReRunWritesNoNewBackup(t *testing.T) { + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + newPipe := func() *Pipeline { + return &Pipeline{ + Mode: ModeDefault, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + } + + // First sync creates exactly the canonical .bak. + _, err := newPipe().Run(t.Context()) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"pyproject.toml.bak"}, listBackups(t, dir)) + + // A second, idempotent run changes nothing, so it must not write another backup. + res, err := newPipe().Run(t.Context()) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"pyproject.toml.bak"}, listBackups(t, dir), "a no-op re-run must not write a new backup") + assert.Empty(t, res.BackupPath, "a no-op re-run created no backup") +} + +func TestApplyMergeSameInstantBackupsGetUniqueNames(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml.bak"), []byte("ORIGINAL\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), []byte("current-1\n"), 0o644)) + + fixed := time.Date(2024, 1, 1, 15, 30, 0, 0, time.UTC) + p := &Pipeline{ + ProjectDir: dir, + nowFn: func() time.Time { return fixed }, + res: &Result{Phases: initialPhases()}, + } + + // First modifying apply: backs up "current-1" under a timestamped name. + require.NoError(t, p.applyMerge(t.Context(), []byte("merged-1\n"), false)) + first := p.res.BackupPath + require.NotEmpty(t, first) + + // Second modifying apply at the SAME instant: backs up "merged-1" but must not + // collide with or overwrite the first timestamped backup. + require.NoError(t, p.applyMerge(t.Context(), []byte("merged-2\n"), false)) + second := p.res.BackupPath + require.NotEmpty(t, second) + + assert.NotEqual(t, first, second, "same-instant backups must get distinct names") + c1, _ := os.ReadFile(first) + assert.Equal(t, "current-1\n", string(c1)) + c2, _ := os.ReadFile(second) + assert.Equal(t, "merged-1\n", string(c2)) + // The pristine .bak is still untouched. + bak, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml.bak")) + assert.Equal(t, "ORIGINAL\n", string(bak)) + // Three backups now coexist: the original plus two timestamped. + assert.Len(t, listBackups(t, dir), 3) +} + +func TestPipelineCheckFirstRunPlansCanonicalBackup(t *testing.T) { + // A --dry-run on an existing project that has never been synced (no .bak yet) + // must plan the canonical pyproject.toml.bak — the first backup a real run + // would create — and must not actually write it. + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + require.NotNil(t, res.Plan) + assert.Equal(t, "pyproject.toml.bak", filepath.Base(res.Plan.WouldBackup)) + assert.NoFileExists(t, filepath.Join(dir, "pyproject.toml.bak"), "--dry-run must not create the backup") +} + func TestPipelineUnreadableExistingIsNotTreatedAsGreenfield(t *testing.T) { if runtime.GOOS == "windows" || os.Getuid() == 0 { // chmod 000 does not block reads for root or on Windows.