Skip to content

Add dual-era MCP support and fault-injection modes#139

Merged
ChrisJBurns merged 8 commits into
mainfrom
feat/dual-era-fault-modes
Jul 23, 2026
Merged

Add dual-era MCP support and fault-injection modes#139
ChrisJBurns merged 8 commits into
mainfrom
feat/dual-era-fault-modes

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Closes #138.

Summary

Extends yardstick to serve as the shared harness for ToolHive's dual-era (Legacy
2025-11-25 / Modern 2026-07-28) proxy e2e tests (stacklok/toolhive#5837), per the design
in #138:

  • yardstick-server fault-injection modes (BACKEND_MODE=echo|barrier|hang|crash):
    a shared AddReceivingMiddleware hook, uniform across stdio/SSE/streamable-http.
    barrier buffers N requests and releases them together (for concurrency/
    cross-delivery testing), hang stops responding after N calls, crash exits
    non-zero after N calls. Lifecycle traffic (initialize/ping/server/discover/
    notifications/initialized) is exempted so the fault fires on the Nth real backend
    call, not during connection setup. All thresholds fail fast on misconfiguration
    (non-positive values) instead of silently never firing.
  • Modern (2026-07-28) support: go-sdk bumped to v1.7.0-pre.3 (isolated in its own
    commit — no stable v1.7.0 tag exists yet, easy to re-pin later), a --stateless
    flag/STATELESS env var wiring StreamableHTTPOptions.Stateless for streamable-http,
    and yardstick-client reporting the negotiated protocol version and correctly
    skipping Ping under Modern sessions (go-sdk rejects ping once a request carries
    the new protocol's per-request _meta).
  • e2e coverage: four new blocks in test/integration-test.sh (barrier/hang/crash/
    stateless), matching the script's existing per-transport conventions — real Docker
    containers, real client binary, run via the existing task test-integration CI gate.

Testing

  • task test / task lint pass.
  • task test-integration (test/integration-test.sh) passes end-to-end, including the
    four new blocks, run repeatedly against real Docker containers.
  • The two most failure-prone assertions (barrier's release-on-N-vs-safety-timeout, and
    crash's exit-code check) were verified by deliberately breaking the corresponding
    server logic, confirming the e2e test fails correctly, then reverting.

Open questions

See #138 for the open questions on the go-sdk pre-release pin, config surface, and
release/image-tagging cadence.

jhrozek and others added 7 commits July 22, 2026 21:57
Standalone counterState (hang/crash triggers at exactly N non-initialize/
non-ping calls) and barrier (buffer N arrivals, release together, with a
safety timer for a misconfigured N) primitives, not yet wired into main.go.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
Registers one AddReceivingMiddleware on the shared *mcp.Server, before the
transport switch, so echo/barrier/hang/crash behave identically across
stdio, SSE, and streamable-http. Mode config is env-only (BACKEND_MODE,
BARRIER_N, HANG_AFTER_N, CRASH_AFTER_N, BARRIER_TIMEOUT_SECONDS) so it works
uniformly through thv run -e, MCPServer CRD env, and plain k8s pod env.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
Isolated version bump, no source changes, so it can be re-pinned to a
stable v1.7.0 tag later with a one-line change once one ships. v1.6.1 has
no knowledge of protocol version 2026-07-28 or the reserved _meta keys
(io.modelcontextprotocol/protocolVersion, clientInfo, clientCapabilities);
v1.7.0-pre.3 is the only available version with real Modern-era support.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
--stateless / STATELESS env override (flag+env pattern, matching --transport/
--port), wired into StreamableHTTPOptions.Stateless for the streamable-http
branch only. Default false: Legacy behavior unchanged. stdio and sse are
unaffected since statelessness is a streamable-http-specific concept.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
GetServerInfo now prints c.session.InitializeResult().ProtocolVersion,
so a test can tell whether a Legacy (2025-11-25) or Modern (2026-07-28)
round-trip actually ran. InitializeResult() is guaranteed non-nil here
since GetServerInfo is only reachable after a successful Connect.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
Extends the project's real black-box e2e script (a required CI gate via
task test-integration) with four new blocks, matching its existing
per-transport conventions exactly: barrier mode (two concurrent calls
released together, with an elapsed-time check proving release-on-N rather
than the safety-timeout fallback), hang mode (a client -timeout-bounded
call that must fail), crash mode (docker wait for exit code 1 - run
without --rm, since docker wait racing container auto-removal is a real
Moby failure mode), and stateless mode (asserts Modern protocol
negotiation and a working round-trip, exercising the GetServerInfo Ping
guard fix end-to-end).

Every prior capability (all three transports) already had a corresponding
black-box block in this script; the fault-mode and --stateless work
landed with only Go-level coverage until now.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
The "echo" backend-mode value was the one mode never given a named
constant (modeHang/modeCrash/modeBarrier already existed), so it kept
getting re-typed as a raw string literal across main.go and the test
files. That pushed golangci-lint's goconst check over threshold and
failed CI - this was assumed pre-existing/out-of-scope at each prior
step, but the baseline before this branch only had a single "echo"
literal (the unrelated tool-name field), so the extra occurrences were
introduced by this branch's own commits. modeEcho now covers every
mode-value site; the tool's Name: "echo" is a separate, unrelated
concept and is left as its own single literal.

Signed-off-by: Jakub Hrozek <jakub@stacklok.com>

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panel review — PR #139 "Add dual-era MCP support and fault-injection modes"

Fixed point: main (three-dot diff, 7dc24dd)
Diff: 11 files, +954/−9, 7 commits
Panel: Spec (vs #138), Standards (CONTRIBUTING/READMEs/script conventions), Domain: secure-code-reviewer, software-architect, library-reuse-reviewer, code-duplication-reviewer, ux-reviewer, devex-reviewer


Spec — does this implement what #138 asked?

Source: issue #138 (fetched via API)

  • Missing: none. All four fault modes + knobs, lifecycle exemption, AddReceivingMiddleware hook, go-sdk bump, --stateless/STATELESS, client protocol-version reporting + Modern Ping skip, and the four e2e blocks are all present.
  • Scope creep: none meaningful. validateFaultConfig is unrequested but prevents silent misconfiguration; the new unit tests are quality practice.
  • Wrong: none. os.Exit(1) is non-zero, the barrier releases exactly N arrivals with a safety-timeout escape, and lifecycle methods are bypassed in both the barrier and decide paths.

(The spec's "open questions" are explicitly deferred decisions, not unmet requirements.)

Standards — does this follow project conventions?

Sources: CONTRIBUTING.md, README.md, cmd/*/README.md, test/integration-test.sh conventions. Tooling-skipped: golangci-lint, gofmt/vet, CI.

  • [Violation] 6 of 7 commit subjects exceed the 50-char limit in CONTRIBUTING.md (90aefa7 72, 19c8df4 64, 5ea2c8f 65, 9d82b62 71, 890c2b1 66, d1a350f 74). Only 7dc24dd (49) complies. All other commit rules (imperative, blank line, DCO trailer) pass. Squash-on-merge makes this mostly cosmetic.
  • Verified clean: server/client READMEs document the new flags/env vars in the existing pattern; the four new script blocks follow the existing #### START/END fencing, docker run/sleep 3/docker rm -f structure exactly — the PR's "matches conventions" claim holds.

Domain — what do the specialist reviewers say?

Ship-blockers (0)

None. Safe defaults (unset BACKEND_MODE → echo passthrough, verified by TestFaultMiddleware_EchoModeIsNoop), auth layering correct (HTTP authWrapper runs before the fault middleware on SSE/streamable-http), backward compatible for existing consumers.

Cross-confirmed (2) — highest confidence

  • [High] Unknown BACKEND_MODE silently runs as echo — ux-reviewer + devex-reviewer. validateFaultConfig switches only on the three known modes, so BACKEND_MODE=barier passes validation, cs.decide matches nothing, and every call passes through. Nothing tells the operator their requested fault will never fire — the exact failure mode the PR's own validation comment says it guards against, one level up. A test engineer who typo'd the env var will debug their client for a fault that was never armed. Fix: reject unknown non-empty values in validateFaultConfig (cmd/yardstick-server/main.go:281), listing the valid set.
  • [Medium] Malformed numeric/bool env vars silently fall back to defaults — ux-reviewer + devex-reviewer. envIntOr (cmd/yardstick-server/mode.go:12) returns the default on parse failure, so BARRIER_N=3x → 2, HANG_AFTER_N=two → 1; same for STATELESS=yes → false (main.go:250). Inconsistent with the fail-fast contract validateFaultConfig establishes for valid-but-non-positive values — and for a deterministic-fault-injection tool, "green-but-wrong test" is the worse failure mode. Fix: fail fast or at least warn on stderr when a set var fails to parse.

Mechanical fixes (3)

  • [Low] Hang mode leaks the goroutine permanently; ctx.Err() after the sleep is dead codemain.go:130. time.Sleep(1<<63-1) doesn't respect context; each hang trigger leaks a goroutine forever. Suggested: <-ctx.Done(); return nil, ctx.Err() — still hangs for the client, but reclaimable on cancellation. (secure-code-reviewer)
  • [Low] modeBarrier constant split from the other threemain.go:32 vs mode.go:38-40. One-line move so all mode strings live in one place. (software-architect, also noted by library-reuse-reviewer)
  • [Low] TestBarrier_NGreaterThanOneWaitsForFullCount redundantmode_test.go:184-194 is a subset of TestBarrier_ReleasesAtN (n=3 proves the same behaviour more strongly). −12 lines. (code-duplication-reviewer)

Judgement calls (3)

  • [Medium] No observability when a fault fires (or at startup) — ux-reviewer. Startup logs transport/port but never the active mode/thresholds/stateless; hang fires with a bare sleep, crash with a bare os.Exit(1) (no stderr). For the intended user — a test engineer reading docker logs — an injected hang is indistinguishable from a real deadlock in the thing under test, and a crash-mode exit shows code 1 with an empty log. Two cheap lines (startup fault mode: hang (HANG_AFTER_N=1), one stderr line at trigger) would close it. Counterpoint: silence may be intentional to keep fault behavior transparent to the client under test — but a startup line has no wire effect. Worth an explicit decision.
  • [Low] BARRIER_TIMEOUT_SECONDS unvalidatedmain.go:264. Negative → time.AfterFunc fires immediately → barrier silently becomes a no-op. Only knob not covered by validateFaultConfig. (secure-code-reviewer)
  • [Low] BARRIER_N=1 silently neuters barrier modejoin() releases immediately (mode.go:100-106), and validation accepts >= 1. Reject 1 or document that it disables the barrier. (ux-reviewer)

Polish (4)

  • [Info] go-sdk v1.7.0-pre.3 pre-release pin — known and called out in the PR's open questions; isolated in its own commit, low risk for a test harness. Re-pin to stable when tagged. (secure-code-reviewer)
  • [Info] counterState.mode docstring lists modeBarrier but decide never handles it — pick an explicit case or trim the docstring. (software-architect)
  • [Info] protocolVersionModern = "2026-07-28" hardcodes the SDK's unexported latest-protocol constant; lexicographic comparison is sound (SDK compares the same way internally) but will rot silently on the next protocol bump — the client test asserting the negotiated version is the real guard. (devex-reviewer)
  • [Info] README says HANG_AFTER_N counts "non-initialize/non-ping" calls, but the exemption also covers server/discover and notifications/initialized — a Modern-client user hand-counting calls could read the behavior as an off-by-one bug. One clause fixes it. (ux-reviewer)

Verified clean (noted to save re-checking)

  • Hand-rolled barrier: genuinely custom windowed-release semantics; no stdlib/x/sync replacement exists; mutex + b.win == w timer-race handling is correct. net: -0 lines possible on reuse. (library-reuse-reviewer)
  • The four new bash blocks are intentional convention-following, behaviourally distinct — extraction would be the Wrong Abstraction. (code-duplication-reviewer)
  • Lexicographic ISO-date version comparison is the standard Go idiom. (library-reuse-reviewer, software-architect)
  • go-sdk bump introduces no newly-restrictive behavior for existing callers (DNS-rebinding guard byte-identical since v1.4.0). (devex-reviewer)
  • No cross-session data leakage in barrier mode; lifecycle-method bypass well-tested. (secure-code-reviewer)

Gaps

None — no K8s/CI surface in this diff.


Summary

  • Spec: 0 findings — implements #138 as written
  • Standards: 1 violation (commit subject lengths, cosmetic under squash-merge)
  • Domain: 0 ship-blockers; 2 cross-confirmed (unknown-mode silent-echo, malformed-env silent-default); 3 mechanical; 3 judgement; 4 polish

Most important single issue: unknown BACKEND_MODE silently degrading to echo (main.go:281) — it undermines the fail-fast-on-misconfiguration contract this PR itself establishes, in a tool whose entire purpose is deterministic fault injection.

Each axis is orthogonal — verify each independently before shipping.

Config surface now fails fast on every silent-misconfiguration path
the review found: unknown BACKEND_MODE values are rejected at startup
instead of silently running as echo, and set-but-unparseable numeric
or boolean env vars (BARRIER_N=3x, STATELESS=yes) exit with a named
error instead of silently substituting defaults. validateFaultConfig
also covers BARRIER_TIMEOUT_SECONDS, the one knob it missed.

Observability: the armed mode and thresholds are logged at startup,
and each fault trigger (barrier release, hang, crash) writes one log
line, so an injected fault is distinguishable from a real wedge in
docker logs. The hang and crash e2e blocks now assert on those lines.

Hang mode now blocks on the request context instead of sleeping
~forever, so a cancelled request no longer leaks its goroutine; the
unit test asserts the hang releases on cancellation.

Also: move modeBarrier next to the other mode constants, drop the
redundant TestBarrier_NGreaterThanOneWaitsForFullCount (subset of
TestBarrier_ReleasesAtN), fix the counterState.mode docstring, note
the protocolVersionModern sync constraint, and document the full
lifecycle-exemption list plus BARRIER_N=1 passthrough in the README.

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
@ChrisJBurns
ChrisJBurns merged commit d380781 into main Jul 23, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add dual-era (Legacy/Modern) MCP support and fault-injection modes for ToolHive e2e tests

3 participants