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

Weird bug in file metadata modified timestamp returned by OS #71

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

twitu
Copy link
Collaborator

@twitu twitu commented Jun 9, 2024

The file metadata modified timestamp returns different values for reading the same file without any changes 🤯. Which is causing the bug in #63.

This is a separate PR to discuss and debug it. Here's output from the modified test that's failing for me locally. It has two different timestamps for the same file even though there is no modification between the two readings.

---- file_storage::tests::test_file_storage_is_storage_updated stdout ----
thread 'file_storage::tests::test_file_storage_is_storage_updated' panicked at fs-storage/src/file_storage.rs:397:9:
assertion `left == right` failed
  left: SystemTime { tv_sec: 1717951936, tv_nsec: 537150997 }
 right: SystemTime { tv_sec: 1717951936, tv_nsec: 529151011 }

Can others reproduce this locally as well?

@twitu
Copy link
Collaborator Author

twitu commented Jun 10, 2024

I've reproduced the issue with an even smaller test. The os is returning different metadata for the same file.

    #[test]
    fn test_file_timestamp_bug() {
        let temp_dir =
            TempDir::new("tmp").expect("Failed to create temporary directory");
        let storage_path = temp_dir.path().join("teststorage.txt");
        let mut file_storage =
            FileStorage::new("TestStorage".to_string(), &storage_path).unwrap();
        file_storage.set("key1".to_string(), "value1".to_string());
        let updated = file_storage.write_fs().unwrap();
        let file_updated = fs::metadata(&file_storage.path)
            .unwrap()
            .modified()
            .unwrap();
        assert_eq!(file_updated, updated);
    }
thread 'file_storage::tests::test_file_timestamp_bug' panicked at fs-storage/src/file_storage.rs:376:9:
assertion `left == right` failed
  left: SystemTime { tv_sec: 1718003019, tv_nsec: 328495061 }
 right: SystemTime { tv_sec: 1718003019, tv_nsec: 324495101 }

@tareknaser
Copy link
Collaborator

The issue seems to be related to BufWriter buffering the writes and not immediately flushing the data to disk. This means the file's timestamp might not be updated right after writer.write_all(value_data.as_bytes())? is called, as the data is still in the buffer.

For reference, see the BufWriter documentation.

To resolve this, I've added a call to writer.flush() to ensure the data is written to the file immediately. This should correctly update the file's metadata and timestamp.

I've pushed a commit with this fix.

@twitu
Copy link
Collaborator Author

twitu commented Jun 11, 2024

That was a very tricky bug. Thanks for the fix. Reading the documentation I realize that BufWriter is not a good fit for our purpose since we want to write the whole mapping to disk at once.

It does not help when writing very large amounts at once, or writing just one or a few times.

I'll remove the BufWriter and use just a writer which will avoid such tricky bugs later.

@twitu twitu merged commit 3501336 into millis-timestamp Jun 11, 2024
@twitu twitu deleted the millis-timestamp-bug branch June 11, 2024 13:25
twitu added a commit that referenced this pull request Jun 14, 2024
* Use millisecond resolution when comparing timestamps

* Use nanos

* Round down scan time

* Use micros

* Update syncing logic with some api changes

* Abstract loading from fs logic

* Add full syncing logic

* Fix need syncing logic

* Address comments

* Fix lints

* add a 1sec sleep

Signed-off-by: pushkarm029 <[email protected]>

* check

Signed-off-by: pushkarm029 <[email protected]>

* fmt

Signed-off-by: pushkarm029 <[email protected]>

* Update fs-storage/src/file_storage.rs

Co-authored-by: Kirill Taran <[email protected]>

* Update fs-storage/src/file_storage.rs

Co-authored-by: Kirill Taran <[email protected]>

* Update fs-storage/src/file_storage.rs

Co-authored-by: Kirill Taran <[email protected]>

* Add doc comments on FileStorage fields

* Weird bug in file metadata modified timestamp returned by OS (#71)

* File modified metadata bug

* Add even smaller test

* fix(fs-storage): flush the buffer in write_fs()

Signed-off-by: Tarek <[email protected]>

---------

Signed-off-by: Tarek <[email protected]>
Co-authored-by: Tarek <[email protected]>

* Use file writer and update test

* cargo fix

* Weird bug in file metadata again

* Update fs-storage/src/file_storage.rs

Co-authored-by: Tarek Elsayed <[email protected]>

* Update fs-storage/src/base_storage.rs

Co-authored-by: Tarek Elsayed <[email protected]>

* Refactor sync_status to cleaner match case

Co-authored-by: Tarek <[email protected]>

* Add MRE for file metadata timestamp not updated on write_fs

* fmt fix

* fix(fs-storage): add delay in tests after file write

Signed-off-by: Tarek <[email protected]>

* Set file modified timestamp from application layer

* Fmt fix

* Update fs-storage/src/file_storage.rs

Co-authored-by: Tarek Elsayed <[email protected]>

* Resolve comments

* Fix fmt

---------

Signed-off-by: pushkarm029 <[email protected]>
Signed-off-by: Tarek <[email protected]>
Co-authored-by: pushkarm029 <[email protected]>
Co-authored-by: Kirill Taran <[email protected]>
Co-authored-by: Tarek <[email protected]>
Co-authored-by: Tarek Elsayed <[email protected]>
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