Skip to content

Commit f25f68d

Browse files
matdexirmatdexir
authored
Make cleanup wait timeout configurable in LocalWorkerConfig (TraceMachina#2456)
- Add max_cleanup_wait (default: 30s) and max_cleanup_backoff (default: 500ms) fields to LocalWorkerConfig - Replace hardcoded MAX_WAIT and MAX_BACKOFF constants with configurable instance fields in RunningActionsManagerImpl - Update local_worker.rs to pass new fields from config to RunningActionsManagerArgs - Update tests with new fields - Update documentation for new config fields
1 parent 414246b commit f25f68d

5 files changed

Lines changed: 106 additions & 7 deletions

File tree

‎nativelink-config/src/cas_server.rs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -778,6 +778,20 @@ pub struct LocalWorkerConfig {
778778
#[serde(default, deserialize_with = "convert_duration_with_shellexpand")]
779779
pub max_upload_timeout: usize,
780780

781+
/// Maximum time to wait for action directory cleanup before timing out.
782+
/// Value in seconds.
783+
///
784+
/// Default: 30 seconds
785+
#[serde(default, deserialize_with = "convert_duration_with_shellexpand")]
786+
pub max_cleanup_wait_s: usize,
787+
788+
/// Maximum backoff duration for exponential backoff when waiting for cleanup.
789+
/// Value in milliseconds.
790+
///
791+
/// Default: 500 milliseconds
792+
#[serde(default, deserialize_with = "convert_duration_with_shellexpand")]
793+
pub max_cleanup_backoff_ms: usize,
794+
781795
/// Maximum number of inflight tasks this worker can cope with.
782796
///
783797
/// Default: 0 (infinite tasks)

‎nativelink-worker/src/local_worker.rs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,8 @@ const DEFAULT_ENDPOINT_TIMEOUT_S: f32 = 5.;
7373
/// If this value gets modified the documentation in `cas_server.rs` must also be updated.
7474
const DEFAULT_MAX_ACTION_TIMEOUT: Duration = Duration::from_mins(20);
7575
const DEFAULT_MAX_UPLOAD_TIMEOUT: Duration = Duration::from_mins(10);
76+
const DEFAULT_MAX_CLEANUP_WAIT: Duration = Duration::from_secs(30);
77+
const DEFAULT_MAX_CLEANUP_BACKOFF: Duration = Duration::from_millis(500);
7678

7779
struct FinishedActionResult {
7880
action_result: ActionResult,
@@ -593,6 +595,16 @@ pub async fn new_local_worker(
593595
} else {
594596
Duration::from_secs(config.max_upload_timeout as u64)
595597
};
598+
let max_cleanup_wait = if config.max_cleanup_wait_s == 0 {
599+
DEFAULT_MAX_CLEANUP_WAIT
600+
} else {
601+
Duration::from_secs(config.max_cleanup_wait_s as u64)
602+
};
603+
let max_cleanup_backoff = if config.max_cleanup_backoff_ms == 0 {
604+
DEFAULT_MAX_CLEANUP_BACKOFF
605+
} else {
606+
Duration::from_millis(config.max_cleanup_backoff_ms as u64)
607+
};
596608

597609
// Initialize directory cache if configured
598610
let directory_cache = if let Some(cache_config) = &config.directory_cache {
@@ -690,6 +702,8 @@ pub async fn new_local_worker(
690702
upload_action_result_config: &config.upload_action_result,
691703
max_action_timeout,
692704
max_upload_timeout,
705+
max_cleanup_wait,
706+
max_cleanup_backoff,
693707
timeout_handled_externally: config.timeout_handled_externally,
694708
directory_cache,
695709
#[cfg(target_os = "linux")]

‎nativelink-worker/src/running_actions_manager.rs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2560,6 +2560,8 @@ pub struct RunningActionsManagerArgs<'a> {
25602560
pub upload_action_result_config: &'a UploadActionResultConfig,
25612561
pub max_action_timeout: Duration,
25622562
pub max_upload_timeout: Duration,
2563+
pub max_cleanup_wait: Duration,
2564+
pub max_cleanup_backoff: Duration,
25632565
pub timeout_handled_externally: bool,
25642566
pub directory_cache: Option<Arc<crate::directory_cache::DirectoryCache>>,
25652567
#[cfg(target_os = "linux")]
@@ -2607,6 +2609,8 @@ pub struct RunningActionsManagerImpl {
26072609
/// attempt's directory is fully cleaned up before creating a new one.
26082610
/// See: <https://github.com/TraceMachina/nativelink/issues/1859>
26092611
cleaning_up_operations: Mutex<HashSet<OperationId>>,
2612+
max_cleanup_wait: Duration,
2613+
max_cleanup_backoff: Duration,
26102614
/// Notify waiters when a cleanup operation completes. This is used in conjunction with
26112615
/// `cleaning_up_operations` to coordinate directory cleanup and creation.
26122616
cleanup_complete_notify: Arc<Notify>,
@@ -2617,11 +2621,6 @@ pub struct RunningActionsManagerImpl {
26172621
}
26182622

26192623
impl RunningActionsManagerImpl {
2620-
/// Maximum time to wait for a cleanup operation to complete before timing out.
2621-
/// TODO(marcussorealheis): Consider making cleanup wait timeout configurable in the future
2622-
const MAX_WAIT: Duration = Duration::from_secs(30);
2623-
/// Maximum backoff duration for exponential backoff when waiting for cleanup.
2624-
const MAX_BACKOFF: Duration = Duration::from_millis(500);
26252624
pub fn new_with_callbacks(
26262625
args: RunningActionsManagerArgs<'_>,
26272626
callbacks: Callbacks,
@@ -2656,6 +2655,8 @@ impl RunningActionsManagerImpl {
26562655
callbacks,
26572656
metrics: Arc::new(Metrics::default()),
26582657
cleaning_up_operations: Mutex::new(HashSet::new()),
2658+
max_cleanup_wait: args.max_cleanup_wait,
2659+
max_cleanup_backoff: args.max_cleanup_backoff,
26592660
cleanup_complete_notify: Arc::new(Notify::new()),
26602661
directory_cache: args.directory_cache,
26612662
persistent_worker_pool: PersistentWorkerPool::default(),
@@ -2741,7 +2742,7 @@ impl RunningActionsManagerImpl {
27412742
return Ok(());
27422743
}
27432744

2744-
if start.elapsed() > Self::MAX_WAIT {
2745+
if start.elapsed() > self.max_cleanup_wait {
27452746
self.metrics.cleanup_wait_timeouts.inc();
27462747
warn!(%operation_id, waited=?start.elapsed(), "Timeout waiting for previous operation cleanup");
27472748
return Err(make_err!(
@@ -2768,7 +2769,7 @@ impl RunningActionsManagerImpl {
27682769
() = self.cleanup_complete_notify.notified() => {},
27692770
() = tokio::time::sleep(backoff) => {
27702771
// Exponential backoff
2771-
backoff = (backoff * 2).min(Self::MAX_BACKOFF);
2772+
backoff = (backoff * 2).min(self.max_cleanup_backoff);
27722773
},
27732774
}
27742775
}

0 commit comments

Comments
 (0)