fix(benchpack-host): close provider connections on request timeout to stop request accumulation - #29
Open
danieltvela wants to merge 1 commit into
Open
Conversation
…continues processing
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
Fixes a resource leak in
@benchlocal/benchpack-hostwhere a scenario request that hits the request timeout is not actually torn down. The host used to fire the abort signal and immediately move on to the next test, leaving the timed-out provider request running in the background. Because provider traffic flows through a shared undiciAgent, the provider process never sees the connection close and keeps generating — so across scenarios it accumulates in-flight requests that appear to loop forever.This PR closes that gap in two complementary layers:
runScenariopromise with a short, bounded grace period so the cooperativeAbortSignalhas a chance to unwind the request before the next test starts.Agentper provider origin, and on timeout the host force-closes that provider's connections (agent.destroy()) so the provider process cancels any still-running generation. The next request to the same provider transparently reopens a fresh connection ("close and reopen").The force-close is gated by a per-provider in-flight counter so it only happens when the timed-out scenario is the sole active request to that provider — concurrent requests in the parallel execution modes are never disrupted.
Problem
Timeout handling in
runScenarioSafelyThe scenario timeout in
packages/benchpack-host/src/index.tswas implemented withPromise.racebetween the real run and a timer:On timeout this:
timedOut = true,scenarioController,ScenarioResult.However,
Promise.racedoes not cancel or await the losing promise. The actualprepared.runScenario(...)keeps running in the background. Thefinallyblock only cleared the timer and removed the parent-abort listener; nothing awaitedrunPromise.The orchestration loops (
executeSerialByModelMode,executeSerialTestCasesMode, and the parallel variants) advance to the next scenario/model as soon asrunScenarioWithRepeatsresolves — there was no drain or wait for the previous request to settle.Why the provider connection stays open
AbortSignalpassed intoprepared.runScenario. Whether the underlying providerfetchactually aborts depends entirely on the Bench Pack honoring that signal; the host never forced the connection shut.Agentinstalled at module load (setGlobalDispatcher(new Agent({ bodyTimeout: 0, headersTimeout: 0 }))). TheglobalThis.fetchwrapper used for provider-error capture passedinitthrough unchanged — it did not manage or scope connections.fetchreceived no signal, and thepipelineto the client did not propagate a client disconnect back to the upstream — so even a well-behaved client abort could leave the provider generation running.Result: on timeout the provider kept generating/streaming to an open connection while the next test piled on top, matching the "accumulating infinite-loop requests" symptom.
Fix
Part A — drain the timed-out request
In
runScenarioSafely,runPromiseis hoisted to function scope. When the timeout fires, before returning the failure result the host now awaits it with a bounded grace period:drainRunPromiseraces the promise against a 2 s grace timer (TIMED_OUT_RUN_DRAIN_GRACE_MS), so a well-behaved Bench Pack that honors the abort signal settles quickly, and a misbehaving one cannot block the run forever.Part B — per-provider connection close and reopen
Per-provider undici Agents. The single anonymous global
Agentwas replaced by:Agent(still{ bodyTimeout: 0, headersTimeout: 0 }) for non-provider fetches, andMap<normalizedBaseUrl, Agent>(providerFetchAgents) that lazily creates a dedicatedAgentper provider origin.Routing provider fetches through the provider's Agent. The
globalThis.fetchwrapper (installProviderFetchCapture) now resolves the provider origin for each request URL and injects that provider'sAgentvia undici'sdispatcheroption before calling the native fetch. This covers both direct Bench Pack fetches and the inference relay's upstream fetches.Force-close on timeout.
closeProviderFetchAgent(baseUrl)looks up the provider'sAgent, removes it from the map, and callsagent.destroy(). This closes the TCP connections to that provider, so the provider process sees the connection close and cancels generation. Because the entry is removed from the map, the next request to the same provider lazily creates a freshAgent— the "close and reopen" behavior.Safety gate for parallel execution
A per-provider-origin in-flight counter (
providerActiveScenarioCounts) tracks how many scenarios are currently using each provider (beginProviderScenario/endProviderScenarioinrunScenarioSafely). The force-close only runs when the timed-out scenario is the sole active request to that provider:This means:
serial_by_model(one request at a time): force-close applies — this is where the accumulation was observed.parallel_by_test_case,parallel_by_model,full_parallel, or multiple models sharing the same provider inserial: force-close is skipped, so healthy concurrent requests to the same provider are never reset.Files changed
packages/benchpack-host/src/index.ts— all changes. No protocol, SDK, or public API changes.Testing
npm run typecheck(all workspaces) — passes.npm test(typecheck + UI contract check) — passes.Notes:
tscfailure in@benchlocal/benchpack-hostexists atHEADwhen the@benchlocal/core/@benchlocal/sdkdist/builds are missing (the workspace typecheck depends on those packages being built). It is unrelated to this change;npm run build:compilebuilds the packages first.request_timeout_seconds), run aserial_by_modelrun, and confirm the provider cancels the in-flight generation on timeout and no requests accumulate across scenarios; then confirm afull_parallelrun with concurrent requests is unaffected.