fix(service-worker): continue serving api requests on cache failure (#32996)

When responses are cached ok during sw initialization,
but caching throws an error when handling api response,
this response never gets to client. Fix response
delivery by catching errors, add logging and 2 test cases.

Fixes #21412

PR Close #32996

packages/service-worker/worker/src/app-version.ts

@@ -11,6 +11,7 @@ import {CacheState, UpdateCacheStatus, UpdateSource} from './api';
 import {AssetGroup, LazyAssetGroup, PrefetchAssetGroup} from './assets';
 import {DataGroup} from './data';
 import {Database} from './database';
+import {DebugHandler} from './debug';
 import {IdleScheduler} from './idle';
 import {Manifest} from './manifest';
 
@@ -60,7 +61,8 @@ export class AppVersion implements UpdateSource {
 
   constructor(
       private scope: ServiceWorkerGlobalScope, private adapter: Adapter, private database: Database,
-      private idle: IdleScheduler, readonly manifest: Manifest, readonly manifestHash: string) {
+      private idle: IdleScheduler, private debugHandler: DebugHandler, readonly manifest: Manifest,
+      readonly manifestHash: string) {
     // The hashTable within the manifest is an Object - convert it to a Map for easier lookups.
     Object.keys(this.manifest.hashTable).forEach(url => {
       this.hashTable.set(url, this.manifest.hashTable[url]);
@@ -85,10 +87,11 @@ export class AppVersion implements UpdateSource {
     });
 
     // Process each `DataGroup` declared in the manifest.
-    this.dataGroups = (manifest.dataGroups || [])
+    this.dataGroups =
+        (manifest.dataGroups || [])
             .map(
                 config => new DataGroup(
-                                  this.scope, this.adapter, config, this.database,
+                    this.scope, this.adapter, config, this.database, this.debugHandler,
                     `${adapter.cacheNamePrefix}:${config.version}:data`));
 
     // This keeps backwards compatibility with app versions without navigation urls.

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

@@ -8,6 +8,7 @@
 
 import {Adapter, Context} from './adapter';
 import {Database, Table} from './database';
+import {DebugHandler} from './debug';
 import {DataGroupConfig} from './manifest';
 
 /**
@@ -243,7 +244,8 @@ export class DataGroup {
 
   constructor(
       private scope: ServiceWorkerGlobalScope, private adapter: Adapter,
-      private config: DataGroupConfig, private db: Database, private prefix: string) {
+      private config: DataGroupConfig, private db: Database, private debugHandler: DebugHandler,
+      private prefix: string) {
     this.patterns = this.config.patterns.map(pattern => new RegExp(pattern));
     this.cache = this.scope.caches.open(`${this.prefix}:dynamic:${this.config.name}:cache`);
     this.lruTable = this.db.open(`${this.prefix}:dynamic:${this.config.name}:lru`);
@@ -356,7 +358,7 @@ export class DataGroup {
       ctx.waitUntil(this.safeCacheResponse(req, networkFetch, lru));
     } else {
       // The request completed in time, so cache it inline with the response flow.
-      await this.cacheResponse(req, res, lru);
+      await this.safeCacheResponse(req, res, lru);
     }
 
     return res;
@@ -385,7 +387,7 @@ export class DataGroup {
       const fromCache = await this.loadFromCache(req, lru);
       res = (fromCache !== null) ? fromCache.res : null;
     } else {
-      await this.cacheResponse(req, res, lru, true);
+      await this.safeCacheResponse(req, res, lru, true);
     }
 
     // Either the network fetch didn't time out, or the cache yielded a usable response.
@@ -442,7 +444,10 @@ export class DataGroup {
       } catch (err) {
         // Saving the API response failed. This could be a result of a full storage.
         // Since this data is cached lazily and temporarily, continue serving clients as usual.
-        // TODO: Log error
+        this.debugHandler.log(
+            err,
+            `DataGroup(${this.config.name}@${this.config.version}).safeCacheResponse(${req.url}, status: ${res.status})`);
+
         // TODO: Better detect/handle full storage; e.g. using
         // [navigator.storage](https://developer.mozilla.org/en-US/docs/Web/API/NavigatorStorage/storage).
       }

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

@@ -533,7 +533,8 @@ export class Driver implements Debuggable, UpdateSource {
       // created for it.
       if (!this.versions.has(hash)) {
         this.versions.set(
-            hash, new AppVersion(this.scope, this.adapter, this.db, this.idle, manifest, hash));
+            hash, new AppVersion(
+                      this.scope, this.adapter, this.db, this.idle, this.debugger, manifest, hash));
       }
     });
 
@@ -783,7 +784,8 @@ export class Driver implements Debuggable, UpdateSource {
   }
 
   private async setupUpdate(manifest: Manifest, hash: string): Promise<void> {
-    const newVersion = new AppVersion(this.scope, this.adapter, this.db, this.idle, manifest, hash);
+    const newVersion =
+        new AppVersion(this.scope, this.adapter, this.db, this.idle, this.debugger, manifest, hash);
 
     // Firstly, check if the manifest version is correct.
     if (manifest.configVersion !== SUPPORTED_CONFIG_VERSION) {

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

@@ -34,6 +34,9 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
           .addFile('/lazy/unchanged2.txt', 'this is unchanged (2)')
           .addUnhashedFile('/unhashed/a.txt', 'this is unhashed', {'Cache-Control': 'max-age=10'})
           .addUnhashedFile('/unhashed/b.txt', 'this is unhashed b', {'Cache-Control': 'no-cache'})
+          .addUnhashedFile('/api/foo', 'this is api foo', {'Cache-Control': 'no-cache'})
+          .addUnhashedFile(
+              '/api-static/bar', 'this is static api bar', {'Cache-Control': 'no-cache'})
           .build();
 
   const distUpdate =
@@ -172,11 +175,21 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
         version: 42,
         maxAge: 3600000,
         maxSize: 100,
-        strategy: 'performance',
+        strategy: 'freshness',
         patterns: [
           '/api/.*',
         ],
       },
+      {
+        name: 'api-static',
+        version: 43,
+        maxAge: 3600000,
+        maxSize: 100,
+        strategy: 'performance',
+        patterns: [
+          '/api-static/.*',
+        ],
+      },
     ],
     navigationUrls: processNavigationUrls(''),
     hashTable: tmpHashTableForFs(dist),
@@ -829,6 +842,9 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
            `ngsw:${baseHref}:42:data:dynamic:api:cache`,
            `ngsw:${baseHref}:db:ngsw:${baseHref}:42:data:dynamic:api:lru`,
            `ngsw:${baseHref}:db:ngsw:${baseHref}:42:data:dynamic:api:age`,
+           `ngsw:${baseHref}:43:data:dynamic:api-static:cache`,
+           `ngsw:${baseHref}:db:ngsw:${baseHref}:43:data:dynamic:api-static:lru`,
+           `ngsw:${baseHref}:db:ngsw:${baseHref}:43:data:dynamic:api-static:age`,
       ];
 
       const getClientAssignments = async(sw: SwTestHarness, baseHref: string) => {
@@ -1230,6 +1246,48 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
         server.assertSawRequestFor('/foo.txt');
       });
 
+      it('keeps serving api requests with freshness strategy when failing to write to cache',
+         async() => {
+           // Initialize the SW.
+           expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
+           await driver.initialized;
+           server.clearRequests();
+
+           // Make the caches unwritable.
+           spyOn(MockCache.prototype, 'put').and.throwError('Can\'t touch this');
+           spyOn(driver.debugger, 'log');
+
+           expect(await makeRequest(scope, '/api/foo')).toEqual('this is api foo');
+           expect(driver.state).toBe(DriverReadyState.NORMAL);
+           // Since we are swallowing an error here, make sure it is at least properly logged
+           expect(driver.debugger.log)
+               .toHaveBeenCalledWith(
+                   new Error('Can\'t touch this'),
+                   'DataGroup(api@42).safeCacheResponse(/api/foo, status: 200)');
+           server.assertSawRequestFor('/api/foo');
+         });
+
+      it('keeps serving api requests with performance strategy when failing to write to cache',
+         async() => {
+           // Initialize the SW.
+           expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
+           await driver.initialized;
+           server.clearRequests();
+
+           // Make the caches unwritable.
+           spyOn(MockCache.prototype, 'put').and.throwError('Can\'t touch this');
+           spyOn(driver.debugger, 'log');
+
+           expect(await makeRequest(scope, '/api-static/bar')).toEqual('this is static api bar');
+           expect(driver.state).toBe(DriverReadyState.NORMAL);
+           // Since we are swallowing an error here, make sure it is at least properly logged
+           expect(driver.debugger.log)
+               .toHaveBeenCalledWith(
+                   new Error('Can\'t touch this'),
+                   'DataGroup(api-static@43).safeCacheResponse(/api-static/bar, status: 200)');
+           server.assertSawRequestFor('/api-static/bar');
+         });
+
       it('enters degraded mode when something goes wrong with the latest version', async() => {
         await driver.initialized;