Skip to content

Initial implementation of remote persistent workers - #1922

Closed
MarcusSorealheis wants to merge 8 commits into
TraceMachina:mainfrom
MarcusSorealheis:remote-persistent-workers
Closed

MarcusSorealheis wants to merge 8 commits into
TraceMachina:mainfrom
MarcusSorealheis:remote-persistent-workers

Conversation

@MarcusSorealheis

@MarcusSorealheis MarcusSorealheis commented Sep 3, 2025 •

Copy link
Copy Markdown
Member

Description

Remote persistent workers are coming to Nativelink! Not there yet.

Fixes # (issue)

Type of change

Please delete options that aren't relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please also list any relevant details for your test configuration

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 Reviewable

@MarcusSorealheis
MarcusSorealheis marked this pull request as draft September 3, 2025 23:41
@MarcusSorealheis
MarcusSorealheis force-pushed the remote-persistent-workers branch from 42c8b8e to c69655d Compare September 4, 2025 01:12
@MarcusSorealheis
MarcusSorealheis force-pushed the remote-persistent-workers branch from c3fde9b to be7c3f4 Compare September 5, 2025 07:16
@MarcusSorealheis
MarcusSorealheis marked this pull request as ready for review September 5, 2025 07:30
Comment thread deployment-examples/persistent-workers/nativelink-config.json Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of this is being run in CI, unlike the buildstream/mongo tests. Does this need adding?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

int32 verbosity = 5;

// Sandbox directory for this request
string sandbox_dir = 6;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this needs removing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does need removing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, I'm not sure it does. At first, when I gave my internal presentation, that was my understanding. However, I have been reading the docs again and reading the Bazel source code. Take a look at: https://bazel.build/remote/persistent#options

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remote persistent actions will use the worker strategy but all others could use sandboxed or local.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should still be removed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not seeing any tests for this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very hard to test the worker runner.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to test a few more things that just the key :)

Comment thread integration_tests/persistent-workers-test/package.json Outdated

```bash
npm run build
node dist/e2e.test.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't exist in dist after running build.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ready. I need to work on this more. Sorry for confusion

Implements remote persistent workers feature for NativeLink, enabling
persistent worker processes to be reused across multiple actions for
improved build performance, particularly for JVM-based builds.

Core implementation:
- Persistent worker protocol based on Bazel worker protocol
- Worker lifecycle management with automatic cleanup
- Scheduler integration for persistent worker routing
- Platform properties-based worker key matching

Components:
- PersistentWorkerManager: Pool management with LRU eviction
- PersistentWorkerInstance: Individual worker process lifecycle
- PersistentWorkerRunner: On-demand worker process spawning
- WorkRequest/WorkResponse protocol implementation

Scheduler integration:
- Dedicated worker pools per persistentWorkerKey
- Modified worker matching to check persistent pools first
- Automatic pool management on worker registration/removal

Testing:
- Full unit test coverage
- Integration tests for worker protocol
- Integration tests for scheduler matching
- All 72 tests passing (bazel test //...)

Code quality improvements:
- All clippy warnings resolved with -D warnings
- Proper code formatting
- Correct license headers (BSL 1.1 for enterprise features)
- Change clippy cast_possible_truncation from deny to allow
- Format Cargo.toml exclude array with alphabetical ordering
Remove clippy flags that pre-commit doesn't want:
- await_holding_lock
- elidable_lifetime_names
- explicit_into_iter_loop
- redundant_closure_for_method_calls
- semicolon_if_nothing_returned

Change todo from deny (-D) to warn (-W)
- Apply generate-bazel-rc auto-fixes to .bazelrc
- Remove unused dependencies (hyper-util, tracing-test)
- Fix Vale documentation style issues in README
  - Replace e.g., with 'for example'
  - Use backticks for filenames and compiler names
  - Fix namespace terminology
The tracing-test dependency is required by the #[nativelink_test] macro.
Cargo-machete incorrectly flagged it as unused because it's used by the
macro expansion, not directly in code.

Added cargo-machete ignore metadata to prevent future false positives.
Comment thread Cargo.toml

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and various other config files appear to have been reset to ~0.7.1 era (given the versions) and they remove a bunch of fixes I've done in that time. Possibly merge issues?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct. Let me try to get this fixed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this is here?

@MarcusSorealheis

Copy link
Copy Markdown
Member Author

I need more time to work on this

@MarcusSorealheis

Copy link
Copy Markdown
Member Author

closing this approach in favor of a more secure implementation.

This branch was previously deployed

1 inactive deployment
production — 899a788d Deployed Nov 4, 2025 by MarcusSorealheis via Web Platform Deployment / ubuntu-24.04 #1712
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants