Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions src/_shims/node-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { type RequestOptions } from '../core';
import { MultipartBody } from './MultipartBody';
import { type Shims } from './registry';
import { ReadableStream } from 'node:stream/web';
import { createUndiciFetch } from '../lib/undici-fetch';
import { createH2Fetch } from '../lib/h2-transport';

type FileFromPathOptions = Omit<FilePropertyBag, 'lastModified'>;

Expand All @@ -39,8 +39,20 @@ async function fileFromPath(path: string, ...args: any[]): Promise<File> {
return await _fileFromPath(path, ...args);
}

const defaultHttpAgent: Agent = new KeepAliveAgent({ keepAlive: true, timeout: 10 * 60 * 1000 });
const defaultHttpsAgent: Agent = new KeepAliveAgent.HttpsAgent({ keepAlive: true, timeout: 10 * 60 * 1000 });
const defaultHttpAgent: Agent = new KeepAliveAgent({
keepAlive: true,
timeout: 10 * 60 * 1000,
maxSockets: Infinity,
maxFreeSockets: 2048,
freeSocketTimeout: 30_000,
});
const defaultHttpsAgent: Agent = new KeepAliveAgent.HttpsAgent({
keepAlive: true,
timeout: 10 * 60 * 1000,
maxSockets: Infinity,
maxFreeSockets: 2048,
freeSocketTimeout: 30_000,
});
Comment on lines +42 to +55

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Setting maxSockets to Infinity creates an unbounded connection pool for the default Node agents, which can exhaust file descriptors and memory under high concurrency. Keep the pool bounded (or make the limit configurable) so request spikes cannot open unlimited sockets. [performance]

Severity Level: Major ⚠️
- ❌ Node HTTP/1.1 default client may exhaust sockets.
- ⚠️ Long-running Node processes risk file descriptor exhaustion.
Steps of Reproduction ✅
1. In a Node.js environment, import the Node shims (see `src/_shims/index.d.ts:22-25`
which exposes the Node runtime, and `src/_shims/node-runtime.ts:73-94` which registers the
Node shims via `getRuntime()`).

2. Instantiate the SDK client without providing a custom `httpAgent`, e.g. `new Runloop({
bearerToken: '...' })` as implemented in `src/index.ts:90-123`, where the constructor
passes `httpAgent: options.httpAgent` (possibly `undefined`) into `Core.APIClient`.

3. When any API method is called (e.g. `runloop.accounts.list()` defined via the resource
fields at `src/index.ts:167-179`), `Core.APIClient.buildRequest()` in `src/core.ts:35-39`
computes `const httpAgent = options.httpAgent ?? this.httpAgent ?? getDefaultAgent(url);`,
so with no user-specified agent it falls back to `getDefaultAgent(url)`.

4. For Node, `getDefaultAgent(url)` is resolved from the registry
(`src/_shims/registry.ts:28,47,74`) and implemented in `src/_shims/node-runtime.ts:91` as
`url.startsWith('https') ? defaultHttpsAgent : defaultHttpAgent`, where both
`defaultHttpAgent` and `defaultHttpsAgent` are created with `maxSockets: Infinity` at
`src/_shims/node-runtime.ts:42-55`.

5. Under a high-concurrency workload (for example, a load test that fires thousands of
concurrent requests using the default client to the same origin), each request path goes
through this default agent; because `maxSockets` is `Infinity`, the agent can open
unbounded concurrent sockets until the process or OS hits file descriptor limits, leading
to errors such as `EMFILE: too many open files` or memory pressure rather than
backpressure at the agent layer.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** src/_shims/node-runtime.ts
**Line:** 42:55
**Comment:**
	*Performance: Setting `maxSockets` to `Infinity` creates an unbounded connection pool for the default Node agents, which can exhaust file descriptors and memory under high concurrency. Keep the pool bounded (or make the limit configurable) so request spikes cannot open unlimited sockets.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


