Repository navigation
Let paused idle workers accept new work - #2876
Merged
corcillo merged 10 commits intoOct 5, 2026
Merged
Conversation
memory.current charges every page of file cache the worker's I/O has touched, and the kernel only reclaims it near the limit, so a busy worker under a cgroup limit reported almost nothing free and declined every action asking for more. Free memory is now the limit less the working set: memory.current less inactive_file, the figure kubelet evicts on, with file_dirty and file_writeback still counted as used. memory.current is read on both sides of memory.stat and the larger taken, so cache that grows or is reclaimed mid-read makes the worker report less free, not more. The reads go through an injectable reader so both races are tested, /proc/meminfo is read only without a limit, and local admission reads nothing for an action that reserves no memory.
clippy flagged the to_vec in capacity_test as unnecessary_to_owned; the returned closure cannot borrow the caller's slice, so the helper takes the Vec it iterates.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
corcillo
force-pushed
the
corcillo-whole-worker-admission
branch
from
October 3, 2026 07:36
8c73ffc to
94726fc
Compare
corcillo
changed the title
corcillo
force-pushed
the
corcillo-whole-worker-admission
branch
from
October 3, 2026 09:36
94726fc to
922c3c1
Compare
corcillo
changed the title
…n connection A worker that declines for load is paused until a keepalive reports room for the declined action. When the worker held nothing else under a cgroup memory limit, that room never comes: its own process, its hot page cache and the kernel always count as used, so a reservation near the whole worker (the last memory escalation step, a ladder class near the top, or a step capped by max_kb) was declined and the worker took no work again until it reconnected. Under a limit the worker now admits whatever arrives while it holds nothing else; waiting frees nothing, and an action that truly does not fit is killed and escalated or failed as before. Without a limit free memory is the host's, which other processes can give back, so the check stands. The worker says which it does with admits_when_idle on ConnectWorkerRequest. A worker that admits when idle declines for load only while busy, so the scheduler treats its decline as a busy worker's even when it has taken that work back and thinks the worker idle. For a worker that does not, a decline while the scheduler sees it idle no longer waits for free memory: the pause lifts on the next keepalive, and the size is not offered to it again until a keepalive reports that much free, rather than bouncing the action back and forth untried.
corcillo
force-pushed
the
corcillo-whole-worker-admission
branch
from
October 3, 2026 18:11
922c3c1 to
20647d1
Compare
Reading memory.current on both sides of memory.stat keeps the common races from over-reporting, but the three reads are not atomic: cache that grows at the memory.stat read and is reclaimed before the second usage read is missed by both usage reads and still subtracted, so a transient cache peak can report more free than there is. Say so in the comment and the test, alongside the counter lag already noted.
corcillo
changed the title
…ld admit A worker that admits when idle can still decline for load while the scheduler sees it holding nothing: an action's result can arrive and be completed before the worker has finished uploading it, and the next dispatch meets a worker that still counts it in flight. That pause waited for a keepalive with enough free, which a worker idle by the time it reports may never send. It now lifts on the next keepalive while the scheduler sees the worker holding nothing; a busy one still waits for room. The live memory veto also kept from an idle worker that admits when idle any action over what it last reported free and under its advertised memory: exactly the actions idle admission exists for. It no longer applies to such a worker while it holds nothing.
…' into corcillo-whole-worker-admission
corcillo
marked this pull request as ready for review
October 5, 2026 00:35
…er-admission # Conflicts: # nativelink-scheduler/src/api_worker_scheduler.rs # nativelink-worker/src/capacity.rs # nativelink-worker/src/local_worker.rs # nativelink-worker/tests/capacity_test.rs
Collaborator
Author
|
/build-image nativelink |
Collaborator
Author
|
/build-image nativelink-worker-init |
Contributor
|
Image built and pushed! |
Contributor
|
Image built and pushed! |
Contributor
|
Image built and pushed! |
Contributor
|
Image built and pushed! |
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
Member
There was a problem hiding this comment.
No verified blocking findings in revision 17a40e27feecf421183ee0e83620bf19a2cea4a9. Reviewed the idle-admission flag, scheduler veto and pause recovery, older-worker fallback, and the worker-provisioner integration.
Verification with pinned Rust 1.97.1: dispatch_ack_test (10), capacity_test (18), local_worker_test (25), and worker_utils_test (8) all reported passing on macOS. The Linux-only free-memory admission branches return early on macOS, so those passes do not establish Linux admission coverage. A separate extracted-source probe in a Linux container confirmed idle admission and busy refusal under a finite cgroup limit; it was not a full NativeLink or Kubernetes deployment test. Live Kubernetes recovery and pod churn remain unverified.
This PR was merged while I was completing the review. I am posting the verified review record without any speculative blocking comments.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Right now, a worker declines an action if it needs more memory than the worker currently has free. After declining, it's paused from accepting any new work until a keepalive reports at least that much free memory. That normally happens once its running actions finish and release memory.
If the worker is idle when it declines, nothing is running that could finish and free memory. Its free memory is already as high as it will ever get, so the keepalive never reports enough and the worker never unpauses.
This PR's solution is by adding a rule to the worker's admission check: when nothing else is running and it has its own memory limit, it skips the free-memory check and accepts the action.
Worker: decides once at registration whether it has its own cgroup limit, uses that for admission, and tells the scheduler with a new
admits_when_idlefield onConnectWorkerRequest. Without its own limit (host memory, or a limit shared with other pods), neighbors can free memory, so it still declines.Scheduler: for a worker that still declines while idle (older, or no own limit), the pause lifts on the next keepalive and that size isn't offered to it again until it reports room, so it keeps taking other work and doesn't loop. For a worker that admits when idle and holds nothing, the pause also lifts on the next keepalive (it can decline while its last upload finishes), and the live memory veto doesn't keep from it the actions it would admit.
How was this verified?
dispatch_ack_testcovers the idle decline (lift, skip, re-offer on room), a flagged worker that holds nothing (lifts on the next keepalive), a busy flagged worker (waits for room), and the veto letting an idle flagged worker take an action over its reported free. Removing any of these fails a test.update_protos_testconfirms the generated proto matches; clippy and CI are clean. Not yet run on a live deployment.Risk
Low to moderate. An idle worker under its own limit now runs actions it reads too little free for, which escalation already handles. The proto field is compatible both ways. An older worker can still misjudge when the scheduler's view of what's running differs from its own, and in a fleet of only older workers an oversized action waits rather than fails.
AI assistance
I gave the agent the bug description in a prompt, and we iterated on its design and solution together.