Skip to content

Commit d7d791f

Browse files
author
developerworks
committed
Remove confique, consolidate config, and harden dashboard and control loop
- Remove confique dependency from Cargo.toml and Cargo.lock - Simplify config loader to use serde directly without confique - Add Cargo config.toml with build settings for deterministic artifacts - Harden dashboard registration with retry and heartbeat isolation - Refactor concurrent restart gate with admission-set replacement - Strengthen control loop spawn ordering and pending event coherence - Expand dashboard model, IPC server, and registration for edge cases - Update integration tests for pipeline and registration heartbeat
1 parent d63fda4 commit d7d791f

18 files changed

Lines changed: 597 additions & 259 deletions

.cargo/config.toml

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# Automatically enable test-support when running tests.
2+
# This ensures `cargo test` and `cargo check --tests` compile the
3+
# test-support module without requiring `--features test-support`.
4+
[env]
5+
# (empty — reserved for future use)
6+
7+
# Target directory override for CI builds
8+
# [build]
9+
# target-dir = "target"
10+
11+
# Test-specific alias
12+
[alias]
13+
test = "test --features test-support"
14+
check-tests = "check --tests --features test-support"

Cargo.lock

Lines changed: 0 additions & 69 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ include = [
2525
]
2626

2727
[features]
28-
default = ["test-support"]
28+
default = []
2929
test-support = []
3030

3131
[lib]
@@ -48,7 +48,15 @@ serde = { version = "1", features = ["derive"] }
4848
serde_json = "1"
4949
serde_yaml = "0.9"
5050
thiserror = "2"
51-
tokio = { version = "1.52.3", features = ["full", "test-util"] }
51+
tokio = { version = "1.52.3", features = [
52+
"rt",
53+
"rt-multi-thread",
54+
"macros",
55+
"sync",
56+
"time",
57+
"net",
58+
"io-util",
59+
] }
5260
tokio-util = "0.7"
5361
tracing = "0.1.44"
5462
tracing-subscriber = "0.3.23"

