From b4705ac4c15bf3fdf443ca1c0955f3f6c7066106 Mon Sep 17 00:00:00 2001 From: Francois Ferrand Date: Mon, 24 Aug 2026 09:51:39 +0200 Subject: [PATCH 1/2] Skip non-localized entries in replication and lifecycle In a clean room, an object's metadata is local but its data still lives on the source cluster: dataStoreName points at an isCRR location. Such a version has nothing local to offer, so the existing workflows must leave it alone until localization has rewritten the metadata. The replication populator already diverted those entries to localization, but only when localization was configured; otherwise they fell through and got replicated, which would copy a location the target cannot read. Skip them in both cases. Nothing is lost: the localization merge produces a new oplog entry with the real location, and replication picks it up then. Lifecycle transitions get the same treatment. Localization is the only valid transition out of an isCRR location and it has its own trigger, so a transition rule matching such a version is simply skipped and the version re-evaluated on a later scan. Issue: BB-816 --- extensions/lifecycle/tasks/LifecycleTask.js | 17 +++- .../replication/ReplicationQueuePopulator.js | 16 ++-- tests/unit/lifecycle/LifecycleTask.spec.js | 84 +++++++++++++++++++ .../ReplicationQueuePopulator.spec.js | 27 +++++- 4 files changed, 133 insertions(+), 11 deletions(-) diff --git a/extensions/lifecycle/tasks/LifecycleTask.js b/extensions/lifecycle/tasks/LifecycleTask.js index 8a8311840..6bf7556f8 100644 --- a/extensions/lifecycle/tasks/LifecycleTask.js +++ b/extensions/lifecycle/tasks/LifecycleTask.js @@ -31,6 +31,8 @@ const errorTransitionInProgress = errors.InternalError. customizeDescription('transition is currently in progress'); const errorTransitionColdObject = errors.InternalError. customizeDescription('transitioning a cold object is forbidden'); +const errorTransitionNonLocalizedObject = errors.InternalError. + customizeDescription('transitioning a non-localized object is forbidden'); const errorObjectTemporarilyRestored = errors.InternalError. customizeDescription('object temporarily restored'); const errorReplicationInProgress = errors.InternalError. @@ -1270,12 +1272,21 @@ class LifecycleTask extends BackbeatTask { return next(errorReplicationInProgress); } const dataStoreName = objectMD.getDataStoreName(); - const isObjectCold = dataStoreName && locationsConfig[dataStoreName] - && locationsConfig[dataStoreName].isCold; + const locationConfig = (dataStoreName + && locationsConfig[dataStoreName]) || {}; // We do not transition cold objects - if (isObjectCold) { + if (locationConfig.isCold) { return next(errorTransitionColdObject); } + // Clean room: the object data still lives on the source + // (isCRR) location. Localization is the only valid transition + // out of such a location, and it is triggered by its own path + // (the queue populator), not by lifecycle rules. The version + // is simply re-evaluated on a later scan, once localization + // has completed. + if (locationConfig.isCRR) { + return next(errorTransitionNonLocalizedObject); + } // If transition is in progress, do not re-publish entry // to data-mover or cold-archive topic. if (objectMD.getTransitionInProgress()) { diff --git a/extensions/replication/ReplicationQueuePopulator.js b/extensions/replication/ReplicationQueuePopulator.js index e8d9b3234..17145a86d 100644 --- a/extensions/replication/ReplicationQueuePopulator.js +++ b/extensions/replication/ReplicationQueuePopulator.js @@ -84,11 +84,17 @@ class ReplicationQueuePopulator extends QueuePopulatorExtension { const locationConfig = (dataStoreName && locationsConfig[dataStoreName]) || {}; // Clean room: the object data still lives on the source (isCRR) - // location and first needs to be localized. This is unrelated to - // replicationInfo, which tracks replication of a *local* object to - // remote sites, hence the check before any replication condition. - if (locationConfig.isCRR && this.localizationConfig) { - this._publishLocalizationAction(entry, queueEntry, value); + // location, there is nothing local to replicate yet, so localize it + // first (when configured) and skip it. Nothing is lost: the + // localization merge rewrites the metadata, which comes back through + // the oplog with the real location and is replicated then. + // This is unrelated to replicationInfo, which tracks replication of a + // *local* object to remote sites, hence the check before any + // replication condition. + if (locationConfig.isCRR) { + if (this.localizationConfig) { + this._publishLocalizationAction(entry, queueEntry, value); + } return; } // Allow a non-versioned object if being replicated from an NFS bucket. diff --git a/tests/unit/lifecycle/LifecycleTask.spec.js b/tests/unit/lifecycle/LifecycleTask.spec.js index 7150bbe14..ae46563b2 100644 --- a/tests/unit/lifecycle/LifecycleTask.spec.js +++ b/tests/unit/lifecycle/LifecycleTask.spec.js @@ -11,6 +11,7 @@ const LifecycleTask = require( const LifecycleTaskV2 = require( '../../../extensions/lifecycle/tasks/LifecycleTaskV2'); const ActionQueueEntry = require('../../../lib/models/ActionQueueEntry'); +const ReplicationAPI = require('../../../extensions/replication/ReplicationAPI'); const { LifecycleMetrics } = require('../../../extensions/lifecycle/LifecycleMetrics'); const fakeLogger = require('../../utils/fakeLogger'); const { withActiveSpan } = require('../../utils/withActiveSpan'); @@ -2473,6 +2474,89 @@ describe('lifecycle task helper methods', () => { }); }); + describe('_applyTransitionRule', () => { + const CRR_LOCATION = 'location-crr-source'; + const testParams = { + bucket: 'test-bucket', + owner: 'test-owner', + objectKey: 'test-key', + versionId: 'test-version-id', + eTag: '"test-etag"', + lastModified: '2023-01-01T00:00:00.000Z', + site: 'test-site', + accountId: 'test-account-id', + transitionTime: Date.now(), + bucketData: { + target: { + bucket: 'test-bucket', + owner: 'test-owner', + accountId: 'test-account-id', + }, + }, + }; + + let lifecycleTask; + let objectMD; + let sendDataMoverAction; + let putObjectMD; + + function setupObjectMD(dataStoreName) { + objectMD = { + getReplicationStatus: () => 'COMPLETED', + getDataStoreName: () => dataStoreName, + getDataStoreVersionId: () => 'version-123', + getTransitionInProgress: () => false, + getArchive: () => undefined, + getContentLength: () => 1024, + getUserMetadata: () => null, + setTransitionInProgress: sinon.spy(), + setOriginOp: sinon.spy(), + getSerialized: () => '{}', + }; + sinon.stub(lifecycleTask, '_getObjectMD') + .callsFake((params, log, cb) => cb(null, objectMD)); + } + + beforeEach(() => { + lifecycleTask = new LifecycleTask(lp); + lifecycleTask.pausedLocations = new Set(); + lifecycleTask.circuitBreakers = { tripped: () => false }; + lifecycleTask.producer = {}; + lifecycleTask.transitionTasksTopic = 'test-transition-topic'; + sendDataMoverAction = sinon.stub( + ReplicationAPI, 'sendDataMoverAction') + .callsFake((producer, entry, log, cb) => cb()); + putObjectMD = sinon.stub(lifecycleTask, '_putObjectMD') + .callsFake((params, log, cb) => cb()); + }); + + it('should not transition an object still on an isCRR location', + done => { + setupObjectMD(CRR_LOCATION); + + lifecycleTask._applyTransitionRule(testParams, fakeLogger, err => { + assert.strictEqual(err.description, + 'transitioning a non-localized object is forbidden'); + sinon.assert.notCalled(sendDataMoverAction); + sinon.assert.notCalled(objectMD.setTransitionInProgress); + sinon.assert.notCalled(putObjectMD); + done(); + }); + }); + + it('should transition an object on a regular location', done => { + setupObjectMD('us-east-1'); + + lifecycleTask._applyTransitionRule(testParams, fakeLogger, err => { + assert.ifError(err); + sinon.assert.calledOnce(sendDataMoverAction); + sinon.assert.calledOnce(objectMD.setTransitionInProgress); + sinon.assert.calledOnce(putObjectMD); + done(); + }); + }); + }); + describe('_sendObjectAction', () => { it('should emit trigger metrics with the entry location', done => { const lifecycleTask = new LifecycleTask(lp); diff --git a/tests/unit/replication/ReplicationQueuePopulator.spec.js b/tests/unit/replication/ReplicationQueuePopulator.spec.js index bb9a95d16..baa31e1dc 100644 --- a/tests/unit/replication/ReplicationQueuePopulator.spec.js +++ b/tests/unit/replication/ReplicationQueuePopulator.spec.js @@ -597,12 +597,33 @@ describe('replication queue populator: clean room localization', () => { sinon.assert.notCalled(params.metricsHandler.localizationObjects); }); - it('should fall back to replication when localization is disabled', () => { + it('should not replicate a non-localized object when localization is ' + + 'disabled', () => { delete params.config.localization; rqp = new RecordingQueuePopulatorMock(params); rqp._filterKeyOp(makeEntry(makeValue())); - assert.strictEqual(rqp.published.length, 1); - assert.strictEqual(rqp.published[0].topic, TOPIC); + assert.strictEqual(rqp.published.length, 0); + }); + + // the data still lives on the source location: there is nothing local to + // replicate, whatever replicationInfo says. + [true, false].forEach(localizationEnabled => { + it('should never replicate a pending non-localized object ' + + `(localization ${localizationEnabled ? 'enabled' : 'disabled'})`, + () => { + if (!localizationEnabled) { + delete params.config.localization; + } + rqp = new RecordingQueuePopulatorMock(params); + const value = makeValue({ + replicationInfo: { ...repInfo, status: 'PENDING' }, + }); + rqp._filterKeyOp(makeEntry(value)); + + assert.strictEqual( + rqp.published.filter(p => p.topic === TOPIC).length, 0); + sinon.assert.notCalled(params.metricsHandler.objects); + }); }); }); From 10de8e833e0ec57c2c9149987cf43534d3c0021f Mon Sep 17 00:00:00 2001 From: Francois Ferrand Date: Mon, 24 Aug 2026 10:30:48 +0200 Subject: [PATCH 2/2] Restore the original clean room comment The rewording added in the previous commit did not carry any new information, and only made the stacked diff harder to read: the localization trigger comment from BB-814 already explains why the check sits before any replication condition. Issue: BB-816 --- extensions/replication/ReplicationQueuePopulator.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/extensions/replication/ReplicationQueuePopulator.js b/extensions/replication/ReplicationQueuePopulator.js index 17145a86d..9d9d89907 100644 --- a/extensions/replication/ReplicationQueuePopulator.js +++ b/extensions/replication/ReplicationQueuePopulator.js @@ -84,13 +84,9 @@ class ReplicationQueuePopulator extends QueuePopulatorExtension { const locationConfig = (dataStoreName && locationsConfig[dataStoreName]) || {}; // Clean room: the object data still lives on the source (isCRR) - // location, there is nothing local to replicate yet, so localize it - // first (when configured) and skip it. Nothing is lost: the - // localization merge rewrites the metadata, which comes back through - // the oplog with the real location and is replicated then. - // This is unrelated to replicationInfo, which tracks replication of a - // *local* object to remote sites, hence the check before any - // replication condition. + // location and first needs to be localized. This is unrelated to + // replicationInfo, which tracks replication of a *local* object to + // remote sites, hence the check before any replication condition. if (locationConfig.isCRR) { if (this.localizationConfig) { this._publishLocalizationAction(entry, queueEntry, value);