enhancement(core): add configurable per-child restart types for supervisors - #1873
Conversation
This comment has been minimized.
This comment has been minimized.
Binary Size Analysis (Agent Data Plane)Baseline: 35ec839 · Comparison: 36c4cc9 · 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 (35)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 ( |
a80937c to
cdc8126
Compare
aeb5585 to
a87eb98
Compare
There was a problem hiding this comment.
💡 Codex Review
With RestartMode::OneForAll, if a RestartType::Transient child has already exited with Ok(()), the supervisor leaves it stopped as intended, but a later failure from any sibling reaches this spawn_all_children() call and starts every static child spec again. That means a transient one-shot child that completed normally can be relaunched even though RestartType::Transient documents that normal exits are intentional and not restarted; this can rerun completed work whenever a sibling trips one-for-all restart.
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR enhances saluki-core’s runtime supervision framework by adding Erlang/OTP-style per-child restart policies, allowing supervisors to selectively restart children based on how they exited.
Changes:
- Introduces
RestartType(Permanent/Transient/Temporary) and exports it viaruntime. - Refactors
ChildSpecificationinto a typestate-based API to allow configuring restart type for workers while preventing invalid configurations for nested supervisors. - Updates supervisor run-loop logic and adds unit tests covering new restart-type behavior (including “no workers left” idling behavior).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/saluki-core/src/runtime/supervisor.rs |
Adds typestate-based child specs, stores per-child RestartType, updates restart/idling logic, and adds restart-type unit tests. |
lib/saluki-core/src/runtime/restart.rs |
Adds the RestartType enum and internal restart-eligibility helper. |
lib/saluki-core/src/runtime/mod.rs |
Re-exports new runtime API surface (RestartType, ChildSpecification, typestate markers). |
.vale/styles/config/vocabularies/technical/accept.txt |
Adds “typestate” to the Vale technical vocabulary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
616e732 to
ffcb7af
Compare
cdc8126 to
6e7c11b
Compare
ffcb7af to
7bebf14
Compare
6e7c11b to
6de0a10
Compare
a688204 to
e678e20
Compare
| None => unreachable!( | ||
| "join set is non-empty here: we park above while empty, and only this method removes workers" | ||
| ), |
e678e20 to
36c4cc9
Compare
ed28518
into
main
…visors (#1873) ## Summary This PR adds support for adjusting the restart type of child processes. In Erlang/OTP, supervisors expose a lot more control over how child processes are supervised, up to and including whether or not to actually restart the process. Our current implementation of `Supervisor` hardcodes the behavior of always restarting a child process, but this is not flexible enough for our needs. This PR introduces a new `RestartType` enum which encodes the possible restart types available: permanent (always restart; **default**), transient (restart if process exited with an error), and temporary (never restart). This mimics the restart types exposed in Erlang/OTP. The corresponding worker restart logic has been updated to handle and respect these new restart types. We've also exposed some new machinery in `ChildSpecification` to allow specifying a non-default restart type, and to do so safely, we've used some typestate magic so that people can't do incorrect things like change the restart type for a supervisor, and so on. Support for restart types sets us up for a number of incremental improvements towards a fuller feature set in supervisors, including significant children and dynamically added children. ## Change Type - [ ] Bug fix - [x] New feature - [ ] Non-functional (chore, refactoring, docs) - [ ] Performance ## How did you test this PR? New and existing unit tests. ## References DADP-2 Co-authored-by: toby.lawrence <toby.lawrence@datadoghq.com> ed28518
…visors (#1873) This PR adds support for adjusting the restart type of child processes. In Erlang/OTP, supervisors expose a lot more control over how child processes are supervised, up to and including whether or not to actually restart the process. Our current implementation of `Supervisor` hardcodes the behavior of always restarting a child process, but this is not flexible enough for our needs. This PR introduces a new `RestartType` enum which encodes the possible restart types available: permanent (always restart; **default**), transient (restart if process exited with an error), and temporary (never restart). This mimics the restart types exposed in Erlang/OTP. The corresponding worker restart logic has been updated to handle and respect these new restart types. We've also exposed some new machinery in `ChildSpecification` to allow specifying a non-default restart type, and to do so safely, we've used some typestate magic so that people can't do incorrect things like change the restart type for a supervisor, and so on. Support for restart types sets us up for a number of incremental improvements towards a fuller feature set in supervisors, including significant children and dynamically added children. - [ ] Bug fix - [x] New feature - [ ] Non-functional (chore, refactoring, docs) - [ ] Performance New and existing unit tests. DADP-2 Co-authored-by: toby.lawrence <toby.lawrence@datadoghq.com>

Summary
This PR adds support for adjusting the restart type of child processes.
In Erlang/OTP, supervisors expose a lot more control over how child processes are supervised, up to and including whether or not to actually restart the process. Our current implementation of
Supervisorhardcodes the behavior of always restarting a child process, but this is not flexible enough for our needs.This PR introduces a new
RestartTypeenum which encodes the possible restart types available: permanent (always restart; default), transient (restart if process exited with an error), and temporary (never restart). This mimics the restart types exposed in Erlang/OTP. The corresponding worker restart logic has been updated to handle and respect these new restart types. We've also exposed some new machinery inChildSpecificationto allow specifying a non-default restart type, and to do so safely, we've used some typestate magic so that people can't do incorrect things like change the restart type for a supervisor, and so on.Support for restart types sets us up for a number of incremental improvements towards a fuller feature set in supervisors, including significant children and dynamically added children.
Change Type
How did you test this PR?
New and existing unit tests.
References
DADP-2