fix(service-worker): correctly handle failed cache-busted request (#39786)

Since 5be4edfa17120ab1b9c9e8d87b7b539cdd180a25, a failing cache-busted
network request (such as requests for fetching uncached assets) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not necessarily signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.
For more details on the problem and the implemented fix see #39775.

Fixes #39775

PR Close #39786

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

@@ -388,18 +388,17 @@ export abstract class AssetGroup {
       // a stale response.
 
       // Fetch the resource from the network (possibly hitting the HTTP cache).
-      const networkResult = await this.safeFetch(req);
+      let response = await this.safeFetch(req);
 
-      // Decide whether a cache-busted request is necessary. It might be for two independent
+      // Decide whether a cache-busted request is necessary. A cache-busted request is necessary
-      // reasons: either the non-cache-busted request failed (hopefully transiently) or if the
+      // only if the request was successful but the hash of the retrieved contents does not match
-      // hash of the content retrieved does not match the canonical hash from the manifest. It's
+      // the canonical hash from the manifest.
-      // only valid to access the content of the first response if the request was successful.
+      let makeCacheBustedRequest = response.ok;
-      let makeCacheBustedRequest: boolean = !networkResult.ok;
+      if (makeCacheBustedRequest) {
-      if (networkResult.ok) {
         // The request was successful. A cache-busted request is only necessary if the hashes
-        // don't match. Compare them, making sure to clone the response so it can be used later
+        // don't match.
-        // if it proves to be valid.
+        // (Make sure to clone the response so it can be used later if it proves to be valid.)
-        const fetchedHash = sha1Binary(await networkResult.clone().arrayBuffer());
+        const fetchedHash = sha1Binary(await response.clone().arrayBuffer());
         makeCacheBustedRequest = (fetchedHash !== canonicalHash);
       }
 
@@ -411,24 +410,13 @@ export abstract class AssetGroup {
         // request will differentiate these two situations.
         // TODO: handle case where the URL has parameters already (unlikely for assets).
         const cacheBustReq = this.adapter.newRequest(this.cacheBust(req.url));
-        const cacheBustedResult = await this.safeFetch(cacheBustReq);
+        response = await this.safeFetch(cacheBustReq);
-
-        // If the response was unsuccessful, there's nothing more that can be done.
-        if (!cacheBustedResult.ok) {
-          if (cacheBustedResult.status === 404) {
-            throw new SwUnrecoverableStateError(
-                `Failed to retrieve hashed resource from the server. (AssetGroup: ${
-                    this.config.name} | URL: ${url})`);
-          } else {
-            throw new SwCriticalError(
-                `Response not Ok (cacheBustedFetchFromNetwork): cache busted request for ${
-                    req.url} returned response ${cacheBustedResult.status} ${
-                    cacheBustedResult.statusText}`);
-          }
-        }
 
+        // If the response was successful, check the contents against the canonical hash.
+        if (response.ok) {
           // Hash the contents.
-        const cacheBustedHash = sha1Binary(await cacheBustedResult.clone().arrayBuffer());
+          // (Make sure to clone the response so it can be used later if it proves to be valid.)
+          const cacheBustedHash = sha1Binary(await response.clone().arrayBuffer());
 
           // If the cache-busted version doesn't match, then the manifest is not an accurate
           // representation of the server's current set of files, and the SW should give up.
@@ -436,14 +424,20 @@ export abstract class AssetGroup {
             throw new SwCriticalError(`Hash mismatch (cacheBustedFetchFromNetwork): ${
                 req.url}: expected ${canonicalHash}, got ${cacheBustedHash} (after cache busting)`);
           }
-
+        }
-        // If it does match, then use the cache-busted result.
-        return cacheBustedResult;
       }
 
-      // Excellent, the version from the network matched on the first try, with no need for
+      // At this point, `response` is either successful with a matching hash or is unsuccessful.
-      // cache-busting. Use it.
+      // Before returning it, check whether it failed with a 404 status. This would signify an
-      return networkResult;
+      // unrecoverable state.
+      if (!response.ok && (response.status === 404)) {
+        throw new SwUnrecoverableStateError(
+            `Failed to retrieve hashed resource from the server. (AssetGroup: ${
+                this.config.name} | URL: ${url})`);
+      }
+
+      // Return the response (successful or unsuccessful).
+      return response;
     } else {
       // This URL doesn't exist in our hash database, so it must be requested directly.
       return this.safeFetch(req);

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

@@ -794,7 +794,7 @@ describe('Driver', () => {
     serverUpdate.assertNoOtherRequests();
   });
 
-  it('should bypass serviceworker on ngsw-bypass parameter', async () => {
+  it('bypasses the ServiceWorker on `ngsw-bypass` parameter', async () => {
     // NOTE:
     // Requests that bypass the SW are not handled at all in the mock implementation of `scope`,
     // therefore no requests reach the server.
@@ -1115,7 +1115,7 @@ describe('Driver', () => {
       server.assertNoOtherRequests();
     });
 
-    it(`doesn't error when 'Cache-Control' is 'no-cache'`, async () => {
+    it(`don't error when 'Cache-Control' is 'no-cache'`, async () => {
       expect(await makeRequest(scope, '/unhashed/b.txt')).toEqual('this is unhashed b');
       server.assertSawRequestFor('/unhashed/b.txt');
       expect(await makeRequest(scope, '/unhashed/b.txt')).toEqual('this is unhashed b');
@@ -1744,6 +1744,25 @@ describe('Driver', () => {
       expect(requestUrls2).toContain(httpsRequestUrl);
     });
 
+    it('does not enter degraded mode when offline while fetching an uncached asset', async () => {
+      // Trigger SW initialization and wait for it to complete.
+      expect(await makeRequest(scope, '/foo.txt')).toBe('this is foo');
+      await driver.initialized;
+
+      // Request an uncached asset while offline.
+      // The SW will not be able to get the content, but it should not enter a degraded mode either.
+      server.online = false;
+      await expectAsync(makeRequest(scope, '/baz.txt'))
+          .toBeRejectedWithError(
+              'Response not Ok (fetchAndCacheOnce): request for /baz.txt returned response 504 Gateway Timeout');
+      expect(driver.state).toBe(DriverReadyState.NORMAL);
+
+      // Once we are back online, everything should work as expected.
+      server.online = true;
+      expect(await makeRequest(scope, '/baz.txt')).toBe('this is baz');
+      expect(driver.state).toBe(DriverReadyState.NORMAL);
+    });
+
     describe('unrecoverable state', () => {
       const generateMockServerState = (fileSystem: MockFileSystem) => {
         const manifest: Manifest = {