From 64b968023c14b39d148d48d4be5c4e7c5263e4c5 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 14 Aug 2026 09:35:23 +0200 Subject: [PATCH 1/4] localenv: keep every pre-run pyproject.toml via timestamped backups *Why* setup-local backed up the pre-first-sync pyproject.toml to pyproject.toml.bak once and never refreshed it. On a re-run that overwrote a managed region the user had edited between syncs (requires-python, the databricks-connect pin, [tool.uv], [tool.databricks.environment]), that edit was discarded with no backup capturing it: the .bak still held the older original, so the pre-run state was unrecoverable. *What* - applyMerge now backs up the current pyproject.toml before every run that will actually change it. The first backup keeps the canonical pyproject.toml.bak name (the permanent pristine original); once it exists, later modifying runs write a distinct pyproject.toml..bak, so no prior state is ever clobbered. Same-second runs get a -N suffix. - The backup is skipped on a no-op re-run (merged output already on disk), so an idempotent re-sync writes nothing and clutters no backups. - Reordered applyMerge to choose the backup path (failing on an unstattable .bak) and no-op-check before copying; an unreadable backup is never shadowed. - mergePlan --dry-run WouldBackup now mirrors this: reported only when the run would change the file, named .bak or timestamped as a real run would. - Added a nowFn clock seam for deterministic backup-name tests. *Verification* go test ./libs/localenv/ (196 pass), go test ./cmd/environments/ (29 pass), go vet + gofmt clean. Co-authored-by: Isaac --- libs/localenv/pipeline.go | 142 +++++++++++++++++++++--------- libs/localenv/pipeline_test.go | 156 +++++++++++++++++++++++++++++++++ 2 files changed, 259 insertions(+), 39 deletions(-) diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 39d13bb1b4..fd7f7ca026 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -24,6 +24,11 @@ const ( venvDir = ".venv" ) +// backupTimestampLayout stamps the timestamp segment of a non-first backup +// filename (pyproject.toml..bak). UTC, second resolution; the trailing Z +// marks it as UTC. Kept as a named constant so the tests can pin the exact name. +const backupTimestampLayout = "20060102T150405Z" + // artifactSource values reported in --json resolved.artifactSource (spec §6). const ( artifactNetwork = "network" @@ -60,6 +65,20 @@ type Pipeline struct { // res accumulates phase statuses and result fields as the run progresses. res *Result + + // nowFn returns the current time, used only to stamp timestamped backup + // filenames. Left nil in production (defaults to time.Now via clock); tests + // inject a fixed clock so backup names are deterministic. + nowFn func() time.Time +} + +// clock returns the current time, honoring an injected nowFn seam and falling +// back to time.Now when unset. +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 +290,53 @@ 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) } +// chooseBackupPath decides where the current pyproject.toml should be copied +// before this run overwrites it. The first backup takes the canonical +// pyproject.toml.bak name and, never being overwritten, stays the permanent +// pristine pre-first-sync original; once it exists, every later modifying run +// gets a distinct pyproject.toml..bak so no earlier state is ever +// clobbered. A returned error means the canonical .bak exists but could not be +// stat'd: callers must not write, so an unreadable backup is never shadowed +// (invariant 2). A not-exist stat is not an error — it is the first-backup case. +func (p *Pipeline) chooseBackupPath() (string, error) { + canonical := p.backupPath() + _, statErr := os.Stat(canonical) + switch { + case errors.Is(statErr, os.ErrNotExist): + return canonical, nil + case statErr == nil: + return p.timestampedBackupPath(), nil + default: + return "", statErr + } +} + +// timestampedBackupPath returns a unique backup path of the form +// pyproject.toml..bak. When a backup with that exact +// second-resolution name already exists (two runs within the same second), a +// -N suffix is appended until the name is free, so an earlier backup is never +// overwritten. +func (p *Pipeline) timestampedBackupPath() string { + base := filepath.Join(p.ProjectDir, pyprojectFile+"."+p.clock().UTC().Format(backupTimestampLayout)) + candidate := base + ".bak" + for i := 1; ; i++ { + // Take the first name that does not already resolve to a file. A stat error + // other than not-exist means we can't confirm a collision here — stop rather + // than spin, and let the subsequent copy surface any real I/O problem. Only a + // nil error (the name is taken) advances to the next -N suffix, so the loop + // always terminates. + if _, err := os.Stat(candidate); err != nil { + return candidate + } + candidate = fmt.Sprintf("%s-%d.bak", base, i) + } +} + // 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 +344,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 +427,15 @@ 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 a real run would actually write one: an existing + // project whose merged output differs from what is on disk. A no-op re-run + // changes nothing and writes no backup, so claiming one here would describe a + // write that won't happen. Name it as applyMerge would — the canonical .bak + // for the first backup, else a fresh timestamped name (an unstattable .bak is + // skipped here; applyMerge fails the real run on it). + if !greenfield && !bytes.Equal(merged, baseBytes) { + if backupName, statErr := p.chooseBackupPath(); statErr == nil { + plan.WouldBackup = filepath.ToSlash(backupName) } } p.res.Plan = plan @@ -381,44 +443,46 @@ 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))) + // Decide where the backup would go before touching anything. An existing + // .bak that cannot be stat'd is fatal: never shadow or overwrite a backup we + // can't read (invariant 2). This runs before the no-op check so an unreadable + // backup fails the run rather than being silently skipped. + backup, statErr := p.chooseBackupPath() + if statErr != nil { + return p.fail(PhaseMerge, false, NewError(ErrMerge, statErr, "cannot stat backup %s", filepath.ToSlash(p.backupPath()))) } - 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) { + // Read the current file: it is both the no-op comparison base and the backup + // source. A read error on an existing pyproject.toml (permission change, + // transient I/O, delete race) must not be swallowed — fail before any write. + // No disk mutation has happened yet. + current, readErr := os.ReadFile(pyproject) + if readErr != nil { + return p.fail(PhaseMerge, false, NewError(ErrMerge, readErr, "read pyproject.toml %s failed", filepath.ToSlash(pyproject))) + } + + // 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 } + + // A change will be written; back up the current content first (invariant 2). + // copyFile creates/truncates the backup path, so a failure mid-copy may leave + // a partial backup: report disk as mutated. + if err := copyFile(pyproject, backup); err != nil { + return p.fail(PhaseMerge, true, NewError(ErrMerge, err, "backup pyproject.toml failed")) + } + p.res.BackupPath = filepath.ToSlash(backup) } if err := os.WriteFile(pyproject, mergedBytes, 0o644); err != nil { diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index c4748a3198..f92955f166 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -779,6 +779,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. From ccb7684e2c78bd4da548bfa1f589aad53a1bc9e1 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 14 Aug 2026 09:52:55 +0200 Subject: [PATCH 2/4] localenv: make backup writes atomic and clobber-proof (O_EXCL) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *Why* Review flagged that the timestamped-backup naming stat'd a candidate and then copied with a truncating write. That left two gaps: a candidate stat error other than not-exist made the copy fall back to overwriting an existing backup, and the stat/copy pair had a TOCTOU window against a concurrent run picking the same name. Both could destroy an earlier backup, breaking the no-clobber invariant. *What* - Replace copyFile (truncating) with writeNew, which creates the backup with O_CREATE|O_EXCL: it fails with os.ErrExist rather than overwriting, so a write can never clobber an existing backup and two runs can't both claim one name. - backupCurrent now tries the canonical pyproject.toml.bak, then timestamped names, advancing the -N suffix only on os.ErrExist (any other error is returned, not spun on) — atomic, terminating, and clobber-proof by construction. - applyMerge stats pyproject.toml up front for the mode bits and fails cleanly on a stat/read error before any write; the backup preserves the source's perms. - Split the dry-run preview into plannedBackupName (best-effort naming only), so --dry-run reporting stays a preview and never fails on an unstattable .bak. *Verification* go test ./libs/localenv/ (197) + ./cmd/environments/ (29) pass; go vet + gofmt clean. New tests: writeNew refuses to overwrite and preserves mode. Co-authored-by: Isaac --- libs/localenv/pipeline.go | 140 ++++++++++++++++++--------------- libs/localenv/pipeline_test.go | 35 ++++++--- 2 files changed, 103 insertions(+), 72 deletions(-) diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index fd7f7ca026..5c5f06cc73 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -295,48 +295,67 @@ func (p *Pipeline) backupPath() string { return filepath.Join(p.ProjectDir, backupFile) } -// chooseBackupPath decides where the current pyproject.toml should be copied -// before this run overwrites it. The first backup takes the canonical -// pyproject.toml.bak name and, never being overwritten, stays the permanent -// pristine pre-first-sync original; once it exists, every later modifying run -// gets a distinct pyproject.toml..bak so no earlier state is ever -// clobbered. A returned error means the canonical .bak exists but could not be -// stat'd: callers must not write, so an unreadable backup is never shadowed -// (invariant 2). A not-exist stat is not an error — it is the first-backup case. -func (p *Pipeline) chooseBackupPath() (string, error) { +// timestampedBackupBase is the path stem for a non-first backup: +// /pyproject.toml., to which ".bak" (or "-N.bak" on a +// collision) is appended. +func (p *Pipeline) timestampedBackupBase() string { + return filepath.Join(p.ProjectDir, pyprojectFile+"."+p.clock().UTC().Format(backupTimestampLayout)) +} + +// backupCurrent copies the current pyproject.toml content to a backup file +// without ever overwriting an existing one, and returns the path it wrote. The +// first backup takes the canonical pyproject.toml.bak name and, never being +// overwritten, stays the permanent pristine pre-first-sync original; once that +// exists, each call writes a distinct pyproject.toml..bak (with a -N +// suffix if that second-resolution name is already taken). Exclusive creation +// (see writeNew) makes each write atomic, so a backup neither clobbers an earlier +// one nor loses a race with a concurrent run to the same name (invariant 2). mode +// is the source file's permission bits, preserved onto the backup. +func (p *Pipeline) backupCurrent(content []byte, mode os.FileMode) (string, error) { + // Claim the canonical name for the first backup; if it already exists (or a + // concurrent run just claimed it), fall through to a timestamped name. canonical := p.backupPath() - _, statErr := os.Stat(canonical) - switch { - case errors.Is(statErr, os.ErrNotExist): + switch err := writeNew(canonical, content, mode); { + case err == nil: return canonical, nil - case statErr == nil: - return p.timestampedBackupPath(), nil - default: - return "", statErr + case !errors.Is(err, os.ErrExist): + return "", err } -} -// timestampedBackupPath returns a unique backup path of the form -// pyproject.toml..bak. When a backup with that exact -// second-resolution name already exists (two runs within the same second), a -// -N suffix is appended until the name is free, so an earlier backup is never -// overwritten. -func (p *Pipeline) timestampedBackupPath() string { - base := filepath.Join(p.ProjectDir, pyprojectFile+"."+p.clock().UTC().Format(backupTimestampLayout)) + base := p.timestampedBackupBase() candidate := base + ".bak" for i := 1; ; i++ { - // Take the first name that does not already resolve to a file. A stat error - // other than not-exist means we can't confirm a collision here — stop rather - // than spin, and let the subsequent copy surface any real I/O problem. Only a - // nil error (the name is taken) advances to the next -N suffix, so the loop - // always terminates. - if _, err := os.Stat(candidate); err != nil { - return candidate + // Only an already-taken name (os.ErrExist) advances to the next -N suffix, so + // the loop always terminates; any other error is a real I/O problem and is + // returned rather than spun on. + 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 right now, +// for --dry-run reporting only (no file is written): the canonical .bak when none +// exists yet, else a timestamped name. It is best-effort — an unstattable +// canonical .bak yields ("", err), which the dry-run caller treats as "no backup +// to report" rather than failing the preview. +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 @@ -434,7 +453,7 @@ func (p *Pipeline) mergePlan(_ context.Context, pyMinor string, c *Constraints, // for the first backup, else a fresh timestamped name (an unstattable .bak is // skipped here; applyMerge fails the real run on it). if !greenfield && !bytes.Equal(merged, baseBytes) { - if backupName, statErr := p.chooseBackupPath(); statErr == nil { + if backupName, statErr := p.plannedBackupName(); statErr == nil { plan.WouldBackup = filepath.ToSlash(backupName) } } @@ -449,19 +468,15 @@ func (p *Pipeline) applyMerge(_ context.Context, mergedBytes []byte, greenfield pyproject := p.pyprojectPath() if !greenfield { - // Decide where the backup would go before touching anything. An existing - // .bak that cannot be stat'd is fatal: never shadow or overwrite a backup we - // can't read (invariant 2). This runs before the no-op check so an unreadable - // backup fails the run rather than being silently skipped. - backup, statErr := p.chooseBackupPath() - if statErr != nil { - return p.fail(PhaseMerge, false, NewError(ErrMerge, statErr, "cannot stat backup %s", filepath.ToSlash(p.backupPath()))) - } - - // Read the current file: it is both the no-op comparison base and the backup - // source. A read error on an existing pyproject.toml (permission change, + // Stat and read the current file up front: its mode is preserved onto the + // backup and its content is both the no-op comparison base and the backup + // source. A stat/read error on an existing pyproject.toml (permission change, // transient I/O, delete race) must not be swallowed — fail before any write. // No disk mutation has happened yet. + 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))) @@ -477,10 +492,11 @@ func (p *Pipeline) applyMerge(_ context.Context, mergedBytes []byte, greenfield } // A change will be written; back up the current content first (invariant 2). - // copyFile creates/truncates the backup path, so a failure mid-copy may leave - // a partial backup: report disk as mutated. - if err := copyFile(pyproject, backup); err != nil { - return p.fail(PhaseMerge, true, NewError(ErrMerge, err, "backup pyproject.toml failed")) + // backupCurrent creates a fresh file (never overwriting an existing backup), + // but a partial write mid-copy is possible: 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) } @@ -771,22 +787,22 @@ 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 writes content to a newly created path, failing with os.ErrExist +// rather than overwriting an existing file (O_EXCL). This is the no-clobber +// primitive backups rely on: it can never destroy an earlier backup, and two +// runs racing to the same name can't both succeed. mode sets the new file's +// permission bits (subject to umask, as for any freshly created file), so a +// 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. +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) + cerr := f.Close() + if werr != nil { + return werr } - return nil + return cerr } diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index f92955f166..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)) From 9c252e826d66fa30618d3f3b3d35dcbf39eccdc5 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 14 Aug 2026 10:02:04 +0200 Subject: [PATCH 3/4] localenv: remove a partial backup if writeNew fails mid-write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *Why* writeNew creates the backup with O_EXCL, then writes. If the write (or the flushing close) failed after the file was created — e.g. ENOSPC — the empty or truncated file was left behind. Because O_EXCL then treats that name as occupied, a later run could never reclaim it, leaving the canonical pyproject.toml.bak permanently truncated and masquerading as the pristine original. *What* On a write/close error, remove the just-created file before returning, so the name is free for a retry and no partial file can pose as a complete backup. Best-effort: a failing Remove means a badly degraded filesystem, where the write error is the one worth surfacing. Errors from write and close are joined so a close-time flush failure is not lost. *Verification* go test ./libs/localenv/ (197) + ./cmd/environments/ (29) pass; go vet + gofmt clean. Co-authored-by: Isaac --- libs/localenv/pipeline.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 5c5f06cc73..211816d244 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -800,9 +800,15 @@ func writeNew(path string, content []byte, mode os.FileMode) error { return err } _, werr := f.Write(content) - cerr := f.Close() - if werr != nil { - return werr + if err := errors.Join(werr, f.Close()); err != nil { + // The file was created but may not hold the whole content (e.g. ENOSPC + // mid-write). Remove it so a partial/empty file can't masquerade as a + // complete backup and, being O_EXCL-occupied, block a later run from + // reclaiming the same name — which would otherwise leave the canonical + // pyproject.toml.bak permanently truncated. Best-effort: if Remove also + // fails the filesystem is badly degraded and the write error is what matters. + _ = os.Remove(path) + return err } - return cerr + return nil } From 9c1d478d824ae91528de8a5affe1aec0d02d59c1 Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Fri, 14 Aug 2026 10:59:43 +0200 Subject: [PATCH 4/4] localenv: tighten backup comments to contract + why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit *Why* The backup helpers' comments restated mechanics the code already shows (backupCurrent re-spec'd canonical→timestamped→-N; writeNew narrated umask; the dry-run block explained each branch). Verbose comments compete with the code and rot; a doc comment should state the contract and the one non-obvious caveat. *What* Trim the comments I added for backupCurrent, writeNew, plannedBackupName, timestampedBackupBase, the nowFn/clock seam, the backupTimestampLayout const, and the applyMerge stat/backup lines down to their load-bearing intent (no-clobber invariant, canonical = pristine original, loop termination, no-op skip). No code or behavior change; the pre-existing no-op comment is left as its author wrote it. *Verification* go test ./libs/localenv/ (197) + ./cmd/environments/ (29) pass; go vet + gofmt clean. Co-authored-by: Isaac --- libs/localenv/pipeline.go | 85 +++++++++++++-------------------------- 1 file changed, 28 insertions(+), 57 deletions(-) diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 211816d244..c50e09160c 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -24,9 +24,8 @@ const ( venvDir = ".venv" ) -// backupTimestampLayout stamps the timestamp segment of a non-first backup -// filename (pyproject.toml..bak). UTC, second resolution; the trailing Z -// marks it as UTC. Kept as a named constant so the tests can pin the exact name. +// 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). @@ -66,14 +65,12 @@ type Pipeline struct { // res accumulates phase statuses and result fields as the run progresses. res *Result - // nowFn returns the current time, used only to stamp timestamped backup - // filenames. Left nil in production (defaults to time.Now via clock); tests - // inject a fixed clock so backup names are deterministic. + // 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, honoring an injected nowFn seam and falling -// back to time.Now when unset. +// clock returns the current time, using nowFn when injected. func (p *Pipeline) clock() time.Time { if p.nowFn != nil { return p.nowFn() @@ -295,25 +292,17 @@ func (p *Pipeline) backupPath() string { return filepath.Join(p.ProjectDir, backupFile) } -// timestampedBackupBase is the path stem for a non-first backup: -// /pyproject.toml., to which ".bak" (or "-N.bak" on a -// collision) is appended. +// 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 copies the current pyproject.toml content to a backup file -// without ever overwriting an existing one, and returns the path it wrote. The -// first backup takes the canonical pyproject.toml.bak name and, never being -// overwritten, stays the permanent pristine pre-first-sync original; once that -// exists, each call writes a distinct pyproject.toml..bak (with a -N -// suffix if that second-resolution name is already taken). Exclusive creation -// (see writeNew) makes each write atomic, so a backup neither clobbers an earlier -// one nor loses a race with a concurrent run to the same name (invariant 2). mode -// is the source file's permission bits, preserved onto the backup. +// 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) { - // Claim the canonical name for the first backup; if it already exists (or a - // concurrent run just claimed it), fall through to a timestamped name. canonical := p.backupPath() switch err := writeNew(canonical, content, mode); { case err == nil: @@ -325,9 +314,7 @@ func (p *Pipeline) backupCurrent(content []byte, mode os.FileMode) (string, erro base := p.timestampedBackupBase() candidate := base + ".bak" for i := 1; ; i++ { - // Only an already-taken name (os.ErrExist) advances to the next -N suffix, so - // the loop always terminates; any other error is a real I/O problem and is - // returned rather than spun on. + // Only a name collision advances the suffix, so the loop terminates. switch err := writeNew(candidate, content, mode); { case err == nil: return candidate, nil @@ -338,11 +325,9 @@ func (p *Pipeline) backupCurrent(content []byte, mode os.FileMode) (string, erro } } -// plannedBackupName previews the backup name a real run would create right now, -// for --dry-run reporting only (no file is written): the canonical .bak when none -// exists yet, else a timestamped name. It is best-effort — an unstattable -// canonical .bak yields ("", err), which the dry-run caller treats as "no backup -// to report" rather than failing the preview. +// 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) @@ -446,12 +431,8 @@ 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: an existing - // project whose merged output differs from what is on disk. A no-op re-run - // changes nothing and writes no backup, so claiming one here would describe a - // write that won't happen. Name it as applyMerge would — the canonical .bak - // for the first backup, else a fresh timestamped name (an unstattable .bak is - // skipped here; applyMerge fails the real run on it). + // 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) @@ -468,11 +449,9 @@ func (p *Pipeline) applyMerge(_ context.Context, mergedBytes []byte, greenfield pyproject := p.pyprojectPath() if !greenfield { - // Stat and read the current file up front: its mode is preserved onto the - // backup and its content is both the no-op comparison base and the backup - // source. A stat/read error on an existing pyproject.toml (permission change, - // transient I/O, delete race) must not be swallowed — fail before any write. - // No disk mutation has happened yet. + // 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))) @@ -491,9 +470,8 @@ func (p *Pipeline) applyMerge(_ context.Context, mergedBytes []byte, greenfield return nil } - // A change will be written; back up the current content first (invariant 2). - // backupCurrent creates a fresh file (never overwriting an existing backup), - // but a partial write mid-copy is possible: report disk as mutated on error. + // 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")) @@ -787,13 +765,10 @@ func sanitizeProjectName(name string) string { return out } -// writeNew writes content to a newly created path, failing with os.ErrExist -// rather than overwriting an existing file (O_EXCL). This is the no-clobber -// primitive backups rely on: it can never destroy an earlier backup, and two -// runs racing to the same name can't both succeed. mode sets the new file's -// permission bits (subject to umask, as for any freshly created file), so a -// 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. +// 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 { @@ -801,12 +776,8 @@ func writeNew(path string, content []byte, mode os.FileMode) error { } _, werr := f.Write(content) if err := errors.Join(werr, f.Close()); err != nil { - // The file was created but may not hold the whole content (e.g. ENOSPC - // mid-write). Remove it so a partial/empty file can't masquerade as a - // complete backup and, being O_EXCL-occupied, block a later run from - // reclaiming the same name — which would otherwise leave the canonical - // pyproject.toml.bak permanently truncated. Best-effort: if Remove also - // fails the filesystem is badly degraded and the write error is what matters. + // 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 }