Make proxy request read timeout configurable - #6285
Conversation
9c4f247 to
14532df
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6285 +/- ##
==========================================
+ Coverage 78.77% 78.82% +0.04%
==========================================
Files 778 778
Lines 77490 77534 +44
==========================================
+ Hits 61042 61114 +72
+ Misses 16443 16415 -28
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f6df8b5 to
2f34254
Compare
13af64e to
25c0bcd
Compare
5502064 to
3f617bc
Compare
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: general-quality, transport-architecture, kubernetes-api-tests, codex
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Workload upgrades drop the configured proxy read timeout | 10/10 | HIGH | Fix |
| 2 | Transport factory silently accepts negative read timeouts | 9/10 | LOW | Fix |
| 3 | Tests do not prove the new configuration changes live proxy behavior | 9/10 | MEDIUM | Fix |
| 4 | Transport documentation needs timeout wording cleanup | 7/10 | LOW | Fix |
Overall
This feature uses the right shape: a unit-explicit optional RunConfig field, API-level Kubernetes validation, runtime validation for persisted configs, and typed propagation through all three proxy implementations while preserving the secure 30-second default. The v1beta1 change is additive, generated artifacts are synchronized, and the implementation does not introduce a WriteTimeout that could disrupt SSE responses.
One correctness issue should be fixed before merge: ToolHive's workload-upgrade rebuild path does not preserve the new field, so thv upgrade silently resets a custom timeout to 30 seconds. The remaining findings concern fail-loud validation at the exported transport factory boundary and coverage that currently verifies stored values rather than the live proxy behavior required by the new CLI and operator entry points.
Documentation
docs/arch/03-transport-architecture.md should qualify the preceding statement about transparent proxies having no request timeout so it clearly refers to the MCP response-correlation timeout, and remove the trailing whitespace on the new paragraph.
Generated with Claude Code
3f617bc to
074ca46
Compare
|
@Sanskarzz I merged one of your PR's and there's a couple conflicts on this PR - you'll need to rebase with main and I can re-review |
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: code-reviewer, toolhive-expert, kubernetes-expert, codex
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Workloads API update drops the configured proxy read timeout | 10/10 | MEDIUM | Follow up |
| 2 | Public configuration entry points lack live E2E coverage | 9/10 | MEDIUM | Follow up |
Overall
This PR adds a per-workload proxy request read timeout across the CLI, persisted RunConfig, transport factory, all proxy implementations, and the MCPServer CRD. The timeout semantics, validation, upgrade preservation, generated API artifacts, and top-level CRD placement are coherent and backward-compatible; resourceOverrides.proxyDeployment is infrastructure-only and is not a better home for this behavioral setting.
Two non-blocking integration gaps remain. Workloads API edits can silently discard a timeout configured through the CLI, and the public CLI and MCPServer configuration paths do not yet have live end-to-end coverage. The focused transport and operator tests otherwise provide strong confidence in the implementation.
Generated with Claude Code
c654d91 to
63870b9
Compare
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: code-reviewer, toolhive-expert, kubernetes-expert, codex
Consensus Summary
No actionable findings.
Overall
This PR consistently threads a per-workload proxy request read timeout through the CLI, persisted RunConfig, Workloads API, transport factory, all proxy implementations, and the MCPServer CRD. The top-level spec.proxyReadTimeout placement is appropriate for workload behavior, and omission or zero retains the 30-second default while negative values are rejected.
The Workloads API preserves the timeout across GET-to-edit round trips, and both public configuration paths have live coverage. The slow-upload operator test exercises the configured inbound deadline because ToolHive consumes the request body before contacting the backend.
The branch currently conflicts with main in pkg/api/v1/workload_service.go; the rebase should retain both this PR's timeout parsing and main's OIDC client-secret preservation. The failing operator integration check is an unrelated timeout in existing MCPRemoteProxy/OIDC deletion coverage.
Generated with Claude Code
|
@Sanskarzz Have approved but you've got a couple more conflicts to resolve |
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
63870b9 to
b7f0221
Compare
Summary
Proxy HTTP servers currently protect against slow or stalled request uploads with a fixed 30-second read timeout.
That secure default cannot be adjusted for workloads that legitimately need more time, or tightened for environments that
want a shorter limit.
thv run --proxy-read-timeoutand MCPServerspec.proxyReadTimeoutconfiguration.Fixes #5503
Type of change
Test plan
task test)task test-e2e)task lint-fix)task build)task operator-test-integration)Generated artifacts were refreshed with
task operator-generate,task operator-manifests,task crdref-gen, andtask docs.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.The MCPServer API change is additive and optional. Existing resources continue to use the proxy's 30-second default.
Changes
cmd/thv/app/run_flags.go--proxy-read-timeoutCLI flag and RunConfig builder wiring.pkg/runner/{config.go,config_builder.go,runner.go}pkg/transport/{types/transport.go,factory.go,stdio.go,http.go}cmd/thv-operator/api/v1beta1/mcpserver_types.gocmd/thv-operator/controllers/mcpserver_runconfig.goDoes this introduce a user-facing change?
Yes. CLI users can set
thv run --proxy-read-timeout <duration>, and Kubernetes users can setMCPServer.spec.proxyReadTimeout.Both accept Go duration values such as
45sor2m. Omitting the setting or specifying zero retains the 30-second default.Implementation plan
Approved implementation plan
persisted RunConfig boundaries.
transport.Configand the existingtransport factory to each proxy's typed
WithReadTimeoutoption.option for zero values.
affected documentation and CRD/OpenAPI artifacts.
Special notes for reviewers
mechanism.
WriteTimeout, so long-lived SSE responses remain unaffected.thv mcp serveretain their independent timeout behavior.