fix(virus-scanner): stop retrying 0-byte S3 objects and clarify logs

S3 has had strong read-after-write consistency since 2020, so a HeadObject
that reports ContentLength === 0 can never recover on retry. The previous
code retried 0-byte objects three times (wasted invocations) and logged the
failure as "Body is empty", which was misleading — the check is on
HeadObject.ContentLength, not a downloaded body.

s3.service.ts:
- Throw a typed ZeroByteS3ObjectError ("S3 object is 0 bytes") for the 0-byte
  case and make it non-retryable via retryIf, so it short-circuits after one
  attempt. 404 and missing-VersionId still retry as before
  (MissingS3VersionIdError is now typed but remains retryable).
- Unwrap ts-retry-promise's RetryError to its original cause before rethrowing
  (duck-typed, since the package's src/dist entry points make instanceof
  unsafe under ts-jest) so callers can branch on the typed error.
- Drop the misleading "after retries" suffix from the failure log, since the
  0-byte path no longer retries.

index.ts:
- Split the collapsed 404 warn into three distinguishable log lines with a
  structured `reason` field: not_found, missing_version_id, zero_byte. The
  HTTP response stays 404 with the same body, preserving caller behaviour.

Tests cover the 0-byte short-circuit (one attempt, no retry), the
missing-VersionId retry-then-fail path (four attempts, unchanged), and the
three distinct handler warn logs.

Closes #9481

Author: mvincentong

services/virus-scanner-guardduty/src/__tests/index.spec.ts

@@ -7,7 +7,11 @@ import { validate } from 'uuid'
 
 import { handler } from '../index'
 import * as LoggerService from '../logger'
-import { S3Service } from '../s3.service'
+import {
+  MissingS3VersionIdError,
+  S3Service,
+  ZeroByteS3ObjectError,
+} from '../s3.service'
 
 // mocked S3Service
 const MockS3Service = jest.mocked(S3Service)
@@ -72,14 +76,14 @@ describe('handler', () => {
     )
   })
 
-  it('should return 404 if versionId not found', async () => {
+  it('should return 404 and log missing_version_id if the object has no version id', async () => {
     // Arrange
     const mockUUID = crypto.randomUUID()
     const mockEvent = { key: mockUUID }
 
     MockS3Service.prototype.getS3ObjectVersionId = jest
       .fn()
-      .mockRejectedValueOnce(new Error('VersionId is empty'))
+      .mockRejectedValueOnce(new MissingS3VersionIdError())
 
     // Act
     const result = await handler(mockEvent as unknown as APIGatewayProxyEvent)
@@ -95,21 +99,26 @@ describe('handler', () => {
     expect(mockLoggerWarn).toHaveBeenCalledTimes(1)
     expect(mockLoggerWarn).toHaveBeenCalledWith(
       expect.objectContaining({
-        message: 'File not found or its content is empty',
-        err: new Error('VersionId is empty'),
+        message: 'S3 object is missing a version id',
+        reason: 'missing_version_id',
+        err: expect.any(MissingS3VersionIdError),
         quarantineFileKey: mockUUID,
       }),
     )
   })
 
-  it('should return 404 if body content length is empty', async () => {
+  it('should return 404 and log not_found if the object is missing', async () => {
     // Arrange
     const mockUUID = crypto.randomUUID()
     const mockEvent = { key: mockUUID }
 
+    const notFoundError = Object.assign(new Error('Not Found'), {
+      name: 'NotFound',
+      $metadata: { httpStatusCode: 404 },
+    })
     MockS3Service.prototype.getS3ObjectVersionId = jest
       .fn()
-      .mockRejectedValueOnce(new Error('Body is empty'))
+      .mockRejectedValueOnce(notFoundError)
 
     // Act
     const result = await handler(mockEvent as unknown as APIGatewayProxyEvent)
@@ -125,8 +134,40 @@ describe('handler', () => {
     expect(mockLoggerWarn).toHaveBeenCalledTimes(1)
     expect(mockLoggerWarn).toHaveBeenCalledWith(
       expect.objectContaining({
+        message: 'File not found in s3',
+        reason: 'not_found',
+        err: notFoundError,
+        quarantineFileKey: mockUUID,
+      }),
+    )
+  })
+
+  it('should return 404 and log zero_byte if the object is 0 bytes', async () => {
+    // Arrange
+    const mockUUID = crypto.randomUUID()
+    const mockEvent = { key: mockUUID }
+
+    MockS3Service.prototype.getS3ObjectVersionId = jest
+      .fn()
+      .mockRejectedValueOnce(new ZeroByteS3ObjectError())
+
+    // Act
+    const result = await handler(mockEvent as unknown as APIGatewayProxyEvent)
+
+    // Assert
+    expect(result.statusCode).toBe(StatusCodes.NOT_FOUND)
+    expect(result.body).toBe(
+      JSON.stringify({
         message: 'File not found or its content is empty',
-        err: new Error('Body is empty'),
+        fileKey: mockUUID,
+      }),
+    )
+    expect(mockLoggerWarn).toHaveBeenCalledTimes(1)
+    expect(mockLoggerWarn).toHaveBeenCalledWith(
+      expect.objectContaining({
+        message: 'S3 object is 0 bytes',
+        reason: 'zero_byte',
+        err: expect.any(ZeroByteS3ObjectError),
         quarantineFileKey: mockUUID,
       }),
     )

services/virus-scanner-guardduty/src/__tests/s3.service.spec.ts

@@ -3,7 +3,11 @@
 import { CopyObjectCommand, DeleteObjectCommand } from '@aws-sdk/client-s3'
 
 import * as LoggerService from '../logger'
-import { S3Service } from '../s3.service'
+import {
+  MissingS3VersionIdError,
+  S3Service,
+  ZeroByteS3ObjectError,
+} from '../s3.service'
 
 const VersionId = 'mockObjectVersionId'
 // Mock S3Client
@@ -107,6 +111,16 @@ describe('S3Service', () => {
     })
   })
   describe('getS3ObjectVersionId', () => {
+    beforeEach(() => {
+      // The real retry backoff (2s/4s/8s/16s) would make these tests slow; fake
+      // timers let the retry waits resolve instantly. We advance up to 40s,
+      // which stays under the retry wrapper's 60s timeout.
+      jest.useFakeTimers()
+    })
+    afterEach(() => {
+      jest.useRealTimers()
+    })
+
     it('should return version id', async () => {
       // Arrange
       const mockS3Service = new S3Service(true, mockLogger)
@@ -121,55 +135,80 @@ describe('S3Service', () => {
       expect(versionIdResult).toEqual('mockObjectVersionId')
     })
 
-    it('should throw error and log if body is empty', async () => {
+    it('should short-circuit without retrying when the object is 0 bytes', async () => {
       // Arrange
       const mockS3Service = new S3Service(true, mockLogger)
       getResult = {
         ContentLength: 0,
         VersionId: 'mockObjectVersionId',
       }
 
-      // Act + assert
+      // Act + assert. A 0-byte HeadObject is non-retryable, so no retry waits
+      // are scheduled and the promise settles immediately.
       await expect(
         mockS3Service.getS3ObjectVersionId({
           bucketName: 'bucketName',
           objectKey: 'objectKey',
         }),
-      ).rejects.toThrow('Body is empty')
+      ).rejects.toThrow(ZeroByteS3ObjectError)
 
+      // Only one attempt ran — proving the retry loop was short-circuited.
+      expect(mockLoggerWarn).toHaveBeenCalledTimes(1)
+      expect(mockLoggerWarn).toHaveBeenCalledWith(
+        expect.objectContaining({
+          meta: expect.objectContaining({ attempt: 1, status: 'missed' }),
+          err: expect.any(ZeroByteS3ObjectError),
+        }),
+        'getS3ObjectVersionId attempt failed',
+      )
       expect(mockLoggerError).toHaveBeenCalledTimes(1)
       expect(mockLoggerError).toHaveBeenCalledWith(
         expect.objectContaining({
-          bucketName: 'bucketName',
-          objectKey: 'objectKey',
-          err: new Error('Body is empty'),
+          meta: expect.objectContaining({
+            bucketName: 'bucketName',
+            objectKey: 'objectKey',
+            status: 'failed',
+            attempts: 1,
+          }),
+          err: expect.any(ZeroByteS3ObjectError),
         }),
         'Failed to get object version ID from s3',
       )
     })
 
-    it('should throw error and log if version id is empty', async () => {
+    it('should retry then fail when the version id is empty', async () => {
       // Arrange
       const mockS3Service = new S3Service(true, mockLogger)
       getResult = {
         ContentLength: 100,
         VersionId: '',
       }
 
-      // Act + assert
-      await expect(
+      // Act + assert. missing-VersionId is still retried (unchanged), so we must
+      // attach the rejection assertion before advancing fake timers, otherwise
+      // the rejection fires unhandled while the retry waits are flushing.
+      // eslint-disable-next-line jest/valid-expect -- assertion is awaited below
+      const expectation = expect(
         mockS3Service.getS3ObjectVersionId({
           bucketName: 'bucketName',
           objectKey: 'objectKey',
         }),
-      ).rejects.toThrow('VersionId is empty')
+      ).rejects.toThrow(MissingS3VersionIdError)
+      await jest.advanceTimersByTimeAsync(40000)
+      await expectation
 
+      // 1 initial attempt + 3 retries = 4 attempts, all logged as missed.
+      expect(mockLoggerWarn).toHaveBeenCalledTimes(4)
       expect(mockLoggerError).toHaveBeenCalledTimes(1)
       expect(mockLoggerError).toHaveBeenCalledWith(
         expect.objectContaining({
-          bucketName: 'bucketName',
-          objectKey: 'objectKey',
-          err: new Error('VersionId is empty'),
+          meta: expect.objectContaining({
+            bucketName: 'bucketName',
+            objectKey: 'objectKey',
+            status: 'failed',
+            attempts: 4,
+          }),
+          err: expect.any(MissingS3VersionIdError),
         }),
         'Failed to get object version ID from s3',
       )

services/virus-scanner-guardduty/src/index.ts

@@ -5,7 +5,11 @@ import { validate } from 'uuid'
 
 import { config } from './config'
 import { getLambdaLogger } from './logger'
-import { S3Service } from './s3.service'
+import {
+  MissingS3VersionIdError,
+  S3Service,
+  ZeroByteS3ObjectError,
+} from './s3.service'
 import { isBodyWithKey, MalwareScanTagValue } from './types'
 
 /**
@@ -66,8 +70,25 @@ export const handler = async (
       objectKey: quarantineFileKey,
     })
   } catch (err) {
+    // Distinguish the three failure modes so the warn log is filterable per
+    // case (404, missing VersionId, 0-byte). The HTTP response stays 404 with
+    // the same body for all three, preserving existing caller behaviour.
+    let reason: 'not_found' | 'missing_version_id' | 'zero_byte'
+    let logMessage: string
+    if (err instanceof ZeroByteS3ObjectError) {
+      reason = 'zero_byte'
+      logMessage = 'S3 object is 0 bytes'
+    } else if (err instanceof MissingS3VersionIdError) {
+      reason = 'missing_version_id'
+      logMessage = 'S3 object is missing a version id'
+    } else {
+      reason = 'not_found'
+      logMessage = 'File not found in s3'
+    }
+
     logger.warn({
-      message: 'File not found or its content is empty',
+      message: logMessage,
+      reason,
       err,
       quarantineFileKey,
     })

services/virus-scanner-guardduty/src/s3.service.ts

@@ -17,6 +17,41 @@ import {
   MoveS3FileParams,
 } from './types'
 
+/**
+ * ts-retry-promise wraps retried errors in its own RetryError, which exposes the
+ * original failure as `lastError`. We duck-type it (instead of `instanceof`)
+ * because the package's source/dist dual entry points make `instanceof` unsafe
+ * under ts-jest.
+ */
+const isRetryError = (err: unknown): err is { lastError: unknown } =>
+  !!err && typeof err === 'object' && 'lastError' in err
+
+/**
+ * Thrown when a HeadObject succeeds but the object has no VersionId.
+ * Retried like any other transient failure, then surfaced to the caller.
+ */
+export class MissingS3VersionIdError extends Error {
+  constructor() {
+    super('VersionId is empty')
+    this.name = 'MissingS3VersionIdError'
+    Object.setPrototypeOf(this, MissingS3VersionIdError.prototype)
+  }
+}
+
+/**
+ * Thrown when a HeadObject reports ContentLength === 0. S3 has had strong
+ * read-after-write consistency since 2020, so a 0-byte HeadObject can never
+ * recover on retry — this error is therefore non-retryable and short-circuits
+ * immediately so callers can log and bail without wasted invocations.
+ */
+export class ZeroByteS3ObjectError extends Error {
+  constructor() {
+    super('S3 object is 0 bytes')
+    this.name = 'ZeroByteS3ObjectError'
+    Object.setPrototypeOf(this, ZeroByteS3ObjectError.prototype)
+  }
+}
+
 export class S3Service {
   private readonly s3Client: S3Client
   private readonly isDevelopmentEnv: boolean
@@ -305,13 +340,13 @@ export class S3Service {
           const { VersionId: versionId, ContentLength } = response
 
           if (!versionId) {
-            const err = new Error('VersionId is empty')
+            const err = new MissingS3VersionIdError()
             logMissedAttempt(err)
             throw err
           }
 
           if (!ContentLength || ContentLength === 0) {
-            const err = new Error('Body is empty')
+            const err = new ZeroByteS3ObjectError()
             logMissedAttempt(err)
             throw err
           }
@@ -336,22 +371,31 @@ export class S3Service {
           delay: 2000,
           backoff: (attempt) =>
             1000 * Math.pow(2, attempt) + Math.floor(Math.random() * 1000),
+          // A 0-byte HeadObject can never recover under S3 strong
+          // read-after-write consistency, so do not retry it. 404 and
+          // missing-VersionId still retry as before.
+          retryIf: (err) => !(err instanceof ZeroByteS3ObjectError),
         },
       )
     } catch (err) {
+      // ts-retry-promise wraps retried errors in its RetryError (exposed via
+      // `lastError`); unwrap to the original cause so callers can branch on the
+      // typed error. The 0-byte case is never retried (see retryIf above) and
+      // surfaces unwrapped.
+      const cause = isRetryError(err) ? err.lastError : err
       this.logger.error(
         {
           meta: {
             ...logMeta,
             status: 'failed',
             attempts: attempt,
           },
-          err,
+          err: cause,
         },
-        'Failed to get object version ID from s3 after retries',
+        'Failed to get object version ID from s3',
       )
 
-      throw err
+      throw cause
     }
   }
 }