stacklok / stacklok/toolhive

vmcp: operational.timeouts and partialFailureMode are validated and documented but never read

Open
#6,164 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug needs-triage operator vmcp
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.default and operational.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, and docs/operator/virtualmcpserver-kubernetes-guide.md:562 recommends best_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 at docs/operator/crd-api.md:749
  • Consumers: none. Only *_test.go files 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:

  1. Implement them.
  2. Remove them from the config structs, CRD, and docs.
  3. 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.perWorkload for 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 initialize success rate down. PR #6162 fixes the health-gating half of that. Wiring operational.timeouts would 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 to WithBackendInitTimeout would 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: fail is a behaviour change for existing deployments: aggregation is currently always best-effort, so anyone on the fail default is silently getting best_effort today and would start seeing hard failures. That argues for treating best_effort as the real default, or gating the change.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.