fix(service-worker): do not unassign clients from a broken version (#…

…43518)

Previously, when a version was found to be broken, any clients assigned
to that version were unassigned (and either assigned to the latest
version or to none if the latest version was the broken one). A version
could be considered broken for several reasons, but most often it is a
response for a hashed asset that eiher does not exist or contains
different content than the SW expects. See
#28114 (comment)
for more details.

However, assigning a client to a different version (or the network) in
the middle of a session, turned out to be more risky than keeping it on
the same version. For angular.io, for example, it has led to #28114.

This commit avoids making things worse when identifying a broken version
by keeping existing clients to their assigned version (but ensuring that
no new clients are assigned to the broken version).

NOTE:
Reloading the page generates a new client ID, so it is like a new client
for the SW, even if the tab and URL are the same.

PR Close #43518

packages/service-worker/worker/src/driver.ts

@@ -481,7 +481,8 @@ export class Driver implements Debuggable, UpdateSource {
             await this.notifyClientsAboutUnrecoverableState(appVersion, err.message);
           }
           if (err.isCritical) {
-            // Something went wrong with the activation of this version.
+            // Something went wrong with handling the request from this version.
+            this.debugger.log(err, `Driver.handleFetch(version: ${appVersion.manifestHash})`);
             await this.versionFailed(appVersion, err);
             return this.safeFetch(event.request);
           }
@@ -800,36 +801,32 @@ export class Driver implements Debuggable, UpdateSource {
     }
 
     const brokenHash = broken[0];
-    const affectedClients = Array.from(this.clientVersionMap.entries())
-                                .filter(([clientId, hash]) => hash === brokenHash)
-                                .map(([clientId]) => clientId);
+
+    // The specified version is broken and new clients should not be served from it. However, it is
+    // deemed even riskier to switch the existing clients to a different version or to the network.
+    // Therefore, we keep clients on their current version (even if broken) and ensure that no new
+    // clients will be assigned to it.
 
     // TODO: notify affected apps.
 
     // The action taken depends on whether the broken manifest is the active (latest) or not.
-    // If so, the SW cannot accept new clients, but can continue to service old ones.
+    // - If the broken version is not the latest, no further action is necessary, since new clients
+    //   will be assigned to the latest version anyway.
+    // - If the broken version is the latest, the SW cannot accept new clients (but can continue to
+    //   service old ones).
     if (this.latestHash === brokenHash) {
-      // The latest manifest is broken. This means that new clients are at the mercy of the
-      // network, but caches continue to be valid for previous versions. This is
-      // unfortunate but unavoidable.
+      // The latest manifest is broken. This means that new clients are at the mercy of the network,
+      // but caches continue to be valid for previous versions. This is unfortunate but unavoidable.
       this.state = DriverReadyState.EXISTING_CLIENTS_ONLY;
       this.stateMessage = `Degraded due to: ${errorToString(err)}`;
 
-      // Cancel the binding for the affected clients.
-      affectedClients.forEach(clientId => this.clientVersionMap.delete(clientId));
-    } else {
-      // The latest version is viable, but this older version isn't. The only
-      // possible remedy is to stop serving the older version and go to the network.
-      // Put the affected clients on the latest version.
-      affectedClients.forEach(clientId => this.clientVersionMap.set(clientId, this.latestHash!));
-    }
-
-    try {
-      await this.sync();
-    } catch (err2) {
-      // We are already in a bad state. No need to make things worse.
-      // Just log the error and move on.
-      this.debugger.log(err2, `Driver.versionFailed(${err.message || err})`);
+      try {
+        await this.sync();
+      } catch (err2) {
+        // We are already in a bad state. No need to make things worse.
+        // Just log the error and move on.
+        this.debugger.log(err2, `Driver.versionFailed(${err.message || err})`);
+      }
     }
   }
 

packages/service-worker/worker/test/happy_spec.ts

