From 3600231d5f059cb7fd7250430a26bf0b0396a87f Mon Sep 17 00:00:00 2001 From: peipeimo <150317697+peipeimo@users.noreply.github.com> Date: Sun, 16 Aug 2026 09:14:48 -0400 Subject: [PATCH] fix(storage): add S3 client timeouts so a dropped connection can't wedge uploads (#1049) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(storage): add S3 client timeouts so a dropped connection can't wedge uploads * fix(storage): use socketTimeout, not requestTimeout, for the dead-connection guard * fix(storage): make S3 timeouts generous — short connectionTimeout breaks pooled reads --------- Co-authored-by: Peifu Mo --- .../storage/__tests__/s3Storage.test.js | 71 +++++++++++++++++++ backend/src/services/storage/index.js | 5 ++ backend/src/services/storage/s3Storage.js | 33 ++++++++- 3 files changed, 107 insertions(+), 2 deletions(-) diff --git a/backend/src/services/storage/__tests__/s3Storage.test.js b/backend/src/services/storage/__tests__/s3Storage.test.js index 13d239d4..53ec11df 100644 --- a/backend/src/services/storage/__tests__/s3Storage.test.js +++ b/backend/src/services/storage/__tests__/s3Storage.test.js @@ -65,6 +65,53 @@ describe('S3StorageAdapter', () => { }) ); }); + + it('should configure connection and socket-inactivity timeouts by default', () => { + expect(S3Client).toHaveBeenCalledWith( + expect.objectContaining({ + requestHandler: { + connectionTimeout: 120000, + socketTimeout: 60000 + } + }) + ); + }); + + it('should keep connectionTimeout generous enough to survive socket-pool queuing', () => { + // connectionTimeout starts at request creation and only clears once a + // socket is assigned AND connected, so waiting for a free socket from + // the agent pool counts against it. A short value (e.g. 10s) fails + // every read under concurrent upload load. These timeouts bound an + // infinite hang; they are not latency targets. + const [[config]] = S3Client.mock.calls; + expect(config.requestHandler.connectionTimeout).toBeGreaterThanOrEqual(60000); + expect(config.requestHandler.socketTimeout).toBeGreaterThanOrEqual(30000); + }); + + it('should not set requestTimeout, which caps total duration and only warns', () => { + // requestTimeout would abort legitimate large uploads (it is a + // total-duration cap, not inactivity) and by default only logs a + // warning — it needs throwOnRequestTimeout to abort at all. + const [[config]] = S3Client.mock.calls; + expect(config.requestHandler).not.toHaveProperty('requestTimeout'); + }); + + it('should allow overriding timeouts via config', () => { + new S3StorageAdapter({ + bucket: 'test-bucket', + connectionTimeout: 5000, + socketTimeout: 30000 + }); + + expect(S3Client).toHaveBeenLastCalledWith( + expect.objectContaining({ + requestHandler: { + connectionTimeout: 5000, + socketTimeout: 30000 + } + }) + ); + }); }); describe('testConnection', () => { @@ -239,6 +286,30 @@ describe('S3StorageAdapter', () => { s3Storage.config.retryDelay = originalDelay; }); + it('should retry when the request handler times out a dead connection', async () => { + // @smithy/node-http-handler rejects with name 'TimeoutError' for both + // its connection-timeout and socket-inactivity timeouts + const timeoutError = new Error('Connection timed out after 10000ms'); + timeoutError.name = 'TimeoutError'; + + const operation = jest.fn() + .mockRejectedValueOnce(timeoutError) + .mockResolvedValueOnce('success'); + + const originalRandom = Math.random; + const originalDelay = s3Storage.config.retryDelay; + Math.random = jest.fn(() => 0); + s3Storage.config.retryDelay = 0; + + const result = await s3Storage._retryOperation(operation); + + expect(result).toBe('success'); + expect(operation).toHaveBeenCalledTimes(2); + + Math.random = originalRandom; + s3Storage.config.retryDelay = originalDelay; + }); + it('should not retry on non-retryable errors', async () => { const nonRetryableError = new Error('Invalid credentials'); nonRetryableError.code = 'InvalidCredentials'; diff --git a/backend/src/services/storage/index.js b/backend/src/services/storage/index.js index cb224484..2e097c2f 100644 --- a/backend/src/services/storage/index.js +++ b/backend/src/services/storage/index.js @@ -27,6 +27,9 @@ let instance = null; * STORAGE_S3_PREFIX — namespace prefix inside the bucket * STORAGE_S3_FORCE_PATH_STYLE=true|false (default: auto when endpoint set) * STORAGE_S3_SSL=true|false (default: true) + * STORAGE_S3_CONNECTION_TIMEOUT — ms to acquire+establish a socket (default 120000) + * STORAGE_S3_SOCKET_TIMEOUT — ms of socket inactivity before a request + * fails and is retried (default 60000) */ function buildStorage() { const backend = (process.env.STORAGE_BACKEND || 'local').toLowerCase(); @@ -48,6 +51,8 @@ function buildStorage() { prefix: process.env.STORAGE_S3_PREFIX, forcePathStyle: process.env.STORAGE_S3_FORCE_PATH_STYLE === 'true' ? true : undefined, sslEnabled: process.env.STORAGE_S3_SSL !== 'false', + connectionTimeout: parseInt(process.env.STORAGE_S3_CONNECTION_TIMEOUT || '120000', 10), + socketTimeout: parseInt(process.env.STORAGE_S3_SOCKET_TIMEOUT || '60000', 10), }); } diff --git a/backend/src/services/storage/s3Storage.js b/backend/src/services/storage/s3Storage.js index f7bd0a90..d25215ab 100644 --- a/backend/src/services/storage/s3Storage.js +++ b/backend/src/services/storage/s3Storage.js @@ -40,6 +40,8 @@ class S3StorageAdapter extends stream.EventEmitter { * @param {number} [config.partSize=10485760] - Part size for multipart upload (default 10MB) * @param {number} [config.maxRetries=3] - Maximum number of retry attempts * @param {number} [config.retryDelay=1000] - Initial retry delay in milliseconds + * @param {number} [config.connectionTimeout=120000] - Ms to acquire+establish a socket + * @param {number} [config.socketTimeout=60000] - Ms of socket inactivity before a request fails */ constructor(config) { super(); @@ -58,13 +60,38 @@ class S3StorageAdapter extends stream.EventEmitter { partSize: 10 * 1024 * 1024, // 10MB maxRetries: 3, retryDelay: 1000, + connectionTimeout: 120000, + socketTimeout: 60000, ...config }; // Initialize S3 client const s3Config = { region: this.config.region, - forcePathStyle: this.config.forcePathStyle + forcePathStyle: this.config.forcePathStyle, + // Without timeouts a silently dropped connection leaves the request — + // and with it every queued upload — hanging forever. + // + // socketTimeout, NOT requestTimeout, is the right knob here: + // requestTimeout is a total-duration cap that would kill legitimate + // large uploads, and by default it only logs a warning (it needs + // throwOnRequestTimeout to abort at all). socketTimeout fires on + // socket INACTIVITY and destroys the request with a TimeoutError, so + // an active transfer of any size is safe and only a dead line trips. + // + // Both values are deliberately GENEROUS. connectionTimeout starts + // when the request object is created and only clears once a socket + // is both assigned and connected — so time spent queuing for a free + // socket from the agent pool (maxSockets 50) counts against it. A + // 10s value looks reasonable and is not: under concurrent uploads + // it expires while merely waiting in line, and every read (photo + // download, thumbnail, background thumbnailing) fails with + // TimeoutError. These timeouts exist to convert an INFINITE hang + // into a bounded failure, not to enforce latency targets. + requestHandler: { + connectionTimeout: this.config.connectionTimeout, + socketTimeout: this.config.socketTimeout + } }; // Add credentials if provided @@ -671,7 +698,9 @@ class S3StorageAdapter extends stream.EventEmitter { } // Check if error is retryable - const retryableErrors = ['ECONNRESET', 'ETIMEDOUT', 'ENOTFOUND', 'ESOCKETTIMEDOUT', 'RequestTimeout', 'SlowDown', 'ServiceUnavailable', 'InternalError']; + // 'TimeoutError' is what @smithy/node-http-handler names both its + // connection-timeout and socket-inactivity rejections. + const retryableErrors = ['ECONNRESET', 'ETIMEDOUT', 'ENOTFOUND', 'ESOCKETTIMEDOUT', 'RequestTimeout', 'TimeoutError', 'SlowDown', 'ServiceUnavailable', 'InternalError']; const isRetryable = retryableErrors.some(code => error.code === code || error.name === code ||