ARSN-620 Fix lifecycle listings stuck on PHD master keys

lib/algos/list/delimiterNonCurrent.js

@@ -1,5 +1,5 @@
 const { DelimiterVersions } = require('./delimiterVersions');
-const { FILTER_END, FILTER_SKIP } = require('./tools');
+const { FILTER_END, FILTER_SKIP, FILTER_ACCEPT } = require('./tools');
 
 const TRIM_METADATA_MIN_BLOB_SIZE = 10000;
 
@@ -30,6 +30,9 @@ class DelimiterNonCurrent extends DelimiterVersions {
         // internal state
         this.prevKey = null;
         this.staleDate = null;
+        // Last PHD master key scanned. handlePHDMaster keeps the resume marker one
+        // PHD key behind. See there for why.
+        this.prevPHDKey = undefined;
 
         this.scannedKeys = 0;
     }
@@ -144,6 +147,70 @@ class DelimiterNonCurrent extends DelimiterVersions {
         return;
     }
 
+    /**
+     * Advance the resume marker over a scanned PHD master key.
+     *
+     * THE BUG IT FIXES: a run of dangling PHD masters longer than
+     * maxScannedLifecycleListingEntries truncated the listing with no
+     * NextKeyMarker. The next page then repeated the first page, and the
+     * listing never moved forward.
+     *
+     * THE RULE: the marker moves to the PREVIOUS PHD key. It never points at
+     * the key being scanned. A marker on the scanned key gives the next listing
+     * a key-marker with no version-id-marker. S3 reads that as "start after
+     * every version of this key". The listing would then skip the versions of a
+     * PHD master that still has some, and NCVE would never see them. One key
+     * behind costs one re-scanned entry per truncation, and skips nothing.
+     *
+     * Example, scan limit 3, dangling PHD masters phd-1 ... phd-6:
+     *   page 1: phd-1, phd-2, phd-3 -> truncated, NextKeyMarker=phd-2
+     *   page 2: phd-3, phd-4, phd-5 -> truncated, NextKeyMarker=phd-4
+     *   page 3: phd-5, phd-6        -> done
+     *
+     * THE FALLBACK: on the first PHD of a listing there is no previous PHD key,
+     * and no version key has set a marker yet. The rule above would leave the
+     * marker empty, and the listing would loop again. The marker points at
+     * `key` instead. This one key loses its versions for this pass, and the
+     * listing moves forward.
+     *
+     * WHAT IT DOES NOT TOUCH: the method updates the marker only. It leaves
+     * prevKey and staleDate alone. The next version key scanned is the newest
+     * surviving version under the PHD, and the repair promotes it back to
+     * master. Untouched state keeps that version classified as current, so the
+     * listing never returns it as an expirable noncurrent version.
+     *
+     * Example, a PHD master with two surviving versions:
+     *   apple (PHD)  -> marker stays behind apple, prevKey untouched
+     *   apple\0v1    -> first version seen for apple -> current, protected
+     *   apple\0v2    -> noncurrent -> expirable, staleDate = v1's date
+     *
+     * apple\0v1 is not deduplicated as the master copy: a PHD gets its
+     * versionId at delete time, and that id matches no version key. Setting
+     * prevKey='apple' here would classify apple\0v1 as noncurrent. NCVE would
+     * then expire the very version the repair needs to promote: data loss.
+     *
+     *  @param {String} key   - The PHD master key
+     *  @param {String} versionId - always undefined for a master key
+     *  @param {String} value - The PHD placeholder value
+     *  @return {number} - filter return value
+     */
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    handlePHDMaster(key, versionId, value) {
+        if (this.prevPHDKey !== undefined && this.prevPHDKey > (this.nextKeyMarker || '')) {
+            // Move the marker forward only. prevPHDKey can hold a key from an
+            // earlier run of PHDs that the listing already passed.
+            this.nextKeyMarker = this.prevPHDKey;
+            this.nextVersionIdMarker = undefined;
+        } else if (!this.nextKeyMarker) {
+            // No previous PHD, and no marker yet. Skip this key's versions
+            // rather than leave the listing unable to move forward.
+            this.nextKeyMarker = key;
+            this.nextVersionIdMarker = undefined;
+        }
+        this.prevPHDKey = key;
+        return FILTER_ACCEPT;
+    }
+
     /**
      * Parses the stringified entry's value and remove the location property if too large.
      * @param {string} s - sringified value

lib/algos/list/delimiterOrphanDeleteMarker.js

@@ -1,5 +1,5 @@
 const DelimiterVersions = require('./delimiterVersions').DelimiterVersions;
-const { FILTER_END } = require('./tools');
+const { FILTER_END, FILTER_ACCEPT } = require('./tools');
 const TRIM_METADATA_MIN_BLOB_SIZE = 10000;
 /**
  * Handle object listing with parameters. This extends the base class DelimiterVersions
@@ -171,6 +171,57 @@ class DelimiterOrphanDeleteMarker extends DelimiterVersions {
         return;
     }
 
+    /**
+     * Process a scanned PHD master key as a key transition, exactly like
+     * the new-key branch of addVersion(). This serves two purposes:
+     *
+     * 1. Advance the resume position. On truncation the marker is
+     *    this.prevKeyName (one key behind, so an undecided key is
+     *    re-scanned by the next listing). Without this method, a run of
+     *    dangling PHD masters longer than
+     *    maxScannedLifecycleListingEntries would truncate the listing
+     *    with no marker, and every retry would restart from scratch,
+     *    forever.
+     *
+     *    Example: scan limit 3, dangling PHD masters phd-1 ... phd-6:
+     *      page 1: phd-1, phd-2, phd-3 -> truncated, NextMarker=phd-2
+     *      page 2: phd-3, phd-4, phd-5 -> truncated, NextMarker=phd-4
+     *      page 3: phd-5, phd-6       -> done
+     *      (before: page 1 had no NextMarker -> page 2 = page 1 -> loop)
+     *
+     * 2. Resolve the held candidate. A delete marker is kept in memory
+     *    (keyName/value) until an entry of ANOTHER key proves it has no
+     *    other version, i.e. that it is an orphan. A PHD master is such
+     *    an entry, so the candidate must be emitted here, BEFORE the
+     *    marker moves past it: once behind the marker it would never be
+     *    scanned again, and the orphan delete marker never expired.
+     *
+     *    Example: scan limit 3, keyspace: banana\0v1 (DM), phd-1 ...:
+     *      banana\0v1 -> held as candidate (orphan? unknown yet)
+     *      phd-1      -> new key: banana proven orphan -> emitted
+     *      (advancing the marker without emitting would silently drop
+     *      banana's delete marker forever)
+     *
+     * The PHD key itself is never held as a candidate (value = null):
+     * it is not a delete marker, and a dangling PHD has no versions.
+     *  @param {String} key   - The PHD master key
+     *  @param {String} versionId - always undefined for a master key
+     *  @param {String} value - The PHD placeholder value
+     *  @return {number} - filter return value
+     */
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    handlePHDMaster(key, versionId, value) {
+        if (key !== this.keyName) {
+            if (this.value) {
+                this._addOrphan();
+            }
+            this.prevKeyName = this.keyName;
+            this.keyName = key;
+            this.value = null;
+        }
+        return FILTER_ACCEPT;
+    }
+
     result() {
         // Only check for remaining last orphan delete marker if the listing is not interrupted.
         // This will help avoid false positives.

lib/algos/list/delimiterVersions.ts

@@ -380,6 +380,28 @@ export class DelimiterVersions extends Extension {
         return this.keyHandlers[this.state.id](key, versionId, value);
     }
 
+    /**
+     * Hook called when a PHD master key is scanned. The default behavior
+     * is to accept and skip it without any effect on the listing state.
+     *
+     * Bounded lifecycle listings (DelimiterOrphanDeleteMarker,
+     * DelimiterNonCurrent) override this hook to record the PHD key as
+     * the resume position: without it, a contiguous run of dangling PHD
+     * masters longer than maxScannedLifecycleListingEntries exhausts the
+     * scan budget without ever advancing the marker, so the truncated
+     * listing carries no resume position and the next one restarts from
+     * scratch, forever.
+     *
+     * @param {string} key - master key holding the PHD placeholder
+     * @param {string} [versionId] - always undefined for a master key
+     * @param {string} value - PHD placeholder metadata value
+     * @return {number} - filter return value
+     */
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    handlePHDMaster(key: string, versionId: string | undefined, value: string): FilterReturnValue {
+        return FILTER_ACCEPT;
+    }
+
     keyHandler_NotSkippingV0(key: string, versionId: string | undefined, value: string): FilterReturnValue {
         if (key.startsWith(DbPrefixes.Replay)) {
             // skip internal replay prefix entirely
@@ -390,7 +412,7 @@ export class DelimiterVersions extends Extension {
             return FILTER_SKIP;
         }
         if (Version.isPHD(value)) {
-            return FILTER_ACCEPT;
+            return this.handlePHDMaster(key, versionId, value);
         }
         return this.filter_onNewKey(key, versionId, value);
     }
@@ -399,7 +421,7 @@ export class DelimiterVersions extends Extension {
         // NOTE: this check on PHD is only useful for Artesca, S3C
         // does not use PHDs in V1 format
         if (Version.isPHD(value)) {
-            return FILTER_ACCEPT;
+            return this.handlePHDMaster(key, versionId, value);
         }
         return this.filter_onNewKey(key, versionId, value);
     }

tests/unit/algos/list/delimiterNonCurrent.spec.js

@@ -450,3 +450,195 @@ function getListingKey(key, vFormat) {
         });
     });
 });
