Skip to content

Conversation

joaomariolago
Copy link
Collaborator

@joaomariolago joaomariolago commented Sep 10, 2025

  • Tries to minimize cases where the save operation can be interrupted and generates invalid files
  • Make the settings save atomic

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:

  • Implement atomic save in Pykson and Pydantic settings by writing to a .tmp file and replacing the original via os.replace
  • Ensure data durability by flushing and fsyncing the temporary settings file before renaming
  • Clean up leftover .tmp files in the Pydantic manager before loading settings

Copy link

sourcery-ai bot commented Sep 10, 2025

Reviewer's Guide

This 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 settings

sequenceDiagram
participant "PydanticManager"
participant FileSystem
"PydanticManager"->>FileSystem: _clear_temp_files()
FileSystem-->>"PydanticManager": Remove all *.tmp files
"PydanticManager"->>"PydanticManager": load() proceeds
Loading

File-Level Changes

Change Details Files
Implement atomic save for settings files
  • Serialize settings to JSON prior to writing
  • Write JSON data to a .tmp file alongside the target
  • Call flush and os.fsync on the temporary file
  • Atomically replace the original file using Path.replace
core/libs/commonwealth/src/commonwealth/settings/bases/pykson_base.py
core/libs/commonwealth/src/commonwealth/settings/bases/pydantic_base.py
Cleanup orphaned temporary files before loading settings
  • Add _clear_temp_files method to remove leftover .tmp files
  • Invoke temporary-file cleanup at the start of the load() method
core/libs/commonwealth/src/commonwealth/settings/managers/pydantic_manager.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@joaomariolago joaomariolago force-pushed the make-settings-save-atomic branch 4 times, most recently from dc63557 to 979100e Compare September 10, 2025 17:55
@joaomariolago joaomariolago marked this pull request as ready for review September 10, 2025 19:02
Copy link

@sourcery-ai sourcery-ai bot left a 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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

* Tries to minimize cases where the save operation can be interrupted
  and generates invalid files
* Make the settings save atomic
@joaomariolago joaomariolago force-pushed the make-settings-save-atomic branch from 979100e to 061a83b Compare September 10, 2025 19:14
@patrickelectric patrickelectric added the move-to-stable Needs to be cherry-picked and move to stable label Sep 11, 2025
@patrickelectric patrickelectric merged commit ede39ad into bluerobotics:master Sep 11, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

move-to-stable Needs to be cherry-picked and move to stable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants