ClientKeepAlive update action ClientKeepAlive

[chrisstaite-menlo]

Description

When the scheduler was updated to add the keep alive to the AwaitedAction the MemoryAwaitedActionDb was not updated to set this when a ClientKeepAlive was received.

Fix the test client_reconnect_keeps_action_alive which was not performing the eviction due to optimisations in the filter_operations function which then detected the issue.

Then update the ActionEvent::ClientKeepAlive event handler to update the client keep alive timestamp in the AwaitedAction.

Fixes #1579.

Type of change

Please delete options that aren't relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Fixed the existing test.

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 

[CLAassistant]

All committers have signed the CLA.

[aaronmondal]

Thanks!

@allada Could this affect the redis scheduler as well?

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / macos-13, Bazel Dev / macos-14, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Coverage, Installation / macos-13, Installation / macos-14, Local / lre-rs / macos-14, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, docker-compose-compiles-nativelink (22.04), windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-scheduler/src/simple_scheduler.rs line 374 at r1 (raw file):

        // tasks are going to be dropped all over the place, this isn't a good
        // setting.
        if client_action_timeout_s <= 10 {

nit: Seems like we could consolidate the CLIENT_KEEPALIVE_DURATIONs in 'memory_awaited_action_dbandstore_awaited_action_db` and reuse that here as well.

[chrisstaite-menlo]

The Redis scheduler already handles this case, it was just a missing edge of the in-memory DB.

[allada]

Yeah, this is why we didn't catch this. I really need to get around to removing memory scheduler and just implement SchedulerStore to MemoryStore, I hate lugging around two implementations :-(

Thanks a lot Chris!

Reviewed 1 of 4 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / macos-13, Bazel Dev / macos-14, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-13, Coverage, Installation / macos-13, Installation / macos-14, Local / lre-rs / macos-14, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, docker-compose-compiles-nativelink (22.04), windows-2022 / stable

[allada]

Reviewed 1 of 4 files at r1.
Reviewable status: 2 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / macos-13, Bazel Dev / macos-14, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-13, Coverage, Installation / macos-13, Installation / macos-14, Local / lre-rs / macos-14, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, docker-compose-compiles-nativelink (22.04), windows-2022 / stable