Skip to content

Commit 58cd63e

Browse files
denikIsaac
denik
and
Isaac
authored
Add libs/atomicfile and use it for in-place file saves (#6708)
Adds `libs/atomicfile.Write` (same-directory temp file + chmod + `os.Rename`) and routes in-place file saves through it, so a crash or a concurrent reader never sees a partial file. The written file always gets the caller-specified mode and does **not** inherit the replaced file's permissions. An optional `atomicfile.MkDir(perm)` creates the parent directory in the same call (the directory mode stays a deliberate per-caller choice: 0o700 for secrets, 0o755 otherwise). ### Pure refactor (already atomic, behavior unchanged) - `bundle/direct/dstate/state.go` — direct-engine deployment `resources.json` - `libs/auth/storage/filestore.go` — token cache (`atomicWriteFile` wrapper inlined) - `libs/aitools/installer/state.go` - `libs/localenv/constraints.go` (inlined the now-thin `writeCacheAtomic`) - `libs/clicompat/clicompat.go` - `experimental/ssh/internal/sshconfig/knownhosts.go` - `cmd/genie/conversations.go` — best-effort store (also drops a double-close) ### Bug fixes - `libs/cache/file_cache.go` — removed the `os.Remove`-before-`Rename` hack that deleted the destination first and defeated atomicity. `os.Rename` already replaces an existing file on Windows. - `cmd/sandbox/state.go`, `cmd/sandbox/sshconfig.go` — used a fixed `<path>.tmp` temp name, so concurrent CLI runs collided on it. `CreateTemp` avoids that. ### New atomicity (was a plain in-place write) Deployment / sync state (the terraform engine matches the direct engine now): - `bundle/statemgmt/state_pull.go` — direct-engine state pulled from the remote (closes a `// TODO: write + rename`) - `bundle/deploy/state_update.go` — terraform-engine deployment state written after deploy - `bundle/deploy/state_pull.go` — terraform-engine state pulled from the remote - `libs/sync/snapshot.go` — the sync snapshot, rewritten every sync and read back to compute the file diff Files a user owns (a torn write would lose their content): - `bundle/configsync/output.go` — local source config files rewritten on sync - `libs/completion/{install,uninstall}.go` — the user's shell RC file; mode preserved via the existing `info.Mode()`. `install` now rewrites the file instead of `O_APPEND`, matching `uninstall` (`installFish` wrapper inlined). - `libs/localenv/pipeline.go` — `pyproject.toml` on merge - `experimental/ssh/internal/sshconfig/sshconfig.go` — the user's `~/.ssh/config` (Include add + migration) and the per-host tunnel config - `experimental/ssh/internal/vscode/settings.go` — the user's VSCode `settings.json` CLI-owned state/cache read back on the next run: - `cmd/labs/project/{login,project}.go` — labs auth and version files - `cmd/labs/localcache/jsonfile.go` — labs JSON cache - `bundle/configsync/diff.go` — local config snapshot used for the next diff ### Deliberately left out - `libs/databrickscfg/ops.go` — writes via go-ini `SaveTo` in place; needs a render-to-temp approach, its own PR - `libs/aitools/installer` file writes — staged in a temp dir already promoted by rename, so the atomicity boundary is the directory - WAL files, `O_EXCL` create-once files (locks, greenfield configs), and append-mode log files — intentionally not atomic-replace This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <[email protected]>
1 parent 6c2479b commit 58cd63e

27 files changed

Lines changed: 295 additions & 374 deletions

File tree

‎.nextchanges/cli/atomic-file-writes.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* Write local state, cache, and config files atomically so an interrupted or concurrent write cannot corrupt them. ([#6708](https://github.com/databricks/cli/pull/6708))

‎bundle/configsync/diff.go‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"io"
88
"io/fs"
99
"os"
10-
"path/filepath"
1110
"strings"
1211

1312
"github.com/databricks/cli/bundle"
@@ -17,6 +16,7 @@ import (
1716
"github.com/databricks/cli/bundle/deployplan"
1817
"github.com/databricks/cli/bundle/direct"
1918
"github.com/databricks/cli/bundle/direct/dstate"
19+
"github.com/databricks/cli/libs/atomicfile"
2020
"github.com/databricks/cli/libs/dyn"
2121
"github.com/databricks/cli/libs/dyn/convert"
2222
"github.com/databricks/cli/libs/log"
@@ -260,13 +260,7 @@ func ensureSnapshotAvailable(ctx context.Context, b *bundle.Bundle, engine engin
260260
return fmt.Errorf("reading snapshot content: %w", err)
261261
}
262262

263-
localStateDir := filepath.Dir(localPathSnapshot)
264-
err = os.MkdirAll(localStateDir, 0o700)
265-
if err != nil {
266-
return fmt.Errorf("creating snapshot directory: %w", err)
267-
}
268-
269-
err = os.WriteFile(localPathSnapshot, content, 0o600)
263+
err = atomicfile.Write(localPathSnapshot, content, 0o600, atomicfile.MkDir(0o700))
270264
if err != nil {
271265
return fmt.Errorf("writing snapshot file: %w", err)
272266
}

‎bundle/configsync/output.go‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,9 @@ import (
66
"errors"
77
"fmt"
88
"io"
9-
"os"
10-
"path/filepath"
119

1210
"github.com/databricks/cli/bundle"
11+
"github.com/databricks/cli/libs/atomicfile"
1312
"github.com/databricks/cli/libs/telemetry"
1413
"github.com/databricks/cli/libs/telemetry/protos"
1514
)
@@ -91,12 +90,7 @@ func WriteResult(out io.Writer, jsonOutput bool, stats *Stats, files []FileChang
9190
// SaveFiles writes all file changes to disk.
9291
func SaveFiles(ctx context.Context, b *bundle.Bundle, files []FileChange) error {
9392
for _, file := range files {
94-
err := os.MkdirAll(filepath.Dir(file.Path), 0o755)
95-
if err != nil {
96-
return err
97-
}
98-
99-
err = os.WriteFile(file.Path, []byte(file.ModifiedContent), 0o644)
93+
err := atomicfile.Write(file.Path, []byte(file.ModifiedContent), 0o644, atomicfile.MkDir(0o755))
10094
if err != nil {
10195
return err
10296
}

‎bundle/deploy/state_pull.go‎

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"github.com/databricks/cli/bundle"
1313
"github.com/databricks/cli/bundle/deploy/files"
14+
"github.com/databricks/cli/libs/atomicfile"
1415
"github.com/databricks/cli/libs/diag"
1516
"github.com/databricks/cli/libs/filer"
1617
"github.com/databricks/cli/libs/log"
@@ -44,37 +45,22 @@ func (s *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic
4445
return diag.FromErr(err)
4546
}
4647

47-
local, err := os.OpenFile(statePath, os.O_CREATE|os.O_RDWR, 0o600)
48-
if err != nil {
49-
return diag.FromErr(err)
50-
}
51-
defer local.Close()
52-
5348
data := remote.Bytes()
5449
err = validateRemoteStateCompatibility(bytes.NewReader(data))
5550
if err != nil {
5651
return diag.FromErr(err)
5752
}
5853

59-
if !isLocalStateStale(local, bytes.NewReader(data)) {
54+
// A missing or unreadable local file counts as stale, so we write the remote copy.
55+
localData, _ := os.ReadFile(statePath)
56+
if !isLocalStateStale(bytes.NewReader(localData), bytes.NewReader(data)) {
6057
log.Infof(ctx, "Local deployment state is the same or newer, ignoring remote state")
6158
return nil
6259
}
6360

64-
// Truncating the file before writing
65-
err = local.Truncate(0)
66-
if err != nil {
67-
return diag.FromErr(err)
68-
}
69-
_, err = local.Seek(0, 0)
70-
if err != nil {
71-
return diag.FromErr(err)
72-
}
73-
7461
// Write file to disk.
7562
log.Infof(ctx, "Writing remote deployment state file to local cache directory")
76-
_, err = io.Copy(local, bytes.NewReader(data))
77-
if err != nil {
63+
if err := atomicfile.Write(statePath, data, 0o600); err != nil {
7864
return diag.FromErr(err)
7965
}
8066

‎bundle/deploy/state_update.go‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,16 @@
11
package deploy
22

33
import (
4-
"bytes"
54
"context"
65
"encoding/json"
76
"errors"
8-
"io"
97
"io/fs"
108
"os"
119
"time"
1210

1311
"github.com/databricks/cli/bundle"
1412
"github.com/databricks/cli/internal/build"
13+
"github.com/databricks/cli/libs/atomicfile"
1514
"github.com/databricks/cli/libs/diag"
1615
"github.com/databricks/cli/libs/log"
1716
"github.com/google/uuid"
@@ -56,21 +55,14 @@ func (s *stateUpdate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnost
5655
if err != nil {
5756
return diag.FromErr(err)
5857
}
59-
// Write the state back to the file.
60-
f, err := os.OpenFile(statePath, os.O_CREATE|os.O_RDWR|os.O_TRUNC, 0o600)
61-
if err != nil {
62-
log.Infof(ctx, "Unable to open deployment state file: %s", err)
63-
return diag.FromErr(err)
64-
}
65-
defer f.Close()
66-
6758
data, err := json.Marshal(state)
6859
if err != nil {
6960
return diag.FromErr(err)
7061
}
7162

72-
_, err = io.Copy(f, bytes.NewReader(data))
73-
if err != nil {
63+
// Write the state back to the file.
64+
if err := atomicfile.Write(statePath, data, 0o600); err != nil {
65+
log.Infof(ctx, "Unable to write deployment state file: %s", err)
7466
return diag.FromErr(err)
7567
}
7668

‎bundle/direct/dstate/state.go‎

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/databricks/cli/bundle/deployplan"
2020
"github.com/databricks/cli/bundle/statemgmt/resourcestate"
2121
"github.com/databricks/cli/internal/build"
22+
"github.com/databricks/cli/libs/atomicfile"
2223
"github.com/databricks/cli/libs/cmdctx"
2324
"github.com/databricks/cli/libs/dms"
2425
"github.com/databricks/cli/libs/log"
@@ -969,31 +970,7 @@ func (db *DeploymentState) unlockedSave() error {
969970
return err
970971
}
971972

972-
dir := filepath.Dir(db.Path)
973-
if err := os.MkdirAll(dir, 0o755); err != nil {
974-
return fmt.Errorf("failed to create directory %#v: %w", dir, err)
975-
}
976-
977-
// CreateTemp creates the file with mode 0o600, matching the state file.
978-
tmp, err := os.CreateTemp(dir, "."+filepath.Base(db.Path)+".tmp-*")
979-
if err != nil {
980-
return fmt.Errorf("failed to create temp file for %#v: %w", db.Path, err)
981-
}
982-
tmpPath := tmp.Name()
983-
// Cleans up the temp file on failure; a no-op once the rename succeeded.
984-
defer os.Remove(tmpPath)
985-
986-
if _, err := tmp.Write(data); err != nil {
987-
tmp.Close()
988-
return fmt.Errorf("failed to write %#v: %w", tmpPath, err)
989-
}
990-
991-
// Close before the rename: on Windows the file must not be open for writing.
992-
if err := tmp.Close(); err != nil {
993-
return fmt.Errorf("failed to close %#v: %w", tmpPath, err)
994-
}
995-
996-
if err := os.Rename(tmpPath, db.Path); err != nil {
973+
if err := atomicfile.Write(db.Path, data, 0o600, atomicfile.MkDir(0o755)); err != nil {
997974
return fmt.Errorf("failed to save resources state to %#v: %w", db.Path, err)
998975
}
999976

‎bundle/statemgmt/state_pull.go‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/databricks/cli/bundle"
1717
"github.com/databricks/cli/bundle/config/engine"
1818
"github.com/databricks/cli/bundle/deploy"
19+
"github.com/databricks/cli/libs/atomicfile"
1920
"github.com/databricks/cli/libs/diag"
2021
"github.com/databricks/cli/libs/filer"
2122
"github.com/databricks/cli/libs/log"
@@ -194,16 +195,7 @@ func PullResourcesState(ctx context.Context, b *bundle.Bundle, alwaysPull Always
194195
localStatePath = localPathDirect
195196
}
196197

197-
localStateDir := filepath.Dir(localStatePath)
198-
199-
err := os.MkdirAll(localStateDir, 0o700)
200-
if err != nil {
201-
logdiag.LogError(ctx, err)
202-
return ctx, winner
203-
}
204-
205-
// TODO: write + rename
206-
err = os.WriteFile(localStatePath, winner.Content, 0o600)
198+
err := atomicfile.Write(localStatePath, winner.Content, 0o600, atomicfile.MkDir(0o700))
207199
if err != nil {
208200
logdiag.LogError(ctx, err)
209201
return ctx, winner

‎cmd/genie/conversations.go‎

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"path/filepath"
88
"time"
99

10+
"github.com/databricks/cli/libs/atomicfile"
1011
"github.com/databricks/cli/libs/env"
1112
)
1213

@@ -82,25 +83,7 @@ func saveStore(path string, store conversationStore) {
8283
if err != nil {
8384
return
8485
}
85-
if err := os.MkdirAll(filepath.Dir(path), conversationDirPerm); err != nil {
86-
return
87-
}
88-
tmp, err := os.CreateTemp(filepath.Dir(path), ".genie-conversations-*.tmp")
89-
if err != nil {
90-
return
91-
}
92-
defer os.Remove(tmp.Name())
93-
defer tmp.Close()
94-
if _, err := tmp.Write(raw); err != nil {
95-
return
96-
}
97-
if err := tmp.Chmod(conversationFilePerm); err != nil {
98-
return
99-
}
100-
if err := tmp.Close(); err != nil {
101-
return
102-
}
103-
_ = os.Rename(tmp.Name(), path)
86+
_ = atomicfile.Write(path, raw, conversationFilePerm, atomicfile.MkDir(conversationDirPerm))
10487
}
10588

10689
// lookupConversationID returns the server conversation id mapped to sessionID on

‎cmd/labs/localcache/jsonfile.go‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"path/filepath"
1212
"time"
1313

14+
"github.com/databricks/cli/libs/atomicfile"
1415
"github.com/databricks/cli/libs/log"
1516
)
1617

@@ -73,19 +74,7 @@ func (r *LocalCache[T]) writeCache(ctx context.Context, data T) (T, error) {
7374
return r.zero, fmt.Errorf("json marshal: %w", err)
7475
}
7576
cacheFile := r.FileName()
76-
err = os.WriteFile(cacheFile, raw, userRW)
77-
if errors.Is(err, fs.ErrNotExist) {
78-
cacheDir := filepath.Dir(cacheFile)
79-
err := os.MkdirAll(cacheDir, ownerRWXworldRX)
80-
if err != nil {
81-
return r.zero, fmt.Errorf("create %s: %w", cacheDir, err)
82-
}
83-
err = os.WriteFile(cacheFile, raw, userRW)
84-
if err != nil {
85-
return r.zero, fmt.Errorf("retry save cache: %w", err)
86-
}
87-
return data, nil
88-
} else if err != nil {
77+
if err := atomicfile.Write(cacheFile, raw, userRW, atomicfile.MkDir(ownerRWXworldRX)); err != nil {
8978
return r.zero, fmt.Errorf("save cache: %w", err)
9079
}
9180
return data, nil

‎cmd/labs/project/login.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7-
"os"
87

98
"github.com/databricks/cli/cmd/root"
9+
"github.com/databricks/cli/libs/atomicfile"
1010
"github.com/databricks/cli/libs/cmdio"
1111
"github.com/databricks/cli/libs/databrickscfg/cfgpickers"
1212
"github.com/databricks/cli/libs/log"
@@ -120,5 +120,5 @@ func (lc *loginConfig) save(ctx context.Context) error {
120120
return err
121121
}
122122
log.Debugf(ctx, "Writing auth configuration to: %s", authFile)
123-
return os.WriteFile(authFile, raw, ownerRW)
123+
return atomicfile.Write(authFile, raw, ownerRW)
124124
}

0 commit comments

Comments
 (0)