feat: use @smithy/undici-http-handler for S3 client#1139
Open
mcollina wants to merge 1 commit into
Open
Conversation
mcollina
force-pushed
the
mcollina/undici-http-handler
branch
from
0f9cf01 to
4f7c37c
Compare
Coverage Report for CI Build 27188171202Coverage decreased (-0.002%) to 76.43%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
mcollina
force-pushed
the
mcollina/undici-http-handler
branch
from
4f7c37c to
22defe5
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates the S3 HTTP stack from @smithy/node-http-handler + agentkeepalive to Undici (@smithy/undici-http-handler + undici Agent), adds request/connect timeouts to the shared agent, and updates lifecycle handling accordingly.
Changes:
- Replace Smithy
NodeHttpHandlerwithUndiciHttpHandleracross S3 and TUS code paths. - Introduce Undici-based
createAgentwith monitoring updates and agent shutdown now returning aPromise. - Add configurable S3 client request timeout wiring via config (
storageS3ClientTimeout).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/storage/renderer/renderer.ts | Updates abort-handling comment to reflect Undici-based Smithy handler. |
| src/storage/events/base-event.ts | Passes connect/request timeouts into the HTTP agent used by background S3 worker flows. |
| src/storage/backend/s3/adapter.ts | Switches S3 backend to UndiciHttpHandler, updates monitoring, and makes close() return a promise. |
| src/storage/backend/adapter.ts | Broadens close() to allow async cleanup (Promise<void>). |
| src/internal/http/agent.ts | Replaces agentkeepalive with Undici Agent, and rewrites metrics collection/close semantics. |
| src/http/routes/tus/index.ts | Migrates TUS S3 usage to UndiciHttpHandler and awaits agent close on shutdown. |
| src/http/plugins/storage.ts | Removes the Fastify onClose hook that previously closed the storage backend. |
| package.json | Removes @smithy/node-http-handler and agentkeepalive, adds @smithy/undici-http-handler and @smithy/types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+50
to
+53
| let errorCount = 0 | ||
| dispatcher.on('connectionError', () => { | ||
| errorCount++ | ||
| }) |
Contributor
Author
There was a problem hiding this comment.
Not a regression — agentkeepalive's errorSocketCount / timeoutSocketCount / createSocketErrorCount were also cumulative counters that only ever increased. Same semantics as before. Switching to a true counter metric is a separate cleanup unrelated to this migration.
Comment on lines
+76
to
+77
| function updateHttpAgentMetrics(name: string, stats: AgentStats) { | ||
| const baseAttrs = { name, protocol: 'https' } |
Comment on lines
+82
to
89
| httpPoolErrors.record(stats.errorSocketCount, { ...baseAttrs, type: 'connect_error' }) | ||
| } | ||
|
|
||
| export function watchAgent(name: string, protocol: 'http' | 'https', agent: Agent | HttpsAgent) { | ||
| export function watchAgent(name: string, dispatcher: Agent, getErrorCount: () => number) { | ||
| return setInterval(() => { | ||
| const httpStatus = agent.getCurrentStatus() | ||
|
|
||
| const httpStats = gatherHttpAgentStats(httpStatus) | ||
|
|
||
| updateHttpAgentMetrics(name, protocol, httpStats) | ||
| const stats = gatherDispatcherStats(dispatcher, getErrorCount()) | ||
| updateHttpAgentMetrics(name, stats) | ||
| }, 5000) |
Comment on lines
38
to
40
| request.storage = new Storage(storageBackend, database, location) | ||
| request.cdnCache = new CdnCacheManager(request.storage) | ||
| }) |
Comment on lines
+97
to
103
| for (const origin of Object.keys(dispatcher.stats)) { | ||
| const s = dispatcher.stats[origin] as { | ||
| running: number | ||
| free?: number | ||
| pending: number | ||
| queued?: number | ||
| } |
Comment on lines
97
to
101
| const httpAgent = createAgent('s3_worker', { | ||
| maxSockets: storageS3MaxSockets, | ||
| connectTimeoutMs: 5000, | ||
| requestTimeoutMs: storageS3ClientTimeout, | ||
| }) |
mcollina
force-pushed
the
mcollina/undici-http-handler
branch
from
22defe5 to
d408f29
Compare
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.
Switches the S3 client request handler from
@smithy/node-http-handler(agentkeepalive) to@smithy/undici-http-handler(undici Dispatcher). Pool metrics rewired onto undici stats.