vmcp: operational.timeouts and partialFailureMode are validated and documented but never read
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 2.2k
- Forks
- 300
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 184
Description
Bug description
Two operational config fields on VirtualMCPServer are fully plumbed as configuration surface — declared in the Go config structs, validated on input, defaulted, present in the CRD schema, and documented — but no production code ever reads them. Setting them has no effect, and there is no warning or error to indicate that.
operational.failureHandling.partialFailureMode(fail|best_effort)operational.timeouts.defaultandoperational.timeouts.perWorkload
The failure mode is silence: the operator accepts the value, the pod starts cleanly, the CRD documents what it should do, and nothing changes. This is arguably worse than an unimplemented feature that is absent, because the documented presence of the knob actively misleads.
Evidence
partialFailureMode — referenced only by config plumbing:
- Declared:
pkg/vmcp/config/config.go:651 - Validated:
pkg/vmcp/config/validator.go:466-468 - Defaulted to
fail:pkg/vmcp/config/defaults.go:32,62 - In the CRD:
deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml:1779 - Documented:
docs/operator/crd-api.md:502, anddocs/operator/virtualmcpserver-kubernetes-guide.md:562recommendsbest_effort - Consumers: none. A repo-wide search for the field outside
pkg/vmcp/config/and generated deepcopy code returns no production hits.
Separately, capability aggregation is hardcoded to best-effort behaviour regardless of the setting: QueryAllCapabilities logs and continues past a failing backend (pkg/vmcp/aggregator/default_aggregator.go:173-175), failing only when every backend fails (:192). So the effective behaviour is always best_effort, and a user who explicitly sets fail — the default — does not get it.
operational.timeouts — same pattern:
- Declared:
pkg/vmcp/config/config.go:611,615 - Validated:
pkg/vmcp/config/validator.go:416-423 - Defaulted to
30s:pkg/vmcp/config/defaults.go:35,55 - In the CRD:
...virtualmcpservers.yaml:1820; documented atdocs/operator/crd-api.md:749 - Consumers: none. Only
*_test.gofiles reference them.
Note there is a ready-made seam for the timeout: vmcpsession.WithBackendInitTimeout (pkg/vmcp/session/factory.go:155) exists and has no production caller. The session factory currently uses a compiled-in defaultBackendInitTimeout = 30 * time.Second (factory.go:31).
Steps to reproduce
apiVersion: toolhive.stacklok.dev/v1beta1
kind: VirtualMCPServer
spec:
config:
operational:
failureHandling:
partialFailureMode: best_effort
timeouts:
default: 5s
Apply it. The resource is accepted and the pod starts. No behaviour changes, and nothing in the logs indicates the settings were ignored.
Expected behavior
Either the fields take effect, or the system tells you they don't. Any of:
- Implement them.
- Remove them from the config structs, CRD, and docs.
- Keep the schema for compatibility but log a clear warning at startup when a non-default value is set, and mark them unimplemented in the docs.
Actual behavior
Silently ignored.
Why this is worth fixing beyond tidiness
This has already misled users twice.
1. It set a false premise in #5861. That report opens by assuming these are working features:
"health monitoring + circuit breaker + partial-failure-mode (
best_effort) already exist as designed features for exactly this kind of scenario"
2. A user in #5861 reasoned carefully about a tradeoff for a knob that does nothing:
"We considered raising
operational.timeouts.perWorkloadfor just this backend, but since its own worst-case latency is 15-25+s, doing so would only convert 'occasional hard failure' into 'every session against this tenant reliably takes 15-25s'"
They rejected it on a cost/benefit analysis of behaviour that does not exist. Had they decided the other way, they would have set it, observed no change, and had no way to find out why — while debugging a production incident.
3. #4856 is related but did not fix this. That issue reported a CrashLoopBackOff when setting partialFailureMode: best_effort, caused by a validator/CRD enum mismatch. It was fixed in #4865 and closed as completed. That fix corrected the validation so the pod no longer crashes — it did not make the field do anything. So the current state is strictly more confusing than before: previously setting best_effort failed loudly; now it succeeds and silently does nothing.
Additional context
- Related: #5861 — a slow backend dragging a tenant's
initializesuccess rate down. PR #6162 fixes the health-gating half of that. Wiringoperational.timeoutswould give operators a way to bound per-session backend-connect cost, which also mitigates the cold-start window #6162 leaves open (before the first health check completes, a slow backend's status is not yet known and sessions still block on it). - Related: #4856 / #4865 — the enum-validation fix described above.
- Note on scope for
operational.timeouts: the field is documented as "the default timeout for backend requests" (config.go:608). Wiring it toWithBackendInitTimeoutwould narrow that to session-establishment timeout specifically. That may warrant a distinct field rather than repurposing this one — worth deciding before implementing, since the CRD is versioned API surface. - Implementing
partialFailureMode: failis a behaviour change for existing deployments: aggregation is currently always best-effort, so anyone on thefaildefault is silently gettingbest_efforttoday and would start seeing hard failures. That argues for treatingbest_effortas the real default, or gating the change.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with pkg/vmcp/config/config.go, validator.go, defaults.go, pkg/vmcp/aggregator/default_aggregator.go, and pkg/vmcp/session/factory.go, then review the referenced CRD and operator documentation. Decide with maintainers whether to implement, remove, or warn about the unused settings, accounting for compatibility and the existing WithBackendInitTimeout seam; done means the chosen behavior is reflected consistently in code, schema, docs, and tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- api, backend, devops
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100