fix(service-worker): ensure obsolete caches are always cleaned up (#42622)

Previously, the SW was only able to clean up caches for app-versions
found in the `Driver`'s `versions` map. If (for some reason) the
`Driver` failed to load a valid stored state (including app-versions)
and ended up with an [empty `versions` map][1], any obsolete versions
would remain in the cache storage. This case was rare but possible.

This commit makes the cache clean-up logic more robust by ensuring that
all app-version caches are removed unless they are currently used by the
SW to serve active clients (with the exception of the latest
app-version, which is always retained).

Fixes #41728

[1]: 9de65dbdce/packages/service-worker/worker/src/driver.ts (L515-L529)

PR Close #42622

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

@@ -284,11 +284,14 @@ export class AppVersion implements UpdateSource {
   }
 
   /**
-   * Erase this application version, by cleaning up all the caches.
+   * Return a list of the names of all caches used by this version.
    */
-  async cleanup(): Promise<void> {
+  async getCacheNames(): Promise<string[]> {
-    await Promise.all(this.assetGroups.map(group => group.cleanup()));
+    const allGroupCacheNames = await Promise.all([
-    await Promise.all(this.dataGroups.map(group => group.cleanup()));
+      ...this.assetGroups.map(group => group.getCacheNames()),
+      ...this.dataGroups.map(group => group.getCacheNames()),
+    ]);
+    return ([] as string[]).concat(...allGroupCacheNames);
   }
 
   /**

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

@@ -9,6 +9,7 @@
 import {Adapter, Context} from './adapter';
 import {CacheState, NormalizedUrl, UpdateCacheStatus, UpdateSource, UrlMetadata} from './api';
 import {Database, Table} from './database';
+import {CacheTable} from './db-cache';
 import {errorToString, SwCriticalError, SwUnrecoverableStateError} from './error';
 import {IdleScheduler} from './idle';
 import {AssetGroupConfig} from './manifest';
@@ -100,13 +101,14 @@ export abstract class AssetGroup {
   abstract initializeFully(updateFrom?: UpdateSource): Promise<void>;
 
   /**
-   * Clean up all the cached data for this group.
+   * Return a list of the names of all caches used by this group.
    */
-  async cleanup(): Promise<void> {
+  async getCacheNames(): Promise<string[]> {
-    await Promise.all([
+    const [cache, metadata] = await Promise.all([
-      this.cache.then(cache => this.adapter.caches.delete(cache.name)),
+      this.cache,
-      this.metadata.then(metadata => this.db.delete(metadata.name)),
+      this.metadata as Promise<CacheTable>,
     ]);
+    return [cache.name, metadata.cacheName];
   }
 
   /**

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

@@ -8,6 +8,7 @@
 
 import {Adapter, Context} from './adapter';
 import {Database, Table} from './database';
+import {CacheTable} from './db-cache';
 import {DebugHandler} from './debug';
 import {DataGroupConfig} from './manifest';
 import {NamedCache} from './named-cache-storage';
@@ -554,6 +555,17 @@ export class DataGroup {
       this.lruTable.then(table => this.db.delete(table.name)),
     ]);
   }
+  /**
+   * Return a list of the names of all caches used by this group.
+   */
+  async getCacheNames(): Promise<string[]> {
+    const [cache, ageTable, lruTable] = await Promise.all([
+      this.cache,
+      this.ageTable as Promise<CacheTable>,
+      this.lruTable as Promise<CacheTable>,
+    ]);
+    return [cache.name, ageTable.cacheName, lruTable.cacheName];
+  }
 
   /**
    * Clear the state of the cache for a particular resource.

packages/service-worker/worker/src/db-cache.ts

@@ -8,6 +8,7 @@
 
 import {Adapter} from './adapter';
 import {Database, NotFound, Table} from './database';
+import {NamedCache} from './named-cache-storage';
 
 
 /**
@@ -51,8 +52,10 @@ export class CacheDatabase implements Database {
  * A `Table` backed by a `Cache`.
  */
 export class CacheTable implements Table {
+  cacheName = this.cache.name;
+
   constructor(
-      readonly name: string, private cache: Cache, private adapter: Adapter,
+      readonly name: string, private cache: NamedCache, private adapter: Adapter,
       private cacheQueryOptions?: CacheQueryOptions) {}
 
   private request(key: string): Request {

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

@@ -10,6 +10,7 @@ import {Adapter} from './adapter';
 import {CacheState, Debuggable, DebugIdleState, DebugState, DebugVersion, NormalizedUrl, UpdateCacheStatus, UpdateSource} from './api';
 import {AppVersion} from './app-version';
 import {Database} from './database';
+import {CacheTable} from './db-cache';
 import {DebugHandler} from './debug';
 import {errorToString} from './error';
 import {IdleScheduler} from './idle';
@@ -110,6 +111,9 @@ export class Driver implements Debuggable, UpdateSource {
 
   debugger: DebugHandler;
 
+  // A promise resolving to the control DB table.
+  private controlTable = this.db.open('control');
+
   constructor(
       private scope: ServiceWorkerGlobalScope, private adapter: Adapter, private db: Database) {
     // Set up all the event handlers that the SW needs.
@@ -517,8 +521,7 @@ export class Driver implements Debuggable, UpdateSource {
     // the SW has run or the DB state has been wiped or is inconsistent. In that case,
     // load a fresh copy of the manifest and reset the state from scratch.
 
-    // Open up the DB table.
+    const table = await this.controlTable;
-    const table = await this.db.open('control');
 
     // Attempt to load the needed state from the DB. If this fails, the catch {} block
     // will populate these variables with freshly constructed values.
@@ -568,12 +571,7 @@ export class Driver implements Debuggable, UpdateSource {
 
     // Schedule cleaning up obsolete caches in the background.
     this.idle.schedule('init post-load (cleanup)', async () => {
-      try {
       await this.cleanupCaches();
-      } catch (err) {
-        // Nothing to do - cleanup failed. Just log it.
-        this.debugger.log(err, 'cleanupCaches @ init post-load');
-      }
     });
 
     // Initialize the `versions` map by setting each hash to a new `AppVersion` instance
@@ -901,8 +899,7 @@ export class Driver implements Debuggable, UpdateSource {
    * Synchronize the existing state to the underlying database.
    */
   private async sync(): Promise<void> {
-    // Open up the DB table.
+    const table = await this.controlTable;
-    const table = await this.db.open('control');
 
     // Construct a serializable map of hashes to manifests.
     const manifests: ManifestMap = {};
@@ -931,25 +928,23 @@ export class Driver implements Debuggable, UpdateSource {
   }
 
   async cleanupCaches(): Promise<void> {
-    // Query for all currently active clients, and list the client ids. This may skip
+    try {
-    // some clients in the browser back-forward cache, but not much can be done about
+      // Query for all currently active clients, and list the client IDs. This may skip some clients
-    // that.
+      // in the browser back-forward cache, but not much can be done about that.
-    const activeClients: ClientId[] =
+      const activeClients =
-        (await this.scope.clients.matchAll()).map(client => client.id);
+          new Set<ClientId>((await this.scope.clients.matchAll()).map(client => client.id));
 
-    // A simple list of client ids that the SW has kept track of. Subtracting
+      // A simple list of client IDs that the SW has kept track of. Subtracting `activeClients` from
-    // activeClients from this list will result in the set of client ids which are
+      // this list will result in the set of client IDs which are being tracked but are no longer
-    // being tracked but are no longer used in the browser, and thus can be cleaned up.
+      // used in the browser, and thus can be cleaned up.
       const knownClients: ClientId[] = Array.from(this.clientVersionMap.keys());
 
-    // Remove clients in the clientVersionMap that are no longer active.
+      // Remove clients in the `clientVersionMap` that are no longer active.
-    knownClients.filter(id => activeClients.indexOf(id) === -1)
+      const obsoleteClients = knownClients.filter(id => !activeClients.has(id));
-        .forEach(id => this.clientVersionMap.delete(id));
+      obsoleteClients.forEach(id => this.clientVersionMap.delete(id));
 
-    // Next, determine the set of versions which are still used. All others can be
+      // Next, determine the set of versions which are still used. All others can be removed.
-    // removed.
+      const usedVersions = new Set(this.clientVersionMap.values());
-    const usedVersions = new Set<string>();
-    this.clientVersionMap.forEach((version, _) => usedVersions.add(version));
 
       // Collect all obsolete versions by filtering out used versions from the set of all versions.
       const obsoleteVersions =
@@ -957,30 +952,21 @@ export class Driver implements Debuggable, UpdateSource {
               .filter(version => !usedVersions.has(version) && version !== this.latestHash);
 
       // Remove all the versions which are no longer used.
-    await obsoleteVersions.reduce(async (previous, version) => {
+      obsoleteVersions.forEach(version => this.versions.delete(version));
-      // Wait for the other cleanup operations to complete.
-      await previous;
-
-      // Try to get past the failure of one particular version to clean up (this
-      // shouldn't happen, but handle it just in case).
-      try {
-        // Get ahold of the AppVersion for this particular hash.
-        const instance = this.versions.get(version)!;
-
-        // Delete it from the canonical map.
-        this.versions.delete(version);
-
-        // Clean it up.
-        await instance.cleanup();
-      } catch (err) {
-        // Oh well? Not much that can be done here. These caches will be removed when
-        // the SW revs its format version, which happens from time to time.
-        this.debugger.log(err, `cleanupCaches - cleanup ${version}`);
-      }
-    }, Promise.resolve());
 
       // Commit all the changes to the saved state.
       await this.sync();
+
+      // Delete all caches that are no longer needed.
+      const allCaches = await this.adapter.caches.keys();
+      const usedCaches = new Set(await this.getCacheNames());
+      const cachesToDelete = allCaches.filter(name => !usedCaches.has(name));
+      await Promise.all(cachesToDelete.map(name => this.adapter.caches.delete(name)));
+    } catch (err) {
+      // Oh well? Not much that can be done here. These caches will be removed on the next attempt
+      // or when the SW revs its format version, which happens from time to time.
+      this.debugger.log(err, 'cleanupCaches');
+    }
   }
 
   /**
@@ -1150,4 +1136,12 @@ export class Driver implements Debuggable, UpdateSource {
       });
     }
   }
+
+  private async getCacheNames(): Promise<string[]> {
+    const controlTable = await this.controlTable as CacheTable;
+    const appVersions = Array.from(this.versions.values());
+    const appVersionCacheNames =
+        await Promise.all(appVersions.map(version => version.getCacheNames()));
+    return [controlTable.cacheName].concat(...appVersionCacheNames);
+  }
 }

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

@@ -698,6 +698,42 @@ describe('Driver', () => {
     expect(hasOriginalCaches).toEqual(false);
   });
 
+  it('cleans up properly when failing to load stored state', async () => {
+    // Initialize the SW and cache the original app-version.
+    expect(await makeRequest(scope, '/foo.txt')).toBe('this is foo');
+    await driver.initialized;
+
+    // Update and cache the updated app-version.
+    scope.updateServerState(serverUpdate);
+    expect(await driver.checkForUpdate()).toBeTrue();
+    expect(await makeRequest(scope, '/foo.txt', 'newClient')).toBe('this is foo v2');
+
+    // Verify both app-versions are stored in the cache.
+    let cacheNames = await scope.caches.keys();
+    let hasOriginalVersion = cacheNames.some(name => name.startsWith(`${manifestHash}:`));
+    let hasUpdatedVersion = cacheNames.some(name => name.startsWith(`${manifestUpdateHash}:`));
+    expect(hasOriginalVersion).withContext('Has caches for original version').toBeTrue();
+    expect(hasUpdatedVersion).withContext('Has caches for updated version').toBeTrue();
+
+    // Simulate failing to load the stored state (and thus starting from an empty state).
+    scope.caches.delete('db:control');
+    driver = new Driver(scope, scope, new CacheDatabase(scope));
+
+    expect(await makeRequest(scope, '/foo.txt')).toBe('this is foo v2');
+    await driver.initialized;
+
+    // Verify that the caches for the obsolete original version are cleaned up.
+    // await driver.cleanupCaches();
+    scope.advance(6000);
+    await driver.idle.empty;
+
+    cacheNames = await scope.caches.keys();
+    hasOriginalVersion = cacheNames.some(name => name.startsWith(`${manifestHash}:`));
+    hasUpdatedVersion = cacheNames.some(name => name.startsWith(`${manifestUpdateHash}:`));
+    expect(hasOriginalVersion).withContext('Has caches for original version').toBeFalse();
+    expect(hasUpdatedVersion).withContext('Has caches for updated version').toBeTrue();
+  });
+
   it('shows notifications for push notifications', async () => {
     expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
     await driver.initialized;
@@ -1481,6 +1517,7 @@ describe('Driver', () => {
       // Another 6 seconds.
       scope.advance(6000);
       await driver.idle.empty;
+      await new Promise(resolve => setTimeout(resolve));  // Wait for async operations to complete.
       serverUpdate.assertSawRequestFor('/unhashed/a.txt');
 
       // Now the new version of the resource should be served.
@@ -1817,16 +1854,19 @@ describe('Driver', () => {
         'ngsuu:active',
         'not:ngsw:active',
         'NgSw:StAgEd',
-        'ngsw:/:active',
+        'ngsw:/:db:control',
-        'ngsw:/foo/:staged',
+        'ngsw:/foo/:active',
+        'ngsw:/bar/:staged',
       ];
       const allCacheNames = oldSwCacheNames.concat(otherCacheNames);
 
       await Promise.all(allCacheNames.map(name => scope.caches.original.open(name)));
-      expect(await scope.caches.original.keys()).toEqual(allCacheNames);
+      expect(await scope.caches.original.keys())
+          .toEqual(jasmine.arrayWithExactContents(allCacheNames));
 
       await driver.cleanupOldSwCaches();
-      expect(await scope.caches.original.keys()).toEqual(otherCacheNames);
+      expect(await scope.caches.original.keys())
+          .toEqual(jasmine.arrayWithExactContents(otherCacheNames));
     });
 
     it('should delete other caches even if deleting one of them fails', async () => {