Interval for keepalives

[palfrey]

Description

Worker keepalive should be happening every timeout/2, but we were doing this with tokio::time::sleep and if the actual keep alive takes more than a trivial amount of time (e.g. because the worker is busy with a job) then sometimes these can drift. Example log:

{"timestamp":"2026-05-06T09:03:00.790132Z","level":"DEBUG","fields":{"message":"Worker received command","command":"Command { arguments: [\"bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/rules_rust+/cargo/cargo_build_script_runner/runner\", ]"},"target":"nativelink_worker::running_actions_manager","span":{"name":"worker_start_action"},"spans":[{"name":"\"worker_0\"","name":"worker"},{"shutdown_rx":"broadcast::Receiver","name":"run"},{"digest_function":"SHA256","operation_id":"bfbf9d50-da91-4f2f-88d1-24c19761a461","name":"worker_start_action_ctx"},{"name":"worker_start_action"}]}
{"timestamp":"2026-05-06T09:03:01.481372Z","level":"DEBUG","fields":{"message":"Sent KeepAlive"},"target":"nativelink_worker::local_worker","span":{"shutdown_rx":"broadcast::Receiver","name":"run"},"spans":[{"name":"\"worker_0\"","name":"worker"},{"shutdown_rx":"broadcast::Receiver","name":"run"}]}
{"timestamp":"2026-05-06T09:03:03.982417Z","level":"DEBUG","fields":{"message":"Sent KeepAlive"},"target":"nativelink_worker::local_worker","span":{"shutdown_rx":"broadcast::Receiver","name":"run"},"spans":[{"name":"\"worker_0\"","name":"worker"},{"shutdown_rx":"broadcast::Receiver","name":"run"}]}
{"timestamp":"2026-05-06T09:03:06.483354Z","level":"DEBUG","fields":{"message":"Sent KeepAlive"},"target":"nativelink_worker::local_worker","span":{"shutdown_rx":"broadcast::Receiver","name":"run"},"spans":[{"name":"\"worker_0\"","name":"worker"},{"shutdown_rx":"broadcast::Receiver","name":"run"}]}
{"timestamp":"2026-05-06T09:03:10.086268Z","level":"DEBUG","fields":{"message":"Sent KeepAlive"},"target":"nativelink_worker::local_worker","span":{"shutdown_rx":"broadcast::Receiver","name":"run"},"spans":[{"name":"\"worker_0\"","name":"worker"},{"shutdown_rx":"broadcast::Receiver","name":"run"}]}
{"timestamp":"2026-05-06T09:03:12.590302Z","level":"DEBUG","fields":{"message":"Sent KeepAlive"},"target":"nativelink_worker::local_worker","span":{"shutdown_rx":"broadcast::Receiver","name":"run"},"spans":[{"name":"\"worker_0\"","name":"worker"},{"shutdown_rx":"broadcast::Receiver","name":"run"}]}
{"timestamp":"2026-05-06T09:03:15.211663Z","level":"DEBUG","fields":{"message":"Sent KeepAlive"},"target":"nativelink_worker::local_worker","span":{"shutdown_rx":"broadcast::Receiver","name":"run"},"spans":[{"name":"\"worker_0\"","name":"worker"},{"shutdown_rx":"broadcast::Receiver","name":"run"}]}
{"timestamp":"2026-05-06T09:03:19.680283Z","level":"DEBUG","fields":{"message":"Sent KeepAlive"},"target":"nativelink_worker::local_worker","span":{"shutdown_rx":"broadcast::Receiver","name":"run"},"spans":[{"name":"\"worker_0\"","name":"worker"},{"shutdown_rx":"broadcast::Receiver","name":"run"}]}
{"timestamp":"2026-05-06T09:03:26.392454Z","level":"DEBUG","fields":{"message":"Sent KeepAlive"},"target":"nativelink_worker::local_worker","span":{"shutdown_rx":"broadcast::Receiver","name":"run"},"spans":[{"name":"\"worker_0\"","name":"worker"},{"shutdown_rx":"broadcast::Receiver","name":"run"}]}

