From ce54b1421f432555b8eb2a627323c27a61b48e7f Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 20 Aug 2026 11:00:35 +0200 Subject: [PATCH 1/2] direct: write deployment state atomically unlockedSave truncated the state file in place. A crash mid-write left a state file the CLI cannot parse next to an intact WAL that Open never reads, because Open parses the state file first and returns on error. The deployment state was then unrecoverable. Write to a temp file in the same directory and rename it over the state file instead; replayWAL already removes the WAL only after the save succeeds. Co-authored-by: Isaac --- .nextchanges/bundles/state-atomic-save.md | 1 + bundle/direct/dstate/state.go | 28 +++++++++++- bundle/direct/dstate/state_test.go | 53 +++++++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 .nextchanges/bundles/state-atomic-save.md diff --git a/.nextchanges/bundles/state-atomic-save.md b/.nextchanges/bundles/state-atomic-save.md new file mode 100644 index 0000000000..1b8128eb7c --- /dev/null +++ b/.nextchanges/bundles/state-atomic-save.md @@ -0,0 +1 @@ +Write the deployment state atomically so an interrupted save cannot leave a state file that the CLI refuses to read. diff --git a/bundle/direct/dstate/state.go b/bundle/direct/dstate/state.go index d7c1e8e44e..a2d3de9816 100644 --- a/bundle/direct/dstate/state.go +++ b/bundle/direct/dstate/state.go @@ -574,6 +574,14 @@ func (db *DeploymentState) ExportState(ctx context.Context) resourcestate.Export return ExportStateFromData(db.Data) } +// unlockedSave persists the in-memory state to db.Path by writing a temp file in +// the same directory and renaming it over the destination, so an interrupted save +// cannot leave a half-written state file behind. +// +// Writing in place would be unrecoverable: replayWAL saves the merged state and +// only then removes the WAL, and Open parses the state file before it looks at +// the WAL. A torn write would therefore leave a state file that Open rejects +// next to an intact WAL it never reads. func (db *DeploymentState) unlockedSave() error { data, err := json.MarshalIndent(db.Data, "", " ") if err != nil { @@ -585,8 +593,26 @@ func (db *DeploymentState) unlockedSave() error { return fmt.Errorf("failed to create directory %#v: %w", dir, err) } - err = os.WriteFile(db.Path, data, 0o600) + // CreateTemp creates the file with mode 0o600, matching the state file. + tmp, err := os.CreateTemp(dir, "."+filepath.Base(db.Path)+".tmp-*") if err != nil { + return fmt.Errorf("failed to create temp file for %#v: %w", db.Path, err) + } + tmpPath := tmp.Name() + // Cleans up the temp file on failure; a no-op once the rename succeeded. + defer os.Remove(tmpPath) + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return fmt.Errorf("failed to write %#v: %w", tmpPath, err) + } + + // Close before the rename: on Windows the file must not be open for writing. + if err := tmp.Close(); err != nil { + return fmt.Errorf("failed to close %#v: %w", tmpPath, err) + } + + if err := os.Rename(tmpPath, db.Path); err != nil { return fmt.Errorf("failed to save resources state to %#v: %w", db.Path, err) } diff --git a/bundle/direct/dstate/state_test.go b/bundle/direct/dstate/state_test.go index 3b5dc06221..6fc1bc4b3c 100644 --- a/bundle/direct/dstate/state_test.go +++ b/bundle/direct/dstate/state_test.go @@ -261,3 +261,56 @@ func TestGetOrInitLineageReadableBeforeWriteAndPersisted(t *testing.T) { assert.Equal(t, lineage, reopened.Data.Lineage) mustFinalize(t, &reopened) } + +// statFile stats through an open handle rather than by path: on Windows a +// path-based os.Stat resolves the file identity lazily, inside os.SameFile, +// which would re-resolve the path after the save and defeat the comparison in +// TestSaveReplacesStateFile. +func statFile(t *testing.T, path string) os.FileInfo { + t.Helper() + f, err := os.Open(path) + require.NoError(t, err) + defer f.Close() + info, err := f.Stat() + require.NoError(t, err) + return info +} + +// TestSaveReplacesStateFile pins that persisting state writes a new file and +// renames it over the previous one instead of truncating it in place. An +// in-place write that is interrupted leaves a state file that Open cannot +// parse, and Open fails on it before it looks at the WAL, so the intact WAL +// sitting next to it is never replayed and the deployment state is lost. +func TestSaveReplacesStateFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "state.json") + + var db DeploymentState + require.NoError(t, db.Open(t.Context(), path, WithRecovery(true), WithWrite(true))) + require.NoError(t, db.SaveState("jobs.my_job", "123", map[string]string{"key": "val"}, nil)) + mustFinalize(t, &db) + + before := statFile(t, path) + + var db2 DeploymentState + require.NoError(t, db2.Open(t.Context(), path, WithRecovery(true), WithWrite(true))) + require.NoError(t, db2.SaveState("jobs.my_job", "456", map[string]string{"key": "val2"}, nil)) + mustFinalize(t, &db2) + + assert.False(t, os.SameFile(before, statFile(t, path)), "state file was written in place") + + // The rename leaves nothing behind: no temp file, and no WAL. + entries, err := os.ReadDir(dir) + require.NoError(t, err) + var names []string + for _, entry := range entries { + names = append(names, entry.Name()) + } + assert.Equal(t, []string{"state.json"}, names) + + var db3 DeploymentState + require.NoError(t, db3.Open(t.Context(), path, WithRecovery(false), WithWrite(false))) + assert.Equal(t, "456", db3.GetResourceID("jobs.my_job")) + assert.Equal(t, 2, db3.Data.Serial) + mustFinalize(t, &db3) +} From f485b0148ea60068e477371b59cab602d42e93cf Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 20 Aug 2026 11:34:54 +0200 Subject: [PATCH 2/2] direct: drop the atomic-save mechanism test The test pinned how the state file is written (replaced, not truncated in place) rather than what callers observe. What matters is that the save lands; the mechanism is an implementation detail explained in a comment. Co-authored-by: Isaac --- bundle/direct/dstate/state_test.go | 53 ------------------------------ 1 file changed, 53 deletions(-) diff --git a/bundle/direct/dstate/state_test.go b/bundle/direct/dstate/state_test.go index 6fc1bc4b3c..3b5dc06221 100644 --- a/bundle/direct/dstate/state_test.go +++ b/bundle/direct/dstate/state_test.go @@ -261,56 +261,3 @@ func TestGetOrInitLineageReadableBeforeWriteAndPersisted(t *testing.T) { assert.Equal(t, lineage, reopened.Data.Lineage) mustFinalize(t, &reopened) } - -// statFile stats through an open handle rather than by path: on Windows a -// path-based os.Stat resolves the file identity lazily, inside os.SameFile, -// which would re-resolve the path after the save and defeat the comparison in -// TestSaveReplacesStateFile. -func statFile(t *testing.T, path string) os.FileInfo { - t.Helper() - f, err := os.Open(path) - require.NoError(t, err) - defer f.Close() - info, err := f.Stat() - require.NoError(t, err) - return info -} - -// TestSaveReplacesStateFile pins that persisting state writes a new file and -// renames it over the previous one instead of truncating it in place. An -// in-place write that is interrupted leaves a state file that Open cannot -// parse, and Open fails on it before it looks at the WAL, so the intact WAL -// sitting next to it is never replayed and the deployment state is lost. -func TestSaveReplacesStateFile(t *testing.T) { - dir := t.TempDir() - path := filepath.Join(dir, "state.json") - - var db DeploymentState - require.NoError(t, db.Open(t.Context(), path, WithRecovery(true), WithWrite(true))) - require.NoError(t, db.SaveState("jobs.my_job", "123", map[string]string{"key": "val"}, nil)) - mustFinalize(t, &db) - - before := statFile(t, path) - - var db2 DeploymentState - require.NoError(t, db2.Open(t.Context(), path, WithRecovery(true), WithWrite(true))) - require.NoError(t, db2.SaveState("jobs.my_job", "456", map[string]string{"key": "val2"}, nil)) - mustFinalize(t, &db2) - - assert.False(t, os.SameFile(before, statFile(t, path)), "state file was written in place") - - // The rename leaves nothing behind: no temp file, and no WAL. - entries, err := os.ReadDir(dir) - require.NoError(t, err) - var names []string - for _, entry := range entries { - names = append(names, entry.Name()) - } - assert.Equal(t, []string{"state.json"}, names) - - var db3 DeploymentState - require.NoError(t, db3.Open(t.Context(), path, WithRecovery(false), WithWrite(false))) - assert.Equal(t, "456", db3.GetResourceID("jobs.my_job")) - assert.Equal(t, 2, db3.Data.Serial) - mustFinalize(t, &db3) -}