Bug fix: Provider should continue to refresh when previous refresh operation didn't complete (#225)

* fix refresh all failure bug

* add testcase

Author: zhiyuanliang-ms

src/appConfigurationImpl.ts

@@ -590,21 +590,19 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration {
      * Updates etag of watched settings from loaded data. If a watched setting is not covered by any selector, a request will be sent to retrieve it.
      */
     async #updateWatchedKeyValuesEtag(existingSettings: ConfigurationSetting[]): Promise<void> {
+        const updatedSentinels: ConfigurationSettingId[] = [];
         for (const sentinel of this.#sentinels) {
             const matchedSetting = existingSettings.find(s => s.key === sentinel.key && s.label === sentinel.label);
             if (matchedSetting) {
-                sentinel.etag = matchedSetting.etag;
+                updatedSentinels.push( {...sentinel, etag: matchedSetting.etag} );
             } else {
                 // Send a request to retrieve key-value since it may be either not loaded or loaded with a different label or different casing
                 const { key, label } = sentinel;
                 const response = await this.#getConfigurationSetting({ key, label });
-                if (response) {
-                    sentinel.etag = response.etag;
-                } else {
-                    sentinel.etag = undefined;
-                }
+                updatedSentinels.push( {...sentinel, etag: response?.etag} );
             }
         }
+        this.#sentinels = updatedSentinels;
     }
 
     /**
@@ -662,7 +660,6 @@ export class AzureAppConfigurationImpl implements AzureAppConfiguration {
             if (response?.statusCode === 200 // created or changed
                 || (response === undefined && sentinel.etag !== undefined) // deleted
             ) {
-                sentinel.etag = response?.etag;// update etag of the sentinel
                 needRefresh = true;
                 break;
             }

test/refresh.test.ts

@@ -25,11 +25,19 @@ function addSetting(key: string, value: any) {
 }
 
 let listKvRequestCount = 0;
+let failNextListKv = false;
 const listKvCallback = () => {
+    if (failNextListKv) {
+        throw new Error("Intended error for test");
+    }
     listKvRequestCount++;
 };
 let getKvRequestCount = 0;
+let failNextGetKv = false;
 const getKvCallback = () => {
+    if (failNextGetKv) {
+        throw new Error("Intended error for test");
+    }
     getKvRequestCount++;
 };
 
@@ -40,6 +48,7 @@ describe("dynamic refresh", function () {
             { value: "red", key: "app.settings.fontColor" },
             { value: "40", key: "app.settings.fontSize" },
             { value: "30", key: "app.settings.fontSize", label: "prod" },
+            { value: "someValue", key: "sentinel" },
             { value: "someValue", key: "TestTagKey", tags: { "env": "dev" } }
         ].map(createMockedKeyValue);
         mockAppConfigurationClientListConfigurationSettings([mockedKVs], listKvCallback);
@@ -48,6 +57,8 @@ describe("dynamic refresh", function () {
 
     afterEach(() => {
         restoreMocks();
+        failNextListKv = false;
+        failNextGetKv = false;
         listKvRequestCount = 0;
         getKvRequestCount = 0;
     });
@@ -239,6 +250,44 @@ describe("dynamic refresh", function () {
         expect(settings.get("app.settings.bgColor")).eq("white");
     });
 
+    it("should continue to refresh when previous refresh-all attempt failed", async () => {
+        const connectionString = createMockedConnectionString();
+        const settings = await load(connectionString, {
+            selectors: [
+                { keyFilter: "app.settings.*" }
+            ],
+            refreshOptions: {
+                enabled: true,
+                refreshIntervalInMs: 2000,
+                watchedSettings: [
+                    { key: "sentinel" }
+                ]
+            }
+        });
+        expect(settings.get("app.settings.fontSize")).eq("40");
+        expect(settings.get("app.settings.fontColor")).eq("red");
+        expect(settings.get("sentinel")).to.be.undefined;
+        expect(listKvRequestCount).eq(1);
+        expect(getKvRequestCount).eq(1); // one getKv request for sentinel key
+
+        // change setting
+        addSetting("app.settings.bgColor", "white");
+        updateSetting("sentinel", "updatedValue");
+        failNextListKv = true; // force next listConfigurationSettings request to fail
+        await sleepInMs(2 * 1000 + 1);
+        await settings.refresh(); // even if the provider detects the sentinel key change, this refresh will fail, so we won't get the updated value of sentinel
+        expect(listKvRequestCount).eq(1);
+        expect(getKvRequestCount).eq(2);
+        expect(settings.get("app.settings.bgColor")).to.be.undefined;
+
+        failNextListKv = false;
+        await sleepInMs(2 * 1000 + 1);
+        await settings.refresh(); // should continue to refresh even if sentinel key doesn't change now
+        expect(listKvRequestCount).eq(2);
+        expect(getKvRequestCount).eq(4);
+        expect(settings.get("app.settings.bgColor")).eq("white");
+    });
+
     it("should execute callbacks on successful refresh", async () => {
         const connectionString = createMockedConnectionString();
         const settings = await load(connectionString, {