fix(storage): add S3 client timeouts so a dropped connection can't wedge uploads (#1049)

* 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 <peipeimo@Peifus-MacBook-Pro.local>

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';

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),
     });
   }
 

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 ||