fix: make SavedGraph.Save work on Windows - #24
Open
spagu wants to merge 1 commit into
Open
Conversation
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
spagu
added a commit
to tradik/mddb
that referenced
this pull request
Aug 24, 2026
github.com/coder/hnsw does not compile for Windows. Its SavedGraph.Save uses
github.com/google/renameio, which has no Windows build in either v1 or v2 —
verified against both, not assumed:
GOOS=windows go build undefined: renameio.TempFile
Go compiles a package as a whole, so this applied to us even though MDDB never
calls Save. We persist vectors ourselves in internal/vector/vector_store.go and
use four symbols from that package: Graph, MakeNode, NewGraph, Node. One
unreachable function was making the whole target impossible.
Fixed upstream rather than worked around: coder/hnsw#24 rewrites that call on
the standard library — os.CreateTemp in the target's directory, fsync, then
os.Rename over the target. Crash safety is unchanged, because os.Rename
replaces an existing file on every platform Go supports; on Windows it compiles
to MoveFileEx with MOVEFILE_REPLACE_EXISTING. The fsync stays before the
rename, which is the part that matters: a rename reaching the disk ahead of its
data loses both graphs instead of one. Upstream CI is green on the PR.
Until it merges, go.mod carries a replace onto a fork holding exactly that
commit — the fork is an unmodified mirror of the PR branch, no module rename
needed, because Go checks the replacement's declared module path against the
left side of the directive. go.sum pins the commit hash, so a force-push to the
fork fails the build loudly rather than substituting code silently. A replace
applies only to the main module, so nothing downstream inherits it. Removal is
tracked as WIN-005.
Drops github.com/google/renameio from the module graph entirely; LICENSE_AUDIT
updated (275 dependencies, was 276).
This does not make MDDB build on Windows. GOOS=windows now fails on
syscall.Statfs and syscall.Getrusage in incident_detector.go,
persistence_check.go and system_handlers.go — which is WIN-001, the next task.
It removes the blocker standing in front of that work.
go build ./... (linux) ok
go test ./... ok, 32 packages
go test -race ./internal/vector/... ok, 37.4s
go vet / gofmt -s / golangci-lint clean, 0 issues
go mod download && verify && tidy ok (the release.yml steps)
govulncheck 0 affecting this code
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
1 participant
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.
The problem
hnswdoes not compile for Windows:github.com/google/renameiois Unix-only —TempFileis not defined on Windows in either v1 or v2 (google/renameio#10). Since Go compiles a package as a whole, this blocks the entirehnswpackage on Windows, not justSavedGraph. Callers that only useGraph,NewGraphandNode— never touchingSavedGraph— cannot build either. That is how I ran into it: adding a Windows target to a project that embedshnswas its vector index.renameiois also the package's only non-test dependency.The change
SavedGraph.Saveis the single call site. It is replaced with the same sequence on the standard library:renameiorenameio.TempFile("", g.Path)os.CreateTemp(filepath.Dir(g.Path), filepath.Base(g.Path)+".tmp-*")tmp.Cleanup()tmp.Close()+os.Remove(tmpName)in a defertmp.CloseAtomicallyReplace()tmp.Sync()+tmp.Close()+os.Rename(tmpName, g.Path)The temporary file stays in the target's directory, so the rename never crosses a filesystem boundary.
The crash-safety property is unchanged.
os.Renamereplaces an existing file on every platform Go supports — on Windows it compiles toMoveFileExwithMOVEFILE_REPLACE_EXISTING— so an interrupted save still leaves the previous graph in place rather than a truncated one. Thefsyncis 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.Tests
Added
TestSavedGraph_ReplacesAndLeavesNoTempFiles, covering the two properties that are platform-dependent here: a secondSavereplaces the first graph rather than failing on an existing file, and no temporary file survives.Verified the test has teeth by mutation — removing the
os.Renamefails it:I did not add a test for the
fsync. Its effect is only observable across a power loss, so a unit test asserting it would assert nothing; removing theSynccall leaves the suite green. It is retained because it is the reasonrenameiowas here.Existing behaviour on Unix is unchanged, and
TestSavedGraphstill passes.