localenv: keep every pre-run pyproject.toml via timestamped backups - #6268
Open
rugpanov wants to merge 4 commits into
Open
localenv: keep every pre-run pyproject.toml via timestamped backups#6268rugpanov wants to merge 4 commits into
rugpanov wants to merge 4 commits into
Conversation
Contributor
Waiting for approvalCould not determine reviewers from git history. Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
Collaborator
Integration test reportCommit: 9c1d478
8 interesting tests: 4 SKIP, 3 KNOWN, 1 RECOVERED
Top 1 slowest tests (at least 2 minutes):
|
*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.<UTC-timestamp>.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
*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
*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
rugpanov
force-pushed
the
dbconnect/setup-local-backup-no-clobber
branch
from
August 14, 2026 08:50
b096530 to
9c252e8
Compare
*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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
environments setup-localrewrites the managed regions of a project'spyproject.tomlon each run. Previously it backed the file up topyproject.toml.bakonce (the pristine pre-first-sync original) and never refreshed it. So if a user edited a managed region between syncs —requires-python, thedatabricks-connectpin,[tool.uv], or[tool.databricks.environment]— the next run overwrote that edit and no backup captured it:.bakstill held the older original, leaving the pre-run state unrecoverable.This change makes the backup capture every pre-run state:
pyproject.toml, the current file is backed up. The first backup keeps the canonicalpyproject.toml.bakname (the permanent pristine original); once that exists, each later modifying run writes a distinctpyproject.toml.<UTC-timestamp>.bak, with a-Nsuffix on a same-second collision — so no prior state is ever clobbered.pyproject.tomlnor clutters the directory.O_CREATE|O_EXCL(writeNew): creation is atomic and fails rather than overwriting, so a write can never destroy an earlier backup and two runs can't both claim one name. A partially written file (e.g.ENOSPCmid-write) is removed so it can't masquerade as a complete backup or block a later run from reclaiming the canonical name. The source file's permission bits are preserved onto the backup.--dry-runwouldBackupmirrors the real run: reported only when the run would change the file, named.bakor timestamped as the real run would name it.Tests
.bakas the original; a no-op re-run writes no new backup; same-second backups get unique names;writeNewrefuses to overwrite and preserves mode; first-run--dry-runplans the canonical.bak.go test ./libs/localenv/(197) and./cmd/environments/(29) pass;go vetandgofmtclean.This pull request and its description were written by Isaac.