From 703f8c08bd97ca588b69b3d00183c0c06e426739 Mon Sep 17 00:00:00 2001 From: Rudra Sharans Date: Wed, 8 Oct 2025 00:01:00 +0530 Subject: [PATCH 01/10] Modifying large size files upload to S3 Signed-off-by: Rudra Sharans --- .changeset/lemon-corners-hug.md | 5 + .../src/stages/publish/awsS3.test.ts | 69 ++-- .../techdocs-node/src/stages/publish/awsS3.ts | 323 ++++++++++++++++-- 3 files changed, 329 insertions(+), 68 deletions(-) create mode 100644 .changeset/lemon-corners-hug.md diff --git a/.changeset/lemon-corners-hug.md b/.changeset/lemon-corners-hug.md new file mode 100644 index 0000000000..b41c09c928 --- /dev/null +++ b/.changeset/lemon-corners-hug.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs-node': patch +--- + +There was an issue in the uploading of large size files to the AWS S3. We have modified the logic by adding retry along with multipart uploading functionality. diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts index 6fb855d199..c2af7febb5 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts @@ -31,7 +31,7 @@ import { AwsCredentialProviderOptions, DefaultAwsCredentialsManager, } from '@backstage/integration-aws-node'; -import { mockClient, AwsClientStub } from 'aws-sdk-client-mock'; +import { mockClient } from 'aws-sdk-client-mock'; import express from 'express'; import request from 'supertest'; import path from 'path'; @@ -44,9 +44,10 @@ import { } from '@backstage/backend-test-utils'; const env = process.env; -let s3Mock: AwsClientStub; +let s3Mock: any; -const mockDir = createMockDirectory(); +// Create a new MockDirectory for each test to avoid Windows file locking issues +let mockDir: ReturnType; function getMockCredentialProvider(): Promise { return Promise.resolve({ @@ -155,7 +156,7 @@ describe('AwsS3Publish', () => { build_timestamp: 612741599, }; - const directory = getEntityRootDir(entity); + let directory: string; const files = { 'index.html': '', @@ -176,7 +177,7 @@ describe('AwsS3Publish', () => { }, }; - beforeEach(() => { + beforeEach(async () => { process.env = { ...env }; process.env.AWS_REGION = 'us-west-2'; @@ -185,20 +186,26 @@ describe('AwsS3Publish', () => { getMockCredentialProvider(), ); + // Create a fresh mockdirectory for each test to avoid windows file locking + mockDir = createMockDirectory(); + // Calculate directory path with the new mockDir instance + directory = getEntityRootDir(entity); + + // Set up the test files mockDir.setContent({ [directory]: files, }); - s3Mock = mockClient(S3Client); + s3Mock = mockClient(S3Client as any); - s3Mock.on(HeadObjectCommand).callsFake(input => { + s3Mock.on(HeadObjectCommand).callsFake((input: any) => { if (!fs.pathExistsSync(mockDir.resolve(input.Key))) { throw new Error('File does not exist'); } return {}; }); - s3Mock.on(GetObjectCommand).callsFake(input => { + s3Mock.on(GetObjectCommand).callsFake((input: any) => { if (fs.pathExistsSync(mockDir.resolve(input.Key))) { return { Body: Readable.from(fs.readFileSync(mockDir.resolve(input.Key))), @@ -208,14 +215,14 @@ describe('AwsS3Publish', () => { throw new Error(`The file ${input.Key} does not exist!`); }); - s3Mock.on(HeadBucketCommand).callsFake(input => { + s3Mock.on(HeadBucketCommand).callsFake((input: any) => { if (input.Bucket === 'errorBucket') { throw new Error('Bucket does not exist'); } return {}; }); - s3Mock.on(ListObjectsV2Command).callsFake(input => { + s3Mock.on(ListObjectsV2Command).callsFake((input: any) => { if ( input.Bucket === 'delete_stale_files_success' || input.Bucket === 'delete_stale_files_error' @@ -227,7 +234,7 @@ describe('AwsS3Publish', () => { return {}; }); - s3Mock.on(DeleteObjectCommand).callsFake(input => { + s3Mock.on(DeleteObjectCommand).callsFake((input: any) => { if (input.Bucket === 'delete_stale_files_error') { throw new Error('Message'); } @@ -235,7 +242,7 @@ describe('AwsS3Publish', () => { }); s3Mock.on(UploadPartCommand).rejects(); - s3Mock.on(PutObjectCommand).callsFake(input => { + s3Mock.on(PutObjectCommand).callsFake((input: any) => { mockDir.addContent({ [input.Key]: input.Body }); }); }); @@ -320,7 +327,7 @@ describe('AwsS3Publish', () => { `default/component/backstage/assets/main.css`, ]), }); - }); + }, 30000); it('should publish a directory as well when legacy casing is used', async () => { const publisher = await createPublisherFromConfig({ @@ -333,7 +340,7 @@ describe('AwsS3Publish', () => { `default/Component/backstage/assets/main.css`, ]), }); - }); + }, 30000); it('should publish a directory when root path is specified', async () => { const publisher = await createPublisherFromConfig({ @@ -346,7 +353,7 @@ describe('AwsS3Publish', () => { `backstage-data/techdocs/default/component/backstage/assets/main.css`, ]), }); - }); + }, 30000); it('should publish a directory when root path is specified and legacy casing is used', async () => { const publisher = await createPublisherFromConfig({ @@ -360,7 +367,7 @@ describe('AwsS3Publish', () => { `backstage-data/techdocs/default/Component/backstage/assets/main.css`, ]), }); - }); + }, 30000); it('should publish a directory when sse is specified', async () => { const publisher = await createPublisherFromConfig({ @@ -373,7 +380,7 @@ describe('AwsS3Publish', () => { 'default/component/backstage/assets/main.css', ]), }); - }); + }, 30000); it('should fail to publish a directory', async () => { const wrongPathToGeneratedDirectory = mockDir.resolve( @@ -408,7 +415,7 @@ describe('AwsS3Publish', () => { expect(loggerInfoSpy).toHaveBeenLastCalledWith( `Successfully deleted stale files for Entity ${entity.metadata.name}. Total number of files: 1`, ); - }); + }, 30000); it('should log error when the stale files deletion fails', async () => { const bucketName = 'delete_stale_files_error'; @@ -419,7 +426,7 @@ describe('AwsS3Publish', () => { expect(loggerErrorSpy).toHaveBeenLastCalledWith( 'Unable to delete file(s) from AWS S3. Error: Message', ); - }); + }, 30000); }); describe('hasDocsBeenGenerated', () => { @@ -427,7 +434,7 @@ describe('AwsS3Publish', () => { const publisher = await createPublisherFromConfig(); await publisher.publish({ entity, directory }); expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true); - }); + }, 30000); it('should return true if docs has been generated even if the legacy case is enabled', async () => { const publisher = await createPublisherFromConfig({ @@ -435,7 +442,7 @@ describe('AwsS3Publish', () => { }); await publisher.publish({ entity, directory }); expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true); - }); + }, 30000); it('should return true if docs has been generated if root path is specified', async () => { const publisher = await createPublisherFromConfig({ @@ -443,7 +450,7 @@ describe('AwsS3Publish', () => { }); await publisher.publish({ entity, directory }); expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true); - }); + }, 30000); it('should return true if docs has been generated if root path is specified and legacy casing is used', async () => { const publisher = await createPublisherFromConfig({ @@ -452,7 +459,7 @@ describe('AwsS3Publish', () => { }); await publisher.publish({ entity, directory }); expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true); - }); + }, 30000); it('should return false if docs has not been generated', async () => { const publisher = await createPublisherFromConfig(); @@ -475,7 +482,7 @@ describe('AwsS3Publish', () => { expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual( techdocsMetadata, ); - }); + }, 30000); it('should return tech docs metadata even if the legacy case is enabled', async () => { const publisher = await createPublisherFromConfig({ @@ -485,7 +492,7 @@ describe('AwsS3Publish', () => { expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual( techdocsMetadata, ); - }); + }, 30000); it('should return tech docs metadata even if root path is specified', async () => { const publisher = await createPublisherFromConfig({ @@ -495,7 +502,7 @@ describe('AwsS3Publish', () => { expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual( techdocsMetadata, ); - }); + }, 30000); it('should return tech docs metadata if root path is specified and legacy casing is used', async () => { const publisher = await createPublisherFromConfig({ @@ -506,7 +513,7 @@ describe('AwsS3Publish', () => { expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual( techdocsMetadata, ); - }); + }, 30000); it('should return tech docs metadata when json encoded with single quotes', async () => { const techdocsMetadataPath = path.join( @@ -528,7 +535,7 @@ describe('AwsS3Publish', () => { ); fs.writeFileSync(techdocsMetadataPath, techdocsMetadataContent); - }); + }, 30000); it('should return an error if the techdocs_metadata.json file is not present', async () => { const publisher = await createPublisherFromConfig(); @@ -549,7 +556,7 @@ describe('AwsS3Publish', () => { }); it('should return an error if the techdocs_metadata.json file cannot be read from stream', async () => { - s3Mock.on(GetObjectCommand).callsFake(_ => { + s3Mock.on(GetObjectCommand).callsFake((_: any) => { return { Body: new ErrorReadable('No stream!'), }; @@ -582,7 +589,7 @@ describe('AwsS3Publish', () => { const publisher = await createPublisherFromConfig(); await publisher.publish({ entity, directory }); app = express().use(publisher.docsRouter()); - }); + }, 30000); it('should pass expected object path to bucket', async () => { // Ensures leading slash is trimmed and encoded path is decoded. @@ -688,7 +695,7 @@ describe('AwsS3Publish', () => { }); it('should return 404 if file cannot be read from stream', async () => { - s3Mock.on(GetObjectCommand).callsFake(_ => { + s3Mock.on(GetObjectCommand).callsFake((_: any) => { return { Body: new ErrorReadable('No stream!'), }; diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.ts b/plugins/techdocs-node/src/stages/publish/awsS3.ts index bd8b96ee14..9f6ce7cab5 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.ts @@ -26,10 +26,12 @@ import { DeleteObjectCommand, HeadBucketCommand, HeadObjectCommand, + PutObjectCommand, PutObjectCommandInput, ListObjectsV2CommandOutput, ListObjectsV2Command, S3Client, + S3ServiceException, } from '@aws-sdk/client-s3'; import { fromTemporaryCredentials } from '@aws-sdk/credential-providers'; import { NodeHttpHandler } from '@smithy/node-http-handler'; @@ -84,6 +86,7 @@ export class AwsS3Publish implements PublisherBase { private readonly logger: LoggerService; private readonly bucketRootPath: string; private readonly sse?: 'aws:kms' | 'AES256'; + private readonly maxAttempts: number; constructor(options: { storageClient: S3Client; @@ -92,6 +95,7 @@ export class AwsS3Publish implements PublisherBase { logger: LoggerService; bucketRootPath: string; sse?: 'aws:kms' | 'AES256'; + maxAttempts: number; }) { this.storageClient = options.storageClient; this.bucketName = options.bucketName; @@ -99,6 +103,7 @@ export class AwsS3Publish implements PublisherBase { this.logger = options.logger; this.bucketRootPath = options.bucketRootPath; this.sse = options.sse; + this.maxAttempts = options.maxAttempts; } static async fromConfig( @@ -175,10 +180,22 @@ export class AwsS3Publish implements PublisherBase { ...(region && { region }), ...(endpoint && { endpoint }), ...(forcePathStyle && { forcePathStyle }), - ...(maxAttempts && { maxAttempts }), + // Enhanced retry configuration for better reliability + maxAttempts: maxAttempts || 5, + retryMode: 'adaptive', ...(httpsProxy && { requestHandler: new NodeHttpHandler({ httpsAgent: new HttpsProxyAgent({ proxy: httpsProxy }), + // Enhanced connection setting for large file uploads + connectionTimeout: 60000, + socketTimeout: 120000, + }), + }), + // Add default request handler with enhanced timeouts if no proxy + ...(!httpsProxy && { + requestHandler: new NodeHttpHandler({ + connectionTimeout: 60000, + socketTimeout: 120000, }), }), }); @@ -195,6 +212,7 @@ export class AwsS3Publish implements PublisherBase { legacyPathCasing, logger, sse, + maxAttempts: maxAttempts || 5, }); } @@ -250,6 +268,126 @@ export class AwsS3Publish implements PublisherBase { return explicitCredentials; } + /** + * Custom retry wrapper for S3 operations with detailed error handling. + */ + private async retryOperation( + operation: () => Promise, + operationName: string, + maxAttempts: number = 3, + ): Promise { + let attempts = maxAttempts; + let LastError: S3ServiceException; + + while (attempts > 0) { + try { + return await operation(); + } catch (error: unknown) { + LastError = error as S3ServiceException; + attempts--; + + const httpStatusCode = LastError.$metadata?.httpStatusCode; + const errorCode = LastError.name; + + this.logger.warn(`${operationName} failed.`, { + errorCode, + httpStatusCode, + attemptsRemaining: attempts, + currentAttempt: maxAttempts - attempts, + totalAttempts: maxAttempts, + error: LastError.message, + }); + // Determine if we should retry based on error type + const shouldRetry = this.shouldRetryOperation(LastError, attempts); + if (!shouldRetry || attempts === 0) { + this.logger.error( + `${operationName} failed after all retries: ${LastError.message}`, + ); + throw LastError; + } + // Enhanced exponential backoff with jitter for upload operation + let baseDelay = 1000; + if (operationName.startsWith('Upload-')) { + // for uploads use longer base delay due to potential multipart commplexity + baseDelay = 2000; + } + const backoffDelay = Math.min( + baseDelay * Math.pow(2, maxAttempts - attempts), + 30000, + ); + const jitter = Math.random() * 1000; + const totalDelay = backoffDelay + jitter; + await new Promise(resolve => setTimeout(resolve, totalDelay)); + } + } + // Final attempt without retry wrapper + return operation(); + } + + /** + * Determines if an S3 operation should be retried based on the error details. + */ + private shouldRetryOperation( + error: S3ServiceException, + attemptsRemaining: number, + ): boolean { + const httpStatusCode = error.$metadata?.httpStatusCode; + const errorCode = error.name; + // Handle invalid part errors first - these are retriable for multipart uploads + if (errorCode === 'InvalidPart') { + return attemptsRemaining > 0; + } + // Dont retry for client errors (4xx) except specific cases + if (httpStatusCode && httpStatusCode >= 400 && httpStatusCode < 500) { + // Retry specfic 4xx errors that might be transient + const retriable4xxErrors = [ + 'RequestTimeOut', + 'RequestTimeoutException', + 'PriorRequestNotComplete', + 'ConnectionError', + 'RequestTimeToooSkewed', + 'InvalidPart', + 'NoSuchUpload', + ]; + if (!retriable4xxErrors.includes(errorCode)) { + return false; + } + } + // Always retry for server errors (5xx) + if (httpStatusCode && httpStatusCode >= 500) { + return attemptsRemaining > 0; + } + // Retry specific network/connection errors and multipart upload errors + const retriableErrors = [ + 'NetworkingError', + 'TimeoutError', + 'ConnectionError', + 'ECONNRESET', + 'ENOTFOUND', + 'ECONNREFUSED', + 'ETIMEDOUT', + 'ServiceUnavailable', + 'SlowDown', + 'Throttling', + 'ThrottlingException', + 'ProvisionedThroughputExceededException', + // Multipart upload specific errors - now handled above but kept for completeness + 'InvalidPart', + 'NoSuchUpload', + 'UploadTimeout', + 'EntityTooLarge', + 'InternalError', + 'IncompleteBody', + 'RequestTimeout', + ]; + return ( + retriableErrors.some( + retriableError => + errorCode.includes(retriableError) || + error.message.includes(retriableError), + ) && attemptsRemaining > 0 + ); + } /** * Check if the defined bucket exists. Being able to connect means the configuration is good @@ -273,13 +411,15 @@ export class AwsS3Publish implements PublisherBase { 'explicitly defining credentials and region in techdocs.publisher.awsS3 in app config or ' + 'by using environment variables. Refer to https://backstage.io/docs/features/techdocs/using-cloud-storage', ); - this.logger.error(`from AWS client library`, error); + this.logger.error( + `from AWS client library`, + error instanceof Error ? error : new Error(String(error)), + ); return { isAvailable: false, }; } } - /** * Upload all the files from the generated `directory` to the S3 bucket. * Directory structure used in the bucket is - entityNamespace/entityKind/entityName/index.html @@ -293,6 +433,9 @@ export class AwsS3Publish implements PublisherBase { const bucketRootPath = this.bucketRootPath; const sse = this.sse; + // Track timing for performance monitoring + const publishStartTime = Date.now(); + // First, try to retrieve a list of all individual files currently existing let existingFiles: string[] = []; try { @@ -302,9 +445,20 @@ export class AwsS3Publish implements PublisherBase { useLegacyPathCasing, bucketRootPath, ); - existingFiles = await this.getAllObjectsFromBucket({ - prefix: remoteFolder, - }); + const response = await this.retryOperation( + async () => { + const listCommand = new ListObjectsV2Command({ + Bucket: this.bucketName, + Prefix: remoteFolder, + }); + return this.storageClient.send(listCommand); + }, + 'ListObjects', + this.maxAttempts, + ); + existingFiles = (response.Contents || []) + .map(f => f.Key || '') + .filter(f => !!f); } catch (e) { assertError(e); this.logger.error( @@ -320,30 +474,103 @@ export class AwsS3Publish implements PublisherBase { // e.g. ['index.html', 'sub-page/index.html', 'assets/images/favicon.png'] absoluteFilesToUpload = await getFileTreeRecursively(directory); + let uploadCounter = 0; + await bulkStorageOperation( async absoluteFilePath => { + uploadCounter++; const relativeFilePath = path.relative(directory, absoluteFilePath); - const fileStream = fs.createReadStream(absoluteFilePath); - + const s3Key = getCloudPathForLocalPath( + entity, + relativeFilePath, + useLegacyPathCasing, + bucketRootPath, + ); const params: PutObjectCommandInput = { Bucket: this.bucketName, - Key: getCloudPathForLocalPath( - entity, - relativeFilePath, - useLegacyPathCasing, - bucketRootPath, - ), - Body: fileStream, + Key: s3Key, + Body: absoluteFilePath, ...(sse && { ServerSideEncryption: sse }), }; objects.push(params.Key!); + // Get file stats before upload + const stats = await fs.stat(absoluteFilePath); + const fileSizeInBytes = stats.size; - const upload = new Upload({ - client: this.storageClient, - params, - }); - return upload.done(); + // Use retry wrapper for uploads with enhanced error handling + try { + const result = await this.retryOperation( + async () => { + const fiveMB = 5 * 1024 * 1024; + // For files smaller than 5MB, use simple PutObject to avoid multipart complexity + if (fileSizeInBytes < fiveMB) { + const fileContent = await fs.readFile(absoluteFilePath); + const putParams = { ...params, Body: fileContent }; + return this.storageClient.send( + new PutObjectCommand(putParams), + ); + } + // For files 5MB and larger, use multipart upload with enhanced configuration + const calaculatedPartSize = Math.max( + fiveMB, + Math.ceil(fileSizeInBytes / 10000), + ); + const upload = new Upload({ + client: this.storageClient, + params, + // Configure miltipart upload option for better reliability + partSize: calaculatedPartSize, + queueSize: 3, + leavePartsOnError: false, + }); + return upload.done(); + }, + `Upload-${params.Key}`, + this.maxAttempts, + ); + return result; + } catch (error) { + const s3Error = error as any; + const errorName = s3Error?.name || 'Unknown'; + + // Check if this is a multipart upload failure that we can handle + if ( + fileSizeInBytes >= 5 * 1024 * 1024 && + (errorName === 'InvalidPart' || errorName === 'NoSuchUpload') + ) { + this.logger.warn( + `Multipart upload failed for ${params.Key}, Attempting simple upload fallback.`, + ); + try { + // Attempt simple upload as a fallback + const fileContent = await fs.readFile(absoluteFilePath); + const simpleParams = { ...params, Body: fileContent }; + const fallbackResult = await this.storageClient.send( + new PutObjectCommand(simpleParams), + ); + this.logger.info( + `Simple upload fallback succeeded for ${params.Key}`, + ); + return fallbackResult; + } catch (fallbackError) { + this.logger.error( + `Both multipart and simple upload failed for ${params.Key}: ${ + fallbackError instanceof Error + ? fallbackError.message + : String(fallbackError) + }`, + ); + // Fall through to throw original error + } + } + this.logger.error( + `Upload failed for ${params.Key}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + throw error; + } }, absoluteFilesToUpload, { concurrencyLimit: 10 }, @@ -373,17 +600,21 @@ export class AwsS3Publish implements PublisherBase { await bulkStorageOperation( async relativeFilePath => { - return await this.storageClient.send( - new DeleteObjectCommand({ - Bucket: this.bucketName, - Key: relativeFilePath, - }), + return this.retryOperation( + async () => { + const deleteCommand = new DeleteObjectCommand({ + Bucket: this.bucketName, + Key: relativeFilePath, + }); + return this.storageClient.send(deleteCommand); + }, + 'DeleteObject', + this.maxAttempts, ); }, staleFiles, { concurrencyLimit: 10 }, ); - this.logger.info( `Successfully deleted stale files for Entity ${entity.metadata.name}. Total number of files: ${staleFiles.length}`, ); @@ -391,6 +622,13 @@ export class AwsS3Publish implements PublisherBase { const errorMessage = `Unable to delete file(s) from AWS S3. ${error}`; this.logger.error(errorMessage); } + const publishEndTime = Date.now(); + const publishDurationMs = publishEndTime - publishStartTime; + this.logger.info( + `Successfully published ${objects.length} files for ${ + entity.metadata.name + } in ${Math.round(publishDurationMs / 1000)}s`, + ); return { objects }; } @@ -413,11 +651,16 @@ export class AwsS3Publish implements PublisherBase { } try { - const resp = await this.storageClient.send( - new GetObjectCommand({ - Bucket: this.bucketName, - Key: `${entityRootDir}/techdocs_metadata.json`, - }), + const resp = await this.retryOperation( + async () => { + const getCommand = new GetObjectCommand({ + Bucket: this.bucketName, + Key: `${entityRootDir}/techdocs_metadata.json`, + }); + return this.storageClient.send(getCommand); + }, + 'GetTechDocsMetadata', + this.maxAttempts, ); const techdocsMetadataJson = await streamToBuffer( @@ -598,12 +841,18 @@ export class AwsS3Publish implements PublisherBase { let allObjects: ListObjectsV2CommandOutput; // Iterate through every file in the root of the publisher. do { - allObjects = await this.storageClient.send( - new ListObjectsV2Command({ - Bucket: this.bucketName, - ContinuationToken: nextContinuation, - ...(prefix ? { Prefix: prefix } : {}), - }), + const currentToken = nextContinuation; + allObjects = await this.retryOperation( + async () => { + const listCommand = new ListObjectsV2Command({ + Bucket: this.bucketName, + ContinuationToken: currentToken, + ...(prefix ? { Prefix: prefix } : {}), + }); + return this.storageClient.send(listCommand); + }, + 'GetAllObjects', + this.maxAttempts, ); objects.push( ...(allObjects.Contents || []).map(f => f.Key || '').filter(f => !!f), From c13704bf528e8ab11c943d74d4dc02da4e28770d Mon Sep 17 00:00:00 2001 From: Rudra Sharans Date: Wed, 8 Oct 2025 00:17:09 +0530 Subject: [PATCH 02/10] Fixing test cases Signed-off-by: Rudra Sharans --- plugins/techdocs-node/src/stages/publish/awsS3.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts index c2af7febb5..f30cf50dfc 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts @@ -412,7 +412,7 @@ describe('AwsS3Publish', () => { bucketName: bucketName, }); await publisher.publish({ entity, directory }); - expect(loggerInfoSpy).toHaveBeenLastCalledWith( + expect(loggerInfoSpy).toHaveBeenCalledWith( `Successfully deleted stale files for Entity ${entity.metadata.name}. Total number of files: 1`, ); }, 30000); From 4fbde9e3f11c86a940556a72fd4bb0f98a91cbc1 Mon Sep 17 00:00:00 2001 From: Rudra Sharans Date: Wed, 22 Oct 2025 22:55:28 +0530 Subject: [PATCH 03/10] PR comments Signed-off-by: Rudra Sharans --- .../src/stages/publish/awsS3.test.ts | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts index f30cf50dfc..3dc594220b 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts @@ -196,16 +196,16 @@ describe('AwsS3Publish', () => { [directory]: files, }); - s3Mock = mockClient(S3Client as any); + s3Mock = mockClient(S3Client); - s3Mock.on(HeadObjectCommand).callsFake((input: any) => { + s3Mock.on(HeadObjectCommand).callsFake((input: { Key: string }) => { if (!fs.pathExistsSync(mockDir.resolve(input.Key))) { throw new Error('File does not exist'); } return {}; }); - s3Mock.on(GetObjectCommand).callsFake((input: any) => { + s3Mock.on(GetObjectCommand).callsFake((input: { Key: string }) => { if (fs.pathExistsSync(mockDir.resolve(input.Key))) { return { Body: Readable.from(fs.readFileSync(mockDir.resolve(input.Key))), @@ -215,14 +215,14 @@ describe('AwsS3Publish', () => { throw new Error(`The file ${input.Key} does not exist!`); }); - s3Mock.on(HeadBucketCommand).callsFake((input: any) => { + s3Mock.on(HeadBucketCommand).callsFake((input: { Bucket: string }) => { if (input.Bucket === 'errorBucket') { throw new Error('Bucket does not exist'); } return {}; }); - s3Mock.on(ListObjectsV2Command).callsFake((input: any) => { + s3Mock.on(ListObjectsV2Command).callsFake((input: { Bucket: string }) => { if ( input.Bucket === 'delete_stale_files_success' || input.Bucket === 'delete_stale_files_error' @@ -234,7 +234,7 @@ describe('AwsS3Publish', () => { return {}; }); - s3Mock.on(DeleteObjectCommand).callsFake((input: any) => { + s3Mock.on(DeleteObjectCommand).callsFake((input: { Bucket: string }) => { if (input.Bucket === 'delete_stale_files_error') { throw new Error('Message'); } @@ -242,9 +242,11 @@ describe('AwsS3Publish', () => { }); s3Mock.on(UploadPartCommand).rejects(); - s3Mock.on(PutObjectCommand).callsFake((input: any) => { - mockDir.addContent({ [input.Key]: input.Body }); - }); + s3Mock + .on(PutObjectCommand) + .callsFake((input: { Key: string; Body: any }) => { + mockDir.addContent({ [input.Key]: input.Body }); + }); }); afterEach(() => { From f2f84ad597093e583e38107f2c93f96bf1c29eb5 Mon Sep 17 00:00:00 2001 From: Vidhan Shah Date: Sat, 8 Nov 2025 07:58:34 +0530 Subject: [PATCH 04/10] Improving code as per reviews Signed-off-by: Vidhan Shah --- .../src/stages/publish/awsS3.test.ts | 137 ++++++++++++++++-- .../techdocs-node/src/stages/publish/awsS3.ts | 97 +++++-------- 2 files changed, 166 insertions(+), 68 deletions(-) diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts index f30cf50dfc..52c57a9c25 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts @@ -22,6 +22,7 @@ import { ListObjectsV2Command, PutObjectCommand, S3Client, + S3ServiceException, UploadPartCommand, } from '@aws-sdk/client-s3'; import { Entity, DEFAULT_NAMESPACE } from '@backstage/catalog-model'; @@ -196,16 +197,16 @@ describe('AwsS3Publish', () => { [directory]: files, }); - s3Mock = mockClient(S3Client as any); + s3Mock = mockClient(S3Client); - s3Mock.on(HeadObjectCommand).callsFake((input: any) => { + s3Mock.on(HeadObjectCommand).callsFake((input: { Key: string }) => { if (!fs.pathExistsSync(mockDir.resolve(input.Key))) { throw new Error('File does not exist'); } return {}; }); - s3Mock.on(GetObjectCommand).callsFake((input: any) => { + s3Mock.on(GetObjectCommand).callsFake((input: { Key: string }) => { if (fs.pathExistsSync(mockDir.resolve(input.Key))) { return { Body: Readable.from(fs.readFileSync(mockDir.resolve(input.Key))), @@ -215,14 +216,14 @@ describe('AwsS3Publish', () => { throw new Error(`The file ${input.Key} does not exist!`); }); - s3Mock.on(HeadBucketCommand).callsFake((input: any) => { + s3Mock.on(HeadBucketCommand).callsFake((input: { Bucket: string }) => { if (input.Bucket === 'errorBucket') { throw new Error('Bucket does not exist'); } return {}; }); - s3Mock.on(ListObjectsV2Command).callsFake((input: any) => { + s3Mock.on(ListObjectsV2Command).callsFake((input: { Bucket: string }) => { if ( input.Bucket === 'delete_stale_files_success' || input.Bucket === 'delete_stale_files_error' @@ -234,7 +235,7 @@ describe('AwsS3Publish', () => { return {}; }); - s3Mock.on(DeleteObjectCommand).callsFake((input: any) => { + s3Mock.on(DeleteObjectCommand).callsFake((input: { Bucket: string }) => { if (input.Bucket === 'delete_stale_files_error') { throw new Error('Message'); } @@ -242,9 +243,11 @@ describe('AwsS3Publish', () => { }); s3Mock.on(UploadPartCommand).rejects(); - s3Mock.on(PutObjectCommand).callsFake((input: any) => { - mockDir.addContent({ [input.Key]: input.Body }); - }); + s3Mock + .on(PutObjectCommand) + .callsFake((input: { Key: string; Body: any }) => { + mockDir.addContent({ [input.Key]: input.Body }); + }); }); afterEach(() => { @@ -299,6 +302,122 @@ describe('AwsS3Publish', () => { }); }); + describe('retry mechanism', () => { + it('should retry with custom retry strategy', async () => { + const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + const customRetryStrategy = jest.fn((error: any) => { + return error.name === 'NetworkingError'; + }); + + s3Mock + .on(ListObjectsV2Command) + .rejectsOnce( + new S3ServiceException({ + name: 'NetworkingError', + $fault: 'client', + $metadata: {}, + }), + ) + .resolvesOnce({ Contents: [] }); + + await (publisher as any).retryOperation( + async () => { + return s3Mock.send( + new ListObjectsV2Command({ Bucket: 'bucketName' }), + ); + }, + 'TestOperation', + 3, + customRetryStrategy, + ); + + expect(customRetryStrategy).toHaveBeenCalled(); + }); + + it('should use default retry strategy when no custom strategy provided', async () => { + const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + s3Mock + .on(ListObjectsV2Command) + .rejectsOnce('RequestTimeout') + .resolvesOnce({ Contents: [] }); + + await (publisher as any).retryOperation( + async () => { + return s3Mock.send( + new ListObjectsV2Command({ Bucket: 'bucketName' }), + ); + }, + 'TestOperation', + 3, + ); + }); + + it('should retry on server errors (5xx)', async () => { + const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + s3Mock + .on(ListObjectsV2Command) + .rejectsOnce( + new S3ServiceException({ + name: 'InternalError', + $fault: 'server', + $metadata: { httpStatusCode: 500 }, + }), + ) + .resolvesOnce({ Contents: [] }); + + await (publisher as any).retryOperation( + async () => { + return s3Mock.send( + new ListObjectsV2Command({ Bucket: 'bucketName' }), + ); + }, + 'TestOperation', + 3, + ); + }); + + it('should retry on specific 4xx errors', async () => { + const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + s3Mock + .on(ListObjectsV2Command) + .rejectsOnce('RequestTimeout') + .resolvesOnce({ Contents: [] }); + + await (publisher as any).retryOperation( + async () => { + return s3Mock.send( + new ListObjectsV2Command({ Bucket: 'bucketName' }), + ); + }, + 'TestOperation', + 3, + ); + }); + + it('should not retry on non-retriable 4xx errors', async () => { + const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + s3Mock.on(ListObjectsV2Command).rejectsOnce( + new S3ServiceException({ + name: 'BadRequest', + $fault: 'client', + $metadata: { httpStatusCode: 400 }, + }), + ); + + await expect( + (publisher as any).retryOperation( + async () => { + return s3Mock.send( + new ListObjectsV2Command({ Bucket: 'bucketName' }), + ); + }, + 'TestOperation', + 3, + ), + ).rejects.toHaveProperty('name', 'BadRequest'); + }); + }); + describe('getReadiness', () => { it('should validate correct config', async () => { const publisher = await createPublisherFromConfig(); diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.ts b/plugins/techdocs-node/src/stages/publish/awsS3.ts index 9f6ce7cab5..79b4e73028 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.ts @@ -16,6 +16,10 @@ import { Entity, CompoundEntityRef } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import { assertError, ForwardedError } from '@backstage/errors'; + +// Maximum size in bytes for a single upload part (5MB) +const MAX_SINGLE_UPLOAD_BYTES = 5 * 1024 * 1024; + import { AwsCredentialsManager, DefaultAwsCredentialsManager, @@ -275,67 +279,48 @@ export class AwsS3Publish implements PublisherBase { operation: () => Promise, operationName: string, maxAttempts: number = 3, + shouldRetry: (error: S3ServiceException) => boolean = this + .defaultShouldRetry, ): Promise { - let attempts = maxAttempts; - let LastError: S3ServiceException; - - while (attempts > 0) { + for (let attempt = 1; attempt < maxAttempts; attempt++) { try { return await operation(); - } catch (error: unknown) { - LastError = error as S3ServiceException; - attempts--; + } catch (error) { + const e = error as S3ServiceException; + if (!shouldRetry(e)) { + this.logger.error(`${operationName} failed: ${e.message}`); + throw e; + } - const httpStatusCode = LastError.$metadata?.httpStatusCode; - const errorCode = LastError.name; - - this.logger.warn(`${operationName} failed.`, { - errorCode, - httpStatusCode, - attemptsRemaining: attempts, - currentAttempt: maxAttempts - attempts, - totalAttempts: maxAttempts, - error: LastError.message, + this.logger.warn(`${operationName} failed, retrying...`, { + attempt, + maxAttempts, + error: e.message, + errorCode: e.name, + httpStatusCode: e.$metadata?.httpStatusCode, }); - // Determine if we should retry based on error type - const shouldRetry = this.shouldRetryOperation(LastError, attempts); - if (!shouldRetry || attempts === 0) { - this.logger.error( - `${operationName} failed after all retries: ${LastError.message}`, - ); - throw LastError; - } - // Enhanced exponential backoff with jitter for upload operation - let baseDelay = 1000; - if (operationName.startsWith('Upload-')) { - // for uploads use longer base delay due to potential multipart commplexity - baseDelay = 2000; - } - const backoffDelay = Math.min( - baseDelay * Math.pow(2, maxAttempts - attempts), - 30000, - ); + + // Enhanced exponential backoff with jitter + const baseDelay = operationName.startsWith('Upload-') ? 2000 : 1000; + const backoffDelay = Math.min(baseDelay * Math.pow(2, attempt), 30000); const jitter = Math.random() * 1000; - const totalDelay = backoffDelay + jitter; - await new Promise(resolve => setTimeout(resolve, totalDelay)); + await new Promise(resolve => + setTimeout(resolve, backoffDelay + jitter), + ); } } - // Final attempt without retry wrapper - return operation(); + return await operation(); } /** * Determines if an S3 operation should be retried based on the error details. */ - private shouldRetryOperation( - error: S3ServiceException, - attemptsRemaining: number, - ): boolean { + private defaultShouldRetry(error: S3ServiceException): boolean { const httpStatusCode = error.$metadata?.httpStatusCode; const errorCode = error.name; // Handle invalid part errors first - these are retriable for multipart uploads if (errorCode === 'InvalidPart') { - return attemptsRemaining > 0; + return true; } // Dont retry for client errors (4xx) except specific cases if (httpStatusCode && httpStatusCode >= 400 && httpStatusCode < 500) { @@ -345,7 +330,7 @@ export class AwsS3Publish implements PublisherBase { 'RequestTimeoutException', 'PriorRequestNotComplete', 'ConnectionError', - 'RequestTimeToooSkewed', + 'RequestTimeTooSkewed', 'InvalidPart', 'NoSuchUpload', ]; @@ -355,7 +340,7 @@ export class AwsS3Publish implements PublisherBase { } // Always retry for server errors (5xx) if (httpStatusCode && httpStatusCode >= 500) { - return attemptsRemaining > 0; + return true; } // Retry specific network/connection errors and multipart upload errors const retriableErrors = [ @@ -380,12 +365,10 @@ export class AwsS3Publish implements PublisherBase { 'IncompleteBody', 'RequestTimeout', ]; - return ( - retriableErrors.some( - retriableError => - errorCode.includes(retriableError) || - error.message.includes(retriableError), - ) && attemptsRemaining > 0 + return retriableErrors.some( + retriableError => + errorCode.includes(retriableError) || + error.message.includes(retriableError), ); } @@ -511,16 +494,12 @@ export class AwsS3Publish implements PublisherBase { new PutObjectCommand(putParams), ); } - // For files 5MB and larger, use multipart upload with enhanced configuration - const calaculatedPartSize = Math.max( - fiveMB, - Math.ceil(fileSizeInBytes / 10000), - ); + // For files larger than MAX_SINGLE_UPLOAD_BYTES, use multipart upload const upload = new Upload({ client: this.storageClient, params, // Configure miltipart upload option for better reliability - partSize: calaculatedPartSize, + partSize: MAX_SINGLE_UPLOAD_BYTES, queueSize: 3, leavePartsOnError: false, }); @@ -536,7 +515,7 @@ export class AwsS3Publish implements PublisherBase { // Check if this is a multipart upload failure that we can handle if ( - fileSizeInBytes >= 5 * 1024 * 1024 && + fileSizeInBytes >= MAX_SINGLE_UPLOAD_BYTES && (errorName === 'InvalidPart' || errorName === 'NoSuchUpload') ) { this.logger.warn( From 39ab1a89358c1ce5759803e55bd3bd2027b65f2f Mon Sep 17 00:00:00 2001 From: Vidhan Shah Date: Sat, 8 Nov 2025 09:04:16 +0530 Subject: [PATCH 05/10] PR comments Signed-off-by: Vidhan Shah --- .../techdocs-node/src/stages/publish/awsS3.ts | 63 +++++++------------ 1 file changed, 22 insertions(+), 41 deletions(-) diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.ts b/plugins/techdocs-node/src/stages/publish/awsS3.ts index 79b4e73028..67e9df59c5 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.ts @@ -318,54 +318,35 @@ export class AwsS3Publish implements PublisherBase { private defaultShouldRetry(error: S3ServiceException): boolean { const httpStatusCode = error.$metadata?.httpStatusCode; const errorCode = error.name; - // Handle invalid part errors first - these are retriable for multipart uploads - if (errorCode === 'InvalidPart') { - return true; - } - // Dont retry for client errors (4xx) except specific cases - if (httpStatusCode && httpStatusCode >= 400 && httpStatusCode < 500) { - // Retry specfic 4xx errors that might be transient - const retriable4xxErrors = [ - 'RequestTimeOut', - 'RequestTimeoutException', - 'PriorRequestNotComplete', - 'ConnectionError', - 'RequestTimeTooSkewed', - 'InvalidPart', - 'NoSuchUpload', - ]; - if (!retriable4xxErrors.includes(errorCode)) { - return false; - } - } - // Always retry for server errors (5xx) - if (httpStatusCode && httpStatusCode >= 500) { - return true; - } - // Retry specific network/connection errors and multipart upload errors - const retriableErrors = [ + + // Truly transient errors that should always be retried + const transientErrors = [ 'NetworkingError', 'TimeoutError', 'ConnectionError', - 'ECONNRESET', - 'ENOTFOUND', - 'ECONNREFUSED', - 'ETIMEDOUT', + 'RequestTimeout', 'ServiceUnavailable', 'SlowDown', - 'Throttling', 'ThrottlingException', - 'ProvisionedThroughputExceededException', - // Multipart upload specific errors - now handled above but kept for completeness - 'InvalidPart', - 'NoSuchUpload', - 'UploadTimeout', - 'EntityTooLarge', - 'InternalError', - 'IncompleteBody', - 'RequestTimeout', ]; - return retriableErrors.some( + + // Server errors are always considered transient + if (httpStatusCode && httpStatusCode >= 500) { + return true; + } + + // Specific 4xx errors that are known to be transient + if (httpStatusCode && httpStatusCode >= 400 && httpStatusCode < 500) { + const retriable4xxErrors = [ + 'RequestTimeout', + 'RequestTimeoutException', + 'PriorRequestNotComplete', + ]; + return retriable4xxErrors.includes(errorCode); + } + + // Check against known transient errors + return transientErrors.some( retriableError => errorCode.includes(retriableError) || error.message.includes(retriableError), From 87da464220fa7d88cf5d56ec181db160474ec864 Mon Sep 17 00:00:00 2001 From: Vidhan Shah Date: Wed, 12 Nov 2025 21:13:34 +0530 Subject: [PATCH 06/10] PR comments Signed-off-by: Vidhan Shah --- .../src/stages/publish/awsS3.test.ts | 74 +++++++++- .../techdocs-node/src/stages/publish/awsS3.ts | 134 ++++++++---------- 2 files changed, 133 insertions(+), 75 deletions(-) diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts index 52c57a9c25..98a2e7d09c 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts @@ -338,7 +338,13 @@ describe('AwsS3Publish', () => { const publisher = (await createPublisherFromConfig()) as AwsS3Publish; s3Mock .on(ListObjectsV2Command) - .rejectsOnce('RequestTimeout') + .rejectsOnce( + new S3ServiceException({ + name: 'RequestTimeout', + $fault: 'client', + $metadata: {}, + }), + ) .resolvesOnce({ Contents: [] }); await (publisher as any).retryOperation( @@ -376,11 +382,17 @@ describe('AwsS3Publish', () => { ); }); - it('should retry on specific 4xx errors', async () => { + it('should retry on specific 4xx errors that are transient', async () => { const publisher = (await createPublisherFromConfig()) as AwsS3Publish; s3Mock .on(ListObjectsV2Command) - .rejectsOnce('RequestTimeout') + .rejectsOnce( + new S3ServiceException({ + name: 'RequestTimeout', + $fault: 'client', + $metadata: { httpStatusCode: 408 }, + }), + ) .resolvesOnce({ Contents: [] }); await (publisher as any).retryOperation( @@ -416,6 +428,62 @@ describe('AwsS3Publish', () => { ), ).rejects.toHaveProperty('name', 'BadRequest'); }); + + it('should use exact error code matching for transient errors', async () => { + const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + // Test that ConnectionError (exact match) is retried, but ConnectionErrorSomething (substring) is not + s3Mock + .on(ListObjectsV2Command) + .rejectsOnce( + new S3ServiceException({ + name: 'ConnectionError', + $fault: 'client', + $metadata: {}, + }), + ) + .resolvesOnce({ Contents: [] }); + + await (publisher as any).retryOperation( + async () => { + return s3Mock.send( + new ListObjectsV2Command({ Bucket: 'bucketName' }), + ); + }, + 'TestOperation', + 3, + ); + }); + + it('should apply exponential backoff with correct calculation', async () => { + const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + const startTime = Date.now(); + + s3Mock + .on(ListObjectsV2Command) + .rejectsOnce( + new S3ServiceException({ + name: 'SlowDown', + $fault: 'server', + $metadata: {}, + }), + ) + .resolvesOnce({ Contents: [] }); + + await (publisher as any).retryOperation( + async () => { + return s3Mock.send( + new ListObjectsV2Command({ Bucket: 'bucketName' }), + ); + }, + 'TestOperation', + 2, + ); + + const elapsedTime = Date.now() - startTime; + // First attempt fails, then backoff with baseDelay * 2^(attempt-1) = 1000 * 2^0 = 1000ms minimum + // Adding jitter (0-1000ms), so we expect at least 1000ms total + expect(elapsedTime).toBeGreaterThanOrEqual(900); + }); }); describe('getReadiness', () => { diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.ts b/plugins/techdocs-node/src/stages/publish/awsS3.ts index 67e9df59c5..76068ad935 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.ts @@ -187,20 +187,13 @@ export class AwsS3Publish implements PublisherBase { // Enhanced retry configuration for better reliability maxAttempts: maxAttempts || 5, retryMode: 'adaptive', - ...(httpsProxy && { - requestHandler: new NodeHttpHandler({ + // Enhanced connection settings for large file uploads + requestHandler: new NodeHttpHandler({ + ...(httpsProxy && { httpsAgent: new HttpsProxyAgent({ proxy: httpsProxy }), - // Enhanced connection setting for large file uploads - connectionTimeout: 60000, - socketTimeout: 120000, - }), - }), - // Add default request handler with enhanced timeouts if no proxy - ...(!httpsProxy && { - requestHandler: new NodeHttpHandler({ - connectionTimeout: 60000, - socketTimeout: 120000, }), + connectionTimeout: 60000, + socketTimeout: 120000, }), }); @@ -279,8 +272,9 @@ export class AwsS3Publish implements PublisherBase { operation: () => Promise, operationName: string, maxAttempts: number = 3, - shouldRetry: (error: S3ServiceException) => boolean = this - .defaultShouldRetry, + shouldRetry: ( + error: S3ServiceException, + ) => boolean = this.defaultShouldRetry.bind(this), ): Promise { for (let attempt = 1; attempt < maxAttempts; attempt++) { try { @@ -302,7 +296,10 @@ export class AwsS3Publish implements PublisherBase { // Enhanced exponential backoff with jitter const baseDelay = operationName.startsWith('Upload-') ? 2000 : 1000; - const backoffDelay = Math.min(baseDelay * Math.pow(2, attempt), 30000); + const backoffDelay = Math.min( + baseDelay * Math.pow(2, attempt - 1), + 30000, + ); const jitter = Math.random() * 1000; await new Promise(resolve => setTimeout(resolve, backoffDelay + jitter), @@ -348,8 +345,7 @@ export class AwsS3Publish implements PublisherBase { // Check against known transient errors return transientErrors.some( retriableError => - errorCode.includes(retriableError) || - error.message.includes(retriableError), + errorCode === retriableError || error.message.includes(retriableError), ); } @@ -462,68 +458,62 @@ export class AwsS3Publish implements PublisherBase { const stats = await fs.stat(absoluteFilePath); const fileSizeInBytes = stats.size; - // Use retry wrapper for uploads with enhanced error handling + // Check if this is a large file that requires multipart upload + if (fileSizeInBytes >= MAX_SINGLE_UPLOAD_BYTES) { + // Try multipart upload for large files + try { + const upload = new Upload({ + client: this.storageClient, + params, + partSize: MAX_SINGLE_UPLOAD_BYTES, + queueSize: 3, + leavePartsOnError: false, + }); + await this.retryOperation( + () => upload.done(), + `Upload-${params.Key}`, + this.maxAttempts, + ); + return; + } catch (multipartError) { + const s3Error = multipartError as any; + const errorName = s3Error?.name || 'Unknown'; + + // For specific multipart errors, attempt simple upload fallback + if (errorName === 'InvalidPart' || errorName === 'NoSuchUpload') { + this.logger.warn( + `Multipart upload failed for ${params.Key}, attempting simple upload fallback.`, + ); + } else { + // Non-recoverable multipart error, throw it + this.logger.error( + `Multipart upload failed for ${params.Key}: ${ + multipartError instanceof Error + ? multipartError.message + : String(multipartError) + }`, + ); + throw multipartError; + } + } + } + + // Use simple upload for small files or as fallback from multipart try { - const result = await this.retryOperation( - async () => { - const fiveMB = 5 * 1024 * 1024; - // For files smaller than 5MB, use simple PutObject to avoid multipart complexity - if (fileSizeInBytes < fiveMB) { - const fileContent = await fs.readFile(absoluteFilePath); - const putParams = { ...params, Body: fileContent }; - return this.storageClient.send( - new PutObjectCommand(putParams), - ); - } - // For files larger than MAX_SINGLE_UPLOAD_BYTES, use multipart upload - const upload = new Upload({ - client: this.storageClient, - params, - // Configure miltipart upload option for better reliability - partSize: MAX_SINGLE_UPLOAD_BYTES, - queueSize: 3, - leavePartsOnError: false, - }); - return upload.done(); - }, + const fileContent = await fs.readFile(absoluteFilePath); + const putParams = { ...params, Body: fileContent }; + await this.retryOperation( + () => this.storageClient.send(new PutObjectCommand(putParams)), `Upload-${params.Key}`, this.maxAttempts, ); - return result; - } catch (error) { - const s3Error = error as any; - const errorName = s3Error?.name || 'Unknown'; - // Check if this is a multipart upload failure that we can handle - if ( - fileSizeInBytes >= MAX_SINGLE_UPLOAD_BYTES && - (errorName === 'InvalidPart' || errorName === 'NoSuchUpload') - ) { - this.logger.warn( - `Multipart upload failed for ${params.Key}, Attempting simple upload fallback.`, + if (fileSizeInBytes >= MAX_SINGLE_UPLOAD_BYTES) { + this.logger.info( + `Simple upload fallback succeeded for ${params.Key}`, ); - try { - // Attempt simple upload as a fallback - const fileContent = await fs.readFile(absoluteFilePath); - const simpleParams = { ...params, Body: fileContent }; - const fallbackResult = await this.storageClient.send( - new PutObjectCommand(simpleParams), - ); - this.logger.info( - `Simple upload fallback succeeded for ${params.Key}`, - ); - return fallbackResult; - } catch (fallbackError) { - this.logger.error( - `Both multipart and simple upload failed for ${params.Key}: ${ - fallbackError instanceof Error - ? fallbackError.message - : String(fallbackError) - }`, - ); - // Fall through to throw original error - } } + } catch (error) { this.logger.error( `Upload failed for ${params.Key}: ${ error instanceof Error ? error.message : String(error) From e6314f25348c755ac4f21502bf4f9f577b66190c Mon Sep 17 00:00:00 2001 From: Vidhan Shah Date: Fri, 21 Nov 2025 20:54:32 +0530 Subject: [PATCH 07/10] PR comments Signed-off-by: Vidhan Shah --- .../src/stages/publish/awsS3.test.ts | 40 ++++++++++--------- .../techdocs-node/src/stages/publish/awsS3.ts | 35 ++++++++++------ 2 files changed, 44 insertions(+), 31 deletions(-) diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts index 98a2e7d09c..c7c859f331 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts @@ -38,6 +38,8 @@ import request from 'supertest'; import path from 'path'; import fs from 'fs-extra'; import { AwsS3Publish } from './awsS3'; + +jest.setTimeout(30_000); import { Readable } from 'stream'; import { createMockDirectory, @@ -45,7 +47,9 @@ import { } from '@backstage/backend-test-utils'; const env = process.env; -let s3Mock: any; +let s3Mock: ReturnType & { + send: (command: any) => Promise; +}; // Create a new MockDirectory for each test to avoid Windows file locking issues let mockDir: ReturnType; @@ -514,7 +518,7 @@ describe('AwsS3Publish', () => { `default/component/backstage/assets/main.css`, ]), }); - }, 30000); + }); it('should publish a directory as well when legacy casing is used', async () => { const publisher = await createPublisherFromConfig({ @@ -527,7 +531,7 @@ describe('AwsS3Publish', () => { `default/Component/backstage/assets/main.css`, ]), }); - }, 30000); + }); it('should publish a directory when root path is specified', async () => { const publisher = await createPublisherFromConfig({ @@ -540,7 +544,7 @@ describe('AwsS3Publish', () => { `backstage-data/techdocs/default/component/backstage/assets/main.css`, ]), }); - }, 30000); + }); it('should publish a directory when root path is specified and legacy casing is used', async () => { const publisher = await createPublisherFromConfig({ @@ -554,7 +558,7 @@ describe('AwsS3Publish', () => { `backstage-data/techdocs/default/Component/backstage/assets/main.css`, ]), }); - }, 30000); + }); it('should publish a directory when sse is specified', async () => { const publisher = await createPublisherFromConfig({ @@ -567,7 +571,7 @@ describe('AwsS3Publish', () => { 'default/component/backstage/assets/main.css', ]), }); - }, 30000); + }); it('should fail to publish a directory', async () => { const wrongPathToGeneratedDirectory = mockDir.resolve( @@ -602,7 +606,7 @@ describe('AwsS3Publish', () => { expect(loggerInfoSpy).toHaveBeenCalledWith( `Successfully deleted stale files for Entity ${entity.metadata.name}. Total number of files: 1`, ); - }, 30000); + }); it('should log error when the stale files deletion fails', async () => { const bucketName = 'delete_stale_files_error'; @@ -613,7 +617,7 @@ describe('AwsS3Publish', () => { expect(loggerErrorSpy).toHaveBeenLastCalledWith( 'Unable to delete file(s) from AWS S3. Error: Message', ); - }, 30000); + }); }); describe('hasDocsBeenGenerated', () => { @@ -621,7 +625,7 @@ describe('AwsS3Publish', () => { const publisher = await createPublisherFromConfig(); await publisher.publish({ entity, directory }); expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true); - }, 30000); + }); it('should return true if docs has been generated even if the legacy case is enabled', async () => { const publisher = await createPublisherFromConfig({ @@ -629,7 +633,7 @@ describe('AwsS3Publish', () => { }); await publisher.publish({ entity, directory }); expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true); - }, 30000); + }); it('should return true if docs has been generated if root path is specified', async () => { const publisher = await createPublisherFromConfig({ @@ -637,7 +641,7 @@ describe('AwsS3Publish', () => { }); await publisher.publish({ entity, directory }); expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true); - }, 30000); + }); it('should return true if docs has been generated if root path is specified and legacy casing is used', async () => { const publisher = await createPublisherFromConfig({ @@ -646,7 +650,7 @@ describe('AwsS3Publish', () => { }); await publisher.publish({ entity, directory }); expect(await publisher.hasDocsBeenGenerated(entity)).toBe(true); - }, 30000); + }); it('should return false if docs has not been generated', async () => { const publisher = await createPublisherFromConfig(); @@ -669,7 +673,7 @@ describe('AwsS3Publish', () => { expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual( techdocsMetadata, ); - }, 30000); + }); it('should return tech docs metadata even if the legacy case is enabled', async () => { const publisher = await createPublisherFromConfig({ @@ -679,7 +683,7 @@ describe('AwsS3Publish', () => { expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual( techdocsMetadata, ); - }, 30000); + }); it('should return tech docs metadata even if root path is specified', async () => { const publisher = await createPublisherFromConfig({ @@ -689,7 +693,7 @@ describe('AwsS3Publish', () => { expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual( techdocsMetadata, ); - }, 30000); + }); it('should return tech docs metadata if root path is specified and legacy casing is used', async () => { const publisher = await createPublisherFromConfig({ @@ -700,7 +704,7 @@ describe('AwsS3Publish', () => { expect(await publisher.fetchTechDocsMetadata(entityName)).toStrictEqual( techdocsMetadata, ); - }, 30000); + }); it('should return tech docs metadata when json encoded with single quotes', async () => { const techdocsMetadataPath = path.join( @@ -722,7 +726,7 @@ describe('AwsS3Publish', () => { ); fs.writeFileSync(techdocsMetadataPath, techdocsMetadataContent); - }, 30000); + }); it('should return an error if the techdocs_metadata.json file is not present', async () => { const publisher = await createPublisherFromConfig(); @@ -776,7 +780,7 @@ describe('AwsS3Publish', () => { const publisher = await createPublisherFromConfig(); await publisher.publish({ entity, directory }); app = express().use(publisher.docsRouter()); - }, 30000); + }); it('should pass expected object path to bucket', async () => { // Ensures leading slash is trimmed and encoded path is decoded. diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.ts b/plugins/techdocs-node/src/stages/publish/awsS3.ts index 76068ad935..0e84a433f6 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.ts @@ -173,7 +173,7 @@ export class AwsS3Publish implements PublisherBase { 'techdocs.publisher.awsS3.s3ForcePathStyle', ); - // AWS MAX ATTEMPTS is an optional config. If missing, default value of 3 is used + // AWS MAX ATTEMPTS is an optional config. If missing, default value of 5 is used const maxAttempts = config.getOptionalNumber( 'techdocs.publisher.awsS3.maxAttempts', ); @@ -434,11 +434,8 @@ export class AwsS3Publish implements PublisherBase { // e.g. ['index.html', 'sub-page/index.html', 'assets/images/favicon.png'] absoluteFilesToUpload = await getFileTreeRecursively(directory); - let uploadCounter = 0; - await bulkStorageOperation( async absoluteFilePath => { - uploadCounter++; const relativeFilePath = path.relative(directory, absoluteFilePath); const s3Key = getCloudPathForLocalPath( entity, @@ -446,10 +443,14 @@ export class AwsS3Publish implements PublisherBase { useLegacyPathCasing, bucketRootPath, ); + // Create params without the Body because the body must be the + // actual file contents (Buffer or Readable), not the path string. + // For multipart uploads we attach a Readable stream to avoid + // buffering large files in memory. For simple uploads we attach + // a Buffer read from disk. const params: PutObjectCommandInput = { Bucket: this.bucketName, Key: s3Key, - Body: absoluteFilePath, ...(sse && { ServerSideEncryption: sse }), }; @@ -462,15 +463,23 @@ export class AwsS3Publish implements PublisherBase { if (fileSizeInBytes >= MAX_SINGLE_UPLOAD_BYTES) { // Try multipart upload for large files try { - const upload = new Upload({ - client: this.storageClient, - params, - partSize: MAX_SINGLE_UPLOAD_BYTES, - queueSize: 3, - leavePartsOnError: false, - }); + // Create stream and Upload inside retry closure so stream is + // recreated on each retry attempt (streams are consumable once). await this.retryOperation( - () => upload.done(), + () => { + // Create a fresh stream on each attempt + const fileStream = fs.createReadStream(absoluteFilePath); + const uploadParams = { ...params, Body: fileStream }; + + const upload = new Upload({ + client: this.storageClient, + params: uploadParams, + partSize: MAX_SINGLE_UPLOAD_BYTES, + queueSize: 3, + leavePartsOnError: false, + }); + return upload.done(); + }, `Upload-${params.Key}`, this.maxAttempts, ); From 0f258e24fa53dd2649dd4c04efcb047a96f5e1fd Mon Sep 17 00:00:00 2001 From: Vidhan Shah Date: Fri, 21 Nov 2025 21:17:50 +0530 Subject: [PATCH 08/10] PR comments Signed-off-by: Vidhan Shah --- .../src/stages/publish/awsS3.test.ts | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts index c7c859f331..35da7485b0 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts @@ -32,24 +32,22 @@ import { AwsCredentialProviderOptions, DefaultAwsCredentialsManager, } from '@backstage/integration-aws-node'; -import { mockClient } from 'aws-sdk-client-mock'; +import { mockClient, AwsClientStub } from 'aws-sdk-client-mock'; import express from 'express'; import request from 'supertest'; import path from 'path'; import fs from 'fs-extra'; import { AwsS3Publish } from './awsS3'; - -jest.setTimeout(30_000); import { Readable } from 'stream'; import { createMockDirectory, mockServices, } from '@backstage/backend-test-utils'; +jest.setTimeout(30_000); + const env = process.env; -let s3Mock: ReturnType & { - send: (command: any) => Promise; -}; +let s3Mock: AwsClientStub; // Create a new MockDirectory for each test to avoid Windows file locking issues let mockDir: ReturnType; @@ -326,7 +324,7 @@ describe('AwsS3Publish', () => { await (publisher as any).retryOperation( async () => { - return s3Mock.send( + return (s3Mock.send as any)( new ListObjectsV2Command({ Bucket: 'bucketName' }), ); }, @@ -353,7 +351,7 @@ describe('AwsS3Publish', () => { await (publisher as any).retryOperation( async () => { - return s3Mock.send( + return (s3Mock.send as any)( new ListObjectsV2Command({ Bucket: 'bucketName' }), ); }, @@ -377,7 +375,7 @@ describe('AwsS3Publish', () => { await (publisher as any).retryOperation( async () => { - return s3Mock.send( + return (s3Mock.send as any)( new ListObjectsV2Command({ Bucket: 'bucketName' }), ); }, @@ -401,7 +399,7 @@ describe('AwsS3Publish', () => { await (publisher as any).retryOperation( async () => { - return s3Mock.send( + return (s3Mock.send as any)( new ListObjectsV2Command({ Bucket: 'bucketName' }), ); }, @@ -423,7 +421,7 @@ describe('AwsS3Publish', () => { await expect( (publisher as any).retryOperation( async () => { - return s3Mock.send( + return (s3Mock.send as any)( new ListObjectsV2Command({ Bucket: 'bucketName' }), ); }, @@ -449,7 +447,7 @@ describe('AwsS3Publish', () => { await (publisher as any).retryOperation( async () => { - return s3Mock.send( + return (s3Mock.send as any)( new ListObjectsV2Command({ Bucket: 'bucketName' }), ); }, @@ -475,7 +473,7 @@ describe('AwsS3Publish', () => { await (publisher as any).retryOperation( async () => { - return s3Mock.send( + return (s3Mock.send as any)( new ListObjectsV2Command({ Bucket: 'bucketName' }), ); }, From e1152729555af6f64bb075a41c4c1122254222e6 Mon Sep 17 00:00:00 2001 From: Vidhan Shah Date: Fri, 21 Nov 2025 21:36:41 +0530 Subject: [PATCH 09/10] PR comments Signed-off-by: Vidhan Shah --- .../src/stages/publish/awsS3.test.ts | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts index 35da7485b0..e33c5c2612 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts @@ -201,14 +201,14 @@ describe('AwsS3Publish', () => { s3Mock = mockClient(S3Client); - s3Mock.on(HeadObjectCommand).callsFake((input: { Key: string }) => { + s3Mock.on(HeadObjectCommand).callsFake(input => { if (!fs.pathExistsSync(mockDir.resolve(input.Key))) { throw new Error('File does not exist'); } return {}; }); - s3Mock.on(GetObjectCommand).callsFake((input: { Key: string }) => { + s3Mock.on(GetObjectCommand).callsFake(input => { if (fs.pathExistsSync(mockDir.resolve(input.Key))) { return { Body: Readable.from(fs.readFileSync(mockDir.resolve(input.Key))), @@ -218,14 +218,14 @@ describe('AwsS3Publish', () => { throw new Error(`The file ${input.Key} does not exist!`); }); - s3Mock.on(HeadBucketCommand).callsFake((input: { Bucket: string }) => { + s3Mock.on(HeadBucketCommand).callsFake(input => { if (input.Bucket === 'errorBucket') { throw new Error('Bucket does not exist'); } return {}; }); - s3Mock.on(ListObjectsV2Command).callsFake((input: { Bucket: string }) => { + s3Mock.on(ListObjectsV2Command).callsFake(input => { if ( input.Bucket === 'delete_stale_files_success' || input.Bucket === 'delete_stale_files_error' @@ -237,7 +237,7 @@ describe('AwsS3Publish', () => { return {}; }); - s3Mock.on(DeleteObjectCommand).callsFake((input: { Bucket: string }) => { + s3Mock.on(DeleteObjectCommand).callsFake(input => { if (input.Bucket === 'delete_stale_files_error') { throw new Error('Message'); } @@ -245,11 +245,9 @@ describe('AwsS3Publish', () => { }); s3Mock.on(UploadPartCommand).rejects(); - s3Mock - .on(PutObjectCommand) - .callsFake((input: { Key: string; Body: any }) => { - mockDir.addContent({ [input.Key]: input.Body }); - }); + s3Mock.on(PutObjectCommand).callsFake(input => { + mockDir.addContent({ [input.Key]: input.Body }); + }); }); afterEach(() => { @@ -306,7 +304,7 @@ describe('AwsS3Publish', () => { describe('retry mechanism', () => { it('should retry with custom retry strategy', async () => { - const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + const publisher = await createPublisherFromConfig(); const customRetryStrategy = jest.fn((error: any) => { return error.name === 'NetworkingError'; }); @@ -337,7 +335,7 @@ describe('AwsS3Publish', () => { }); it('should use default retry strategy when no custom strategy provided', async () => { - const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + const publisher = await createPublisherFromConfig(); s3Mock .on(ListObjectsV2Command) .rejectsOnce( @@ -361,7 +359,7 @@ describe('AwsS3Publish', () => { }); it('should retry on server errors (5xx)', async () => { - const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + const publisher = await createPublisherFromConfig(); s3Mock .on(ListObjectsV2Command) .rejectsOnce( @@ -385,7 +383,7 @@ describe('AwsS3Publish', () => { }); it('should retry on specific 4xx errors that are transient', async () => { - const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + const publisher = await createPublisherFromConfig(); s3Mock .on(ListObjectsV2Command) .rejectsOnce( @@ -409,7 +407,7 @@ describe('AwsS3Publish', () => { }); it('should not retry on non-retriable 4xx errors', async () => { - const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + const publisher = await createPublisherFromConfig(); s3Mock.on(ListObjectsV2Command).rejectsOnce( new S3ServiceException({ name: 'BadRequest', @@ -432,7 +430,7 @@ describe('AwsS3Publish', () => { }); it('should use exact error code matching for transient errors', async () => { - const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + const publisher = await createPublisherFromConfig(); // Test that ConnectionError (exact match) is retried, but ConnectionErrorSomething (substring) is not s3Mock .on(ListObjectsV2Command) @@ -457,7 +455,7 @@ describe('AwsS3Publish', () => { }); it('should apply exponential backoff with correct calculation', async () => { - const publisher = (await createPublisherFromConfig()) as AwsS3Publish; + const publisher = await createPublisherFromConfig(); const startTime = Date.now(); s3Mock From 848335461ea32f19f33b48cf1aa464204560da14 Mon Sep 17 00:00:00 2001 From: Vidhan Shah Date: Mon, 24 Nov 2025 16:42:38 +0530 Subject: [PATCH 10/10] PR comments Signed-off-by: Vidhan Shah --- .../src/stages/publish/awsS3.test.ts | 73 ++++++------------- .../techdocs-node/src/stages/publish/awsS3.ts | 4 +- 2 files changed, 24 insertions(+), 53 deletions(-) diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts index e33c5c2612..8be8c05994 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.test.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.test.ts @@ -305,7 +305,7 @@ describe('AwsS3Publish', () => { describe('retry mechanism', () => { it('should retry with custom retry strategy', async () => { const publisher = await createPublisherFromConfig(); - const customRetryStrategy = jest.fn((error: any) => { + const customRetryStrategy = jest.fn(error => { return error.name === 'NetworkingError'; }); @@ -320,11 +320,10 @@ describe('AwsS3Publish', () => { ) .resolvesOnce({ Contents: [] }); - await (publisher as any).retryOperation( + await (publisher as AwsS3Publish).retryOperation( async () => { - return (s3Mock.send as any)( - new ListObjectsV2Command({ Bucket: 'bucketName' }), - ); + const command = new ListObjectsV2Command({ Bucket: 'bucketName' }); + return (publisher as AwsS3Publish).storageClient.send(command); }, 'TestOperation', 3, @@ -347,11 +346,10 @@ describe('AwsS3Publish', () => { ) .resolvesOnce({ Contents: [] }); - await (publisher as any).retryOperation( + await (publisher as AwsS3Publish).retryOperation( async () => { - return (s3Mock.send as any)( - new ListObjectsV2Command({ Bucket: 'bucketName' }), - ); + const command = new ListObjectsV2Command({ Bucket: 'bucketName' }); + return (publisher as AwsS3Publish).storageClient.send(command); }, 'TestOperation', 3, @@ -371,11 +369,10 @@ describe('AwsS3Publish', () => { ) .resolvesOnce({ Contents: [] }); - await (publisher as any).retryOperation( + await (publisher as AwsS3Publish).retryOperation( async () => { - return (s3Mock.send as any)( - new ListObjectsV2Command({ Bucket: 'bucketName' }), - ); + const command = new ListObjectsV2Command({ Bucket: 'bucketName' }); + return (publisher as AwsS3Publish).storageClient.send(command); }, 'TestOperation', 3, @@ -395,40 +392,16 @@ describe('AwsS3Publish', () => { ) .resolvesOnce({ Contents: [] }); - await (publisher as any).retryOperation( + await (publisher as AwsS3Publish).retryOperation( async () => { - return (s3Mock.send as any)( - new ListObjectsV2Command({ Bucket: 'bucketName' }), - ); + const command = new ListObjectsV2Command({ Bucket: 'bucketName' }); + return (publisher as AwsS3Publish).storageClient.send(command); }, 'TestOperation', 3, ); }); - it('should not retry on non-retriable 4xx errors', async () => { - const publisher = await createPublisherFromConfig(); - s3Mock.on(ListObjectsV2Command).rejectsOnce( - new S3ServiceException({ - name: 'BadRequest', - $fault: 'client', - $metadata: { httpStatusCode: 400 }, - }), - ); - - await expect( - (publisher as any).retryOperation( - async () => { - return (s3Mock.send as any)( - new ListObjectsV2Command({ Bucket: 'bucketName' }), - ); - }, - 'TestOperation', - 3, - ), - ).rejects.toHaveProperty('name', 'BadRequest'); - }); - it('should use exact error code matching for transient errors', async () => { const publisher = await createPublisherFromConfig(); // Test that ConnectionError (exact match) is retried, but ConnectionErrorSomething (substring) is not @@ -443,11 +416,10 @@ describe('AwsS3Publish', () => { ) .resolvesOnce({ Contents: [] }); - await (publisher as any).retryOperation( + await (publisher as AwsS3Publish).retryOperation( async () => { - return (s3Mock.send as any)( - new ListObjectsV2Command({ Bucket: 'bucketName' }), - ); + const command = new ListObjectsV2Command({ Bucket: 'bucketName' }); + return (publisher as AwsS3Publish).storageClient.send(command); }, 'TestOperation', 3, @@ -469,14 +441,13 @@ describe('AwsS3Publish', () => { ) .resolvesOnce({ Contents: [] }); - await (publisher as any).retryOperation( + await (publisher as AwsS3Publish).retryOperation( async () => { - return (s3Mock.send as any)( - new ListObjectsV2Command({ Bucket: 'bucketName' }), - ); + const command = new ListObjectsV2Command({ Bucket: 'bucketName' }); + return (publisher as AwsS3Publish).storageClient.send(command); }, 'TestOperation', - 2, + 3, ); const elapsedTime = Date.now() - startTime; @@ -743,7 +714,7 @@ describe('AwsS3Publish', () => { }); it('should return an error if the techdocs_metadata.json file cannot be read from stream', async () => { - s3Mock.on(GetObjectCommand).callsFake((_: any) => { + s3Mock.on(GetObjectCommand).callsFake(() => { return { Body: new ErrorReadable('No stream!'), }; @@ -882,7 +853,7 @@ describe('AwsS3Publish', () => { }); it('should return 404 if file cannot be read from stream', async () => { - s3Mock.on(GetObjectCommand).callsFake((_: any) => { + s3Mock.on(GetObjectCommand).callsFake(() => { return { Body: new ErrorReadable('No stream!'), }; diff --git a/plugins/techdocs-node/src/stages/publish/awsS3.ts b/plugins/techdocs-node/src/stages/publish/awsS3.ts index 0e84a433f6..157c2ffdeb 100644 --- a/plugins/techdocs-node/src/stages/publish/awsS3.ts +++ b/plugins/techdocs-node/src/stages/publish/awsS3.ts @@ -84,7 +84,7 @@ const streamToBuffer = (stream: Readable): Promise => { }; export class AwsS3Publish implements PublisherBase { - private readonly storageClient: S3Client; + public readonly storageClient: S3Client; private readonly bucketName: string; private readonly legacyPathCasing: boolean; private readonly logger: LoggerService; @@ -268,7 +268,7 @@ export class AwsS3Publish implements PublisherBase { /** * Custom retry wrapper for S3 operations with detailed error handling. */ - private async retryOperation( + public async retryOperation( operation: () => Promise, operationName: string, maxAttempts: number = 3,