Skip to content

test(middleware): run channelApi suites in-process (#564) - #613

Merged
Weegy merged 3 commits into
byte5ai:mainfrom
sneumannb5:feat/564-harden-tests-against-parallelism
Aug 7, 2026
Merged

test(middleware): run channelApi suites in-process (#564)#613
Weegy merged 3 commits into
byte5ai:mainfrom
sneumannb5:feat/564-harden-tests-against-parallelism

Conversation

@sneumannb5

@sneumannb5 sneumannb5 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

What

Harden the channelApi router test suites against parallelism (Closes #564, follow-up to #550). The three suites (adminKeysRouter, chatRouter,chatRouterPrivacyIntegration) no longer bind a real Express server on an ephemeral port + make loopback fetch calls; they run through a shared in-process transport (test/support/inProcessHttp.ts) instead.

Why

node --test forks a worker per file across availableParallelism() - 1 workers with no --test-concurrency cap, so the port bind + TCP accept + round-trips these suites paid stretched under a loaded runner — the failure mode #564 traces (disjoint failures, one 301107 ms hang). This attacks that cost directly rather than masking it: a real http.Server that is never listen()ed, fed a synthetic in-memory socket pair and driven by Node's own HTTP client (so responses are parsed by Node, not by hand). No port, no handshake, nothing to contend for. --test-concurrency is deliberately left unset — bounding parallelism hides the fragility instead of removing it.

Test plan

  • npx tsc --noEmit (middleware) — 0 errors
  • eslint on the new/changed files — clean
  • 3 converted suites + fixture self-test: 36/36 pass (30 suite + 6 fixture)
  • Full npm run test: same 404 pre-existing failures as clean main (build/Postgres-dependent), +6 passing, 0 new failures
  • Fixture self-test mutation-checked: breaking status / headers / streamed body / the no-port-bound invariant each fails a distinct case
  • Under 24 CPU hogs on 12 cores (loaded-runner sim): in-process ~820–925 ms vs TCP ~835–1025 ms, tighter variance

Risk / blast radius

Test-only. No schema, no public API, no CI/release tooling, no new env-var. Assertions and per-suite test counts (30) unchanged — streaming (NDJSON, flushHeaders) and mid-turn throws run the same code path. New file test/support/inProcessHttp.ts is a test helper; not shipped.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@Weegy

Weegy commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Merge prep + independent blast-radius verification

I merged main into this branch (7 commits behind, including the large MCP wave PR #550) and re-verified from scratch. Pushed as ff946ab5.

The merge is a no-op for the diff, which is the good news. The four package.json / package-lock.json edits from the build fix commit (fast-uri 3.1.4→3.1.5, brace-expansion 5.0.8→5.0.9 in both workspaces) had already landed independently on main via #610 and #623. Post-merge the PR collapses from 9 files / +338−133 to its true delta: 5 files, +326−121, test-only. The js-yaml ^4.3.1 override from #623 (GHSA-5p4m-2wfm-xmqj) is preserved — worth stating explicitly, since a merge resolved the wrong way here would have silently reverted a security override.

Verification on the merged base (fresh npm ci + full workspace build first — these suites import built dist/, so a stale tree proves nothing):

Check Result
Full middleware suite 6071 tests, 6067 pass, 0 fail, 4 skipped (44s)
tsc --noEmit clean
Core decoupling ratchet (#470) held at 3448, not raised
3 converted suites + fixture 36/36
Residual .listen( in converted files none

Note this is better than the "404 pre-existing failures" in the test plan above — those were build-dependent and clear once the workspaces are built.

Mutation-checked the new transport, because a test helper that isn't load-bearing makes the suites weaker while looking green:

  • forcing status: 50027 of 30 consumer tests fail
  • dropping req.write(init.body)17 fail, including the fixture's own body test

So the transport is genuinely threaded through to its consumers, not decorative.

I also probed one hypothesis and want to record that it was refuted, so nobody re-litigates it: http.globalAgent has keepAlive: true on Node 22, and every client here is nominally localhost:80, so I expected pooled sockets to bleed across separate createInProcessClient instances (app A answering a request meant for app B) plus one orphaned serverSide Duplex per call. Measured: createConnection fires 6/6, zero orphans, zero bleed. Not an issue.


Two findings — neither is a merge blocker

1. Closes #564 is broader than the change. Issue #564 is "harden the middleware test suite against parallelism". This converts 3 files; 74 others still call .listen(. The seam is the valuable part and the first slice is the right size — but as written the issue auto-closes on merge and the remaining ~96% loses its tracker. Suggest reframing to Refs #564 plus a follow-up for the rest, or explicitly scoping #564 down to the channelApi suites.

2. There are now two in-process HTTP test helpers. test/_helpers/httpInvoke.ts already exists with the same motivation (its docstring makes nearly the same argument about listen(0) and socket contention) and has 2 consumers; this adds test/support/inProcessHttp.ts with 3.

To be clear, I think the second one is justified — they are not redundant:

  • httpInvoke builds IncomingMessage/ServerResponse by hand and monkey-patches res.write/res.end. It cannot send a request body, so it can't serve the POST-heavy suites here.
  • inProcessHttp drives a real socket pair through Node's own client/server state machines, so bodies, header parsing and chunked framing are real.

That reasoning just isn't written down anywhere, and "second helper, no explanation, different directory convention" is the kind of thing that costs a review round-trip. Either fold the rationale into the PR body, or migrate the 2 httpInvoke consumers onto the new helper and delete the old one — still test-only, and it leaves one obvious path for the remaining 74 files.

Minor: middleware's lint script covers src/ and packages/ only, so nothing under test/ is linted in CI — the "eslint clean" line in the test plan must have been a manual run.

@Weegy
Weegy merged commit 71079ca into byte5ai:main Aug 7, 2026
7 checks passed
Weegy added a commit that referenced this pull request Aug 7, 2026
Resolves five conflicts against main (#550 MCP waves 0-6, #624, #613):

- mcpClient.ts: keep BOTH the new pool-lifetime members (entries map,
  idleTtlMs, MCP_POOL_IDLE_TTL_MS, mcpPoolScopeMatches) and main's
  structuredSink / pendingInput options and outputSchemas cache.
- src/index.ts: keep the runtimeMcpManager handle alongside main's
  structured-sink wiring and the W2-1 (#544) input replayer.
- routes/agentBuilder.ts: keep main's W0-1 ownership check on
  DELETE /mcp-servers/:id/token (404/403 fail-closed) and invalidate the
  pooled connection after the token row is deleted.
- docs/adr: main landed 0007-mcp-client-id-metadata-documents (2026-07-30)
  first, so this ADR is renumbered 0007 -> 0008 and every reference updated.
- CHANGELOG: both Unreleased entries retained.

Also fixes the mcpPool fixture: serverRow() predates #550 and omitted the
required `delegation` field, so resolveMcpUserKey failed closed and the
token-revocation test got 403 instead of 204.

Verified on the merge result: lint, typecheck and the core-decoupling
ratchet pass, and `npm test` is green 3/3 (6075 pass, 0 fail, ~35-49s) --
the parallelism failures documented in the PR body are resolved by #613,
now on main. Both new behaviours mutation-checked: disabling
onMcpServerChanged turns 3 tests red, weakening the '#' pool-key separator
turns 2 red.
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.

Harden the middleware test suite against parallelism

2 participants