fix(virus-scanner): stop retrying 0-byte S3 objects and clarify logs#9646
Open
mvincentong wants to merge 2 commits into
Open
fix(virus-scanner): stop retrying 0-byte S3 objects and clarify logs#9646mvincentong wants to merge 2 commits into
mvincentong wants to merge 2 commits into
Conversation
The virus-scanner-guardduty service ships jest + ts-jest as devDependencies and has spec files under src/__tests, but no jest config and no test script, so the specs never compiled or ran (CI has no service test job either). Add a minimal ts-jest jest.config.js and a `test` script that loads .env.development (for the localstack bucket names the existing specs assert against) so `pnpm --filter formsg-virus-scanner-guardduty test` runs the suite. No production behaviour change; the next commit updates the specs to match the 0-byte retry fix.
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 opengovsg#9481
mvincentong
force-pushed
the
fix/virus-scanner-zero-byte-no-retry
branch
from
a2eca40 to
463e519
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Closes #9481.
getS3ObjectVersionIdretried 0-byteHeadObjectresponses and surfaced a misleading"Body is empty"error. For S3, a 0-byteContentLengthis not recoverable by retry, so retries added cost/noise without changing outcome.Solution
services/virus-scanner-guardduty/src/s3.service.ts:ZeroByteS3ObjectError("S3 object is 0 bytes").VersionId.ts-retry-promiseretry wrappers before rethrow.services/virus-scanner-guardduty/src/index.ts:not_found,missing_version_id, andzero_bytereasons while keeping the same 404 response body.jest.config.js+testscript) in a separate commit so reviewers can drop it independently if preferred.Breaking Changes
Features:
Improvements:
Bug Fixes:
Before & After Screenshots
N/A - backend lambda behavior/logging only.
Tests
Focused coverage:
ZeroByteS3ObjectErrorafter one attempt.VersionIdstill retries to exhaustion (unchanged behavior).reasonfields fornot_found,missing_version_id, andzero_byte.Deploy Notes
New environment variables:
New scripts:
services/virus-scanner-guardduty:test(for service-local unit tests)New dependencies:
New dev dependencies: