refactor(storage): make onResponse async and improve error formatting for retry failures

Author: thiyaguk09

handwritten/storage/src/resumable-upload.ts

@@ -1345,7 +1345,7 @@ export class Upload extends Writable {
       },
     };
     const res = await this.authClient.request(combinedReqOpts);
-    const successfulRequest = this.onResponse(res);
+    const successfulRequest = await this.onResponse(res);
     this.removeListener('error', errorCallback);
 
     return successfulRequest ? res : null;
@@ -1354,7 +1354,7 @@ export class Upload extends Writable {
   /**
    * @return {bool} is the request good?
    */
-  private onResponse(resp: GaxiosResponse) {
+  private async onResponse(resp: GaxiosResponse) {
     if (
       resp.status !== 200 &&
       this.retryOptions.retryableErrorFn!({
@@ -1363,7 +1363,7 @@ export class Upload extends Writable {
         name: resp.statusText,
       })
     ) {
-      this.attemptDelayedRetry(resp);
+      await this.attemptDelayedRetry(resp);
       return false;
     }
 
@@ -1386,9 +1386,7 @@ export class Upload extends Writable {
 
         if (retryDelay <= 0) {
           this.destroy(
-            new Error(
-              `Retry total time limit exceeded - ${JSON.stringify(resp.data)}`,
-            ),
+            formatRetryError('Retry total time limit exceeded', resp),
           );
           return;
         }
@@ -1409,9 +1407,7 @@ export class Upload extends Writable {
       }
       this.numRetries++;
     } else {
-      this.destroy(
-        new Error(`Retry limit exceeded - ${JSON.stringify(resp.data)}`),
-      );
+      this.destroy(formatRetryError('Retry limit exceeded', resp));
     }
   }
 
@@ -1456,6 +1452,99 @@ export class Upload extends Writable {
   }
 }
 
+function formatRetryError(
+  prefix: string,
+  resp: Pick<GaxiosResponse, 'data' | 'status'>,
+): Error {
+  const parts: string[] = [];
+
+  if (resp.status !== undefined && !isNaN(resp.status)) {
+    parts.push(`status: ${resp.status}`);
+  }
+
+  const err = resp.data;
+  if (err !== undefined && err !== null) {
+    if (err instanceof Error) {
+      const gaxiosErr = err as GaxiosError;
+      const errParts: string[] = [];
+      if (gaxiosErr.message) {
+        errParts.push(gaxiosErr.message);
+      }
+      const status = gaxiosErr.status ?? gaxiosErr.response?.status;
+      if (status !== undefined && !isNaN(status) && status !== resp.status) {
+        errParts.push(`status: ${status}`);
+      }
+      const statusText = gaxiosErr.response?.statusText;
+      if (statusText) {
+        errParts.push(`statusText: ${statusText}`);
+      }
+      const responseData = gaxiosErr.response?.data;
+      if (responseData !== undefined && responseData !== null && responseData !== '') {
+        errParts.push(
+          `response: ${
+            typeof responseData === 'object'
+              ? JSON.stringify(responseData)
+              : responseData
+          }`,
+        );
+      }
+      if (gaxiosErr.code) {
+        errParts.push(`code: ${gaxiosErr.code}`);
+      }
+      if (errParts.length > 0) {
+        parts.push(...errParts);
+      } else {
+        parts.push(gaxiosErr.toString() || gaxiosErr.name || 'Unknown Error');
+      }
+    } else if (typeof err === 'object') {
+      const errParts: string[] = [];
+      const gaxiosErrLike = err as any;
+      if (gaxiosErrLike.message) {
+        errParts.push(String(gaxiosErrLike.message));
+      }
+      const status = gaxiosErrLike.status ?? gaxiosErrLike.response?.status;
+      if (status !== undefined && !isNaN(status) && status !== resp.status) {
+        errParts.push(`status: ${status}`);
+      }
+      const statusText = gaxiosErrLike.response?.statusText;
+      if (statusText) {
+        errParts.push(`statusText: ${statusText}`);
+      }
+      const responseData = gaxiosErrLike.response?.data;
+      if (responseData !== undefined && responseData !== null && responseData !== '') {
+        errParts.push(
+          `response: ${
+            typeof responseData === 'object'
+              ? JSON.stringify(responseData)
+              : responseData
+          }`,
+        );
+      }
+      if (gaxiosErrLike.code) {
+        errParts.push(`code: ${String(gaxiosErrLike.code)}`);
+      }
+
+      if (errParts.length > 0) {
+        parts.push(...errParts);
+      } else {
+        const stringified = JSON.stringify(err);
+        if (stringified && stringified !== '{}') {
+          parts.push(stringified);
+        }
+      }
+    } else if (typeof err === 'string') {
+      if (err !== '') {
+        parts.push(err);
+      }
+    } else {
+      parts.push(String(err));
+    }
+  }
+
+  const suffix = parts.join(' - ');
+  return new Error(`${prefix} - ${suffix || 'Unknown Error'}`);
+}
+
 export function upload(cfg: UploadConfig) {
   return new Upload(cfg);
 }

handwritten/storage/test/resumable-upload.ts

@@ -2275,10 +2275,10 @@ describe('resumable-upload', () => {
     describe('500s', () => {
       const RESP = {status: 500, data: 'error message from server'};
 
-      it('should increase the retry count if less than limit', () => {
+      it('should increase the retry count if less than limit', async () => {
         up.getRetryDelay = () => 1;
         assert.strictEqual(up.numRetries, 0);
-        assert.strictEqual(up.onResponse(RESP), false);
+        assert.strictEqual(await up.onResponse(RESP), false);
         assert.strictEqual(up.numRetries, 1);
       });
 
@@ -2287,15 +2287,17 @@ describe('resumable-upload', () => {
         up.destroy = (err: Error) => {
           assert.strictEqual(
             err.message,
-            `Retry limit exceeded - ${JSON.stringify(RESP.data)}`
+            `Retry limit exceeded - status: 500 - error message from server`,
           );
           done();
         };
 
-        up.onResponse(RESP);
-        up.onResponse(RESP);
-        up.onResponse(RESP);
-        up.onResponse(RESP);
+        (async () => {
+          await up.onResponse(RESP);
+          await up.onResponse(RESP);
+          await up.onResponse(RESP);
+          await up.onResponse(RESP);
+        })().catch(done);
       });
 
       describe('exponential back off', () => {
@@ -2321,19 +2323,19 @@ describe('resumable-upload', () => {
             assert(delay <= maxTime);
 
             // make it keep retrying until the limit is reached
-            up.onResponse(RESP);
+            void up.onResponse(RESP);
           };
 
           up.on('error', (err: Error) => {
             assert.strictEqual(up.numRetries, 3);
             assert.strictEqual(
               err.message,
-              `Retry limit exceeded - ${JSON.stringify(RESP.data)}`
+              `Retry limit exceeded - status: 500 - error message from server`,
             );
             done();
           });
 
-          up.onResponse(RESP);
+          void up.onResponse(RESP);
           clock.runAll();
         });
       });
@@ -2348,23 +2350,22 @@ describe('resumable-upload', () => {
           assert.strictEqual(resp, RESP);
           done();
         });
-        up.onResponse(RESP);
+        void up.onResponse(RESP);
       });
 
-      it('should return true', () => {
+      it('should return true', async () => {
         up.getRetryDelay = () => 1;
-        assert.strictEqual(up.onResponse(RESP), true);
+        assert.strictEqual(await up.onResponse(RESP), true);
       });
 
-      it('should handle a custom status code when passed a retry function', () => {
+      it('should handle a custom status code when passed a retry function', async () => {
         up.getRetryDelay = () => 1;
         const RESP = {status: 1000};
         const customHandlerFunction = (err: ApiError) => {
           return err.code === 1000;
         };
         up.retryOptions.retryableErrorFn = customHandlerFunction;
-
-        assert.strictEqual(up.onResponse(RESP), false);
+        assert.strictEqual(await up.onResponse(RESP), false);
       });
     });
   });
@@ -2490,6 +2491,80 @@ describe('resumable-upload', () => {
 
       up.attemptDelayedRetry({});
     });
+
+    it('should include correct details for standard native Errors', done => {
+      up.numRetries = 3;
+      up.retryLimit = 3;
+      const nativeError = new Error('native connection issue');
+      
+      up.on('error', (err: Error) => {
+        assert.strictEqual(
+          err.message,
+          'Retry limit exceeded - native connection issue',
+        );
+        done();
+      });
+
+      up.attemptDelayedRetry({
+        status: NaN,
+        data: nativeError,
+      });
+    });
+
+    it('should include correct details for custom errors with empty messages', done => {
+      up.numRetries = 3;
+      up.retryLimit = 3;
+      const customError = new Error('');
+      (customError as any).code = 'ERR_SOMETHING_SPECIAL';
+
+      up.on('error', (err: Error) => {
+        assert.strictEqual(
+          err.message,
+          'Retry limit exceeded - code: ERR_SOMETHING_SPECIAL',
+        );
+        done();
+      });
+
+      up.attemptDelayedRetry({
+        status: NaN,
+        data: customError,
+      });
+    });
+
+    it('should include correct details for GaxiosErrors with empty/missing response bodies', done => {
+      up.numRetries = 3;
+      up.retryLimit = 3;
+
+      const gaxiosError = new GaxiosError(
+        'Request failed with status code 429',
+        {
+          method: 'POST',
+          url: 'https://example.com',
+        } as any,
+        {
+          status: 429,
+          statusText: 'Too Many Requests',
+          data: '',
+          config: {},
+          headers: {},
+        } as any
+      );
+
+      up.on('error', (err: Error) => {
+        // Let's check for the presence of key details rather than an exact string if we are unsure of exact GaxiosError fields,
+        // or assert the expected string. Let's assert the expected string:
+        assert(err.message.includes('Retry limit exceeded'));
+        assert(err.message.includes('Request failed with status code 429'));
+        assert(err.message.includes('status: 429') || err.message.includes('code: 429'));
+        assert(err.message.includes('statusText: Too Many Requests'));
+        done();
+      });
+
+      up.attemptDelayedRetry({
+        status: NaN,
+        data: gaxiosError,
+      });
+    });
   });
 
   describe('PROTOCOL_REGEX', () => {