Add safe request timeout for running actions manager

[zbirenbaum]

Description

Backport of running actions manager timeout changes which checks the timeout for an action and defaults to a maximum if not set.

Type of change

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

Checklist

• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[allada]

Reviewable status: 0 of 1 LGTMs obtained

a discussion (no related file):
lgtm, but we need a test.

---

[adam-singer]

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

[zbirenbaum]

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

a discussion (no related file):

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

lgtm, but we need a test.

Test has been added

---

[adam-singer]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks

[adam-singer]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable

[allada]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable

---

nativelink-worker/tests/running_actions_manager_test.rs line 2660 at r4 (raw file):

    #[tokio::test]
    async fn running_actions_manager_respects_action_timeout() -> Result<(), Box<dyn std::error::Error>> {
        #[cfg(target_family = "unix")]

nit: This should probably be removed.

---

nativelink-worker/tests/running_actions_manager_test.rs line 2698 at r4 (raw file):

        )
        .await?;
        const TASK_TIMEOUT: Duration = Duration::from_secs(1);

Tests should not use wall-time. In this case, you probably want to use RunningActionsManagerImpl::new_with_timeout or something to give test control of the timeout function, then use some signaling to facilitate this test.

[zbirenbaum]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, vale, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04

---

nativelink-worker/tests/running_actions_manager_test.rs line 2698 at r4 (raw file):

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

Tests should not use wall-time. In this case, you probably want to use RunningActionsManagerImpl::new_with_timeout or something to give test control of the timeout function, then use some signaling to facilitate this test.

I think the easiest way to do this without actually waiting is to set both the timeout and max_timeout to 0, at which point the checks for time elapsed will just always return true.

[adam-singer]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable

[notion-workspace]

Add safe request timeout for running actions manager

[allada]

Reviewed 1 of 2 files at r6.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable, and 3 discussions need to be resolved

---

nativelink-worker/tests/running_actions_manager_test.rs line 2698 at r4 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

I think the easiest way to do this without actually waiting is to set both the timeout and max_timeout to 0, at which point the checks for time elapsed will just always return true.

I'm not a huge fan of doing this, it makes future bugs even more difficult to write tests for and people will end up copy-pasta the pattern around.

It should be fairly trivial to hand in a custom sleep function and allow the test to override it here, we do this in quite a few places.

[adam-singer]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable, and 3 discussions need to be resolved

[zbirenbaum]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Bazel Dev / ubuntu-22.04, Publish image, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, and 2 discussions need to be resolved

---

nativelink-worker/tests/running_actions_manager_test.rs line 2660 at r4 (raw file):

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

nit: This should probably be removed.

Done.

---

nativelink-worker/tests/running_actions_manager_test.rs line 2698 at r4 (raw file):

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

I'm not a huge fan of doing this, it makes future bugs even more difficult to write tests for and people will end up copy-pasta the pattern around.

It should be fairly trivial to hand in a custom sleep function and allow the test to override it here, we do this in quite a few places.

I've updated it to take a custom sleep function which will immediately return if the correct timeout is passed and stall indefinitely causing the action to complete and the test to fail otherwise.

[allada]

Reviewed 1 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: windows-2022 / stable, and 2 discussions need to be resolved

---

nativelink-worker/tests/running_actions_manager_test.rs line 2944 at r8 (raw file):

        fs::create_dir_all(&root_work_directory).await?;

        // ignore the sleep and immediately timeout

nit: Caps and period.

---

nativelink-worker/tests/running_actions_manager_test.rs line 2973 at r8 (raw file):

                // otherwise return pending and fail the test.
                sleep_fn: |duration| {
                    if duration.as_secs() == ACTION_TIMEOUT as u64 {

nit: For readability, assert that duration is ACTION_TIMEOUT and then return ready.

[adam-singer]

Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: windows-2022 / stable, and 2 discussions need to be resolved

[zbirenbaum]

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

---

nativelink-worker/tests/running_actions_manager_test.rs line 2944 at r8 (raw file):

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

nit: Caps and period.

Done.

---

nativelink-worker/tests/running_actions_manager_test.rs line 2973 at r8 (raw file):

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

nit: For readability, assert that duration is ACTION_TIMEOUT and then return ready.

Done.

[adam-singer]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: windows-2022 / stable

[allada]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable

[zbirenbaum]

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: 2 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, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04

[zbirenbaum]

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

        let arguments = vec!["sh".to_string(), "-c".to_string(), "sleep 2".to_string()];
        #[cfg(target_family = "windows")]
        let arguments = vec![

This is what is causing the test to fail. It is returning error code 1 which is incorrect function. I have been reading up on it but can't figure it out so I'm just going to remove to remove this test for windows if that's alright @allada

[adam-singer]

Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

[allada]

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

This is what is causing the test to fail. It is returning error code 1 which is incorrect function. I have been reading up on it but can't figure it out so I'm just going to remove to remove this test for windows if that's alright @allada

Why not down below set the error code on windows to expect exit code 1?

[zbirenbaum]

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

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

Why not down below set the error code on windows to expect exit code 1?

Judging by the docs it seems like exit code 1 corresponds to something else that wouldn't actually be showing the proper behavior here. It's difficult to tell so I guess its possible that's the expected error code but I'm not certain. If not being sure is preferable to removing the test we can do that

[allada]

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

Judging by the docs it seems like exit code 1 corresponds to something else that wouldn't actually be showing the proper behavior here. It's difficult to tell so I guess its possible that's the expected error code but I'm not certain. If not being sure is preferable to removing the test we can do that

This test is not actually testing the error code, it's checking that the timeout triggers. The error code is a side-effect. In this case, I would just make a TODO and keep the test for windows.

[zbirenbaum]

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-22.04

---

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

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

This test is not actually testing the error code, it's checking that the timeout triggers. The error code is a side-effect. In this case, I would just make a TODO and keep the test for windows.

Done.