Fixed error forwarding to client for failed command executions

[TripleKai]

Description

Added error forwarding to client upon any command execution failures. Allows proper error workflow to complete for internal errors.

Fixes #257

Type of change

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

How Has This Been Tested?

Tested with plain bazel test //... to ensure all pass as well as bazel test //... after adding deliberate modifications to deployment-examples/docker-compose/worker.json to ensure error forwarding to client (ex. "entrypoint_cmd": "foo").

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 

[MarcusSorealheis]

There are two small concerns

First, it would be great if you could amend this commit, git commit --amend, and force push a test, git push -f. The reason for it is we need to have regression detection in pace specifically for this bug. It was a tricky one for a first contribution, especially when new to Rust, so I commend you.

The second thing is a nit:

I would change the commit message to Fixes because this code lives on. Also, in the PR description, is "displays" the correct word?

Reviewable status: 0 of 1 files reviewed, all discussions resolved

[MarcusSorealheis]

note (I'm still ramping on Reviewable myself some days): Needs tests

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @TripleKai)

---

-- commits line 2 at r1:
nit: Fixes

[allada]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @MarcusSorealheis and @TripleKai)

a discussion (no related file):
We need a test for this. @TripleKai , do you want to take a swing at making a test for it?

---

[TripleKai]

Sounds good, I'll make those changes for the commit and reword the description to something like "Added error forwarding to client upon any command execution failures. Allows proper error workflow to complete for internal errors."

[TripleKai]

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

We need a test for this. @TripleKai , do you want to take a swing at making a test for it?

Sure thing, I'll give it a try

[TripleKai]

And thanks for the praise, I'm glad I could contribute!

[allada]

Awesome, good job. Just some minor nits and

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @blakehatch and @TripleKai)

---

native-link-scheduler/tests/simple_scheduler_test.rs line 1409 at r2 (raw file):

    #[tokio::test]
    async fn ensure_task_or_worker_change_notification_received_test() -> Result<(), Error> {

nit: Please add a comment above referencing the ticket this regression test is checking.

---

native-link-scheduler/tests/simple_scheduler_test.rs line 1420 at r2 (raw file):

        let mut rx_from_worker1 = setup_new_worker(&scheduler, WORKER_ID1, PlatformProperties::default()).await?;
        let insert_timestamp = make_system_time(1);

nit: inline this.

---

native-link-scheduler/tests/simple_scheduler_test.rs line 1441 at r2 (raw file):

        }

        let action_info_hash_key = ActionInfoHashKey {

nit: inline this.

---

native-link-scheduler/tests/simple_scheduler_test.rs line 1459 at r2 (raw file):

        {
            // Other tests check full data. We only care if we got StartAction.
            match rx_from_worker2.recv().await.unwrap().update {

Nit: use ?

Code quote:

.unwrap()

[TripleKai]

Sure thing, will do

[TripleKai]

native-link-scheduler/tests/simple_scheduler_test.rs line 1459 at r2 (raw file):

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

Nit: use ?

I tried converting the whole "match" expression into something like "rx_from_worker2.recv().await?.update;"
but this doesn't work because "recv()" returns an Option and our test function returns a Result, so I see a complaint when trying to use "?". Am I misusing this "?" operator?

[allada]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @blakehatch and @TripleKai)

---

native-link-scheduler/tests/simple_scheduler_test.rs line 1459 at r2 (raw file):

Previously, TripleKai (Kailash Saravanan) wrote…

I tried converting the whole "match" expression into something like "rx_from_worker2.recv().await?.update;"
but this doesn't work because "recv()" returns an Option and our test function returns a Result, so I see a complaint when trying to use "?". Am I misusing this "?" operator?

try:

.er_tip(|| "worker went away")?

[allada]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @blakehatch and @TripleKai)

[adam-singer]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @blakehatch and @TripleKai)

[TripleKai]

-- commits line 2 at r1:

Previously, MarcusSorealheis (Marcus Eagan) wrote…

nit: Fixes

Just commenting here to mention this is done

[allada]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @blakehatch and @MarcusSorealheis)

[allada]

Dismissed @MarcusSorealheis and @TripleKai from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TripleKai)

[MarcusSorealheis]

Amazing!