Skip to content

fix: Config saves are non-atomic and can corrupt config.yaml on interruption#745

Closed
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-f645de51
Closed

fix: Config saves are non-atomic and can corrupt config.yaml on interruption#745
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-f645de51

Conversation

@sam-saffron-jarvis
Copy link
Copy Markdown
Contributor

What changed

  • replaced the direct os.WriteFile calls used by config saves in internal/config/config.go with a small writeFileAtomically helper
  • the helper writes to a temp file in the same directory, fsyncs it, applies the expected mode, and then atomically renames it into place
  • updated both config write paths covered by this issue:
    • rewriteProviderEnvSections
    • Save
  • added focused tests that verify:
    • atomic writes replace the file contents and enforce 0600
    • if temp-file creation fails, the existing config.yaml contents remain untouched

Why this is high-value

~/.config/term-llm/config.yaml is core persisted state for provider defaults and auth-related settings. Before this change, these saves truncated the live file and then rewrote it in place. If the process was interrupted mid-write, or the write only partially completed, users could be left with an empty or malformed config and broken startup/auth flows.

With the temp-file + sync + rename pattern, the old config stays intact until the replacement file is fully written and ready, which removes the torn-write corruption mode from these paths.

Validation

  • gofmt -w internal/config/config.go internal/config/config_test.go
  • go build ./...
  • go test ./internal/config
  • go test ./...
  • git diff --check

@SamSaffron
Copy link
Copy Markdown
Owner

Closing in favor of applying the worthwhile changes directly on main staging.

@SamSaffron SamSaffron closed this May 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants