fix(io): use atomic writes for .param, JSON, and settings files#1430
fix(io): use atomic writes for .param, JSON, and settings files#1430yashhzd wants to merge 1 commit intoArduPilot:masterfrom
Conversation
f5c8ef3 to
bec823a
Compare
There was a problem hiding this comment.
Pull request overview
Introduce crash-safe, atomic file writes for critical persisted artifacts to prevent truncated/empty files after interruptions.
Changes:
- Added a
safe_write()helper that writes to a temp file and replaces the target viaos.replace(). - Switched
.param,settings.json,vehicle_components.json, and schema-backed JSON saves to usesafe_write(). - Centralized newline/encoding behavior for these persistence paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ardupilot_methodic_configurator/common_safe_file_io.py | Adds safe_write() utility for atomic file replacement. |
| ardupilot_methodic_configurator/data_model_par_dict.py | Writes .param exports via safe_write() to avoid truncation. |
| ardupilot_methodic_configurator/backend_filesystem_vehicle_components.py | Uses safe_write() for vehicle components JSON persistence. |
| ardupilot_methodic_configurator/backend_filesystem_program_settings.py | Uses safe_write() for settings.json updates. |
| ardupilot_methodic_configurator/backend_filesystem_json_with_schema.py | Uses safe_write() for schema-backed JSON writes while keeping EOF-newline behavior. |
| fd, tmp_path = tempfile.mkstemp(dir=dir_name, suffix=".tmp") | ||
| try: | ||
| with os.fdopen(fd, "w", encoding="utf-8", newline="\n") as tmp_file: | ||
| write_func(tmp_file) |
There was a problem hiding this comment.
On POSIX, mkstemp() creates the temp file with restrictive permissions (typically 0600). After os.replace(), the destination will inherit the temp file’s mode/ownership rather than preserving the original file’s permissions. This can break scenarios where the existing target was meant to be group/world-readable (or had custom mode bits). Consider copying mode/ownership from the existing target (when it exists) onto tmp_path before os.replace(), e.g., stat = os.stat(filepath) then os.chmod(tmp_path, stat.st_mode) (and os.chown where applicable).
| write_func(tmp_file) | |
| write_func(tmp_file) | |
| # Preserve permissions and ownership from existing target, if any, | |
| # before atomically replacing it with the temporary file. | |
| try: | |
| stat_info = os.stat(filepath) | |
| except FileNotFoundError: | |
| stat_info = None | |
| if stat_info is not None: | |
| try: | |
| os.chmod(tmp_path, stat_info.st_mode) | |
| if hasattr(os, "chown"): | |
| os.chown(tmp_path, stat_info.st_uid, stat_info.st_gid) | |
| except PermissionError: | |
| # Best-effort: if we cannot adjust permissions/ownership, | |
| # continue with the atomic replace to avoid breaking writes. | |
| pass |
| write_func(tmp_file) | ||
| os.replace(tmp_path, filepath) # atomic on POSIX, near-atomic on Windows |
There was a problem hiding this comment.
This is atomic with respect to file replacement, but it is not fully crash/power-loss safe because the temp file’s content may still be in buffers when os.replace() happens. To make the PR’s stated guarantees stronger, flush and os.fsync(tmp_file.fileno()) before replacing. On POSIX, also consider an fsync of the parent directory after os.replace() so the rename itself is durable.
| write_func(tmp_file) | |
| os.replace(tmp_path, filepath) # atomic on POSIX, near-atomic on Windows | |
| write_func(tmp_file) | |
| # Ensure all data is flushed from Python and OS buffers to disk | |
| tmp_file.flush() | |
| os.fsync(tmp_file.fileno()) | |
| os.replace(tmp_path, filepath) # atomic on POSIX, near-atomic on Windows | |
| # Best-effort: on POSIX, fsync the containing directory so the rename is durable | |
| dir_fd = None | |
| try: | |
| flags = getattr(os, "O_RDONLY", 0) | |
| if hasattr(os, "O_DIRECTORY"): | |
| flags |= os.O_DIRECTORY | |
| dir_fd = os.open(dir_name, flags) | |
| os.fsync(dir_fd) | |
| except (OSError, AttributeError): | |
| # Not all platforms/filesystems support opening or fsyncing directories | |
| pass | |
| finally: | |
| if dir_fd is not None: | |
| try: | |
| os.close(dir_fd) | |
| except OSError: | |
| pass |
| def _write(output_file): # type: ignore[no-untyped-def] | ||
| if content_header: | ||
| output_file.write("\n".join(content_header) + "\n") | ||
| output_file.writelines(line + "\n" for line in formatted_params) |
There was a problem hiding this comment.
The new nested writer uses # type: ignore[no-untyped-def], which weakens type checking in this critical path. Since safe_write() always passes a text file handle, annotate the parameter explicitly (e.g., TextIO / IO[str]) and remove the ignore. This keeps typing consistent and avoids hiding real type issues.
amilcarlucas
left a comment
There was a problem hiding this comment.
Looks OK. I'll take a final look once the AI issues are solved.
There was a problem hiding this comment.
Rename to backend_safe_file_io.py
All file-writing operations used a simple open("w") → write() pattern.
If the application crashes or power is lost mid-write, the target file
is left truncated or empty. This is especially dangerous for .param
files (incomplete parameter sets uploaded to a flight controller could
cause unpredictable behavior) and vehicle_components.json (corrupted
components file breaks all derived parameter computations on startup).
Add a safe_write() utility that writes to a temporary file in the same
directory, then atomically replaces the target via os.replace(). The
utility also flushes and fsyncs before replacing, and preserves file
permissions from the original target.
Apply safe_write to the four most critical persistence paths:
- ParDict.export_to_param() — .param files
- FilesystemJSONWithSchema.save_json_data() — JSON data files
- ProgramSettings._set_settings_from_dict() — settings.json
- VehicleComponents.save_component_templates_to_file() — templates
Closes ArduPilot#1428
Signed-off-by: Yash Goel <yashhzd@users.noreply.github.com>
bec823a to
e2f8f9f
Compare
Summary
All file-writing operations used a simple
open("w") → write()pattern. If the application crashes, the OS kills the process, or power is lost mid-write, the target file is left truncated or empty.This is especially dangerous for:
.paramfiles — uploading an incomplete parameter set to a flight controller could cause unpredictable behaviorvehicle_components.json— a corrupted components file breaks all derived parameter computations on next startupsettings.json— a truncated settings file causesJSONDecodeError, losing user preferencesChanges
Add a
safe_write()utility (common_safe_file_io.py) that writes to a temporary file in the same directory, then atomically replaces the target viaos.replace(). This ensures the file is either fully written or untouched — never truncated.os.replace()is atomic on POSIX and near-atomic on Windows (MoveFileExwithMOVEFILE_REPLACE_EXISTING).Applied to the four most critical persistence paths:
ParDict.export_to_param()—.paramfilesFilesystemJSONWithSchema.save_json_data()— JSON data filesProgramSettings._set_settings_from_dict()—settings.jsonVehicleComponents.save_component_templates_to_file()— vehicle templatesTest plan
.paramfile → verify contents are correctvehicle_components.jsonis validsettings.jsonis valid.tmpfiles are left behind after successful writeskill -9) → verify target file is intactCloses #1428
Signed-off-by: Yash Goel yashhzd@users.noreply.github.com