From 0fdab9fdb36597a9724a6352e7d91265db1b730c Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 6 Sep 2025 14:06:22 +0200 Subject: [PATCH 1/5] test: fix timeouts --- .archgate/adrs/ARCH-005-testing-standards.md | 48 ++++++++++++++++++- .../agent-memory/archgate-developer/MEMORY.md | 1 + .github/workflows/code-pull-request.yml | 1 + .github/workflows/release-binaries.yml | 2 + .github/workflows/release.yml | 3 ++ package.json | 4 +- tests/mcp/resources.test.ts | 7 +-- tests/mcp/server.test.ts | 8 ++-- tests/mcp/tools.test.ts | 7 +-- tests/mcp/tools/check.test.ts | 7 +-- tests/mcp/tools/list-adrs.test.ts | 7 +-- tests/mcp/tools/review-context.test.ts | 7 +-- tests/mcp/tools/session-context.test.ts | 7 +-- 13 files changed, 84 insertions(+), 25 deletions(-) diff --git a/.archgate/adrs/ARCH-005-testing-standards.md b/.archgate/adrs/ARCH-005-testing-standards.md index e434dc18..0695f6dd 100644 --- a/.archgate/adrs/ARCH-005-testing-standards.md +++ b/.archgate/adrs/ARCH-005-testing-standards.md @@ -40,6 +40,7 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - Use `tests/fixtures/` for sample data files - Use temp directories (`mkdtemp`) for tests that write to the filesystem - Clean up temp directories in `afterEach` or `afterAll` +- **Close external SDK instances** (servers, clients, transports) in `afterEach` or `afterAll` by calling their cleanup method (e.g., `await server.close()`). Manage their lifecycle in `beforeEach`/`afterEach` rather than inside individual test bodies so cleanup is guaranteed. - Test public module interfaces, not private implementation details - Use descriptive test names that explain the expected behavior @@ -48,6 +49,7 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - Don't test private implementation details — test the public API of each module - Don't depend on network access in unit tests - Don't leave temp files after test runs +- **Don't leave external SDK instances open after tests** — instances from libraries such as `@modelcontextprotocol/sdk` hold internal references (e.g., `AjvJsonSchemaValidator` backed by `ajv`) that keep Bun's event loop alive on Linux, causing `bun test` to hang indefinitely after all tests complete even though every test passes. Always call the cleanup method in `afterEach`. - Don't skip tests without a tracking issue - Don't import test utilities from `node:test` — use Bun's built-in `bun:test` module @@ -83,6 +85,37 @@ describe("runChecks", () => { }); ``` +### Good Example — External SDK Cleanup + +```typescript +// tests/mcp/resources.test.ts +import { describe, expect, test, beforeEach, afterEach } from "bun:test"; +import { mkdtempSync, rmSync, mkdirSync } from "node:fs"; +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { registerResources } from "../../src/mcp/resources"; + +describe("registerResources", () => { + let tempDir: string; + let server: McpServer; + + // GOOD: lifecycle managed in beforeEach/afterEach, not inside test bodies + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-mcp-res-test-")); + mkdirSync(join(tempDir, ".archgate", "adrs"), { recursive: true }); + server = new McpServer({ name: "test", version: "0.0.0" }); + }); + + afterEach(async () => { + await server.close(); // GOOD: releases internal validator references + rmSync(tempDir, { recursive: true, force: true }); + }); + + test("does not throw when registering resources", () => { + expect(() => registerResources(server, tempDir)).not.toThrow(); + }); +}); +``` + ### Bad Example ```typescript @@ -102,6 +135,13 @@ it("writes output", async () => { await Bun.write("/tmp/test-output.json", data); // No cleanup — file persists after test }); + +// BAD: SDK instance created inside test body — no guaranteed cleanup path +it("registers resources", () => { + const server = new McpServer({ name: "test", version: "0.0.0" }); + // server.close() never called — event loop held open on Linux + expect(() => registerResources(server, tempDir)).not.toThrow(); +}); ``` ## Consequences @@ -124,13 +164,15 @@ it("writes output", async () => { - **Mitigation:** The project pins a specific Bun version via `.prototools` (currently 1.3.8). Test runner API changes are caught during controlled Bun upgrades with full test suite validation. - **Coverage reporting gaps** — `bun test --coverage` may not report accurate coverage for all code paths, especially for dynamically imported modules. - **Mitigation:** Coverage is a guideline (80% target), not a hard gate. Critical modules (engine, formats) are tested thoroughly regardless of coverage numbers. +- **Third-party SDK event loop retention** — External SDK instances that hold internal resource references (e.g., `AjvJsonSchemaValidator` inside `@modelcontextprotocol/sdk`) keep Bun's event loop alive on Linux after all tests complete, causing `bun test` to hang indefinitely. This does not surface on macOS (event loop drains normally there), making it a Linux-CI-only failure that is hard to reproduce locally. + - **Mitigation:** Always manage SDK lifecycle in `beforeEach`/`afterEach` and call the cleanup method (`close()`, `destroy()`, `disconnect()`) in `afterEach`. Add `timeout-minutes` to CI jobs as a safety net — the `code-pull-request.yml` job is set to 10 minutes to cap any future regressions. ## Compliance and Enforcement ### Automated Enforcement - **Archgate rule** `ARCH-005/test-mirrors-src`: Scans all source files in `src/` and verifies a corresponding `.test.ts` file exists in `tests/`. Severity: `error`. -- **CI pipeline**: `bun test` runs on every pull request. Test failures block merge. +- **CI pipeline**: `bun test --timeout 60000` runs on every pull request. Test failures and per-test timeouts block merge. All workflow jobs have `timeout-minutes` set to prevent indefinite hangs. ### Manual Enforcement @@ -138,9 +180,11 @@ Code reviewers MUST verify: 1. New source files have corresponding test files 2. Tests use temp directories for filesystem operations (no hardcoded paths) -3. Tests clean up after themselves (`afterEach`/`afterAll` cleanup) +3. Tests clean up after themselves (`afterEach`/`afterAll` cleanup) — including both temp directories and external SDK instances +4. Tests that instantiate SDK objects (servers, clients, connections) manage their lifecycle in `beforeEach`/`afterEach`, not inside individual test bodies ## References - [Bun test runner documentation](https://bun.sh/docs/cli/test) - [ARCH-001 — Command Structure](./ARCH-001-command-structure.md) — In-process execution enables testing commands directly without process spawning +- [ARCH-006 — Dependency Policy](./ARCH-006-dependency-policy.md) — Third-party dependencies introduce runtime behaviors (like event loop retention) that must be accounted for in test teardown diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 3800baa5..0926933c 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -26,6 +26,7 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv ## Patterns & Fixes +- **`McpServer` event loop retention on Linux** — `new McpServer()` from `@modelcontextprotocol/sdk` creates an `AjvJsonSchemaValidator` (backed by `ajv` + `ajv-formats`) that keeps Bun's event loop alive on Linux after tests complete, causing `bun test` to hang for 30+ minutes. macOS drains the event loop fine — this is a Linux-CI-only failure. Fix: manage server lifecycle in `beforeEach`/`afterEach` and call `await server.close()` in `afterEach`. Also captured in ARCH-005 Don'ts. - **Bun import cache-busting**: Bun caches `import()` per-process. For long-running processes (MCP server), append `?t=${Date.now()}` to the import path to force re-reading from disk. Applied in `src/engine/loader.ts`. - **Never use `bunx prettier` directly** — Always use `bun run format` (to fix) or `bun run format:check` (to verify). Using `bunx prettier` can fail or use a different version than the project's devDependency. The same applies to all dev tools: prefer `bun run