#4205: mark potentially stale features (#4217)

This PR lays most of the groundwork required for emitting events when
features are marked as potentially stale by Unleash. It does **not**
emit any events just yet. The summary is:
- periodically look for features that are potentially stale and mark
them (set to run every 10 seconds for now; can be changed)
- when features are updated, if the update data contains changes to the
feature's type or createdAt date, also update the potentially stale
status.

It is currently about 220 lines of tests and about 100 lines of
application code (primarily db migration and two new methods on the
IFeatureToggleStore interface).

The reason I wanted to put this into a single PR (instead of just the db
migration, then just the potentially stale marking, then the update
logic) is:
If users get the db migration first, but not the rest of the update
logic until the events are fired, then they could get a bunch of new
events for features that should have been marked as potentially stale
several days/weeks/months ago. That seemed undesirable to me, so I
decided to bunch those changes together. Of course, I'd be happy to
break it into smaller parts.

## Rules

A toggle will be marked as potentially stale iff:
- it is not already stale
- its createdAt date is older than its feature type's expected lifetime
would dictate

## Migration

The migration adds a new `potentially_stale` column to the features
table and sets this to true for any toggles that have exceeded their
expected lifetime and that have not already been marked as `stale`.

## Discussion

### The `currentTime` parameter of `markPotentiallyStaleFeatures`

The `markPotentiallyStaleFetaures` method takes an optional
`currentTime` parameter. This was added to make it easier to test (so
you can test "into the future"), but it's not used in the application.
We can rewrite the tests to instead update feature toggles manually, but
that wouldn't test the actual marking method. Happy to discuss.

src/lib/db/feature-toggle-store.ts

@@ -262,7 +262,26 @@ export default class FeatureToggleStore implements IFeatureToggleStore {
             .where({ name: data.name })
             .update(this.dtoToRow(project, data))
             .returning(FEATURE_COLUMNS);
-        return this.rowToFeature(row[0]);
+
+        const feature = this.rowToFeature(row[0]);
+        // if a feature toggle's type or createdAt has changed, update its potentially stale status
+        if (!feature.stale && (data.type || data.createdAt)) {
+            await this.db(TABLE)
+                .where({ name: data.name })
+                .update(
+                    'potentially_stale',
+                    this.db.raw(
+                        `(? > (features.created_at + ((
+                            SELECT feature_types.lifetime_days
+                            FROM feature_types
+                            WHERE feature_types.id = features.type
+                        ) * INTERVAL '1 day')))`,
+                        this.db.fn.now(),
+                    ),
+                );
+        }
+
+        return feature;
     }
 
     async archive(name: string): Promise<FeatureToggle> {
@@ -377,6 +396,40 @@ export default class FeatureToggleStore implements IFeatureToggleStore {
 
         return toggle;
     }
+
+    async markPotentiallyStaleFeatures(
+        currentTime?: string,
+    ): Promise<string[]> {
+        const query = this.db(TABLE)
+            .update('potentially_stale', true)
+            .whereRaw(
+                `? > (features.created_at + ((
+                    SELECT feature_types.lifetime_days
+                    FROM feature_types
+                    WHERE feature_types.id = features.type
+                ) * INTERVAL '1 day'))`,
+                [currentTime || this.db.fn.now()],
+            )
+            .andWhere(function () {
+                this.where('potentially_stale', null).orWhere(
+                    'potentially_stale',
+                    false,
+                );
+            })
+            .andWhereNot('stale', true);
+
+        const updatedFeatures = await query.returning(FEATURE_COLUMNS);
+        return updatedFeatures.map(({ name }) => name);
+    }
+
+    async isPotentiallyStale(featureName: string): Promise<boolean> {
+        const result = await this.db(TABLE)
+            .first(['potentially_stale'])
+            .from(TABLE)
+            .where({ name: featureName });
+
+        return result?.potentially_stale ?? false;
+    }
 }
 
 module.exports = FeatureToggleStore;

src/lib/services/feature-toggle-service.ts

@@ -1965,6 +1965,10 @@ class FeatureToggleService {
             }
         }
     }
+
+    async markPotentiallyStaleFeatures(): Promise<void> {
+        await this.featureToggleStore.markPotentiallyStaleFeatures();
+    }
 }
 
 export default FeatureToggleService;

src/lib/services/index.ts

@@ -74,6 +74,7 @@ export const scheduleServices = async (
         configurationRevisionService,
         maintenanceService,
         eventAnnouncerService,
+        featureToggleService,
     } = services;
 
     if (await maintenanceService.isMaintenanceMode()) {
@@ -125,6 +126,13 @@ export const scheduleServices = async (
         ),
         secondsToMilliseconds(1),
     );
+
+    schedulerService.schedule(
+        featureToggleService.markPotentiallyStaleFeatures.bind(
+            featureToggleService,
+        ),
+        minutesToMilliseconds(1),
+    );
 };
 
 export const createServices = (

src/lib/types/stores/feature-toggle-store.ts

@@ -32,6 +32,9 @@ export interface IFeatureToggleStore extends Store<FeatureToggle, string> {
         range?: string[];
         dateAccessor: string;
     }): Promise<number>;
+    markPotentiallyStaleFeatures(currentTime?: string): Promise<string[]>;
+    isPotentiallyStale(featureName: string): Promise<boolean>;
+
     /**
      * @deprecated - Variants should be fetched from FeatureEnvironmentStore (since variants are now; since 4.18, connected to environments)
      * @param featureName

src/migrations/20230711094214-add-potentially-stale-flag.js

@@ -0,0 +1,25 @@
+'use strict';
+
+exports.up = function (db, cb) {
+    db.runSql(
+        `
+        ALTER TABLE features ADD COLUMN IF NOT EXISTS potentially_stale boolean;
+        UPDATE features
+            SET potentially_stale = TRUE
+            FROM feature_types
+            WHERE feature_types.id = features.type
+            AND CURRENT_TIMESTAMP > (features.created_at + (feature_types.lifetime_days * INTERVAL '1 day'))
+            AND features.stale IS NOT TRUE;
+        `,
+        cb,
+    );
+};
+
+exports.down = function (db, cb) {
+    db.runSql(
+        `
+        ALTER table features DROP COLUMN IF EXISTS potentially_stale;
+        `,
+        cb,
+    );
+};

src/test/e2e/stores/feature-toggle-store.e2e.test.ts

@@ -1,9 +1,10 @@
 import dbInit from '../helpers/database-init';
 import getLogger from '../../fixtures/no-logger';
+import { FeatureToggleDTO, IFeatureToggleStore } from '../../../lib/types';
 
 let stores;
 let db;
-let featureToggleStore;
+let featureToggleStore: IFeatureToggleStore;
 
 beforeAll(async () => {
     getLogger.setMuteError(true);
@@ -27,3 +28,247 @@ test('should not crash for undefined toggle name', async () => {
     const project = await featureToggleStore.getProjectId(undefined);
     expect(project).toBe(undefined);
 });
+
+describe('potentially_stale marking', () => {
+    afterEach(async () => {
+        await featureToggleStore.deleteAll();
+    });
+    const getFutureTimestamp = (days: number) => {
+        return new Date(
+            new Date().getTime() +
+                days * 24 * 60 * 60 * 1000 +
+                // add an extra second
+                1000,
+        ).toISOString();
+    };
+
+    test('it returns an empty list of no toggles were updated', async () => {
+        const features: FeatureToggleDTO[] = [
+            {
+                name: 'feature1',
+                type: 'release',
+            },
+        ];
+        await Promise.all(
+            features.map((feature) =>
+                featureToggleStore.create('default', feature),
+            ),
+        );
+
+        const markedToggles =
+            await featureToggleStore.markPotentiallyStaleFeatures();
+
+        expect(markedToggles).toStrictEqual([]);
+    });
+    test('it returns the names of only the marked toggles', async () => {
+        const features: FeatureToggleDTO[] = [
+            {
+                name: 'feature1',
+                type: 'release',
+            },
+            {
+                name: 'feature2',
+                type: 'kill-switch',
+            },
+        ];
+        await Promise.all(
+            features.map((feature) =>
+                featureToggleStore.create('default', feature),
+            ),
+        );
+
+        const markedToggles =
+            await featureToggleStore.markPotentiallyStaleFeatures(
+                getFutureTimestamp(41),
+            );
+
+        expect(markedToggles).toStrictEqual(['feature1']);
+    });
+
+    test.each([
+        [0, []],
+        [7, ['operational']],
+        [40, ['operational', 'release', 'experiment']],
+        [10000, ['operational', 'release', 'experiment']],
+    ])(
+        'it marks toggles based on their type (days elapsed: %s)',
+        async (daysElapsed, expectedMarkedFeatures) => {
+            const features: FeatureToggleDTO[] = [
+                'release',
+                'experiment',
+                'operational',
+                'kill-switch',
+                'permission',
+            ].map((type) => ({ name: type, type }));
+            await Promise.all(
+                features.map((feature) =>
+                    featureToggleStore.create('default', feature),
+                ),
+            );
+
+            for (const feature of expectedMarkedFeatures) {
+                expect(await featureToggleStore.get(feature)).toBeTruthy();
+            }
+
+            const markedToggles =
+                await featureToggleStore.markPotentiallyStaleFeatures(
+                    getFutureTimestamp(daysElapsed),
+                );
+
+            expect(markedToggles).toEqual(
+                expect.arrayContaining(expectedMarkedFeatures),
+            );
+            expect(markedToggles.length).toEqual(expectedMarkedFeatures.length);
+
+            for (const feature of expectedMarkedFeatures) {
+                expect(
+                    await featureToggleStore.isPotentiallyStale(feature),
+                ).toBeTruthy();
+            }
+        },
+    );
+    test('it does not mark toggles already flagged as stale', async () => {
+        const features: FeatureToggleDTO[] = [
+            {
+                name: 'feature1',
+                type: 'release',
+                stale: true,
+            },
+        ];
+        await Promise.all(
+            features.map((feature) =>
+                featureToggleStore.create('default', feature),
+            ),
+        );
+        const markedToggles =
+            await featureToggleStore.markPotentiallyStaleFeatures(
+                getFutureTimestamp(1000),
+            );
+        expect(markedToggles).toStrictEqual([]);
+    });
+    test('it does not return toggles previously marked as potentially_stale', async () => {
+        const features: FeatureToggleDTO[] = [
+            {
+                name: 'feature1',
+                type: 'release',
+            },
+        ];
+        await Promise.all(
+            features.map((feature) =>
+                featureToggleStore.create('default', feature),
+            ),
+        );
+        const markedToggles =
+            await featureToggleStore.markPotentiallyStaleFeatures(
+                getFutureTimestamp(50),
+            );
+        expect(markedToggles).toStrictEqual(['feature1']);
+
+        const secondPass =
+            await featureToggleStore.markPotentiallyStaleFeatures(
+                getFutureTimestamp(100),
+            );
+        expect(secondPass).toStrictEqual([]);
+    });
+
+    describe('changing feature types', () => {
+        const getPastDate = (days: number) => {
+            return new Date(
+                new Date().getTime() -
+                    days * 24 * 60 * 60 * 1000 -
+                    // subtract an extra second
+                    1000,
+            );
+        };
+
+        test("if a potentially stale feature changes to a type that shouldn't be stale, it's 'potentially_stale' marker is removed.", async () => {
+            const features: FeatureToggleDTO[] = [
+                {
+                    name: 'feature1',
+                    type: 'release',
+                },
+            ];
+            await Promise.all(
+                features.map((feature) =>
+                    featureToggleStore.create('default', feature),
+                ),
+            );
+            const markedToggles =
+                await featureToggleStore.markPotentiallyStaleFeatures(
+                    getFutureTimestamp(50),
+                );
+
+            expect(markedToggles).toStrictEqual(['feature1']);
+            expect(
+                await featureToggleStore.isPotentiallyStale('feature1'),
+            ).toBeTruthy();
+
+            await featureToggleStore.update('default', {
+                name: 'feature1',
+                type: 'kill-switch',
+            });
+
+            const potentiallyStale =
+                await featureToggleStore.isPotentiallyStale('feature1');
+
+            expect(potentiallyStale).toBeFalsy();
+        });
+
+        test('if a fresh feature changes to a type that should be stale, it gets marked as potentially stale', async () => {
+            const features: FeatureToggleDTO[] = [
+                {
+                    name: 'feature1',
+                    type: 'kill-switch',
+                },
+            ];
+            await Promise.all(
+                features.map((feature) =>
+                    featureToggleStore.create('default', feature),
+                ),
+            );
+            const markedToggles =
+                await featureToggleStore.markPotentiallyStaleFeatures(
+                    getFutureTimestamp(50),
+                );
+
+            expect(markedToggles).toStrictEqual([]);
+
+            await featureToggleStore.update('default', {
+                name: 'feature1',
+                type: 'release',
+                createdAt: getPastDate(40),
+            });
+
+            const potentiallyStale =
+                await featureToggleStore.isPotentiallyStale('feature1');
+
+            expect(potentiallyStale).toBeTruthy();
+        });
+
+        test('if a stale feature changes to a type that should be stale, it does not get marked as potentially stale', async () => {
+            const features: FeatureToggleDTO[] = [
+                {
+                    name: 'feature1',
+                    type: 'kill-switch',
+                    stale: true,
+                },
+            ];
+            await Promise.all(
+                features.map((feature) =>
+                    featureToggleStore.create('default', feature),
+                ),
+            );
+
+            await featureToggleStore.update('default', {
+                name: 'feature1',
+                type: 'release',
+                createdAt: getPastDate(40),
+            });
+
+            const potentiallyStale =
+                await featureToggleStore.isPotentiallyStale('feature1');
+
+            expect(potentiallyStale).toBeFalsy();
+        });
+    });
+});

src/test/fixtures/fake-feature-toggle-store.ts

@@ -252,4 +252,12 @@ export default class FakeFeatureToggleStore implements IFeatureToggleStore {
         this.features.forEach((feature) => (feature.variants = []));
         return Promise.resolve();
     }
+
+    markPotentiallyStaleFeatures(): Promise<string[]> {
+        throw new Error('Method not implemented.');
+    }
+
+    isPotentiallyStale(): Promise<boolean> {
+        throw new Error('Method not implemented.');
+    }
 }