Make hanging-request fake-server teardown deterministic and bounded - #17
Merged
Conversation
…nded A post-merge macOS/node 20 run spent the full 30-second Vitest budget inside "a timeout aborts the request deterministically" -- a test whose own contract is ~1.5 seconds -- while 1,649 of 1,650 tests passed. The test drives the fake Ollama server''s `timeout` behavior (accept the request, never respond) and then closes the server in a finally block, so the anonymous outer timeout could not even say whether the client abort or the fixture teardown was the side that hung. The fixture teardown was the suspect with an actual design gap: it relied on `closeAllConnections?.()` plus the server close callback, with nothing bounding the wait and nothing tracking the socket the aborted request leaves mid-unwind. Both fake servers now share one hardened lifecycle (`trackedServerLifecycle`): every accepted TCP socket is tracked from the `connection` event, `close()` stops the listener, destroys every live socket explicitly (idle keep-alive AND intentionally hanging requests), and bounds the whole wait -- on expiry it force-destroys stragglers and rejects with "<server> teardown timed out after N ms with K tracked socket(s) still open", so any future hang fails fast at the actual location instead of hiding behind the outer test budget. Nothing about the CLIENT path changed: the timeout fixture still leaves a real hanging HTTP request that only AbortSignal.timeout ends, production timeout/cancellation semantics are untouched, and no Vitest budget was raised anywhere. New regression tests (fake server lifecycle): teardown after a client-aborted hanging request completes well inside a 2 s bound with the timeout still classified; teardown WHILE a request is still in flight destroys it and settles; and ten abort-to-teardown cycles stay bounded. Stress: 20 consecutive runs of the whole Ollama file (about 20 timeout plus 200 cancellation abort/teardown cycles) with zero failures. Full suite: 1,651 tests passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A post-merge
mainrun on macOS/node 20 spent the full 30-second Vitest budget insideollama.test.ts > a timeout aborts the request deterministically— a test whose own contract is ~1.5 seconds — while 1,649 of 1,650 tests passed (failing job). The test drives the fake server''stimeoutbehavior (accept the request, never respond) and closes the server in afinally, so the anonymous outer timeout could not even distinguish a hung client abort from a hung fixture teardown.Inspection found the concrete design gap on the teardown side:
close()relied oncloseAllConnections?.()plus the server close callback, with no bound on the wait and no tracking of the socket a client abort leaves mid-unwind — a code path that could wait forever on an OS/runtime scheduling hiccup. Both fake HTTP servers (Ollama and OpenAI-compatible, which shared the identical pattern) now use one hardened lifecycle,trackedServerLifecycle: every accepted TCP socket is tracked from theconnectionevent;close()stops the listener, explicitly destroys every live socket (idle keep-alive and intentionally hanging requests), and bounds the whole wait — on expiry it force-destroys stragglers and rejects with<server> teardown timed out after N ms with K tracked socket(s) still open, so any future hang fails fast at the actual location with actionable diagnostics instead of hiding behind the outer test budget.Deliberately unchanged: the
timeoutfixture still leaves a real hanging HTTP request that only the client''sAbortSignal.timeoutends (the abort path stays genuinely tested); production timeout/cancellation semantics insafeHttpRequestand the Ollama runner are untouched; and no Vitest timeout budget was raised anywhere — the point is to remove the hang, not to hide it. Loopback-only traffic, no platform-specific behavior.New regression tests (
fake server lifecycledescribe block): teardown after a client-aborted hanging request completes well inside a 2s bound with the timeout still classifiedtimed-out/timed_out; teardown while a request is still in flight destroys it and settles; ten abort→teardown cycles stay bounded. The adjacent cancellation test (same hanging fixture) is covered by the same hardened helper.Validation
tests/runners/ollama.test.ts: 27/27 (24 existing + 3 new)openai-compatible,mcp-runner-tools,openai-execution,multi-runner,conformance,cli-v06-runner): 90/90pnpm lint/pnpm typecheck/pnpm build: passpnpm test: 116 files, 1,651 tests, exit 0Checklist
pnpm lint,pnpm typecheck, andpnpm testpass locallypnpm check:public-contractspasses — no contract touched (test helpers only)