You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
config: atomic config replacement does not sync file data or parent directory #1087
Linux amd64; Go 1.26.6. AI-assisted source review. No power-loss test was performed; exact failure behavior depends on the OS/filesystem and its write-ordering guarantees.
Steps to reproduce / validation needed
Use a disposable filesystem/VM and perform a config mutation that reaches writeConfigData.
Observe the write/close/rename syscall sequence; no file or directory sync is issued by this function.
For an end-to-end durability reproduction, inject a simulated crash/power loss after successful return but before writeback, then remount and inspect the config.
Step 3 is proposed validation, not an executed reproduction. This report establishes the missing durability barriers from the source, not a guaranteed corruption outcome on every filesystem.
Expected behavior
If a successful config update is intended to survive a system crash, persist the temporary file before replacement and persist the directory update where supported. Otherwise document that success provides atomic visibility, not crash durability.
Actual behavior / source evidence
writeConfigData:976-1008 writes a sibling temporary file, closes it and renames it over the destination. It does not call tmp.Sync or sync the parent directory before returning success.
The current code correctly avoids an ordinary partial in-place overwrite. However, acknowledgement can precede durable storage of the replacement. An update may be lost after a system crash; stronger corruption claims require filesystem-specific testing.
Suggested fix / regression coverage
Sync the temporary file before close/rename, then sync the parent directory where supported. Propagate failures and define platform-specific behavior. Add fault-injection coverage for sync failures and ordering; use a disposable crash-consistency harness for the end-to-end guarantee.
Related work
Merged #631 addresses securefile and credstore, not config.writeConfigData. #960 addresses cross-process read-modify-write locking, which is a different property.
Verification
Existing go vet ./... and go test ./... passed during the audit. No crash-durability test was run. This is a low-priority durability gap, not evidence of corruption during ordinary successful writes.
Confirmed on main: writeConfigData writes the temp file, closes it and renames it over the config, with no sync on the file or the directory. A successful return promises atomic visibility, not that the new config survives a crash.
Approving the narrow fix you describe, in the shape internal/sessions/store.go already uses: tmp.Sync() before the close, then the parent directory synced after the rename, with both failures returned. That store's syncDir skips Windows, where a directory handle can't be synced, and this should do the same rather than fail there. A seam that makes each sync fail is enough coverage. I wouldn't ask for a crash harness.
Activity
Vasanthdev2004 commentedon Sep 26, 2026
Confirmed on main:
writeConfigDatawrites the temp file, closes it and renames it over the config, with no sync on the file or the directory. A successful return promises atomic visibility, not that the new config survives a crash.Approving the narrow fix you describe, in the shape
internal/sessions/store.goalready uses:tmp.Sync()before the close, then the parent directory synced after the rename, with both failures returned. That store'ssyncDirskips Windows, where a directory handle can't be synced, and this should do the same rather than fail there. A seam that makes each sync fail is enough coverage. I wouldn't ask for a crash harness.