From ad04fee59f41da3ae8ca988183fda2b825d60bef Mon Sep 17 00:00:00 2001 From: spagu Date: Mon, 24 Aug 2026 10:17:51 +0100 Subject: [PATCH] fix: make SavedGraph.Save work on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SavedGraph.Save is the package's only use of github.com/google/renameio, and renameio does not build on Windows — neither v1 nor v2 defines TempFile there. Because Go compiles a package as a whole, that makes the entire hnsw package uncompilable for GOOS=windows, including for callers that never touch SavedGraph: $ GOOS=windows GOARCH=amd64 go build ./... encode.go:304:14: undefined: renameio.TempFile This replaces the call with the same sequence on the standard library: create a temporary file in the target's directory, write, fsync, close, rename over the target. os.Rename replaces an existing file on every platform Go supports — on Windows it is MoveFileEx with MOVEFILE_REPLACE_EXISTING — so the crash-safety property is unchanged: an interrupted save leaves the previous graph in place rather than a truncated one. The fsync is kept, and kept before the rename. A rename that reaches the disk ahead of the data it points at is what turns a crash into the loss of both graphs rather than one. Adds a test for the two properties that differ by platform: a second Save replaces the first graph, and no temporary file survives. Verified by mutation — removing the rename fails the test. Drops the only non-test dependency in go.mod. go test ./... ok go vet ./... ok GOOS=windows GOARCH=amd64 go build ./... ok GOOS=windows GOARCH=amd64 go vet ./... ok --- encode.go | 31 ++++++++++++++++++++++++++----- encode_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ go.mod | 5 +---- go.sum | 2 -- 4 files changed, 67 insertions(+), 11 deletions(-) diff --git a/encode.go b/encode.go index de3cf83..6e2cc24 100644 --- a/encode.go +++ b/encode.go @@ -7,8 +7,7 @@ import ( "fmt" "io" "os" - - "github.com/google/renameio" + "path/filepath" ) // errorEncoder is a helper type to encode multiple values @@ -300,12 +299,22 @@ func LoadSavedGraph[K cmp.Ordered](path string) (*SavedGraph[K], error) { } // Save writes the graph to the file. +// +// The graph is written to a temporary file in the same directory, fsynced, and +// renamed over the target, so an interrupted save leaves the previous graph in +// place rather than a truncated one. os.Rename replaces an existing file on +// every platform Go supports, Windows included. func (g *SavedGraph[K]) Save() error { - tmp, err := renameio.TempFile("", g.Path) + tmp, err := os.CreateTemp(filepath.Dir(g.Path), filepath.Base(g.Path)+".tmp-*") if err != nil { return err } - defer tmp.Cleanup() + tmpName := tmp.Name() + defer func() { + // Both are no-ops once the rename below has succeeded. + _ = tmp.Close() + _ = os.Remove(tmpName) + }() wr := bufio.NewWriter(tmp) err = g.Export(wr) @@ -318,7 +327,19 @@ func (g *SavedGraph[K]) Save() error { return fmt.Errorf("flushing: %w", err) } - err = tmp.CloseAtomicallyReplace() + // Before the rename, not after: a rename that reaches the disk ahead of the + // data it points at is what loses both graphs instead of one. + err = tmp.Sync() + if err != nil { + return fmt.Errorf("syncing: %w", err) + } + + err = tmp.Close() + if err != nil { + return fmt.Errorf("closing: %w", err) + } + + err = os.Rename(tmpName, g.Path) if err != nil { return fmt.Errorf("closing atomically: %w", err) } diff --git a/encode_test.go b/encode_test.go index b19210e..19ea850 100644 --- a/encode_test.go +++ b/encode_test.go @@ -3,6 +3,8 @@ package hnsw import ( "bytes" "cmp" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -231,3 +233,41 @@ func BenchmarkGraph_Export(b *testing.B) { buf.Reset() } } + +// TestSavedGraph_ReplacesAndLeavesNoTempFiles covers what the atomic write is +// for: a second Save must replace the first graph rather than fail on an +// existing file, and it must not leave its temporary file behind. +// +// Both are worth asserting because os.Rename over an existing file is the one +// part of this that is platform-dependent — it is a plain rename on Unix and +// MoveFileEx with MOVEFILE_REPLACE_EXISTING on Windows. +func TestSavedGraph_ReplacesAndLeavesNoTempFiles(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "graph") + + g1, err := LoadSavedGraph[int](path) + require.NoError(t, err) + for i := 0; i < 8; i++ { + g1.Add(Node[int]{i, randFloats(1)}) + } + require.NoError(t, g1.Save()) + + // The second save writes over a file that already exists. + for i := 8; i < 24; i++ { + g1.Add(Node[int]{i, randFloats(1)}) + } + require.NoError(t, g1.Save()) + + g2, err := LoadSavedGraph[int](path) + require.NoError(t, err) + require.Equal(t, 24, g2.Len(), "the second save should have replaced the first") + requireGraphApproxEquals(t, g1.Graph, g2.Graph) + + entries, err := os.ReadDir(dir) + require.NoError(t, err) + names := make([]string, 0, len(entries)) + for _, e := range entries { + names = append(names, e.Name()) + } + require.Equal(t, []string{"graph"}, names, "no temporary file should survive a save") +} diff --git a/go.mod b/go.mod index 9d0d0f6..340f661 100644 --- a/go.mod +++ b/go.mod @@ -2,10 +2,7 @@ module github.com/coder/hnsw go 1.21.4 -require ( - github.com/google/renameio v1.0.1 - github.com/stretchr/testify v1.9.0 -) +require github.com/stretchr/testify v1.9.0 require ( github.com/chewxy/math32 v1.10.1 // indirect diff --git a/go.sum b/go.sum index f74eb2b..0bf4cf3 100644 --- a/go.sum +++ b/go.sum @@ -3,8 +3,6 @@ github.com/chewxy/math32 v1.10.1/go.mod h1:dOB2rcuFrCn6UHrze36WSLVPKtzPMRAQvBvUw github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/google/renameio v1.0.1 h1:Lh/jXZmvZxb0BBeSY5VKEfidcbcbenKjZFzM/q0fSeU= -github.com/google/renameio v1.0.1/go.mod h1:t/HQoYBZSsWSNK35C6CO/TpPLDVWvxOHboWUAweKUpk= github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=