Skip to content
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

fix(record-store): ensure record sync before marking as stored #2591

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dirvine
Copy link
Member

@dirvine dirvine commented Jan 2, 2025

Record Store Sync Improvements

Problem

The can_store_after_restart test was failing because records were not being properly synced to disk before being marked as stored. This could lead to potential data loss in case of system crashes or unexpected restarts.

Solution

  • Added proper file sync operation after writing record to disk using sync_all()
  • Combined file write and sync operations into a single Result chain using and_then
  • Improved error handling for both write and sync operations
  • Only send AddLocalRecordAsStored command after both operations succeed

Testing

  • All tests now pass, including the previously failing can_store_after_restart test
  • Added proper error handling and logging for write/sync failures
  • Verified that records are properly persisted after system restarts

Impact

This change improves data reliability by ensuring that records are properly synced to disk before being marked as stored. This prevents potential data loss in scenarios where the system crashes or restarts unexpectedly after a write but before the data is fully synced to disk.

Added proper file sync after writing record to disk. Fixed race condition in can_store_after_restart test. Improved error handling for write/sync operations. This change ensures that records are properly synced to disk before being marked as stored, preventing potential data loss in case of system crashes or unexpected restarts.
Copy link
Member

@grumbach grumbach left a comment

Choose a reason for hiding this comment

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

LGTM

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.

2 participants