Errors are now properly handled in subscription

[allada]

We no longer drop errors in subscription handling.

Also cleanup some small todo!'s.

---

This change is 

[allada]

+@adam-singer
+@zbirenbaum

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: vale (waiting on @adam-singer and @zbirenbaum)

[adam-singer]

Reviewed 19 of 20 files at r1.
Reviewable status: 0 of 2 LGTMs obtained, and 1 discussions need to be resolved (waiting on @zbirenbaum)

---

nativelink-util/src/action_messages.rs line 44 at r1 (raw file):

/// Exit code sent if there is an internal error.
pub const INTERNAL_ERROR_EXIT_CODE: i32 = -178;

Could we resuse existing encodings form https://doc.rust-lang.org/std/process/struct.ExitCode.html#

[allada]

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: vale (waiting on @adam-singer and @zbirenbaum)

---

nativelink-util/src/action_messages.rs line 44 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

Could we resuse existing encodings form https://doc.rust-lang.org/std/process/struct.ExitCode.html#

Maybe, but this code is only to ensure we have a mostly-uniqueish code if the proto forgot to set it.

[zbirenbaum]

Reviewable status: 0 of 2 LGTMs obtained, and 3 discussions need to be resolved (waiting on @adam-singer)

---

nativelink-scheduler/src/memory_scheduler_state/awaited_action_db/awaited_action_db.rs line 305 at r6 (raw file):

            Err(err) => return Err(err),
            Ok(Some(subscription)) => return Ok(subscription),
            Ok(None) => { /* Add item to queue. */ }

nit: Can we put a flag here to mark this as todo?

---

nativelink-scheduler/src/memory_scheduler_state/awaited_action_db/awaited_action_db.rs line 351 at r6 (raw file):

        let unique_key = match unique_qualifier {
            ActionUniqueQualifier::Cachable(unique_key) => unique_key,
            ActionUniqueQualifier::Uncachable(_unique_key) => return Ok(None),

nit: Can we either do something with this _unique_key or just set it to _ to follow convention?

---

nativelink-scheduler/tests/simple_scheduler_test.rs line 1581 at r6 (raw file):

        };
        let mut received_state = action_state.as_ref().clone();
        if let ActionStage::Completed(stage) = &mut received_state.stage {

nit: I'm really hesitant to add more tests which do any sort of raw string comparison with error messages. I remember similar tests causing a lot of confusion previously during the refactor. Could we just assert that some error is thrown, or better yet, create an error code corresponding to it like MAX_ATTEMPTS_EXCEEDED?

Then we could just implement display for it and have that enum value be this string.

[zbirenbaum]

Reviewable status: 1 of 2 LGTMs obtained, and 3 discussions need to be resolved (waiting on @adam-singer)

[adam-singer]

Reviewed 2 of 4 files at r5, 34 of 34 files at r6, all commit messages.
Reviewable status: 2 of 2 LGTMs obtained, and 3 discussions need to be resolved

[allada]

Reviewable status: 2 of 2 LGTMs obtained, and 3 discussions need to be resolved

---

nativelink-scheduler/src/memory_scheduler_state/awaited_action_db/awaited_action_db.rs line 305 at r6 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

nit: Can we put a flag here to mark this as todo?

This is not a todo, this is simply saying continue we are adding to the queue.

---

nativelink-scheduler/src/memory_scheduler_state/awaited_action_db/awaited_action_db.rs line 351 at r6 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

nit: Can we either do something with this _unique_key or just set it to _ to follow convention?

_unique_key is just saying what the value is. Sure i can make it underscore, but it's the same thing but unnamed.

---

nativelink-scheduler/tests/simple_scheduler_test.rs line 1581 at r6 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

nit: I'm really hesitant to add more tests which do any sort of raw string comparison with error messages. I remember similar tests causing a lot of confusion previously during the refactor. Could we just assert that some error is thrown, or better yet, create an error code corresponding to it like MAX_ATTEMPTS_EXCEEDED?

Then we could just implement display for it and have that enum value be this string.

I didn't add a test, this is making the test pass. We want to make sure it fails because of max attempts, and the only way to do that is via the error message. Before this was not an issue because we had it a hard codded message, but now it's a dynamic message, so it required the test to have more conditions to unwrap it.