fix(service-worker): keep serving clients on older versions if latest…

… is invalidated (angular#31865)

Previously, when the latest version was invalidated (e.g. due to a hash
mismatch), the SW entered a degraded `EXISTING_CLIENTS_ONLY` mode and
removed _all_ clients from its client-version map (essentially stopping
to serve any clients). Based on the code and surrounding comments, the
intention seems to have been to only remove clients that were on the
invalidated version, but keep other clients on older versions.

This commit fixes it by only unassigning clients what were on the latest
version and keep clients assigned to older versions.

PR Close angular#31865

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

@@ -742,11 +742,16 @@ export class Driver implements Debuggable, UpdateSource {
     // This particular AppVersion is broken. First, find the manifest hash.
     const broken =
         Array.from(this.versions.entries()).find(([hash, version]) => version === appVersion);
+
     if (broken === undefined) {
       // This version is no longer in use anyway, so nobody cares.
       return;
     }
+
     const brokenHash = broken[0];
+    const affectedClients = Array.from(this.clientVersionMap.entries())
+                                .filter(([clientId, hash]) => hash === brokenHash)
+                                .map(([clientId]) => clientId);
 
     // TODO: notify affected apps.
 
@@ -759,17 +764,12 @@ export class Driver implements Debuggable, UpdateSource {
       this.state = DriverReadyState.EXISTING_CLIENTS_ONLY;
       this.stateMessage = `Degraded due to: ${errorToString(err)}`;
 
-      // Cancel the binding for these clients.
-      Array.from(this.clientVersionMap.keys())
-          .forEach(clientId => this.clientVersionMap.delete(clientId));
+      // Cancel the binding for the affected clients.
+      affectedClients.forEach(clientId => this.clientVersionMap.delete(clientId));
     } else {
-      // The current version is viable, but this older version isn't. The only
+      // 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.
-      // Figure out which clients are affected and put them on the latest.
-      const affectedClients =
-          Array.from(this.clientVersionMap.keys())
-              .filter(clientId => this.clientVersionMap.get(clientId) ! === brokenHash);
-      // Push the affected clients onto the latest version.
+      // Put the affected clients on the latest version.
       affectedClients.forEach(clientId => this.clientVersionMap.set(clientId, this.latestHash !));
     }
 

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

@@ -52,7 +52,10 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
           .addUnhashedFile('/ignored/dir/file2', 'this is not handled by the SW either')
           .build();
 
-  const brokenFs = new MockFileSystemBuilder().addFile('/foo.txt', 'this is foo').build();
+  const brokenFs = new MockFileSystemBuilder()
+                       .addFile('/foo.txt', 'this is foo (broken)')
+                       .addFile('/bar.txt', 'this is bar (broken)')
+                       .build();
 
   const brokenManifest: Manifest = {
     configVersion: 1,
@@ -72,6 +75,35 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
     hashTable: tmpHashTableForFs(brokenFs, {'/foo.txt': true}),
   };
 
+  const brokenLazyManifest: Manifest = {
+    configVersion: 1,
+    timestamp: 1234567890123,
+    index: '/foo.txt',
+    assetGroups: [
+      {
+        name: 'assets',
+        installMode: 'prefetch',
+        updateMode: 'prefetch',
+        urls: [
+          '/foo.txt',
+        ],
+        patterns: [],
+      },
+      {
+        name: 'lazy-assets',
+        installMode: 'lazy',
+        updateMode: 'lazy',
+        urls: [
+          '/bar.txt',
+        ],
+        patterns: [],
+      },
+    ],
+    dataGroups: [],
+    navigationUrls: processNavigationUrls(''),
+    hashTable: tmpHashTableForFs(brokenFs, {'/bar.txt': true}),
+  };
+
   // Manifest without navigation urls to test backward compatibility with
   // versions < 6.0.0.
   interface ManifestV5 {
@@ -226,6 +258,11 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
   const brokenServer =
       new MockServerStateBuilder().withStaticFiles(brokenFs).withManifest(brokenManifest).build();
 
+  const brokenLazyServer = new MockServerStateBuilder()
+                               .withStaticFiles(brokenFs)
+                               .withManifest(brokenLazyManifest)
+                               .build();
+
   const server404 = new MockServerStateBuilder().withStaticFiles(dist).build();
 
   const manifestHash = sha1(JSON.stringify(manifest));
@@ -1150,7 +1187,7 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
         (scope.registration as any).scope = 'http://site.com';
         driver = new Driver(scope, scope, new CacheDatabase(scope, scope));
 
-        expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
+        expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo (broken)');
       });
 
       it('enters degraded mode when update has a bad index', async() => {
@@ -1193,6 +1230,45 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
         server.assertSawRequestFor('/foo.txt');
       });
 
+      it('enters degraded mode when something goes wrong with the latest version', async() => {
+        await driver.initialized;
+
+        // Two 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');
+
+        // Install a broken version (`bar.txt` has invalid hash).
+        scope.updateServerState(brokenLazyServer);
+        await driver.checkForUpdate();
+
+        // Update `client1` but not `client2`.
+        await makeNavigationRequest(scope, '/', 'client1');
+        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');
+        server.assertNoOtherRequests();
+        brokenLazyServer.assertNoOtherRequests();
+
+        // Trying to fetch `bar.txt` (which has an invalid hash) should invalidate the latest
+        // version, enter degraded mode and "forget" clients that are on that version (i.e.
+        // `client1`).
+        expect(await makeRequest(scope, '/bar.txt', 'client1')).toBe('this is bar (broken)');
+        expect(driver.state).toBe(DriverReadyState.EXISTING_CLIENTS_ONLY);
+        brokenLazyServer.sawRequestFor('/bar.txt');
+        brokenLazyServer.clearRequests();
+
+        // `client1` should not be served from the network.
+        expect(await makeRequest(scope, '/foo.txt', 'client1')).toBe('this is foo (broken)');
+        brokenLazyServer.sawRequestFor('/foo.txt');
+
+        // `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();
+      });
+
       it('ignores invalid `only-if-cached` requests ', async() => {
         const requestFoo = (cache: RequestCache | 'only-if-cached', mode: RequestMode) =>
             makeRequest(scope, '/foo.txt', undefined, {cache, mode});
@@ -1234,7 +1310,7 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
         expect(requestUrls2).toContain(httpsRequestUrl);
       });
 
-      describe('Backwards compatibility with v5', () => {
+      describe('backwards compatibility with v5', () => {
         beforeEach(() => {
           const serverV5 = new MockServerStateBuilder()
                                .withStaticFiles(dist)
@@ -1246,7 +1322,7 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
         });
 
         // Test this bug: https://github.com/angular/angular/issues/27209
-        it('Fill previous versions of manifests with default navigation urls for backwards compatibility',
+        it('fills previous versions of manifests with default navigation urls for backwards compatibility',
            async() => {
              expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
              await driver.initialized;