Don't cache HTTP request errors/exceptions

[aisamu]

What would you like Renovate to be able to do?

HTTP requests have an opt-out caching behavior for all GET and HEAD requests. It appears that errors are also being cached (e.g. when the request ends up failing/throwing), preventing retries to go through down the line.

If you have any ideas on how this should be implemented, please tell us here.

Full context: https://github.com/renovatebot/renovate/pull/17761 The above PR evicts the cache entry for a given request when the request ends up failing/throwing, allowing a retry to go through down the line (e.g. on another renovate run).

The following is a delineation of the reasoning leading us there. The goal of the verbosity is to make incorrect assumptions as explicit as possible:

1. Error message from Renovate logs:

   DEBUG: Failed to look up dependency @xxx/yyy (@xxx/yyy) (packageFile="package.json", dependency="@xxx/yyy")
   DEBUG: Unknown npm lookup error

2. Single occurrence of that error string on the codebase: https://github.com/aisamu/renovate/blob/ef17f8a11103a42ee72b4a819a68204712280c76/lib/modules/datasource/npm/get.ts#L181

   logger.debug({ err }, 'Unknown npm lookup error');

3. First line of the containing try-catch block making an HTTP call (as the error message contains a status code) https://github.com/aisamu/renovate/blob/ef17f8a11103a42ee72b4a819a68204712280c76/lib/modules/datasource/npm/get.ts#L67

   const raw = await http.getJson<NpmResponse>(packageUrl);

4. getJson defers to requestJson, which defers to request: https://github.com/aisamu/renovate/blob/ef17f8a11103a42ee72b4a819a68204712280c76/lib/util/http/index.ts#L212-L232

   getJson<T = unknown>(
   ...
       return this.requestJson<T>(url, { ...options });
   
   private async requestJson<T = unknown>(
   ...
       const res = await this.request<T>(url, {

5. request reads all get and head requests from the cache unless opted out: https://github.com/aisamu/renovate/blob/ef17f8a11103a42ee72b4a819a68204712280c76/lib/util/http/index.ts#L147-L153

    // Cache GET requests unless useCache=false
   if (
     (options.method === 'get' || options.method === 'head') &&
     options.useCache !== false
   ) {
     resPromise = memCache.get(cacheKey);
   }

6. We didn't find useCache being set/overridden on the npm datasource: https://github.com/aisamu/renovate/tree/ef17f8a11103a42ee72b4a819a68204712280c76/lib/modules/datasource/npm/get.ts

7. request caches all get and head promises: https://github.com/aisamu/renovate/blob/ef17f8a11103a42ee72b4a819a68204712280c76/lib/util/http/index.ts#L147-L153

     if (options.method === 'get' || options.method === 'head') {
       memCache.set(cacheKey, resPromise); // always set if it's a get or a head
     }

8. request uses the cached promise if it's non-falsy: https://github.com/aisamu/renovate/blob/ef17f8a11103a42ee72b4a819a68204712280c76/lib/util/http/index.ts#L156

       if (!resPromise) {

9. A rejected promise is non-falsy on node:

   > Promise.reject(new Error()) ? true : false
   true

10. 500 requests are thrown by the http abstraction: https://github.com/aisamu/renovate/blob/ef17f8a11103a42ee72b4a819a68204712280c76/lib/util/http/index.spec.ts#L58-L59

     httpMock.scope(baseUrl).get('/test').reply(500).get('/test').reply(200);
     await expect(http.get('http://renovate.com/test')).rejects.toThrow('500');

The combination of the above would reasonably explain the issue observed on the PR (i.e. never-ending 504s caused by caching a failed response).

Is this a feature you are interested in implementing yourself?

Yes

[viceice]

THisis only a per repo memory cache, so renovate should continue on next run without any issues. so temporary registry issues shouldn't be a problem

[aisamu]

Oh, I see! Would retries on the same repo clear the cache?

[viceice]

not on current run. it will be cleared after each repo.

[rarkins]

It's possible that values are saved to the package cache

[viceice]

It's possible that values are saved to the package cache

i don't think we cache there on error