(actual job truncated for readability)
Note how the first few keepalives after the job are still at 2.5s intervals (using default 5s spacing), but then they start to drift and the 09:03:15-19 gap is 4s, which is still ok enough, but then the 19-26 gap is 7s which is enough for the scheduler to timeout the worker.

This PR does three things:

• Redoes the keep alive with tokio::time::interval to explicitly do keep alives every timeout/2 (or as close as possible) even if the individual ones take a few seconds
• Explicitly spawns the actual keep alive loop so it doesn't get blocked when there's a heavy task running on the worker
• Upgrades tokio from 1.50.0 to 1.52.2 - possibly not strictly necessary, but using the most recent version seemed like a good idea

I previously had a test case that reliably drifted anywhere up to 106% extra time (31s v.s. 15s) and now drifts only up to ~2% (15.3s max in my most recent run vs. 15s).

Type of change

Please delete options that aren't relevant.

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

How Has This Been Tested?

bazel test //... and manual testing with the image

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 

[palfrey]

/build-image

[palfrey]

/build-image nativelink-worker-init

[github-actions]

Image built and pushed!

ghcr.io/TraceMachina/nativelink-worker-init:8804b8b

[github-actions]

Image built and pushed!

ghcr.io/TraceMachina/nativelink:8804b8b

[MarcusSorealheis]

sometimes these can drift.

That's not good.

Once soak tested, we can merge it. Maybe share this image with the users to get their opinion? I'm worried about it becoming to chatty.

Another thought is I don't like how duration here is f32. That's not your fault and I believe it should predate you but I didn't check the blame. Just something to think about. When debugging on a seconds basis, f32 can get weird in my experience.

[MarcusSorealheis]

That said, MissedTickBehavior::Skip is the wrong default for a keepalive. A keepalive's job is to assert liveness before a timeout expires. If a tick was missed, i.e. something blocked the task long enough that a timeout/2 heartbeat slipped, that's exactly when you most want to fire immediately, not wait for the next aligned window. Delay ("I'm late, send now, re-anchor from here") is the more defensible policy. Skip makes sense for metrics emission where catching up is pointless; it's a worse fit when the tick is the thing keeping a connection alive. The point about Http2 managing most timeouts is a fair argument, though.

[palfrey]

That said, MissedTickBehavior::Skip is the wrong default for a keepalive

I think Skip is the right behaviour. See https://docs.rs/tokio/latest/tokio/time/enum.MissedTickBehavior.html#variant.Skip but my reading of that is that is that Skip does one immediately after a delay, and then resets to the cadence we want v.s. say Burst that would try and catchup with many of them which doesn't feel needed? Delay might even be the right answer, as keepalives need the interval between them not the actual number of them to be preserved.

[chrisstaite-menlo]

@chrisstaite-menlo reviewed 1 file and made 1 comment.
Reviewable status: complete! 1 of 1 LGTMs obtained, and all files reviewed.

[palfrey]

Hmm. Except I'm still seeing weird numbers. I ran a test with this image, and the timeout set to 30s, and got the following diffs between keepalives

diff 0:00:14.999453
diff 0:00:15.000596
diff 0:00:14.999291
diff 0:00:15.000670
diff 0:00:14.999775
diff 0:00:15.000225
diff 0:00:14.999451
diff 0:00:15.000499
diff 0:00:14.999892
diff 0:00:14.999433
diff 0:00:15.000401
diff 0:00:15.000318
diff 0:00:14.999925
diff 0:00:14.999898
diff 0:00:14.999531
diff 0:00:15.000099
diff 0:00:17.481680
diff 0:00:31.200054

Note most of them are stable around the expected 15s mark, but then you get a slight drift to 17 and then a 31! I need to investigate this further.

[MarcusSorealheis]

@palfrey is this related to f32 type, rather than u64 or something higher precision?

[palfrey]

/build-image nativelink-worker-init

[palfrey]

/build-image

[palfrey]

