enhancement(core): add support for dynamically added child processes in Supervisor - #1874
Conversation
Binary Size Analysis (Agent Data Plane)Baseline: f4b7e76 · Comparison: bf9ca0f · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
Supervisor
acb0691 to
db98447
Compare
aeb5585 to
a87eb98
Compare
2c9e3ed to
b7baf78
Compare
ffcb7af to
7bebf14
Compare
b7baf78 to
e743bed
Compare
This comment has been minimized.
This comment has been minimized.
7bebf14 to
a688204
Compare
dbef154 to
91233f3
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances saluki-core’s runtime supervision system to support dynamic child processes and bring restart/shutdown behavior closer to Erlang/OTP’s supervisor.erl, enabling patterns like supervising per-connection tasks under a long-lived supervisor.
Changes:
- Added
SupervisorHandle/ChildIdAPIs for spawning workers dynamically while a supervisor is running. - Introduced
ShutdownMode(ordered vs concurrent) andAutoShutdownfor significant-child-driven supervisor shutdown. - Refactored worker bookkeeping into a dedicated
WorkerStatemodule and expanded unit tests to cover the new semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/saluki-core/src/runtime/worker_state.rs | New worker/task bookkeeping module, including ordered vs concurrent shutdown behavior. |
| lib/saluki-core/src/runtime/supervisor.rs | Adds dynamic spawn handle/channel, significant child auto-shutdown, shutdown mode, and extensive tests; refactors supervision loop to use WorkerState. |
| lib/saluki-core/src/runtime/mod.rs | Re-exports new runtime API types and wires in the new worker_state module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91233f322d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
c6b9d24 to
944fd29
Compare
a688204 to
e678e20
Compare
944fd29 to
ce07aa0
Compare
e678e20 to
36c4cc9
Compare
9444d49 to
1a2baee
Compare
1a2baee to
33bac87
Compare
webern
left a comment
There was a problem hiding this comment.
My visual reading of it isn't worth much, I more-or-less understand what's going on. Gave it an opus max pass with no bug findings as well.
33bac87 to
bf9ca0f
Compare

Summary
This PR adds support for dynamically adding child processes to a
Supervisor, along with other changes to better align our supervisor behavior with the behavior of the granddaddy of all supervisors,supervisor.erlin Erlang/OTP.Currently,
Supervisoris designed to be configured before running, and then, once running, only ever manage the workers it was configured with. This is sufficient for almost all of our usage, but it makes it impossible to support a small but reasonable use case: supervision of dynamically spawned tasks, like network connection handlers (DSD stream handlers, for example).This PR introduces support for dynamically spawned workers, along with some other behavioral uplifts, to support a higher-fidelity implementation of supervisors:
SupervisorHandle, a new type that can be used to dynamically spawn workers at runtime, when a supervisor is already runningShutdownMode, to support concurrently stopping processes during shutdown, which is useful in a world where a supervisor may be supervising many dynamic child processesWhat this will enable us to do is actually spawn each topology component as a dedicated supervisor, with the traditional component task as a significant child in that supervisor, and then give the component a handle to its supervisor so it can spawn whatever other child processes it requires. In doing so, we can ensure all of those descendant child processes are shutdown when the component task itself exits (ab)normally.. but that we can track their association (at a high level) to the component that spawned them, rather than really having no good idea where some arbitrary async task originated from.
Most of the code in this PR is tests: we've altered some of the restart/shutdown behavior not only to support dynamic child processes, but also to properly match the behavior exhibited by Erlang/OTP's
supervisor.erl.Change Type
How did you test this PR?
A lot of new and existing unit tests.
References
DADP-2