+
+describe('DelimiterNonCurrent over PHD master keys', () => {
+    const valuePHD = '{"isPHD":true,"versionId":"phd-vid"}';
+
+    ['v0', 'v1'].forEach(v => {
+        describe(`with ${v} bucket format`, () => {
+            it('should set NextKeyMarker to the PHD key when truncation happens inside a run of ' +
+            'dangling PHD masters', () => {
+                const maxScannedLifecycleListingEntries = 5;
+                const delimiter = new DelimiterNonCurrent(
+                    { maxScannedLifecycleListingEntries }, fakeLogger, v);
+
+                for (let i = 1; i <= 5; i++) {
+                    assert.strictEqual(delimiter.filter({
+                        key: getListingKey(`img-00${i}`, v),
+                        value: valuePHD,
+                    }), FILTER_ACCEPT);
+                }
+                assert.strictEqual(delimiter.filter({
+                    key: getListingKey('img-006', v),
+                    value: valuePHD,
+                }), FILTER_END);
+
+                const result = delimiter.result();
+                assert.strictEqual(result.IsTruncated, true);
+                // Before the fix, NextKeyMarker was undefined. The next listing
+                // restarted from scratch on any desert longer than the scan limit.
+                // The marker now stays one PHD key behind the last one scanned,
+                // img-005. The next page re-scans img-005. That costs one entry,
+                // and it keeps the versions of a PHD master that still has some.
+                assert.strictEqual(result.NextKeyMarker, 'img-004');
+                assert.strictEqual(result.NextVersionIdMarker, undefined);
+                assert.deepStrictEqual(result.Contents, []);
+            });
+
+            it('should keep protecting the newest surviving version under a PHD master', () => {
+                const delimiter = new DelimiterNonCurrent({}, fakeLogger, v);
+
+                const key = 'apple';
+                const survivorVersionId = 'version1';
+                const survivorDate = '1970-01-01T00:00:00.002Z';
+                const survivorValue = `{"versionId":"${survivorVersionId}","last-modified":"${survivorDate}"}`;
+                const olderVersionId = 'version2';
+                const olderDate = '1970-01-01T00:00:00.001Z';
+                const olderValue = `{"versionId":"${olderVersionId}","last-modified":"${olderDate}"}`;
+
+                // the PHD master's generated versionId matches none of the
+                // surviving version keys, so no master/version deduplication
+                // applies to them
+                assert.strictEqual(delimiter.filter({
+                    key: getListingKey(key, v),
+                    value: valuePHD,
+                }), FILTER_ACCEPT);
+                assert.strictEqual(delimiter.filter({
+                    key: getListingKey(`${key}${VID_SEP}${survivorVersionId}`, v),
+                    value: survivorValue,
+                }), FILTER_ACCEPT);
+                assert.strictEqual(delimiter.filter({
+                    key: getListingKey(`${key}${VID_SEP}${olderVersionId}`, v),
+                    value: olderValue,
+                }), FILTER_ACCEPT);
+
+                const result = delimiter.result();
+                assert.strictEqual(result.IsTruncated, false);
+                // the newest surviving version is the first version key scanned
+                // for this object: it must be classified current (it is what the
+                // PHD repair promotes back into the master) and never listed as
+                // an expirable noncurrent version. Only the older version is
+                // noncurrent, with its stale date taken from the survivor.
+                assert.strictEqual(result.Contents.length, 1);
+                assert.strictEqual(result.Contents[0].key, key);
+                const parsed = JSON.parse(result.Contents[0].value);
+                assert.strictEqual(parsed.versionId, olderVersionId);
+                assert.strictEqual(parsed.staleDate, survivorDate);
+            });
+        });
+    });
+
+    describe('crawling a v0 keyspace with marker feedback', () => {
+        function crawlNonCurrentListing(keyspace, maxScannedLifecycleListingEntries, maxPages) {
+            const pages = [];
+            let keyMarker;
+            let versionIdMarker;
+            for (let i = 0; i < maxPages; i++) {
+                const delimiter = new DelimiterNonCurrent(
+                    { keyMarker, versionIdMarker, maxScannedLifecycleListingEntries }, fakeLogger, 'v0');
+                const params = delimiter.genMDParams();
+                for (const entry of keyspace) {
+                    if (params.gt !== undefined && entry.key <= params.gt) {
+                        continue;
+                    }
+                    if (params.gte !== undefined && entry.key < params.gte) {
+                        continue;
+                    }
+                    if (delimiter.filter(entry) === FILTER_END) {
+                        break;
+                    }
+                }
+                const result = delimiter.result();
+                pages.push(result);
+                if (!result.IsTruncated) {
+                    return pages;
+                }
+                assert(result.NextKeyMarker,
+                    `truncated page ${pages.length} returned no NextKeyMarker: ` +
+                    'the next listing would restart from scratch');
+                if (keyMarker !== undefined) {
+                    const prevTuple = `${keyMarker}${VID_SEP}${versionIdMarker || ''}`;
+                    const newTuple = `${result.NextKeyMarker}${VID_SEP}${result.NextVersionIdMarker || ''}`;
+                    assert.notStrictEqual(newTuple, prevTuple,
+                        `marker did not advance on truncated page ${pages.length}`);
+                }
+                keyMarker = result.NextKeyMarker;
+                versionIdMarker = result.NextVersionIdMarker;
+            }
+            throw new Error(`listing did not terminate within ${maxPages} pages: ` +
+                'markerless truncation restarts it from scratch');
+        }
+
+        it('should cross a PHD desert and list only the noncurrent versions on both sides', () => {
+            const appleDate = '1970-01-01T00:00:00.004Z';
+            const appleOldDate = '1970-01-01T00:00:00.003Z';
+            const zebraDate = '1970-01-01T00:00:00.002Z';
+            const zebraOldDate = '1970-01-01T00:00:00.001Z';
+            const appleValue = `{"versionId":"apple-v1","last-modified":"${appleDate}"}`;
+            const appleOldValue = `{"versionId":"apple-v2","last-modified":"${appleOldDate}"}`;
+            const zebraValue = `{"versionId":"zebra-v1","last-modified":"${zebraDate}"}`;
+            const zebraOldValue = `{"versionId":"zebra-v2","last-modified":"${zebraOldDate}"}`;
+
+            const keyspace = [
+                { key: 'apple', value: appleValue },
+                { key: `apple${VID_SEP}apple-v1`, value: appleValue },
+                { key: `apple${VID_SEP}apple-v2`, value: appleOldValue },
+            ];
+            for (let i = 1; i <= 8; i++) {
+                keyspace.push({ key: `img-00${i}`, value: valuePHD });
+            }
+            keyspace.push({ key: 'zebra', value: zebraValue });
+            keyspace.push({ key: `zebra${VID_SEP}zebra-v1`, value: zebraValue });
+            keyspace.push({ key: `zebra${VID_SEP}zebra-v2`, value: zebraOldValue });
+
+            const pages = crawlNonCurrentListing(keyspace, 5, 10);
+
+            // 4 pages, not 3. The marker stays one PHD key behind, so each
+            // truncated page re-scans one entry. The desert advances by
+            // scanLimit - 1 keys per page.
+            assert.strictEqual(pages.length, 4);
+            const listed = pages
+                .reduce((acc, page) => acc.concat(page.Contents), [])
+                .map(entry => {
+                    const parsed = JSON.parse(entry.value);
+                    return { key: entry.key, versionId: parsed.versionId, staleDate: parsed.staleDate };
+                });
+            assert.deepStrictEqual(listed, [
+                { key: 'apple', versionId: 'apple-v2', staleDate: appleDate },
+                { key: 'zebra', versionId: 'zebra-v2', staleDate: zebraDate },
+            ]);
+        });
+
+        // The scan limit can end on a PHD master that still has version keys. A
+        // bookmark on that key gives a bare keyMarker. A bare keyMarker resumes
+        // after all versions of the key (genMDParamsV0: gt = keyMarker +
+        // inc(VID_SEP)), so the listing skips that key's noncurrent work.
+        // handlePHDMaster keeps the marker one PHD key behind instead. This needs
+        // no versionIdMarker sentinel, because the next page re-scans the key.
+        it('should not skip the versions of a PHD master when the scan limit lands exactly ' +
+        'on the master', () => {
+            const keyspace = [
+                { key: 'k1', value: '{"versionId":"k1-v1","last-modified":"1970-01-01T00:00:00.001Z"}' },
+                { key: 'k2', value: '{"versionId":"k2-v1","last-modified":"1970-01-01T00:00:00.001Z"}' },
+                { key: 'k3', value: '{"versionId":"k3-v1","last-modified":"1970-01-01T00:00:00.001Z"}' },
+                { key: 'k4', value: '{"versionId":"k4-v1","last-modified":"1970-01-01T00:00:00.001Z"}' },
+                { key: 'kilo', value: valuePHD },
+                { key: `kilo${VID_SEP}kilo-v1`,
+                    value: '{"versionId":"kilo-v1","last-modified":"1970-01-01T00:00:00.002Z"}' },
+                { key: `kilo${VID_SEP}kilo-v2`,
+                    value: '{"versionId":"kilo-v2","last-modified":"1970-01-01T00:00:00.001Z"}' },
+                { key: 'mango', value: '{"versionId":"mango-v1","last-modified":"1970-01-01T00:00:00.001Z"}' },
+            ];
+
+            const pages = crawlNonCurrentListing(keyspace, 5, 10);
+
+            const listedVersionIds = pages
+                .reduce((acc, page) => acc.concat(page.Contents), [])
+                .map(entry => JSON.parse(entry.value).versionId);
+            // kilo-v2 is noncurrent (kilo-v1 is the de-facto current version)
+            // and must be listed even though the scan limit landed exactly on
+            // the PHD master right above it
+            assert(listedVersionIds.includes('kilo-v2'));
+        });
+    });
+});