I wonder if a better alternative would be to try and grow the file to the exact required size on flush. If we don't have enough space, we'll abort without touching the file.

We currently grow the file automatically by writing to it, which means it's more likely to abort outside of entry boundaries.

One edge case with my suggestion is that we might have a bunch of zeroes at the end of the file. If is possible if we crash half way during the flush while we've already grown the file. But, we can easily detect this as there's no valid 0x00 entry (did this on purpose while building this tracker yay!).

Speaking of which: a crash during flush is likely another scenario to reproduce the problem you described - writing a partial entry.

Thoughts?

I wonder if a better alternative would be to try and grow the file to the exact required size on flush. If we don't have enough space, we'll abort without touching the file.

We currently grow the file automatically by writing to it, which means it's more likely to abort outside of entry boundaries.

One edge case with my suggestion is that we might have a bunch of zeroes at the end of the file. If is possible if we crash half way during the flush while we've already grown the file. But, we can easily detect this as there's no valid 0x00 entry (did this on purpose while building this tracker yay!).

Speaking of which: a crash during flush is likely another scenario to reproduce the problem you described - writing a partial entry.

Thoughts?

As we discussed with @agourlay, in general, we cannot check if the data in the file is correct. We can check the entry, but if the malformed data is the last bytes after the last entry, we cannot check it - the external id may be any possible value. In case of out-of-space it's fine, but I'm not sure if this proposal works fine in case of crash

I wonder if a better alternative would be to try and grow the file to the exact required size on flush. If we don't have enough space, we'll abort without touching the file.

We currently grow the file automatically by writing to it, which means it's more likely to abort outside of entry boundaries.

One edge case with my suggestion is that we might have a bunch of zeroes at the end of the file. If is possible if we crash half way during the flush while we've already grown the file. But, we can easily detect this as there's no valid 0x00 entry (did this on purpose while building this tracker yay!).

Speaking of which: a crash during flush is likely another scenario to reproduce the problem you described - writing a partial entry.

Thoughts?

There is a draft PR with collecting all pending mappings into a single buffer and writing it in one shot. It contains the size estimation of the pending changes and is a partial implementation of your proposal, we can extend it to implement your idea
#7872

I tested this PR locally using tmpfs (filesystem in RAM) with small disk size (8MB). Test code with repro steps. It shows the same error Corrupted ID tracker mapping storage, got malformed mapping change byte 0x00. This PR fixes this problem

    #[test]
    fn mutable_id_tracker_overflow() {
        // First, create a file of 1MB size to simulate later disk space cleaning.
        let tmp_file_path = Path::new("/mnt/tiny/tmp_file");
        if fs::exists(&tmp_file_path).unwrap() {
            fs::remove_file(&tmp_file_path).unwrap();
        }
        let mut tmp_file = fs::File::create(&tmp_file_path).unwrap();
        tmp_file.write(&vec![0; 1024 * 1024]).unwrap();
        tmp_file.flush().unwrap();
        tmp_file.sync_all().unwrap();
        drop(tmp_file);

        // Then, create ID tracker and fill it while possible.
        let path = Path::new("/mnt/tiny/id_tracker_overflow_test");
        if fs::exists(path).unwrap() {
            fs::remove_dir_all(path).unwrap();
        }
        std::fs::create_dir_all(path).unwrap();
        let mut id_tracker = make_mutable_tracker(&path);
        for i in 0.. {
            let internal_id = id_tracker.total_point_count() as PointOffsetType;
            let point_id = PointIdType::NumId(internal_id as u64);
            match id_tracker.set_link(point_id, internal_id) {
                Ok(_) => {}
                Err(err) => {
                    println!("Stopped at iteration {i} due to error: {err}");
                    break;
                }
            }

            // flush each 1000 points
            if i % 1000 == 0 {
                match id_tracker.mapping_flusher()() {
                    Ok(_) => {}
                    Err(err) => {
                        println!("Flushing stopped at iteration {i} due to error: {err}");
                        break;
                    }
                }
            }
        }

        // check that free space is not available
        let fs_stats = fs2::available_space(path).unwrap();
        println!("Available space before cleanup: {fs_stats} bytes");

        // then, add additional space by removing the temporary file.
        fs::remove_file(&tmp_file_path).unwrap();

        // flush file deletion to ensure the OS registers the freed space
        #[cfg(unix)]
        {
            use std::process::Command;
            // Force filesystem sync
            let _ = Command::new("sync").output();
        }
        std::thread::sleep(std::time::Duration::from_secs(3)); // wait for the system to register the freed space

        // check that free space is available
        let fs_stats = fs2::available_space(path).unwrap();
        println!("Available space after cleanup: {fs_stats} bytes");

        // Add some more points to verify that ID tracker can continue working.
        for i in 0..10 {
            let point_id = PointIdType::Uuid(Uuid::new_v4());
            let internal_id = id_tracker.total_point_count() as PointOffsetType;
            match id_tracker.set_link(point_id, internal_id) {
                Ok(_) => {}
                Err(err) => {
                    println!("Stopped at iteration {i} due to error: {err}");
                    break;
                }
            }
        }

        // Flush has to work at the end.
        id_tracker.mapping_flusher()().unwrap();

        // Reload ID tracker to verify data integrity.
        drop(id_tracker);

        // The bug appears here with some probability.
        MutableIdTracker::open(&path).unwrap();
    }

Corrupted ID tracker mapping storage, got malformed mapping change byte 0x00

Actually, this is critical.

This shows that we do get 0's in places where we don't expect it. It means the file was grown without actually writing bytes to it.

Turns out that message was from before the fix. No pending concerns then. 👍