Skip to content

fix: Split timeouts #833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JoshC8C7 merged 6 commits into from
Mar 12, 2026
Merged

fix: Split timeouts #833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b33c77d
wire up split timeouts to config w/ fallback to kind of avoid breakin…
JoshC8C7 Mar 10, 2026
d33ec18
lint
JoshC8C7 Mar 10, 2026
3c9069d
header -> first_chunk
JoshC8C7 Mar 10, 2026
4c5cd92
Merge remote-tracking branch 'refs/remotes/origin/main' into split-ti…
JoshC8C7 Mar 11, 2026
5144ff4
Merge remote-tracking branch 'origin/main' into split-timeouts
JoshC8C7 Mar 12, 2026
2286053
fix
JoshC8C7 Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ batches:
max_backoff_ms: 10000 # Maximum backoff duration in milliseconds

# Timeout Settings
# timeout_ms: 600000 # DEPRECATED: applies uniformly to all three granular timeouts below
# timeout_ms: 600000 # DEPRECATED: splits into 90% first_chunk_timeout_ms, 10% body_timeout_ms
first_chunk_timeout_ms: 86400000 # Max time waiting for response headers (24 hours)
chunk_timeout_ms: 86400000 # Max idle time between response body chunks (24 hours)
body_timeout_ms: 86400000 # Max total time for entire response body (24 hours)
Expand Down
2 changes: 1 addition & 1 deletion config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ background_services:
max_backoff_ms: 10000 # Maximum backoff duration in milliseconds

# Timeout Settings
# timeout_ms: 600000 # DEPRECATED: applies uniformly to all three granular timeouts below
# timeout_ms: 600000 # DEPRECATED: splits into 90% first_chunk_timeout_ms, 10% body_timeout_ms
first_chunk_timeout_ms: 86400000 # Max time waiting for response headers (24 hours)
chunk_timeout_ms: 86400000 # Max idle time between response body chunks (24 hours)
body_timeout_ms: 86400000 # Max total time for entire response body (24 hours)
Expand Down
9 changes: 3 additions & 6 deletions dwctl/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,7 +1054,7 @@ pub struct DaemonConfig {
pub max_backoff_ms: u64,

/// Deprecated: use first_chunk_timeout_ms, chunk_timeout_ms, and body_timeout_ms instead.
/// If set, applies the same value uniformly to all three granular timeouts.
/// If set, splits into 90% first_chunk_timeout_ms and 10% body_timeout_ms.
/// Ignored when the granular timeout fields are explicitly set.
pub timeout_ms: Option<u64>,

Expand Down Expand Up @@ -1173,18 +1173,15 @@ impl DaemonConfig {
model_capacity_limits: Option<std::sync::Arc<dashmap::DashMap<String, usize>>>,
) -> fusillade::daemon::DaemonConfig {
// If the deprecated timeout_ms is set and the granular fields are at their
// defaults, apply it uniformly to all three granular timeouts. This is
// conservative: existing deployments using e.g. timeout_ms=600000 keep
// the same 10-minute budget for every phase instead of getting a
// surprising 60-second body_timeout_ms from a 90/10 split.
// defaults, split it: 90% header (connect + TTFT), 10% body.
let (first_chunk_timeout_ms, chunk_timeout_ms, body_timeout_ms) = if let Some(timeout) = self.timeout_ms {
if self.first_chunk_timeout_ms == 86_400_000 && self.chunk_timeout_ms == 86_400_000 && self.body_timeout_ms == 86_400_000 {
tracing::warn!(
timeout_ms = timeout,
"batch_daemon.timeout_ms is deprecated; \
use first_chunk_timeout_ms, chunk_timeout_ms, and body_timeout_ms instead"
);
(timeout, timeout, timeout)
(timeout * 9 / 10, 86_400_000, timeout / 10)
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeout * 9 / 10 can overflow u64 (wrap in release / panic in debug), producing incorrect timeouts. Consider using checked_mul/saturating_mul (or an order of operations that can’t overflow) and also ensure the 90/10 split can’t yield a 0ms timeout for small timeout_ms values (e.g., clamp to at least 1ms).

Suggested change
(timeout * 9 / 10, 86_400_000, timeout / 10)
(
(timeout.saturating_mul(9) / 10).max(1),
86_400_000,
(timeout / 10).max(1),
)

Copilot uses AI. Check for mistakes.
Comment on lines 1178 to +1184
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch hard-codes the default timeout value 86_400_000 both for the “are defaults” check and for chunk_timeout_ms in the returned tuple. To avoid drift if the defaults ever change, consider centralizing this as a named const or comparing/returning against DaemonConfig::default() (or self.chunk_timeout_ms in this branch).

Copilot uses AI. Check for mistakes.
Comment on lines 1175 to +1184
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new backwards-compat behavior for timeout_ms (splitting into first_chunk_timeout_ms/body_timeout_ms when granular fields are defaults) isn’t covered by tests. Given there’s an existing Rust test suite under dwctl/src/test/, it would be good to add a unit/integration test asserting the split values (and that explicitly-set granular fields cause timeout_ms to be ignored).

Copilot uses AI. Check for mistakes.
} else {
// Granular fields were explicitly set — ignore deprecated field
(self.first_chunk_timeout_ms, self.chunk_timeout_ms, self.body_timeout_ms)
Expand Down
Loading