mirror of
https://github.com/RightNow-AI/openfang.git
synced 2026-07-30 06:32:17 +00:00
fix(runtime): add subprocess timeout config for claude-code driver
The claude-code driver hardcodes its per-message turn timeout inside
ClaudeCodeDriver and exposed no operator-facing knob, so long-running
CC subprocess turns (large prompt-caches, deep tool chains) hit the
internal default with no escape hatch. Adds a public config surface,
honored today only by the claude-code driver, designed so future
subprocess drivers can opt in without re-shaping the API.
Public surface
- DriverConfig.subprocess_timeout_secs: Option<u64> (llm_driver.rs)
- OPENFANG_SUBPROCESS_TIMEOUT_SECS env var (drivers/mod.rs)
- Precedence in create_driver(): env var > config field > driver default
Naming rationale
- Field/env are scope-flavored, not semantic, on purpose: the name
telegraphs that HTTP providers (default/Anthropic, openai, bedrock,
qwen-code) accept-but-silently-ignore the field today. A semantic
name (message_timeout_secs) would have invited the same silent-no-op
footgun on those providers.
- Driver-internal field in claude_code.rs intentionally kept as
message_timeout_secs — it's not on the public boundary and the
semantic name accurately describes what it stores.
Tests (drivers/mod.rs)
- default_when_unset: no env, no config -> driver default
- config_set: config field flows through
- env_overrides_config: env var wins over config (construction-only
assertion; trait-object opacity prevents reading the value back)
- malformed_env_falls_through: unparseable env silently falls through
to config, matching the .parse::<u64>().ok() chain in production
- All four tests scrub OPENFANG_SUBPROCESS_TIMEOUT_SECS pre/post to
avoid cross-test pollution
Mechanical pass-throughs
- 12 x DriverConfig { .. } test fixtures in drivers/mod.rs gain
subprocess_timeout_secs: None
- routes.rs (1), kernel.rs (6), agent_loop.rs (2): same pass-through
fills in DriverConfig literals; no logic touched
Forward-compat note
- A NOTE block in drivers/mod.rs flags the scope-vs-implementation
gap so the next contributor adding a subprocess driver knows
exactly where to wire the config in.
Validated end-to-end against a live daemon: dry-run + full deploy
(deploy-local.sh, all 7 phases) + post-swap agent_send round-trip
through the claude-code dispatch path.
This commit is contained in:
@@ -7698,6 +7698,7 @@ pub async fn test_provider(
|
||||
Some(base_url)
|
||||
},
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
|
||||
match openfang_runtime::drivers::create_driver(&driver_config) {
|
||||
|
||||
@@ -660,6 +660,7 @@ impl OpenFangKernel {
|
||||
.cloned()
|
||||
}),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
// Primary driver failure is non-fatal: the dashboard should remain accessible
|
||||
// even if the LLM provider is misconfigured. Users can fix config via dashboard.
|
||||
@@ -683,6 +684,7 @@ impl OpenFangKernel {
|
||||
.map(|z: zeroize::Zeroizing<String>| z.to_string()),
|
||||
base_url: config.provider_urls.get(provider).cloned(),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
match drivers::create_driver(&auto_config) {
|
||||
Ok(d) => {
|
||||
@@ -731,6 +733,7 @@ impl OpenFangKernel {
|
||||
.clone()
|
||||
.or_else(|| config.provider_urls.get(&fb.provider).cloned()),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
match drivers::create_driver(&fb_config) {
|
||||
Ok(d) => {
|
||||
@@ -5025,6 +5028,7 @@ impl OpenFangKernel {
|
||||
api_key,
|
||||
base_url,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
|
||||
match drivers::create_driver(&driver_config) {
|
||||
@@ -5101,6 +5105,7 @@ impl OpenFangKernel {
|
||||
.or_else(|| dm.base_url.clone())
|
||||
.or_else(|| self.lookup_provider_url(&fb_provider)),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
match drivers::create_driver(&config) {
|
||||
Ok(d) => chain.push((d, strip_provider_prefix(&fb_model_name, &fb_provider))),
|
||||
@@ -5131,6 +5136,7 @@ impl OpenFangKernel {
|
||||
.clone()
|
||||
.or_else(|| self.lookup_provider_url(&fb.provider)),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
match drivers::create_driver(&fb_config) {
|
||||
Ok(d) => {
|
||||
|
||||
@@ -1143,6 +1143,7 @@ async fn call_with_retry(
|
||||
api_key,
|
||||
base_url: fb.base_url.clone(),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let fb_driver = match crate::drivers::create_driver(&fb_config) {
|
||||
Ok(d) => d,
|
||||
@@ -1326,6 +1327,7 @@ async fn stream_with_retry(
|
||||
api_key,
|
||||
base_url: fb.base_url.clone(),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let fb_driver = match crate::drivers::create_driver(&fb_config) {
|
||||
Ok(d) => d,
|
||||
|
||||
@@ -325,10 +325,24 @@ pub fn create_driver(config: &DriverConfig) -> Result<Arc<dyn LlmDriver>, LlmErr
|
||||
// Claude Code CLI — subprocess-based, no API key needed
|
||||
if provider == "claude-code" {
|
||||
let cli_path = config.base_url.clone();
|
||||
return Ok(Arc::new(claude_code::ClaudeCodeDriver::new(
|
||||
cli_path,
|
||||
config.skip_permissions,
|
||||
)));
|
||||
// Timeout precedence (highest wins):
|
||||
// 1. OPENFANG_SUBPROCESS_TIMEOUT_SECS env var (no-rebuild override for emergencies)
|
||||
// 2. DriverConfig.subprocess_timeout_secs (config.toml-driven)
|
||||
// 3. Driver default (currently 300s, set inside ClaudeCodeDriver::new)
|
||||
// NOTE: The field and env var are scope-named to apply to any subprocess
|
||||
// driver, but today only `provider = "claude-code"` reads them. Other
|
||||
// drivers accept the field silently (forward-compat); future subprocess
|
||||
// drivers (qwen-code, etc.) will opt in here individually.
|
||||
let timeout = std::env::var("OPENFANG_SUBPROCESS_TIMEOUT_SECS")
|
||||
.ok()
|
||||
.and_then(|s| s.parse::<u64>().ok())
|
||||
.or(config.subprocess_timeout_secs);
|
||||
return Ok(Arc::new(match timeout {
|
||||
Some(secs) => {
|
||||
claude_code::ClaudeCodeDriver::with_timeout(cli_path, config.skip_permissions, secs)
|
||||
}
|
||||
None => claude_code::ClaudeCodeDriver::new(cli_path, config.skip_permissions),
|
||||
}));
|
||||
}
|
||||
|
||||
// Qwen Code CLI — subprocess-based, uses Qwen OAuth (free tier)
|
||||
@@ -648,6 +662,7 @@ mod tests {
|
||||
api_key: Some("test".to_string()),
|
||||
base_url: Some("http://localhost:9999/v1".to_string()),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(driver.is_ok());
|
||||
@@ -660,6 +675,7 @@ mod tests {
|
||||
api_key: None,
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(driver.is_err());
|
||||
@@ -779,6 +795,7 @@ mod tests {
|
||||
api_key: None,
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(
|
||||
@@ -795,6 +812,7 @@ mod tests {
|
||||
api_key: None,
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(driver.is_err());
|
||||
@@ -810,6 +828,7 @@ mod tests {
|
||||
api_key: None, // picked up from env via provider_defaults
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(
|
||||
@@ -827,6 +846,7 @@ mod tests {
|
||||
api_key: None,
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(driver.is_err());
|
||||
@@ -842,6 +862,7 @@ mod tests {
|
||||
api_key: None,
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let result = create_driver(&config);
|
||||
assert!(result.is_err());
|
||||
@@ -870,6 +891,7 @@ mod tests {
|
||||
api_key: Some("explicit-key".to_string()),
|
||||
base_url: Some("https://api.example.com/v1".to_string()),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(driver.is_ok());
|
||||
@@ -897,6 +919,7 @@ mod tests {
|
||||
api_key: Some("test-azure-key".to_string()),
|
||||
base_url: Some("https://myresource.openai.azure.com/openai/deployments".to_string()),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(driver.is_ok(), "Azure driver with key + URL should succeed");
|
||||
@@ -909,6 +932,7 @@ mod tests {
|
||||
api_key: None,
|
||||
base_url: Some("https://myresource.openai.azure.com/openai/deployments".to_string()),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let result = create_driver(&config);
|
||||
assert!(result.is_err(), "Azure driver without key should error");
|
||||
@@ -927,6 +951,7 @@ mod tests {
|
||||
api_key: Some("test-azure-key".to_string()),
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let result = create_driver(&config);
|
||||
assert!(result.is_err(), "Azure driver without URL should error");
|
||||
@@ -945,6 +970,7 @@ mod tests {
|
||||
api_key: Some("test-azure-key".to_string()),
|
||||
base_url: Some("https://myresource.openai.azure.com/openai/deployments".to_string()),
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(
|
||||
@@ -969,6 +995,7 @@ mod tests {
|
||||
api_key: Some("test-bedrock-api-key".to_string()),
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
// Should succeed because api_key is provided
|
||||
let driver = create_driver(&config);
|
||||
@@ -977,4 +1004,77 @@ mod tests {
|
||||
"Bedrock with explicit api_key should construct successfully"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_claude_code_driver_constructs_with_default_timeout() {
|
||||
// No timeout in config and no env override → driver uses its built-in default.
|
||||
std::env::remove_var("OPENFANG_SUBPROCESS_TIMEOUT_SECS");
|
||||
let config = DriverConfig {
|
||||
provider: "claude-code".to_string(),
|
||||
api_key: None,
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: None,
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(driver.is_ok(), "claude-code driver should construct");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_claude_code_driver_constructs_with_config_timeout() {
|
||||
// Timeout set via config field → with_timeout path is exercised.
|
||||
std::env::remove_var("OPENFANG_SUBPROCESS_TIMEOUT_SECS");
|
||||
let config = DriverConfig {
|
||||
provider: "claude-code".to_string(),
|
||||
api_key: None,
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: Some(480),
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
assert!(
|
||||
driver.is_ok(),
|
||||
"claude-code driver should construct with custom timeout"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_claude_code_driver_constructs_with_env_timeout_override() {
|
||||
// Env var present → wins over config field. We can't read the timeout off the
|
||||
// trait object here, but at minimum the construction path must not panic
|
||||
// when both are set and the env var parses cleanly.
|
||||
std::env::set_var("OPENFANG_SUBPROCESS_TIMEOUT_SECS", "600");
|
||||
let config = DriverConfig {
|
||||
provider: "claude-code".to_string(),
|
||||
api_key: None,
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: Some(120),
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
std::env::remove_var("OPENFANG_SUBPROCESS_TIMEOUT_SECS");
|
||||
assert!(
|
||||
driver.is_ok(),
|
||||
"claude-code driver should construct when env override is set"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_claude_code_driver_ignores_unparseable_env_timeout() {
|
||||
// Garbage env var → falls through to config field, doesn't error.
|
||||
std::env::set_var("OPENFANG_SUBPROCESS_TIMEOUT_SECS", "not-a-number");
|
||||
let config = DriverConfig {
|
||||
provider: "claude-code".to_string(),
|
||||
api_key: None,
|
||||
base_url: None,
|
||||
skip_permissions: true,
|
||||
subprocess_timeout_secs: Some(420),
|
||||
};
|
||||
let driver = create_driver(&config);
|
||||
std::env::remove_var("OPENFANG_SUBPROCESS_TIMEOUT_SECS");
|
||||
assert!(
|
||||
driver.is_ok(),
|
||||
"unparseable env override should fall through to config field"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -188,6 +188,27 @@ pub struct DriverConfig {
|
||||
/// restricts what agents can do, making this safe.
|
||||
#[serde(default = "default_skip_permissions")]
|
||||
pub skip_permissions: bool,
|
||||
|
||||
/// Per-message subprocess turn timeout in seconds.
|
||||
///
|
||||
/// Caps how long the runtime will wait for a single CLI subprocess turn
|
||||
/// (one message round-trip) before killing the process and reporting a
|
||||
/// timeout failure. When unset, the driver's own default is used
|
||||
/// (currently 300s). Long-context Opus calls with heavy tool surfaces
|
||||
/// routinely take >4 minutes, so users running large prompts may want
|
||||
/// to bump this to 480–600s.
|
||||
///
|
||||
/// Can also be overridden at runtime via the
|
||||
/// `OPENFANG_SUBPROCESS_TIMEOUT_SECS` env var, which wins over both
|
||||
/// this field and the driver default.
|
||||
///
|
||||
/// **Scope:** Currently only honored by `provider = "claude-code"`.
|
||||
/// Other providers (`default`, `qwen-code`, `openai`, `bedrock`, etc.)
|
||||
/// accept the field for forward-compatibility but silently ignore it
|
||||
/// today. As additional subprocess-based drivers are added, they will
|
||||
/// opt in to this field individually.
|
||||
#[serde(default)]
|
||||
pub subprocess_timeout_secs: Option<u64>,
|
||||
}
|
||||
|
||||
fn default_skip_permissions() -> bool {
|
||||
@@ -202,6 +223,7 @@ impl std::fmt::Debug for DriverConfig {
|
||||
.field("api_key", &self.api_key.as_ref().map(|_| "<redacted>"))
|
||||
.field("base_url", &self.base_url)
|
||||
.field("skip_permissions", &self.skip_permissions)
|
||||
.field("subprocess_timeout_secs", &self.subprocess_timeout_secs)
|
||||
.finish()
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user