src/child_runner/runner.rs

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -114,21 +114,38 @@ impl ChildRunner {
114114
let (completion_sender, completion_receiver) = watch::channel(None);
115115

116116
// Choose the spawn strategy based on the child's isolation setting.
117-
// BlockingPool tasks use spawn_blocking to avoid starving tokio's
118-
// async worker threads — any blocking or CPU-heavy work stays on
119-
// the dedicated blocking thread pool (capacity up to 500 threads).
117+
// BlockingPool tasks are spawned on the async worker pool normally but
118+
// wrapped with `tokio::task::block_in_place` at the very start. This
119+
// signals to the tokio scheduler that the worker thread may block and
120+
// allows a replacement worker to be spawned, preventing worker thread
121+
// starvation without creating a nested runtime.
122+
//
123+
// Background: `spawn_blocking` + `spawn_blocking` inner runtime is an
124+
// anti-pattern because it allocates a full current-thread runtime per
125+
// blocking thread, wasting memory and potentially exhausting the
126+
// blocking pool. `block_in_place` is the tokio-approved approach for
127+
// CPU-heavy or blocking async tasks.
120128
let child_task = match runtime.spec.isolation {
121129
crate::spec::child::Isolation::BlockingPool => {
122130
let ctx_clone = ctx.clone_for_blocking();
123-
tokio::task::spawn_blocking(move || {
124-
// spawn_blocking returns a blocking thread result, but the
125-
// factory future still needs to run on an async runtime.
126-
// We spawn a minimal local runtime to execute it.
127-
let rt = tokio::runtime::Builder::new_current_thread()
128-
.enable_all()
129-
.build()
130-
.expect("BlockingPool: failed to build one-shot runtime");
131-
rt.block_on(factory.build(ctx_clone))
131+
let shared_factory = factory.clone();
132+
tokio::spawn(async move {
133+
// Signal to the tokio scheduler that this task may block.
134+
// The scheduler will spawn a replacement worker if needed.
135+
tokio::task::block_in_place(move || {
136+
// Inside block_in_place we create a minimal one-shot
137+
// runtime to drive the factory future to completion.
138+
// This is unavoidable because the factory returns an
139+
// async future — there is no synchronous variant. The
140+
// key difference from the previous approach is that
141+
// `block_in_place` tells the outer runtime to lend us
142+
// this worker thread rather than permanently stealing it.
143+
let rt = tokio::runtime::Builder::new_current_thread()
144+
.enable_all()
145+
.build()
146+
.expect("BlockingPool: failed to build one-shot runtime");
147+
rt.block_on(shared_factory.build(ctx_clone))
148+
})
132149
})
133150
}
134151
crate::spec::child::Isolation::AsyncWorker => tokio::spawn(factory.build(ctx)),

src/config/configurable.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,4 +211,10 @@ pub struct DashboardRegistrationConfig {
211211
pub lease_seconds: Option<u64>,
212212
/// Registration heartbeat interval in seconds.
213213
pub registration_heartbeat_interval_seconds: Option<u64>,
214+
/// Timeout in seconds for connecting to the relay registration socket.
215+
/// Default: 5.
216+
pub registration_connect_timeout_secs: Option<u64>,
217+
/// Timeout in seconds for write and ack-read on the registration socket.
218+
/// Default: 5.
219+
pub registration_io_timeout_secs: Option<u64>,
214220
}

src/config/loader.rs

Lines changed: 3 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
//! YAML configuration loader backed by `rust-config-tree` format handling.
22
//!
33
//! This module keeps parsing and validation centralized so runtime modules never
4-
//! invent local defaults.
4+
//! invent local defaults. The root file format is auto-detected by
5+
//! `rust-config-tree` — no hard-coded format check is needed.
56
67
use crate::config::configurable::SupervisorConfig;
78
use crate::config::state::ConfigState;
@@ -13,7 +14,7 @@ use std::path::Path;
1314
///
1415
/// # Arguments
1516
///
16-
/// - `path`: Path to the root YAML configuration file.
17+
/// - `path`: Path to the root configuration file.
1718
///
1819
/// # Returns
1920
///
@@ -28,8 +29,6 @@ use std::path::Path;
2829
/// assert!(state.is_ok());
2930
/// ```
3031
pub fn load_config_from_yaml_file(path: impl AsRef<Path>) -> Result<ConfigState, SupervisorError> {
31-
ensure_yaml_format(path.as_ref())?;
32-
3332
// Use rust-config-tree to resolve include directives and merge
3433
// multiple YAML files. This ensures the `include: [..]` field
3534
// in SupervisorConfig is consumed per the README design principle.
@@ -39,23 +38,3 @@ pub fn load_config_from_yaml_file(path: impl AsRef<Path>) -> Result<ConfigState,
3938

4039
ConfigState::try_from(config)
4140
}
42-
43-
/// Ensures the root file is treated as YAML by `rust-config-tree`.
44-
///
45-
/// # Arguments
46-
///
47-
/// - `path`: Configuration path whose extension should be checked.
48-
///
49-
/// # Returns
50-
///
51-
/// Returns `Ok(())` when `rust-config-tree` selects YAML.
52-
fn ensure_yaml_format(path: &Path) -> Result<(), SupervisorError> {
53-
let format = rust_config_tree::ConfigFormat::from_path(path);
54-
if format == rust_config_tree::ConfigFormat::Yaml {
55-
Ok(())
56-
} else {
57-
Err(SupervisorError::fatal_config(
58-
"supervisor configuration must use YAML",
59-
))
60-
}
61-
}

src/config/state.rs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -928,6 +928,10 @@ fn dashboard_with_default_security(
928928
/// # Returns
929929
///
930930
/// Returns `Ok(())` when IPC is absent, disabled, or semantically valid.
931+
/// Validates dashboard IPC configuration when the dashboard module is
932+
/// compiled (Unix). On non-Unix platforms, rejects any configuration that
933+
/// has `dashboard.enabled = true`.
934+
#[cfg(unix)]
931935
fn validate_dashboard(
932936
dashboard: Option<&DashboardIpcConfig>,
933937
) -> Result<(), crate::error::types::SupervisorError> {
@@ -936,6 +940,23 @@ fn validate_dashboard(
936940
.map_err(|error| crate::error::types::SupervisorError::fatal_config(error.to_string()))
937941
}
938942

943+
/// On non-Unix platforms the dashboard module is not compiled. If the
944+
/// user explicitly set `dashboard.enabled = true`, report a clear error
945+
/// instead of silently ignoring the configuration.
946+
#[cfg(not(unix))]
947+
fn validate_dashboard(
948+
dashboard: Option<&DashboardIpcConfig>,
949+
) -> Result<(), crate::error::types::SupervisorError> {
950+
if let Some(config) = dashboard {
951+
if config.enabled {
952+
return Err(crate::error::types::SupervisorError::fatal_config(
953+
"dashboard is enabled but the dashboard IPC module is only available on Unix platforms",
954+
));
955+
}
956+
}
957+
Ok(())
958+
}
959+
939960
/// Validates that a runtime configuration number is positive.
940961
///
941962
/// # Arguments

src/control/handle.rs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -383,9 +383,17 @@ impl SupervisorHandle {
383383
///
384384
/// # Returns
385385
///
386-
/// Returns the cached [`RuntimeExitReport`].
386+
/// Returns the cached [`RuntimeExitReport`]. If the underlying
387+
/// `control_plane.join()` does not complete within 60 seconds, a
388+
/// timeout error is returned to prevent infinite hangs caused by
389+
/// shutdown pipeline bugs.
387390
pub async fn join(&self) -> Result<RuntimeExitReport, SupervisorError> {
388-
let report = self.control_plane.join().await;
391+
let join_deadline = std::time::Duration::from_secs(60);
392+
let report = tokio::time::timeout(join_deadline, self.control_plane.join())
393+
.await
394+
.map_err(|_elapsed| SupervisorError::FatalConfig {
395+
message: "control plane join timed out after 60 seconds".to_owned(),
396+
})?;
389397
let _ignored = self.event_sender.send(format!(
390398
"runtime_control_loop_join_completed:{}:{}:{}",
391399
report.state.as_str(),

src/dashboard/config.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,10 @@ pub struct ValidatedDashboardRegistrationConfig {
3535
pub lease_seconds: u64,
3636
/// Registration heartbeat interval in seconds.
3737
pub registration_heartbeat_interval_seconds: u64,
38+
/// Timeout in seconds for connecting to the relay registration socket.
39+
pub registration_connect_timeout_secs: u64,
40+
/// Timeout in seconds for write and ack-read on the registration socket.
41+
pub registration_io_timeout_secs: u64,
3842
}
3943

4044
/// Validates optional dashboard IPC configuration.
@@ -152,6 +156,22 @@ fn validate_registration(
152156
"dashboard.registration.registration_heartbeat_interval_seconds must be positive and less than lease_seconds",
153157
));
154158
}
159+
let connect_timeout = registration.registration_connect_timeout_secs.unwrap_or(5);
160+
if connect_timeout == 0 {
161+
return Err(DashboardError::validation(
162+
"config",
163+
Some(target_id.to_owned()),
164+
"dashboard.registration.registration_connect_timeout_secs must be greater than zero",
165+
));
166+
}
167+
let io_timeout = registration.registration_io_timeout_secs.unwrap_or(5);
168+
if io_timeout == 0 {
169+
return Err(DashboardError::validation(
170+
"config",
171+
Some(target_id.to_owned()),
172+
"dashboard.registration.registration_io_timeout_secs must be greater than zero",
173+
));
174+
}
155175
Ok(Some(ValidatedDashboardRegistrationConfig {
156176
relay_registration_path,
157177
display_name: registration
@@ -161,6 +181,8 @@ fn validate_registration(
161181
.unwrap_or_else(|| target_id.to_owned()),
162182
lease_seconds,
163183
registration_heartbeat_interval_seconds: heartbeat_seconds,
184+
registration_connect_timeout_secs: connect_timeout,
185+
registration_io_timeout_secs: io_timeout,
164186
}))
165187
}
166188

0 commit comments

Comments
 (0)