Honor vMCP partial failure mode and timeouts

[lorenzozanee]

Summary

operational.failureHandling.partialFailureMode and operational.timeouts
(default / perWorkload) on VirtualMCPServer have been declared, validated,
defaulted, present in the CRD schema and documented since the vMCP config model
landed, but no production code ever read them: capability aggregation was
hardcoded to best-effort behavior (a failing backend was logged and skipped,
and the query only failed when every backend failed), and backend queries had
no per-request deadline.

This PR wires the two settings into the default aggregator:

• NewDefaultAggregator gains a WithOperationalConfig option (variadic, so
  existing callers are unaffected) that carries partialFailureMode and the
  timeout settings into the aggregator.
• With partialFailureMode: fail, the first failing backend now fails the
  whole capability query and the remaining in-flight queries are cancelled.
  best_effort preserves the previous log-and-continue behavior.
• timeouts.default and timeouts.perWorkload now bound each backend
  capability query with a per-backend context deadline (perWorkload entries
  keyed by workload name take precedence).
• cli/serve.go passes the loaded operational config to the aggregator via a
  small newAggregator helper so the production wiring is directly testable.

Fixes #6164

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

Unit tests for the affected vmcp packages pass (go test ./pkg/vmcp/...),
including race detection on the new tests. The regression tests fail on the
pre-fix code (the aggregator ignored the setting) and pass after the fix.
go vet and gofmt are clean.

Does this introduce a user-facing change?

Yes. Three behavior changes deserve attention:

1. Default failure mode is now effective. partialFailureMode defaults to
   fail (the CRD default and the documented default). Previously every
   deployment behaved as best-effort regardless of configuration; now a
   deployment that does not set the field fails capability queries when any
   backend's live capability query fails. Deployments that rely on the old
   best-effort behavior should set partialFailureMode: best_effort
   explicitly. Note that quick mode (thv vmcp serve --group) does not run
   the config defaulting step, so it keeps best-effort behavior.
2. operational.timeouts currently bounds capability aggregation queries.
   The config field documents a default timeout for backend requests; this PR
   applies it to the capability queries in aggregation. Other backend request
   paths (session establishment, individual tool calls) are not yet bound by
   this setting and keep their existing fixed timeouts.
3. ListBackends under the authorized view aggregates the full backend set
   without health filtering (health is a status, not a visibility filter).
   With fail mode, a single unreachable backend now fails that call, where
   previously it was skipped. This matches the documented "fail: Fail entire
   request if any backend is unavailable" contract.

Special notes for reviewers

The aggregator's QueryAllCapabilities doc comment, the Aggregator
interface contract and core.aggregateBackends were updated to describe the
mode-dependent failure behavior so the docs match the implementation.

[jerm-dro]

Thanks for picking this up — the diagnosis is right and the timeout wiring is good. The tests are deterministic (blocking on ctx.Done() rather than sleeps), and TestNewAggregator_WiresOperationalFailureMode pinning the production wiring is exactly the kind of thing that stops this regressing quietly.

fail doing what it says when someone deliberately configures it is fine. My concern is narrower: it shouldn't be the default.

blocker: default to best_effort

fail is the defaulted value (defaults.go:32, applied via EnsureOperationalDefaults in both converter.go:155 and yaml_loader.go:79). Since the field has never been read, every deployment in existence has effectively been running best_effort — so on upgrade, everyone who never touched this setting silently switches to the stricter behaviour.

The PR suggests operators set best_effort explicitly to opt out. That asks every existing user to take action just to keep the behaviour they already have, for a setting they never configured, to avoid a failure mode they've never seen.

Defaulting to best_effort makes the knob honest without changing anyone's behaviour, and it resolves the quick-mode divergence for free — generateQuickModeConfig (serve.go:557) doesn't call EnsureOperationalDefaults, so as written --group stays best-effort while the operator gets fail. Same product, different reliability default depending on how you launched it.

blocker: the default lives in two places — change both

Heads up, because this one is easy to get half-right:

• Go: defaultPartialFailureMode = "fail" (defaults.go:32)
• CRD schema: default: fail, in both API versions (toolhive.stacklok.dev_virtualmcpservers.yaml:1863 for v1alpha1, :5410 for v1beta1)

The API server applies the CRD default at admission, before the Go code ever sees the object. Change only the Go constant and the API server keeps stamping fail onto stored resources — the diff looks correct and nothing actually changes. Needs task operator-generate / task operator-manifests and task crdref-gen from the repo root.

blocker: the tests verify the mechanism, not the behaviour

The five new tests all follow the same shape: construct a config with the mode set explicitly, make a backend fail, assert the aggregator returns an error. That proves the component does what its code says. It doesn't establish what the system does, which is what needs to be pinned here.

Two gaps follow from that, and both sit exactly where the risk is:

Nothing exercises the defaulted path. Every test sets the mode by hand. Real deployments leave the field blank and let EnsureOperationalDefaults fill it in — a code path no test touches. So the behaviour that ships to everyone who upgrades and changes nothing is the one behaviour with no coverage.

Worth knowing before you write that test: the existing default assertions are assert.Equal(t, defaultPartialFailureMode, cfg.FailureHandling.PartialFailureMode) (defaults_test.go:32,97,173) — the constant compared against itself. Flip the constant and they all still pass while telling you nothing. Whatever asserts the new default needs to hardcode the literal "best_effort", or it isn't testing anything. Same applies to a check that the Go default and the CRD schema default agree; crd_cli_roundtrip_test.go looks like the natural home, and it's cheap insurance against exactly the two-places drift above.

Nothing shows what a user experiences. aggregatedView (core_vmcp.go:560) is on the path of every CallTool, ListTools, ReadResource and GetPrompt — so under fail, one failing backend fails requests that have nothing to do with it, for every user of that vMCP. That's the actual consequence of the change, and you can't see it anywhere in the diff; you have to already know how the layers compose to work it out.

Not a correctness objection now that fail is opt-in. But a test at the core level — one failing backend, an unrelated CallTool, assert it fails — turns an implicit design decision into an explicit, executable one. That's worth more than another test of the aggregator internals.

suggestion: the CRD description doesn't match the behaviour

partialFailureMode is documented as "fail: Fail entire request if any backend is unavailable." But unavailable backends are already removed by the health filter (core_vmcp.go:560 → health.ShouldAdvertise) before aggregation runs. So fail never fires for the documented case, and fires only for backends that pass the health filter but fail their live query — which the description doesn't cover.

Same issue in docs/operator/virtualmcpserver-kubernetes-guide.md:565-567, which tells operators to use fail "to require all backends to be healthy." Worth a pass over both now that the field does something, so nobody predicts the wrong behaviour from the docs.

---

Last thing: CI doesn't appear to have run here (likely a fork awaiting workflow approval), and the test plan lists go test ./pkg/vmcp/... with task lint-fix unchecked. AGENTS.md asks for task test / task lint-fix specifically because the Taskfile carries exclusions and flags the direct commands miss — could you run those? Given the CRD regeneration above, task lint-fix matters more than usual here.