Evict on touch failure

[chrisstaite-menlo]

Description

There's an unknown failure occuring under load where the filesystem store ends up with an entry in the temp path that doesn't exist. I can't find that fault, but rather than persisting, it makes sense to avoid the failure persisting by removing it from the eviciting map in that case.

Therefore, this change introduces a change to the LenEntry such that touch() returns a bool. If it returns false then it is removed from the EvictingMap immediately.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Running the code up on my cluster.

Checklist

• Updated documentation if needed
• Tests added/amended
• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nativelink-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 26, 2024 5:59pm

[adam-singer]

@chrisstaite-menlo there are some similarities in problem and solution at #601 , would you want to pull in that pr and see if it solves issue? I think there are bits that could be merged in terms of implementation here. What isn't obvious is how the unit tests aren't failing with this patch compared to #601

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04

[allada]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04

a discussion (no related file):
Is it possible the root cause is what is being addressed in:
#496

?

What you would be seeing is the file being added to the eviction map, the future then is dropped before fs::rename happens. This results in:

            encoded_file_path.path_type = PathType::Content;
            encoded_file_path.digest = final_digest;

never being triggered, so the entry in the evicting map is now pointing to the wrong file, and the file ends up being in temp, but not reachable.

---

[chrisstaite-menlo]

It is similar to #601, but in this case, if the file was removed underneath the running process, then this will catch that case. It's much more of an edge case since #496 has been completed, but it's still possible that a file could be deleted on disk and then it is tried to be accessed.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Vercel, pre-commit-checks

a discussion (no related file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Is it possible the root cause is what is being addressed in:
#496

?

What you would be seeing is the file being added to the eviction map, the future then is dropped before fs::rename happens. This results in:

            encoded_file_path.path_type = PathType::Content;
            encoded_file_path.digest = final_digest;

never being triggered, so the entry in the evicting map is now pointing to the wrong file, and the file ends up being in temp, but not reachable.

That is the root cause, but I think it would be prudent for the EvicitingMap to handle the case where the conditions under it change (such as an external factor deleting a file). This would allow recovery without restarting the process.

---

[chrisstaite-menlo]

nativelink-util/src/evicting_map.rs line 308 at r3 (raw file):

                        // Important to track the eviction size, otherwise if we
                        // reach the maximum we end up eviciting everything!
                        sum_store_size -= entry.data.len() as u64;

@allada @adam-singer In implementing this I spotted this fatal bug!

[chrisstaite-menlo]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04)

---

nativelink-util/src/evicting_map.rs line 338 at r4 (raw file):

            async move {
                for digest in remove_digests {
                    // Do not use inner_remove as it calls evict_items, which

And this one too...

[adam-singer]

Reviewable status: 0 of 1 LGTMs obtained

---

nativelink-util/src/evicting_map.rs line 308 at r3 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

@allada @adam-singer In implementing this I spotted this fatal bug!

wow great catch! Guessing our tests don't have something around eviction for max_bytes / max_count settings. Could you include test coverage for that in fixing this bug? Should be able to get a few tests in that validate this as a bug and provide the tests as a protection against regression, evicting_map_test.rs and adjusting the EvictionPolicy should be enough to provide/show/protect against it again

&EvictionPolicy {
                max_count: 0,
                max_seconds: 0,
                max_bytes: 0,
                evict_bytes: 0,
            },

---

nativelink-util/src/evicting_map.rs line 338 at r4 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

And this one too...

another great catch! Was hoping for the freebie on that call :/

[chrisstaite-menlo]

Reviewable status: 0 of 1 LGTMs obtained

---

nativelink-util/src/evicting_map.rs line 308 at r3 (raw file):

Previously, adam-singer (Adam Singer) wrote…

wow great catch! Guessing our tests don't have something around eviction for max_bytes / max_count settings. Could you include test coverage for that in fixing this bug? Should be able to get a few tests in that validate this as a bug and provide the tests as a protection against regression, evicting_map_test.rs and adjusting the EvictionPolicy should be enough to provide/show/protect against it again

&EvictionPolicy {
                max_count: 0,
                max_seconds: 0,
                max_bytes: 0,
                evict_bytes: 0,
            },

I'm not actually sure if we can get to this state here now I think about it, because we would have evicted on insertion if we were over the size or length limits, right? So perhaps this isn't as bad as I thought. However, the lower call to evict_items for every removal by calling remove_internal will have definitely been significantly slower.

[allada]

Reviewed 3 of 4 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained

---

nativelink-util/src/evicting_map.rs line 308 at r3 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

I'm not actually sure if we can get to this state here now I think about it, because we would have evicted on insertion if we were over the size or length limits, right? So perhaps this isn't as bad as I thought. However, the lower call to evict_items for every removal by calling remove_internal will have definitely been significantly slower.

Yeah, evicting on insert was an original design decision. I think the code was safe the way it was, but doing what you have is technically safer.

---

nativelink-util/src/evicting_map.rs line 298 at r4 (raw file):

        let mut lru_len = state.lru.len();
        let mut sum_store_size = state.sum_store_size;
        let digest_entries: Vec<Option<T>> = digests

nit: Maybe call this to_touch_or_remove or something?

[adam-singer]

Reviewable status: 2 of 1 LGTMs obtained

[chrisstaite-menlo]

Reviewable status: 2 of 1 LGTMs obtained

a discussion (no related file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

That is the root cause, but I think it would be prudent for the EvicitingMap to handle the case where the conditions under it change (such as an external factor deleting a file). This would allow recovery without restarting the process.

@allada just waiting on a resolution to this comment to merge.

---

[allada]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 2 of 1 LGTMs obtained