feat(js-sdk)!: drop undici 7 and the in-flight cap, depend on undici@^8 only - #1733
feat(js-sdk)!: drop undici 7 and the in-flight cap, depend on undici@^8 only#1733devin-ai-integration[bot] wants to merge 3 commits into
Conversation
undici 8 honors the server's SETTINGS_MAX_CONCURRENT_STREAMS per h2 connection and spills to additional connections when saturated, so the dual undici/undici8 selection is no longer needed. Requires Node.js >= 22.19.0 (undici 8's engine floor); older Node falls back to the capped global fetch instead of loading undici. Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: 245f93b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Package ArtifactsBuilt from 1a47afc. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.42.1-devin-1787234613-drop-undici7.0.tgzCLI ( npm install ./e2b-cli-2.16.4-devin-1787234613-drop-undici7.0.tgzPython SDK ( pip install ./e2b-2.42.0+devin.1787234613.drop.undici7-py3-none-any.whl |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
TASTE.md review — 0 public-surface violations, 3 correctness/consistency notes
Checked the rules that could bite a dependency/runtime-gating change: parity (T-1, T-2), option and naming shape (T-3, T-9, T-10, T-12, T-22, T-23), undefined over null (T-20), version gating via named constants (T-55), version never hardcoded (T-56), flat entry point (T-54), error actionability (T-62, T-63), and comments/docs (T-69, T-70). The PR changes no public API surface, keeps the Node floor in a named constant (UNDICI_MIN_NODE, T-55-style), and the changeset correctly marks the engines bump as major. No TASTE violation to anchor inline.
The notes below are correctness/consistency, not TASTE. Two of them are on lines the diff doesn't touch:
- Stale comment in
packages/js-sdk/src/template/buildApi.ts(~line 183–184) — it still says "loadUndici tries undici 8 first and falls back to undici 7 where 8 doesn't import (Bun)", which this PR makes false. - Leftover pnpm override in root
package.json(line 60) —"undici@>=7.0.0 <7.29.0": "^7.29.0"pins a major that is no longer a direct dependency. Harmless if a transitive dep still pulls undici 7, dead config otherwise; worth deciding explicitly rather than leaving behind. - Bun: see the inline comment on
getUndiciPackageCandidates.
| if (compareVersions(nodeVersion, UNDICI_MIN_NODE) >= 0) { | ||
| return ['undici'] | ||
| } | ||
|
|
||
| return ['undici'] | ||
| return [] |
There was a problem hiding this comment.
Not a TASTE rule, a behavior regression the diff makes easy to miss. buildApi.ts documents that undici 8 doesn't import on Bun, which is exactly why undici (7) was the trailing candidate. With undici 7 gone and the gate returning [] below 22.19.0, loadUndici() resolves to undefined on any runtime whose process.versions.node is below the gate (Bun's emulated version) — so uploadFile loses the undici fetch it prefers for honoring an explicit Content-Length on stream bodies. Worth confirming against test:bun before merging, and either way the changeset should mention Bun, since it only mentions Node.
Also, now that the list can hold at most one entry, the plural ...Candidates name and the loop in loadUndici are vestigial — a supportsUndici(nodeVersion): boolean (or returning 'undici' | undefined) says what is actually being decided.
| return await dynamicImport<UndiciModule>(packageName) | ||
| } catch { | ||
| // Try the next package supported by this Node version. | ||
| // Fall back to the capped global fetch when undici cannot load. |
There was a problem hiding this comment.
T-69 (comments/docs are part of the surface and should state the failure mode where it happens): this catch doesn't fall back to anything — it continues the loop, and the capped-global-fetch fallback lives in buildDispatchedFetch. Describing the caller's behavior here misplaces it; the local truth is just that the import failed and undefined is returned.
| // Fall back to the capped global fetch when undici cannot load. | |
| // Undici is unavailable in this runtime; callers fall back themselves. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c19158a812
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, | ||
| "engines": { | ||
| "node": ">=20.18.1 <21 || >=22" | ||
| "node": ">=22.19.0" |
There was a problem hiding this comment.
When the changesets release process updates @e2b/cli's workspace:^ dependency to this new SDK major, the CLI will still advertise Node >=20.18.1 <21 || >=22.9.0, while this dependency rejects every version below 22.19.0. Consequently, installing the current CLI on its documented Node 20 or Node 22.9–22.18 range fails for users with strict engine enforcement (including this repository's pnpm configuration), before the intended global-fetch fallback can run. Update the CLI engine range and changeset/tests as part of this breaking release, or retain a compatible SDK engine range for the fallback path.
Useful? React with 👍 / 👎.
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
Ran offline end-to-end tests of the undici 8-only dependency change (no live E2B key; local servers driven through the SDK's real Key result — undici 8 vs the h2 stream-limit failure mode: Version selection & dispatch path: Global-fetch fallback (old Node simulated) + unit testsUnit tests: Not covered: live E2B API traffic (offline-only test environment). Tested by Devin — session link |
|
@devin-ai-integration we should also remove all the inflight machinery |
Concurrency is governed by undici 8's dispatcher: per-connection HTTP/2 stream limits advertised by the server plus the configured connection pools. Removes limitConcurrency and the E2B_API_INFLIGHT_REQUESTS, E2B_ENVD_INFLIGHT_REQUESTS, and E2B_ENVD_RPC_INFLIGHT_REQUESTS env vars. Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
devin-ai-integration
Bot
changed the title
|
Done in 245f93b: removed Note this supersedes #1717 — with no cap there's no slot accounting to fix, so #1717 can be closed once this lands. One trade-off to be aware of: the global-fetch fallback paths (Node < 22.19, Bun/Deno/workerd/browsers) now have no client-side concurrency bound at all. |
1 participant
Summary
Follow-up to the discussion on #1717 (#1717 (comment)): drop the dual
undici/undici8selection introduced in #1575, depend onundici@^8only, and remove the SDK-level in-flight request cap entirely (per #1733 (comment)).Why: undici 8 gates h2 dispatch on the server's
SETTINGS_MAX_CONCURRENT_STREAMS(itsbusy()returns true whenrunning >= maxConcurrentStreams) and the pool spills to additional connections when one saturates, then queues — it never throwsERR_HTTP2_TOO_MANY_CONCURRENT_STREAMS. That makes both undici 7 and the SDK's own semaphore redundant. With Node 20 EOL (April 2026), the only reason to keep undici 7 was Node 22.0–22.18, which just needs a patch update.export function getUndiciPackageCandidates(nodeVersion: string): string[] { - if (compareVersions(nodeVersion, '22.19.0') >= 0) return ['undici8', 'undici'] - return ['undici'] + if (compareVersions(nodeVersion, '22.19.0') >= 0) return ['undici'] + return [] }package.json:undici: ^8.10.0as a regular dependency,undici8alias removed,engines.nodenarrowed from>=20.18.1 <21 || >=22to>=22.19.0(undici 8's engine floor) — hence the major changeset.src/api/inflight.ts(limitConcurrency) is deleted,buildDispatchedFetchno longer takesinflightLimit, and theE2B_API_INFLIGHT_REQUESTS/E2B_ENVD_INFLIGHT_REQUESTS/E2B_ENVD_RPC_INFLIGHT_REQUESTSenv vars are gone. Concurrency is governed by undici's dispatcher (server-advertised per-connection h2 stream limits + theE2B_API_CONNECTIONS/E2B_ENVD_RPC_CONNECTIONSpools).buildDispatchedFetch, so e.g. the CLI on old Node degrades gracefully instead of crashing..tool-versions+codegen.Dockerfile:nodejs 22.18.0 → 22.19.0so CI exercises the undici path (22.18 would silently take the global-fetch fallback).Usage is unchanged; no user-facing API changes beyond the Node requirement and the removed env vars:
This supersedes #1717: with the cap gone there is no slot accounting to fix.
Verified:
pnpm run format/lint/typecheck, js-sdk build, and the undici/api/envd http2 unit tests (including the real-undici MockAgent tests now running against undici 8) all pass locally on Node 24. Offline e2e evidence in the comment below.Link to Devin session: https://app.devin.ai/sessions/66bc0b6c5e204bb98db02aacffea68fe
Requested by: @mishushakov