Unify validate_output logic in CompletenessCheckingStore and CacheLookupScheduler

[blizzardc0der]

Description

We have duplicated work in CacheLookupScheduler and CompletenessCheckingStore.
To resolve this, I removed the validation logic in CacheLookupScheduler and instead make it have CompletenessCheckingStore always.

Fixes #677

Type of change

Please delete options that aren't relevant.

• Bug fix (non-breaking change which fixes an issue)
• This change requires a documentation update

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 

[blizzardc0der]

I hope you to review this. cc: @allada.

[allada]

Reviewed 4 of 4 files at r1.
Reviewable status: 1 of 1 LGTMs obtained, and 5 discussions need to be resolved

---

-- commits line 2 at r1:
nit: Change this to:

[Breaking] Remove completeness checking logic in CacheLookupScheduler

Since the CompletenessCheckingStore had landed, users should now use this
store if they need CompletenessChecking for their action cache store.

Breaking change only affects CacheLookupScheduler.

---

nativelink-config/src/schedulers.rs line 148 at r1 (raw file):

#[serde(deny_unknown_fields)]
pub struct CacheLookupScheduler {
    /// The reference to the CompletenessCheckingStore used to return cached

nit: Lets instead accept any Store, but add:

To prevent unintended issues, this store should probably be a CompletenessCheckingStore.

because they might use a GRPC store that does this under the hood.

---

nativelink-scheduler/src/cache_lookup_scheduler.rs line 107 at r1 (raw file):

    ) -> Result<Self, Error> {
        let any_store = ac_store.clone().inner_store_arc(None).as_any_arc();
        let _ = any_store.downcast::<CompletenessCheckingStore>().map_err(|_| {

nit: We can remove this, we'll put the blame on the user if they don't honor documentation.

---

nativelink-scheduler/src/cache_lookup_scheduler.rs line 175 at r1 (raw file):

                    .await
            {
                if let Ok(_) = Pin::new(ac_store.clone().as_ref())

This is not correct, it should be Ok(Some(_))...

But regardless lets print warning if an error is returned.

---

nativelink-scheduler/tests/cache_lookup_scheduler_test.rs line 52 at r1 (raw file):

fn make_cache_scheduler() -> Result<TestContext, Error> {
    let mock_scheduler = Arc::new(MockActionScheduler::new());
    let ac_store = Arc::new(CompletenessCheckingStore::new(

for the test, lets just go back to memory store.

[allada]

Reviewed all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and 5 discussions need to be resolved

[blizzardc0der]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 2 discussions need to be resolved

---

nativelink-scheduler/src/cache_lookup_scheduler.rs line 175 at r1 (raw file):

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

This is not correct, it should be Ok(Some(_))...

But regardless lets print warning if an error is returned.

Done.

---

nativelink-scheduler/tests/cache_lookup_scheduler_test.rs line 52 at r1 (raw file):

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

for the test, lets just go back to memory store.

Done.

[allada]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Cargo Dev / ubuntu-22.04, pre-commit-checks

[adam-singer]

Reviewed 1 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Cargo Dev / ubuntu-22.04, pre-commit-checks

[allada]

Could you rebase this on latest main, CI should be resolved now.

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Cargo Dev / ubuntu-22.04, pre-commit-checks

[blizzardc0der]

Done. cc: @allada.

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04

[adam-singer]

Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04