From 6dfeeca45e1feb21a96e71174d19656bcc86b795 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 4 Aug 2026 15:01:19 -0400 Subject: [PATCH 01/19] perf(agent): skip Daytona folder request for edits --- lib/components/fabro-agent/src/tools.rs | 4 +- .../fabro-sandbox/src/daytona/mod.rs | 84 ++++++++++++++++--- lib/components/fabro-sandbox/src/sandbox.rs | 10 +++ .../fabro-sandbox/src/test_support.rs | 14 +++- 4 files changed, 100 insertions(+), 12 deletions(-) diff --git a/lib/components/fabro-agent/src/tools.rs b/lib/components/fabro-agent/src/tools.rs index f892c971c..aad8e4e3d 100644 --- a/lib/components/fabro-agent/src/tools.rs +++ b/lib/components/fabro-agent/src/tools.rs @@ -226,7 +226,7 @@ pub fn make_edit_file_tool() -> RegisteredTool { }; ctx.env - .write_file(file_path, &new_content) + .write_existing_file(file_path, &new_content) .await .map_err(|e| e.display_with_causes())?; Ok(format!("Successfully edited {file_path}")) @@ -1002,6 +1002,7 @@ mod tests { ) .await; assert_eq!(result.unwrap(), "Successfully wrote to /out.txt"); + assert_eq!(env.existing_file_write_count(), 0); let written = env.written_files.lock().unwrap(); assert_eq!(written.len(), 1); assert_eq!(written[0].0, "/out.txt"); @@ -1036,6 +1037,7 @@ mod tests { ) .await; assert_eq!(result.unwrap(), "Successfully edited /f.txt"); + assert_eq!(env.existing_file_write_count(), 1); let written = env.written_files.lock().unwrap(); assert_eq!(written.len(), 1); assert_eq!(written[0].1, "goodbye world"); diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 6371f23d2..72b0d2fff 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -511,6 +511,19 @@ impl DaytonaSandbox { resolve_path(path, self.working_directory()) } + async fn upload_file_content(&self, resolved_path: &str, content: &str) -> crate::Result<()> { + let sandbox = self.sandbox()?; + let fs_svc = sandbox + .fs() + .await + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; + + fs_svc + .upload_file_bytes(resolved_path, content.as_bytes()) + .await + .map_err(|e| crate::Error::context(format!("Failed to write file {resolved_path}"), e)) + } + /// Verify a Daytona sandbox evaluates commands as non-login Bash. /// /// Runs on a freshly created sandbox before any Fabro-owned setup, and @@ -1613,17 +1626,12 @@ impl Sandbox for DaytonaSandbox { } } - let fs_svc = sandbox - .fs() - .await - .map_err(|e| crate::Error::context("Failed to get fs service", e))?; + self.upload_file_content(&resolved, content).await + } - fs_svc - .upload_file_bytes(&resolved, content.as_bytes()) - .await - .map_err(|e| crate::Error::context(format!("Failed to write file {resolved}"), e))?; - - Ok(()) + async fn write_existing_file(&self, path: &str, content: &str) -> crate::Result<()> { + let resolved = self.resolve_path(path); + self.upload_file_content(&resolved, content).await } async fn delete_file(&self, path: &str) -> crate::Result<()> { @@ -3432,6 +3440,62 @@ mod tests { delete.assert_async().await; } + #[tokio::test] + async fn write_existing_file_skips_parent_directory_creation() { + let server = MockServer::start_async().await; + let server_url = server.base_url(); + let sandbox_response = server + .mock_async(|when, then| { + when.method(GET).path("/sandbox/sandbox-edit"); + then.status(200) + .header("content-type", "application/json") + .json_body(sandbox_body("sandbox-edit", SandboxState::Started)); + }) + .await; + let toolbox_response = server + .mock_async(|when, then| { + when.method(GET) + .path("/sandbox/sandbox-edit/toolbox-proxy-url"); + then.status(200) + .header("content-type", "application/json") + .json_body(serde_json::json!({"url": server_url})); + }) + .await; + let folder = server + .mock_async(|when, then| { + when.method(POST).path("/sandbox-edit/files/folder"); + then.status(200); + }) + .await; + let upload = server + .mock_async(|when, then| { + when.method(POST) + .path("/sandbox-edit/files/upload") + .query_param("path", "/home/daytona/workspace/src/lib.rs") + .body_includes("updated contents"); + then.status(200); + }) + .await; + + let sandbox = mock_daytona_sandbox(&server, "dtn_test", DaytonaConfig::default()).await; + let sdk_sandbox = sandbox + .client + .get("sandbox-edit") + .await + .expect("get mock sandbox"); + assert!(sandbox.sandbox.set(sdk_sandbox).is_ok()); + + sandbox + .write_existing_file("src/lib.rs", "updated contents") + .await + .expect("write existing file"); + + sandbox_response.assert_async().await; + toolbox_response.assert_async().await; + upload.assert_async().await; + folder.assert_calls_async(0).await; + } + /// Recover the inner command a wrapper carries, proving it survives the /// base64 transport byte-for-byte. fn decode_wrapped_command(wrapped: &str) -> String { diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index 6e6e2b336..31a873300 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -1048,6 +1048,16 @@ pub trait Sandbox: Send + Sync { } async fn write_file(&self, path: &str, content: &str) -> crate::Result<()>; + + /// Write a file that the caller has already confirmed exists. + /// + /// Providers can override this method to skip setup that is only needed + /// when creating a new path. The default preserves the behavior of + /// [`Sandbox::write_file`]. + async fn write_existing_file(&self, path: &str, content: &str) -> crate::Result<()> { + self.write_file(path, content).await + } + async fn delete_file(&self, path: &str) -> crate::Result<()>; async fn file_exists(&self, path: &str) -> crate::Result; async fn list_directory( diff --git a/lib/components/fabro-sandbox/src/test_support.rs b/lib/components/fabro-sandbox/src/test_support.rs index 7364dc8b0..d25d9bbf4 100644 --- a/lib/components/fabro-sandbox/src/test_support.rs +++ b/lib/components/fabro-sandbox/src/test_support.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; use std::sync::Mutex; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::time::Duration; use async_trait::async_trait; @@ -29,6 +29,8 @@ pub struct MockSandbox { pub os_version_str: String, /// Captures (path, content) pairs from `write_file` calls. pub written_files: Mutex>, + /// Counts calls to `write_existing_file`. + pub existing_file_writes: AtomicUsize, /// Captures the `timeout_ms` argument from `exec_command` calls. pub captured_timeout: Mutex>, /// Captures the `command` argument from `exec_command` calls (last only). @@ -104,6 +106,10 @@ impl MockSandbox { .expect("delete_calls lock poisoned") } + pub fn existing_file_write_count(&self) -> usize { + self.existing_file_writes.load(Ordering::Relaxed) + } + pub fn set_stdio_process(&self, process: MockStdioProcess) { *self .stdio_process @@ -156,6 +162,7 @@ impl Default for MockSandbox { platform_str: "darwin", os_version_str: "Darwin 24.0.0".into(), written_files: Mutex::new(Vec::new()), + existing_file_writes: AtomicUsize::new(0), captured_timeout: Mutex::new(None), captured_command: Mutex::new(None), captured_commands: Mutex::new(Vec::new()), @@ -250,6 +257,11 @@ impl Sandbox for MockSandbox { Ok(()) } + async fn write_existing_file(&self, path: &str, content: &str) -> crate::Result<()> { + self.existing_file_writes.fetch_add(1, Ordering::Relaxed); + self.write_file(path, content).await + } + async fn delete_file(&self, _path: &str) -> crate::Result<()> { Ok(()) } From e11d268e30f8ce9a161328b4bd4a4b5ad21d980b Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 5 Aug 2026 08:43:41 -0400 Subject: [PATCH 02/19] Bound doctor diagnostics within client timeout --- docs/public/api-reference/fabro-api.yaml | 9 ++ lib/apps/fabro-server/src/diagnostics.rs | 118 +++++++++++++++--- .../fabro-server/src/server/handler/system.rs | 59 ++++++++- 3 files changed, 168 insertions(+), 18 deletions(-) diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 261badbbb..4382d19fa 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -594,6 +594,15 @@ paths: application/json: schema: $ref: "#/components/schemas/DiagnosticsReport" + "504": + description: Diagnostics operation timed out + headers: + x-request-id: + $ref: "#/components/headers/XRequestId" + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" /api/v1/openapi.json: get: diff --git a/lib/apps/fabro-server/src/diagnostics.rs b/lib/apps/fabro-server/src/diagnostics.rs index 988f84404..86f625366 100644 --- a/lib/apps/fabro-server/src/diagnostics.rs +++ b/lib/apps/fabro-server/src/diagnostics.rs @@ -6,7 +6,7 @@ use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use fabro_auth::auth_issue_message; use fabro_llm::client::Client as LlmClient; -use fabro_llm::model_test::{ModelTestStatus, run_basic_model_probe}; +use fabro_llm::model_test::{ModelTestOutcome, ModelTestStatus, run_basic_model_probe}; use fabro_model::{Catalog, ProviderId}; use fabro_redact::redact_string; use fabro_sandbox::{DockerSandboxProvider, daytona}; @@ -23,6 +23,9 @@ use tokio::time::timeout; use crate::server::AppState; +const EXTERNAL_SERVICE_PROBE_TIMEOUT: Duration = Duration::from_secs(15); +const DOCKER_PROBE_TIMEOUT: Duration = Duration::from_secs(5); + fn http_client_or_check( name: &str, status: CheckStatus, @@ -254,11 +257,38 @@ async fn probe_single_provider( }; let model_id = model.id.clone(); - let outcome = run_basic_model_probe(model_id.as_str(), &provider, client).await; + let outcome = run_basic_model_probe(model_id.as_str(), provider.clone(), client); + provider_probe_with_timeout( + provider, + model_id.to_string(), + outcome, + EXTERNAL_SERVICE_PROBE_TIMEOUT, + ) + .await +} + +async fn provider_probe_with_timeout( + provider: ProviderId, + model_id: String, + probe: F, + probe_timeout: Duration, +) -> ProviderProbeResult +where + F: Future, +{ + let Ok(outcome) = timeout(probe_timeout, probe).await else { + return provider_probe_error( + provider, + Some(model_id), + probe_timeout_message(probe_timeout), + None, + ); + }; + match outcome.status { ModelTestStatus::Ok => ProviderProbeResult { provider, - model_id: Some(model_id.to_string()), + model_id: Some(model_id), status: ProviderProbeStatus::Ok, error_message: None, diagnostic_detail: None, @@ -267,16 +297,19 @@ async fn probe_single_provider( let raw = outcome .error_message .unwrap_or_else(|| "provider probe failed".to_string()); - provider_probe_error( - provider, - Some(model_id.to_string()), - redact_string(&raw), - None, - ) + provider_probe_error(provider, Some(model_id), redact_string(&raw), None) } } } +fn probe_timeout_message(probe_timeout: Duration) -> String { + if probe_timeout.subsec_nanos() == 0 { + format!("timeout ({}s)", probe_timeout.as_secs()) + } else { + format!("timeout ({}ms)", probe_timeout.as_millis()) + } +} + fn provider_probe_error( provider: ProviderId, model_id: Option, @@ -388,7 +421,7 @@ async fn check_github_app(state: &AppState) -> CheckResult { Err(result) => return result, }; let probe = timeout( - Duration::from_secs(15), + EXTERNAL_SERVICE_PROBE_TIMEOUT, http.get(format!("{}/user", fabro_github::github_api_base_url())) .header("Authorization", format!("Bearer {token}")) .header("Accept", "application/vnd.github+json") @@ -538,7 +571,7 @@ async fn check_github_app(state: &AppState) -> CheckResult { Err(result) => return result, }; let auth_result = timeout( - Duration::from_secs(15), + EXTERNAL_SERVICE_PROBE_TIMEOUT, fabro_github::get_authenticated_app(&http, &jwt, &fabro_github::github_api_base_url()), ) .await; @@ -581,7 +614,7 @@ async fn check_docker_sandbox(state: &AppState) -> CheckResult { .await .map_err(|err| err.display_with_causes()) }, - Duration::from_secs(5), + DOCKER_PROBE_TIMEOUT, ) .await } @@ -656,7 +689,29 @@ async fn check_cloud_sandbox(state: &AppState) -> CheckResult { }; }; - match state.check_daytona_api_key(api_key).await { + check_cloud_sandbox_with_probe( + || state.check_daytona_api_key(api_key), + EXTERNAL_SERVICE_PROBE_TIMEOUT, + ) + .await +} + +async fn check_cloud_sandbox_with_probe(probe: F, probe_timeout: Duration) -> CheckResult +where + F: FnOnce() -> Fut, + Fut: Future>, +{ + let Ok(probe) = timeout(probe_timeout, probe()).await else { + return CheckResult { + name: "Cloud Sandbox".to_string(), + status: CheckStatus::Error, + summary: probe_timeout_message(probe_timeout), + details: vec![CheckDetail::new("Daytona probe timed out".to_string())], + remediation: Some("Verify DAYTONA_API_KEY value and Daytona reachability".to_string()), + }; + }; + + match probe { Ok(check) if check.ok() => CheckResult { name: "Cloud Sandbox".to_string(), status: CheckStatus::Pass, @@ -750,7 +805,7 @@ async fn check_brave_search(state: &AppState) -> CheckResult { Err(result) => return result, }; - let probe = timeout(Duration::from_secs(15), async move { + let probe = timeout(EXTERNAL_SERVICE_PROBE_TIMEOUT, async move { http.get("https://api.search.brave.com/res/v1/web/search?q=test&count=1") .header("X-Subscription-Token", api_key) .send() @@ -1035,6 +1090,27 @@ mod tests { ); } + #[tokio::test] + async fn provider_probe_reports_provider_specific_timeout() { + assert_eq!( + probe_timeout_message(EXTERNAL_SERVICE_PROBE_TIMEOUT), + "timeout (15s)" + ); + + let result = provider_probe_with_timeout( + ProviderId::new("modal"), + "modal/test-model".to_string(), + std::future::pending::(), + Duration::from_millis(1), + ) + .await; + + assert_eq!(result.provider, ProviderId::new("modal")); + assert_eq!(result.model_id.as_deref(), Some("modal/test-model")); + assert_eq!(result.status, ProviderProbeStatus::Error); + assert_eq!(result.error_message.as_deref(), Some("timeout (1ms)")); + } + #[test] fn docker_sandbox_probe_passes_when_daemon_responds() { let result = docker_sandbox_probe_check(Ok(())); @@ -1154,6 +1230,20 @@ enabled = false ); } + #[tokio::test] + async fn check_cloud_sandbox_reports_timeout() { + let result = check_cloud_sandbox_with_probe( + std::future::pending::>, + Duration::from_millis(1), + ) + .await; + + assert_eq!(result.name, "Cloud Sandbox"); + assert_eq!(result.status, CheckStatus::Error); + assert_eq!(result.summary, "timeout (1ms)"); + assert_eq!(result.details[0].text, "Daytona probe timed out"); + } + #[tokio::test] async fn check_brave_search_ignores_env_backed_api_key() { let state = TestAppStateBuilder::new() diff --git a/lib/apps/fabro-server/src/server/handler/system.rs b/lib/apps/fabro-server/src/server/handler/system.rs index d26e445a6..a38fe33f5 100644 --- a/lib/apps/fabro-server/src/server/handler/system.rs +++ b/lib/apps/fabro-server/src/server/handler/system.rs @@ -1,5 +1,7 @@ use std::collections::BTreeMap; +use std::future::Future; use std::sync::Arc; +use std::time::Duration; use chrono::Utc; use fabro_slack::config::{ @@ -9,6 +11,7 @@ use fabro_slack::config::{ use fabro_static::EnvVars; use fabro_types::settings::server::GithubIntegrationSettings; use fabro_vault::Vault; +use tokio::time::timeout; use super::super::{ AggregateBilling, AggregateBillingTotals, ApiError, AppState, BilledTokenCounts, @@ -21,6 +24,8 @@ use super::super::{ resource_sampler, spawn_blocking, system_sandbox_provider, to_i64, }; +const SERVER_DIAGNOSTICS_TIMEOUT: Duration = Duration::from_secs(25); + pub(super) fn routes() -> Router> { Router::new() .route("/repos/github/{owner}/{name}", get(get_github_repo)) @@ -683,11 +688,34 @@ async fn get_github_repo( } async fn run_diagnostics(_auth: RequiredUser, State(state): State>) -> Response { - ( - StatusCode::OK, - Json(diagnostics::run_all(state.as_ref()).await), + diagnostics_response_with_timeout( + Box::pin(diagnostics::run_all(state.as_ref())), + SERVER_DIAGNOSTICS_TIMEOUT, ) - .into_response() + .await +} + +async fn diagnostics_response_with_timeout( + diagnostics: F, + operation_timeout: Duration, +) -> Response +where + F: Future, +{ + let Ok(report) = timeout(operation_timeout, diagnostics).await else { + tracing::warn!( + timeout_secs = operation_timeout.as_secs(), + "server diagnostics timed out" + ); + return ApiError::with_code( + StatusCode::GATEWAY_TIMEOUT, + "Server diagnostics timed out.", + "diagnostics_timeout", + ) + .into_response(); + }; + + (StatusCode::OK, Json(report)).into_response() } pub(in crate::server) async fn openapi_spec() -> Response { @@ -737,3 +765,26 @@ async fn get_aggregate_billing( }; (StatusCode::OK, Json(response)).into_response() } + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn diagnostics_response_returns_gateway_timeout_before_client_deadline() { + let response = diagnostics_response_with_timeout( + std::future::pending::(), + Duration::from_millis(1), + ) + .await; + + assert_eq!(response.status(), StatusCode::GATEWAY_TIMEOUT); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .expect("diagnostics timeout response body should be readable"); + let body: serde_json::Value = serde_json::from_slice(&body) + .expect("diagnostics timeout response should contain JSON"); + assert_eq!(body["errors"][0]["code"], "diagnostics_timeout"); + assert_eq!(body["errors"][0]["detail"], "Server diagnostics timed out."); + } +} From f6932529faed7d758efd6d6bb8dbd08576223ffe Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 5 Aug 2026 21:01:53 -0400 Subject: [PATCH 03/19] Let a node execute max_visits times before the cycle guard fires The executor incremented a node's visit count on entry and refused the visit once the count reached the limit, so a node with max_visits=N executed at most N-1 times. The documented contract in stages-and-nodes.mdx is "Max times this node can execute in a run", and both published examples describe bounded retry loops under that reading. A graph with max_visits=2 on a designed one-correction loop therefore failed as "stuck in a cycle" before the correction could run. Check the completed-visit count before entry instead: a node with max_visits=N now executes exactly N times, and the refused entry is not reported as a visit, so the error's count names the executions that actually happened. Also correct the nlspec example prose, which claimed the workflow "moves on with the best result" at the limit; exceeding max_visits fails the run. Co-Authored-By: Claude Fable 5 --- docs/public/examples/nlspec-conformance.mdx | 2 +- lib/foundation/fabro-core/src/executor.rs | 26 ++++++++++++++++----- lib/foundation/fabro-core/src/state.rs | 4 ++++ 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/docs/public/examples/nlspec-conformance.mdx b/docs/public/examples/nlspec-conformance.mdx index cfeb30fec..6a1161769 100644 --- a/docs/public/examples/nlspec-conformance.mdx +++ b/docs/public/examples/nlspec-conformance.mdx @@ -123,7 +123,7 @@ Do not rewrite working code. Make targeted fixes to the specific failures. ### Max visits as a safety valve -`max_visits=5` on the `fix` node prevents infinite loops. If the agent can't pass in 5 iterations, the workflow moves on with the best result so far. Tune this based on spec complexity: a 30-line spec might need 2 iterations, a 2,000-line spec might need 10. +`max_visits=5` on the `fix` node prevents infinite loops. The node can execute up to 5 times; a sixth visit fails the run rather than looping forever. Tune this based on spec complexity: a 30-line spec might need 2 iterations, a 2,000-line spec might need 10. ### Goal gate on full conformance diff --git a/lib/foundation/fabro-core/src/executor.rs b/lib/foundation/fabro-core/src/executor.rs index 3a94eb332..f6e3a6aa3 100644 --- a/lib/foundation/fabro-core/src/executor.rs +++ b/lib/foundation/fabro-core/src/executor.rs @@ -179,8 +179,11 @@ impl Executor { } } - // Check visit limits (>= matches fabro-workflow semantics) - let visits = state.increment_visits(node.id()); + // Check visit limits before entry: a node with a limit of N may + // execute N times, matching the documented contract. The count + // covers completed entries only, so the refused visit is not + // reported as one. + let visits = state.visits(node.id()); if let Some(max) = node.max_visits() { if visits >= max { return Err(Error::VisitLimitExceeded { @@ -201,6 +204,7 @@ impl Executor { }); } } + state.increment_visits(node.id()); // before_node lifecycle let node_result = match self.lifecycle.before_node(&node, &state).await? { @@ -807,7 +811,8 @@ mod tests { #[tokio::test] async fn executor_visit_limit_per_node() { - // Node with max_visits=2, loops back — fails on 2nd visit (>= semantics) + // Node with max_visits=2, loops back — executes exactly twice, then + // the third entry is refused. The error reports completed visits. let g = TestGraph::new( vec![ TestNode::new("loop_node").with_max_visits(2), @@ -821,11 +826,20 @@ mod tests { "loop_node", ); let state = ExecutionState::new(&g).unwrap(); + let handler = Arc::new(CountingHandler::new(vec![])); let executor = - ExecutorBuilder::new(Arc::new(AlwaysSucceedHandler) as Arc>) - .build(); + ExecutorBuilder::new(Arc::clone(&handler) as Arc>).build(); let result = executor.run(&g, state).await; - assert!(matches!(result, Err(Error::VisitLimitExceeded { .. }))); + match result { + Err(Error::VisitLimitExceeded { visits, limit, .. }) => { + assert_eq!(visits, 2); + assert_eq!(limit, 2); + } + Err(other) => panic!("expected VisitLimitExceeded, got {other:?}"), + Ok(_) => panic!("expected VisitLimitExceeded, got success"), + } + // Two full loop_node -> other iterations ran before the refusal. + assert_eq!(handler.calls(), 4); } #[tokio::test] diff --git a/lib/foundation/fabro-core/src/state.rs b/lib/foundation/fabro-core/src/state.rs index ca3b8e56b..c44eeeb7d 100644 --- a/lib/foundation/fabro-core/src/state.rs +++ b/lib/foundation/fabro-core/src/state.rs @@ -79,6 +79,10 @@ impl ExecutionState { graph.get_node(&self.current_node_id) } + pub fn visits(&self, node_id: &str) -> usize { + self.node_visits.get(node_id).copied().unwrap_or(0) + } + pub fn increment_visits(&mut self, node_id: &str) -> usize { let count = self.node_visits.entry(node_id.to_string()).or_insert(0); *count += 1; From a4db43a8892a38b625014fc642d9e9fc79a818f2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 6 Aug 2026 11:54:32 -0400 Subject: [PATCH 04/19] Report live billing totals for in-progress runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run summaries previously populated billing only from the terminal conclusion event, so the web UI's size chip showed dollar amounts only after a run completed — even though the size letter was already derived from live per-stage usage. Derive billing from the same projected total the size uses. projected_billing already prefers the conclusion's billing once a run concludes, so completed runs still report the authoritative final total. Co-Authored-By: Claude Fable 5 --- lib/components/fabro-store/src/run_state.rs | 26 +++++++++------------ 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/lib/components/fabro-store/src/run_state.rs b/lib/components/fabro-store/src/run_state.rs index 5e4ab5e37..720a76f0e 100644 --- a/lib/components/fabro-store/src/run_state.rs +++ b/lib/components/fabro-store/src/run_state.rs @@ -1360,8 +1360,7 @@ pub(crate) fn build_summary(state: &RunProjection, run_id: &RunId) -> Run { .conclusion .as_ref() .map(|conclusion| conclusion.timing); - let terminal_total = terminal_total_usd_micros(state); - let current_total = projected_billing(state).total_usd_micros; + let total_usd_micros = projected_billing(state).total_usd_micros; Run { id: *run_id, @@ -1405,10 +1404,10 @@ pub(crate) fn build_summary(state: &RunProjection, run_id: &RunId) -> Run { completed_at, }, timing: run_timing, - billing: terminal_total.map(|total_usd_micros| RunBillingSummary { + billing: total_usd_micros.map(|total_usd_micros| RunBillingSummary { total_usd_micros: Some(total_usd_micros), }), - size: RunSize::from_total_usd_micros(current_total), + size: RunSize::from_total_usd_micros(total_usd_micros), ask_fabro: AskFabro::default(), diff: diff_summary, pull_request: state.pull_request.clone(), @@ -1421,14 +1420,6 @@ pub(crate) fn build_summary(state: &RunProjection, run_id: &RunId) -> Run { } } -fn terminal_total_usd_micros(state: &RunProjection) -> Option { - state - .conclusion - .as_ref() - .and_then(|conclusion| conclusion.billing.as_ref()) - .and_then(|billing| billing.total_usd_micros) -} - pub(crate) fn projected_billing(state: &RunProjection) -> BilledTokenCounts { if let Some(billing) = state .conclusion @@ -1693,8 +1684,8 @@ mod tests { BilledTokenCounts, BlockedReason, Checkpoint, CheckpointRecord, CommandTermination, EventBody, FailureCategory, FailureDetail, FailureReason, Graph, McpServerStatus, Node, Outcome, ParallelBranchId, PendingReason, PermissionLevel, PullRequestCreationStatus, - PullRequestLink, QuestionType, ReasoningEffort, RunApprovalState, RunBlobId, - RunControlAction, RunDiff, RunEvent, RunSize, RunSpec, RunStatus, Speed, + PullRequestLink, QuestionType, ReasoningEffort, RunApprovalState, RunBillingSummary, + RunBlobId, RunControlAction, RunDiff, RunEvent, RunSize, RunSpec, RunStatus, Speed, StageContextWindowBreakdownItem, StageContextWindowCategory, StageContextWindowCountMethod, StageContextWindowProjection, StageContextWindowStaleness, StageContextWindowWarning, StageHandler, StageModelUsage, StageOutcome, StageState, StageTiming, SubAgentStatus, @@ -5284,7 +5275,12 @@ mod tests { let summary = build_summary(&state, &fixtures::RUN_1); assert_eq!(summary.size, RunSize::S); - assert_eq!(summary.billing, None); + assert_eq!( + summary.billing, + Some(RunBillingSummary { + total_usd_micros: Some(20_000_001), + }) + ); } #[test] From 57547ed7b6384be9e2096ff4b4f4c213adb48e8d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 6 Aug 2026 11:54:38 -0400 Subject: [PATCH 05/19] Populate repo and workflow filters in runs list view The Repo and Workflow dropdowns on the runs page derived their options from the board query, which is disabled in list view. With ?view=list, the options were always empty even when runs were visible. Derive the options from whichever data source the current view loads: the board query in columns view, or the current page of the paginated list query in list view. Extract the option-building into an exported buildFilterOptions helper that also keeps the active selection in the options when no loaded run matches it, so the filter button never renders an undefined label while paginating. A future change will replace page-derived options with a facets endpoint plus server-side repo/workflow query params. Co-Authored-By: Claude Fable 5 --- apps/fabro-web/app/routes/runs.test.tsx | 34 +++++++++++++++++++++++++ apps/fabro-web/app/routes/runs.tsx | 34 +++++++++++++++++-------- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/apps/fabro-web/app/routes/runs.test.tsx b/apps/fabro-web/app/routes/runs.test.tsx index 52196878b..71f30dd43 100644 --- a/apps/fabro-web/app/routes/runs.test.tsx +++ b/apps/fabro-web/app/routes/runs.test.tsx @@ -3,6 +3,7 @@ import type { BoardColumn, Run } from "@qltysh/fabro-api-client"; import { buildBoardColumns, + buildFilterOptions, loadStoredRunsWorkspaceSearchParams, placeArchivedColumnLast, persistRunsWorkspacePreferences, @@ -11,6 +12,7 @@ import { shouldRefreshBoardForEvent, } from "./runs"; import { summarizeBatchLifecycleAction } from "../components/runs-list/batch-lifecycle"; +import { mapRunListItem } from "../data/runs"; import { TEST_PRINCIPAL } from "../lib/test-fixtures"; function boardRun(id: string, column: BoardColumn, questionText?: string): Run { @@ -217,6 +219,38 @@ describe("runs route board mapping", () => { }); }); +describe("runs route filter options", () => { + function runWith(id: string, repoName: string, workflowName: string): Run { + const run = boardRun(id, "running"); + return { + ...run, + repository: { ...run.repository, name: repoName }, + workflow: { ...run.workflow, name: workflowName }, + }; + } + + test("derives sorted unique options from run items", () => { + const items = [ + runWith("a", "qlty/beta", "release"), + runWith("b", "qlty/alpha", "hello"), + runWith("c", "qlty/beta", "release"), + ].map(mapRunListItem); + + expect(buildFilterOptions(items, (item) => item.repo, "all")).toEqual(["alpha", "beta"]); + expect(buildFilterOptions(items, (item) => item.workflow, "all")).toEqual([ + "hello", + "release", + ]); + }); + + test("keeps the active selection when no loaded run matches it", () => { + const items = [runWith("a", "qlty/beta", "release")].map(mapRunListItem); + + expect(buildFilterOptions(items, (item) => item.repo, "gamma")).toEqual(["beta", "gamma"]); + expect(buildFilterOptions([], (item) => item.workflow, "release")).toEqual(["release"]); + }); +}); + describe("runs route workspace preferences", () => { class MemoryStorage { values = new Map(); diff --git a/apps/fabro-web/app/routes/runs.tsx b/apps/fabro-web/app/routes/runs.tsx index b1e09bf67..9aeceb78c 100644 --- a/apps/fabro-web/app/routes/runs.tsx +++ b/apps/fabro-web/app/routes/runs.tsx @@ -140,6 +140,18 @@ export function buildBoardColumns( }); } +export function buildFilterOptions( + items: RunItem[], + pick: (item: RunItem) => string, + selected: string, +): string[] { + const values = new Set(items.map(pick)); + // Keep the active selection visible even when no loaded run matches it, + // e.g. a stored repo filter while paginating the list view. + if (selected !== "all") values.add(selected); + return Array.from(values).sort(); +} + export function placeArchivedColumnLast(columns: Column[], includeArchived: boolean): Column[] { if (!includeArchived) return columns; const archived = columns.find((column) => column.id === "archived"); @@ -771,18 +783,18 @@ export default function Runs() { ); const hasGitHubAuth = authConfig.data?.methods.includes("github") === true; const serverUrl = systemInfo.data?.server_url; - const allRepos = Array.from( - new Set( - initialColumns.flatMap((col: Column) => col.items.map((item: RunItem) => String(item.repo))), - ), + // Filter options come from the loaded runs: all runs in columns view, the + // current page in list view (until a facets endpoint provides the full set). + const filterSourceItems: RunItem[] = + view === "list" + ? (listRunsPage.data?.data ?? []).map(mapRunListItem) + : initialColumns.flatMap((col: Column) => col.items); + const allRepos = buildFilterOptions(filterSourceItems, (item) => item.repo, repoFilter); + const allWorkflows = buildFilterOptions( + filterSourceItems, + (item) => item.workflow, + workflowFilter, ); - allRepos.sort(); - const allWorkflows = Array.from( - new Set( - initialColumns.flatMap((col: Column) => col.items.map((item: RunItem) => String(item.workflow))), - ), - ); - allWorkflows.sort(); const [columnsState, setColumnsState] = useState(() => ({ base: initialColumns, columns: initialColumns, From 4e24dcb68a49037ee64c087514eca4e0524f7ebe Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 6 Aug 2026 21:10:49 -0400 Subject: [PATCH 06/19] Add failing tests for run-spec redaction corruption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entropy redactor rewrites NAME= assignment pairs to a bare REDACTED, and the worker rehydrates its executable RunSpec from the projection folded from redacted stored events. Together these broke Daytona snapshot builds for any run definition whose inline Dockerfile pins a git SHA: the spec came back as `ARG REDACTED`, the build died on the unset variable under `set -eu`, and the environment's snapshot identity silently changed. Pin the intended contracts with red tests: - fabro-redact: an assignment whose value alone is below the entropy threshold survives redaction (pure hex cannot exceed 4.0 bits; only the name+value charset merge crosses 4.5), and a genuinely high-entropy value is redacted without destroying the key name. - fabro-workflow: the spec that load_from_store rehydrates round-trips byte-identical through the store, including content that looks like a secret — event redaction must not reach execution. Co-Authored-By: Claude Fable 5 --- .../fabro-workflow/src/pipeline/persist.rs | 44 +++++++++++++++++++ lib/foundation/fabro-redact/src/lib.rs | 21 +++++++++ 2 files changed, 65 insertions(+) diff --git a/lib/components/fabro-workflow/src/pipeline/persist.rs b/lib/components/fabro-workflow/src/pipeline/persist.rs index cc12c3cba..846f9b853 100644 --- a/lib/components/fabro-workflow/src/pipeline/persist.rs +++ b/lib/components/fabro-workflow/src/pipeline/persist.rs @@ -272,6 +272,50 @@ mod tests { assert!(loaded.diagnostics().is_empty()); } + #[tokio::test] + async fn load_from_store_preserves_high_entropy_dockerfile_content() { + // The spec the worker executes must survive the store byte-identical. + // Event redaction is a storage/display concern; when it reaches the + // spec that `load_from_store` rehydrates, the sandbox builds a + // corrupted Dockerfile: `ARG NAME=` pairs come back as + // `ARG REDACTED`, the build's `set -eu` step fails on the unset + // variable, and the environment's snapshot identity silently changes. + let temp = tempfile::tempdir().unwrap(); + let run_dir = temp.path().join("run"); + std::fs::create_dir_all(&run_dir).unwrap(); + let (graph, source) = graph_and_source(); + + // Two shapes that must both survive: the hex pins that triggered the + // production failure, and a token high-entropy enough that any + // detector will keep flagging it in stored events. The second keeps + // this test red until execution stops reading redacted content, + // independent of how the entropy heuristic evolves. + let dockerfile = "FROM buildpack-deps:noble\n\ + ARG DOCKER_INSTALL_COMMIT=5ce20f2eef3615d08fea941eda5a109e949e8ebf\n\ + ARG DOCKER_INSTALL_SHA256=b991f2806186f7287bb9e53362060c382e906d154599b2fb0982f34246bacfd4\n\ + ENV CACHE_SALT=xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6p\n\ + RUN install-docker \"${DOCKER_INSTALL_COMMIT}\" \"${DOCKER_INSTALL_SHA256}\"\n"; + + let mut record = sample_record(different_graph()); + record.graph = graph; + record.settings.run.environment.image.dockerfile = Some( + fabro_types::settings::run::DockerfileSource::Inline(dockerfile.to_string()), + ); + + let run_store = seeded_store(&record, Some(&source)).await; + let loaded = load_from_store(&run_store.clone().into(), &run_dir) + .await + .unwrap(); + + assert_eq!( + loaded.run_spec().settings.run.environment.image.dockerfile, + Some(fabro_types::settings::run::DockerfileSource::Inline( + dockerfile.to_string() + )), + "the executable run spec must round-trip through the store unredacted" + ); + } + #[test] fn persist_returns_error_on_io_failure() { let temp = tempfile::tempdir().unwrap(); diff --git a/lib/foundation/fabro-redact/src/lib.rs b/lib/foundation/fabro-redact/src/lib.rs index 8ed562e53..4a7efa45b 100644 --- a/lib/foundation/fabro-redact/src/lib.rs +++ b/lib/foundation/fabro-redact/src/lib.rs @@ -111,6 +111,27 @@ mod tests { assert_eq!(result, "key=REDACTED"); } + #[test] + fn redact_string_keeps_assignment_with_low_entropy_value() { + // A pinned git SHA is pure hex, so the value alone can never exceed + // 4.0 bits of entropy. Only the merged NAME=value token crosses the + // 4.5-bit threshold, because the uppercase name widens the charset. + // Measuring the name together with the value redacts innocuous + // pins; the pair must survive. + let input = "ARG DOCKER_INSTALL_COMMIT=5ce20f2eef3615d08fea941eda5a109e949e8ebf"; + assert_eq!(redact_string(input), input); + } + + #[test] + fn redact_string_keeps_assignment_key_for_high_entropy_value() { + // The value alone is above the entropy threshold, so it is + // redacted either way — but the name says which setting was + // redacted and must survive, as the gitleaks layer already + // does for `key=REDACTED`. + let result = redact_string("BUILD_STAMP=xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6p"); + assert_eq!(result, "BUILD_STAMP=REDACTED"); + } + #[test] fn redact_string_overlapping_detections_produce_single_redacted() { // A high-entropy string that also matches a gitleaks pattern From 3421c4f06fb77af09cc33b33bb57cbfbd226c752 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 6 Aug 2026 21:44:46 -0400 Subject: [PATCH 07/19] Keep the executable run spec out of reach of event redaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two root-cause fixes for the sandbox failure where an inline Dockerfile came back from the store as `ARG REDACTED` and the Daytona snapshot build died on the unset variable. Entropy redaction measures values, not assignment pairs. The detector matched `NAME=value` as one token, so an uppercase name merged its charset into a pure-hex value (which alone can never exceed 4.0 bits) and pushed the pair over the 4.5-bit threshold — then replaced the whole pair, destroying the name. `find_entropy_regions` now strips an identifier-shaped `NAME=` prefix before measuring and redacts only the value, matching the gitleaks layer's `key=REDACTED` shape. Execution no longer reads redacted content. Every stored event passes through the redaction sink, and `load_from_store` rehydrated the worker's RunSpec from the projection folded from those events — so a redactor false positive silently rewrote the spec the sandbox builds from (and changed its snapshot identity). The creation path now writes the exact spec bytes to the content-addressed blob store and records `spec_blob` on run.created; `load_from_store` loads the spec from the blob, keeping the event stream authoritative for run identity, provenance, and event-recorded blob ids. Retry and fork carry the source run's `spec_blob` forward, so derived runs stop inheriting the redacted copy. Runs created before the blob existed fall back to the folded spec. The projection and every API surface keep serving the redacted fold; blobs were already stored unredacted (the workflow bundle carries the same bytes), so this adds no new exposure at rest. Co-Authored-By: Claude Fable 5 --- docs/public/api-reference/fabro-api.yaml | 2 + lib/apps/fabro-cli/src/commands/run/attach.rs | 1 + lib/apps/fabro-cli/tests/it/cmd/attach.rs | 1 + lib/apps/fabro-cli/tests/it/support/mod.rs | 1 + lib/apps/fabro-server/src/run_files.rs | 1 + .../fabro-server/src/server/handler/events.rs | 1 + .../fabro-server/src/server/handler/pair.rs | 1 + .../src/server/handler/sessions.rs | 1 + lib/apps/fabro-server/src/server/tests.rs | 8 ++ .../fabro-server/tests/it/api/run_files.rs | 1 + lib/components/fabro-dump/src/lib.rs | 1 + lib/components/fabro-store/src/run_state.rs | 4 + .../fabro-store/src/run_summary_store.rs | 1 + lib/components/fabro-store/src/slate/mod.rs | 1 + .../tests/serializable_projection.rs | 1 + .../fabro-workflow/src/billing_rollup.rs | 1 + .../fabro-workflow/src/event/convert.rs | 3 + .../fabro-workflow/src/event/events.rs | 2 + .../fabro-workflow/src/event/sink.rs | 1 + lib/components/fabro-workflow/src/git.rs | 1 + .../fabro-workflow/src/handler/agent.rs | 1 + .../fabro-workflow/src/handler/command.rs | 2 + .../fabro-workflow/src/handler/parallel.rs | 1 + .../fabro-workflow/src/handler/prompt.rs | 1 + .../fabro-workflow/src/lifecycle/git.rs | 1 + .../fabro-workflow/src/operations/archive.rs | 1 + .../fabro-workflow/src/operations/create.rs | 8 ++ .../fabro-workflow/src/operations/fork.rs | 4 + .../fabro-workflow/src/operations/retry.rs | 5 + .../fabro-workflow/src/operations/timeline.rs | 1 + .../src/pipeline/execute/tests.rs | 2 + .../fabro-workflow/src/pipeline/finalize.rs | 2 + .../fabro-workflow/src/pipeline/initialize.rs | 2 + .../fabro-workflow/src/pipeline/persist.rs | 109 +++++++++++++++--- .../src/pipeline/pull_request.rs | 9 ++ .../fabro-workflow/src/run_lookup.rs | 2 + .../fabro-workflow/src/run_metadata.rs | 1 + .../fabro-workflow/src/runtime_store.rs | 2 + .../fabro-workflow/src/stage_execution.rs | 1 + .../fabro-workflow/src/test_support.rs | 1 + .../tests/run_projection_round_trip.rs | 1 + lib/foundation/fabro-redact/src/entropy.rs | 33 +++++- lib/foundation/fabro-redact/src/jsonl.rs | 4 +- lib/foundation/fabro-test/src/lib.rs | 4 + lib/foundation/fabro-types/src/run.rs | 5 + .../fabro-types/src/run_event/run.rs | 5 + .../fabro-types/src/run_projection.rs | 3 + .../fabro-types/tests/run_event_serde.rs | 2 + .../fabro-types/tests/run_spec_methods.rs | 1 + .../fabro-types/tests/run_spec_serde.rs | 1 + 50 files changed, 229 insertions(+), 20 deletions(-) diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 261badbbb..1875a9662 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -11304,6 +11304,8 @@ components: type: ["string", "null"] definition_blob: type: ["string", "null"] + spec_blob: + type: ["string", "null"] git: oneOf: - $ref: "#/components/schemas/GitContext" diff --git a/lib/apps/fabro-cli/src/commands/run/attach.rs b/lib/apps/fabro-cli/src/commands/run/attach.rs index cd356ffa0..7c1cf93b7 100644 --- a/lib/apps/fabro-cli/src/commands/run/attach.rs +++ b/lib/apps/fabro-cli/src/commands/run/attach.rs @@ -849,6 +849,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }; diff --git a/lib/apps/fabro-cli/tests/it/cmd/attach.rs b/lib/apps/fabro-cli/tests/it/cmd/attach.rs index 813bf7424..cc00e14a2 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/attach.rs @@ -1024,6 +1024,7 @@ fn attach_json_errors_without_prompting_for_human_input() { } }, "source_directory": "[TEMP_DIR]", + "spec_blob": "[BLOB_ID]", "title": "Wait for approval", "web_url": "http://localhost:3000/runs/[ULID]", "workflow_slug": "human-gate", diff --git a/lib/apps/fabro-cli/tests/it/support/mod.rs b/lib/apps/fabro-cli/tests/it/support/mod.rs index 2f3557044..57e4a98e5 100644 --- a/lib/apps/fabro-cli/tests/it/support/mod.rs +++ b/lib/apps/fabro-cli/tests/it/support/mod.rs @@ -53,6 +53,7 @@ pub(crate) fn run_projection_json(run_id: &str, status: &serde_json::Value) -> s provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }; diff --git a/lib/apps/fabro-server/src/run_files.rs b/lib/apps/fabro-server/src/run_files.rs index 920460f79..2343933b6 100644 --- a/lib/apps/fabro-server/src/run_files.rs +++ b/lib/apps/fabro-server/src/run_files.rs @@ -2387,6 +2387,7 @@ index 1111111..2222222 160000 provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }, diff --git a/lib/apps/fabro-server/src/server/handler/events.rs b/lib/apps/fabro-server/src/server/handler/events.rs index 1bfb7f889..5fd655f1d 100644 --- a/lib/apps/fabro-server/src/server/handler/events.rs +++ b/lib/apps/fabro-server/src/server/handler/events.rs @@ -627,6 +627,7 @@ mod stage_events_tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/apps/fabro-server/src/server/handler/pair.rs b/lib/apps/fabro-server/src/server/handler/pair.rs index ec31d2e54..6e244bcac 100644 --- a/lib/apps/fabro-server/src/server/handler/pair.rs +++ b/lib/apps/fabro-server/src/server/handler/pair.rs @@ -1027,6 +1027,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/apps/fabro-server/src/server/handler/sessions.rs b/lib/apps/fabro-server/src/server/handler/sessions.rs index c2ee94b32..e9bacdfff 100644 --- a/lib/apps/fabro-server/src/server/handler/sessions.rs +++ b/lib/apps/fabro-server/src/server/handler/sessions.rs @@ -1923,6 +1923,7 @@ reasoning = false provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }; diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index a05443a00..4278eeec5 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -4650,6 +4650,7 @@ async fn append_default_run_created(run_store: &fabro_store::RunDatabase, run_id automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, @@ -4701,6 +4702,7 @@ async fn create_slack_notification_run( automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, @@ -5774,6 +5776,7 @@ async fn list_run_stages_distinguishes_visits() { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, @@ -5910,6 +5913,7 @@ async fn list_run_stages_exposes_execution_identity_for_resumed_stage() { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, @@ -7097,6 +7101,7 @@ async fn create_completed_run_ready_for_pull_request( provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, }; @@ -7113,6 +7118,7 @@ async fn create_completed_run_ready_for_pull_request( automation: None, provenance: run_spec.provenance.clone(), manifest_blob: None, + spec_blob: None, git, fork_source_ref: None, retried_from: None, @@ -14082,6 +14088,7 @@ async fn create_preserved_local_sandbox_run(state: &Arc, run_id: RunId automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, @@ -14831,6 +14838,7 @@ async fn delete_run_retry_after_missing_provider_resource_removes_metadata() { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/apps/fabro-server/tests/it/api/run_files.rs b/lib/apps/fabro-server/tests/it/api/run_files.rs index 6d286cbab..307c4a574 100644 --- a/lib/apps/fabro-server/tests/it/api/run_files.rs +++ b/lib/apps/fabro-server/tests/it/api/run_files.rs @@ -68,6 +68,7 @@ async fn append_completed_run_with_final_patch( automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-dump/src/lib.rs b/lib/components/fabro-dump/src/lib.rs index 1b3a10a4c..39c73e1d8 100644 --- a/lib/components/fabro-dump/src/lib.rs +++ b/lib/components/fabro-dump/src/lib.rs @@ -501,6 +501,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, } } diff --git a/lib/components/fabro-store/src/run_state.rs b/lib/components/fabro-store/src/run_state.rs index 5e4ab5e37..dedcb0ef5 100644 --- a/lib/components/fabro-store/src/run_state.rs +++ b/lib/components/fabro-store/src/run_state.rs @@ -1046,6 +1046,7 @@ fn projection_from_created(event: &EventEnvelope) -> Result { provenance: props.provenance.clone(), manifest_blob: props.manifest_blob, definition_blob: None, + spec_blob: props.spec_blob, git: props.git.clone(), fork_source_ref: props.fork_source_ref.clone(), }; @@ -2292,6 +2293,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, } @@ -4098,6 +4100,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, }; @@ -4124,6 +4127,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, }; diff --git a/lib/components/fabro-store/src/run_summary_store.rs b/lib/components/fabro-store/src/run_summary_store.rs index 5db1a49b1..dcbec847e 100644 --- a/lib/components/fabro-store/src/run_summary_store.rs +++ b/lib/components/fabro-store/src/run_summary_store.rs @@ -601,6 +601,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }, diff --git a/lib/components/fabro-store/src/slate/mod.rs b/lib/components/fabro-store/src/slate/mod.rs index 5d9ae3bd9..04be6ba1f 100644 --- a/lib/components/fabro-store/src/slate/mod.rs +++ b/lib/components/fabro-store/src/slate/mod.rs @@ -602,6 +602,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: Some(fabro_types::GitContext { origin_url: "https://github.com/fabro-sh/fabro".to_string(), branch: "main".to_string(), diff --git a/lib/components/fabro-store/tests/serializable_projection.rs b/lib/components/fabro-store/tests/serializable_projection.rs index ef0ed067b..4e03e1782 100644 --- a/lib/components/fabro-store/tests/serializable_projection.rs +++ b/lib/components/fabro-store/tests/serializable_projection.rs @@ -25,6 +25,7 @@ fn sample_run_spec() -> RunSpec { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: Some(fabro_types::GitContext { origin_url: "https://github.com/fabro-sh/fabro.git".to_string(), branch: "main".to_string(), diff --git a/lib/components/fabro-workflow/src/billing_rollup.rs b/lib/components/fabro-workflow/src/billing_rollup.rs index 986b541d4..ed26de465 100644 --- a/lib/components/fabro-workflow/src/billing_rollup.rs +++ b/lib/components/fabro-workflow/src/billing_rollup.rs @@ -322,6 +322,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, } diff --git a/lib/components/fabro-workflow/src/event/convert.rs b/lib/components/fabro-workflow/src/event/convert.rs index 6403d8933..ef255d5b8 100644 --- a/lib/components/fabro-workflow/src/event/convert.rs +++ b/lib/components/fabro-workflow/src/event/convert.rs @@ -36,6 +36,7 @@ fn event_body_from_event(event: &Event) -> EventBody { automation, provenance, manifest_blob, + spec_blob, git, fork_source_ref, retried_from, @@ -54,6 +55,7 @@ fn event_body_from_event(event: &Event) -> EventBody { automation: automation.clone(), provenance: provenance.clone(), manifest_blob: *manifest_blob, + spec_blob: *spec_blob, git: git.clone(), fork_source_ref: fork_source_ref.clone(), retried_from: *retried_from, @@ -2669,6 +2671,7 @@ mod tests { automation: Some(automation.clone()), provenance, manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/event/events.rs b/lib/components/fabro-workflow/src/event/events.rs index f36d01a16..f465a5cfe 100644 --- a/lib/components/fabro-workflow/src/event/events.rs +++ b/lib/components/fabro-workflow/src/event/events.rs @@ -41,6 +41,8 @@ pub enum Event { #[serde(default, skip_serializing_if = "Option::is_none")] manifest_blob: Option, #[serde(default, skip_serializing_if = "Option::is_none")] + spec_blob: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] git: Option, #[serde(default, skip_serializing_if = "Option::is_none")] fork_source_ref: Option, diff --git a/lib/components/fabro-workflow/src/event/sink.rs b/lib/components/fabro-workflow/src/event/sink.rs index f6abb1724..150c69f72 100644 --- a/lib/components/fabro-workflow/src/event/sink.rs +++ b/lib/components/fabro-workflow/src/event/sink.rs @@ -290,6 +290,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/git.rs b/lib/components/fabro-workflow/src/git.rs index de144b6a1..e7ca4feda 100644 --- a/lib/components/fabro-workflow/src/git.rs +++ b/lib/components/fabro-workflow/src/git.rs @@ -364,6 +364,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/handler/agent.rs b/lib/components/fabro-workflow/src/handler/agent.rs index 48b234a8b..99d20431b 100644 --- a/lib/components/fabro-workflow/src/handler/agent.rs +++ b/lib/components/fabro-workflow/src/handler/agent.rs @@ -501,6 +501,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/handler/command.rs b/lib/components/fabro-workflow/src/handler/command.rs index 5dfc41e46..60f2b9a51 100644 --- a/lib/components/fabro-workflow/src/handler/command.rs +++ b/lib/components/fabro-workflow/src/handler/command.rs @@ -374,6 +374,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }, @@ -471,6 +472,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/handler/parallel.rs b/lib/components/fabro-workflow/src/handler/parallel.rs index 62fc59630..44323be72 100644 --- a/lib/components/fabro-workflow/src/handler/parallel.rs +++ b/lib/components/fabro-workflow/src/handler/parallel.rs @@ -956,6 +956,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/handler/prompt.rs b/lib/components/fabro-workflow/src/handler/prompt.rs index d586ca6b7..630332878 100644 --- a/lib/components/fabro-workflow/src/handler/prompt.rs +++ b/lib/components/fabro-workflow/src/handler/prompt.rs @@ -279,6 +279,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/lifecycle/git.rs b/lib/components/fabro-workflow/src/lifecycle/git.rs index fce72da6e..6fa277b31 100644 --- a/lib/components/fabro-workflow/src/lifecycle/git.rs +++ b/lib/components/fabro-workflow/src/lifecycle/git.rs @@ -750,6 +750,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/operations/archive.rs b/lib/components/fabro-workflow/src/operations/archive.rs index c44ff0006..92d9124cf 100644 --- a/lib/components/fabro-workflow/src/operations/archive.rs +++ b/lib/components/fabro-workflow/src/operations/archive.rs @@ -227,6 +227,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 59f424e2d..3f23acd5b 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -470,6 +470,7 @@ pub async fn persist_create_run( provenance, manifest_blob: None, definition_blob: None, + spec_blob: None, git, fork_source_ref, }; @@ -528,6 +529,12 @@ async fn persist_created_run( } None => None, }; + // The spec on the run.created event is subject to secret redaction in + // stored copies; the blob keeps the exact bytes execution needs. + let spec_blob = { + let bytes = serde_json::to_vec(record).map_err(|err| Error::engine(err.to_string()))?; + Some(run_store.write_blob(&bytes).await.map_err(store_error)?) + }; let title = explicit_title.unwrap_or_else(|| fabro_types::infer_run_title(record.graph.goal())); let stored = to_run_event_at( @@ -554,6 +561,7 @@ async fn persist_created_run( automation: record.automation.clone(), provenance: record.provenance.clone(), manifest_blob, + spec_blob, git: record.git.clone(), fork_source_ref: record.fork_source_ref.clone(), retried_from: None, diff --git a/lib/components/fabro-workflow/src/operations/fork.rs b/lib/components/fabro-workflow/src/operations/fork.rs index 7e5e65516..3f728211d 100644 --- a/lib/components/fabro-workflow/src/operations/fork.rs +++ b/lib/components/fabro-workflow/src/operations/fork.rs @@ -162,6 +162,9 @@ async fn persist_forked_run( automation: spec.automation.clone(), provenance: spec.provenance.clone(), manifest_blob: spec.manifest_blob, + // Content-addressed, so the forked run reads the source run's + // unredacted spec bytes through the same id. + spec_blob: spec.spec_blob, git: spec.git.clone(), fork_source_ref: spec.fork_source_ref.clone(), retried_from: None, @@ -381,6 +384,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: Some(fabro_types::GitContext { origin_url: "https://github.com/example/repo.git".to_string(), branch: "main".to_string(), diff --git a/lib/components/fabro-workflow/src/operations/retry.rs b/lib/components/fabro-workflow/src/operations/retry.rs index 27a9df68a..49b5861f4 100644 --- a/lib/components/fabro-workflow/src/operations/retry.rs +++ b/lib/components/fabro-workflow/src/operations/retry.rs @@ -54,6 +54,7 @@ pub async fn retry_run( provenance: _, manifest_blob, definition_blob, + spec_blob, git, fork_source_ref, } = source.spec; @@ -78,6 +79,9 @@ pub async fn retry_run( automation, provenance: input.provenance.clone(), manifest_blob, + // Blobs are content-addressed, so the retried run reads the source + // run's unredacted spec bytes through the same id. + spec_blob, git, fork_source_ref, retried_from: Some(source_run_id), @@ -185,6 +189,7 @@ mod tests { automation: None, provenance: provenance("source-user"), manifest_blob, + spec_blob: None, git: Some(git_context()), fork_source_ref, retried_from: None, diff --git a/lib/components/fabro-workflow/src/operations/timeline.rs b/lib/components/fabro-workflow/src/operations/timeline.rs index b96d10aa4..83319745e 100644 --- a/lib/components/fabro-workflow/src/operations/timeline.rs +++ b/lib/components/fabro-workflow/src/operations/timeline.rs @@ -252,6 +252,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }, diff --git a/lib/components/fabro-workflow/src/pipeline/execute/tests.rs b/lib/components/fabro-workflow/src/pipeline/execute/tests.rs index 118767be8..e69d19f29 100644 --- a/lib/components/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/components/fabro-workflow/src/pipeline/execute/tests.rs @@ -173,6 +173,7 @@ fn persisted_workflow(graph: Graph, source: String, run_dir: &Path, run_id: RunI provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, }, ) @@ -218,6 +219,7 @@ async fn seed_created_and_starting( automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: run_options.pre_run_git.clone(), fork_source_ref: run_options.fork_source_ref.clone(), retried_from: None, diff --git a/lib/components/fabro-workflow/src/pipeline/finalize.rs b/lib/components/fabro-workflow/src/pipeline/finalize.rs index 8c497175d..74b1e9579 100644 --- a/lib/components/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/components/fabro-workflow/src/pipeline/finalize.rs @@ -788,6 +788,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, @@ -906,6 +907,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }, diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index 5a7dd6069..49c32de19 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -870,6 +870,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref, }, ) @@ -1053,6 +1054,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: run_options.fork_source_ref.clone(), retried_from: None, diff --git a/lib/components/fabro-workflow/src/pipeline/persist.rs b/lib/components/fabro-workflow/src/pipeline/persist.rs index 846f9b853..8c224c700 100644 --- a/lib/components/fabro-workflow/src/pipeline/persist.rs +++ b/lib/components/fabro-workflow/src/pipeline/persist.rs @@ -2,6 +2,7 @@ use std::path::Path; use super::types::{PersistOptions, Persisted, Validated}; use crate::error::Error; +use crate::records::RunSpec; use crate::runtime_store::RunStoreHandle; /// PERSIST phase: create the run directory and return durable metadata for @@ -37,7 +38,7 @@ pub(crate) async fn load_from_store( .state() .await .map_err(|err| Error::engine(err.to_string()))?; - let run_spec = state.spec; + let run_spec = executable_run_spec(run_store, state.spec).await?; let graph = run_spec.graph.clone(); let source = run_spec.graph_source.clone().unwrap_or_default(); @@ -50,6 +51,40 @@ pub(crate) async fn load_from_store( )) } +/// Replace the event-folded spec content with the exact bytes from the spec +/// blob. Stored events pass through secret redaction, so the folded spec is +/// display data; the blob written at creation is what execution must see. +/// Runs created before the blob existed fall back to the folded spec. +async fn executable_run_spec( + run_store: &RunStoreHandle, + folded: RunSpec, +) -> Result { + let Some(blob_id) = folded.spec_blob else { + return Ok(folded); + }; + let bytes = run_store + .read_blob(&blob_id) + .await + .map_err(|err| Error::engine(err.to_string()))? + .ok_or_else(|| { + Error::engine(format!( + "run spec blob is missing from the run store: {blob_id}" + )) + })?; + let mut spec: RunSpec = + serde_json::from_slice(&bytes).map_err(|err| Error::Parse(err.to_string()))?; + // The event stream stays authoritative for run identity, for provenance + // (a retry rewrites it), for blob ids recorded on events after the spec + // blob was written, and for a graph source the blob does not carry. + spec.run_id = folded.run_id; + spec.provenance = folded.provenance; + spec.manifest_blob = folded.manifest_blob; + spec.definition_blob = folded.definition_blob; + spec.spec_blob = folded.spec_blob; + spec.graph_source = spec.graph_source.or(folded.graph_source); + Ok(spec) +} + #[cfg(test)] #[expect(clippy::disallowed_methods, reason = "tests stage pipeline fixtures")] mod tests { @@ -150,30 +185,52 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, } } async fn seeded_store(record: &RunSpec, source: Option<&str>) -> RunDatabase { + seeded_store_with(record, source, true).await + } + + async fn seeded_store_with( + record: &RunSpec, + source: Option<&str>, + write_spec_blob: bool, + ) -> RunDatabase { let store = memory_store(); let run_store = store.create_run(&record.run_id).await.unwrap(); + // Mirror the production producer: the unredacted spec rides a blob + // and the redacted event carries its id. + let spec_blob = if write_spec_blob { + Some( + run_store + .write_blob(&serde_json::to_vec(record).unwrap()) + .await + .unwrap(), + ) + } else { + None + }; append_event(&run_store, &record.run_id, &Event::RunCreated { - run_id: record.run_id, - title: None, - settings: serde_json::to_value(&record.settings).unwrap(), - graph: serde_json::to_value(&record.graph).unwrap(), - workflow_source: source.map(ToOwned::to_owned), - labels: record.labels.clone().into_iter().collect(), + run_id: record.run_id, + title: None, + settings: serde_json::to_value(&record.settings).unwrap(), + graph: serde_json::to_value(&record.graph).unwrap(), + workflow_source: source.map(ToOwned::to_owned), + labels: record.labels.clone().into_iter().collect(), source_directory: record.source_directory.clone(), - workflow_slug: record.workflow_slug.clone(), - automation: record.automation.clone(), - provenance: record.provenance.clone(), - manifest_blob: None, - git: record.git.clone(), - fork_source_ref: record.fork_source_ref.clone(), - retried_from: None, - parent_id: None, - web_url: None, + workflow_slug: record.workflow_slug.clone(), + automation: record.automation.clone(), + provenance: record.provenance.clone(), + manifest_blob: None, + spec_blob, + git: record.git.clone(), + fork_source_ref: record.fork_source_ref.clone(), + retried_from: None, + parent_id: None, + web_url: None, }) .await .unwrap(); @@ -316,6 +373,26 @@ mod tests { ); } + #[tokio::test] + async fn load_from_store_falls_back_to_folded_spec_without_spec_blob() { + // Runs created before the spec blob existed carry no spec_blob on + // run.created; the folded spec is their only copy. + let temp = tempfile::tempdir().unwrap(); + let run_dir = temp.path().join("run"); + std::fs::create_dir_all(&run_dir).unwrap(); + let (graph, source) = graph_and_source(); + let mut record = sample_record(different_graph()); + record.graph = graph; + + let run_store = seeded_store_with(&record, Some(&source), false).await; + let loaded = load_from_store(&run_store.clone().into(), &run_dir) + .await + .unwrap(); + + assert_eq!(loaded.run_spec().settings, record.settings); + assert_eq!(loaded.run_spec().spec_blob, None); + } + #[test] fn persist_returns_error_on_io_failure() { let temp = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-workflow/src/pipeline/pull_request.rs b/lib/components/fabro-workflow/src/pipeline/pull_request.rs index 1ab76a24b..f3f7e4a78 100644 --- a/lib/components/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/components/fabro-workflow/src/pipeline/pull_request.rs @@ -830,6 +830,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }, @@ -1112,6 +1113,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, }; append_event(&run_store, &fixtures::RUN_1, &Event::RunCreated { @@ -1126,6 +1128,7 @@ mod tests { automation: None, provenance: run_spec.provenance.clone(), manifest_blob: None, + spec_blob: None, git: run_spec.git.clone(), fork_source_ref: None, retried_from: None, @@ -1179,6 +1182,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, }; append_event(&run_store, &fixtures::RUN_1, &Event::RunCreated { @@ -1193,6 +1197,7 @@ mod tests { automation: None, provenance: run_spec.provenance.clone(), manifest_blob: None, + spec_blob: None, git: run_spec.git.clone(), fork_source_ref: None, retried_from: None, @@ -1596,6 +1601,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, }; append_event(&run_store, &fixtures::RUN_1, &Event::RunCreated { @@ -1610,6 +1616,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, @@ -1813,6 +1820,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, }; append_event(&run_store, &fixtures::RUN_1, &Event::RunCreated { @@ -1827,6 +1835,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/run_lookup.rs b/lib/components/fabro-workflow/src/run_lookup.rs index 99e9825fe..e70caf216 100644 --- a/lib/components/fabro-workflow/src/run_lookup.rs +++ b/lib/components/fabro-workflow/src/run_lookup.rs @@ -487,6 +487,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, } } @@ -512,6 +513,7 @@ mod tests { automation: None, provenance: run_spec.provenance.clone(), manifest_blob: None, + spec_blob: None, git: run_spec.git.clone(), fork_source_ref: run_spec.fork_source_ref.clone(), retried_from: None, diff --git a/lib/components/fabro-workflow/src/run_metadata.rs b/lib/components/fabro-workflow/src/run_metadata.rs index 74be989df..754bb5dfa 100644 --- a/lib/components/fabro-workflow/src/run_metadata.rs +++ b/lib/components/fabro-workflow/src/run_metadata.rs @@ -641,6 +641,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, }, chrono::Utc::now(), diff --git a/lib/components/fabro-workflow/src/runtime_store.rs b/lib/components/fabro-workflow/src/runtime_store.rs index c376c47e7..8f43ef647 100644 --- a/lib/components/fabro-workflow/src/runtime_store.rs +++ b/lib/components/fabro-workflow/src/runtime_store.rs @@ -151,6 +151,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, fork_source_ref: None, } } @@ -169,6 +170,7 @@ mod tests { automation: None, provenance: test_support::test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/components/fabro-workflow/src/stage_execution.rs b/lib/components/fabro-workflow/src/stage_execution.rs index c05a3547e..ec755127f 100644 --- a/lib/components/fabro-workflow/src/stage_execution.rs +++ b/lib/components/fabro-workflow/src/stage_execution.rs @@ -209,6 +209,7 @@ mod tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }; diff --git a/lib/components/fabro-workflow/src/test_support.rs b/lib/components/fabro-workflow/src/test_support.rs index 19390762d..c7407b7b9 100644 --- a/lib/components/fabro-workflow/src/test_support.rs +++ b/lib/components/fabro-workflow/src/test_support.rs @@ -204,6 +204,7 @@ async fn initialized( }, }, manifest_blob: None, + spec_blob: None, git: run_options.pre_run_git.clone(), fork_source_ref: run_options.fork_source_ref.clone(), retried_from: None, diff --git a/lib/foundation/fabro-api/tests/run_projection_round_trip.rs b/lib/foundation/fabro-api/tests/run_projection_round_trip.rs index 77d28178b..cbe1c6239 100644 --- a/lib/foundation/fabro-api/tests/run_projection_round_trip.rs +++ b/lib/foundation/fabro-api/tests/run_projection_round_trip.rs @@ -140,6 +140,7 @@ fn run_spec_json() -> serde_json::Value { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }) diff --git a/lib/foundation/fabro-redact/src/entropy.rs b/lib/foundation/fabro-redact/src/entropy.rs index dc744cf39..884aa3713 100644 --- a/lib/foundation/fabro-redact/src/entropy.rs +++ b/lib/foundation/fabro-redact/src/entropy.rs @@ -36,6 +36,12 @@ pub(super) fn shannon_entropy(s: &str) -> f64 { /// Returns regions where tokens match `[A-Za-z0-9+_=-]{10,}` and have /// Shannon entropy above the threshold (4.5 bits). Protects against /// consuming characters from JSON escape sequences. +/// +/// An assignment token (`NAME=value`) is measured and redacted by its +/// value alone. Measuring the pair merges the name's charset into the +/// value's and pushes innocuous values (a pure-hex git SHA can never +/// exceed 4.0 bits by itself) over the threshold, and redacting the +/// pair destroys the name that says what was redacted. pub(super) fn find_entropy_regions(s: &str) -> Vec { let mut regions = Vec::new(); for m in SECRET_PATTERN.find_iter(s) { @@ -58,6 +64,10 @@ pub(super) fn find_entropy_regions(s: &str) -> Vec { } } + if let Some(offset) = assignment_value_offset(&s[start..end]) { + start += offset; + } + if shannon_entropy(&s[start..end]) > ENTROPY_THRESHOLD { regions.push(Region { start, end }); } @@ -65,6 +75,24 @@ pub(super) fn find_entropy_regions(s: &str) -> Vec { regions } +/// For an assignment token (`NAME=value` with an identifier-shaped name), +/// return the byte offset where the value begins. Entropy above the 4.5-bit +/// threshold needs at least 23 distinct characters, so a value too short to +/// qualify simply measures under the threshold; no length guard is needed. +fn assignment_value_offset(token: &str) -> Option { + let eq = token.find('=')?; + let name = &token[..eq]; + let mut chars = name.chars(); + let first = chars.next()?; + if !(first.is_ascii_alphabetic() || first == '_') { + return None; + } + if !chars.all(|c| c.is_ascii_alphanumeric() || c == '_') { + return None; + } + Some(eq + 1) +} + #[cfg(test)] mod tests { use super::*; @@ -98,11 +126,12 @@ mod tests { #[test] fn regions_finds_high_entropy_token() { - // `=` is in the regex pattern, so "key=xK9..." matches as one token + // "key=xK9..." matches as one token, but only the value is + // measured and flagged; the name survives redaction. let input = "key=xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6p"; let regions = find_entropy_regions(input); assert_eq!(regions.len(), 1); - assert_eq!(regions[0].start, 0); + assert_eq!(regions[0].start, "key=".len()); assert_eq!(regions[0].end, input.len()); } diff --git a/lib/foundation/fabro-redact/src/jsonl.rs b/lib/foundation/fabro-redact/src/jsonl.rs index e466f7002..f0f50f6fa 100644 --- a/lib/foundation/fabro-redact/src/jsonl.rs +++ b/lib/foundation/fabro-redact/src/jsonl.rs @@ -209,7 +209,7 @@ mod tests { let redacted = redact_json_value(input); assert_eq!(redacted["name"], "fabro-01KQR3V9D4VPFFWMNTVH09J48G"); - assert_eq!(redacted["content"], "REDACTED"); + assert_eq!(redacted["content"], "token=REDACTED"); } #[test] @@ -262,7 +262,7 @@ mod tests { let redacted = redact_json_value(input); - assert_eq!(redacted["content"], "REDACTED"); + assert_eq!(redacted["content"], "key=REDACTED"); assert_eq!(redacted["session_id"], HIGH_ENTROPY_SECRET); } diff --git a/lib/foundation/fabro-test/src/lib.rs b/lib/foundation/fabro-test/src/lib.rs index 74a5f6727..eec273d32 100644 --- a/lib/foundation/fabro-test/src/lib.rs +++ b/lib/foundation/fabro-test/src/lib.rs @@ -1963,6 +1963,10 @@ pub fn json_snapshot_filters(mut filters: Vec<(String, String)>) -> Vec<(String, r#""definition_blob":\s*"[0-9a-f]{64}""#.to_string(), r#""definition_blob": "[BLOB_ID]""#.to_string(), )); + filters.push(( + r#""spec_blob":\s*"[0-9a-f]{64}""#.to_string(), + r#""spec_blob": "[BLOB_ID]""#.to_string(), + )); filters.push(( r#""run_dir":\s*"\[STORAGE_DIR\]/scratch/\d{8}-\[ULID\]""#.to_string(), r#""run_dir": "[RUN_DIR]""#.to_string(), diff --git a/lib/foundation/fabro-types/src/run.rs b/lib/foundation/fabro-types/src/run.rs index cf0fbe1ff..0c79bb23d 100644 --- a/lib/foundation/fabro-types/src/run.rs +++ b/lib/foundation/fabro-types/src/run.rs @@ -76,6 +76,11 @@ pub struct RunSpec { pub manifest_blob: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub definition_blob: Option, + /// Unredacted copy of this spec in the blob store. Stored events pass + /// through secret redaction, so the spec folded from them is display + /// data; execution must load the spec from this blob. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub spec_blob: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub git: Option, #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/lib/foundation/fabro-types/src/run_event/run.rs b/lib/foundation/fabro-types/src/run_event/run.rs index d070d8aef..b8c7fa34a 100644 --- a/lib/foundation/fabro-types/src/run_event/run.rs +++ b/lib/foundation/fabro-types/src/run_event/run.rs @@ -28,6 +28,11 @@ pub struct RunCreatedProps { pub provenance: RunProvenance, #[serde(default, skip_serializing_if = "Option::is_none")] pub manifest_blob: Option, + /// Unredacted copy of the run spec in the blob store. The settings and + /// graph on this event are redacted at the sink; execution loads the + /// spec from this blob instead. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub spec_blob: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub git: Option, #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/lib/foundation/fabro-types/src/run_projection.rs b/lib/foundation/fabro-types/src/run_projection.rs index 3a6be70d4..3616aa33c 100644 --- a/lib/foundation/fabro-types/src/run_projection.rs +++ b/lib/foundation/fabro-types/src/run_projection.rs @@ -1087,6 +1087,7 @@ mod title_tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }; @@ -1161,6 +1162,7 @@ mod iter_stages_tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }, @@ -1365,6 +1367,7 @@ mod live_timing_tests { provenance: test_support::test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: None, fork_source_ref: None, }, diff --git a/lib/foundation/fabro-types/tests/run_event_serde.rs b/lib/foundation/fabro-types/tests/run_event_serde.rs index 633abab04..f287a6acc 100644 --- a/lib/foundation/fabro-types/tests/run_event_serde.rs +++ b/lib/foundation/fabro-types/tests/run_event_serde.rs @@ -32,6 +32,7 @@ fn run_created_props_round_trip_templated_settings() { }), provenance: test_run_provenance(), manifest_blob: None, + spec_blob: None, git: Some(GitContext { origin_url: "https://github.com/fabro-sh/fabro.git".to_string(), branch: "main".to_string(), @@ -93,6 +94,7 @@ fn run_created_props_omits_web_url_when_absent() { automation: None, provenance: test_run_provenance(), manifest_blob: None, + spec_blob: None, git: None, fork_source_ref: None, retried_from: None, diff --git a/lib/foundation/fabro-types/tests/run_spec_methods.rs b/lib/foundation/fabro-types/tests/run_spec_methods.rs index f6f76fecf..50dff20b0 100644 --- a/lib/foundation/fabro-types/tests/run_spec_methods.rs +++ b/lib/foundation/fabro-types/tests/run_spec_methods.rs @@ -31,6 +31,7 @@ fn sample_run_spec() -> RunSpec { provenance: test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: Some(GitContext { origin_url: "https://github.com/fabro-sh/fabro.git".to_string(), branch: "main".to_string(), diff --git a/lib/foundation/fabro-types/tests/run_spec_serde.rs b/lib/foundation/fabro-types/tests/run_spec_serde.rs index 97529bc93..ec7a7ef40 100644 --- a/lib/foundation/fabro-types/tests/run_spec_serde.rs +++ b/lib/foundation/fabro-types/tests/run_spec_serde.rs @@ -31,6 +31,7 @@ fn run_spec_round_trips_templated_settings() { provenance: test_run_provenance(), manifest_blob: None, definition_blob: None, + spec_blob: None, git: Some(GitContext { origin_url: "https://github.com/fabro-sh/fabro.git".to_string(), branch: "main".to_string(), From 18a71ac310f8f429896930500b5d5fb4a6e98682 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 18 Aug 2026 12:08:36 -0400 Subject: [PATCH 08/19] Classify provider 412s as failover-eligible account lockouts Fireworks reports an account suspension (spending cap reached or unpaid invoices) as HTTP 412 with code PRECONDITION_FAILED. The status had no explicit mapping, and the openai_compatible dialect extracts error.type ("error") as the code, so the suspension fell through to InvalidRequest -- a deterministic request defect -- which suppressed both retry and the configured model fallback chain. A live run then died mid-stage with five healthy fallback candidates configured. No LLM request carries conditional-request preconditions, so a 412 is never about the request. Map it to AccessDenied, the same family as the account_deactivated error code: non-retryable on the same provider, eligible for failover to a provider with independent billing. Co-Authored-By: Claude Fable 5 --- lib/components/fabro-llm/src/error.rs | 54 ++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/lib/components/fabro-llm/src/error.rs b/lib/components/fabro-llm/src/error.rs index 6ec6d7886..0409d378e 100644 --- a/lib/components/fabro-llm/src/error.rs +++ b/lib/components/fabro-llm/src/error.rs @@ -355,7 +355,12 @@ pub fn error_from_status_code( // error types let kind = match status_code { 401 => ProviderErrorKind::Authentication, - 403 => ProviderErrorKind::AccessDenied, + // A 412 is never about the request: no LLM request carries + // conditional-request preconditions. Fireworks uses it for + // account-level lockouts (suspension over a spending cap or unpaid + // invoices), the same family as `account_deactivated`: deterministic + // here, but another provider has independent billing. + 403 | 412 => ProviderErrorKind::AccessDenied, 404 => ProviderErrorKind::NotFound, 408 => { return Error::RequestTimeout { @@ -728,6 +733,53 @@ mod tests { assert_eq!(err.provider_kind(), Some(ProviderErrorKind::QuotaExceeded)); } + /// Fireworks reports an account suspension (spending cap reached or + /// unpaid invoices) as HTTP 412 with `code: "PRECONDITION_FAILED"` in + /// the body. A chat completion carries no conditional-request + /// preconditions, so a 412 is always an account-level lockout, never a + /// defect in the request: it must not classify as `InvalidRequest`, and + /// a fallback provider with independent billing must stay eligible. + #[test] + fn account_suspension_412_is_failover_eligible() { + let err = error_from_status_code( + 412, + "Account lithoscomputer is suspended, possibly due to reaching \ + the monthly spending limit or failure to pay past invoices." + .into(), + "fireworks".into(), + // The openai_compatible dialect reads `error.type` as the code, + // so the discriminating `PRECONDITION_FAILED` only reaches this + // mapping through the status code. + Some("error".into()), + Some(serde_json::json!({ + "error": { + "message": "Account lithoscomputer is suspended, possibly due to reaching the monthly spending limit or failure to pay past invoices. Please go to https://fireworks.ai/account/billing for more information.", + "param": null, + "code": "PRECONDITION_FAILED", + "type": "error" + }, + "request_id": "chatcmpl-d9652b89a6604931ac27dddd5ef5bdc0" + })), + None, + ); + + assert_eq!(err.provider_kind(), Some(ProviderErrorKind::AccessDenied)); + assert!(!err.retryable()); + assert!(err.failover_eligible()); + + // A bare 412 with no parseable body classifies the same way. + let err = error_from_status_code( + 412, + "Precondition Failed".into(), + "fireworks".into(), + None, + None, + None, + ); + assert_eq!(err.provider_kind(), Some(ProviderErrorKind::AccessDenied)); + assert!(err.failover_eligible()); + } + #[test] fn kind_from_error_code_covers_every_dialect() { for (code, expected) in [ From 1226ed737776c944fad7921601a31377cd82fb29 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 18 Aug 2026 12:10:50 -0400 Subject: [PATCH 09/19] Cite Fireworks' documentation for the 412 mapping Co-Authored-By: Claude Fable 5 --- lib/components/fabro-llm/src/error.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/components/fabro-llm/src/error.rs b/lib/components/fabro-llm/src/error.rs index 0409d378e..dd9a98347 100644 --- a/lib/components/fabro-llm/src/error.rs +++ b/lib/components/fabro-llm/src/error.rs @@ -356,10 +356,12 @@ pub fn error_from_status_code( let kind = match status_code { 401 => ProviderErrorKind::Authentication, // A 412 is never about the request: no LLM request carries - // conditional-request preconditions. Fireworks uses it for - // account-level lockouts (suspension over a spending cap or unpaid - // invoices), the same family as `account_deactivated`: deterministic - // here, but another provider has independent billing. + // conditional-request preconditions. Fireworks documents it as + // "Account is suspended or there's an issue with account status", + // also emitted for a LoRA model that failed to load + // (https://docs.fireworks.ai/guides/inference-error-codes). The same + // family as `account_deactivated`: deterministic here, but another + // provider has independent billing and model inventory. 403 | 412 => ProviderErrorKind::AccessDenied, 404 => ProviderErrorKind::NotFound, 408 => { From 0845c331cb38c25db5265d2e1056dc3eb0fee6ec Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 17:26:13 -0400 Subject: [PATCH 10/19] Default Daytona auto-stop to 120 minutes Omitting autoStopInterval from the create-sandbox request inherits Daytona's server-side default of 15 idle minutes. Daytona counts inactivity from the last sandbox interaction, and LLM inference never touches the sandbox, so a single long inference call is enough for the sandbox to auto-stop mid-run: a workflow failed exactly this way, with the sandbox entering its stop transition 15 minutes after the last command while the agent was still thinking. Send an explicit 120-minute default when lifecycle.auto_stop is unset. That clears any realistic inference call while still reclaiming sandboxes leaked by a dead worker. An explicit auto_stop = "0s" still disables auto-stop entirely. Co-Authored-By: Claude Fable 5 --- docs/public/execution/run-configuration.mdx | 2 +- docs/public/integrations/daytona.mdx | 4 +++ .../fabro-sandbox/src/daytona/mod.rs | 36 ++++++++++++++++++- 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index a8f1bc927..d2ce9bd5e 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -321,7 +321,7 @@ memory = "8GB" | `network.allow` | CIDRs for `cidr_allow_list`; entries are validated as CIDRs. | | `lifecycle.preserve` | Keep the created sandbox after the run finishes. | | `lifecycle.stop_on_terminal` | Stop the sandbox when the run reaches a terminal state. | -| `lifecycle.auto_stop` | Daytona auto-stop duration, such as `"30m"`. | +| `lifecycle.auto_stop` | Daytona auto-stop duration, such as `"30m"`. Defaults to `"120m"`; `"0s"` disables auto-stop. | | `labels` | Provider labels. Merge by key across layers. | | `env` | Environment variables passed to command and agent execution. Merge by key across layers. | diff --git a/docs/public/integrations/daytona.mdx b/docs/public/integrations/daytona.mdx index 6ad19b9b3..1a2f25790 100644 --- a/docs/public/integrations/daytona.mdx +++ b/docs/public/integrations/daytona.mdx @@ -198,6 +198,10 @@ The `lifecycle.auto_stop` setting tells Daytona to stop the sandbox after a peri auto_stop = "30m" ``` +When `auto_stop` is unset, Fabro applies a default of 120 minutes so a sandbox leaked by an interrupted run is still reclaimed. Set `auto_stop = "0s"` to disable auto-stop and let the sandbox run indefinitely. + +Daytona counts inactivity from the last sandbox interaction (a command, file operation, or other API call). Time an agent spends on LLM inference does not touch the sandbox, so intervals shorter than your longest inference call risk stopping the sandbox mid-run. + ## Server defaults When running via `fabro server start`, the server config at `~/.fabro/settings.toml` can set default Daytona settings for all runs. Run config TOML values override server defaults. Labels are **merged** — run config labels win on key collisions. The `network` setting uses simple override (run config replaces the server default entirely). diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 6371f23d2..91a0cc797 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -68,6 +68,12 @@ const DAYTONA_START_TIMEOUT: Duration = Duration::from_mins(1); /// deletion, temporary stdin files) so a stalled REST call cannot block /// cancellation/timeout paths indefinitely. const DAYTONA_CLEANUP_TIMEOUT: Duration = Duration::from_secs(10); +/// Auto-stop applied when `lifecycle.auto_stop` is unset. Omitting the field +/// would inherit Daytona's server-side default of 15 idle minutes, which is +/// shorter than a single long inference call and stops the sandbox mid-run; +/// 120 minutes clears any realistic call while still reclaiming sandboxes +/// leaked by a dead worker. An explicit `0` disables auto-stop entirely. +const DEFAULT_AUTO_STOP_INTERVAL_MINUTES: i32 = 120; /// Permissions a Daytona API key needs for Fabro's snapshot and sandbox flow. pub const REQUIRED_DAYTONA_PERMISSIONS: &[Permissions] = &[ @@ -727,7 +733,10 @@ impl DaytonaSandbox { daytona_sdk::SandboxBaseParams { name: Some(name), env_vars: Some(clean_bash_env(None)), - auto_stop_interval: self.config.auto_stop_interval, + auto_stop_interval: self + .config + .auto_stop_interval + .or(Some(DEFAULT_AUTO_STOP_INTERVAL_MINUTES)), labels: Some(managed_labels::merge_for_run( self.config.labels.as_ref(), self.run_id.as_ref(), @@ -2950,6 +2959,10 @@ mod tests { assert_eq!(params.ephemeral, Some(false)); assert_eq!(params.auto_delete_interval, Some(-1)); + assert_eq!( + params.auto_stop_interval, + Some(DEFAULT_AUTO_STOP_INTERVAL_MINUTES) + ); assert_eq!( params.env_vars, Some(HashMap::from([(BASH_ENV_VAR.to_string(), String::new())])) @@ -2963,6 +2976,27 @@ mod tests { ); } + #[tokio::test] + async fn base_params_passes_explicit_auto_stop_through() { + for interval in [0, 45] { + let sandbox = DaytonaSandbox::new( + DaytonaConfig { + auto_stop_interval: Some(interval), + ..DaytonaConfig::default() + }, + None, + None, + None, + None, + Some("dtn_test".to_string()), + ) + .await + .expect("sandbox config should be valid"); + + assert_eq!(sandbox.base_params().auto_stop_interval, Some(interval)); + } + } + #[tokio::test] async fn activate_skips_start_when_daytona_reports_started() { let server = MockServer::start_async().await; From 0eedb1798c0f9da34917ad5544571d6fb7f054ce Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 17:35:11 -0400 Subject: [PATCH 11/19] Treat Daytona state transitions as wait-and-retry in activate/start/stop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Daytona rejects start/stop with HTTP 400 "State change in progress" while a lifecycle transition is in flight, and activate() only handled the Started and Starting states: any other state fell through to start(), which surfaced the rejection as a hard failure. A run died exactly this way when an inactivity auto-stop began seconds before the stage finished — activate() saw the sandbox mid-stop and failed the whole run 35ms later. The cleanup stop() then failed on the same rejection. Transitions finish on their own within seconds, so treat them as wait-and-retry conditions: - activate() now waits out a Stopping sandbox and dispatches on whatever state the transition lands on. - start() and stop() retry the rejected call within a bounded budget, re-inspecting state between attempts: a transition that lands on Started needs no further start, and one that lands on Stopped or Destroyed needs no further stop. All call sites go through these three provider methods, so no lifecycle-layer changes are needed. Co-Authored-By: Claude Fable 5 --- .../fabro-sandbox/src/daytona/mod.rs | 404 +++++++++++++++--- 1 file changed, 342 insertions(+), 62 deletions(-) diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 6371f23d2..22d3adbca 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -68,6 +68,12 @@ const DAYTONA_START_TIMEOUT: Duration = Duration::from_mins(1); /// deletion, temporary stdin files) so a stalled REST call cannot block /// cancellation/timeout paths indefinitely. const DAYTONA_CLEANUP_TIMEOUT: Duration = Duration::from_secs(10); +/// Budget for waiting out an in-flight Daytona lifecycle transition (for +/// example an auto-stop racing an activation) before giving up. Transitions +/// normally finish within seconds; the budget only bounds a wedged sandbox. +const DAYTONA_STATE_CHANGE_TIMEOUT: Duration = Duration::from_mins(2); +/// Poll interval while waiting out an in-flight Daytona lifecycle transition. +const DAYTONA_STATE_CHANGE_POLL_INTERVAL: Duration = Duration::from_secs(1); /// Permissions a Daytona API key needs for Fabro's snapshot and sandbox flow. pub const REQUIRED_DAYTONA_PERMISSIONS: &[Permissions] = &[ @@ -852,6 +858,118 @@ impl DaytonaSandbox { "Timed out waiting for snapshot '{name}' to become active" ))) } + + /// Start the sandbox, retrying while Daytona reports a lifecycle + /// transition in flight, up to `deadline`. + /// + /// Daytona rejects `start` with "state change in progress" while a + /// transition (such as an inactivity auto-stop) is still running. The + /// transition finishes on its own within seconds, so the rejection is a + /// wait-and-retry condition, not a failure. Between attempts the state is + /// re-inspected: a transition that lands on `Started` (a concurrent + /// activation won the race) needs no further start call. + async fn start_with_deadline(&self, deadline: time::Instant) -> crate::Result<()> { + self.emit(SandboxEvent::StartStarted { + provider: "daytona".into(), + }); + let start = Instant::now(); + let sandbox = self.sandbox()?; + loop { + match self.client.start(&sandbox.name).await { + Ok(_) => break, + Err(e) if is_state_change_in_progress(&e) && time::Instant::now() < deadline => { + tracing::debug!( + "Daytona start rejected while a state change is in progress; retrying" + ); + time::sleep(DAYTONA_STATE_CHANGE_POLL_INTERVAL).await; + if let Ok(current) = self.client.get(&sandbox.name).await { + if current.state == Some(SandboxState::Started) { + break; + } + } + } + Err(e) => { + let err = crate::Error::context("Failed to start Daytona sandbox", e); + self.emit(SandboxEvent::StartFailed { + provider: "daytona".into(), + error: err.to_string(), + causes: err.causes(), + }); + return Err(err); + } + } + } + if let Err(err) = Self::probe_bash(sandbox).await { + self.emit(SandboxEvent::StartFailed { + provider: "daytona".into(), + error: err.to_string(), + causes: err.causes(), + }); + return Err(err); + } + let duration_ms = elapsed_ms(start); + self.emit(SandboxEvent::StartCompleted { + provider: "daytona".into(), + duration_ms, + }); + Ok(()) + } + + /// Stop the sandbox, retrying while Daytona reports a lifecycle + /// transition in flight, up to `deadline`. + /// + /// The in-flight transition may be the stop itself (an inactivity + /// auto-stop): between attempts the state is re-inspected, and a sandbox + /// that landed on `Stopped` or `Destroyed` needs no further stop call. + async fn stop_with_deadline(&self, deadline: time::Instant) -> crate::Result<()> { + self.emit(SandboxEvent::StopStarted { + provider: "daytona".into(), + }); + let start = Instant::now(); + let sandbox = self.sandbox()?; + loop { + match self.client.stop(&sandbox.name).await { + Ok(_) => break, + Err(e) if is_state_change_in_progress(&e) && time::Instant::now() < deadline => { + tracing::debug!( + "Daytona stop rejected while a state change is in progress; retrying" + ); + time::sleep(DAYTONA_STATE_CHANGE_POLL_INTERVAL).await; + if let Ok(current) = self.client.get(&sandbox.name).await { + if matches!( + current.state, + Some(SandboxState::Stopped | SandboxState::Destroyed) + ) { + break; + } + } + } + Err(e) => { + let err = crate::Error::context("Failed to stop Daytona sandbox", e); + self.emit(SandboxEvent::StopFailed { + provider: "daytona".into(), + error: err.to_string(), + causes: err.causes(), + }); + return Err(err); + } + } + } + let duration_ms = elapsed_ms(start); + self.emit(SandboxEvent::StopCompleted { + provider: "daytona".into(), + duration_ms, + }); + Ok(()) + } +} + +/// Whether a Daytona API error reports a lifecycle transition in flight +/// (HTTP 400 "State change in progress" on start/stop). +fn is_state_change_in_progress(err: &DaytonaError) -> bool { + err.to_string() + .to_ascii_lowercase() + .contains("state change in progress") } /// Detect the git remote URL and current branch from a local repository. @@ -1336,76 +1454,51 @@ impl Sandbox for DaytonaSandbox { } async fn start(&self) -> crate::Result<()> { - self.emit(SandboxEvent::StartStarted { - provider: "daytona".into(), - }); - let start = Instant::now(); - let sandbox = self.sandbox()?; - if let Err(e) = self.client.start(&sandbox.name).await { - let err = crate::Error::context("Failed to start Daytona sandbox", e); - self.emit(SandboxEvent::StartFailed { - provider: "daytona".into(), - error: err.to_string(), - causes: err.causes(), - }); - return Err(err); - } - if let Err(err) = Self::probe_bash(sandbox).await { - self.emit(SandboxEvent::StartFailed { - provider: "daytona".into(), - error: err.to_string(), - causes: err.causes(), - }); - return Err(err); - } - let duration_ms = elapsed_ms(start); - self.emit(SandboxEvent::StartCompleted { - provider: "daytona".into(), - duration_ms, - }); - Ok(()) + self.start_with_deadline(time::Instant::now() + DAYTONA_STATE_CHANGE_TIMEOUT) + .await } async fn activate(&self) -> crate::Result<()> { let sandbox = self.sandbox()?; - let current = self.client.get(&sandbox.name).await.map_err(|e| { - crate::Error::context("Failed to inspect Daytona sandbox before activation", e) - })?; - if current.state == Some(SandboxState::Started) { - return Ok(()); + let deadline = time::Instant::now() + DAYTONA_STATE_CHANGE_TIMEOUT; + loop { + let current = self.client.get(&sandbox.name).await.map_err(|e| { + crate::Error::context("Failed to inspect Daytona sandbox before activation", e) + })?; + match current.state { + Some(SandboxState::Started) => return Ok(()), + Some(SandboxState::Starting) => { + return current + .wait_for_start(Some(DAYTONA_START_TIMEOUT)) + .await + .map_err(|e| { + crate::Error::context( + "Failed to wait for Daytona sandbox activation", + e, + ) + }); + } + // An inactivity auto-stop can be in flight when a stage + // returns after a long period with no sandbox traffic (LLM + // inference generates none). Wait out the transition and + // dispatch on whatever state it lands on. + Some(SandboxState::Stopping) => { + if time::Instant::now() >= deadline { + return Err(crate::Error::message(format!( + "Daytona sandbox stop still in progress after {}s", + DAYTONA_STATE_CHANGE_TIMEOUT.as_secs() + ))); + } + time::sleep(DAYTONA_STATE_CHANGE_POLL_INTERVAL).await; + } + _ => return self.start_with_deadline(deadline).await, + } } - if current.state == Some(SandboxState::Starting) { - return current - .wait_for_start(Some(DAYTONA_START_TIMEOUT)) - .await - .map_err(|e| { - crate::Error::context("Failed to wait for Daytona sandbox activation", e) - }); - } - self.start().await } async fn stop(&self) -> crate::Result<()> { - self.emit(SandboxEvent::StopStarted { - provider: "daytona".into(), - }); - let start = Instant::now(); - let sandbox = self.sandbox()?; - if let Err(e) = self.client.stop(&sandbox.name).await { - let err = crate::Error::context("Failed to stop Daytona sandbox", e); - self.emit(SandboxEvent::StopFailed { - provider: "daytona".into(), - error: err.to_string(), - causes: err.causes(), - }); - return Err(err); - } - let duration_ms = elapsed_ms(start); - self.emit(SandboxEvent::StopCompleted { - provider: "daytona".into(), - duration_ms, - }); - Ok(()) + self.stop_with_deadline(time::Instant::now() + DAYTONA_STATE_CHANGE_TIMEOUT) + .await } async fn delete(&self) -> crate::Result<()> { @@ -3064,6 +3157,193 @@ mod tests { start_sandbox.assert_calls_async(0).await; } + #[tokio::test] + async fn activate_waits_out_a_stop_in_progress() { + let server = MockServer::start_async().await; + let response_count = Arc::new(AtomicU32::new(0)); + let get_sandbox = server + .mock_async({ + let response_count = Arc::clone(&response_count); + move |when, then| { + when.method(GET) + .path("/sandbox/test-sandbox") + .header("authorization", "Bearer dtn_test"); + then.respond_with(move |_| { + let state = if response_count.fetch_add(1, Ordering::Relaxed) == 1 { + SandboxState::Stopping + } else { + SandboxState::Started + }; + HttpMockResponse::builder() + .status(200) + .header("content-type", "application/json") + .body(sandbox_body("test-sandbox", state).to_string()) + .build() + }); + } + }) + .await; + let start_sandbox = server + .mock_async(|when, then| { + when.method(POST) + .path("/sandbox/test-sandbox/start") + .header("authorization", "Bearer dtn_test"); + then.status(200) + .header("content-type", "application/json") + .json_body(sandbox_body("test-sandbox", SandboxState::Started)); + }) + .await; + let sandbox = mock_daytona_sandbox(&server, "dtn_test", DaytonaConfig::default()).await; + let sdk_sandbox = sandbox + .client + .get("test-sandbox") + .await + .expect("test sandbox should load"); + sandbox + .sandbox + .set(sdk_sandbox) + .expect("test sandbox should initialize once"); + + let get_calls_before = get_sandbox.calls_async().await; + sandbox + .activate() + .await + .expect("an in-progress stop should be waited out"); + + assert_eq!(get_sandbox.calls_async().await, get_calls_before + 2); + start_sandbox.assert_calls_async(0).await; + } + + #[tokio::test] + async fn stop_succeeds_when_a_pending_auto_stop_finishes_first() { + let server = MockServer::start_async().await; + let response_count = Arc::new(AtomicU32::new(0)); + let get_sandbox = server + .mock_async({ + let response_count = Arc::clone(&response_count); + move |when, then| { + when.method(GET) + .path("/sandbox/test-sandbox") + .header("authorization", "Bearer dtn_test"); + then.respond_with(move |_| { + let state = if response_count.fetch_add(1, Ordering::Relaxed) == 0 { + SandboxState::Started + } else { + SandboxState::Stopped + }; + HttpMockResponse::builder() + .status(200) + .header("content-type", "application/json") + .body(sandbox_body("test-sandbox", state).to_string()) + .build() + }); + } + }) + .await; + let stop_sandbox = server + .mock_async(|when, then| { + when.method(POST) + .path("/sandbox/test-sandbox/stop") + .header("authorization", "Bearer dtn_test"); + then.status(400) + .header("content-type", "application/json") + .json_body(serde_json::json!({ + "message": "Sandbox state change in progress", + "statusCode": 400 + })); + }) + .await; + let sandbox = mock_daytona_sandbox(&server, "dtn_test", DaytonaConfig::default()).await; + let sdk_sandbox = sandbox + .client + .get("test-sandbox") + .await + .expect("test sandbox should load"); + sandbox + .sandbox + .set(sdk_sandbox) + .expect("test sandbox should initialize once"); + + let get_calls_before = get_sandbox.calls_async().await; + sandbox + .stop() + .await + .expect("a stop already in flight should count as stopped"); + + stop_sandbox.assert_calls_async(1).await; + assert_eq!(get_sandbox.calls_async().await, get_calls_before + 1); + } + + #[tokio::test] + async fn start_surfaces_state_change_rejection_after_the_deadline() { + let server = MockServer::start_async().await; + let _get_sandbox = server + .mock_async(|when, then| { + when.method(GET) + .path("/sandbox/test-sandbox") + .header("authorization", "Bearer dtn_test"); + then.status(200) + .header("content-type", "application/json") + .json_body(sandbox_body("test-sandbox", SandboxState::Stopping)); + }) + .await; + let start_sandbox = server + .mock_async(|when, then| { + when.method(POST) + .path("/sandbox/test-sandbox/start") + .header("authorization", "Bearer dtn_test"); + then.status(400) + .header("content-type", "application/json") + .json_body(serde_json::json!({ + "message": "Sandbox state change in progress", + "statusCode": 400 + })); + }) + .await; + let sandbox = mock_daytona_sandbox(&server, "dtn_test", DaytonaConfig::default()).await; + let sdk_sandbox = sandbox + .client + .get("test-sandbox") + .await + .expect("test sandbox should load"); + sandbox + .sandbox + .set(sdk_sandbox) + .expect("test sandbox should initialize once"); + + let err = sandbox + .start_with_deadline(time::Instant::now() + Duration::from_millis(1500)) + .await + .expect_err("a state change that outlives the deadline should fail"); + + assert!( + start_sandbox.calls_async().await >= 2, + "start should be retried while the deadline allows" + ); + assert!( + err.causes() + .iter() + .any(|cause| cause.to_ascii_lowercase().contains("state change in progress")), + "error should carry the Daytona rejection: {err}" + ); + } + + #[test] + fn state_change_in_progress_matcher_ignores_case_and_context() { + assert!(is_state_change_in_progress(&DaytonaError::api( + 400, + "Sandbox state change in progress" + ))); + assert!(is_state_change_in_progress(&DaytonaError::api( + 400, + "State Change In Progress" + ))); + assert!(!is_state_change_in_progress(&DaytonaError::api( + 400, + "Sandbox already started" + ))); + } + #[tokio::test] async fn base_params_merges_managed_daytona_labels() { let run_id: RunId = "01HY0000000000000000000000".parse().unwrap(); From f88df59163ad287a093bf26dc600c13be5343daa Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 17:39:19 -0400 Subject: [PATCH 12/19] Classify sandbox state-change rejections as transient infra MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Daytona "Sandbox state change in progress" rejection surfacing through the pipeline lifecycle path ("Pipeline lifecycle operation failed") matched no transient-infra hint, so the run failure was categorized deterministic. The condition is a provider lifecycle transition that finishes on its own — the definition of transient infrastructure — and the deterministic label misinforms retry machinery and anyone reading the failure. Add two transient-infra hints: the provider rejection ("state change in progress") and the bounded-wait timeout an activation reports when a stop transition outlives its budget ("sandbox stop still in progress"). Co-Authored-By: Claude Fable 5 --- lib/components/fabro-workflow/src/error.rs | 35 +++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/lib/components/fabro-workflow/src/error.rs b/lib/components/fabro-workflow/src/error.rs index f8c08ca15..21833a8df 100644 --- a/lib/components/fabro-workflow/src/error.rs +++ b/lib/components/fabro-workflow/src/error.rs @@ -84,6 +84,8 @@ const TRANSIENT_INFRA_HINTS: &[&str] = &[ "cross-device link", "invalid cross-device link", "os error 18", + "state change in progress", + "sandbox stop still in progress", ]; const BUDGET_EXHAUSTED_HINTS: &[&str] = &[ @@ -807,6 +809,18 @@ mod tests { assert_eq!(err.failure_category(), FailureCategory::TransientInfra); } + #[test] + fn engine_error_with_sandbox_state_change_cause_classifies_transient() { + let source = TestOuterError { + message: "Failed to start Daytona sandbox", + source: TestCause("Sandbox state change in progress"), + }; + let err = Error::engine_with_source("Pipeline lifecycle operation failed", source); + + assert_eq!(err.failure_category(), FailureCategory::TransientInfra); + assert!(err.is_retryable()); + } + #[test] fn handler_error_display() { let err = Error::handler("LLM call failed"); @@ -1281,7 +1295,7 @@ mod tests { #[test] fn transient_infra_hints_count() { - assert_eq!(TRANSIENT_INFRA_HINTS.len(), 38); + assert_eq!(TRANSIENT_INFRA_HINTS.len(), 40); } #[test] @@ -1450,6 +1464,25 @@ mod tests { ); } + #[test] + fn classify_reason_sandbox_state_change_in_progress() { + assert_eq!( + classify_failure_reason( + "Pipeline lifecycle operation failed: failed to activate sandbox after node \ + attempt survey: Failed to start Daytona sandbox: Sandbox state change in progress" + ), + FailureCategory::TransientInfra + ); + } + + #[test] + fn classify_reason_sandbox_stop_still_in_progress() { + assert_eq!( + classify_failure_reason("Daytona sandbox stop still in progress after 120s"), + FailureCategory::TransientInfra + ); + } + #[test] fn classify_reason_500() { assert_eq!( From 2456356a9fd4bb3181880ec4740f8d6de2ba3786 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 10:19:27 -0400 Subject: [PATCH 13/19] Label sandbox git execs with git_op tracing spans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sandbox exec logs previously required command_len fingerprinting to tell a push from a credential refresh or a checkpoint commit. The shared git helpers now instrument their futures with a git_op span, so Daytona's and Docker's `exec_command: entered` lines inherit the operation label and the log renders as `git_op{op=push}: exec_command: entered timeout_ms=...`. Ops: push (git_push_via_exec), refresh-credentials (both providers' refresh_push_credentials), checkpoint-commit (checked_git_checkpoint), fetch (fetch_source_run_ref), and metadata-push (the run-metadata snapshot write). Spans are attached with #[tracing::instrument] — attached to the future, never an entered() guard held across an await — so they follow the task across worker threads. No trait or signature changes. Plan: .ai/plans/git-push-token-resilience.md (PR 3: item 10). Co-Authored-By: Claude Fable 5 --- lib/components/fabro-sandbox/src/daytona/mod.rs | 1 + lib/components/fabro-sandbox/src/docker.rs | 1 + lib/components/fabro-sandbox/src/sandbox.rs | 2 ++ lib/components/fabro-workflow/src/run_metadata.rs | 1 + lib/components/fabro-workflow/src/sandbox_git.rs | 1 + 5 files changed, 6 insertions(+) diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 0375a51cf..5a6da1d6c 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -1542,6 +1542,7 @@ impl Sandbox for DaytonaSandbox { Ok(Some((preview.url, headers))) } + #[tracing::instrument(name = "git_op", skip_all, fields(op = "refresh-credentials"))] async fn refresh_push_credentials(&self) -> crate::Result { if !self.repo_cloned() { return Ok(RefreshOutcome::Skipped); diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index d69dcca4a..21530cc94 100644 --- a/lib/components/fabro-sandbox/src/docker.rs +++ b/lib/components/fabro-sandbox/src/docker.rs @@ -2180,6 +2180,7 @@ impl Sandbox for DockerSandbox { self.origin_url.get().map(String::as_str) } + #[tracing::instrument(name = "git_op", skip_all, fields(op = "refresh-credentials"))] async fn refresh_push_credentials(&self) -> crate::Result { if !self.repo_cloned() { return Ok(RefreshOutcome::Skipped); diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index 31a873300..c70c13ee9 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -1465,6 +1465,7 @@ pub async fn setup_git_via_exec( }) } +#[tracing::instrument(name = "git_op", skip_all, fields(op = "fetch"))] pub(crate) async fn fetch_source_run_ref( sandbox: &dyn Sandbox, source_run_id: &str, @@ -1513,6 +1514,7 @@ pub(crate) async fn fetch_source_run_ref( /// Helper for sandbox implementations that manage git internally. /// Pushes a refspec to origin via exec_command inside the sandbox. +#[tracing::instrument(name = "git_op", skip_all, fields(op = "push"))] pub async fn git_push_via_exec(sandbox: &dyn Sandbox, refspec: &str) -> crate::Result<()> { if let Err(e) = sandbox.refresh_push_credentials().await { tracing::warn!( diff --git a/lib/components/fabro-workflow/src/run_metadata.rs b/lib/components/fabro-workflow/src/run_metadata.rs index 74be989df..39d686857 100644 --- a/lib/components/fabro-workflow/src/run_metadata.rs +++ b/lib/components/fabro-workflow/src/run_metadata.rs @@ -184,6 +184,7 @@ impl RunMetadataWriterHandle { .unwrap() } + #[tracing::instrument(name = "git_op", skip_all, fields(op = "metadata-push"))] pub(crate) async fn write_snapshot( &self, dump: &RunDump, diff --git a/lib/components/fabro-workflow/src/sandbox_git.rs b/lib/components/fabro-workflow/src/sandbox_git.rs index 914153955..c6084a977 100644 --- a/lib/components/fabro-workflow/src/sandbox_git.rs +++ b/lib/components/fabro-workflow/src/sandbox_git.rs @@ -158,6 +158,7 @@ pub async fn git_checkpoint( clippy::too_many_arguments, reason = "Checkpointing needs explicit run metadata, checkpoint settings, and author inputs." )] +#[tracing::instrument(name = "git_op", skip_all, fields(op = "checkpoint-commit"))] pub(crate) async fn checked_git_checkpoint( runtime: &SandboxGitRuntime, sandbox: &dyn Sandbox, From 4eea9b816a365e48755bb2d43665d2c768acba7f Mon Sep 17 00:00:00 2001 From: Release Repro Date: Thu, 20 Aug 2026 19:38:10 -0400 Subject: [PATCH 14/19] Clarify visit-limit counter comment --- lib/foundation/fabro-core/src/executor.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/foundation/fabro-core/src/executor.rs b/lib/foundation/fabro-core/src/executor.rs index f6e3a6aa3..6701e30be 100644 --- a/lib/foundation/fabro-core/src/executor.rs +++ b/lib/foundation/fabro-core/src/executor.rs @@ -181,8 +181,8 @@ impl Executor { // Check visit limits before entry: a node with a limit of N may // execute N times, matching the documented contract. The count - // covers completed entries only, so the refused visit is not - // reported as one. + // covers previously admitted entries, so the refused visit is + // not reported as one. let visits = state.visits(node.id()); if let Some(max) = node.max_visits() { if visits >= max { From f8a82d6865eea4b5d9e7c91e87f10043857352ac Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 19:56:06 -0400 Subject: [PATCH 15/19] fix: harden GitHub token refresh handling --- lib/components/fabro-github/Cargo.toml | 3 + lib/components/fabro-github/src/lib.rs | 3 + .../fabro-github/src/test_support.rs | 26 ++ .../fabro-github/src/token_source.rs | 89 +++++- lib/components/fabro-sandbox/Cargo.toml | 1 + .../fabro-sandbox/src/daytona/mod.rs | 24 +- lib/components/fabro-sandbox/src/docker.rs | 25 +- lib/components/fabro-sandbox/src/error.rs | 14 + .../fabro-sandbox/src/provider/docker.rs | 9 +- .../fabro-sandbox/src/push_credentials.rs | 81 +++--- lib/components/fabro-sandbox/src/sandbox.rs | 52 +++- .../fabro-sandbox/src/sandbox_spec.rs | 2 +- lib/components/fabro-workflow/Cargo.toml | 1 + .../fabro-workflow/src/github_token_source.rs | 274 ------------------ .../fabro-workflow/src/handler/command.rs | 7 +- .../fabro-workflow/src/handler/llm/acp.rs | 191 ++++++------ lib/components/fabro-workflow/src/lib.rs | 1 - .../fabro-workflow/src/pipeline/initialize.rs | 33 +-- .../fabro-workflow/src/run_metadata.rs | 13 +- lib/components/fabro-workflow/src/services.rs | 28 +- 20 files changed, 365 insertions(+), 512 deletions(-) create mode 100644 lib/components/fabro-github/src/test_support.rs delete mode 100644 lib/components/fabro-workflow/src/github_token_source.rs diff --git a/lib/components/fabro-github/Cargo.toml b/lib/components/fabro-github/Cargo.toml index 788c6efe1..8f755dd79 100644 --- a/lib/components/fabro-github/Cargo.toml +++ b/lib/components/fabro-github/Cargo.toml @@ -9,6 +9,9 @@ description = "GitHub App authentication and API helpers for Fabro" [lib] doctest = false +[features] +test-support = [] + [lints] workspace = true diff --git a/lib/components/fabro-github/src/lib.rs b/lib/components/fabro-github/src/lib.rs index 9973c4377..a38c6c8ce 100644 --- a/lib/components/fabro-github/src/lib.rs +++ b/lib/components/fabro-github/src/lib.rs @@ -11,6 +11,9 @@ use tokio::process::Command; pub mod token_source; +#[cfg(any(test, feature = "test-support"))] +pub mod test_support; + pub const GITHUB_API_BASE_URL: &str = "https://api.github.com"; /// Returns the GitHub API base URL, allowing override via `GITHUB_BASE_URL` env diff --git a/lib/components/fabro-github/src/test_support.rs b/lib/components/fabro-github/src/test_support.rs new file mode 100644 index 000000000..09cca93ce --- /dev/null +++ b/lib/components/fabro-github/src/test_support.rs @@ -0,0 +1,26 @@ +use std::sync::Arc; + +use crate::InstallationToken; +use crate::token_source::{InstallationTokenMinter as InnerMinter, InstallationTokenSource}; + +#[async_trait::async_trait] +pub trait InstallationTokenMinter: Send + Sync { + async fn mint(&self) -> anyhow::Result; +} + +struct TestMinterAdapter(Arc); + +#[async_trait::async_trait] +impl InnerMinter for TestMinterAdapter { + async fn mint(&self) -> anyhow::Result { + self.0.mint().await + } +} + +#[must_use] +pub fn installation_token_source( + repo: impl Into, + minter: Arc, +) -> Arc { + InstallationTokenSource::with_minter(repo.into(), Box::new(TestMinterAdapter(minter))) +} diff --git a/lib/components/fabro-github/src/token_source.rs b/lib/components/fabro-github/src/token_source.rs index 11743e077..5af872af6 100644 --- a/lib/components/fabro-github/src/token_source.rs +++ b/lib/components/fabro-github/src/token_source.rs @@ -1,12 +1,10 @@ //! Cached GitHub installation-token source. //! -//! One [`InstallationTokenSource`] serves every GitHub-token consumer for an -//! origin repository — the clone-based sandbox providers and the run-metadata -//! writer share a single source, so "reuse a token until near expiry" is the -//! default behavior instead of a per-call-site special case. Reusing mature -//! tokens keeps consumers out of GitHub's token-replication lag window, where -//! a token minted milliseconds earlier is rejected with 404 "Repository not -//! found" or an authentication failure. +//! One [`InstallationTokenSource`] can serve GitHub-token consumers that share +//! a repository and permission scope. Reusing mature tokens keeps consumers +//! out of GitHub's token-replication lag window, where a token minted +//! milliseconds earlier is rejected with 404 "Repository not found" or an +//! authentication failure. //! //! The source also reports *provenance*: when it minted the token it returned, //! and which mint generation it belongs to. Retry classification, logging, and @@ -138,7 +136,7 @@ pub struct ResolvedToken { /// Mints installation tokens for [`InstallationTokenSource`]. Abstracted so /// tests can script mint results without HTTP. #[async_trait::async_trait] -pub trait InstallationTokenMinter: Send + Sync { +pub(crate) trait InstallationTokenMinter: Send + Sync { async fn mint(&self) -> anyhow::Result; } @@ -228,6 +226,16 @@ impl InstallationTokenSource { let normalized = crate::normalize_repo_origin_url(origin_url); let (owner, repo) = crate::parse_github_owner_repo(&normalized) .context("parsing GitHub origin for token source")?; + Self::for_repository(creds, owner, repo, permissions) + } + + /// Build a source for an already parsed GitHub repository. + pub fn for_repository( + creds: &GitHubCredentials, + owner: String, + repo: String, + permissions: serde_json::Value, + ) -> anyhow::Result> { let repo_display = format!("{owner}/{repo}"); let state = match creds { GitHubCredentials::Pat(token) => SourceState::Pat(SecretString::new(token.clone())), @@ -255,9 +263,28 @@ impl InstallationTokenSource { })) } - /// Build a minting source over a custom minter. For tests. + /// Build a source for a personal access token. #[must_use] - pub fn with_minter(repo: String, minter: Box) -> Arc { + pub fn pat(token: String) -> Arc { + Arc::new(Self { + repo: String::new(), + state: SourceState::Pat(SecretString::new(token)), + }) + } + + /// Build a source for a pre-minted installation token. + #[must_use] + pub fn installation(token: InstallationToken) -> Arc { + Arc::new(Self { + repo: String::new(), + state: SourceState::Installation(token), + }) + } + + /// Build a minting source over a custom minter. + #[cfg(any(test, feature = "test-support"))] + #[must_use] + pub(crate) fn with_minter(repo: String, minter: Box) -> Arc { Arc::new(Self { repo, state: SourceState::App { @@ -296,7 +323,27 @@ impl InstallationTokenSource { return Ok(resolved); } } - self.mint_locked(minter.as_ref(), &mut cache).await + match self.mint_locked(minter.as_ref(), &mut cache).await { + Ok(resolved) => Ok(resolved), + Err(err) => { + if let Some(cached) = cache.as_ref() { + if cached.token.valid_token().is_ok() { + tracing::warn!( + error = %format!("{err:#}"), + repo = %self.repo, + generation = cached.generation, + expires_at = %cached.token.expires_at, + "GitHub installation token refresh failed; using cached token" + ); + return Ok(cached.resolved(TokenProvenance::Reused { + minted_at: cached.minted_at, + expires_at: cached.token.expires_at, + })); + } + } + Err(err) + } + } } } } @@ -517,6 +564,26 @@ mod tests { assert_eq!(second.token.expose(), "ghs_gen2"); } + #[tokio::test] + async fn resolve_uses_a_valid_cached_token_when_refresh_fails() { + let (source, minter) = mintable(vec![ + MintAction::Token("ghs_gen1", Utc::now() + chrono::Duration::minutes(5)), + MintAction::Error("mint failed"), + ]); + + let first = source.resolve().await.unwrap(); + let second = source.resolve().await.unwrap(); + + assert_eq!(minter.calls(), 2); + assert_eq!(first.snapshot.generation, 1); + assert_eq!(second.snapshot.generation, 1); + assert!(matches!( + second.snapshot.provenance, + TokenProvenance::Reused { .. } + )); + assert_eq!(second.token.expose(), "ghs_gen1"); + } + #[tokio::test] async fn concurrent_resolves_share_one_generation() { // Single mint in the script: a second mint would panic on an empty diff --git a/lib/components/fabro-sandbox/Cargo.toml b/lib/components/fabro-sandbox/Cargo.toml index 7153e3ea1..251e6bd77 100644 --- a/lib/components/fabro-sandbox/Cargo.toml +++ b/lib/components/fabro-sandbox/Cargo.toml @@ -64,6 +64,7 @@ futures-util = { workspace = true, optional = true } rustls = { version = "0.23", default-features = false, features = ["std", "ring"], optional = true } [dev-dependencies] +fabro-github = { path = "../fabro-github", features = ["test-support"] } tokio = { workspace = true, features = ["test-util", "macros"] } tempfile = "3" serde_json.workspace = true diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 79658120e..e78877ef7 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -336,7 +336,6 @@ pub struct DaytonaSandbox { config: DaytonaConfig, client: daytona_sdk::Client, api_key: Option, - github_app: Option, push_credentials: PushCredentialState, sandbox: OnceCell, snapshot_name: OnceCell, @@ -379,7 +378,6 @@ impl DaytonaSandbox { config, client, api_key, - github_app, push_credentials, sandbox: OnceCell::new(), snapshot_name: OnceCell::new(), @@ -432,7 +430,6 @@ impl DaytonaSandbox { config: DaytonaConfig::default(), client, api_key, - github_app: None, push_credentials: PushCredentialState::new(None), sandbox: sandbox_cell, snapshot_name: OnceCell::new(), @@ -1072,10 +1069,11 @@ impl Sandbox for DaytonaSandbox { // against the clone token instead of believing nothing was // ever embedded. let resolved_token = match self.push_credentials.source() { - Some(source) => Some(source.mint_for_clone().await.map_err(|e| { - let err = crate::Error::message(format!( - "Failed to get GitHub App credentials for clone: {e}" - )); + Some(source) => Some(source.mint_for_clone().await.map_err(|source| { + let err = crate::Error::context_anyhow( + "Failed to get GitHub App credentials for clone", + source, + ); self.emit(SandboxEvent::GitCloneFailed { url: origin_url.clone(), error: err.to_string(), @@ -1295,7 +1293,7 @@ impl Sandbox for DaytonaSandbox { } Err(e) => { tracing::warn!( - origin = %origin_url, + origin = %fabro_redact::redacted_url_for_log(&origin_url), error = %e, "Failed to build authenticated origin URL — \ subsequent git push from this sandbox will fail" @@ -1304,7 +1302,7 @@ impl Sandbox for DaytonaSandbox { } } } - Err(e) if self.github_app.is_none() => { + Err(e) if self.push_credentials.source().is_none() => { let err = crate::Error::context( "Git clone failed. If this is a private repository, \ configure a GitHub App with `fabro install` and install it \ @@ -1554,9 +1552,10 @@ impl Sandbox for DaytonaSandbox { let result = self .exec_command(&cmd, 10_000, None, None, None) .await - .map_err(|_| { - crate::Error::message( - "Failed to refresh push credentials: set_url_exec_failed", + .map_err(|err| { + crate::Error::context( + "Failed to refresh push credentials: set origin URL", + err, ) })?; if !result.is_success() { @@ -2762,7 +2761,6 @@ mod tests { config, client, api_key: Some(api_key.to_string()), - github_app: None, push_credentials: PushCredentialState::new(None), sandbox: OnceCell::new(), snapshot_name: OnceCell::new(), diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index 532e7cbb2..70c4da230 100644 --- a/lib/components/fabro-sandbox/src/docker.rs +++ b/lib/components/fabro-sandbox/src/docker.rs @@ -133,7 +133,6 @@ impl Default for DockerSandboxOptions { pub struct DockerSandbox { docker: Docker, config: DockerSandboxOptions, - github_app: Option, push_credentials: PushCredentialState, run_id: Option, clone_origin_url: Option, @@ -164,7 +163,7 @@ enum ContainerStartAction { impl DockerSandbox { pub fn new( config: DockerSandboxOptions, - github_app: Option, + github_app: Option<&GitHubCredentials>, run_id: Option, clone_origin_url: Option, clone_branch: Option, @@ -183,19 +182,18 @@ impl DockerSandbox { fn with_docker_client( docker: Docker, config: DockerSandboxOptions, - github_app: Option, + github_app: Option<&GitHubCredentials>, run_id: Option, clone_origin_url: Option, clone_branch: Option, ) -> crate::Result { let push_credentials = PushCredentialState::new(push_credentials::build_token_source( - github_app.as_ref(), + github_app, clone_origin_url.as_deref(), )?); Ok(Self { docker, config, - github_app, push_credentials, run_id, clone_origin_url, @@ -724,7 +722,7 @@ impl DockerSandbox { ) -> crate::Error { let error = result .into_exec_error_with_redactor("git clone", |output| redact_auth_url(output, auth_url)); - let message = if self.github_app.is_none() { + let message = if self.push_credentials.source().is_none() { "Git clone failed. If this is a private repository, configure a GitHub App with \ `fabro install` and install it for your organization." } else { @@ -753,10 +751,8 @@ impl DockerSandbox { // the shared source, so the first refresh compares against the clone // token instead of believing nothing was ever embedded. let resolved_token = match self.push_credentials.source() { - Some(source) => Some(source.mint_for_clone().await.map_err(|e| { - crate::Error::message(format!( - "Failed to get GitHub App credentials for clone: {e}" - )) + Some(source) => Some(source.mint_for_clone().await.map_err(|err| { + crate::Error::context_anyhow("Failed to get GitHub App credentials for clone", err) })?), None => None, }; @@ -767,10 +763,11 @@ impl DockerSandbox { let auth_url = match &resolved_token { Some(token) => Some( fabro_github::embed_token_in_url(&origin_url, token.token.expose()).map_err( - |e| { - crate::Error::message(format!( - "Failed to get GitHub App credentials for clone: {e}" - )) + |err| { + crate::Error::context_anyhow( + "Failed to build authenticated GitHub clone URL", + err, + ) }, )?, ), diff --git a/lib/components/fabro-sandbox/src/error.rs b/lib/components/fabro-sandbox/src/error.rs index 8d6bf0d73..76f096d4b 100644 --- a/lib/components/fabro-sandbox/src/error.rs +++ b/lib/components/fabro-sandbox/src/error.rs @@ -18,6 +18,13 @@ pub enum Error { source: Box, }, + #[error("{message}")] + AnyhowContext { + message: String, + #[source] + source: anyhow::Error, + }, + #[cfg(feature = "docker")] #[error("Failed to connect to Docker daemon")] DockerConnect { @@ -68,6 +75,13 @@ impl Error { } } + pub fn context_anyhow(message: impl Into, source: anyhow::Error) -> Self { + Self::AnyhowContext { + message: message.into(), + source, + } + } + pub fn exec(label: impl Into, result: ExecResult) -> Self { Self::Exec { label: label.into(), diff --git a/lib/components/fabro-sandbox/src/provider/docker.rs b/lib/components/fabro-sandbox/src/provider/docker.rs index f1f8bb725..865e361be 100644 --- a/lib/components/fabro-sandbox/src/provider/docker.rs +++ b/lib/components/fabro-sandbox/src/provider/docker.rs @@ -98,8 +98,13 @@ impl SandboxProvider for DockerSandboxProvider { )); }; - let sandbox = - DockerSandbox::new(config, github_app, run_id, clone_origin_url, clone_branch)?; + let sandbox = DockerSandbox::new( + config, + github_app.as_ref(), + run_id, + clone_origin_url, + clone_branch, + )?; sandbox.initialize().await?; let container_id = sandbox.container_identifier()?.to_string(); self.get(&container_id).await?.ok_or_else(|| { diff --git a/lib/components/fabro-sandbox/src/push_credentials.rs b/lib/components/fabro-sandbox/src/push_credentials.rs index 90116317f..2c0496544 100644 --- a/lib/components/fabro-sandbox/src/push_credentials.rs +++ b/lib/components/fabro-sandbox/src/push_credentials.rs @@ -15,7 +15,7 @@ use fabro_github::token_source::{InstallationTokenSource, ResolvedToken}; use fabro_redact::DisplaySafeUrl; use tokio::sync::Mutex; -use crate::sandbox::{RefreshOutcome, RemoteCredentialAction}; +use crate::sandbox::RefreshOutcome; /// Build the shared installation-token source for a clone-based sandbox. /// @@ -33,18 +33,19 @@ pub(crate) fn build_token_source( return Ok(None); }; let normalized = fabro_github::normalize_repo_origin_url(origin_url); - if fabro_github::parse_github_owner_repo(&normalized).is_err() { + let Ok((owner, repo)) = fabro_github::parse_github_owner_repo(&normalized) else { // Non-GitHub origins never clone in these providers, so there is no // remote to keep credentials fresh for. return Ok(None); - } - InstallationTokenSource::for_origin( + }; + InstallationTokenSource::for_repository( creds, - &normalized, + owner, + repo, serde_json::json!({ "contents": "write" }), ) .map(Some) - .map_err(|err| crate::Error::message(format!("Failed to build GitHub token source: {err:#}"))) + .map_err(|err| crate::Error::context_anyhow("Failed to build GitHub token source", err)) } /// Push-credential state one provider instance tracks for its `origin` @@ -122,8 +123,9 @@ impl PushCredentialState { "GitHub token refresh failed and no credentials were ever embedded" ); } - return Err(crate::Error::message( - "Failed to refresh push credentials: token_mint_failed", + return Err(crate::Error::context_anyhow( + "Failed to refresh push credentials", + err, )); } }; @@ -131,22 +133,16 @@ impl PushCredentialState { .as_ref() .is_some_and(|prev| prev.snapshot.generation == resolved.snapshot.generation) { - return Ok(RefreshOutcome { - action: RemoteCredentialAction::Unchanged, - token: Some(resolved.snapshot), - }); + return Ok(RefreshOutcome::unchanged(resolved.snapshot)); } let auth_url = fabro_github::embed_token_in_url(origin_url, resolved.token.expose()) .map_err(|err| { - crate::Error::message(format!("Failed to build authenticated origin URL: {err:#}")) + crate::Error::context_anyhow("Failed to build authenticated origin URL", err) })?; set_url(auth_url).await?; let snapshot = resolved.snapshot; *embedded = Some(resolved); - Ok(RefreshOutcome { - action: RemoteCredentialAction::Embedded, - token: Some(snapshot), - }) + Ok(RefreshOutcome::embedded(snapshot)) } } @@ -156,9 +152,10 @@ mod tests { use chrono::Utc; use fabro_github::InstallationToken; - use fabro_github::token_source::InstallationTokenMinter; + use fabro_github::test_support::{InstallationTokenMinter, installation_token_source}; use super::*; + use crate::sandbox::RemoteCredentialAction; struct FixedMinter { calls: AtomicUsize, @@ -186,9 +183,9 @@ mod tests { } fn minting_state(ttl: chrono::Duration) -> PushCredentialState { - PushCredentialState::new(Some(InstallationTokenSource::with_minter( - "owner/repo".to_string(), - Box::new(FixedMinter { + PushCredentialState::new(Some(installation_token_source( + "owner/repo", + Arc::new(FixedMinter { calls: AtomicUsize::new(0), ttl, }), @@ -204,8 +201,7 @@ mod tests { .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) .await .unwrap(); - assert_eq!(outcome.action, RemoteCredentialAction::None); - assert_eq!(outcome.token, None); + assert_eq!(outcome, RefreshOutcome::none()); } #[tokio::test] @@ -221,8 +217,8 @@ mod tests { }) .await .unwrap(); - assert_eq!(first.action, RemoteCredentialAction::Embedded); - assert_eq!(first.token.unwrap().generation, 1); + assert_eq!(first.action(), RemoteCredentialAction::Embedded); + assert_eq!(first.token().unwrap().generation, 1); // The cached token is fresh, so the second refresh must skip set-url. let second = state @@ -232,8 +228,8 @@ mod tests { }) .await .unwrap(); - assert_eq!(second.action, RemoteCredentialAction::Unchanged); - assert_eq!(second.token.unwrap().generation, 1); + assert_eq!(second.action(), RemoteCredentialAction::Unchanged); + assert_eq!(second.token().unwrap().generation, 1); assert_eq!(set_url_calls.load(Ordering::SeqCst), 1); } @@ -258,9 +254,9 @@ mod tests { .await .unwrap(); - assert_eq!(first.token.unwrap().generation, 1); - assert_eq!(second.action, RemoteCredentialAction::Embedded); - assert_eq!(second.token.unwrap().generation, 2); + assert_eq!(first.token().unwrap().generation, 1); + assert_eq!(second.action(), RemoteCredentialAction::Embedded); + assert_eq!(second.token().unwrap().generation, 2); assert_eq!(set_url_calls.load(Ordering::SeqCst), 2); } @@ -274,8 +270,8 @@ mod tests { .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) .await .unwrap(); - assert_eq!(outcome.action, RemoteCredentialAction::Unchanged); - assert_eq!(outcome.token.unwrap().generation, 1); + assert_eq!(outcome.action(), RemoteCredentialAction::Unchanged); + assert_eq!(outcome.token().unwrap().generation, 1); } #[tokio::test] @@ -293,8 +289,8 @@ mod tests { // The generation was not recorded, so the retry embeds again instead // of wrongly skipping. let retried = state.refresh(ORIGIN, |_| async { Ok(()) }).await.unwrap(); - assert_eq!(retried.action, RemoteCredentialAction::Embedded); - assert_eq!(retried.token.unwrap().generation, 1); + assert_eq!(retried.action(), RemoteCredentialAction::Embedded); + assert_eq!(retried.token().unwrap().generation, 1); } #[tokio::test] @@ -313,22 +309,25 @@ mod tests { .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) .await .unwrap(); - assert_eq!(outcome.action, RemoteCredentialAction::Unchanged); - assert!(outcome.token.unwrap().is_static()); + assert_eq!(outcome.action(), RemoteCredentialAction::Unchanged); + assert!(outcome.token().unwrap().is_static()); } #[tokio::test] - async fn mint_failure_maps_to_the_token_mint_failed_error() { - let state = PushCredentialState::new(Some(InstallationTokenSource::with_minter( - "owner/repo".to_string(), - Box::new(FailingMinter), + async fn mint_failure_preserves_the_mint_error_chain() { + let state = PushCredentialState::new(Some(installation_token_source( + "owner/repo", + Arc::new(FailingMinter), ))); let err = state .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) .await .unwrap_err(); - assert!(err.to_string().contains("token_mint_failed"), "{err}"); + assert_eq!(err.causes(), vec![ + "minting GitHub installation access token", + "mint failed" + ]); } #[test] diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index fb8cb0102..94def6760 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -1038,22 +1038,48 @@ pub enum RemoteCredentialAction { None, } -/// Outcome of [`Sandbox::refresh_push_credentials`]: what this call did to the -/// remote, and the non-secret description of the token embedded in it. -/// `token` is `None` only when `action` is [`RemoteCredentialAction::None`]. +/// Outcome of [`Sandbox::refresh_push_credentials`]. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct RefreshOutcome { - pub action: RemoteCredentialAction, - pub token: Option, +pub enum RefreshOutcome { + /// No managed credentials exist for this sandbox. + None, + /// The remote already carried this token generation. + Unchanged(TokenSnapshot), + /// The remote was updated to carry this token generation. + Embedded(TokenSnapshot), } impl RefreshOutcome { /// No managed credentials to refresh. #[must_use] - pub fn none() -> Self { - Self { - action: RemoteCredentialAction::None, - token: None, + pub const fn none() -> Self { + Self::None + } + + #[must_use] + pub const fn unchanged(token: TokenSnapshot) -> Self { + Self::Unchanged(token) + } + + #[must_use] + pub const fn embedded(token: TokenSnapshot) -> Self { + Self::Embedded(token) + } + + #[must_use] + pub const fn action(self) -> RemoteCredentialAction { + match self { + Self::None => RemoteCredentialAction::None, + Self::Unchanged(_) => RemoteCredentialAction::Unchanged, + Self::Embedded(_) => RemoteCredentialAction::Embedded, + } + } + + #[must_use] + pub const fn token(self) -> Option { + match self { + Self::None => None, + Self::Unchanged(token) | Self::Embedded(token) => Some(token), } } } @@ -1550,17 +1576,17 @@ pub(crate) async fn fetch_source_run_ref( pub async fn git_push_via_exec(sandbox: &dyn Sandbox, refspec: &str) -> crate::Result<()> { let token = match sandbox.refresh_push_credentials().await { Ok(outcome) => { - if let Some(token) = outcome.token { + if let Some(token) = outcome.token() { tracing::debug!( refspec = %refspec, - action = %outcome.action, + action = %outcome.action(), generation = token.generation, provenance = %token.provenance, token_age_ms = token.age_ms(), "Resolved push credentials before git push" ); } - outcome.token + outcome.token() } Err(e) => { // The provider logged which token stays embedded; the push diff --git a/lib/components/fabro-sandbox/src/sandbox_spec.rs b/lib/components/fabro-sandbox/src/sandbox_spec.rs index b6a56bd40..7fcc7232a 100644 --- a/lib/components/fabro-sandbox/src/sandbox_spec.rs +++ b/lib/components/fabro-sandbox/src/sandbox_spec.rs @@ -205,7 +205,7 @@ impl SandboxSpec { } => { let mut sandbox = DockerSandbox::new( config.clone(), - github_app.clone(), + github_app.as_ref(), *run_id, clone_origin_url.clone(), clone_branch.clone(), diff --git a/lib/components/fabro-workflow/Cargo.toml b/lib/components/fabro-workflow/Cargo.toml index b3ab70260..0024c8b79 100644 --- a/lib/components/fabro-workflow/Cargo.toml +++ b/lib/components/fabro-workflow/Cargo.toml @@ -76,6 +76,7 @@ toml.workspace = true fabro-vault = { path = "../../foundation/fabro-vault" } [dev-dependencies] fabro-auth = { path = "../../foundation/fabro-auth", features = ["test-support"] } +fabro-github = { path = "../fabro-github", features = ["test-support"] } base64.workspace = true fabro-acp = { path = "../fabro-acp", features = ["test-support"] } fabro-workflow = { path = ".", features = ["test-support"] } diff --git a/lib/components/fabro-workflow/src/github_token_source.rs b/lib/components/fabro-workflow/src/github_token_source.rs deleted file mode 100644 index 2abf95bd3..000000000 --- a/lib/components/fabro-workflow/src/github_token_source.rs +++ /dev/null @@ -1,274 +0,0 @@ -use std::sync::Arc; -use std::time::Duration; - -use anyhow::Context as _; -use fabro_github::{GitHubAppCredentials, InstallationToken}; -use tokio::sync::Mutex; -use tracing::warn; - -const REFRESH_THRESHOLD: Duration = Duration::from_mins(15); - -#[async_trait::async_trait] -pub trait IatMinter: Send + Sync { - async fn mint(&self) -> anyhow::Result; -} - -pub struct AppIatMinter { - creds: GitHubAppCredentials, - http: fabro_http::HttpClient, - owner: String, - repo: String, - api_base: String, - install_url: Option, - permissions: serde_json::Value, -} - -impl AppIatMinter { - #[must_use] - pub fn new( - creds: GitHubAppCredentials, - http: fabro_http::HttpClient, - owner: String, - repo: String, - api_base: String, - install_url: Option, - permissions: serde_json::Value, - ) -> Self { - Self { - creds, - http, - owner, - repo, - api_base, - install_url, - permissions, - } - } -} - -#[async_trait::async_trait] -impl IatMinter for AppIatMinter { - async fn mint(&self) -> anyhow::Result { - self.creds - .mint_installation_token( - &self.http, - &self.owner, - &self.repo, - &self.api_base, - self.permissions.clone(), - self.install_url.as_deref(), - ) - .await - } -} - -pub struct GitHubTokenSource { - state: SourceState, -} - -enum SourceState { - Pat(String), - StaticIat(InstallationToken), - Mintable { - minter: Arc, - cache: Mutex>, - }, -} - -impl GitHubTokenSource { - #[must_use] - pub fn pat(token: String) -> Self { - Self { - state: SourceState::Pat(token), - } - } - - #[must_use] - pub fn static_iat(token: InstallationToken) -> Self { - Self { - state: SourceState::StaticIat(token), - } - } - - #[must_use] - pub fn mintable(minter: Arc) -> Self { - Self { - state: SourceState::Mintable { - minter, - cache: Mutex::new(None), - }, - } - } - - #[must_use] - pub fn is_refreshable(&self) -> bool { - matches!(self.state, SourceState::Mintable { .. }) - } - - pub async fn current_token(&self) -> anyhow::Result { - match &self.state { - SourceState::Pat(token) => Ok(token.clone()), - SourceState::StaticIat(token) => token.valid_token().map(str::to_owned), - SourceState::Mintable { minter, cache } => { - let mut cache = cache.lock().await; - let cached_is_fresh = cache - .as_ref() - .is_some_and(|token| !token.near_expiry(REFRESH_THRESHOLD)); - - if !cached_is_fresh { - match minter.mint().await { - Ok(token) => *cache = Some(token), - Err(err) => { - if let Some(token) = cache.as_ref() { - if let Ok(value) = token.valid_token() { - warn!( - error = %err, - "GitHub installation token refresh failed; using cached token" - ); - return Ok(value.to_owned()); - } - } - return Err(err) - .context("failed to mint GitHub installation access token"); - } - } - } - - let token = cache - .as_ref() - .ok_or_else(|| anyhow::anyhow!("mintable token source has no cached token"))?; - token.valid_token().map(str::to_owned) - } - } - } -} - -#[cfg(test)] -mod tests { - use std::collections::VecDeque; - use std::sync::atomic::{AtomicUsize, Ordering}; - - use anyhow::anyhow; - - use super::*; - - enum MintAction { - Token(&'static str, chrono::DateTime), - Error(&'static str), - } - - struct MockMinter { - calls: AtomicUsize, - script: Mutex>, - } - - impl MockMinter { - fn new(script: Vec) -> Self { - Self { - calls: AtomicUsize::new(0), - script: Mutex::new(script.into()), - } - } - - fn calls(&self) -> usize { - self.calls.load(Ordering::SeqCst) - } - } - - #[async_trait::async_trait] - impl IatMinter for MockMinter { - async fn mint(&self) -> anyhow::Result { - self.calls.fetch_add(1, Ordering::SeqCst); - match self.script.lock().await.pop_front().expect("mint script") { - MintAction::Token(token, expires_at) => Ok(InstallationToken { - token: token.to_string(), - expires_at, - }), - MintAction::Error(message) => Err(anyhow!(message)), - } - } - } - - #[tokio::test] - async fn pat_returns_same_token_without_minting() { - let source = GitHubTokenSource::pat("ghp_pat".to_string()); - - assert_eq!(source.current_token().await.unwrap(), "ghp_pat"); - assert_eq!(source.current_token().await.unwrap(), "ghp_pat"); - assert!(!source.is_refreshable()); - } - - #[tokio::test] - async fn static_iat_returns_valid_token_and_rejects_expired_token() { - let valid = GitHubTokenSource::static_iat(InstallationToken { - token: "ghs_valid".to_string(), - expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), - }); - assert_eq!(valid.current_token().await.unwrap(), "ghs_valid"); - assert!(!valid.is_refreshable()); - - let expired = GitHubTokenSource::static_iat(InstallationToken { - token: "ghs_expired".to_string(), - expires_at: chrono::Utc::now() - chrono::Duration::seconds(1), - }); - assert!(expired.current_token().await.is_err()); - } - - #[tokio::test] - async fn mintable_reuses_cached_token_until_refresh_threshold() { - let minter = Arc::new(MockMinter::new(vec![MintAction::Token( - "ghs_cached", - chrono::Utc::now() + chrono::Duration::minutes(30), - )])); - let source = GitHubTokenSource::mintable(minter.clone()); - - assert!(source.is_refreshable()); - assert_eq!(source.current_token().await.unwrap(), "ghs_cached"); - assert_eq!(source.current_token().await.unwrap(), "ghs_cached"); - assert_eq!(minter.calls(), 1); - } - - #[tokio::test] - async fn mintable_refreshes_cached_token_near_expiry() { - let minter = Arc::new(MockMinter::new(vec![ - MintAction::Token( - "ghs_first", - chrono::Utc::now() + chrono::Duration::minutes(10), - ), - MintAction::Token( - "ghs_second", - chrono::Utc::now() + chrono::Duration::minutes(30), - ), - ])); - let source = GitHubTokenSource::mintable(minter.clone()); - - assert_eq!(source.current_token().await.unwrap(), "ghs_first"); - assert_eq!(source.current_token().await.unwrap(), "ghs_second"); - assert_eq!(minter.calls(), 2); - } - - #[tokio::test] - async fn mintable_uses_valid_cached_token_when_refresh_fails() { - let minter = Arc::new(MockMinter::new(vec![ - MintAction::Token( - "ghs_cached", - chrono::Utc::now() + chrono::Duration::minutes(10), - ), - MintAction::Error("mint failed"), - ])); - let source = GitHubTokenSource::mintable(minter.clone()); - - assert_eq!(source.current_token().await.unwrap(), "ghs_cached"); - assert_eq!(source.current_token().await.unwrap(), "ghs_cached"); - assert_eq!(minter.calls(), 2); - } - - #[tokio::test] - async fn mintable_errors_when_no_cached_token_can_cover_mint_failure() { - let minter = Arc::new(MockMinter::new(vec![MintAction::Error("mint failed")])); - let source = GitHubTokenSource::mintable(minter); - - let err = format!("{:#}", source.current_token().await.unwrap_err()); - assert!(err.contains("mint failed"), "got: {err}"); - } -} diff --git a/lib/components/fabro-workflow/src/handler/command.rs b/lib/components/fabro-workflow/src/handler/command.rs index d36ef8be8..82593c59b 100644 --- a/lib/components/fabro-workflow/src/handler/command.rs +++ b/lib/components/fabro-workflow/src/handler/command.rs @@ -1693,7 +1693,7 @@ mod tests { } #[async_trait::async_trait] - impl crate::github_token_source::IatMinter for RefreshingMinter { + impl fabro_github::test_support::InstallationTokenMinter for RefreshingMinter { async fn mint(&self) -> anyhow::Result { let call = self.calls.fetch_add(1, std::sync::atomic::Ordering::SeqCst) + 1; Ok(fabro_github::InstallationToken { @@ -1834,8 +1834,9 @@ mod tests { calls: std::sync::atomic::AtomicUsize::new(0), }); let mut services = make_sandbox_services(spy.clone()); - services.github_token = Some(std::sync::Arc::new( - crate::github_token_source::GitHubTokenSource::mintable(minter.clone()), + services.github_token = Some(fabro_github::test_support::installation_token_source( + "owner/repo", + minter.clone(), )); let handler = CommandHandler; diff --git a/lib/components/fabro-workflow/src/handler/llm/acp.rs b/lib/components/fabro-workflow/src/handler/llm/acp.rs index 4edcf3e81..cfd498beb 100644 --- a/lib/components/fabro-workflow/src/handler/llm/acp.rs +++ b/lib/components/fabro-workflow/src/handler/llm/acp.rs @@ -11,8 +11,7 @@ use fabro_acp::{ render_stop_reason, }; use fabro_agent::{ - AgentEvent, RefreshOutcome, RemoteCredentialAction, Sandbox, StaticEnvProvider, SteeringItem, - ToolEnvProvider, + AgentEvent, RefreshOutcome, Sandbox, StaticEnvProvider, SteeringItem, ToolEnvProvider, }; use fabro_github::token_source::REFRESH_MARGIN; use fabro_graphviz::graph::Node; @@ -115,12 +114,8 @@ fn push_cred_refresh_interval() -> Option { /// tick. Schedule from the token's own `expires_at` instead: wake when the /// cache margin opens, so that tick re-mints. `None` disables the loop — /// static credentials cannot be re-minted by waiting. -fn next_refresh_delay(outcome: &RefreshOutcome, fallback: Duration) -> Option { - let Some(token) = outcome.token else { - // No managed credentials to watch; keep the configured cadence in - // case a later tick sees them (e.g. after a reconnect). - return Some(fallback); - }; +fn next_refresh_delay(outcome: &RefreshOutcome) -> Option { + let token = outcome.token()?; let expires_at = token.expires_at()?; let margin = chrono::Duration::from_std(REFRESH_MARGIN).unwrap_or(chrono::Duration::MAX); let until_margin = ((expires_at - margin) - chrono::Utc::now()) @@ -140,9 +135,10 @@ async fn refresh_ahead_loop( sandbox: Arc, cancel: CancellationToken, interval: Duration, + initial_delay: Duration, ) { let retry_delay = interval.min(Duration::from_mins(1)); - let mut delay = interval; + let mut delay = initial_delay; loop { tokio::select! { () = cancel.cancelled() => break, @@ -151,26 +147,26 @@ async fn refresh_ahead_loop( .await { Ok(Ok(outcome)) => { - match outcome.action { - RemoteCredentialAction::Embedded => { + match outcome { + RefreshOutcome::Embedded(token) => { tracing::info!( - generation = outcome.token.map(|token| token.generation), + generation = token.generation, "refresh-ahead re-embedded push credentials mid-turn" ); } - RemoteCredentialAction::Unchanged => { + RefreshOutcome::Unchanged(token) => { tracing::debug!( - generation = outcome.token.map(|token| token.generation), + generation = token.generation, "refresh-ahead tick: embedded push credentials still fresh" ); } - RemoteCredentialAction::None => { + RefreshOutcome::None => { tracing::debug!( "refresh-ahead tick: no managed push credentials to refresh" ); } } - if let Some(next) = next_refresh_delay(&outcome, interval) { + if let Some(next) = next_refresh_delay(&outcome) { delay = next; } else { tracing::debug!( @@ -312,77 +308,57 @@ impl AgentAcpBackend { }) as Arc) + Send + Sync> }); - // Keep the sandbox's push credentials fresh for the duration of this ACP - // turn so the agent's own `git push` uses a live token instead of the one - // baked into the clone at run start. - // - // Part 2 (turn-entry): resolve through the cached token source and - // rewrite the origin URL before the ACP process spawns, covering a push - // early in the turn. A fresh cached token makes this a no-op exec-wise. - // Non-fatal and timeout-bounded — a stalled mint must neither fail nor - // hang node entry. Part 3 (loop): a background task keeps the embedded - // token fresh so a single turn that outlives the ~60-min - // installation-token TTL still pushes with a fresh token; ticks - // reschedule from the embedded token's expiry, so a normal short turn - // never ticks (the drop-guard aborts the task at turn end). - // - // FABRO_PUSH_CRED_REFRESH_AHEAD=0 (or false/off/no/empty, case- - // insensitive) disables the WHOLE feature — turn-entry refresh AND loop — - // so an operator who manages `origin` themselves can opt out of all - // fabro-side origin rewriting. FABRO_PUSH_CRED_REFRESH_INTERVAL_SECONDS - // overrides the loop cadence for ticks without token expiry info; 0 - // disables just the loop. - // - // Known limitations tracked as follow-ups (not addressed here): (a) - // resumed/parked runs reconnect the sandbox with no GitHub App creds, so - // refresh no-ops until those creds are threaded through the reconnect - // path; (b) the background `git remote set-url` can contend with the - // agent's own git on `.git/config.lock` (skipped entirely while the - // cached generation is already embedded); (c) parallel ACP branches each - // run their own loop; (d) this refresh lives in the ACP handler only, - // though the stale-origin problem is stage-type-agnostic (native/command - // stages that push are not covered); (e) refresh failures are logged via - // tracing but not surfaced as a RunNotice event on the run stream. + // Refresh before launch for early pushes. Schedule later refreshes from + // token expiry so the loop cannot sleep past the cache margin. let refresh_enabled = push_cred_refresh_enabled(); - if refresh_enabled { + let refresh_interval = refresh_enabled.then(push_cred_refresh_interval).flatten(); + let refresh_schedule = if refresh_enabled { match timeout(REFRESH_MINT_TIMEOUT, sandbox.refresh_push_credentials()).await { - Ok(Ok(outcome)) => match outcome.action { - RemoteCredentialAction::Embedded => { - tracing::debug!( - generation = outcome.token.map(|token| token.generation), - "refreshed sandbox push credentials at ACP turn entry" - ); + Ok(Ok(outcome)) => { + match outcome { + RefreshOutcome::Embedded(token) => { + tracing::debug!( + generation = token.generation, + "refreshed sandbox push credentials at ACP turn entry" + ); + } + RefreshOutcome::Unchanged(token) => { + tracing::debug!( + generation = token.generation, + "sandbox push credentials already fresh at ACP turn entry" + ); + } + RefreshOutcome::None => {} } - RemoteCredentialAction::Unchanged => { - tracing::debug!( - generation = outcome.token.map(|token| token.generation), - "sandbox push credentials already fresh at ACP turn entry" - ); - } - RemoteCredentialAction::None => {} - }, + refresh_interval.zip(next_refresh_delay(&outcome)) + } Ok(Err(e)) => { tracing::warn!( error = %fabro_sandbox::display_for_log(&e), "node-entry push-credential refresh failed (non-fatal)" ); + refresh_interval + .map(|interval| (interval, interval.min(Duration::from_mins(1)))) } Err(_elapsed) => { tracing::warn!( timeout_secs = REFRESH_MINT_TIMEOUT.as_secs(), "node-entry push-credential refresh timed out (non-fatal)" ); + refresh_interval + .map(|interval| (interval, interval.min(Duration::from_mins(1)))) } } - } - let _refresh_ahead_guard: Option = refresh_enabled - .then(push_cred_refresh_interval) - .flatten() - .map(|interval| { + } else { + None + }; + let _refresh_ahead_guard: Option = + refresh_schedule.map(|(interval, initial_delay)| { AbortOnDrop(tokio::spawn(refresh_ahead_loop( Arc::clone(sandbox), cancel_token.child_token(), interval, + initial_delay, ))) }); @@ -766,23 +742,22 @@ mod tests { expires_at, } }; - RefreshOutcome { - action, - token: Some(TokenSnapshot { - generation, - provenance, - }), + let token = TokenSnapshot { + generation, + provenance, + }; + match action { + RemoteCredentialAction::Embedded => RefreshOutcome::embedded(token), + RemoteCredentialAction::Unchanged => RefreshOutcome::unchanged(token), + RemoteCredentialAction::None => RefreshOutcome::none(), } } fn static_outcome() -> RefreshOutcome { - RefreshOutcome { - action: RemoteCredentialAction::Unchanged, - token: Some(TokenSnapshot { - generation: 0, - provenance: TokenProvenance::Static, - }), - } + RefreshOutcome::unchanged(TokenSnapshot { + generation: 0, + provenance: TokenProvenance::Static, + }) } #[test] @@ -794,7 +769,7 @@ mod tests { chrono::Duration::minutes(60), false, ); - let delay = next_refresh_delay(&outcome, Duration::from_mins(45)).unwrap(); + let delay = next_refresh_delay(&outcome).unwrap(); // Expiry minus the 10-minute refresh margin: ~50 minutes out. assert!(delay > Duration::from_mins(49), "{delay:?}"); assert!(delay <= Duration::from_mins(50), "{delay:?}"); @@ -809,26 +784,17 @@ mod tests { chrono::Duration::minutes(5), true, ); - assert_eq!( - next_refresh_delay(&outcome, Duration::from_mins(45)), - Some(REFRESH_RESCHEDULE_FLOOR) - ); + assert_eq!(next_refresh_delay(&outcome), Some(REFRESH_RESCHEDULE_FLOOR)); } #[test] fn next_refresh_delay_disables_the_loop_for_static_credentials() { - assert_eq!( - next_refresh_delay(&static_outcome(), Duration::from_mins(45)), - None - ); + assert_eq!(next_refresh_delay(&static_outcome()), None); } #[test] - fn next_refresh_delay_keeps_the_cadence_without_managed_credentials() { - assert_eq!( - next_refresh_delay(&RefreshOutcome::none(), Duration::from_mins(45)), - Some(Duration::from_mins(45)) - ); + fn next_refresh_delay_disables_the_loop_without_managed_credentials() { + assert_eq!(next_refresh_delay(&RefreshOutcome::none()), None); } /// Sandbox stub whose refresh outcomes are scripted, recording when each @@ -989,6 +955,7 @@ mod tests { Arc::clone(&sandbox) as Arc, cancel.clone(), interval, + interval, )); while sandbox.ticks().len() < 3 { @@ -1011,19 +978,41 @@ mod tests { } #[tokio::test(start_paused = true)] - async fn refresh_ahead_stops_by_itself_for_static_credentials() { - let sandbox = ScriptedRefreshSandbox::new(vec![static_outcome()]); + async fn refresh_ahead_honors_the_expiry_based_initial_delay() { + let interval = Duration::from_mins(45); + let entry_outcome = minted_outcome( + RemoteCredentialAction::Unchanged, + 1, + chrono::Duration::minutes(45), + chrono::Duration::minutes(15), + true, + ); + let initial_delay = next_refresh_delay(&entry_outcome).unwrap(); + let sandbox = ScriptedRefreshSandbox::new(vec![minted_outcome( + RemoteCredentialAction::Embedded, + 2, + chrono::Duration::zero(), + chrono::Duration::minutes(60), + false, + )]); let cancel = CancellationToken::new(); + let start = tokio::time::Instant::now(); let loop_task = tokio::spawn(refresh_ahead_loop( Arc::clone(&sandbox) as Arc, cancel.clone(), - Duration::from_mins(45), + interval, + initial_delay, )); - // The loop exits after the first tick without being cancelled: static - // credentials cannot be re-minted, so there is nothing to keep fresh. - loop_task.await.expect("refresh loop should stop by itself"); - assert_eq!(sandbox.ticks().len(), 1); + while sandbox.ticks().is_empty() { + tokio::time::sleep(Duration::from_secs(1)).await; + } + cancel.cancel(); + loop_task.await.expect("refresh loop should exit cleanly"); + + let first_tick = sandbox.ticks()[0] - start; + assert!(first_tick <= Duration::from_mins(5), "{first_tick:?}"); + assert!(first_tick > Duration::from_mins(4), "{first_tick:?}"); } #[tokio::test] diff --git a/lib/components/fabro-workflow/src/lib.rs b/lib/components/fabro-workflow/src/lib.rs index c34bec62c..3178542d7 100644 --- a/lib/components/fabro-workflow/src/lib.rs +++ b/lib/components/fabro-workflow/src/lib.rs @@ -293,7 +293,6 @@ pub mod error; pub mod event; pub mod file_resolver; pub mod git; -pub mod github_token_source; pub(crate) mod graph; pub mod handler; mod hook_context; diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index 2ccb3b6f2..4d4d529c4 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -7,6 +7,7 @@ use fabro_agent::{Sandbox, ToolSecrets}; use fabro_auth::{ CredentialSource, ExtraHeadersCredentialSource, VaultCredentialSource, auth_issue_message, }; +use fabro_github::token_source::InstallationTokenSource; use fabro_graphviz::graph; use fabro_hooks::{HookContext, HookDecision, HookEvent, HookExecutionContext, HookRunner}; use fabro_model::Catalog; @@ -23,7 +24,6 @@ use super::types::{InitOptions, Initialized, LlmSpec, Persisted, SandboxEnvSpec} use crate::error::Error; use crate::event::{Event, RunNoticeCode, RunNoticeLevel}; use crate::git::GitAuthor; -use crate::github_token_source::{AppIatMinter, GitHubTokenSource}; use crate::handler::llm::{AgentAcpBackend, AgentApiBackend, BackendRouter, routing}; use crate::handler::{HandlerRegistry, default_registry}; #[cfg(test)] @@ -37,7 +37,10 @@ use crate::services::{ use crate::stage_execution::{StageExecutionSeed, StageExecutionTracker}; use crate::steering_hub::SteeringHub; -type BuiltSandboxEnv = (HashMap, Option>); +type BuiltSandboxEnv = ( + HashMap, + Option>, +); async fn run_hooks( hook_runner: Option<&HookRunner>, @@ -99,12 +102,12 @@ fn build_sandbox_env( let source = match creds { fabro_github::GitHubCredentials::Pat(token) => { - Some(Arc::new(GitHubTokenSource::pat(token.clone()))) + Some(InstallationTokenSource::pat(token.clone())) } fabro_github::GitHubCredentials::Installation(token) => { - Some(Arc::new(GitHubTokenSource::static_iat(token.clone()))) + Some(InstallationTokenSource::installation(token.clone())) } - fabro_github::GitHubCredentials::App(app) => { + fabro_github::GitHubCredentials::App(_) => { let Some(origin_url) = spec.origin_url.as_deref() else { return Ok((env, None)); }; @@ -114,19 +117,11 @@ fn build_sandbox_env( let permissions = serde_json::to_value(permissions).map_err(|err| { Error::engine_with_source("Failed to serialize GitHub permissions", err) })?; - let http = fabro_http::http_client() - .map_err(|err| Error::engine_with_source("Failed to build HTTP client", err))?; - let install_url = app.installation_url(&owner); - let minter = AppIatMinter::new( - app.clone(), - http, - owner, - repo, - fabro_github::github_api_base_url(), - install_url, - permissions, - ); - Some(Arc::new(GitHubTokenSource::mintable(Arc::new(minter)))) + Some( + InstallationTokenSource::for_repository(creds, owner, repo, permissions).map_err( + |err| Error::engine_with_anyhow("Failed to build GitHub token source", err), + )?, + ) } }; @@ -458,7 +453,7 @@ pub async fn initialize( }); let github_token_refresh_managed = github_token .as_deref() - .is_some_and(GitHubTokenSource::is_refreshable); + .is_some_and(InstallationTokenSource::mints_installation_tokens); let (registry, effective_dry_run) = if let Some(registry) = options.registry_override.clone() { // A caller-supplied registry owns execution behavior for its handlers. (registry, options.dry_run) diff --git a/lib/components/fabro-workflow/src/run_metadata.rs b/lib/components/fabro-workflow/src/run_metadata.rs index 9fa002b57..76b9fa23d 100644 --- a/lib/components/fabro-workflow/src/run_metadata.rs +++ b/lib/components/fabro-workflow/src/run_metadata.rs @@ -222,19 +222,16 @@ pub(crate) fn build_metadata_writer( if !normalized_url.starts_with("https://") { return Ok(None); } - if fabro_github::parse_github_owner_repo(&normalized_url).is_err() { + let Ok((owner, repo)) = fabro_github::parse_github_owner_repo(&normalized_url) else { return Ok(None); - } + }; - // Share the sandbox's token source so the metadata writer reuses the - // same cached token as every other consumer for this origin. Resumed - // runs reconnect the sandbox without one; they build their own cached - // source from the run's credentials. let source = match token_source { Some(source) => source, - None => InstallationTokenSource::for_origin( + None => InstallationTokenSource::for_repository( creds, - &normalized_url, + owner, + repo, serde_json::json!({ "contents": "write" }), ) .map_err(RunMetadataError::TokenMint)?, diff --git a/lib/components/fabro-workflow/src/services.rs b/lib/components/fabro-workflow/src/services.rs index 2af76a9c7..64facc23e 100644 --- a/lib/components/fabro-workflow/src/services.rs +++ b/lib/components/fabro-workflow/src/services.rs @@ -8,6 +8,7 @@ use fabro_agent::{Sandbox, ToolEnvProvider}; use fabro_auth::CredentialSource; #[cfg(test)] use fabro_auth::ResolvedCredentials; +use fabro_github::token_source::InstallationTokenSource; use fabro_hooks::{HookContext, HookDecision, HookExecutionContext, HookRunner}; use fabro_interview::Interviewer; use fabro_model::{Catalog, ProviderId}; @@ -15,7 +16,6 @@ use fabro_types::{ManifestPath, RunId}; use tokio_util::sync::CancellationToken; use crate::event::Emitter; -use crate::github_token_source::GitHubTokenSource; use crate::handler::HandlerRegistry; use crate::interview_runtime::RunInterviewBlocker; use crate::run_metadata::{RunMetadataRuntime, RunMetadataWriterHandle}; @@ -238,7 +238,7 @@ pub struct EngineServices { /// Environment variables from `[sandbox.env]` config. pub base_env: HashMap, /// GitHub token source used to inject `GITHUB_TOKEN` at the point of use. - pub github_token: Option>, + pub github_token: Option>, /// Typed values from `[run.inputs]`, available to prompt templates. pub inputs: HashMap, /// When true, handlers should skip real execution and return simulated @@ -342,7 +342,7 @@ impl EngineServices { pub struct WorkflowToolEnvProvider { pub base_env: HashMap, - pub github_token: Option>, + pub github_token: Option>, } #[async_trait::async_trait] @@ -354,11 +354,15 @@ impl ToolEnvProvider for WorkflowToolEnvProvider { async fn resolve_workflow_env( base_env: &HashMap, - github_token: Option<&Arc>, + github_token: Option<&Arc>, ) -> anyhow::Result> { let mut env = base_env.clone(); if let Some(source) = github_token { - env.insert("GITHUB_TOKEN".to_string(), source.current_token().await?); + let resolved = source.resolve().await?; + env.insert( + "GITHUB_TOKEN".to_string(), + resolved.token.expose().to_owned(), + ); } Ok(env) } @@ -371,9 +375,10 @@ mod tests { use anyhow::anyhow; use fabro_agent::ToolEnvProvider as _; use fabro_github::InstallationToken; + use fabro_github::test_support::{InstallationTokenMinter, installation_token_source}; + use fabro_github::token_source::InstallationTokenSource; use super::{EngineServices, WorkflowToolEnvProvider}; - use crate::github_token_source::{GitHubTokenSource, IatMinter}; #[tokio::test] async fn test_default_uses_stub_credential_source() { @@ -406,7 +411,7 @@ mod tests { async fn workflow_tool_env_provider_merges_current_github_token() { let provider = WorkflowToolEnvProvider { base_env: HashMap::from([("FOO".to_string(), "bar".to_string())]), - github_token: Some(Arc::new(GitHubTokenSource::pat("ghp_pat".to_string()))), + github_token: Some(InstallationTokenSource::pat("ghp_pat".to_string())), }; let env = provider.resolve().await.unwrap(); @@ -418,7 +423,7 @@ mod tests { struct FailingMinter; #[async_trait::async_trait] - impl IatMinter for FailingMinter { + impl InstallationTokenMinter for FailingMinter { async fn mint(&self) -> anyhow::Result { Err(anyhow!("GITHUB_TOKEN refresh failed")) } @@ -428,9 +433,10 @@ mod tests { async fn workflow_tool_env_provider_propagates_token_refresh_errors() { let provider = WorkflowToolEnvProvider { base_env: HashMap::new(), - github_token: Some(Arc::new(GitHubTokenSource::mintable(Arc::new( - FailingMinter, - )))), + github_token: Some(installation_token_source( + "owner/repo", + Arc::new(FailingMinter), + )), }; let err = format!("{:#}", provider.resolve().await.unwrap_err()); From e5c0301ccb8e56a441a790db7145e0605f3fed73 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 19:57:19 -0400 Subject: [PATCH 16/19] Simplify Daytona lifecycle retries --- .../fabro-sandbox/src/daytona/mod.rs | 315 +++++++++++------- 1 file changed, 192 insertions(+), 123 deletions(-) diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 22d3adbca..8ffccfa81 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -63,7 +63,6 @@ pub(crate) const DAYTONA_DASHBOARD_SANDBOXES_URL: &str = "https://app.daytona.io/dashboard/sandboxes"; const FABRO_SANDBOX_USER_AGENT: &str = concat!("fabro-sandbox/", env!("CARGO_PKG_VERSION")); const DAYTONA_PROBE_TIMEOUT: Duration = Duration::from_secs(20); -const DAYTONA_START_TIMEOUT: Duration = Duration::from_mins(1); /// Upper bound on explicit and Drop-triggered Daytona cleanup calls (session /// deletion, temporary stdin files) so a stalled REST call cannot block /// cancellation/timeout paths indefinitely. @@ -335,6 +334,33 @@ fn command_kind(command: &str) -> &'static str { } } +#[derive(Clone, Copy, strum::Display)] +#[strum(serialize_all = "lowercase")] +enum DaytonaLifecycleAction { + Start, + Stop, +} + +impl DaytonaLifecycleAction { + async fn execute( + self, + client: &daytona_sdk::Client, + sandbox_name: &str, + ) -> Result<(), DaytonaError> { + match self { + Self::Start => client.start(sandbox_name).await.map(drop), + Self::Stop => client.stop(sandbox_name).await.map(drop), + } + } + + fn is_complete(self, state: Option) -> bool { + match self { + Self::Start => state == Some(SandboxState::Started), + Self::Stop => matches!(state, Some(SandboxState::Stopped | SandboxState::Destroyed)), + } + } +} + /// Sandbox that runs all operations inside a Daytona cloud sandbox. pub struct DaytonaSandbox { config: DaytonaConfig, @@ -859,117 +885,156 @@ impl DaytonaSandbox { ))) } - /// Start the sandbox, retrying while Daytona reports a lifecycle - /// transition in flight, up to `deadline`. - /// - /// Daytona rejects `start` with "state change in progress" while a - /// transition (such as an inactivity auto-stop) is still running. The - /// transition finishes on its own within seconds, so the rejection is a - /// wait-and-retry condition, not a failure. Between attempts the state is - /// re-inspected: a transition that lands on `Started` (a concurrent - /// activation won the race) needs no further start call. + async fn wait_for_stable_state( + &self, + sandbox_name: &str, + ) -> Result, DaytonaError> { + loop { + time::sleep(DAYTONA_STATE_CHANGE_POLL_INTERVAL).await; + let state = self.client.get(sandbox_name).await?.state; + if !is_transitional_state(state) { + return Ok(state); + } + } + } + + async fn run_lifecycle_action( + &self, + sandbox_name: &str, + action: DaytonaLifecycleAction, + deadline: time::Instant, + ) -> crate::Result<()> { + loop { + let request = time::timeout_at(deadline, action.execute(&self.client, sandbox_name)); + match request.await { + Ok(Ok(())) => return Ok(()), + Ok(Err(source)) if is_state_change_in_progress(&source) => { + tracing::debug!( + action = %action, + sandbox = sandbox_name, + "Daytona lifecycle request rejected during state change" + ); + match time::timeout_at(deadline, self.wait_for_stable_state(sandbox_name)).await + { + Ok(Ok(state)) if action.is_complete(state) => return Ok(()), + Ok(Ok(_)) => {} + Ok(Err(wait_source)) => { + return Err(crate::Error::context( + format!( + "Failed to inspect Daytona sandbox while waiting to {action}" + ), + wait_source, + )); + } + Err(_) => { + return Err(crate::Error::context( + format!("Timed out waiting to {action} Daytona sandbox"), + source, + )); + } + } + } + Ok(Err(source)) => { + return Err(crate::Error::context( + format!("Failed to {action} Daytona sandbox"), + source, + )); + } + Err(_) => { + return Err(crate::Error::message(format!( + "Timed out waiting to {action} Daytona sandbox" + ))); + } + } + } + } + + fn start_error(&self, error: crate::Error) -> crate::Result<()> { + self.emit(SandboxEvent::StartFailed { + provider: "daytona".into(), + error: error.to_string(), + causes: error.causes(), + }); + Err(error) + } + + fn stop_error(&self, error: crate::Error) -> crate::Result<()> { + self.emit(SandboxEvent::StopFailed { + provider: "daytona".into(), + error: error.to_string(), + causes: error.causes(), + }); + Err(error) + } + async fn start_with_deadline(&self, deadline: time::Instant) -> crate::Result<()> { self.emit(SandboxEvent::StartStarted { provider: "daytona".into(), }); let start = Instant::now(); - let sandbox = self.sandbox()?; - loop { - match self.client.start(&sandbox.name).await { - Ok(_) => break, - Err(e) if is_state_change_in_progress(&e) && time::Instant::now() < deadline => { - tracing::debug!( - "Daytona start rejected while a state change is in progress; retrying" - ); - time::sleep(DAYTONA_STATE_CHANGE_POLL_INTERVAL).await; - if let Ok(current) = self.client.get(&sandbox.name).await { - if current.state == Some(SandboxState::Started) { - break; - } - } - } - Err(e) => { - let err = crate::Error::context("Failed to start Daytona sandbox", e); - self.emit(SandboxEvent::StartFailed { - provider: "daytona".into(), - error: err.to_string(), - causes: err.causes(), - }); - return Err(err); - } - } + let result = async { + let sandbox = self.sandbox()?; + self.run_lifecycle_action(&sandbox.name, DaytonaLifecycleAction::Start, deadline) + .await?; + Self::probe_bash(sandbox).await } - if let Err(err) = Self::probe_bash(sandbox).await { - self.emit(SandboxEvent::StartFailed { - provider: "daytona".into(), - error: err.to_string(), - causes: err.causes(), - }); - return Err(err); + .await; + if let Err(error) = result { + return self.start_error(error); } - let duration_ms = elapsed_ms(start); self.emit(SandboxEvent::StartCompleted { - provider: "daytona".into(), - duration_ms, + provider: "daytona".into(), + duration_ms: elapsed_ms(start), }); Ok(()) } - /// Stop the sandbox, retrying while Daytona reports a lifecycle - /// transition in flight, up to `deadline`. - /// - /// The in-flight transition may be the stop itself (an inactivity - /// auto-stop): between attempts the state is re-inspected, and a sandbox - /// that landed on `Stopped` or `Destroyed` needs no further stop call. async fn stop_with_deadline(&self, deadline: time::Instant) -> crate::Result<()> { self.emit(SandboxEvent::StopStarted { provider: "daytona".into(), }); let start = Instant::now(); - let sandbox = self.sandbox()?; - loop { - match self.client.stop(&sandbox.name).await { - Ok(_) => break, - Err(e) if is_state_change_in_progress(&e) && time::Instant::now() < deadline => { - tracing::debug!( - "Daytona stop rejected while a state change is in progress; retrying" - ); - time::sleep(DAYTONA_STATE_CHANGE_POLL_INTERVAL).await; - if let Ok(current) = self.client.get(&sandbox.name).await { - if matches!( - current.state, - Some(SandboxState::Stopped | SandboxState::Destroyed) - ) { - break; - } - } - } - Err(e) => { - let err = crate::Error::context("Failed to stop Daytona sandbox", e); - self.emit(SandboxEvent::StopFailed { - provider: "daytona".into(), - error: err.to_string(), - causes: err.causes(), - }); - return Err(err); - } - } + let result = async { + let sandbox = self.sandbox()?; + self.run_lifecycle_action(&sandbox.name, DaytonaLifecycleAction::Stop, deadline) + .await + } + .await; + if let Err(error) = result { + return self.stop_error(error); } - let duration_ms = elapsed_ms(start); self.emit(SandboxEvent::StopCompleted { - provider: "daytona".into(), - duration_ms, + provider: "daytona".into(), + duration_ms: elapsed_ms(start), }); Ok(()) } } -/// Whether a Daytona API error reports a lifecycle transition in flight -/// (HTTP 400 "State change in progress" on start/stop). fn is_state_change_in_progress(err: &DaytonaError) -> bool { - err.to_string() - .to_ascii_lowercase() - .contains("state change in progress") + err.status_code() == Some(400) + && err + .message() + .to_ascii_lowercase() + .contains("state change in progress") +} + +fn is_transitional_state(state: Option) -> bool { + matches!( + state, + Some( + SandboxState::Creating + | SandboxState::Restoring + | SandboxState::Destroying + | SandboxState::Starting + | SandboxState::Stopping + | SandboxState::PendingBuild + | SandboxState::BuildingSnapshot + | SandboxState::PullingSnapshot + | SandboxState::Archiving + | SandboxState::Resizing + ) + ) } /// Detect the git remote URL and current branch from a local repository. @@ -1461,39 +1526,35 @@ impl Sandbox for DaytonaSandbox { async fn activate(&self) -> crate::Result<()> { let sandbox = self.sandbox()?; let deadline = time::Instant::now() + DAYTONA_STATE_CHANGE_TIMEOUT; - loop { - let current = self.client.get(&sandbox.name).await.map_err(|e| { + let current = time::timeout_at(deadline, self.client.get(&sandbox.name)) + .await + .map_err(|_| { + crate::Error::message("Timed out inspecting Daytona sandbox before activation") + })? + .map_err(|e| { crate::Error::context("Failed to inspect Daytona sandbox before activation", e) })?; - match current.state { - Some(SandboxState::Started) => return Ok(()), - Some(SandboxState::Starting) => { - return current - .wait_for_start(Some(DAYTONA_START_TIMEOUT)) - .await - .map_err(|e| { - crate::Error::context( - "Failed to wait for Daytona sandbox activation", - e, - ) - }); - } - // An inactivity auto-stop can be in flight when a stage - // returns after a long period with no sandbox traffic (LLM - // inference generates none). Wait out the transition and - // dispatch on whatever state it lands on. - Some(SandboxState::Stopping) => { - if time::Instant::now() >= deadline { - return Err(crate::Error::message(format!( - "Daytona sandbox stop still in progress after {}s", - DAYTONA_STATE_CHANGE_TIMEOUT.as_secs() - ))); - } - time::sleep(DAYTONA_STATE_CHANGE_POLL_INTERVAL).await; - } - _ => return self.start_with_deadline(deadline).await, - } + let state = if is_transitional_state(current.state) { + time::timeout_at(deadline, self.wait_for_stable_state(&sandbox.name)) + .await + .map_err(|_| { + crate::Error::message( + "Timed out waiting for Daytona sandbox state change before activation", + ) + })? + .map_err(|e| { + crate::Error::context( + "Failed to wait for Daytona sandbox state change before activation", + e, + ) + })? + } else { + current.state + }; + if state == Some(SandboxState::Started) { + return Ok(()); } + self.start_with_deadline(deadline).await } async fn stop(&self) -> crate::Result<()> { @@ -3316,14 +3377,15 @@ mod tests { .await .expect_err("a state change that outlives the deadline should fail"); - assert!( - start_sandbox.calls_async().await >= 2, - "start should be retried while the deadline allows" + assert_eq!( + start_sandbox.calls_async().await, + 1, + "start should not be retried while the current transition is in flight" ); assert!( - err.causes() - .iter() - .any(|cause| cause.to_ascii_lowercase().contains("state change in progress")), + err.causes().iter().any(|cause| cause + .to_ascii_lowercase() + .contains("state change in progress")), "error should carry the Daytona rejection: {err}" ); } @@ -3342,6 +3404,13 @@ mod tests { 400, "Sandbox already started" ))); + assert!(!is_state_change_in_progress(&DaytonaError::api( + 500, + "Sandbox state change in progress" + ))); + assert!(!is_state_change_in_progress(&DaytonaError::general( + "Sandbox state change in progress" + ))); } #[tokio::test] From b7e3b660ffa076e4f9897c0769897a3530eca089 Mon Sep 17 00:00:00 2001 From: Release Repro Date: Thu, 20 Aug 2026 20:34:38 -0400 Subject: [PATCH 17/19] Simplify diagnostics timeout handling --- lib/apps/fabro-server/src/diagnostics.rs | 130 ++++++------------ lib/apps/fabro-server/src/server.rs | 19 ++- .../fabro-server/src/server/handler/system.rs | 12 +- lib/components/fabro-llm/src/model_test.rs | 43 +++++- .../fabro-sandbox/src/daytona/mod.rs | 89 ++++++++++-- 5 files changed, 176 insertions(+), 117 deletions(-) diff --git a/lib/apps/fabro-server/src/diagnostics.rs b/lib/apps/fabro-server/src/diagnostics.rs index 86f625366..9373cc81e 100644 --- a/lib/apps/fabro-server/src/diagnostics.rs +++ b/lib/apps/fabro-server/src/diagnostics.rs @@ -6,7 +6,7 @@ use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use fabro_auth::auth_issue_message; use fabro_llm::client::Client as LlmClient; -use fabro_llm::model_test::{ModelTestOutcome, ModelTestStatus, run_basic_model_probe}; +use fabro_llm::model_test::{ModelTestStatus, run_basic_model_probe_with_timeout}; use fabro_model::{Catalog, ProviderId}; use fabro_redact::redact_string; use fabro_sandbox::{DockerSandboxProvider, daytona}; @@ -255,35 +255,15 @@ async fn probe_single_provider( None, ); }; - let model_id = model.id.clone(); + let model_id = model.id.to_string(); - let outcome = run_basic_model_probe(model_id.as_str(), provider.clone(), client); - provider_probe_with_timeout( - provider, - model_id.to_string(), - outcome, + let outcome = run_basic_model_probe_with_timeout( + &model_id, + &provider, + client, EXTERNAL_SERVICE_PROBE_TIMEOUT, ) - .await -} - -async fn provider_probe_with_timeout( - provider: ProviderId, - model_id: String, - probe: F, - probe_timeout: Duration, -) -> ProviderProbeResult -where - F: Future, -{ - let Ok(outcome) = timeout(probe_timeout, probe).await else { - return provider_probe_error( - provider, - Some(model_id), - probe_timeout_message(probe_timeout), - None, - ); - }; + .await; match outcome.status { ModelTestStatus::Ok => ProviderProbeResult { @@ -302,14 +282,6 @@ where } } -fn probe_timeout_message(probe_timeout: Duration) -> String { - if probe_timeout.subsec_nanos() == 0 { - format!("timeout ({}s)", probe_timeout.as_secs()) - } else { - format!("timeout ({}ms)", probe_timeout.as_millis()) - } -} - fn provider_probe_error( provider: ProviderId, model_id: Option, @@ -689,28 +661,13 @@ async fn check_cloud_sandbox(state: &AppState) -> CheckResult { }; }; - check_cloud_sandbox_with_probe( - || state.check_daytona_api_key(api_key), - EXTERNAL_SERVICE_PROBE_TIMEOUT, - ) - .await + let probe = state + .check_daytona_api_key_with_timeout(api_key, EXTERNAL_SERVICE_PROBE_TIMEOUT) + .await; + cloud_sandbox_probe_check(probe) } -async fn check_cloud_sandbox_with_probe(probe: F, probe_timeout: Duration) -> CheckResult -where - F: FnOnce() -> Fut, - Fut: Future>, -{ - let Ok(probe) = timeout(probe_timeout, probe()).await else { - return CheckResult { - name: "Cloud Sandbox".to_string(), - status: CheckStatus::Error, - summary: probe_timeout_message(probe_timeout), - details: vec![CheckDetail::new("Daytona probe timed out".to_string())], - remediation: Some("Verify DAYTONA_API_KEY value and Daytona reachability".to_string()), - }; - }; - +fn cloud_sandbox_probe_check(probe: anyhow::Result) -> CheckResult { match probe { Ok(check) if check.ok() => CheckResult { name: "Cloud Sandbox".to_string(), @@ -733,13 +690,29 @@ where daytona::required_perms_display() )), }, - Err(err) => CheckResult { - name: "Cloud Sandbox".to_string(), - status: CheckStatus::Error, - summary: "Daytona credential rejected".to_string(), - details: vec![CheckDetail::new(format!("{err:#}"))], - remediation: Some("Verify DAYTONA_API_KEY value and Daytona reachability".to_string()), - }, + Err(err) => { + if let Some(timeout) = err.downcast_ref::() { + return CheckResult { + name: "Cloud Sandbox".to_string(), + status: CheckStatus::Error, + summary: format!("timeout ({:?})", timeout.timeout()), + details: vec![CheckDetail::new("Daytona probe timed out".to_string())], + remediation: Some( + "Verify DAYTONA_API_KEY value and Daytona reachability".to_string(), + ), + }; + } + + CheckResult { + name: "Cloud Sandbox".to_string(), + status: CheckStatus::Error, + summary: "Daytona credential rejected".to_string(), + details: vec![CheckDetail::new(format!("{err:#}"))], + remediation: Some( + "Verify DAYTONA_API_KEY value and Daytona reachability".to_string(), + ), + } + } } } @@ -1090,27 +1063,6 @@ mod tests { ); } - #[tokio::test] - async fn provider_probe_reports_provider_specific_timeout() { - assert_eq!( - probe_timeout_message(EXTERNAL_SERVICE_PROBE_TIMEOUT), - "timeout (15s)" - ); - - let result = provider_probe_with_timeout( - ProviderId::new("modal"), - "modal/test-model".to_string(), - std::future::pending::(), - Duration::from_millis(1), - ) - .await; - - assert_eq!(result.provider, ProviderId::new("modal")); - assert_eq!(result.model_id.as_deref(), Some("modal/test-model")); - assert_eq!(result.status, ProviderProbeStatus::Error); - assert_eq!(result.error_message.as_deref(), Some("timeout (1ms)")); - } - #[test] fn docker_sandbox_probe_passes_when_daemon_responds() { let result = docker_sandbox_probe_check(Ok(())); @@ -1230,13 +1182,11 @@ enabled = false ); } - #[tokio::test] - async fn check_cloud_sandbox_reports_timeout() { - let result = check_cloud_sandbox_with_probe( - std::future::pending::>, - Duration::from_millis(1), - ) - .await; + #[test] + fn check_cloud_sandbox_reports_timeout() { + let result = cloud_sandbox_probe_check(Err(anyhow::Error::new( + daytona::DaytonaCredentialProbeTimeout::new(Duration::from_millis(1)), + ))); assert_eq!(result.name, "Cloud Sandbox"); assert_eq!(result.status, CheckStatus::Error); diff --git a/lib/apps/fabro-server/src/server.rs b/lib/apps/fabro-server/src/server.rs index 5445ad619..4976880f8 100644 --- a/lib/apps/fabro-server/src/server.rs +++ b/lib/apps/fabro-server/src/server.rs @@ -1455,6 +1455,15 @@ impl AppState { pub(crate) async fn check_daytona_api_key( &self, api_key: String, + ) -> anyhow::Result { + self.check_daytona_api_key_with_timeout(api_key, daytona::DAYTONA_CREDENTIAL_PROBE_TIMEOUT) + .await + } + + pub(crate) async fn check_daytona_api_key_with_timeout( + &self, + api_key: String, + probe_timeout: Duration, ) -> anyhow::Result { let base_url = self .config_env_lookup(EnvVars::DAYTONA_API_URL) @@ -1463,8 +1472,14 @@ impl AppState { let org_id = self.config_env_lookup(EnvVars::DAYTONA_ORGANIZATION_ID); let http_client = fabro_http::http_client().context("failed to build HTTP client")?; - daytona::check_daytona_api_key_with(&base_url, org_id.as_deref(), api_key, http_client) - .await + daytona::check_daytona_api_key_with_timeout( + &base_url, + org_id.as_deref(), + api_key, + http_client, + probe_timeout, + ) + .await } /// Borrow the persistent store so sibling modules can open run readers diff --git a/lib/apps/fabro-server/src/server/handler/system.rs b/lib/apps/fabro-server/src/server/handler/system.rs index a38fe33f5..d74f65944 100644 --- a/lib/apps/fabro-server/src/server/handler/system.rs +++ b/lib/apps/fabro-server/src/server/handler/system.rs @@ -778,12 +778,12 @@ mod tests { ) .await; - assert_eq!(response.status(), StatusCode::GATEWAY_TIMEOUT); - let body = axum::body::to_bytes(response.into_body(), usize::MAX) - .await - .expect("diagnostics timeout response body should be readable"); - let body: serde_json::Value = serde_json::from_slice(&body) - .expect("diagnostics timeout response should contain JSON"); + let body = fabro_test::expect_axum_json( + response, + StatusCode::GATEWAY_TIMEOUT, + "GET /api/v1/system/diagnostics timeout", + ) + .await; assert_eq!(body["errors"][0]["code"], "diagnostics_timeout"); assert_eq!(body["errors"][0]["detail"], "Server diagnostics timed out."); } diff --git a/lib/components/fabro-llm/src/model_test.rs b/lib/components/fabro-llm/src/model_test.rs index 6a985fb1b..de09f6fde 100644 --- a/lib/components/fabro-llm/src/model_test.rs +++ b/lib/components/fabro-llm/src/model_test.rs @@ -1,3 +1,4 @@ +use std::future::Future; use std::sync::Arc; use std::time::Duration; @@ -63,22 +64,38 @@ pub async fn run_basic_model_probe( model_id: &str, provider: impl ToString, client: Arc, +) -> ModelTestOutcome { + run_basic_model_probe_with_timeout( + model_id, + provider, + client, + Duration::from_secs(ModelTestMode::Basic.timeout_secs()), + ) + .await +} + +pub async fn run_basic_model_probe_with_timeout( + model_id: &str, + provider: impl ToString, + client: Arc, + probe_timeout: Duration, ) -> ModelTestOutcome { let params = GenerateParams::new(model_id, client) .provider(provider.to_string()) .prompt("Say OK") .max_tokens(16); - let result = time::timeout( - Duration::from_secs(ModelTestMode::Basic.timeout_secs()), - generate::generate(params), - ) - .await; + basic_model_probe_outcome(generate::generate(params), probe_timeout).await +} - match result { +async fn basic_model_probe_outcome(probe: F, probe_timeout: Duration) -> ModelTestOutcome +where + F: Future>, +{ + match time::timeout(probe_timeout, probe).await { Ok(Ok(_)) => ModelTestOutcome::ok(), Ok(Err(err)) => ModelTestOutcome::error(err.to_string()), - Err(_) => ModelTestOutcome::error("timeout (30s)"), + Err(_) => ModelTestOutcome::error(format!("timeout ({probe_timeout:?})")), } } @@ -244,6 +261,18 @@ mod tests { ); } + #[tokio::test] + async fn basic_model_probe_reports_configured_timeout() { + let outcome = basic_model_probe_outcome( + std::future::pending::>(), + Duration::from_millis(1), + ) + .await; + + assert_eq!(outcome.status, ModelTestStatus::Error); + assert_eq!(outcome.error_message.as_deref(), Some("timeout (1ms)")); + } + #[test] fn deep_test_omits_effort_for_reasoning_without_effort_controls() { let info = test_model_with(ModelFeatures { diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 6371f23d2..44d014159 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -1,5 +1,6 @@ use std::collections::HashMap; use std::fmt::Write; +use std::future::Future; use std::path::Path; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; @@ -62,7 +63,8 @@ pub const DEFAULT_DAYTONA_API_URL: &str = "https://app.daytona.io/api"; pub(crate) const DAYTONA_DASHBOARD_SANDBOXES_URL: &str = "https://app.daytona.io/dashboard/sandboxes"; const FABRO_SANDBOX_USER_AGENT: &str = concat!("fabro-sandbox/", env!("CARGO_PKG_VERSION")); -const DAYTONA_PROBE_TIMEOUT: Duration = Duration::from_secs(20); +pub const DAYTONA_CREDENTIAL_PROBE_TIMEOUT: Duration = Duration::from_secs(20); +const DAYTONA_BASH_SESSION_PROBE_TIMEOUT: Duration = Duration::from_secs(20); const DAYTONA_START_TIMEOUT: Duration = Duration::from_mins(1); /// Upper bound on explicit and Drop-triggered Daytona cleanup calls (session /// deletion, temporary stdin files) so a stalled REST call cannot block @@ -156,6 +158,24 @@ pub struct DaytonaKeyCheck { pub missing: Vec, } +#[derive(Debug, thiserror::Error)] +#[error("Daytona credential probe timed out after {timeout:?}")] +pub struct DaytonaCredentialProbeTimeout { + timeout: Duration, +} + +impl DaytonaCredentialProbeTimeout { + #[must_use] + pub const fn new(timeout: Duration) -> Self { + Self { timeout } + } + + #[must_use] + pub const fn timeout(&self) -> Duration { + self.timeout + } +} + impl DaytonaKeyCheck { pub fn ok(&self) -> bool { self.missing.is_empty() @@ -253,6 +273,23 @@ pub async fn check_daytona_api_key_with( org_id: Option<&str>, api_key: String, http_client: fabro_http::HttpClient, +) -> anyhow::Result { + check_daytona_api_key_with_timeout( + base_url, + org_id, + api_key, + http_client, + DAYTONA_CREDENTIAL_PROBE_TIMEOUT, + ) + .await +} + +pub async fn check_daytona_api_key_with_timeout( + base_url: &str, + org_id: Option<&str>, + api_key: String, + http_client: fabro_http::HttpClient, + probe_timeout: Duration, ) -> anyhow::Result { let work = async { let client = build_daytona_client_with( @@ -287,12 +324,21 @@ pub async fn check_daytona_api_key_with( }) }; - match time::timeout(DAYTONA_PROBE_TIMEOUT, work).await { + daytona_credential_probe_with_timeout(work, probe_timeout).await +} + +async fn daytona_credential_probe_with_timeout( + probe: F, + probe_timeout: Duration, +) -> anyhow::Result +where + F: Future>, +{ + match time::timeout(probe_timeout, probe).await { Ok(result) => result, - Err(_) => Err(anyhow::anyhow!( - "Daytona credential probe timed out after {}s", - DAYTONA_PROBE_TIMEOUT.as_secs() - )), + Err(_) => Err(anyhow::Error::new(DaytonaCredentialProbeTimeout::new( + probe_timeout, + ))), } } @@ -576,16 +622,16 @@ impl DaytonaSandbox { /// non-POSIX, and completion assertions all hold. /// /// Costs one session round trip plus a single status poll per sandbox - /// lifecycle transition. `DAYTONA_PROBE_TIMEOUT` is the outer backstop for - /// a stalled REST call; the inner [`BASH_PROBE_TIMEOUT_MS`] is the deadline - /// for the command itself. Session cleanup runs outside that deadline under - /// its own bounded timeout. + /// lifecycle transition. `DAYTONA_BASH_SESSION_PROBE_TIMEOUT` is the outer + /// backstop for a stalled REST call; the inner [`BASH_PROBE_TIMEOUT_MS`] is + /// the deadline for the command itself. Session cleanup runs outside that + /// deadline under its own bounded timeout. async fn probe_bash_session(sandbox: &daytona_sdk::Sandbox) -> crate::Result<()> { - let deadline = time::Instant::now() + DAYTONA_PROBE_TIMEOUT; + let deadline = time::Instant::now() + DAYTONA_BASH_SESSION_PROBE_TIMEOUT; let timeout_error = || { crate::Error::message(format!( "Daytona Bash session check timed out after {}s", - DAYTONA_PROBE_TIMEOUT.as_secs() + DAYTONA_BASH_SESSION_PROBE_TIMEOUT.as_secs() )) }; let mut session = match time::timeout_at(deadline, DaytonaSession::create(sandbox)).await { @@ -3358,6 +3404,25 @@ mod tests { auth.assert_async().await; } + #[tokio::test] + async fn daytona_credential_probe_reports_configured_timeout() { + let err = daytona_credential_probe_with_timeout( + std::future::pending::>(), + Duration::from_millis(1), + ) + .await + .expect_err("probe should time out"); + let timeout = err + .downcast_ref::() + .expect("timeout should preserve its type"); + + assert_eq!(timeout.timeout(), Duration::from_millis(1)); + assert_eq!( + err.to_string(), + "Daytona credential probe timed out after 1ms" + ); + } + #[tokio::test] async fn daytona_stdin_file_uploads_exact_bytes_and_is_deleted() { let server = MockServer::start_async().await; From 2b095612c8eb315d35f80ef4ac6eb728b22ddd3a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 20:35:52 -0400 Subject: [PATCH 18/19] Address run spec persistence review findings --- .../fabro-workflow/src/operations/create.rs | 54 +++++++++-------- .../fabro-workflow/src/pipeline/persist.rs | 58 +++++++++++++------ lib/foundation/fabro-redact/src/entropy.rs | 17 ++++++ lib/foundation/fabro-test/src/lib.rs | 18 ++---- 4 files changed, 95 insertions(+), 52 deletions(-) diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 3f23acd5b..a872f0186 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -13,10 +13,11 @@ use std::sync::Arc; use fabro_config::Storage; use fabro_graphviz::graph::{AttrValue, Graph}; use fabro_model::{Catalog, ProviderId}; -use fabro_store::Database; +use fabro_store::{Database, RunDatabase}; use fabro_template::TemplateContext; use fabro_types::{ - AutomationRef, ForkSourceRef, GitContext, ManifestPath, RunId, RunProvenance, WorkflowSettings, + AutomationRef, ForkSourceRef, GitContext, ManifestPath, RunBlobId, RunId, RunProvenance, + WorkflowSettings, }; use fabro_util::json::normalize_json_value; use tokio::task::spawn_blocking; @@ -517,24 +518,17 @@ async fn persist_created_run( .create_run(&record.run_id) .await .map_err(|err| Error::engine_with_source("failed to create run store", err))?; - let manifest_blob = match submitted_manifest_bytes { - Some(bytes) => Some(run_store.write_blob(bytes).await.map_err(store_error)?), - None => None, - }; - let definition_blob = match accepted_definition { - Some(definition) => { - let bytes = - serde_json::to_vec(definition).map_err(|err| Error::engine(err.to_string()))?; - Some(run_store.write_blob(&bytes).await.map_err(store_error)?) - } - None => None, - }; - // The spec on the run.created event is subject to secret redaction in - // stored copies; the blob keeps the exact bytes execution needs. - let spec_blob = { - let bytes = serde_json::to_vec(record).map_err(|err| Error::engine(err.to_string()))?; - Some(run_store.write_blob(&bytes).await.map_err(store_error)?) - }; + let definition_bytes = accepted_definition + .map(serde_json::to_vec) + .transpose() + .map_err(|err| Error::engine_with_source("failed to serialize run definition", err))?; + let spec_bytes = serde_json::to_vec(record) + .map_err(|err| Error::engine_with_source("failed to serialize run spec", err))?; + let (manifest_blob, definition_blob, spec_blob) = tokio::try_join!( + write_optional_blob(&run_store, submitted_manifest_bytes), + write_optional_blob(&run_store, definition_bytes.as_deref()), + async { run_store.write_blob(&spec_bytes).await.map_err(store_error) }, + )?; let title = explicit_title.unwrap_or_else(|| fabro_types::infer_run_title(record.graph.goal())); let stored = to_run_event_at( @@ -561,7 +555,7 @@ async fn persist_created_run( automation: record.automation.clone(), provenance: record.provenance.clone(), manifest_blob, - spec_blob, + spec_blob: Some(spec_blob), git: record.git.clone(), fork_source_ref: record.fork_source_ref.clone(), retried_from: None, @@ -588,8 +582,22 @@ async fn persist_created_run( .map_err(store_error) } -fn store_error(err: impl std::fmt::Display) -> Error { - Error::engine(err.to_string()) +async fn write_optional_blob( + run_store: &RunDatabase, + bytes: Option<&[u8]>, +) -> Result, Error> { + match bytes { + Some(bytes) => run_store + .write_blob(bytes) + .await + .map(Some) + .map_err(store_error), + None => Ok(None), + } +} + +fn store_error(err: impl Into) -> Error { + Error::engine_with_source("run store operation failed", err) } /// Parse, transform, and validate `dot_source`. diff --git a/lib/components/fabro-workflow/src/pipeline/persist.rs b/lib/components/fabro-workflow/src/pipeline/persist.rs index 8c224c700..303241ff5 100644 --- a/lib/components/fabro-workflow/src/pipeline/persist.rs +++ b/lib/components/fabro-workflow/src/pipeline/persist.rs @@ -65,22 +65,23 @@ async fn executable_run_spec( let bytes = run_store .read_blob(&blob_id) .await - .map_err(|err| Error::engine(err.to_string()))? + .map_err(|err| Error::engine_with_anyhow("failed to read run spec blob", err))? .ok_or_else(|| { Error::engine(format!( "run spec blob is missing from the run store: {blob_id}" )) })?; - let mut spec: RunSpec = - serde_json::from_slice(&bytes).map_err(|err| Error::Parse(err.to_string()))?; - // The event stream stays authoritative for run identity, for provenance - // (a retry rewrites it), for blob ids recorded on events after the spec - // blob was written, and for a graph source the blob does not carry. + let mut spec: RunSpec = serde_json::from_slice(&bytes) + .map_err(|err| Error::engine_with_source("run spec blob was not valid JSON", err))?; + // The event stream stays authoritative for run identity, provenance, and + // blob ids. Prefer the unredacted graph source from the blob, with the + // folded source as a compatibility fallback. spec.run_id = folded.run_id; spec.provenance = folded.provenance; spec.manifest_blob = folded.manifest_blob; spec.definition_blob = folded.definition_blob; spec.spec_blob = folded.spec_blob; + spec.fork_source_ref = folded.fork_source_ref; spec.graph_source = spec.graph_source.or(folded.graph_source); Ok(spec) } @@ -191,27 +192,24 @@ mod tests { } async fn seeded_store(record: &RunSpec, source: Option<&str>) -> RunDatabase { - seeded_store_with(record, source, true).await + seeded_store_with(record, source, Some(record)).await } async fn seeded_store_with( record: &RunSpec, source: Option<&str>, - write_spec_blob: bool, + blob_record: Option<&RunSpec>, ) -> RunDatabase { let store = memory_store(); let run_store = store.create_run(&record.run_id).await.unwrap(); - // Mirror the production producer: the unredacted spec rides a blob - // and the redacted event carries its id. - let spec_blob = if write_spec_blob { - Some( + let spec_blob = match blob_record { + Some(blob_record) => Some( run_store - .write_blob(&serde_json::to_vec(record).unwrap()) + .write_blob(&serde_json::to_vec(blob_record).unwrap()) .await .unwrap(), - ) - } else { - None + ), + None => None, }; append_event(&run_store, &record.run_id, &Event::RunCreated { run_id: record.run_id, @@ -384,7 +382,7 @@ mod tests { let mut record = sample_record(different_graph()); record.graph = graph; - let run_store = seeded_store_with(&record, Some(&source), false).await; + let run_store = seeded_store_with(&record, Some(&source), None).await; let loaded = load_from_store(&run_store.clone().into(), &run_dir) .await .unwrap(); @@ -393,6 +391,32 @@ mod tests { assert_eq!(loaded.run_spec().spec_blob, None); } + #[tokio::test] + async fn load_from_store_uses_fork_reference_from_event_fold() { + let temp = tempfile::tempdir().unwrap(); + let run_dir = temp.path().join("run"); + std::fs::create_dir_all(&run_dir).unwrap(); + let (graph, source) = graph_and_source(); + let source_record = sample_record(graph.clone()); + let mut fork_record = source_record.clone(); + fork_record.run_id = fixtures::RUN_7; + fork_record.fork_source_ref = Some(fabro_types::ForkSourceRef { + source_run_id: source_record.run_id, + checkpoint_sha: "checkpoint-sha".to_string(), + }); + + let run_store = seeded_store_with(&fork_record, Some(&source), Some(&source_record)).await; + let loaded = load_from_store(&run_store.clone().into(), &run_dir) + .await + .unwrap(); + + assert_eq!(loaded.run_spec().run_id, fork_record.run_id); + assert_eq!( + loaded.run_spec().fork_source_ref, + fork_record.fork_source_ref + ); + } + #[test] fn persist_returns_error_on_io_failure() { let temp = tempfile::tempdir().unwrap(); diff --git a/lib/foundation/fabro-redact/src/entropy.rs b/lib/foundation/fabro-redact/src/entropy.rs index 884aa3713..ec6029c3c 100644 --- a/lib/foundation/fabro-redact/src/entropy.rs +++ b/lib/foundation/fabro-redact/src/entropy.rs @@ -81,6 +81,10 @@ pub(super) fn find_entropy_regions(s: &str) -> Vec { /// qualify simply measures under the threshold; no length guard is needed. fn assignment_value_offset(token: &str) -> Option { let eq = token.find('=')?; + let value = &token[eq + 1..]; + if value.is_empty() || value.starts_with('=') { + return None; + } let name = &token[..eq]; let mut chars = name.chars(); let first = chars.next()?; @@ -135,6 +139,19 @@ mod tests { assert_eq!(regions[0].end, input.len()); } + #[test] + fn regions_find_padded_base64_tokens() { + for input in [ + "WxFhjC5EAnh30M0JIe0Wa58Xb1BYf8kedTTdKUbbd9Y=", + "AbCdEfGhIjKlMnOpQrStUvWxYz0123456789ABCDEF==", + ] { + assert_eq!(find_entropy_regions(input), vec![Region { + start: 0, + end: input.len(), + }]); + } + } + #[test] fn regions_empty_for_json_escape_sequence() { // "controller.go\nmodel.go" — the regex could match across the \n boundary diff --git a/lib/foundation/fabro-test/src/lib.rs b/lib/foundation/fabro-test/src/lib.rs index eec273d32..5c53f9ba2 100644 --- a/lib/foundation/fabro-test/src/lib.rs +++ b/lib/foundation/fabro-test/src/lib.rs @@ -1955,18 +1955,12 @@ pub fn json_snapshot_filters(mut filters: Vec<(String, String)>) -> Vec<(String, r#""id": "[EVENT_ID]""#.to_string(), )); filters = json_elapsed_ms_snapshot_filters(filters); - filters.push(( - r#""manifest_blob":\s*"[0-9a-f]{64}""#.to_string(), - r#""manifest_blob": "[BLOB_ID]""#.to_string(), - )); - filters.push(( - r#""definition_blob":\s*"[0-9a-f]{64}""#.to_string(), - r#""definition_blob": "[BLOB_ID]""#.to_string(), - )); - filters.push(( - r#""spec_blob":\s*"[0-9a-f]{64}""#.to_string(), - r#""spec_blob": "[BLOB_ID]""#.to_string(), - )); + for field in ["manifest_blob", "definition_blob", "spec_blob"] { + filters.push(( + format!(r#""{field}":\s*"[0-9a-f]{{64}}""#), + format!(r#""{field}": "[BLOB_ID]""#), + )); + } filters.push(( r#""run_dir":\s*"\[STORAGE_DIR\]/scratch/\d{8}-\[ULID\]""#.to_string(), r#""run_dir": "[RUN_DIR]""#.to_string(), From a64b65b88c1b6d33e65116cc28d428cc7e1c1951 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 20:50:41 -0400 Subject: [PATCH 19/19] Update blob hash CLI snapshot --- lib/apps/fabro-cli/tests/it/cmd/attach.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/apps/fabro-cli/tests/it/cmd/attach.rs b/lib/apps/fabro-cli/tests/it/cmd/attach.rs index b2b3904ee..9f388c14b 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/attach.rs @@ -1012,7 +1012,7 @@ fn attach_json_errors_without_prompting_for_human_input() { } }, "source_directory": "[TEMP_DIR]", - "spec_blob": "[BLOB_ID]", + "spec_blob": "[BLOB_HASH]", "title": "Wait for approval", "web_url": "http://localhost:3000/runs/[ULID]", "workflow_slug": "human-gate",