Skip to content

feature: bring back action runners#14240

Open
rgrinberg wants to merge 1 commit into
ocaml:mainfrom
rgrinberg:push-psryzmryxwzv
Open

feature: bring back action runners#14240
rgrinberg wants to merge 1 commit into
ocaml:mainfrom
rgrinberg:push-psryzmryxwzv

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg commented Apr 18, 2026

Bring back action runners and use them in conjunction with bubble wrap to make it impossible to modify the shared cache.

I'm going to enable this through the makefile for folks on the dev team to get some beta testing. If all goes well, it should be available for 3.24.

@anmonteiro I've been told that sandbox-exec offers comparable functionality on macos. It would be good to have this as an alternative to bwrap.

Work Included

  • Add --action-runner and the internal action-runner worker command.
  • Add --sandbox-actions, which runs eligible actions through a bubblewrap-wrapped
    worker and protects the shared cache from worker writes.
  • Extend process execution with a path-based runner hook, runner-safe metadata,
    output/capture handling, and parent-owned trace events.
  • Add action eligibility plumbing (Allow_action_runner, can_run_in_action_runner,
    runs_process) for user actions, cram tests, pp/ppx, inline tests, and selected
    action extensions.
  • Add RPC protocol/server support for runner ready/exec/cancel requests,
    per-generation lifecycle tracking, disconnect handling, and build-cancellation
    propagation.
  • Add trace events and inherited trace-fd support for worker lifecycle and
    process events.
  • Account for sandbox-actions in rule digests only for actions that spawn
    processes.
  • Add black-box coverage for runner execution, failures, disconnects,
    cancellation, watch shutdown, tracing, and sandboxed actions.

@rgrinberg rgrinberg force-pushed the push-psryzmryxwzv branch 8 times, most recently from 147f9a4 to 570c0f7 Compare April 21, 2026 21:18
@rgrinberg rgrinberg marked this pull request as ready for review April 21, 2026 21:42
@rgrinberg rgrinberg force-pushed the push-psryzmryxwzv branch 2 times, most recently from aabe781 to 3f29e77 Compare April 24, 2026 07:12
@rgrinberg rgrinberg force-pushed the push-psryzmryxwzv branch 3 times, most recently from 040eb08 to d67f1e5 Compare May 5, 2026 21:28
@rgrinberg rgrinberg force-pushed the push-psryzmryxwzv branch 3 times, most recently from e48454a to 2dd926d Compare May 16, 2026 16:38
@Alizter Alizter self-requested a review May 16, 2026 16:39
@Alizter Alizter self-assigned this May 18, 2026
Comment thread src/dune_engine/action_runner.ml Outdated
Comment on lines +395 to +398
| Some active when active.run_id = run_id && active.count > 1 ->
active.count <- active.count - 1;
Fiber.return ()
| Some active when active.run_id = run_id && active.count = 1 ->
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| Some active when active.run_id = run_id && active.count > 1 ->
active.count <- active.count - 1;
Fiber.return ()
| Some active when active.run_id = run_id && active.count = 1 ->
| Some active when Run_id.equal active.run_id run_id && active.count > 1 ->
active.count <- active.count - 1;
Fiber.return ()
| Some active when Run_id.equal active.run_id run_id && active.count = 1 ->

Comment thread src/dune_engine/action_runner.ml
Comment thread src/dune_trace/out.ml
; path : Path.t
; path : Path.t option
; mutable alloc : Alloc.t option
; terminate_process_on_error : bool
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What's the reason for storing this separately? Isn't it just Option.is_none path?

Comment thread src/dune_trace/dune_trace.ml Outdated
Option.iter alloc_summary ~f:(Out.emit out);
Out.emit out (Event.exit ());
Out.close out;
(match Env.(get initial Dune_action_trace.Private.trace_dir_env_var), out.path with
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wondering if out.path and Owened / Borrowed should just be tied together.

Something like Owned of Path.t | Borrowed for both?

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented May 18, 2026

I've noticed that quite a few sets of rules don't pass ~can_run_in_action_runner:true at the moment. What is the reason for excluding them here? Do you plan to include them eventually? I'd even suggest we make this argument non-optional in command.ml so that we can explicitly track it.

, Terminal { output_on_success = Print; _ } )
| ( Terminal { output_on_success = Swallow; _ }
, Terminal { output_on_success = Swallow; _ } ) ->
(* We don't merge when both are [Must_be_empty]. If we did and an
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why remove this comment? Is it stale?

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented May 18, 2026

Could you add a test demonstrating that --action-runner leaves the rule digest invariant.

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented May 18, 2026

I'm going to enable this through the makefile for folks on the dev team to get some beta testing. If all goes well, it should be available for 3.24.

FYI this isn't currently done.

Copy link
Copy Markdown
Collaborator

@Alizter Alizter left a comment

Choose a reason for hiding this comment

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

I've had an initial look through the implementation, paying special attention to the changes to build_system and the trace and things look good. I've made some comments as I was passing through.

Needs a changes entry.

I'll do another pass tomorrow.

@rgrinberg
Copy link
Copy Markdown
Member Author

What is the reason for excluding them here? Do you plan to include them eventually? I'd even suggest we make this argument non-optional in command.ml so that we can explicitly track it.

The goal is to sandbox all unsafe actions. For now, that includes preprocessors and user defined actions. I do not plan to use action runners for ocamldep, ocamlopt or anything else that is performance sensitive or is white listed.

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented May 18, 2026

What is the reason for excluding them here? Do you plan to include them eventually? I'd even suggest we make this argument non-optional in command.ml so that we can explicitly track it.

The goal is to sandbox all unsafe actions. For now, that includes preprocessors and user defined actions. I do not plan to use action runners for ocamldep, ocamlopt or anything else that is performance sensitive or is white listed.

Could you document this intent in somewhere like command.mli or action_runner.mli since this was not apparent in my first read and would be important to mention.

Add an opt-in action runner for executing eligible build processes in a
separate Dune worker process.

- Add --action-runner and the internal action-runner worker command.
- Add --sandbox-actions, which runs eligible actions through a bubblewrap-wrapped
  worker and protects the shared cache from worker writes.
- Extend process execution with a path-based runner hook, runner-safe metadata,
  output/capture handling, and parent-owned trace events.
- Add action eligibility plumbing (Allow_action_runner, can_run_in_action_runner,
  runs_process) for user actions, cram tests, pp/ppx, inline tests, and selected
  action extensions.
- Add RPC protocol/server support for runner ready/exec/cancel requests,
  per-generation lifecycle tracking, disconnect handling, and build-cancellation
  propagation.
- Add trace events and inherited trace-fd support for worker lifecycle and
  process events.
- Account for sandbox-actions in rule digests only for actions that spawn
  processes.
- Add black-box coverage for runner execution, failures, disconnects,
  cancellation, watch shutdown, tracing, and sandboxed actions.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

refactor: pass run ids to action runner requests

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

refactor(action): store runner eligibility on Action.Full

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

fix(cache): avoid unnecessary rule digest churn

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg force-pushed the push-psryzmryxwzv branch from 2dd926d to 31ba228 Compare June 3, 2026 22:03
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.

2 participants