fix: retry for backup object event - #1229
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the backup-object queue handler so that failures propagate (allowing pg-boss retries to occur as configured), and corrects typos in structured log fields used by several event handlers.
Changes:
- Fix structured logging field typo (
jodId→jobId) across multiple event handlers. - Correct the
eventvalue forBackupObjectEventerror logs and rethrow errors to enable retries (retryLimit: 5). - Add a unit test to ensure backup failures reject and the DB connection is disposed.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/storage/events/objects/object-admin-delete.ts | Fixes log field typo to use jobId. |
| src/storage/events/objects/object-admin-delete-all-before.ts | Fixes log field typo to use jobId. |
| src/storage/events/objects/backup-object.ts | Fixes error log fields and rethrows on failure so the queue can retry. |
| src/storage/events/objects/backup-object.test.ts | Adds coverage for failure rethrow + connection disposal behavior. |
| src/storage/events/lifecycle/webhook.ts | Fixes log field typo to use jobId. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage Report for CI Build 36020182684Coverage increased (+0.03%) to 83.665%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
aa2f1dd to
44f02e3
Compare
44f02e3 to
d0d03c7
Compare
|
@claude review always |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
1b3a4b8 to
1697636
Compare
There was a problem hiding this comment.
Approve — the new job.id !== SYNC_JOB_ID guard in backup-object.ts, added in the latest commit, directly addresses the earlier finding that the unconditional rethrow could abort the whole admin orphan-cleanup sweep when BackupObjectEvent.handle is invoked synchronously (via batchSend/send with pgQueueEnable false), and it's now covered by a dedicated test ("logs synchronous backup failures and completes without deleting the source").
Reviewed: the idempotent backup/retry logic in backup-object.ts (NoSuchKey/404 handling, headObject confirmMissing check distinguishing already-backed-up vs. both-missing), the sync-vs-async job id plumbing in queue/event.ts and constants.ts, and the jodId→jobId log-field typo fixes in webhook.ts and the object-admin-delete files.
Extended reasoning...
The diff makes S3 object backup/delete idempotent and retry-safe (backup-object.ts), introduces a shared SYNC_JOB_ID sentinel to distinguish synchronous invocations from queued pg-boss jobs (queue/event.ts, constants.ts), and fixes a log field typo in three unrelated files; none of this touches auth, crypto, or permissions. My prior inline finding (from an earlier run) was that the unconditional rethrow could break ObjectScanner's admin orphan sweep, which calls handle() synchronously in a Promise.all batch with no per-item isolation; the latest commit (1697636) adds the SYNC_JOB_ID check that skips rethrowing exactly in that synchronous path, with a new unit test verifying the behavior. The change is now well covered by a 211-line test file exercising the new retry/idempotency branches, two independent human reviewers already approved on earlier revisions, and no outstanding third-party objections remain unaddressed.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Backup object handler doesn't throw error so failure doesn't retry.
There are some typos in logs fields.
What is the new behavior?
Rethrow the error so queue can retry (count 5 can work).
Fix typos.
Additional context
We should extend
logSchema.errorto extendEventLogand cut duplication between error and success but it's for another PR.