async function getMultipartRequestOptions<T = Record<string, unknown>>(
form: fd.FormData,
Expand All @@ -67,7 +79,7 @@ export function getRuntime(): Shims {
return {
kind: 'node',
fetch: nf.default,
makeHttp2Fetch: createUndiciFetch,
makeHttp2Fetch: createH2Fetch,
Request: nf.Request,
Response: nf.Response,
Headers: nf.Headers,
Expand Down
46 changes: 13 additions & 33 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,36 +292,18 @@ export interface ClientOptions {
fetch?: Core.Fetch | undefined;

/**
* Send requests over HTTP/2 (with automatic fallback to HTTP/1.1).
* Send requests over HTTP/2 using native `node:http2` connection pools.
*
* In Node.js this swaps the default `node-fetch` transport for an undici-backed
* adapter (`Agent({ allowH2: true })`) that negotiates HTTP/2 via ALPN. On the
* web the platform `fetch` already speaks HTTP/2, so this is a no-op there.
* Ignored when a custom `fetch` is provided.
* - `true` uses the SDK's default bounded HTTP/2 pool.
* - Pass `H2FetchOptions` to tune pool size, timeouts, etc.
*
* - `true` uses the SDK's default bounded HTTP/2 pool (a few TLS sessions, many
* multiplexed streams each).
* - Pass a configured undici `Dispatcher` (e.g. `new Agent({ allowH2: true,
* connections, pipelining })`) to control the pool yourself — the SDK uses it
* verbatim and does not manage its lifecycle, exactly like `httpAgent`.
*
* **Intended for HTTP/2-capable origins (such as the Runloop API).** When the
* origin does not negotiate h2, undici falls back to HTTP/1.1 with request
* pipelining enabled on the shared dispatcher; pipelining is unsafe against
* many HTTP/1.1 servers and proxies. Do not enable this flag if your traffic
* may be routed through a non-h2 intermediary.
*
* On the HTTP/2 path the `httpAgent` option is not used, since undici manages
* connections through its own dispatcher rather than a Node `http.Agent` — to
* tune connections here, pass a `Dispatcher` as shown above. A one-time warning
* is emitted if both `http2` and `httpAgent` are set.
* On the HTTP/2 path the `httpAgent` option is not used — the H2 transport
* manages its own persistent connections. A one-time warning is emitted if
* both `http2` and `httpAgent` are set.
*
* @default false
*/
// The `import('undici').Dispatcher` type is inlined (rather than a top-of-file
// import) to keep this manual addition to a generated file regen-friendly and to
// avoid pulling undici types onto the web/deno code paths; it is type-only/erased.
http2?: boolean | import('undici').Dispatcher | undefined;
http2?: boolean | import('./lib/h2-transport').H2FetchOptions | undefined;

/**
* The maximum number of times that the client will retry a request in case of a
Expand Down Expand Up @@ -377,7 +359,7 @@ export class Runloop extends Core.APIClient {
* @param {number} [opts.timeout=30 seconds] - The maximum amount of time (in milliseconds) the client will wait for a response before timing out.
* @param {number} [opts.httpAgent] - An HTTP agent used to manage HTTP(s) connections.
* @param {Core.Fetch} [opts.fetch] - Specify a custom `fetch` function implementation.
* @param {boolean | import('undici').Dispatcher} [opts.http2=false] - Send requests over HTTP/2 (Node only; ignored when `fetch` is provided). `true` uses the default bounded pool; pass an undici `Dispatcher` to control the pool yourself.
* @param {boolean | H2FetchOptions} [opts.http2=false] - Send requests over HTTP/2 (Node only; ignored when `fetch` is provided). `true` uses the default bounded pool; pass H2FetchOptions to tune.
* @param {number} [opts.maxRetries=5] - The maximum number of times the client will retry a request.
* @param {Core.Headers} opts.defaultHeaders - Default headers to include with every request to the API.
* @param {Core.DefaultQuery} opts.defaultQuery - Default query parameters to include with every request to the API.
Expand All @@ -399,16 +381,14 @@ export class Runloop extends Core.APIClient {
baseURL: baseURL || `https://api.runloop.ai`,
};

// `httpAgent` (a Node `http.Agent`) does not apply to the HTTP/2 transport —
// undici manages its own dispatcher and has no `http.Agent` concept. Warn once
// instead of silently ignoring it. (Skipped when a custom `fetch` supersedes
// `http2` entirely.)
// `httpAgent` does not apply to the HTTP/2 transport — it manages its own
// persistent connections. Warn once instead of silently ignoring.
if (!options.fetch && options.http2 && options.httpAgent && !http2HttpAgentWarned) {
http2HttpAgentWarned = true;
console.warn(
'[runloop] `httpAgent` is ignored when `http2` is set: undici manages its own ' +
'dispatcher and has no Node http.Agent concept. To configure the HTTP/2 transport, ' +
'pass a configured undici Dispatcher as `http2` (e.g. `http2: new Agent({ connections, pipelining })`).',
'[runloop] `httpAgent` is ignored when `http2` is set: the HTTP/2 transport manages ' +
'its own connections. To tune the H2 pool, pass options as `http2` ' +
'(e.g. `http2: { maxConnections: 20 }`).',
);
}

Expand Down
27 changes: 6 additions & 21 deletions tests/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Runloop } from '@runloop/api-client';
import { APIUserAbortError } from '@runloop/api-client';
import { Headers } from '@runloop/api-client/core';
import defaultFetch, { Response, type RequestInit, type RequestInfo } from 'node-fetch';
import { MockAgent } from 'undici';
// MockAgent import removed: h2-transport replaces undici

describe('instantiate client', () => {
const env = process.env;
Expand Down Expand Up @@ -120,31 +120,16 @@ describe('instantiate client', () => {
expect(customFetch).toHaveBeenCalledTimes(1);
});

test('http2 passthrough routes requests through a user-supplied undici Dispatcher', async () => {
// Passing an undici Dispatcher as `http2` must thread it all the way to
// undici.fetch's `dispatcher` (client -> _shims/makeHttp2Fetch -> createUndiciFetch).
// A MockAgent is a real Dispatcher, so if the request is served by our intercept,
// the SDK provably used the dispatcher we passed (net connect is disabled).
const mockAgent = new MockAgent();
mockAgent.disableNetConnect();
mockAgent
.get('http://localhost:5000')
.intercept({ path: /^\/foo/, method: 'GET' })
.reply(200, { mocked: true }, { headers: { 'content-type': 'application/json' } });

test('http2 option accepts H2FetchOptions for pool tuning', () => {
// Passing H2FetchOptions as `http2` configures the native H2 connection pool.
const client = new Runloop({
baseURL: 'http://localhost:5000/',
bearerToken: 'My Bearer Token',
maxRetries: 0,
http2: mockAgent,
http2: { maxConnections: 10, minConnections: 2 },
});

try {
const response = await client.get('/foo');
expect(response).toEqual({ mocked: true });
} finally {
await mockAgent.close();
}
// If construction succeeds without error, the options were accepted.
expect(client).toBeDefined();
});

test('warns once when http2 and httpAgent are combined', () => {
Expand Down