Add Completeness Checking Store

[blakehatch]

Description

The proto promises that all results will exist in the CAS when requested using the associated actions. However we currently allow stores to prune themselves which violates this condition, because the action cache and CAS are different. Therefore we need this completeness checking store to check that all results exist in the CAS before we allow the the action result to be returned to the client.

Fixes #358 #362

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Made a test where all files are present in the cas causing completeness to succeed and another where the cas does not have all of the files causing completeness checking to fail.

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 

[aaronmondal]

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @allada and @blakehatch)

---

cas/scheduler/action_messages.rs line 756 at r1 (raw file):

            .output_file_symlinks
            .into_iter()
            .map(|output_symlink| SymlinkInfo::try_from(output_symlink).unwrap())

I think it's a good idea to essentially "never" use unwrap, except for cases where you really want turbo-cache to completely crash or when you're absolutely certain that a panic is virtually impossible. An example would be when a mutex lock/unlock doesn't work and puts the entire program in an undefined/hard to recover state.

For everything else expect or most likely better our own err_tip/make_err infrastructure seems more appropriate since they return handleable/recoverable Results.

In this case, since an external source could have modified the files these errors are not completely unlikely, but they seem recoverable, so returning a Result approach seems more appropriate. Making this entire impl a TryFrom could be an option.

---

cas/store/BUILD line 585 at r1 (raw file):

        "@crate_index//:tokio",
    ],
)

Newline.

---

cas/store/completeness_checking_store.rs line 24 at r1 (raw file):

use futures::FutureExt;
use futures::{
    future::BoxFuture,

Nit: I personally prefer the nested import syntax, but at the moment everything else uses the non-nested variant. Let's make this non-nested and have a discussion on uniform import formatting so that there is and automatically enforced "one true way".

---

cas/store/completeness_checking_store.rs line 43 at r1 (raw file):

// Sadly we cannot use `async fn` here because the rust compiler cannot determine the auto traits
// of the future. So we need to force this function to return a dynamic future instead.
// see: https://github.com/rust-lang/rust/issues/78649

There might be a way to make this an async fn by utilizing async-trait.

nit: inconsistent comment slashes.

---

cas/store/completeness_checking_store.rs line 83 at r1 (raw file):

        }

        while futures.try_next().await?.is_some() {}

You could join_all here.

---

cas/store/completeness_checking_store.rs line 158 at r1 (raw file):

        }

        while (futures.next().await).is_some() {}

nit: (and here)

---

config/stores.rs line 338 at r1 (raw file):

pub struct CompletenessCheckingStore {
    /// The underlying store wrap around. All content will first flow
    /// through self before forwarding to backend. In the event there

Talking about "self" in a comment geared towards end users might be a bit too technical.

[blakehatch]

Reviewable status: 6 of 11 files reviewed, 5 unresolved discussions (waiting on @aaronmondal and @allada)

---

cas/scheduler/action_messages.rs line 756 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I think it's a good idea to essentially "never" use unwrap, except for cases where you really want turbo-cache to completely crash or when you're absolutely certain that a panic is virtually impossible. An example would be when a mutex lock/unlock doesn't work and puts the entire program in an undefined/hard to recover state.

For everything else expect or most likely better our own err_tip/make_err infrastructure seems more appropriate since they return handleable/recoverable Results.

In this case, since an external source could have modified the files these errors are not completely unlikely, but they seem recoverable, so returning a Result approach seems more appropriate. Making this entire impl a TryFrom could be an option.

Yes we should almost never be using unwrap, leaving this was an error. Decided to change to a TryFrom as it returns a Result which is the more appropriate approach as you pointed out.

---

cas/store/BUILD line 585 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Newline.

Done.

---

cas/store/completeness_checking_store.rs line 24 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Nit: I personally prefer the nested import syntax, but at the moment everything else uses the non-nested variant. Let's make this non-nested and have a discussion on uniform import formatting so that there is and automatically enforced "one true way".

Done.

---

cas/store/completeness_checking_store.rs line 43 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

There might be a way to make this an async fn by utilizing async-trait.

nit: inconsistent comment slashes.

As this is a problem that @allada ran into when using this same pattern in running_actions_manager don't want to go down a rabbithole a second time if it has already been dug into and resulted in the same place. Seems like the issue referenced did end up having solutions that could use async by manually inlining the function, happy to dig into this more if it seems worth it.

---

cas/store/completeness_checking_store.rs line 83 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

You could join_all here.

Done.

---

cas/store/completeness_checking_store.rs line 158 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: (and here)

Done.

---

config/stores.rs line 338 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Talking about "self" in a comment geared towards end users might be a bit too technical.

Done.

[aaronmondal]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @allada)

---

cas/store/completeness_checking_store.rs line 43 at r1 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

As this is a problem that @allada ran into when using this same pattern in running_actions_manager don't want to go down a rabbithole a second time if it has already been dug into and resulted in the same place. Seems like the issue referenced did end up having solutions that could use async by manually inlining the function, happy to dig into this more if it seems worth it.

Nah doesn't seem worth it 😅

[allada]

Also needs tests. I suggest adding tests now, as it'll help you see some of the problems I've pointed out.

Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @blakehatch)

---

cas/store/completeness_checking_store.rs line 43 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Nah doesn't seem worth it 😅

FYI, the big reason this is a problem is because this async function calls itself and that causes the compiler to be unable to find the size (or something like that).

---

cas/store/completeness_checking_store.rs line 44 at r2 (raw file):

    cas_store: Pin<&'a dyn StoreTrait>,
    digest: &'a DigestInfo,
    current_directory: &'a str,

nit: Unused.

---

cas/store/completeness_checking_store.rs line 59 at r2 (raw file):

                .err_tip(|| "In Directory::file::digest")?;
            // Maybe could be made more efficient
            futures.push(cas_store.has(digest).boxed());

nit: use .left_future() instead of .boxed().

---

cas/store/completeness_checking_store.rs line 59 at r2 (raw file):

                .err_tip(|| "In Directory::file::digest")?;
            // Maybe could be made more efficient
            futures.push(cas_store.has(digest).boxed());

nit: Use .and_then(|exists| exists.ok_or_else(|| make_err!(Code::NotFound, "")))

---

cas/store/completeness_checking_store.rs line 59 at r2 (raw file):

                .err_tip(|| "In Directory::file::digest")?;
            // Maybe could be made more efficient
            futures.push(cas_store.has(digest).boxed());

nit: Lets have this use .has_many or one of the related functions. As it is significantly more efficient.

---

cas/store/completeness_checking_store.rs line 74 at r2 (raw file):

                        .await
                        .err_tip(|| format!("in traverse_ : {new_directory_path}"))?;
                    Ok(Some(1))

nit: we don't use this value. Just use Ok(())

---

cas/store/completeness_checking_store.rs line 76 at r2 (raw file):

                    Ok(Some(1))
                }
                .boxed(),

nit: use .right_future() instead of .boxed().

---

cas/store/completeness_checking_store.rs line 80 at r2 (raw file):

        }

        join_all(futures).await;

try_join_all, And also check to ensure all the files actually exist either above by translating None to errors or here by iterating the results and validating them.

---

cas/store/completeness_checking_store.rs line 110 at r2 (raw file):

        // requested using the associated actions. However we currently allow
        // stores to prune themselves which violates this condition, because the
        // action cache and CAS are different. Therefore we need this completeness checking

nit: Technically not because they are different, even if they were shared the same problem would exist.

---

cas/store/completeness_checking_store.rs line 123 at r2 (raw file):

        for digest in digests {
            let action_result: ActionResult = get_and_decode_digest::<ProtoActionResult>(pin_cas, digest)

note: This is incorrect. It should be reading this digest from the Action Cache store, since CompletenessCheckingStore is an ActionCache`-only store.

---

cas/store/completeness_checking_store.rs line 130 at r2 (raw file):

            for output_file in &action_result.output_files {
                let file = output_file.digest;
                let file_digest = DigestInfo::try_new(&file.hash_str(), file.size_bytes as usize)?;

nit: Use let file_digest: DigestInfo = output_file.digest.try_into().err_tip(...) or a similar function.

---

cas/store/completeness_checking_store.rs line 133 at r2 (raw file):

                futures.push(
                    async move {
                        if let Err(e) = check_directory_files_in_cas(pin_cas, &file_digest, "").await {

Can we instead use .err_tip() here?

---

cas/store/completeness_checking_store.rs line 137 at r2 (raw file):

                        }
                    }
                    .boxed(),

nit: Also: .left_future()

---

cas/store/completeness_checking_store.rs line 146 at r2 (raw file):

                futures.push(
                    async move {
                        if let Err(e) = check_directory_files_in_cas(pin_cas, &tree_digest, &path).await {

ditto.

---

cas/store/completeness_checking_store.rs line 150 at r2 (raw file):

                        }
                    }
                    .boxed(),

nit: Also: .right_future()

---

cas/store/completeness_checking_store.rs line 155 at r2 (raw file):

        }

        join_all(futures).await;

nit: try_join_all and return None in the case it is not present.

---

cas/store/completeness_checking_store.rs line 166 at r2 (raw file):

        size_info: UploadSizeInfo,
    ) -> Result<(), Error> {
        self.pin_cas().update(digest, reader, size_info).await

nit: Should be the action cache store.

---

cas/store/completeness_checking_store.rs line 176 at r2 (raw file):

        length: Option<usize>,
    ) -> Result<(), Error> {
        self.pin_cas().get_part_ref(digest, writer, offset, length).await

nit: We need to also check all files if they exist here.

---

cas/store/completeness_checking_store.rs line 176 at r2 (raw file):

        length: Option<usize>,
    ) -> Result<(), Error> {
        self.pin_cas().get_part_ref(digest, writer, offset, length).await

nit: Should be the action cache store.

[blakehatch]

It has tests in the completeness_checking_store_test.rs file

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @allada)

---

cas/store/completeness_checking_store.rs line 43 at r1 (raw file):

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

FYI, the big reason this is a problem is because this async function calls itself and that causes the compiler to be unable to find the size (or something like that).

Yeah async recursion in rust is a bit of a pain

---

cas/store/completeness_checking_store.rs line 44 at r2 (raw file):

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

nit: Unused.

Done.

---

cas/store/completeness_checking_store.rs line 59 at r2 (raw file):

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

nit: use .left_future() instead of .boxed().

Done.

---

cas/store/completeness_checking_store.rs line 59 at r2 (raw file):

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

nit: Lets have this use .has_many or one of the related functions. As it is significantly more efficient.

Done.

---

cas/store/completeness_checking_store.rs line 59 at r2 (raw file):

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

nit: Use .and_then(|exists| exists.ok_or_else(|| make_err!(Code::NotFound, "")))

Done.

---

cas/store/completeness_checking_store.rs line 74 at r2 (raw file):

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

nit: we don't use this value. Just use Ok(())

Done.

---

cas/store/completeness_checking_store.rs line 76 at r2 (raw file):

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

nit: use .right_future() instead of .boxed().

Done.

---

cas/store/completeness_checking_store.rs line 80 at r2 (raw file):

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

try_join_all, And also check to ensure all the files actually exist either above by translating None to errors or here by iterating the results and validating them.

Done.

---

cas/store/completeness_checking_store.rs line 110 at r2 (raw file):

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

nit: Technically not because they are different, even if they were shared the same problem would exist.

Done.

---

cas/store/completeness_checking_store.rs line 123 at r2 (raw file):

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

note: This is incorrect. It should be reading this digest from the Action Cache store, since CompletenessCheckingStore is an ActionCache`-only store.

Done.

---

cas/store/completeness_checking_store.rs line 130 at r2 (raw file):

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

nit: Use let file_digest: DigestInfo = output_file.digest.try_into().err_tip(...) or a similar function.

Done.

---

cas/store/completeness_checking_store.rs line 133 at r2 (raw file):

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

Can we instead use .err_tip() here?

Done.

---

cas/store/completeness_checking_store.rs line 137 at r2 (raw file):

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

nit: Also: .left_future()

Done.

---

cas/store/completeness_checking_store.rs line 146 at r2 (raw file):

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

ditto.

Done.

---

cas/store/completeness_checking_store.rs line 150 at r2 (raw file):

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

nit: Also: .right_future()

Done.

---

cas/store/completeness_checking_store.rs line 155 at r2 (raw file):

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

nit: try_join_all and return None in the case it is not present.

Done.

---

cas/store/completeness_checking_store.rs line 166 at r2 (raw file):

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

nit: Should be the action cache store.

Done.

---

cas/store/completeness_checking_store.rs line 176 at r2 (raw file):

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

nit: We need to also check all files if they exist here.

Done.

---

cas/store/completeness_checking_store.rs line 176 at r2 (raw file):

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

nit: Should be the action cache store.

Done.

[allada]

Reviewed 7 of 10 files at r3, all commit messages.
Reviewable status: 9 of 12 files reviewed, 42 unresolved discussions (waiting on @aaronmondal and @blakehatch)

---

cas/scheduler/action_messages.rs line 761 at r3 (raw file):

    fn try_from(val: ProtoActionResult) -> Result<Self, Error> {
        let output_file_symlinks: Result<Vec<_>, _> = val

Return the error here, why force the rest of the function to execute then return the error?

---

cas/scheduler/action_messages.rs line 761 at r3 (raw file):

    fn try_from(val: ProtoActionResult) -> Result<Self, Error> {
        let output_file_symlinks: Result<Vec<_>, _> = val

nit: No need to declare the type here, it'll figure it out at the end.

---

cas/scheduler/action_messages.rs line 768 at r3 (raw file):

                    .err_tip(|| "Output File Symlinks could not be converted to SymlinkInfo")
            })
            .collect();

ditto.

---

cas/scheduler/action_messages.rs line 777 at r3 (raw file):

                    .err_tip(|| "Output File Symlinks could not be converted to SymlinkInfo")
            })
            .collect();

ditto.

---

cas/scheduler/action_messages.rs line 783 at r3 (raw file):

            .into_iter()
            .map(|output_file| output_file.try_into().err_tip(|| "Output File could not be converted"))
            .collect();

ditto.

---

cas/scheduler/action_messages.rs line 793 at r3 (raw file):

                    .err_tip(|| "Output File could not be converted")
            })
            .collect();

ditto.

---

cas/scheduler/action_messages.rs line 803 at r3 (raw file):

            stdout_digest: val
                .stdout_digest
                .map(|digest| digest.try_into().unwrap_or(DigestInfo::default()))

nit: Lets grab the proper error and return it.

---

cas/scheduler/action_messages.rs line 807 at r3 (raw file):

            stderr_digest: val
                .stderr_digest
                .map(|digest| digest.try_into().unwrap_or(DigestInfo::default()))

ditto.

---

cas/scheduler/action_messages.rs line 811 at r3 (raw file):

            execution_metadata: val
                .execution_metadata
                .map(|metadata| metadata.try_into().unwrap_or(ExecutionMetadata::default()))

ditto.

---

cas/store/BUILD line 179 at r3 (raw file):

        "//util:error",
        "//util:metrics_utils",
        "@crate_index//:hashbrown",

nit: We are not modifying existance_store, so why add externals?

---

cas/store/BUILD line 196 at r3 (raw file):

        ":ac_utils",
        "//proto",
        "//config",

nit: unused

---

cas/store/BUILD line 200 at r3 (raw file):

        "//util:common",
        "//util:error",
        "//util:metrics_utils",   

nit: blank spaces at end of line

---

cas/store/BUILD line 200 at r3 (raw file):

        "//util:common",
        "//util:error",
        "//util:metrics_utils",   

nit: unused.

---

cas/store/BUILD line 203 at r3 (raw file):

        "//cas/scheduler:action_messages",
        "@crate_index//:futures",
        "@crate_index//:hashbrown",

nit: unused.

---

cas/store/BUILD line 204 at r3 (raw file):

        "@crate_index//:futures",
        "@crate_index//:hashbrown",
        "@crate_index//:hex",

nit: unused.

---

cas/store/BUILD line 205 at r3 (raw file):

        "@crate_index//:hashbrown",
        "@crate_index//:hex",
        "@crate_index//:sha2",

nit: unused.

---

