From 9ec9bd1fffc4afba95599cdb13447f9bbaa55e55 Mon Sep 17 00:00:00 2001 From: Amir Date: Thu, 21 May 2026 12:27:38 +0200 Subject: [PATCH 1/2] feat: enable HTTP/2 and restore keep-alive in Node transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backs up the "h2 multiplexing replaces /batch" claim from #2 for Node consumers (browsers already negotiate h2 automatically). Without these, Node consumers — including the MCP — were stuck on HTTP/1.1 with near-disabled keep-alive, so every request opened a fresh TCP+TLS connection. Changes: - `EnvHttpProxyAgent({ allowH2: true })` so ALPN can pick h2 when the server offers it (Comms via CloudFront does; fallback to h1.1 is automatic when it doesn't). - Drop the 1ms keepAliveTimeout/keepAliveMaxTimeout hack; use undici defaults so the connection pool actually persists across requests. - Wrap the full dispatcher construction in `suppressExperimentalWarningsSync` so the `allowH2` experimental warning is swallowed alongside the decompress one. - Add `closeDefaultDispatcher()` (exported from the package root) and `CommsApi.close()` so short-lived CLIs / scripts — including ones that only use the OAuth helpers — can drain the pool before exit. Validated direction with expert-letter (Claude + Codex) and Doistbot. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 22 ++++++++++ src/comms-api.ts | 12 ++++++ src/index.ts | 1 + src/transport/http-dispatcher.test.ts | 28 ++++++++++++ src/transport/http-dispatcher.ts | 62 ++++++++++++++++----------- 5 files changed, 101 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index f58d72a..c46aaf0 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,28 @@ const api = new CommsApi(tokenResponse.accessToken) const user = await api.users.getSessionUser() ``` +### Short-lived processes (CLIs, scripts) + +On Node, the SDK keeps a connection pool alive across requests so HTTP/2 +multiplexing and TLS reuse actually work. Long-running processes don't +need to think about it. **Short-lived processes should `await api.close()` +before exit**, otherwise Node's event loop waits ~4 seconds for idle +sockets to time out: + +```typescript +const api = new CommsApi('YOUR_API_TOKEN') +try { + await api.users.getSessionUser() +} finally { + await api.close() +} +``` + +`api.close()` drains the process-global pool, so it also covers code +paths that only use the standalone OAuth helpers (`getAuthToken`, +`revokeAuthToken`, `registerClient`). Those flows can also import +`closeDefaultDispatcher` directly. + ## Development - `npm install` diff --git a/src/comms-api.ts b/src/comms-api.ts index aab4a26..602a132 100644 --- a/src/comms-api.ts +++ b/src/comms-api.ts @@ -10,6 +10,7 @@ import { ThreadsClient } from './clients/threads-client' import { UsersClient } from './clients/users-client' import { WorkspaceUsersClient } from './clients/workspace-users-client' import { WorkspacesClient } from './clients/workspaces-client' +import { closeDefaultDispatcher } from './transport/http-dispatcher' import type { CustomFetch } from './types/http' export type CommsApiOptions = { @@ -70,4 +71,15 @@ export class CommsApi { this.reactions = new ReactionsClient(clientConfig) this.search = new SearchClient(clientConfig) } + + /** + * Drains the SDK's process-global connection pool. CLIs and scripts + * should `await api.close()` before exit so Node's event loop empties + * immediately instead of waiting ~4s on keep-alive. Affects every + * `CommsApi` and OAuth helper in the same process — it's a + * process-shutdown gesture, not an instance teardown. Browser-safe. + */ + async close(): Promise { + await closeDefaultDispatcher() + } } diff --git a/src/index.ts b/src/index.ts index cee4de7..01cc407 100644 --- a/src/index.ts +++ b/src/index.ts @@ -13,5 +13,6 @@ export { WorkspaceUsersClient } from './clients/workspace-users-client' export { WorkspacesClient } from './clients/workspaces-client' export { CommsApi } from './comms-api' export type { CommsApiOptions } from './comms-api' +export { closeDefaultDispatcher } from './transport/http-dispatcher' export * from './types/index' export * from './utils/index' diff --git a/src/transport/http-dispatcher.test.ts b/src/transport/http-dispatcher.test.ts index 1522c19..0979eaa 100644 --- a/src/transport/http-dispatcher.test.ts +++ b/src/transport/http-dispatcher.test.ts @@ -2,6 +2,7 @@ import { createServer, type Server } from 'node:http' import type { AddressInfo } from 'node:net' import { gzipSync } from 'node:zlib' import { + closeDefaultDispatcher, getDefaultDispatcher, resetDefaultDispatcherForTests, suppressExperimentalWarningsSync, @@ -138,6 +139,33 @@ describe('httpDispatcher', () => { await new Promise((resolve) => httpServer.close(() => resolve())) } }) + + it('close() invokes the dispatcher close and lets a new one be created', async () => { + const closeCalls: number[] = [] + + vi.doMock('undici', () => ({ + EnvHttpProxyAgent: class { + compose() { + return this + } + async close() { + closeCalls.push(1) + } + }, + interceptors: { + decompress: () => (dispatch: unknown) => dispatch, + }, + })) + + const first = await getDefaultDispatcher() + expect(first).toBeDefined() + + await closeDefaultDispatcher() + expect(closeCalls).toHaveLength(1) + + const second = await getDefaultDispatcher() + expect(second).not.toBe(first) + }) }) describe('suppressExperimentalWarningsSync', () => { diff --git a/src/transport/http-dispatcher.ts b/src/transport/http-dispatcher.ts index 9aa074e..37da387 100644 --- a/src/transport/http-dispatcher.ts +++ b/src/transport/http-dispatcher.ts @@ -1,12 +1,5 @@ import type { Dispatcher } from 'undici' -// Use effectively-disabled keep-alive so short-lived CLI processes do not stay -// open waiting on idle sockets. Undici requires positive values, so we use 1ms. -const keepAliveOptions = { - keepAliveTimeout: 1, - keepAliveMaxTimeout: 1, -} - let defaultDispatcher: Dispatcher | undefined let defaultDispatcherPromise: Promise | undefined @@ -31,6 +24,27 @@ export async function getDefaultDispatcher(): Promise { return defaultDispatcherPromise } +/** + * Drains the default dispatcher's connection pool. CLIs and scripts should + * `await` this before exit so Node's event loop empties immediately instead + * of waiting ~4s on keep-alive. No-op in the browser branch. + */ +export async function closeDefaultDispatcher(): Promise { + if (defaultDispatcherPromise) { + try { + await defaultDispatcherPromise + } catch { + // init already failed; nothing to close + } + } + const dispatcher = defaultDispatcher + defaultDispatcher = undefined + defaultDispatcherPromise = undefined + if (dispatcher) { + await dispatcher.close() + } +} + export function resetDefaultDispatcherForTests(): void { defaultDispatcher = undefined defaultDispatcherPromise = undefined @@ -50,21 +64,26 @@ async function createDefaultDispatcher(): Promise { // branch, so undici is safe to load when we get here. const { EnvHttpProxyAgent, interceptors } = await import('undici') - // Compose the response-decompression interceptor so gzip/deflate/br/zstd - // bodies are decoded before consumers parse them. Required on Node 24+: - // attaching any custom dispatcher to the global `fetch` strips the - // `content-encoding` header but does not actually decompress the body, - // so callers receive raw gzipped bytes and `JSON.parse` fails. + // `allowH2: true` opts into HTTP/2 via ALPN; undici falls back to h1.1 + // when the server doesn't negotiate h2. Without this flag undici + // defaults to h1.1 even when the server supports h2. + // + // `interceptors.decompress()` decodes gzip/deflate/br/zstd bodies. On + // Node 24+, attaching any custom dispatcher to global `fetch` strips + // `content-encoding` without actually decompressing the body. // See https://github.com/Doist/todoist-cli/issues/318. - const decompress = suppressExperimentalWarningsSync(() => interceptors.decompress()) - - return new EnvHttpProxyAgent(keepAliveOptions).compose(decompress) + // + // Both emit ExperimentalWarning on first use; suppressed during init. + return suppressExperimentalWarningsSync(() => { + const decompress = interceptors.decompress() + return new EnvHttpProxyAgent({ allowH2: true }).compose(decompress) + }) } -// undici emits an `ExperimentalWarning` the first time `interceptors.decompress()` -// runs. The interceptor is stable for our gzipped-JSON-over-HTTPS use case; -// silence the warning during dispatcher init only so it does not leak to every -// consumer's stderr on the first request. +// `suppressExperimentalWarningsSync` is exported for direct unit testing — +// the integration path through `getDefaultDispatcher()` can't reliably +// exercise it because both the dispatcher singleton and undici's internal +// `warningEmitted` flag are once-per-process. // // `fn` must be synchronous so the override covers a single critical section // (microseconds) — no unrelated `ExperimentalWarning` from elsewhere can @@ -72,11 +91,6 @@ async function createDefaultDispatcher(): Promise { // pattern-matching the message text: the message wording is an undici // implementation detail (not a stable API), and the suppression window is // narrow enough that a coarse type filter is safe. -// -// Exported for direct unit testing — the integration path through -// `getDefaultDispatcher()` cannot reliably exercise the helper because both -// the dispatcher singleton and undici's internal `warningEmitted` flag are -// once-per-process. export function suppressExperimentalWarningsSync(fn: () => T): T { const originalEmit = process.emitWarning process.emitWarning = (( From f45b71137df96a09b044c6669d89705082e6448e Mon Sep 17 00:00:00 2001 From: Amir Date: Thu, 21 May 2026 12:33:53 +0200 Subject: [PATCH 2/2] fix: close race in closeDefaultDispatcher Clear the singleton before awaiting init. Previously, `await defaultDispatcherPromise` yielded to the event loop with the singleton still set; a concurrent `getDefaultDispatcher()` could grab the old dispatcher reference and then have it torn down underneath it. Per Doistbot review on #3. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/transport/http-dispatcher.ts | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/transport/http-dispatcher.ts b/src/transport/http-dispatcher.ts index 37da387..f4066a0 100644 --- a/src/transport/http-dispatcher.ts +++ b/src/transport/http-dispatcher.ts @@ -30,18 +30,22 @@ export async function getDefaultDispatcher(): Promise { * of waiting ~4s on keep-alive. No-op in the browser branch. */ export async function closeDefaultDispatcher(): Promise { - if (defaultDispatcherPromise) { - try { - await defaultDispatcherPromise - } catch { - // init already failed; nothing to close - } - } - const dispatcher = defaultDispatcher + // Clear the singleton *before* awaiting init, so any concurrent + // `getDefaultDispatcher()` after this point creates a fresh dispatcher + // instead of receiving a reference to the one we're about to close. + const initPromise = defaultDispatcherPromise defaultDispatcher = undefined defaultDispatcherPromise = undefined - if (dispatcher) { - await dispatcher.close() + + if (!initPromise) return + + try { + const dispatcher = await initPromise + if (dispatcher) { + await dispatcher.close() + } + } catch { + // init failed; nothing to close } }