Merge branch 'bugfix/ARSN-398-doNotRefreshGapBuildingIfDisabled' into user/jonathan/S3C-3926

- Fix the situation where gap building is disabled by
  `_saveBuildingGap()` but we attempted to reset the building gap state
  anyway.

- Introduce a new state 'Expired' that can be differentiated from
  'Disabled': it makes `getGapBuildingValidityPeriodMs()` return 0
  instead of 'null' to hint the listing backend that it should trigger
  a new listing.

lib/algos/cache/GapCache.ts

@@ -121,20 +121,23 @@ export default class GapCache implements GapCacheInterface {
     }
 
     /**
-     * Internal helper to remove gaps in the frozen set overlapping
-     * with previously updated keys, right before the frozen gaps get
-     * exposed.
+     * Internal helper to remove gaps in the staging and frozen sets
+     * overlapping with previously updated keys, right before the
+     * frozen gaps get exposed.
      *
      * @return {undefined}
      */
     _removeOverlappingGapsBeforeExpose(): void {
-        // simple optimization to avoid looping over all updates if
-        // there is no gap in the frozen set
-        if (this._frozenUpdates.newGaps.size === 0) {
-            return;
-        }
-        for (const updateSet of [this._stagingUpdates, this._frozenUpdates]) {
-            this._frozenUpdates.newGaps.removeOverlappingGaps(updateSet.updatedKeys);
+        for (const { updatedKeys } of [this._stagingUpdates, this._frozenUpdates]) {
+            if (updatedKeys.size() === 0) {
+                continue;
+            }
+            for (const { newGaps } of [this._stagingUpdates, this._frozenUpdates]) {
+                if (newGaps.size === 0) {
+                    continue;
+                }
+                newGaps.removeOverlappingGaps(updatedKeys);
+            }
         }
     }
 

lib/algos/list/delimiterMaster.ts

@@ -73,13 +73,14 @@ type GapCachingInfo = GapCachingInfo_NoGapCache
 
 
 export const enum GapBuildingState {
-    Disabled = 0, // no gap cache or not allowed to build due to exposure delay timeout
+    Disabled = 0, // no gap cache or no gap building needed (e.g. in V1 versioning format)
     NotBuilding = 1, // not currently building a gap (i.e. not listing within a gap)
     Building = 2, // currently building a gap (i.e. listing within a gap)
+    Expired = 3, // not allowed to build due to exposure delay timeout
 };
 
-type GapBuildingInfo_Disabled = {
-    state: GapBuildingState.Disabled;
+type GapBuildingInfo_NothingToBuild = {
+    state: GapBuildingState.Disabled | GapBuildingState.Expired;
 };
 
 type GapBuildingParams = {
@@ -118,7 +119,7 @@ type GapBuildingInfo_Building = {
     gapWeight: number;
 };
 
-type GapBuildingInfo = GapBuildingInfo_Disabled
+type GapBuildingInfo = GapBuildingInfo_NothingToBuild
     | GapBuildingInfo_NotBuilding
     | GapBuildingInfo_Building;
 
@@ -214,6 +215,8 @@ export class DelimiterMaster extends Delimiter {
         switch (this._gapBuilding.state) {
         case GapBuildingState.Disabled:
             return null;
+        case GapBuildingState.Expired:
+            return 0;
         case GapBuildingState.NotBuilding:
             gapBuilding = <GapBuildingInfo_NotBuilding> this._gapBuilding;
             break;
@@ -314,6 +317,7 @@ export class DelimiterMaster extends Delimiter {
     _checkGapOnMasterDeleteMarker(key: string): FilterReturnValue {
         switch (this._gapBuilding.state) {
         case GapBuildingState.Disabled:
+        case GapBuildingState.Expired:
             break;
         case GapBuildingState.NotBuilding:
             this._createBuildingGap(key, 1);
@@ -494,16 +498,23 @@ export class DelimiterMaster extends Delimiter {
         return params;
     }
 
-    _saveBuildingGap(): void {
+    /**
+     * Save the gap being built if allowed (i.e. still within the
+     * allocated exposure time window).
+     *
+     * @return {boolean} - true if the gap was saved, false if we are
+     * outside the allocated exposure time window.
+     */
+    _saveBuildingGap(): boolean {
         const { gapCache, params, gap, gapWeight } =
               <GapBuildingInfo_Building> this._gapBuilding;
         const totalElapsed = Date.now() - params.initTimestamp;
         if (totalElapsed >= gapCache.exposureDelayMs) {
             this._gapBuilding = {
-                state: GapBuildingState.Disabled,
+                state: GapBuildingState.Expired,
             };
             this._refreshedBuildingParams = null;
-            return;
+            return false;
         }
         const { firstKey, lastKey, weight } = gap;
         gapCache.setGap(firstKey, lastKey, weight);
@@ -518,6 +529,7 @@ export class DelimiterMaster extends Delimiter {
             },
             gapWeight,
         };
+        return true;
     }
 
     /**
@@ -569,7 +581,10 @@ export class DelimiterMaster extends Delimiter {
             // only set gaps that are significant enough in weight and
             // with a non-empty extension
             if (gapWeight >= params.minGapWeight && gap.weight > 0) {
-                this._saveBuildingGap();
+                // we're done if we were not allowed to save the gap
+                if (!this._saveBuildingGap()) {
+                    return;
+                }
                 // params may have been refreshed, reload them
                 gapBuilding = <GapBuildingInfo_Building> this._gapBuilding;
                 params = gapBuilding.params;

package.json

@@ -3,7 +3,7 @@
   "engines": {
     "node": ">=16"
   },
-  "version": "7.70.23",
+  "version": "7.70.24",
   "description": "Common utilities for the S3 project components",
   "main": "build/index.js",
   "repository": {

tests/unit/algos/cache/GapCache.spec.ts

@@ -174,6 +174,11 @@ describe('GapCache', () => {
 
     it('removeOverlappingGaps() should invalidate gaps created later by setGap() but ' +
     'within the exposure delay', async () => {
+        // wait for 80ms (slightly less than exposure delay of 100ms)
+        // before calling removeOverlappingGaps(), so that the next
+        // exposure timer kicks in before the call to setGap()
+        await new Promise(resolve => setTimeout(resolve, 80));
+
         // there is no exposed gap yet, so expect 0 gap removed
         expect(gapCache.removeOverlappingGaps(['dog'])).toEqual(0);
 

tests/unit/algos/list/delimiterMaster.spec.ts

@@ -1186,6 +1186,11 @@ describe('DelimiterMaster listing algorithm: gap caching and lookup', () => {
         resumeFromState = filterEntries(listing, 'Ddv Ddv Ddv Vvv', 'ass ass ass ass',
                                         resumeFromState);
         expect(gapCache.toArray()).toEqual(gapsArray);
+        // gap building should be in expired state
+        expect(listing._gapBuilding.state).toEqual(GapBuildingState.Expired);
+        // remaining validity period should still be 0 because gap building has expired
+        validityPeriod = listing.getGapBuildingValidityPeriodMs();
+        expect(validityPeriod).toEqual(0);
 
         // we should still be able to skip over the existing cached gaps
         expect(listing._gapCaching.state).toEqual(GapCachingState.GapLookupInProgress);