cas/store/BUILD line 206 at r3 (raw file):

        "@crate_index//:hex",
        "@crate_index//:sha2",
        "@crate_index//:tokio",

nit: unused.

---

cas/store/BUILD line 548 at r3 (raw file):

        ":traits",
        ":completeness_checking_store",
        ":store",

nit: unused.

---

cas/store/BUILD line 551 at r3 (raw file):

        "//proto",
        "//config",
        "//util:buf_channel",

nit: unused.

---

cas/store/BUILD line 554 at r3 (raw file):

        "//util:common",
        "//util:error",
        "//cas/scheduler:action_messages",

nit: unused.

---

cas/store/BUILD line 556 at r3 (raw file):

        "//cas/scheduler:action_messages",
        "@crate_index//:prost",
        "@crate_index//:futures",

nit: unused.

---

cas/store/completeness_checking_store.rs line 1 at r3 (raw file):

// Copyright 2023 The Turbo Cache Authors. All rights reserved.

nit: Native Link

---

cas/store/completeness_checking_store.rs line 44 at r3 (raw file):

        let directory = get_and_decode_digest::<ProtoDirectory>(ac_store, digest)
            .await
            .err_tip(|| "Converting digest to Directory")?;

nit: Add in completeness_checking_store::check_directory_files_in_cas

---

cas/store/completeness_checking_store.rs line 45 at r3 (raw file):

            .await
            .err_tip(|| "Converting digest to Directory")?;
        let mut futures = vec![];

nit: You know how many items are going to be used in this vector, so use Vec::with_capacity().

---

