Skip to content

Bound doctor diagnostics within client timeout - #729

Draft
brynary wants to merge 1 commit into
mainfrom
fix/doctor-timeout-budget
Draft

Bound doctor diagnostics within client timeout#729
brynary wants to merge 1 commit into
mainfrom
fix/doctor-timeout-budget

Conversation

@brynary

@brynary brynary commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

  • cap each external doctor probe at 15 seconds and keep Docker at 5 seconds
  • return provider-specific LLM timeout results while other provider checks complete
  • cap the server diagnostics operation at 25 seconds so the existing 30-second client deadline receives a structured 504 response
  • document the diagnostics 504 response in OpenAPI

Test plan

  • cargo nextest run --workspace
  • cargo nextest run -p fabro-server
  • cargo nextest run -p fabro-cli doctor
  • cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings
  • cargo +nightly-2026-04-14 fmt --check --all
  • cargo build -p fabro-api
  • bun run generate in lib/packages/fabro-api-client

Security

  • the diagnostics route still requires RequiredUser
  • the timeout response uses fixed text and does not expose upstream errors or credentials
  • no new retries or outbound targets are added

Copilot AI lite review requested due to automatic review settings August 5, 2026 12:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR bounds the server “doctor/diagnostics” work so it reliably completes within the existing client deadline, returning a structured 504 when diagnostics exceed a capped duration, while still providing per-provider timeout results for external probes.

Changes:

  • Added a server-side timeout wrapper for the diagnostics handler to return a structured 504 Gateway Timeout response.
  • Capped external diagnostic probes (e.g., LLM providers, GitHub, Brave, Daytona) to 15s and Docker to 5s, with provider-specific timeout outcomes.
  • Documented the new 504 response for diagnostics in the OpenAPI spec and added unit tests covering timeout behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
lib/apps/fabro-server/src/server/handler/system.rs Wraps the diagnostics handler with a 25s timeout and returns a structured 504 on timeout; adds a test for the timeout response shape.
lib/apps/fabro-server/src/diagnostics.rs Introduces shared timeout constants and applies them across external probes; adds helper(s) + tests for timeout reporting.
docs/public/api-reference/fabro-api.yaml Documents the diagnostics 504 timeout response in OpenAPI.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 258 to +262
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,
Comment on lines +691 to 694
diagnostics_response_with_timeout(
Box::pin(diagnostics::run_all(state.as_ref())),
SERVER_DIAGNOSTICS_TIMEOUT,
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants