diff --git a/app/api/files/S3Storage.ts b/app/api/files/S3Storage.ts index a456ef3110..f06e7a7648 100644 --- a/app/api/files/S3Storage.ts +++ b/app/api/files/S3Storage.ts @@ -4,24 +4,30 @@ import { ListObjectsV2Command, PutObjectCommand, S3Client, + S3ServiceException, _Object, } from '@aws-sdk/client-s3'; import { config } from 'api/config'; -class S3TimeoutError extends Error { - constructor(cause: Error) { +class S3Error extends Error { + constructor(cause: S3ServiceException) { super(cause.message, { cause }); } + + get originalError() { + return this.cause as S3ServiceException; + } + + get httpStatusCode() { + return this.originalError.$metadata.httpStatusCode; + } } const catchS3Errors = async (cb: () => Promise): Promise => { try { return await cb(); } catch (err) { - if (err.name === 'TimeoutError') { - throw new S3TimeoutError(err); - } - throw err; + throw new S3Error(err); } }; @@ -92,4 +98,4 @@ export class S3Storage { } } -export { S3TimeoutError }; +export { S3Error }; diff --git a/app/api/files/specs/s3Storage.spec.ts b/app/api/files/specs/s3Storage.spec.ts index 35c601fe97..76d32a869c 100644 --- a/app/api/files/specs/s3Storage.spec.ts +++ b/app/api/files/specs/s3Storage.spec.ts @@ -1,45 +1,81 @@ -import { S3Storage, S3TimeoutError } from '../S3Storage'; +// eslint-disable-next-line max-classes-per-file +import { S3Client } from '@aws-sdk/client-s3'; +import { S3Storage, S3Error } from '../S3Storage'; -let s3Storage: S3Storage; +const expectedMetadata = { + requestId: 'mock-request-123', + cfId: 'mock-cf-456', + httpStatusCode: 500, + attempts: 3, + totalRetryDelay: 1000, +}; -class S3TimeoutClient { +class MockS3Error extends Error { + $metadata = expectedMetadata; + + constructor() { + super('Mock S3 Error'); + this.name = 'S3ServiceError'; + } +} + +class MockS3Client { // eslint-disable-next-line class-methods-use-this send() { - const error = new Error(); - error.name = 'TimeoutError'; - throw error; + throw new MockS3Error(); } } -describe('s3Storage', () => { - beforeAll(async () => { - // @ts-ignore - s3Storage = new S3Storage(new S3TimeoutClient()); +describe('s3Storage error handling', () => { + let s3Storage: S3Storage; + + beforeEach(() => { + s3Storage = new S3Storage(new MockS3Client() as unknown as S3Client); }); - describe('get', () => { - it('should throw S3TimeoutError on timeout', async () => { - await expect(s3Storage.get('dummy_key')).rejects.toBeInstanceOf(S3TimeoutError); + describe('error wrapping', () => { + it('should wrap S3 errors with metadata for get operation', async () => { + try { + await s3Storage.get('some_key'); + fail('Should have thrown an error'); + } catch (error) { + expect(error).toBeInstanceOf(S3Error); + expect(error.originalError.$metadata).toEqual(expectedMetadata); + expect(error.httpStatusCode).toBe(500); + } }); - }); - describe('upload', () => { - it('should throw S3TimeoutError on timeout', async () => { - await expect( - s3Storage.upload('dummy_key', Buffer.from('dummy buffer', 'utf-8')) - ).rejects.toBeInstanceOf(S3TimeoutError); + it('should wrap S3 errors with metadata for upload operation', async () => { + try { + await s3Storage.upload('some_key', Buffer.from('test')); + fail('Should have thrown an error'); + } catch (error) { + expect(error).toBeInstanceOf(S3Error); + expect(error.originalError.$metadata).toEqual(expectedMetadata); + expect(error.httpStatusCode).toBe(500); + } }); - }); - describe('delete', () => { - it('should throw S3TimeoutError on timeout', async () => { - await expect(s3Storage.delete('dummy_key')).rejects.toBeInstanceOf(S3TimeoutError); + it('should wrap S3 errors with metadata for delete operation', async () => { + try { + await s3Storage.delete('some_key'); + fail('Should have thrown an error'); + } catch (error) { + expect(error).toBeInstanceOf(S3Error); + expect(error.originalError.$metadata).toEqual(expectedMetadata); + expect(error.httpStatusCode).toBe(500); + } }); - }); - describe('list', () => { - it('should throw S3TimeoutError on timeout', async () => { - await expect(s3Storage.list()).rejects.toBeInstanceOf(S3TimeoutError); + it('should wrap S3 errors with metadata for list operation', async () => { + try { + await s3Storage.list('some_prefix'); + fail('Should have thrown an error'); + } catch (error) { + expect(error).toBeInstanceOf(S3Error); + expect(error.originalError.$metadata).toEqual(expectedMetadata); + expect(error.httpStatusCode).toBe(500); + } }); }); }); diff --git a/app/api/files/storage.ts b/app/api/files/storage.ts index b282539345..71eaff77ce 100644 --- a/app/api/files/storage.ts +++ b/app/api/files/storage.ts @@ -23,7 +23,7 @@ import { deleteFile, uploadsPath, } from './filesystem'; -import { S3Storage } from './S3Storage'; +import { S3Error, S3Storage } from './S3Storage'; export type FileTypes = NonNullable | 'activitylog' | 'segmentation'; @@ -81,7 +81,10 @@ const catchFileNotFound = async (cb: () => Promise, filename: string): Pro try { return await cb(); } catch (err) { - if (err?.code === 'ENOENT' || err instanceof NoSuchKey) { + if ( + err?.code === 'ENOENT' || + (err instanceof S3Error && err.originalError instanceof NoSuchKey) + ) { throw new FileNotFound(filename, storageType); } throw err; @@ -126,7 +129,10 @@ export const storage = { await access(paths[type](filename)); } } catch (err) { - if (err?.code === 'ENOENT' || err instanceof NoSuchKey) { + if ( + err?.code === 'ENOENT' || + (err instanceof S3Error && err.originalError instanceof NoSuchKey) + ) { return false; } if (err) { diff --git a/app/api/utils/handleError.js b/app/api/utils/handleError.js index 044121352a..bf91d753c1 100644 --- a/app/api/utils/handleError.js +++ b/app/api/utils/handleError.js @@ -2,7 +2,7 @@ import Ajv from 'ajv'; import { UnauthorizedError } from 'api/authorization.v2/errors/UnauthorizedError'; import { ValidationError } from 'api/common.v2/validation/ValidationError'; import { FileNotFound } from 'api/files/FileNotFound'; -import { S3TimeoutError } from 'api/files/S3Storage'; +import { S3Error } from 'api/files/S3Storage'; import { legacyLogger } from 'api/log'; import { appContext } from 'api/utils/AppContext'; import { createError } from 'api/utils/index'; @@ -64,8 +64,8 @@ const prettifyError = (error, { req = {}, uncaught = false } = {}) => { result = { code: 500, message: util.inspect(error), logLevel: 'error' }; } - if (error instanceof S3TimeoutError) { - result = { code: 408, message: util.inspect(error), logLevel: 'debug' }; + if (error instanceof S3Error) { + result = { code: error.httpStatusCode || 503, message: util.inspect(error), logLevel: 'debug' }; } if (error instanceof Ajv.ValidationError) { diff --git a/app/api/utils/specs/handleError.spec.js b/app/api/utils/specs/handleError.spec.js index 840486b144..0b034b4c05 100644 --- a/app/api/utils/specs/handleError.spec.js +++ b/app/api/utils/specs/handleError.spec.js @@ -2,7 +2,7 @@ import { legacyLogger } from 'api/log'; import { createError } from 'api/utils'; import { errors as elasticErrors } from '@elastic/elasticsearch'; -import { S3TimeoutError } from 'api/files/S3Storage'; +import { S3Error } from 'api/files/S3Storage'; import { appContext } from 'api/utils/AppContext'; import util from 'node:util'; import { handleError, prettifyError } from '../handleError'; @@ -20,17 +20,31 @@ describe('handleError', () => { }); describe('errors by type', () => { - describe('and is instance of S3TimeoutError', () => { - it('should be a debug logLevel and a 408 http code', () => { - const errorInstance = new S3TimeoutError(new Error('timeout')); + describe('and is instance of S3Error', () => { + it('should be a debug logLevel and use the error httpStatusCode', () => { + const originalError = new Error('original error'); + originalError.$metadata = { httpStatusCode: 404 }; + const errorInstance = new S3Error(originalError); const error = handleError(errorInstance); expect(error).toMatchObject({ - code: 408, + code: 404, logLevel: 'debug', }); - expect(legacyLogger.debug.mock.calls[0][0]).toContain('timeout'); + expect(legacyLogger.debug.mock.calls[0][0]).toContain('original error'); + }); + it('should use 503 status code if httpStatusCode is undefined', () => { + const originalError = new Error('original error'); + originalError.$metadata = {}; + const errorInstance = new S3Error(originalError); + const error = handleError(errorInstance); + expect(error).toMatchObject({ + code: 503, + logLevel: 'debug', + }); + expect(legacyLogger.debug.mock.calls[0][0]).toContain('original error'); }); }); + describe('when error is instance of Error', () => { it('should return the error with 500 code without the original error and error stack', () => { const errorInstance = new Error('error');