fix: make atomic state writes work on Windows - #88
Merged
nianzhibai merged 2 commits intoAug 14, 2026
Conversation
The directory fsync issued after rename in backup and backuptransfer is now best-effort. Windows rejects FlushFileBuffers on a read-only directory handle with ERROR_ACCESS_DENIED, so the server aborted at startup with "write server identity: ... Access is denied" even though the rename had already committed and the file on disk was intact. This mirrors the convention already documented in internal/config. Also skip the Unix permission guard on backup transfer state files when running on Windows: Chmod only toggles the read-only attribute there and Lstat always reports 0666 for a writable file, so the guard rejected every state file the process had just written itself. Access is governed by the directory ACL on that platform. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Problem
On Windows the server aborts during startup:
writeJSONAtomicininternal/backuptransfer/store.go— and its twin ininternal/backup/manager.go— fsyncs the parent directory afteros.Rename.Windows rejects
FlushFileBufferson a directory handle opened read-only viaos.Openand returnsERROR_ACCESS_DENIED. That error is propagated as fatal,even though the rename has already committed:
identity.jsonis intact on diskwhen the process dies.
A second, latent problem surfaces once the first is fixed.
readJSONFilerejects state files whose mode has any of
0o077set. On WindowsChmod(0600)only toggles the read-only attribute and
Lstatreports0666for any writablefile, so the guard rejects the very file the process just wrote itself. First
start hits bug 1 (file absent, write path); every restart afterwards hits bug 2
(file present, read path). Both have to go for the server to run.
This reproduces in the test suite, not just at runtime. On
main(8cda4a5), on Windows:
Change
the directory ACL instead.
internal/config/config.goalready does exactly this and documents why; thisbrings the other two atomic writers in line with that convention. The comment
wording is taken from there.
Tradeoff worth flagging
Making the directory fsync best-effort also silences genuine fsync failures on
Linux, so this is not a Windows-only behaviour change. The rationale is the one
already recorded in
internal/config: the rename has committed by that point,so returning an error tells the caller the write failed when it did not. Calling
it out explicitly rather than burying it.
Verification
go build ./...andgo vet ./internal/backup/... ./internal/backuptransfer/...cleango test ./internal/backuptransfer/... ./internal/config/...pass on Windows(they fail on
main, as shown above).peer-transfer/identity.jsonsuccessfullyinternal/backuptests could not be executed on the machineused here — the test binary is blocked by a Windows Application Control policy
(
go test -ccompiles fine; running it is denied regardless of path). The editin that package is byte-identical to the
backuptransferone, whose testspass. Worth confirming on CI or Linux.
🤖 Generated with Claude Code