workbox-precache caches error responses

[bukharin]

Hi

Library Affected:
workbox-precache

Browser & Platform:
Google Chrome latest

Issue or Feature Request Description:

Workbox precache module caches 404 error response, but it shouldn't, See screenshot:

Using workbox 6.0.2

My sw.js

const precacheUrls = self.__WB_MANIFEST;
if (precacheUrls) {
  precacheAndRoute(precacheUrls);
}

[jeffposnick]

Hello!

The expected behavior is that any responses with 4xx or 5xx statuses should not be precached by default, and cause the service worker installation to fail:

workbox/packages/workbox-precaching/src/PrecacheStrategy.ts

Lines 148 to 154 in 3151c3a

	// Also consider it an error if the user didn't pass their own
	// cacheWillUpdate plugin, and the response is a 400+ (note: this means
	// that by default opaque responses can be precached).
	if (response && response.status >= 400 &&
	!this._usesCustomCacheableResponseLogic()) {
	responseSafeToPrecache = false;
	}

Are you possibly passing in a cacheWillUpdate plugin when you set up precaching? That's the one situation in which you might end up with a 404 response cached. I know that you included a snippet of your service worker code above, but is there any chance that there's more code you didn't include?

[jeffposnick]

...what also leads me to believe that your screenshot either shows a cache created by a much earlier version of Workbox than v6.0.2, or alternatively, that your service worker file includes additional code that you didn't include, is that your precache is named talk-precache-v1.

The default precache name in Workbox v6.0.2 ends in precache-v2:

workbox/packages/workbox-core/src/_private/cacheNames.ts

Lines 30 to 36 in 3151c3a

	const _cacheNameDetails: CacheNameDetails = {
	googleAnalytics: 'googleAnalytics',
	precache: 'precache-v2',
	prefix: 'workbox',
	runtime: 'runtime',
	suffix: typeof registration !== 'undefined' ? registration.scope : '',
	};

[bukharin]

@jeffposnick our project start using Workbox from v5, cache names was not changed. Later project have been upgraded to v6.

This is full sw.js:

const appName = 'talk';

setCacheNameDetails({
  prefix: appName,
  suffix: 'v1',
  precache: 'precache',
  runtime: 'runtime-cache'
});

self.skipWaiting();
self.addEventListener('activate', () => self.clients.claim());

// Cache page navigations (html) with a Network First strategy

registerRoute(
  new NavigationRoute(// Use a Network First caching strategy
    new NetworkFirst({
      plugins: [
        // Ensure that only requests that result in a 200 status are cached
        new CacheableResponsePlugin({
          statuses: [200],
        }),
      ],
    })
  )
);

const precacheUrls = self.__WB_MANIFEST;
if (precacheUrls) {
  precacheAndRoute(precacheUrls);
}

// other routes

[jeffposnick]

Hmm... we do have a test case for this behavior:

workbox/test/workbox-precaching/sw/test-PrecacheStrategy.mjs

Lines 105 to 117 in 3151c3a

	it(`errors on 400+ responses during install if no custom cacheWillUpdate plugin callback is used`, async function() {
	sandbox.stub(self, 'fetch').callsFake((request) => {
	return new Response('Server Error', {status: 400});
	});
	const request = new Request('/index.html');
	const event = new ExtendableEvent('install');
	spyOnEvent(event);
	const ps = new PrecacheStrategy();
	await expectError(
	() => ps.handle({event, request}), 'bad-precaching-response');
	});

I'm surprised by what you're seeing.

Is there perhaps a 3xx redirect involved here? In other words, you include a URL that doesn't exist in your precache manifest, and then the server responds first with a 3xx response pointing to another URL, and then that other URL results in the 404?

Is there any chance you could DM @jeffposnick on Twitter the link to a live version of your site exhibiting this behavior, if you can't share the URL publicly?

[bukharin]

@jeffposnick I can share URL, but the problem not reproduce there. I catched this situation when we deploying a new version and some of our server nodes not received specified js files and return 404 response code.

But I can easy reproduce the problem in test branch by adding unexisting js file path:

const precacheUrls = self.__WB_MANIFEST;
if (precacheUrls) {
  precacheAndRoute([
    ...precacheUrls,
    {"revision": null, "url": "not-found.js"}
  ]);
}

First installation of service-worker fails, but in Cache Storage I could find not-found.js record with 404 response

Service worker was not successfully installed:

But if I refresh the page - the service-worker will be installed successfully and continue to work:

After that SW will response from Cache Storage:

Is it enough information to reproduce the problem? If not I can try to create separate app to illustrate this behaviour.

[bukharin]

How we build SW:

1. We have source sw.js in ES6 format
2. Build and minify it using Rollup - rollup -c rollup-sw.config.js
3. Inject workbox manifest, using workbox-cli workbox injectManifest

rollup-sw.config.js:

import resolve from "rollup-plugin-node-resolve";
import { terser } from "rollup-plugin-terser";
import replace from "rollup-plugin-replace";

export default {
    input: `src/app/service-worker/sw.js`,
    output: [
        {file: 'src/app/service-worker/sw.bundle.js', format: "esm"}
    ],
    plugins: [
        resolve(),
        replace({
            'process.env.NODE_ENV': JSON.stringify('production'),
        }),
        terser()
    ]
};

workbox-config.js`:

module.exports = {
  "globDirectory": "dist/",
  "globPatterns": ["**/*.{css,html,js,json,ttf,woff,woff2}"],
  "globFollow": true,
  "globStrict": true,
  "globIgnores": [
    '**/*-es5.*.js',
    'sw.bundle.js',
    'index.html',
  ],
  "dontCacheBustURLsMatching": new RegExp('.+.[a-f0-9]{20}..+'),
  "maximumFileSizeToCacheInBytes": 5000000,
  "swSrc": "src/app/service-worker/sw.bundle.js",
  "swDest": "dist/sw.bundle.js"
};

[bukharin]

Also I tried to swap steps of bundling and injecting (I think it's more right way), but it has no effect

[jeffposnick]

Ah, thank you for sharing all those steps. I believe this is a regression due to some changes in our precaching logic introduced in Workbox v6:

workbox/packages/workbox-precaching/src/PrecacheStrategy.ts

Lines 142 to 166 in 590d126

	async _handleInstall(request: Request, handler: StrategyHandler) {
	const response = await handler.fetchAndCachePut(request);
	// Any time there's no response, consider it a precaching error.
	let responseSafeToPrecache = Boolean(response);
	// Also consider it an error if the user didn't pass their own
	// cacheWillUpdate plugin, and the response is a 400+ (note: this means
	// that by default opaque responses can be precached).
	if (response && response.status >= 400 &&
	!this._usesCustomCacheableResponseLogic()) {
	responseSafeToPrecache = false;
	}
	if (!responseSafeToPrecache) {
	// Throwing here will lead to the `install` handler failing, which
	// we want to do if *any* of the responses aren't safe to cache.
	throw new WorkboxError('bad-precaching-response', {
	url: request.url,
	status: response.status,
	});
	}
	return response;
	}

That const response = await handler.fetchAndCachePut(request) line should just be a handler.fetch(), and the handler.cachePut() should be deferred until after the responseSafeToPrecache result is confirmed to be true. In the current implementation, we're ending up saving entries to the cache and failing the current install, but the next install attempt succeeds (because we don't attempt to re-cache entries that are already cached unless their revision has changed).

I'll get a fix out for this targeted for the upcoming 6.1.0 release. Thanks for pointing it out!

CC: @philipwalton

[jeffposnick]

This is now fixed in the v6.1.0 release.

[huykon]

I am using version v6.2.4 and the error still there