Skip to content

fix: atomically write config.json via temp+rename#1387

Merged
AllyW merged 9 commits into
masterfrom
feature/aone-84342946-atomic-config-write
Jul 21, 2026
Merged

fix: atomically write config.json via temp+rename#1387
AllyW merged 9 commits into
masterfrom
feature/aone-84342946-atomic-config-write

Conversation

@yndu13

@yndu13 yndu13 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Atomically persist CLI configuration through same-directory temporary files and replacement on Unix and Windows.
  • Sync temporary content, preserve existing config symlinks, and keep the original file when replacement fails.
  • Cover overwrite, cleanup, symlink compatibility, and replacement-failure behavior.

Avoid truncated or half-written ~/.aliyun/config.json when CloudSSO/OAuth
STS refresh races with configure set, or the process is interrupted mid-write.
Use same-directory unique temp files and os.Rename (Windows-safe replace).
@codecov-commenter

codecov-commenter commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.11%. Comparing base (ac4eeec) to head (1713e32).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1387      +/-   ##
==========================================
+ Coverage   84.91%   85.11%   +0.20%     
==========================================
  Files          69       69              
  Lines        8981     9018      +37     
==========================================
+ Hits         7626     7676      +50     
+ Misses        915      899      -16     
- Partials      440      443       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates configuration persistence to use an atomic temp-file + rename strategy when writing config.json, reducing the risk of corrupted/truncated configs during concurrent writes (e.g., refresh flows racing with configure set).

Changes:

  • Switch SaveConfiguration / SaveConfigurationWithContext to write via atomicWriteFile (same-directory temp file then os.Rename).
  • Normalize config.json path building with filepath.Join in the affected save paths.
  • Add unit tests covering overwrite behavior and ensuring temp files are not left behind after successful writes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
config/configuration.go Introduces atomicWriteFile and routes config saves through it for atomic replace semantics.
config/configuration_test.go Adds tests validating overwrite behavior and that temp files are cleaned up.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread config/configuration.go
Comment on lines +254 to +268
func atomicWriteFile(path string, data []byte, perm os.FileMode) error {
dir := filepath.Dir(path)
base := filepath.Base(path)
tempFile := filepath.Join(dir, "."+base+".tmp-"+strconv.Itoa(os.Getpid())+"-"+strconv.FormatInt(time.Now().UnixNano(), 10))

if err := os.WriteFile(tempFile, data, perm); err != nil {
return fmt.Errorf("failed to write temp config %q: %w", tempFile, err)
}

if err := os.Rename(tempFile, path); err != nil {
_ = os.Remove(tempFile)
return fmt.Errorf("failed to rename temp config to %q: %w", path, err)
}
return nil
}
Use collision-safe temporary files, sync content before replacement, and preserve existing config symlinks so interrupted writes do not put user configuration at risk.
Cover symlink/create/inspect/temp-file failure paths and
SaveConfigurationWithContext custom path creation.
@yndu13

yndu13 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up

  • Addressed Copilot’s temp-name collision note earlier via os.CreateTemp in the target directory (a7f7a2ab).
  • Added failure-path unit tests for atomic write / custom config path; new helper coverage is 100%, overall new-code block ~94.5%.
  • Local build + concurrent configure set E2E: config.json stayed valid JSON with no leftover temp files.

Parent-is-file paths return IsNotExist on Windows, so inject Lstat
to cover the inspect-error branch on all platforms.
@AllyW
AllyW merged commit 269f9a8 into master Jul 21, 2026
6 checks passed
@AllyW
AllyW deleted the feature/aone-84342946-atomic-config-write branch July 21, 2026 02:45
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.

5 participants