cas/store/completeness_checking_store.rs line 48 at r3 (raw file):

        let mut file_digests = vec![];
        for file in directory.files {

nit: Use into_iter() and .map() and .try_collect().

---

cas/store/completeness_checking_store.rs line 53 at r3 (raw file):

                .err_tip(|| "Expected Digest to exist in Directory::file::digest")?
                .try_into()
                .err_tip(|| "In Directory::file::digest")?;

nit: Add in completeness_checking_store::check_directory_files_in_cas

---

cas/store/completeness_checking_store.rs line 62 at r3 (raw file):

                    cas_store.has_many(&file_digests).await.and_then(|exists_vec| {
                        if exists_vec.iter().all(|result| {
                            println!("res: {:?}", result);

nit: Remove needless println

---

cas/store/completeness_checking_store.rs line 80 at r3 (raw file):

                .err_tip(|| "Expected Digest to exist in Directory::directories::digest")?
                .try_into()
                .err_tip(|| "In Directory::file::digest")?;

nit: Add in completeness_checking_store::check_directory_files_in_cas

---

cas/store/completeness_checking_store.rs line 84 at r3 (raw file):

                async move {
                    check_directory_files_in_cas(cas_store, ac_store, &digest)
                        .await

nit: Just return the value here instead of returning Ok(()) below.

---

cas/store/completeness_checking_store.rs line 85 at r3 (raw file):

                    check_directory_files_in_cas(cas_store, ac_store, &digest)
                        .await
                        .err_tip(|| "Could not recursively traverse")?;

nit: No need to err_tip here, since it already has all data embedded.

---

cas/store/completeness_checking_store.rs line 92 at r3 (raw file):

        }

        let result = try_join_all(futures).await;

This will require a needless Vec allocation, so instead lets just drain out future directly. Now we don't need to store a needlessVec<None>. (note: you may need to pin your future).

while let Some(()) = futures.try_next()? {}
Ok(())

---

cas/store/completeness_checking_store.rs line 140 at r3 (raw file):

        let pin_cas = self.pin_cas();
        let pin_ac = self.pin_ac();
        let mut futures = vec![];

nit: use Vec::with_capacity() when you know how many items you are going to store in it.

---

cas/store/completeness_checking_store.rs line 142 at r3 (raw file):

        let mut futures = vec![];

        for digest in digests {

nit: The logic in this looks nearly identical to check_directory_files_in_cas... can we rewrite the function signature to pass in the folder and file protos and let it deal with the details? This way we don't need duplicate code for this logic.

---

cas/store/completeness_checking_store.rs line 199 at r3 (raw file):

    ) -> Result<(), Error> {
        let pin_ac = self.pin_ac();
        match pin_ac.has_with_results(&[digest], &mut []).await {

The &mut [] is where your results are being placed, so this does not do what you think it does.

Also, this code would also likely cause panics because we assume the two slices are the same size (ie: input and output slices must be the same length).

In your case you just want to use .has(digest).

---

cas/store/completeness_checking_store.rs line 199 at r3 (raw file):

    ) -> Result<(), Error> {
        let pin_ac = self.pin_ac();
        match pin_ac.has_with_results(&[digest], &mut []).await {

nit: Just use questionmark, it makes the code much cleaner.

---

cas/store/tests/completeness_checking_store_test.rs line 15 at r3 (raw file):

// limitations under the License.

// use action_messages::{ActionResult, ExecutionMetadata};

nit: Remove.

---

cas/store/tests/completeness_checking_store_test.rs line 16 at r3 (raw file):

// use action_messages::{ActionResult, ExecutionMetadata};
// use proto::build::bazel::remote::execution::v2::ActionResult as ProtoActionResult;

nit: Remove.

---

cas/store/tests/completeness_checking_store_test.rs line 19 at r3 (raw file):

use prost::Message;
use proto::build::bazel::remote::execution::v2::{Directory, DirectoryNode, FileNode};
use std::pin::Pin;

nit: Reorder these into the {std}\n\n{externals}\n\n{internals} ordering.

---

cas/store/tests/completeness_checking_store_test.rs line 21 at r3 (raw file):

use std::pin::Pin;
use std::sync::Arc;
// use std::time::{Duration, UNIX_EPOCH};

nit: Remove.

---

cas/store/tests/completeness_checking_store_test.rs line 27 at r3 (raw file):

    use super::*;

    use common::DigestInfo;

nit: Move these to the top.

---

cas/store/tests/completeness_checking_store_test.rs line 31 at r3 (raw file):

    use error::Error;
    use memory_store::MemoryStore;
    // use traits::StoreTrait;

nit: Remove.

---

cas/store/tests/completeness_checking_store_test.rs line 32 at r3 (raw file):

    use memory_store::MemoryStore;
    // use traits::StoreTrait;
    // use prost::Message;

nit: Remove.

[allada]

Reviewed 2 of 7 files at r4, all commit messages.
Reviewable status: 5 of 12 files reviewed, 24 unresolved discussions (waiting on @aaronmondal and @blakehatch)

---

cas/store/tests/completeness_checking_store_test.rs line 187 at r4 (raw file):

        action_result.encode(&mut bytes)?;

        let mut hasher = Sha256::new();

nit: We don't actually use sha256 anywhere in this store. Given that, We should use dummy values and not do anything with hashing.

[allada]

Reviewable status: 5 of 12 files reviewed, 15 unresolved discussions (waiting on @aaronmondal and @blakehatch)

---

cas/store/BUILD line 548 at r4 (raw file):

        "//util:common",
        "//util:error",
        "@crate_index//:sha2",

nit: Will be unused.

---

cas/store/completeness_checking_store.rs line 38 at r4 (raw file):

/// of the future. So we need to force this function to return a dynamic future instead.
/// see: https://github.com/rust-lang/rust/issues/78649
pub fn check_files_and_directories_in_cas<'a>(

nit: Should not be pub.

---

cas/store/completeness_checking_store.rs line 68 at r4 (raw file):

        }

        for directory_digest in directory_digests {

nit: If you remember, we don't need to be recursive here because tree_digest has all the directories in it.

---

cas/store/completeness_checking_store.rs line 108 at r4 (raw file):

        }

        while let Some(result) = futures.next().await {

nit: Use .try_next().await?. This will make the code cleaner.

---

cas/store/completeness_checking_store.rs line 207 at r4 (raw file):

        let pin_ac = self.pin_ac();
        pin_ac.has(digest).await.err_tip(|| "Items not found in CAS")?;
        pin_ac.get_part_ref(digest, writer, offset, length).await

If you look at what is happening here, we are calling .has() which calls get_and_decode_digest() which calls .get().

This results in an additional needless request. I suggest either fixing it, or adding a ticket w/ a todo.

[blakehatch]

Reviewable status: 0 of 19 files reviewed, 1 unresolved discussion (waiting on @aaronmondal and @allada)

---

cas/scheduler/action_messages.rs line 761 at r3 (raw file):

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

Return the error here, why force the rest of the function to execute then return the error?

Done.

---

cas/scheduler/action_messages.rs line 761 at r3 (raw file):

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

nit: No need to declare the type here, it'll figure it out at the end.

Compiler was not able to infer the type

---

cas/scheduler/action_messages.rs line 768 at r3 (raw file):

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

ditto.

Compiler was not able to infer the type

---

cas/scheduler/action_messages.rs line 777 at r3 (raw file):

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

ditto.

Compiler was not able to infer the type

---

cas/scheduler/action_messages.rs line 783 at r3 (raw file):

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

ditto.

Done.

---

cas/scheduler/action_messages.rs line 793 at r3 (raw file):

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

ditto.

Done.

---

cas/scheduler/action_messages.rs line 803 at r3 (raw file):

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

nit: Lets grab the proper error and return it.

Done.

---

cas/scheduler/action_messages.rs line 807 at r3 (raw file):

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

ditto.

Done.

---

cas/scheduler/action_messages.rs line 811 at r3 (raw file):

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

ditto.

Done.

---

cas/store/BUILD line 179 at r3 (raw file):

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

nit: We are not modifying existance_store, so why add externals?

Not sure what this is referring to, existence_store is not modified in this PR this line is not changed in the commit.

---

cas/store/BUILD line 196 at r3 (raw file):

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

nit: unused

Done.

---

cas/store/BUILD line 200 at r3 (raw file):

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

nit: blank spaces at end of line

Done.

---

cas/store/BUILD line 200 at r3 (raw file):

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

nit: unused.

Done.

---

cas/store/BUILD line 203 at r3 (raw file):

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

nit: unused.

Done.

---

cas/store/BUILD line 204 at r3 (raw file):

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

nit: unused.

Done.

---

cas/store/BUILD line 205 at r3 (raw file):

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

nit: unused.

Done.

---

cas/store/BUILD line 206 at r3 (raw file):

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

nit: unused.

Now used.

---

cas/store/BUILD line 548 at r3 (raw file):

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

nit: unused.

Done.

---

cas/store/BUILD line 551 at r3 (raw file):

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

nit: unused.

Done.

---

cas/store/BUILD line 554 at r3 (raw file):

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

nit: unused.

Done.

---

cas/store/BUILD line 556 at r3 (raw file):

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

nit: unused.

Done.

---

cas/store/BUILD line 548 at r4 (raw file):

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

nit: Will be unused.

Done.

---

cas/store/completeness_checking_store.rs line 1 at r3 (raw file):

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

nit: Native Link

Done.

---

cas/store/completeness_checking_store.rs line 44 at r3 (raw file):

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

nit: Add in completeness_checking_store::check_directory_files_in_cas

Done.

---

cas/store/completeness_checking_store.rs line 45 at r3 (raw file):

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

nit: You know how many items are going to be used in this vector, so use Vec::with_capacity().

Was able to switch to FuturesUnordered which should be more optimal.

---

cas/store/completeness_checking_store.rs line 48 at r3 (raw file):

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

nit: Use into_iter() and .map() and .try_collect().

Clippy would not let try_collect be used on an iter as it's an unstable feature but restructured otherwise.

---

cas/store/completeness_checking_store.rs line 53 at r3 (raw file):

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

nit: Add in completeness_checking_store::check_directory_files_in_cas

Done.

---

cas/store/completeness_checking_store.rs line 62 at r3 (raw file):

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

nit: Remove needless println

Done.

---

cas/store/completeness_checking_store.rs line 80 at r3 (raw file):

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

nit: Add in completeness_checking_store::check_directory_files_in_cas

Done.

---

cas/store/completeness_checking_store.rs line 84 at r3 (raw file):

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

nit: Just return the value here instead of returning Ok(()) below.

Done.

---

cas/store/completeness_checking_store.rs line 85 at r3 (raw file):

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

nit: No need to err_tip here, since it already has all data embedded.

Done.

---

cas/store/completeness_checking_store.rs line 92 at r3 (raw file):

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

This will require a needless Vec allocation, so instead lets just drain out future directly. Now we don't need to store a needlessVec<None>. (note: you may need to pin your future).

while let Some(()) = futures.try_next()? {}
Ok(())

Done, did end up having to pin futures.

---

cas/store/completeness_checking_store.rs line 140 at r3 (raw file):

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

nit: use Vec::with_capacity() when you know how many items you are going to store in it.

Was able to switch to FuturesUnordered which should be more optimal.

---

cas/store/completeness_checking_store.rs line 142 at r3 (raw file):

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

nit: The logic in this looks nearly identical to check_directory_files_in_cas... can we rewrite the function signature to pass in the folder and file protos and let it deal with the details? This way we don't need duplicate code for this logic.

Done.

---

cas/store/completeness_checking_store.rs line 199 at r3 (raw file):

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

The &mut [] is where your results are being placed, so this does not do what you think it does.

Also, this code would also likely cause panics because we assume the two slices are the same size (ie: input and output slices must be the same length).

In your case you just want to use .has(digest).

Done.

---

cas/store/completeness_checking_store.rs line 199 at r3 (raw file):

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

nit: Just use questionmark, it makes the code much cleaner.

Done.

---

cas/store/completeness_checking_store.rs line 38 at r4 (raw file):

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

nit: Should not be pub.

Done.

---

cas/store/completeness_checking_store.rs line 68 at r4 (raw file):

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

nit: If you remember, we don't need to be recursive here because tree_digest has all the directories in it.

Done.

---

cas/store/completeness_checking_store.rs line 108 at r4 (raw file):

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

nit: Use .try_next().await?. This will make the code cleaner.

Done.

---

cas/store/completeness_checking_store.rs line 207 at r4 (raw file):

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

If you look at what is happening here, we are calling .has() which calls get_and_decode_digest() which calls .get().

This results in an additional needless request. I suggest either fixing it, or adding a ticket w/ a todo.

Done.

---

cas/store/tests/completeness_checking_store_test.rs line 15 at r3 (raw file):

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

nit: Remove.

Done.

---

cas/store/tests/completeness_checking_store_test.rs line 16 at r3 (raw file):

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

nit: Remove.

Done.

---

cas/store/tests/completeness_checking_store_test.rs line 19 at r3 (raw file):

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

nit: Reorder these into the {std}\n\n{externals}\n\n{internals} ordering.

Done.

---

cas/store/tests/completeness_checking_store_test.rs line 21 at r3 (raw file):

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

nit: Remove.

Done.

---

cas/store/tests/completeness_checking_store_test.rs line 27 at r3 (raw file):

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

nit: Move these to the top.

Done.

---

cas/store/tests/completeness_checking_store_test.rs line 31 at r3 (raw file):

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

nit: Remove.

Done.

---

cas/store/tests/completeness_checking_store_test.rs line 32 at r3 (raw file):

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

nit: Remove.

Done.

---

cas/store/tests/completeness_checking_store_test.rs line 187 at r4 (raw file):

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

nit: We don't actually use sha256 anywhere in this store. Given that, We should use dummy values and not do anything with hashing.

Done.

[allada]

Reviewed 14 of 18 files at r5, all commit messages.
Reviewable status: 14 of 19 files reviewed, 22 unresolved discussions (waiting on @aaronmondal and @blakehatch)

---

gencargo/completeness_checking_store/Cargo.toml line 1 at r5 (raw file):

# This file is automatically generated from `tools/build_cargo_manifest.py`.

nit: These files are no longer needed after rebase.

---

gencargo/completeness_checking_store_test/Cargo.toml line 1 at r5 (raw file):

# This file is automatically generated from `tools/build_cargo_manifest.py`.

nit: These files are no longer needed after rebase.

---

native-link-config/src/stores.rs line 47 at r5 (raw file):

    verify(Box<VerifyStore>),

    /// Because pruning stale actions and the actions need to exist for a

Can we make this more grammatically correct? starting with a proposition is not really explaining what this store is.

---

native-link-config/src/stores.rs line 53 at r5 (raw file):

    /// CAS or else return not found.
    ///
    /// Blaise comment:

nit: Remove this line.

---

native-link-config/src/stores.rs line 58 at r5 (raw file):

    /// and will verify that all outputs of a previously ran result still exist
    /// in the CAS.
    completeness_checking_store(Box<CompletenessCheckingStore>),

nit: remove _store (the other's don't use _store).

---

native-link-config/src/stores.rs line 343 at r5 (raw file):

#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct CompletenessCheckingStore {
    /// The underlying store to wrap around. All content will initially pass

This comment looks like CodePilot generated it. Can we clean it up? It is way too verbose.

---

native-link-config/src/stores.rs line 347 at r5 (raw file):

    /// detected in this store, the connection to the backend will be terminated.
    /// Any early termination should always cause updates to fail on the backend.
    pub ac_store: StoreConfig,

nit: This should be named backend

---

native-link-config/src/stores.rs line 348 at r5 (raw file):

    /// Any early termination should always cause updates to fail on the backend.
    pub ac_store: StoreConfig,
    pub cas_store: StoreConfig,

nit: Add comment. All fields should have comments in config.

---

native-link-config/src/stores.rs line 348 at r5 (raw file):

    /// Any early termination should always cause updates to fail on the backend.
    pub ac_store: StoreConfig,
    pub cas_store: StoreConfig,

nit: There's never a case when this is not a RefStore, so lets force it to be a RefStore here.

---

native-link-store/BUILD.bazel line 17 at r5 (raw file):

        "src/default_store_factory.rs",
        "src/existence_store.rs",
        "src/completeness_checking_store.rs",

nit: Put in alphabetical order.

---

native-link-store/BUILD.bazel line 76 at r5 (raw file):

        "tests/dedup_store_test.rs",
        "tests/existence_store_test.rs",
        "tests/completeness_checking_store_test.rs",

nit: ditto.

---

native-link-store/BUILD.bazel line 108 at r5 (raw file):

        "@crate_index//:pretty_assertions",
        "@crate_index//:rand",
        "@crate_index//:prost",

nit: ditto.

---

native-link-store/src/completeness_checking_store.rs line 49 at r5 (raw file):

        let mut futures = FuturesUnordered::new();

        let mut file_vec: Vec<_> = file_digests.to_vec();

nit: If you know you always need to make a copy of your data, always force the caller to make the copy. This allows the caller to use move-semantics to transfer ownership without a copy or make a copy if it cannot move it giving possible optimizations, where doing it this way never allows any optimizations and forces a copy every time.

---

native-link-store/src/completeness_checking_store.rs line 50 at r5 (raw file):

        let mut file_vec: Vec<_> = file_digests.to_vec();
        let mut directory_queue: VecDeque<_> = tree_digests.iter().collect();

Why perform another heap allocation when we just iterating over our data? Lets just iterate over tree_digests directly, no need to make a needless copy of everything.

---

native-link-store/src/completeness_checking_store.rs line 53 at r5 (raw file):

        while let Some(directory_digest) = directory_queue.pop_front() {
            let tree = get_and_decode_digest::<Tree>(ac_store, directory_digest)

Tree does not live in ac_store it lives in cas_store.

---

native-link-store/src/completeness_checking_store.rs line 59 at r5 (raw file):

            let root_files: Result<Vec<DigestInfo>, _> = tree
                .root
                .ok_or_else(|| make_err!(Code::Aborted, "Could not get root from tree"))

nit: use .err_tip(). don't use Code::Aborted use Code::InvalidArgument (which is what .err_tip does by default).

---

native-link-store/src/completeness_checking_store.rs line 65 at r5 (raw file):

                .map(|file| {
                    file.digest
                        .ok_or_else(|| make_err!(Code::NotFound, "Expected Digest to exist in Directory::file::digest"))

nit: try using .err_tip_with_code()

---

native-link-store/src/completeness_checking_store.rs line 66 at r5 (raw file):

                    file.digest
                        .ok_or_else(|| make_err!(Code::NotFound, "Expected Digest to exist in Directory::file::digest"))
                        .map_err(|_| make_err!(Code::NotFound, "Expected Digest to exist in Directory::file::digest"))

nit: Why duplicate this here?

---

native-link-store/src/completeness_checking_store.rs line 70 at r5 (raw file):

                            digest
                                .try_into()
                                .map_err(|_| make_err!(Code::NotFound, "Failed to convert digest"))

nit: This should be .err_tip() as this should never happen and should always return Code::InvalidArgument.

---

native-link-store/src/completeness_checking_store.rs line 73 at r5 (raw file):

                        })
                })
                .collect();

nit: Use question mark here.

---

native-link-store/src/completeness_checking_store.rs line 75 at r5 (raw file):

                .collect();

            file_vec.extend(root_files?);

nit: use .chain() as it won't force a copy, it'll use some fancy template stuff instead.

---

native-link-store/src/completeness_checking_store.rs line 79 at r5 (raw file):

            for child in tree.children {
                futures.push({
                    futures::future::ready({

Why are we doing this in futures? Just do this logic here, no need to create a needless ready future.

[MarcusSorealheis]

I think we should fix the recurring misspelling for certain.

I'm not willing to block this PR for the assert, though I do think my suggestion might be better stylistically.

[blakehatch]

Fixed spellings and assert, hopefully this one's all set ❤️

Reviewable status: 20 of 22 files reviewed, 11 unresolved discussions (waiting on @aaronmondal, @adam-singer, @allada, and @MarcusSorealheis)

---

native-link-store/src/completeness_checking_store.rs line 212 at r17 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

existence*

Done.

---

native-link-store/src/completeness_checking_store.rs line 218 at r17 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

existence* (annoying, I know)

Done.

---

native-link-store/src/completeness_checking_store.rs line 242 at r17 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

Nice work overall, though. My goodness. Very complicated.

This is better and clippy wants it on three lines actually 😂

---

native-link-store/src/completeness_checking_store.rs line 268 at r17 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

existence*

Done.

---

native-link-store/src/completeness_checking_store.rs line 272 at r17 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

existence*

Done.

---

native-link-store/src/completeness_checking_store.rs line 275 at r17 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

existence*

Done.

---

native-link-store/src/completeness_checking_store.rs line 297 at r17 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

existence*

Done.

---

native-link-store/src/completeness_checking_store.rs line 299 at r17 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

existence*

Done.

[allada]

Reviewed 1 of 2 files at r17, 1 of 1 files at r18.
Reviewable status: all files reviewed (commit messages unreviewed), 10 unresolved discussions (waiting on @aaronmondal, @adam-singer, and @MarcusSorealheis)

---

-- commits line 4 at r18:
fyi: In the future, try to have git messages no more than 70 characters, so users that live in their terminal don't need to scroll to the right to read the message.

[allada]

Reviewable status: all files reviewed (commit messages unreviewed), 10 unresolved discussions (waiting on @aaronmondal, @adam-singer, and @MarcusSorealheis)

[blakehatch]

Reviewable status: all files reviewed (commit messages unreviewed), 10 unresolved discussions (waiting on @aaronmondal, @adam-singer, and @MarcusSorealheis)

---

-- commits line 4 at r18:

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

fyi: In the future, try to have git messages no more than 70 characters, so users that live in their terminal don't need to scroll to the right to read the message.

Good note ty.

[aaronmondal]

Reviewed all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @adam-singer, @allada, @blakehatch, and @MarcusSorealheis)

---

-- commits line 4 at r18:

Previously, blakehatch (Blake Hatch) wrote…

Good note ty.

Pls fix, I live in my terminal and this destroys the aesthetic of the git log 🥹

---

native-link-store/src/completeness_checking_store.rs line 119 at r15 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

Done.

fyi: instead of Checks xxx, prefer Check xxx. Try to avoid "filler words" as much as possible to reduce word count which in turn reduces cognitive complexity.

For instance: separate sets of futures that are polled concurrently -> separate sets of concurrently polled futures.

On that note, wdyt about enabling some sort of styleguide like https://github.com/errata-ai/vale?

[blakehatch]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @aaronmondal, @adam-singer, @allada, and @MarcusSorealheis)

---

-- commits line 4 at r18:

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Pls fix, I live in my terminal and this destroys the aesthetic of the git log 🥹

Haha fair I gotchu

---

native-link-store/src/completeness_checking_store.rs line 119 at r15 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

fyi: instead of Checks xxx, prefer Check xxx. Try to avoid "filler words" as much as possible to reduce word count which in turn reduces cognitive complexity.

For instance: separate sets of futures that are polled concurrently -> separate sets of concurrently polled futures.

On that note, wdyt about enabling some sort of styleguide like https://github.com/errata-ai/vale?

Yes going to set up vale before my next large rust PR to avoid this tedium for everyone!

[aaronmondal]

Fantastic work! ❤️

Reviewed 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @adam-singer, @allada, and @MarcusSorealheis)

[allada]

Reviewed 1 of 1 files at r20, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @adam-singer, @blakehatch, and @MarcusSorealheis)

---

native-link-store/Cargo.toml line 38 at r20 (raw file):

tonic = { version = "0.9.2", features = ["gzip"] }
tracing = "0.1.40"
uuid = { version = "1.6.1", features = ["v4"] }

nit: Don't change this, this is not needed for this PR.

[allada]

Reviewed 1 of 1 files at r19.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @adam-singer, @blakehatch, and @MarcusSorealheis)

[adam-singer]

Reviewed all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @blakehatch and @MarcusSorealheis)

Already made requested formatting and spelling changes

[blakehatch]

❤️

Dismissed @MarcusSorealheis from a discussion.
Reviewable status: 21 of 22 files reviewed, 7 unresolved discussions (waiting on @allada and @MarcusSorealheis)

---

native-link-store/src/completeness_checking_store.rs line 268 at r17 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

still wrong

I changed all instances of "existance" to "existence" it was corrected upon another pass.

---

native-link-store/Cargo.toml line 38 at r20 (raw file):

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

nit: Don't change this, this is not needed for this PR.

Done. Accidentally updated when fixing merge confict.

[MarcusSorealheis]

LGTM. Very nice work from you on a difficult feature.

[blakehatch]

Ty :)

Reviewable status: 21 of 22 files reviewed, 7 unresolved discussions (waiting on @allada and @MarcusSorealheis)

[blakehatch]

Dismissed @MarcusSorealheis from 7 discussions.
Reviewable status: 21 of 22 files reviewed, all discussions resolved (waiting on @allada)

[blakehatch]

Reviewed 8 of 18 files at r5, 3 of 8 files at r6, 1 of 9 files at r7, 2 of 5 files at r10, 4 of 7 files at r15, 1 of 3 files at r16, 1 of 2 files at r17, 1 of 1 files at r19, 1 of 1 files at r21, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @blakehatch)