fix(mcp): inject livez probe to kill mcp-standalone test flake (#449)#451
Conversation
The MCP standalone shim's REST proxy probed `:3111/agentmemory/livez` via a hard-coded `fetch()` with a 2s AbortController timeout. In CI, vitest's mock setup raced the in-flight probe — tests randomly failed on call counts and response-shape assertions depending on whether the probe completed before each test's own deadline. This file has been the source of every "10-11 pre-existing failures unrelated to this PR" footnote for the last five releases. Add a `setLivezProbe(fn)` module-level seam in `src/mcp/rest-proxy.ts` (option A in the issue). Default stays the real fetch-based probe — wire behaviour is unchanged in production. `resetHandleForTests()` also resets the probe so tests can't accidentally leak a stub across files. In `test/mcp-standalone.test.ts`, install an instant `ok:false` probe in `beforeEach` so the shim always takes the deterministic InMemoryKV fallback path. Replace `globalThis.fetch` with a trap that throws if hit — any regression that bypasses the seam fails loudly instead of timing out silently and re-introducing the flake. Validation: 10/10 consecutive runs green (~185ms each, down from 2.2s); proxy test file unaffected (14/14); full suite 988/988 stable across 3 runs.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
ChangesProbe Injection and Test Stabilization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mcp/rest-proxy.ts (1)
43-50: ⚡ Quick winRemove WHAT-style explanatory comments in source per repo rule.
These blocks are explanatory comments about behavior; prefer self-descriptive naming and keep source comments minimal.
Proposed cleanup
-/** - * Probes the agentmemory server's livez endpoint. Returns a Response-shaped - * object whose `ok` flag drives the proxy/local-fallback decision. - * - * Tests can swap this via {`@link` setLivezProbe} to avoid the real 2s - * AbortController race that destabilises mcp-standalone test runs (`#449`). - * Production callers should leave it on the default. - */ export type LivezProbe = ( url: string, timeoutMs: number, headers: Record<string, string>, ) => Promise<{ ok: boolean; status?: number; statusText?: string }>; @@ -/** - * Override the livez probe. Intended for tests — production code should rely - * on the default fetch-based probe. Calling without an argument restores the - * default. Pair with {`@link` resetHandleForTests} so the cached handle is - * dropped before the next call. - */ export function setLivezProbe(fn?: LivezProbe): void { livezProbe = fn ?? defaultLivezProbe; }As per coding guidelines, "Do not use code comments explaining WHAT — use clear naming instead".
Also applies to: 68-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/rest-proxy.ts` around lines 43 - 50, Replace the verbose WHAT-style block comment with a concise, self-descriptive doc and/or a clearer name: remove the multi-line explanatory comment above the livez probe implementation and instead use a single-line JSDoc like "Default livez probe used by setLivezProbe" or rename the probe to a descriptive identifier (e.g., defaultLivezProbe or probeAgentMemoryLivez) so callers and tests (setLivezProbe) are self-explanatory; apply the same cleanup to the other explanatory block at 68-73. Ensure the symbols setLivezProbe and the probe function/variable are updated to match any renaming and keep comments minimal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mcp/rest-proxy.ts`:
- Around line 43-50: Replace the verbose WHAT-style block comment with a
concise, self-descriptive doc and/or a clearer name: remove the multi-line
explanatory comment above the livez probe implementation and instead use a
single-line JSDoc like "Default livez probe used by setLivezProbe" or rename the
probe to a descriptive identifier (e.g., defaultLivezProbe or
probeAgentMemoryLivez) so callers and tests (setLivezProbe) are
self-explanatory; apply the same cleanup to the other explanatory block at
68-73. Ensure the symbols setLivezProbe and the probe function/variable are
updated to match any renaming and keep comments minimal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f70312b7-73d6-4a16-b61c-32ed8af26cf5
📒 Files selected for processing (2)
src/mcp/rest-proxy.tstest/mcp-standalone.test.ts
Closes #449.
DI shape chosen: option A (module-level singleton with override)
Added
setLivezProbe(fn?: LivezProbe)insrc/mcp/rest-proxy.ts. Testsswap in an instant-fail probe before each call; default stays the real
fetch-based probe so production wire behaviour is identical.
Why option A over constructor DI (option B): the
rest-proxy.tsmoduleis already singleton-shaped —
resolveHandle()reads a module-scoped cache,invalidateHandle()andresetHandleForTests()mutate that cache. The probeis the same shape of dependency. A constructor seam would have meant
refactoring every caller of
resolveHandle()(and thehandleToolCallchain in
standalone.ts) to thread the proxy instance through. That'sbeyond "DI seam, don't touch the logic." Option A keeps the diff to two
files and 84 added lines.
Also wired the override into
resetHandleForTests()so a stub from one testfile can't leak into another.
10-run flake-kill validation
25 tests = 24 original + 1 new regression-guard (
livez probe stub is invoked instead of the real fetch (issue #449)) that asserts the DI seamis wired and the real network is never touched.
Pre-fix baseline was 2.2s per run with intermittent timeouts; post-fix is
~185ms with zero variance across 10 runs.
Full-suite stability
988 = the 987 main has today + 1 new regression-guard test in this PR.
mcp-standalone-proxy.test.ts(the sibling file that intentionally exercisesthe proxy path via
globalThis.fetchstubbing) still passes 14/14unchanged —
resetHandleForTests()restores the default probe in itsbeforeEach/afterEach, so it never sees the stub installed by thestandalone file.
npm run buildclean.Diff stat
Constraints honoured
--no-verifylocal KV) is byte-identical in production
fix/449-mcp-standalone-probe-di, conventional commit, not mergedSummary by CodeRabbit