Bug description
PR #4108 made health check parameters configurable but left behind two cleanup items identified during review:
-
Two dead zero-value guards in transparent_proxy.go that can never trigger, since the constructor and functional options guarantee positive values for healthCheckInterval and healthCheckPingTimeout.
-
A hardcoded context.WithTimeout(ctx, 5*time.Second) in healthcheck.go:checkMCPStatus that redundantly wraps the ping call. The MCPPinger already enforces its own timeout via http.Client.Timeout (set to the configured value through TOOLHIVE_HEALTH_CHECK_PING_TIMEOUT). The redundant context timeout silently caps the configured ping timeout at 5s — if a user sets TOOLHIVE_HEALTH_CHECK_PING_TIMEOUT=10s for a slow backend, the context cancels first and the configured value is ineffective.
Steps to reproduce
- Set
TOOLHIVE_HEALTH_CHECK_PING_TIMEOUT=10s
- The transparent proxy's
MCPPinger is correctly created with a 10s HTTP client timeout
- But
checkMCPStatus wraps the ping in context.WithTimeout(ctx, 5*time.Second), which fires first
Expected behavior
The configured ping timeout should be the effective timeout for health check pings.
Actual behavior
The hardcoded 5s context timeout in checkMCPStatus overrides any configured value above 5s.
Additional context
Bug description
PR #4108 made health check parameters configurable but left behind two cleanup items identified during review:
Two dead zero-value guards in
transparent_proxy.gothat can never trigger, since the constructor and functional options guarantee positive values forhealthCheckIntervalandhealthCheckPingTimeout.A hardcoded
context.WithTimeout(ctx, 5*time.Second)inhealthcheck.go:checkMCPStatusthat redundantly wraps the ping call. TheMCPPingeralready enforces its own timeout viahttp.Client.Timeout(set to the configured value throughTOOLHIVE_HEALTH_CHECK_PING_TIMEOUT). The redundant context timeout silently caps the configured ping timeout at 5s — if a user setsTOOLHIVE_HEALTH_CHECK_PING_TIMEOUT=10sfor a slow backend, the context cancels first and the configured value is ineffective.Steps to reproduce
TOOLHIVE_HEALTH_CHECK_PING_TIMEOUT=10sMCPPingeris correctly created with a 10s HTTP client timeoutcheckMCPStatuswraps the ping incontext.WithTimeout(ctx, 5*time.Second), which fires firstExpected behavior
The configured ping timeout should be the effective timeout for health check pings.
Actual behavior
The hardcoded 5s context timeout in
checkMCPStatusoverrides any configured value above 5s.Additional context
HealthChecker. Both existingMCPPingerimplementations handle timeouts internally: the transparent proxy pinger useshttp.Client.Timeout, and the httpsse pinger is non-blocking (fire-and-forget channel send). TheHealthCheckerdoesn't need to duplicate timeout enforcement that the pinger already owns.retryDelaywas already removed in PR Make transparent proxy health check parameters configurable #4108 commita41f8a87.