@@ -2048,16 +2048,73 @@ describe('Driver', () => {
       brokenLazyServer.assertSawRequestFor('/bar.txt');
       brokenLazyServer.clearRequests();
 
-      // `client1` should now be served from the network.
+      // `client1` should still be served from the latest (broken) version.
       expect(await makeRequest(scope, '/foo.txt', 'client1')).toBe('this is foo (broken)');
-      brokenLazyServer.assertSawRequestFor('/foo.txt');
+      brokenLazyServer.assertNoOtherRequests();
 
       // `client2` should still be served from the old version (since it never updated).
       expect(await makeRequest(scope, '/foo.txt', 'client2')).toBe('this is foo');
       server.assertNoOtherRequests();
       brokenLazyServer.assertNoOtherRequests();
+
+      // New clients should be served from the network.
+      expect(await makeRequest(scope, '/foo.txt', 'client3')).toBe('this is foo (broken)');
+      brokenLazyServer.assertSawRequestFor('/foo.txt');
     });
 
+    it('enters does not enter degraded mode when something goes wrong with an older version',
+       async () => {
+         await driver.initialized;
+
+         // Three clients on initial version.
+         expect(await makeRequest(scope, '/foo.txt', 'client1')).toBe('this is foo');
+         expect(await makeRequest(scope, '/foo.txt', 'client2')).toBe('this is foo');
+         expect(await makeRequest(scope, '/foo.txt', 'client3')).toBe('this is foo');
+
+         // Install a broken version (`bar.txt` has invalid hash).
+         scope.updateServerState(brokenLazyServer);
+         await driver.checkForUpdate();
+
+         // Update `client1` and `client2` but not `client3`.
+         await makeNavigationRequest(scope, '/', 'client1');
+         await makeNavigationRequest(scope, '/', 'client2');
+         server.clearRequests();
+         brokenLazyServer.clearRequests();
+
+         expect(await makeRequest(scope, '/foo.txt', 'client1')).toBe('this is foo (broken)');
+         expect(await makeRequest(scope, '/foo.txt', 'client2')).toBe('this is foo (broken)');
+         expect(await makeRequest(scope, '/foo.txt', 'client3')).toBe('this is foo');
+         server.assertNoOtherRequests();
+         brokenLazyServer.assertNoOtherRequests();
+
+         // Install a newer, non-broken version.
+         scope.updateServerState(serverUpdate);
+         await driver.checkForUpdate();
+
+         // Update `client1` bot not `client2` or `client3`.
+         await makeNavigationRequest(scope, '/', 'client1');
+         expect(await makeRequest(scope, '/foo.txt', 'client1')).toBe('this is foo v2');
+
+         // Trying to fetch `bar.txt` (which has an invalid hash on the broken version) from
+         // `client2` should invalidate that particular version (which is not the latest one).
+         // (NOTE: Since the file is not cached locally, it is fetched from the server.)
+         expect(await makeRequest(scope, '/bar.txt', 'client2')).toBe('this is bar');
+         expect(driver.state).toBe(DriverReadyState.NORMAL);
+         serverUpdate.clearRequests();
+
+         // Existing clients should still be served from their assigned versions.
+         expect(await makeRequest(scope, '/foo.txt', 'client1')).toBe('this is foo v2');
+         expect(await makeRequest(scope, '/foo.txt', 'client2')).toBe('this is foo (broken)');
+         expect(await makeRequest(scope, '/foo.txt', 'client3')).toBe('this is foo');
+         server.assertNoOtherRequests();
+         brokenLazyServer.assertNoOtherRequests();
+         serverUpdate.assertNoOtherRequests();
+
+         // New clients should be served from the latest version.
+         expect(await makeRequest(scope, '/foo.txt', 'client4')).toBe('this is foo v2');
+         serverUpdate.assertNoOtherRequests();
+       });
+
     it('recovers from degraded `EXISTING_CLIENTS_ONLY` mode as soon as there is a valid update',
        async () => {
          await driver.initialized;