-
Notifications
You must be signed in to change notification settings - Fork 111
core:libs:commonwealth:settings: Make save atomic #3530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core:libs:commonwealth:settings: Make save atomic #3530
Conversation
Reviewer's GuideThis PR enhances the reliability of settings persistence by switching to atomic file writes—writing to a temporary file, flushing, fsyncing, and then atomically replacing the original—for both Pykson and Pydantic bases, and adds cleanup of leftover temporary files before loading settings in the Pydantic manager. Sequence diagram for clearing orphaned temporary files before loading settingssequenceDiagram
participant "PydanticManager"
participant FileSystem
"PydanticManager"->>FileSystem: _clear_temp_files()
FileSystem-->>"PydanticManager": Remove all *.tmp files
"PydanticManager"->>"PydanticManager": load() proceeds
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
dc63557
to
979100e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `core/libs/commonwealth/src/commonwealth/settings/managers/pydantic_manager.py:136` </location>
<code_context>
self._settings = PydanticManager.load_from_file(self.settings_type, self.settings_file_path())
+
+ def _clear_temp_files(self) -> None:
+ """Clear temporary files"""
+ for temp_file in self.config_folder.glob("*.tmp"):
+ temp_file.unlink()
</code_context>
<issue_to_address>
Consider logging or handling errors when deleting temp files.
If temp_file.unlink() fails, the error is ignored. Logging or handling exceptions will improve reliability and debuggability.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
core/libs/commonwealth/src/commonwealth/settings/managers/pydantic_manager.py
Outdated
Show resolved
Hide resolved
* Tries to minimize cases where the save operation can be interrupted and generates invalid files * Make the settings save atomic
979100e
to
061a83b
Compare
Summary by Sourcery
Make settings save operations atomic by writing to a temporary file, flushing and fsyncing data, and replacing the original file, and clear leftover temporary files before loading settings.
Enhancements: