From 0c257d77df94b2d436de171902d311598be6f2d5 Mon Sep 17 00:00:00 2001 From: Francois Ferrand Date: Fri, 27 Dec 2024 16:23:08 +0100 Subject: [PATCH] Make allSettled usage safer allSettled does not follow the usuage fullfil pattern: it will never reject, and always fullfil with an array of the results of each promises. This is not an issue in the case of lifecycle, where we actually ignore all errors; but it makes the code look inconsistent, as it suggests errors are possible but not handle them. To avoid future issues, add proper processing of the results of allSettled to build a single error when appropriate. Issue: BB-641 --- extensions/lifecycle/tasks/LifecycleTask.js | 6 ++++-- extensions/lifecycle/tasks/LifecycleTaskV2.js | 12 ++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/extensions/lifecycle/tasks/LifecycleTask.js b/extensions/lifecycle/tasks/LifecycleTask.js index fd05b4ca5..eeb506c50 100644 --- a/extensions/lifecycle/tasks/LifecycleTask.js +++ b/extensions/lifecycle/tasks/LifecycleTask.js @@ -429,7 +429,7 @@ class LifecycleTask extends BackbeatTask { // if no versions to process, skip further processing for this batch if (allVersionsWithStaleDate.length === 0) { - return Promise.allSettled(promises).then(() => done(), done); + return Promise.allSettled(promises).then(() => done()); } // for each version, get their relative rules, compare with @@ -457,7 +457,9 @@ class LifecycleTask extends BackbeatTask { return resolve(); }))); - return Promise.allSettled(promises).then(() => done(), done); + return Promise.allSettled(promises).then(results => done( + results.find(r => r.status === 'rejected')?.reason + )); }); } diff --git a/extensions/lifecycle/tasks/LifecycleTaskV2.js b/extensions/lifecycle/tasks/LifecycleTaskV2.js index 83038fc9b..e41e6cb1b 100644 --- a/extensions/lifecycle/tasks/LifecycleTaskV2.js +++ b/extensions/lifecycle/tasks/LifecycleTaskV2.js @@ -114,7 +114,7 @@ class LifecycleTaskV2 extends LifecycleTask { return this.backbeatMetadataProxy.listLifecycle(listType, params, log, (err, contents, isTruncated, markerInfo) => { if (err) { - return Promise.allSettled(promises).then(() => done(err), () => done(err)); + return Promise.allSettled(promises).then(() => done(err)); } // re-queue truncated listing only once. @@ -145,7 +145,9 @@ class LifecycleTaskV2 extends LifecycleTask { bucketData, bucketLCRules, contents, log, )); - return Promise.allSettled(promises).then(() => done(), done); + return Promise.allSettled(promises).then(results => done( + results.find(r => r.status === 'rejected')?.reason + )); }); } @@ -195,7 +197,7 @@ class LifecycleTaskV2 extends LifecycleTask { return this.backbeatMetadataProxy.listLifecycle(listType, params, log, (err, contents, isTruncated, markerInfo) => { if (err) { - return Promise.allSettled(promises).then(() => done(err), () => done(err)); + return Promise.allSettled(promises).then(() => done(err)); } // create Set of unique keys not matching the next marker to @@ -255,7 +257,9 @@ class LifecycleTaskV2 extends LifecycleTask { return resolve(); }))); - return Promise.allSettled(promises).then(() => done(), done); + return Promise.allSettled(promises).then(results => done( + results.find(r => r.status === 'rejected')?.reason + )); }); }