Skip to content

fix(io): use atomic writes for .param, JSON, and settings files#1437

Merged
amilcarlucas merged 1 commit intoArduPilot:masterfrom
yashhzd:fix/atomic-file-writes
Mar 28, 2026
Merged

fix(io): use atomic writes for .param, JSON, and settings files#1437
amilcarlucas merged 1 commit intoArduPilot:masterfrom
yashhzd:fix/atomic-file-writes

Conversation

@yashhzd
Copy link
Copy Markdown
Contributor

@yashhzd yashhzd commented Mar 27, 2026

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:

  • .param files — uploading an incomplete parameter set to a flight controller could cause unpredictable behavior
  • vehicle_components.json — a corrupted components file breaks all derived parameter computations on next startup
  • settings.json — a truncated settings file causes JSONDecodeError, losing user preferences

Changes

Add a safe_write() utility (backend_safe_file_io.py) that writes to a temporary file in the same directory, then atomically replaces the target via os.replace(). The utility also:

  • Flushes and fsyncs the file before replacing
  • Preserves file permissions and ownership from the original target
  • Fsyncs the parent directory after replace for full rename durability
  • Cleans up temp files on failure

Applied 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() — vehicle templates

All save-related tests updated to patch safe_write directly and verify written data content via captured callbacks.

os.replace() is atomic on POSIX and near-atomic on Windows (MoveFileEx with MOVEFILE_REPLACE_EXISTING).

Continuation of #1430 (accidentally closed after force push).
Closes #1428

Signed-off-by: Yash Goel yashhzd@users.noreply.github.com

Copilot AI review requested due to automatic review settings March 27, 2026 20:23
@yashhzd yashhzd requested a review from amilcarlucas as a code owner March 27, 2026 20:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 introduces crash-safe, atomic file writes across several critical persistence paths in ArduPilot Methodic Configurator to prevent truncated/corrupted .param, JSON data, and settings.json files after interruptions.

Changes:

  • Added a shared safe_write() helper that writes to a same-directory temp file and atomically replaces the target.
  • Switched key save paths (.param, JSON-with-schema, settings, vehicle component templates) to use safe_write().
  • Updated unit/integration tests to patch safe_write() instead of open() and validate behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ardupilot_methodic_configurator/backend_safe_file_io.py New atomic write utility using temp file + os.replace(), fsync, and best-effort metadata preservation.
ardupilot_methodic_configurator/data_model_par_dict.py .param export now writes via safe_write() callback.
ardupilot_methodic_configurator/backend_filesystem_json_with_schema.py JSON persistence now uses safe_write() and preserves single trailing newline behavior.
ardupilot_methodic_configurator/backend_filesystem_program_settings.py Settings persistence now uses safe_write() instead of direct open()/json_dump().
ardupilot_methodic_configurator/backend_filesystem_vehicle_components.py Vehicle component template saving now uses safe_write().
tests/test_backend_filesystem_json_with_schema.py Tests updated to patch safe_write() in save paths.
tests/test_backend_filesystem_program_settings.py Tests updated to patch safe_write() for settings save.
tests/test_backend_filesystem_vehicle_components.py Tests updated to patch safe_write() and capture written JSON via callback.
tests/unit_backend_filesystem_vehicle_components.py Unit tests updated to patch safe_write() for error-path coverage.

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

Update all save-related tests to patch safe_write directly and verify
written data content via captured callbacks.

Closes ArduPilot#1428

Signed-off-by: Yash Goel <yashhzd@users.noreply.github.com>
@yashhzd yashhzd force-pushed the fix/atomic-file-writes branch from 7810212 to 4f96985 Compare March 27, 2026 21:03
@amilcarlucas
Copy link
Copy Markdown
Collaborator

The capture_safe_write function is defined 6 times. Can you find a way to define it only once and use it?
You could do it on the backend_safe_file_io.py or on the conftest.py file.

@amilcarlucas
Copy link
Copy Markdown
Collaborator

Thanks, I'll merge it now, it would be great if you could do the changes in a follow up PR

@amilcarlucas amilcarlucas merged commit 6d5ac86 into ArduPilot:master Mar 28, 2026
19 checks passed
yashhzd added a commit to yashhzd/MethodicConfigurator that referenced this pull request Mar 28, 2026
Move the capture_safe_write helper that was defined 6 times in
test_backend_filesystem_vehicle_components.py into a single
make_capture_safe_write() factory in conftest.py.

Follow-up to PR ArduPilot#1437 as requested by @amilcarlucas.

Signed-off-by: Yash Goel <yashhzd@users.noreply.github.com>
@yashhzd
Copy link
Copy Markdown
Contributor Author

yashhzd commented Mar 28, 2026

The capture_safe_write function is defined 6 times. Can you find a way to define it only once and use it? You could do it on the backend_safe_file_io.py or on the conftest.py file.

thanks for the review! please check out #1444

amilcarlucas pushed a commit to yashhzd/MethodicConfigurator that referenced this pull request Mar 29, 2026
…e_io.py

Move the capture_safe_write helper that was defined 6 times in
test_backend_filesystem_vehicle_components.py into a single
make_capture_safe_write() factory in backend_safe_file_io.py.
Address Copilot review comments:
- Use precise type annotations (dict[str, Any], Callable[[IO[str]], object])
  in make_capture_safe_write for pyright/mypy compatibility
- Restore captured_data in test_save_component_templates_empty_input to
  verify that the written JSON is actually an empty dict
use implicit booleaness for empty dict check (pylint C1803)

Follow-up to PR ArduPilot#1437 as requested by @amilcarlucas.

Signed-off-by: Yash Goel <yashhzd@users.noreply.github.com>
amilcarlucas pushed a commit that referenced this pull request Mar 29, 2026
…e_io.py

Move the capture_safe_write helper that was defined 6 times in
test_backend_filesystem_vehicle_components.py into a single
make_capture_safe_write() factory in backend_safe_file_io.py.
Address Copilot review comments:
- Use precise type annotations (dict[str, Any], Callable[[IO[str]], object])
  in make_capture_safe_write for pyright/mypy compatibility
- Restore captured_data in test_save_component_templates_empty_input to
  verify that the written JSON is actually an empty dict
use implicit booleaness for empty dict check (pylint C1803)

Follow-up to PR #1437 as requested by @amilcarlucas.

Signed-off-by: Yash Goel <yashhzd@users.noreply.github.com>
OmkarSarkar204 pushed a commit to OmkarSarkar204/MethodicConfigurator that referenced this pull request Mar 29, 2026
…e_io.py

Move the capture_safe_write helper that was defined 6 times in
test_backend_filesystem_vehicle_components.py into a single
make_capture_safe_write() factory in backend_safe_file_io.py.
Address Copilot review comments:
- Use precise type annotations (dict[str, Any], Callable[[IO[str]], object])
  in make_capture_safe_write for pyright/mypy compatibility
- Restore captured_data in test_save_component_templates_empty_input to
  verify that the written JSON is actually an empty dict
use implicit booleaness for empty dict check (pylint C1803)

Follow-up to PR ArduPilot#1437 as requested by @amilcarlucas.

Signed-off-by: Yash Goel <yashhzd@users.noreply.github.com>
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.

Bug: Non-atomic file writes risk data corruption on crash or power loss

3 participants