PR comments

Signed-off-by: Vidhan Shah <vidhans958@gmail.com>
This commit is contained in:
Vidhan Shah
2025-11-12 21:13:34 +05:30
parent 39ab1a8935
commit 87da464220
2 changed files with 133 additions and 75 deletions
@@ -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', () => {
@@ -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<TOutput>,
operationName: string,
maxAttempts: number = 3,
shouldRetry: (error: S3ServiceException) => boolean = this
.defaultShouldRetry,
shouldRetry: (
error: S3ServiceException,
) => boolean = this.defaultShouldRetry.bind(this),
): Promise<TOutput> {
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)