From 790762fb8ddb7c517e66adfaa8da02311280f2ac Mon Sep 17 00:00:00 2001 From: Chad Woolley Date: Wed, 8 Jul 2026 09:38:38 -0700 Subject: [PATCH] feat(config): make the per-node checkpoint commit timeout configurable (#552) ## Problem The post-node run-branch checkpoint commit runs repository commit hooks unless `skip_git_hooks` is enabled, but its sandbox command timeout was hardcoded to 30 seconds. Consumers whose hooks run a multi-minute gate cannot complete a checkpoint. ## Change Adds `commit_timeout_ms` to the existing `[run.checkpoint]` table. - Defaults to `30000`, preserving existing behavior. - Threads the value through config raw layer -> merge -> resolve -> resolved settings -> `RunOptions` -> `GitState` -> both checkpoint call sites. - Applies the configured timeout to checkpoint `git add -A` and `git commit`. - Keeps old serialized run manifests compatible via serde default. ## Testing - `cargo +nightly-2026-04-14 fmt --all` - `cargo +nightly-2026-04-14 clippy --locked --workspace --all-targets -- -D warnings` - `cargo nextest run --locked -p fabro-config -p fabro-types -p fabro-workflow` - 1795 passed, 31 skipped - `cargo nextest run --locked -p fabro-cli attach_json_errors_without_prompting_for_human_input` - `cargo nextest run --locked --workspace --status-level slow --profile ci --no-fail-fast` - 6951 passed, 3 timed out, 187 skipped - The 3 timeouts are preexisting on clean `upstream/main`: verified by running `CARGO_TARGET_DIR=/data/projects/fabro/target cargo nextest run --locked -p fabro-cli --profile ci --no-fail-fast workflow::acp::acp` from a detached worktree at `upstream/main` (`8c7d5dc7d`), which timed out the same three tests: - `workflow::acp::acp_artifacts_are_listed_when_touched_file_mtime_precedes_attempt_start` - `workflow::acp::acp_backend_does_not_inject_registered_provider_credentials` - `workflow::acp::acp_backend_workflow` ## Compatibility No behavior change without explicit opt-in. Omitted config resolves to the existing 30 second timeout, and old serialized run manifests deserialize unchanged. --------- Co-authored-by: thewoolleyman Co-authored-by: Bryan Helmkamp Co-authored-by: Claude Fable 5 --- .../administration/server-configuration.mdx | 4 +- docs/public/execution/run-configuration.mdx | 4 +- lib/crates/fabro-cli/tests/it/cmd/attach.rs | 1 + lib/crates/fabro-cli/tests/it/cmd/inspect.rs | 3 +- lib/crates/fabro-config/src/layers/combine.rs | 2 + lib/crates/fabro-config/src/layers/run.rs | 3 + lib/crates/fabro-config/src/resolve/run.rs | 20 ++-- .../fabro-config/src/tests/resolve_run.rs | 56 ++++++++- .../fabro-types/src/settings/duration.rs | 6 + lib/crates/fabro-types/src/settings/run.rs | 33 +++++- .../fabro-workflow/src/handler/parallel.rs | 20 ++-- .../fabro-workflow/src/lifecycle/git.rs | 3 +- .../fabro-workflow/src/pipeline/execute.rs | 3 +- lib/crates/fabro-workflow/src/run_options.rs | 10 +- lib/crates/fabro-workflow/src/sandbox_git.rs | 108 ++++++++++++------ 15 files changed, 202 insertions(+), 74 deletions(-) diff --git a/docs/public/administration/server-configuration.mdx b/docs/public/administration/server-configuration.mdx index b4f00c5e1..4a2f7d69e 100644 --- a/docs/public/administration/server-configuration.mdx +++ b/docs/public/administration/server-configuration.mdx @@ -112,6 +112,7 @@ team = "platform" [run.checkpoint] exclude_globs = ["**/node_modules/**", "**/.cache/**"] skip_git_hooks = false +commit_timeout = "30s" [run.inputs] default_branch = "main" @@ -329,8 +330,9 @@ Configure checkpoint behavior for all runs. |---|---| | `exclude_globs` | Glob patterns for files to exclude from checkpoint commits (for example, `["**/node_modules/**"]`) | | `skip_git_hooks` | When `true`, Fabro-managed run-branch checkpoint commits bypass local Git commit hooks. Defaults to `false`. Does not affect Fabro workflow `[[run.hooks]]` or metadata-branch snapshots. | +| `commit_timeout` | Max duration for the per-node run-branch checkpoint commit (e.g. `"30s"`, `"10m"`). This commit runs repository commit hooks unless `skip_git_hooks` is `true`. Defaults to `"30s"`. | -`exclude_globs` replaces across layers — the highest-precedence layer wins wholesale. `skip_git_hooks` uses normal override semantics. See [Run Configuration — Checkpoint](/execution/run-configuration#runcheckpoint) for per-run configuration. +`exclude_globs` replaces across layers — the highest-precedence layer wins wholesale. `skip_git_hooks` and `commit_timeout` use normal override semantics. See [Run Configuration — Checkpoint](/execution/run-configuration#runcheckpoint) for per-run configuration. ## Secrets and environment variables diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index 9c3cd8487..1b51a9161 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -356,14 +356,16 @@ Configure how git checkpoint commits behave. [run.checkpoint] exclude_globs = ["**/node_modules/**", "**/.cache/**", "**/dist/**"] skip_git_hooks = false +commit_timeout = "30s" ``` | Field | Description | |---|---| | `exclude_globs` | Glob patterns for files to exclude from checkpoint commits. Uses git pathspec `:(glob,exclude)` syntax. | | `skip_git_hooks` | When `true`, Fabro-managed run-branch checkpoint commits bypass local Git commit hooks (e.g. `pre-commit`, `commit-msg`). Defaults to `false`. Does not affect Fabro workflow `[[run.hooks]]` or metadata-branch snapshots. | +| `commit_timeout` | Max duration for the per-node run-branch checkpoint commit (e.g. `"30s"`, `"10m"`). This commit runs repository commit hooks unless `skip_git_hooks` is `true`. Defaults to `"30s"`. | -`exclude_globs` replaces across layers — the higher-precedence layer wins wholesale. `skip_git_hooks` uses normal override semantics: the highest layer that sets it wins. +`exclude_globs` replaces across layers — the higher-precedence layer wins wholesale. `skip_git_hooks` and `commit_timeout` use normal override semantics: the highest layer that sets the field wins. ### `[run.inputs]` diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index 6e7a6a051..327eab2d2 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -927,6 +927,7 @@ fn attach_json_errors_without_prompting_for_human_input() { "include": [] }, "checkpoint": { + "commit_timeout_ms": 30000, "exclude_globs": [], "skip_git_hooks": false }, diff --git a/lib/crates/fabro-cli/tests/it/cmd/inspect.rs b/lib/crates/fabro-cli/tests/it/cmd/inspect.rs index fef015390..086db6b32 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/inspect.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/inspect.rs @@ -148,7 +148,8 @@ fn inspect_resolves_selector_via_server_endpoint() { }, "checkpoint": { "exclude_globs": [], - "skip_git_hooks": false + "skip_git_hooks": false, + "commit_timeout_ms": 30000 }, "clone": { "enabled": true diff --git a/lib/crates/fabro-config/src/layers/combine.rs b/lib/crates/fabro-config/src/layers/combine.rs index 820c07906..bcadbfc75 100644 --- a/lib/crates/fabro-config/src/layers/combine.rs +++ b/lib/crates/fabro-config/src/layers/combine.rs @@ -168,9 +168,11 @@ impl Combine for RunCheckpointLayer { self.exclude_globs }; let skip_git_hooks = self.skip_git_hooks.or(other.skip_git_hooks); + let commit_timeout = self.commit_timeout.or(other.commit_timeout); Self { exclude_globs, skip_git_hooks, + commit_timeout, } } } diff --git a/lib/crates/fabro-config/src/layers/run.rs b/lib/crates/fabro-config/src/layers/run.rs index da4fb0d07..655b79640 100644 --- a/lib/crates/fabro-config/src/layers/run.rs +++ b/lib/crates/fabro-config/src/layers/run.rs @@ -286,6 +286,9 @@ pub struct RunCheckpointLayer { pub exclude_globs: Vec, #[serde(default, skip_serializing_if = "Option::is_none")] pub skip_git_hooks: Option, + /// Optional timeout for the per-node run-branch checkpoint commit. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub commit_timeout: Option, } /// `[run.clone]` — source workspace clone policy. diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index 55e883371..46287d85d 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -202,9 +202,9 @@ fn resolve_prepare( RunPrepareSettings { steps, - timeout_ms: prepare.timeout.map_or(300_000, |timeout| { - u64::try_from(timeout.as_std().as_millis()).unwrap_or(u64::MAX) - }), + timeout_ms: prepare + .timeout + .map_or(300_000, |timeout| timeout.as_millis()), } } @@ -223,12 +223,18 @@ fn resolve_execution(execution: Option<&RunExecutionLayer>) -> RunExecutionSetti fn resolve_checkpoint(checkpoint: Option<&RunCheckpointLayer>) -> RunCheckpointSettings { RunCheckpointSettings { - exclude_globs: checkpoint + exclude_globs: checkpoint .map(|checkpoint| checkpoint.exclude_globs.clone()) .unwrap_or_default(), - skip_git_hooks: checkpoint + skip_git_hooks: checkpoint .and_then(|checkpoint| checkpoint.skip_git_hooks) .unwrap_or(false), + commit_timeout_ms: checkpoint + .and_then(|checkpoint| checkpoint.commit_timeout) + .map_or( + RunCheckpointSettings::DEFAULT_COMMIT_TIMEOUT_MS, + |timeout| timeout.as_millis(), + ), } } @@ -514,9 +520,7 @@ fn resolve_hook(hook: &HookEntry, index: usize, errors: &mut Vec) hook_type, matcher: hook.matcher.clone(), blocking: hook.blocking, - timeout_ms: hook - .timeout - .map(|timeout| u64::try_from(timeout.as_std().as_millis()).unwrap_or(u64::MAX)), + timeout_ms: hook.timeout.map(|timeout| timeout.as_millis()), sandbox: hook.sandbox, } } diff --git a/lib/crates/fabro-config/src/tests/resolve_run.rs b/lib/crates/fabro-config/src/tests/resolve_run.rs index cb754659a..3abf6d580 100644 --- a/lib/crates/fabro-config/src/tests/resolve_run.rs +++ b/lib/crates/fabro-config/src/tests/resolve_run.rs @@ -917,8 +917,8 @@ fabro_tools = true } } -mod run_checkpoint_skip_git_hooks { - //! Layer + resolver tests for `[run.checkpoint] skip_git_hooks`. +mod run_checkpoint { + //! Layer + resolver tests for `[run.checkpoint]`. use crate::SettingsLayer; use crate::layers::Combine; @@ -954,6 +954,31 @@ skip_git_hooks = true assert!(!settings.checkpoint.skip_git_hooks); } + #[test] + fn resolves_commit_timeout_default_when_omitted() { + let settings = super::workflow_settings_from_layer(SettingsLayer::default()) + .expect("empty settings should resolve") + .run; + + assert_eq!(settings.checkpoint.commit_timeout_ms, 30_000); + } + + #[test] + fn resolves_commit_timeout_when_set() { + let settings = super::workflow_settings_from_toml( + r#" +_version = 1 + +[run.checkpoint] +commit_timeout = "10m" +"#, + ) + .expect("settings should resolve") + .run; + + assert_eq!(settings.checkpoint.commit_timeout_ms, 600_000); + } + #[test] fn higher_layer_false_overrides_lower_layer_true() { let workflow = parse_settings( @@ -981,6 +1006,33 @@ skip_git_hooks = true assert!(!settings.checkpoint.skip_git_hooks); } + #[test] + fn higher_layer_commit_timeout_overrides_lower_layer() { + let workflow = parse_settings( + r#" +_version = 1 + +[run.checkpoint] +commit_timeout = "10m" +"#, + ); + let user = parse_settings( + r#" +_version = 1 + +[run.checkpoint] +commit_timeout = "30s" +"#, + ); + let merged = workflow.combine(user); + + let settings = super::workflow_settings_from_layer(merged) + .expect("merged settings should resolve") + .run; + + assert_eq!(settings.checkpoint.commit_timeout_ms, 600_000); + } + #[test] fn exclude_globs_replace_behavior_preserved_when_skip_git_hooks_added() { // Higher layer provides skip_git_hooks but no exclude_globs; diff --git a/lib/crates/fabro-types/src/settings/duration.rs b/lib/crates/fabro-types/src/settings/duration.rs index 17d64c12b..f8efdcef7 100644 --- a/lib/crates/fabro-types/src/settings/duration.rs +++ b/lib/crates/fabro-types/src/settings/duration.rs @@ -35,6 +35,12 @@ impl Duration { pub const fn from_millis(millis: u64) -> Self { Self(StdDuration::from_millis(millis)) } + + /// Total milliseconds, saturating at `u64::MAX`. + #[must_use] + pub fn as_millis(&self) -> u64 { + u64::try_from(self.0.as_millis()).unwrap_or(u64::MAX) + } } impl From for Duration { diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index 6d00f7e2b..6befaf457 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -541,8 +541,9 @@ mod run_namespace_variable_substitution_tests { fn substitutes_variables_in_string_backed_settings_families() { let mut run = RunNamespace { checkpoint: RunCheckpointSettings { - exclude_globs: vec!["tmp/{{ vars.ENV }}/**".to_string()], - skip_git_hooks: false, + exclude_globs: vec!["tmp/{{ vars.ENV }}/**".to_string()], + skip_git_hooks: false, + commit_timeout_ms: 30_000, }, environment: RunEnvironmentSettings { image: EnvironmentImageSettings { @@ -845,15 +846,37 @@ impl Default for RunExecutionSettings { } } -#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RunCheckpointSettings { - pub exclude_globs: Vec, + pub exclude_globs: Vec, /// When `true`, Fabro-managed run-branch checkpoint commits bypass /// local Git commit hooks (e.g. `pre-commit`, `commit-msg`). This does /// not affect Fabro workflow `[[run.hooks]]` or metadata-branch /// snapshots, which already bypass repository hooks. #[serde(default)] - pub skip_git_hooks: bool, + pub skip_git_hooks: bool, + /// Timeout (ms) for the per-node run-branch checkpoint commit, which runs + /// repository commit hooks unless `skip_git_hooks` is set. Default 30_000. + #[serde(default = "default_checkpoint_commit_timeout_ms")] + pub commit_timeout_ms: u64, +} + +impl RunCheckpointSettings { + pub const DEFAULT_COMMIT_TIMEOUT_MS: u64 = 30_000; +} + +fn default_checkpoint_commit_timeout_ms() -> u64 { + RunCheckpointSettings::DEFAULT_COMMIT_TIMEOUT_MS +} + +impl Default for RunCheckpointSettings { + fn default() -> Self { + Self { + exclude_globs: Vec::new(), + skip_git_hooks: false, + commit_timeout_ms: default_checkpoint_commit_timeout_ms(), + } + } } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/lib/crates/fabro-workflow/src/handler/parallel.rs b/lib/crates/fabro-workflow/src/handler/parallel.rs index 108a72369..22aa2aa9c 100644 --- a/lib/crates/fabro-workflow/src/handler/parallel.rs +++ b/lib/crates/fabro-workflow/src/handler/parallel.rs @@ -193,9 +193,8 @@ impl Handler for ParallelHandler { "parallel_base", 0, None, - &gs.checkpoint_exclude_globs, + &gs.checkpoint, &gs.git_author, - gs.checkpoint_skip_git_hooks, ) .await; match result { @@ -320,9 +319,10 @@ impl Handler for ParallelHandler { .as_ref() .map(|gs| gs.git_author.clone()) .unwrap_or_default(); - let skip_git_hooks = git_state + let checkpoint = git_state .as_ref() - .is_some_and(|gs| gs.checkpoint_skip_git_hooks); + .map(|gs| gs.checkpoint.clone()) + .unwrap_or_default(); let group_id = parallel_group_id.clone(); let branch_scope = StageScope::for_parallel_branch( setup.target_id.clone(), @@ -407,7 +407,7 @@ impl Handler for ParallelHandler { let add_cmd = format!("{git_r} add -A"); let add_result = setup .sandbox - .exec_command(&add_cmd, 30_000, None, None, None) + .exec_command(&add_cmd, checkpoint.commit_timeout_ms, None, None, None) .await; if add_result .as_ref() @@ -419,11 +419,17 @@ impl Handler for ParallelHandler { &git_author.name, &git_author.email, &msg, - skip_git_hooks, + checkpoint.skip_git_hooks, ); let _ = setup .sandbox - .exec_command(&commit_cmd, 30_000, None, None, None) + .exec_command( + &commit_cmd, + checkpoint.commit_timeout_ms, + None, + None, + None, + ) .await; } let sha_cmd = format!("{git_r} rev-parse HEAD"); diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index 5133fa602..a1aab9bc7 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -280,9 +280,8 @@ impl RunLifecycle for GitLifecycle { &result.outcome.status.to_string(), completed_count, shadow_sha, - &self.run_options.checkpoint_exclude_globs(), + self.run_options.checkpoint(), &git_author, - self.run_options.checkpoint_skip_git_hooks(), ) .await; diff --git a/lib/crates/fabro-workflow/src/pipeline/execute.rs b/lib/crates/fabro-workflow/src/pipeline/execute.rs index 4a82a509c..0be068e88 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute.rs @@ -62,8 +62,7 @@ pub async fn execute(init: Initialized) -> Executed { base_sha, run_branch: git.run_branch.clone(), meta_branch: git.meta_branch.clone(), - checkpoint_exclude_globs: run_options.checkpoint_exclude_globs(), - checkpoint_skip_git_hooks: run_options.checkpoint_skip_git_hooks(), + checkpoint: run_options.checkpoint().clone(), git_author: run_options.git_author(), })) }); diff --git a/lib/crates/fabro-workflow/src/run_options.rs b/lib/crates/fabro-workflow/src/run_options.rs index e3817e5f0..2f84d21e8 100644 --- a/lib/crates/fabro-workflow/src/run_options.rs +++ b/lib/crates/fabro-workflow/src/run_options.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use std::path::PathBuf; -use fabro_types::settings::run::RunMode; +use fabro_types::settings::run::{RunCheckpointSettings, RunMode}; use fabro_types::{ForkSourceRef, GitContext, RunId, WorkflowSettings}; use tokio_util::sync::CancellationToken; @@ -50,12 +50,8 @@ impl RunOptions { self.settings.run.execution.mode == RunMode::DryRun } - pub fn checkpoint_exclude_globs(&self) -> Vec { - self.settings.run.checkpoint.exclude_globs.clone() - } - - pub fn checkpoint_skip_git_hooks(&self) -> bool { - self.settings.run.checkpoint.skip_git_hooks + pub fn checkpoint(&self) -> &RunCheckpointSettings { + &self.settings.run.checkpoint } pub fn git_author(&self) -> GitAuthor { diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index b746e2f89..68b7e1bed 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -5,6 +5,7 @@ use fabro_checkpoint::trailer as trailerlink; use fabro_checkpoint::trailer::Trailer; use fabro_sandbox::shell_quote; use fabro_types::RunId; +use fabro_types::settings::run::RunCheckpointSettings; use fabro_util::error::SharedError; use crate::artifact_snapshot; @@ -22,13 +23,12 @@ pub struct GitCommandError { /// Captured git state for a workflow run, shared with handlers. #[derive(Debug, Clone)] pub struct GitState { - pub run_id: RunId, - pub base_sha: String, - pub run_branch: Option, - pub meta_branch: Option, - pub checkpoint_exclude_globs: Vec, - pub checkpoint_skip_git_hooks: bool, - pub git_author: GitAuthor, + pub run_id: RunId, + pub base_sha: String, + pub run_branch: Option, + pub meta_branch: Option, + pub checkpoint: RunCheckpointSettings, + pub git_author: GitAuthor, } pub const GIT_REMOTE: &str = @@ -58,7 +58,7 @@ pub(crate) fn exec_err(label: &str, r: fabro_sandbox::ExecResult) -> GitCommandE /// Run a git checkpoint commit via the sandbox. #[allow( clippy::too_many_arguments, - reason = "Checkpointing needs explicit run metadata, excludes, and author inputs." + reason = "Checkpointing needs explicit run metadata, checkpoint settings, and author inputs." )] pub async fn git_checkpoint( sandbox: &dyn Sandbox, @@ -67,15 +67,14 @@ pub async fn git_checkpoint( status: &str, completed_count: usize, shadow_sha: Option, - exclude_globs: &[String], + checkpoint: &RunCheckpointSettings, author: &GitAuthor, - skip_git_hooks: bool, ) -> std::result::Result { let mut all_excludes: Vec = artifact_snapshot::EXCLUDE_DIRS .iter() .map(|d| format!("**/{d}/**")) .collect(); - all_excludes.extend(exclude_globs.iter().cloned()); + all_excludes.extend(checkpoint.exclude_globs.iter().cloned()); let pathspecs: Vec = all_excludes .iter() @@ -83,7 +82,7 @@ pub async fn git_checkpoint( .collect(); let add_cmd = format!("{GIT_REMOTE} add -A -- . {}", pathspecs.join(" ")); let add_result = sandbox - .exec_command(&add_cmd, 30_000, None, None, None) + .exec_command(&add_cmd, checkpoint.commit_timeout_ms, None, None, None) .await; match add_result { Ok(r) if r.is_success() => {} @@ -127,14 +126,18 @@ pub async fn git_checkpoint( } let msg_path_q = shell_quote(&msg_path); - let no_verify = if skip_git_hooks { " --no-verify" } else { "" }; + let no_verify = if checkpoint.skip_git_hooks { + " --no-verify" + } else { + "" + }; let commit_cmd = format!( "{GIT_REMOTE} -c user.name={name} -c user.email={email} commit --allow-empty{no_verify} -F {msg_path_q}", name = shell_quote(&author.name), email = shell_quote(&author.email), ); let commit_result = sandbox - .exec_command(&commit_cmd, 30_000, None, None, None) + .exec_command(&commit_cmd, checkpoint.commit_timeout_ms, None, None, None) .await; let _ = sandbox.delete_file(&msg_path).await; match commit_result { @@ -165,7 +168,7 @@ pub async fn git_checkpoint( /// Run a git checkpoint after the per-run sandbox git capability probe. #[allow( clippy::too_many_arguments, - reason = "Checkpointing needs explicit run metadata, excludes, and author inputs." + reason = "Checkpointing needs explicit run metadata, checkpoint settings, and author inputs." )] pub(crate) async fn checked_git_checkpoint( runtime: &SandboxGitRuntime, @@ -175,9 +178,8 @@ pub(crate) async fn checked_git_checkpoint( status: &str, completed_count: usize, shadow_sha: Option, - exclude_globs: &[String], + checkpoint: &RunCheckpointSettings, author: &GitAuthor, - skip_git_hooks: bool, ) -> std::result::Result { runtime.ensure_git_available(sandbox).await.map_err(|err| { SharedError::new(anyhow::Error::new(err).context("sandbox git unavailable")) @@ -189,9 +191,8 @@ pub(crate) async fn checked_git_checkpoint( status, completed_count, shadow_sha, - exclude_globs, + checkpoint, author, - skip_git_hooks, ) .await .map_err(|err| SharedError::new(anyhow::Error::new(err))) @@ -877,6 +878,7 @@ mod tests { struct ScriptedSandbox { exec_results: Mutex>, commands: Mutex>, + timeouts: Mutex>, write_paths: Mutex>, delete_paths: Mutex>, } @@ -886,6 +888,7 @@ mod tests { Self { exec_results: Mutex::new(exec_results.into()), commands: Mutex::new(Vec::new()), + timeouts: Mutex::new(Vec::new()), write_paths: Mutex::new(Vec::new()), delete_paths: Mutex::new(Vec::new()), } @@ -898,6 +901,13 @@ mod tests { .clone() } + fn timeouts(&self) -> Vec { + self.timeouts + .lock() + .expect("timeouts lock poisoned") + .clone() + } + fn write_paths(&self) -> Vec { self.write_paths .lock() @@ -950,7 +960,7 @@ mod tests { async fn exec_command( &self, command: &str, - _timeout_ms: u64, + timeout_ms: u64, _working_dir: Option<&str>, _env_vars: Option<&std::collections::HashMap>, _cancel_token: Option, @@ -959,6 +969,10 @@ mod tests { .lock() .expect("commands lock poisoned") .push(command.to_string()); + self.timeouts + .lock() + .expect("timeouts lock poisoned") + .push(timeout_ms); self.exec_results .lock() .expect("exec_results lock poisoned") @@ -1066,9 +1080,8 @@ mod tests { "success", 1, None, - &[], + &RunCheckpointSettings::default(), &crate::git::GitAuthor::default(), - false, ) .await .unwrap_err(); @@ -1093,9 +1106,8 @@ mod tests { "success", 1, None, - &[], + &RunCheckpointSettings::default(), &crate::git::GitAuthor::default(), - false, ) .await .unwrap_err(); @@ -1124,9 +1136,8 @@ mod tests { "success", 1, None, - &[], + &RunCheckpointSettings::default(), &crate::git::GitAuthor::default(), - false, ) .await .unwrap_err(); @@ -1144,9 +1155,8 @@ mod tests { "success", 1, None, - &[], + &RunCheckpointSettings::default(), &crate::git::GitAuthor::default(), - false, ) .await .unwrap_err(); @@ -1173,9 +1183,8 @@ mod tests { "success", 1, None, - &[], + &RunCheckpointSettings::default(), &author, - false, ) .await; let second = git_checkpoint( @@ -1185,9 +1194,8 @@ mod tests { "success", 1, None, - &[], + &RunCheckpointSettings::default(), &author, - false, ) .await; @@ -1225,6 +1233,29 @@ mod tests { } } + #[tokio::test] + async fn git_checkpoint_uses_configured_timeout_for_add_and_commit() { + let sandbox = ScriptedSandbox::new(vec![exec_ok(), exec_ok(), exec_ok()]); + let checkpoint = RunCheckpointSettings { + commit_timeout_ms: 600_000, + ..RunCheckpointSettings::default() + }; + git_checkpoint( + &sandbox, + "run1", + "work", + "success", + 1, + None, + &checkpoint, + &crate::git::GitAuthor::default(), + ) + .await + .expect("checkpoint should succeed"); + + assert_eq!(sandbox.timeouts(), vec![600_000, 600_000, 10_000]); + } + #[tokio::test] async fn git_diff_reports_timeout() { let sandbox = ScriptedSandbox::new(vec![exec_timed_out(99)]); @@ -1253,6 +1284,10 @@ mod tests { async fn git_checkpoint_appends_no_verify_when_skip_hooks_enabled() { // add, commit, rev-parse let sandbox = ScriptedSandbox::new(vec![exec_ok(), exec_ok(), exec_ok()]); + let checkpoint = RunCheckpointSettings { + skip_git_hooks: true, + ..RunCheckpointSettings::default() + }; git_checkpoint( &sandbox, "run1", @@ -1260,9 +1295,8 @@ mod tests { "success", 1, None, - &[], + &checkpoint, &crate::git::GitAuthor::default(), - true, ) .await .expect("checkpoint should succeed"); @@ -1288,9 +1322,8 @@ mod tests { "success", 1, None, - &[], + &RunCheckpointSettings::default(), &crate::git::GitAuthor::default(), - false, ) .await .expect("checkpoint should succeed"); @@ -1350,9 +1383,8 @@ mod tests { "success", 1, None, - &[], + &RunCheckpointSettings::default(), &author, - false, ) .await; assert!(result.is_ok(), "git_checkpoint failed: {:?}", result.err());