@palfrey is this related to f32 type, rather than u64 or something higher precision?

I don't think so, as the conversion from f32 -> Duration will occur once, and that seems to be getting a good number to begin with. I've made a change to do the / 2 to the Duration, not the f32, but that's somewhat cosmetic as the floating point accuracy issues are going to be way smaller than the issues I'm seeing here.

[github-actions]

Image built and pushed!

ghcr.io/TraceMachina/nativelink-worker-init:a959a06

[github-actions]

Image built and pushed!

ghcr.io/TraceMachina/nativelink:a959a06

[palfrey]

/build-image nativelink-worker-init

[palfrey]

/build-image

[github-actions]

Image built and pushed!

ghcr.io/TraceMachina/nativelink:2a13885

[github-actions]

Image built and pushed!

ghcr.io/TraceMachina/nativelink-worker-init:2a13885

The spawn is a notable change

[chrisstaite-menlo]

nativelink-worker/src/local_worker.rs line 200 at r6 (raw file):

        })
        .await
        .unwrap()

I don't think the unwrap is necessary, just drop it.

[palfrey]

nativelink-worker/src/local_worker.rs line 200 at r6 (raw file):

        })
        .await
        .unwrap()

I don't think the unwrap is necessary, just drop it.

I've tried a few variants of this, and it seems to break various of the other worker tests (experimental_precondition_script_fails in particular, but also others), but the unwrap variant as done right now seems to just work, so I'd prefer to keep it in.

[chrisstaite-menlo]

nativelink-worker/src/local_worker.rs line 200 at r6 (raw file):

Previously, palfrey (Tom Parker-Shemilt) wrote…

I've tried a few variants of this, and it seems to break various of the other worker tests (experimental_precondition_script_fails in particular, but also others), but the unwrap variant as done right now seems to just work, so I'd prefer to keep it in.

I don't see how this is different to drop(spawn!(...).await);

[palfrey]

@palfrey made 1 comment.
Reviewable status: 1 of 1 LGTMs obtained, and 0 of 12 files reviewed, and pending CI: Redis store tester, macos-26, ubuntu-24.04, integration-tests, ubuntu-24.04 / stable, windows-2022 / stable, pre-commit-checks, Analyze (javascript-typescript), Analyze (python), Coverage, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Publish nativelink-worker-lre-rs, Web Platform Deployment / macos-26, Web Platform Deployment / ubuntu-24.04, Local / lre-cc / ubuntu-24.04, Local / lre-rs / macos-26, Local / lre-rs / ubuntu-24.04, Bazel Dev / macos-26, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-26, Cargo Dev / ubuntu-24.04, Installation / macos-26, Installation / ubuntu-24.04, buck2, buildstream, mongo, rbe-toolchain, asan / ubuntu-24.04, CodeQL, and 1 discussions need to be resolved.

---

nativelink-worker/src/local_worker.rs line 200 at r6 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

I don't see how this is different to drop(spawn!(...).await);

I'm pretty sure I did this before, and it didn't work. And now I've tried it again and it works. sigh

[chrisstaite-menlo]

@chrisstaite-menlo resolved 1 discussion.
Reviewable status: 1 of 1 LGTMs obtained, and 0 of 12 files reviewed, and pending CI: macos-26, ubuntu-24.04, integration-tests, ubuntu-24.04 / stable, windows-2022 / stable, Coverage, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-rs, Web Platform Deployment / macos-26, Web Platform Deployment / ubuntu-24.04, Local / lre-rs / macos-26, Bazel Dev / macos-26, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-26, Cargo Dev / ubuntu-24.04, Installation / macos-26, Installation / ubuntu-24.04, buck2, buildstream, mongo, rbe-toolchain, asan / ubuntu-24.04.

[MarcusSorealheis]

I love you both and both of your work, but "it works in tests" is not quite sufficient for this issue IMHO. A silently dropped keepalive task is exactly the failure mode you're trying to prevent. I like the new edit

[MarcusSorealheis]

.unwrap() is very dangerous

[MarcusSorealheis]

LGTM