Repository: anuraghazra/github-readme-stats
Pull request #2448 (2023-01-21 09:12:04)

feat: rate limit error chaching
Rate limit error caching to alleviate PATs.


vercel[bot]: [vc]: #VvI5JhFhyrRNZlq4Tf18cTpBJrcoZy21qOgE8fZvu48=:eyJpc01vbm9yZXBvIjp0cnVlLCJ0eXBlIjoiZ2l0aHViIiwicHJvamVjdHMiOlt7Im5hbWUiOiJnaXRodWItcmVhZG1lLXN0YXRzIiwiaW5zcGVjdG9yVXJsIjoiaHR0cHM6Ly92ZXJjZWwuY29tL2dpdGh1Yi1yZWFkbWUtc3RhdHMtdGVhbS9naXRodWItcmVhZG1lLXN0YXRzL0g2b1BoSkI4WUo4QXFFN2VwZGJ0YllSVzZBRHQiLCJwcmV2aWV3VXJsIjoiIiwibmV4dENvbW1pdFN0YXR1cyI6IlBFTkRJTkcifV19
**The latest updates on your projects**. Learn more about [Vercel for Git ↗︎](https://vercel.link/github-learn-more)

| Name | Status | Preview | Updated |
| :--- | :----- | :------ | :------ |
| **github-readme-stats** | 🔄 Building ([Inspect](https://vercel.com/github-readme-stats-team/github-readme-stats/H6oPhJB8YJ8AqE7epdbtbYRW6ADt)) |  | Jan 21, 2023 at 9:12AM (UTC) |


codecov[bot]: # [Codecov](https://codecov.io/gh/anuraghazra/github-readme-stats/pull/2448?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra) Report
Base: **96.75**% // Head: **96.77**% // Increases project coverage by **`+0.02%`** :tada:
> Coverage data is based on head [(`58e9f9c`)](https://codecov.io/gh/anuraghazra/github-readme-stats/pull/2448?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra) compared to base [(`8bc69e7`)](https://codecov.io/gh/anuraghazra/github-readme-stats/commit/8bc69e761ff7c922f47e6561d7a2f4fd0cc153c7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra).
> Patch coverage: 100.00% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #2448      +/-   ##
==========================================
+ Coverage   96.75%   96.77%   +0.02%     
==========================================
  Files          22       22              
  Lines        3883     3908      +25     
  Branches      329      329              
==========================================
+ Hits         3757     3782      +25     
  Misses        124      124              
  Partials        2        2              
```


| [Impacted Files](https://codecov.io/gh/anuraghazra/github-readme-stats/pull/2448?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra) | Coverage Δ | |
|---|---|---|
| [api/index.js](https://codecov.io/gh/anuraghazra/github-readme-stats/pull/2448?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra#diff-YXBpL2luZGV4Lmpz) | `96.15% <100.00%> (+0.19%)` | :arrow_up: |
| [api/pin.js](https://codecov.io/gh/anuraghazra/github-readme-stats/pull/2448?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra#diff-YXBpL3Bpbi5qcw==) | `93.40% <100.00%> (+0.38%)` | :arrow_up: |
| [api/top-langs.js](https://codecov.io/gh/anuraghazra/github-readme-stats/pull/2448?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra#diff-YXBpL3RvcC1sYW5ncy5qcw==) | `95.60% <100.00%> (+0.25%)` | :arrow_up: |
| [src/common/retryer.js](https://codecov.io/gh/anuraghazra/github-readme-stats/pull/2448?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra#diff-c3JjL2NvbW1vbi9yZXRyeWVyLmpz) | `75.00% <100.00%> (ø)` | |
| [src/common/utils.js](https://codecov.io/gh/anuraghazra/github-readme-stats/pull/2448?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra#diff-c3JjL2NvbW1vbi91dGlscy5qcw==) | `100.00% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/anuraghazra/github-readme-stats/pull/2448?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Anurag+Hazra).

rickstaa: Maybe let's change this to using `if-stale-error` https://developer.fastly.com/learning/concepts/stale/#:~:text=stale%2Dif%2Derror%20tells%20Fastly,during%20periods%20of%20server%20instability (see image below).

![image](https://user-images.githubusercontent.com/17570430/213879784-282d756f-5b48-4a94-a25a-df817727d907.png)

We can then throw a Vercel error instead of displaying the `max tries` error on the card and let the browser serve the card from the cache (see image below).

https://developer.fastly.com/learning/concepts/stale/#:~:text=stale%2Dif%2Derror%20tells%20Fastly,during%20periods%20of%20server%20instability
anuraghazra: Yes, but let's test it on dummy deployment before going to prod with this
rickstaa: > Yes, but let's test it on dummy deployment before going to prod with this

100% 👍🏻.
rickstaa: > Yes, but let's test it on dummy deployment before going to prod with this

I currently do not have the time to implement this, but it should be doable if we find a way to throw a Vercel error. We can then catch the `MAX_RETRY` and possibly `REST_API_ERROR` (see https://github.com/anuraghazra/github-readme-stats/pull/2177) in the API endpoints and throw this Vercel error. If we add `if-stale-error` to the header, the cards should be served from the cache if the GraphQL or Rest APIs timeout.
rickstaa: Not sure if `stale-if-error` is already supported with Vercel (see https://github.com/vercel/vercel/issues/3362).
Zo-Bro-23: I'm not sure if I understand this correctly, but returning a 400 or a 500 error should trigger `stale-if-error` right? There's nothing special we have to do apart from throwing an API error (but I could be wrong).
rickstaa: > I'm not sure if I understand this correctly, but returning a 400 or a 500 error should trigger `stale-if-error` right? There's nothing special we have to do apart from throwing an API error (but I could be wrong).

I thought the same but up till now I don't know exactly how Vercel deals with errors. You can see what I already tried in:

https://github.com/anuraghazra/github-readme-stats/blob/c504c9338ed830ad619e008ba9ff76422398687f/api/index.js#L104-L108

I can currently respond with a custom error card and attach a `500` status code or show Vercels own `500` error. I was not able to change Vercels error messages. Could be that Vercel shows the previous cached card when a response with a `500` status is given and only shows the error card after the `if-stale-error` period is over but I could not find documentation on this matter and did not yet test. We could try to add a condition that breaks the `retryer` after a certain time. @anuraghazra what do you think?
rickstaa: Just putting this here for future reference. Fixing this issue will fix our most mentioned bug https://github.com/anuraghazra/github-readme-stats/issues/1515. 👍
rickstaa: If we like this new method, we can also improve it the [fetchers/stats-fetcher.js/totalCommitsFetcher](https://github.com/anuraghazra/github-readme-stats/blob/c5063b92b6e260dcc405a0a4cd27552dc103f8f5/src/fetchers/stats-fetcher.js#L139-L170) so that it throws an error instead of returning `0`. This will fix #1515 and #2358.
anuraghazra: >I can currently respond with a custom error card and attach a 500 status code or show Vercels own 500 error. I was not able to change Vercels error messages.

Just to confirm we will always be rendering our own custom error SVG card right? not the vercel's error page. If we render vercel's error page that might cause issues since we will not be serving content-type=image/svg at that point. 
rickstaa: Merging this will fix #1515. @qwerty541 I will check this later this week so we can close #1515.
rickstaa: > > I can currently respond with a custom error card and attach a 500 status code or show Vercels own 500 error. I was not able to change Vercels error messages.
> 
> Just to confirm we will always be rendering our own custom error SVG card right? not the vercel's error page. If we render vercel's error page that might cause issues since we will not be serving content-type=image/svg at that point.

@anuraghazra Yea, in the end I replaced the following code

https://github.com/anuraghazra/github-readme-stats/blob/c504c9338ed830ad619e008ba9ff76422398687f/api/index.js#L99C24-L109

with

https://github.com/anuraghazra/github-readme-stats/blob/0a5ff9647742a4424ebc6574f45d101e0afacec0/api/index.js#L103-L109
rickstaa: Hey @qwerty541 and @anuraghazra,

In our case, it seems that there's no significant advantage to having user requests occur at a frequency twice that of the server, especially since we don't regularly clear the cache or dynamically alter server data. Based on insights from [this source](https://stackoverflow.com/a/15972973/8135687), it appears that we might be able to simplify our caching headers like this:

```js
Cache-control: max-age=CONSTANTS.ERROR_CACHE_SECONDS, stale-while-revalidate=${CONSTANTS.ONE_DAY}
```


However, it's important to note that the information presented in that StackOverflow answer might be outdated, as it conflicts with more recent [specifications](https://datatracker.ietf.org/doc/html/rfc7234#section-5.2.2.8). Unfortunately, I couldn't find any documentation supporting the behaviour described in the new specifications. 

Various sources on the internet suggest using both the `max-age` and `s-maxage` directives. I'm not an expert on caching headers, so I'm unsure whether specifying both `max-age` and `s-maxage` is necessary in our case. Your insights on this would be greatly appreciated!

rickstaa: @anuraghazra, @qwerty541, this pull request can be merged if we decide which caching period we want to use for errors and whether we can simplify the caching headers 👍🏻.

Patch:
diff --git a/api/gist.js b/api/gist.js
index 3134bb5..3e7dca0 100644
--- a/api/gist.js
+++ b/api/gist.js
@@ -77,7 +77,12 @@ export default async (req, res) => {
       }),
     );
   } catch (err) {
-    res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.
+    res.setHeader(
+      "Cache-Control",
+      `max-age=${CONSTANTS.ERROR_CACHE_SECONDS / 2}, s-maxage=${
+        CONSTANTS.ERROR_CACHE_SECONDS
+      }, stale-while-revalidate=${CONSTANTS.ONE_DAY}`,
+    ); // Use lower cache period for errors.
     return res.send(renderError(err.message, err.secondaryMessage));
   }
 };
diff --git a/api/index.js b/api/index.js
index d171c80..c513f7c 100644
--- a/api/index.js
+++ b/api/index.js
@@ -57,7 +57,7 @@ export default async (req, res) => {
     );
 
     let cacheSeconds = clampValue(
-      parseInt(cache_seconds || CONSTANTS.FOUR_HOURS, 10),
+      parseInt(cache_seconds || CONSTANTS.CARD_CACHE_SECONDS, 10),
       CONSTANTS.FOUR_HOURS,
       CONSTANTS.ONE_DAY,
     );
@@ -100,7 +100,12 @@ export default async (req, res) => {
       }),
     );
   } catch (err) {
-    res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.
+    res.setHeader(
+      "Cache-Control",
+      `max-age=${CONSTANTS.ERROR_CACHE_SECONDS / 2}, s-maxage=${
+        CONSTANTS.ERROR_CACHE_SECONDS
+      }, stale-while-revalidate=${CONSTANTS.ONE_DAY}`,
+    ); // Use lower cache period for errors.
     return res.send(renderError(err.message, err.secondaryMessage));
   }
 };
diff --git a/api/pin.js b/api/pin.js
index 3383b00..d505746 100644
--- a/api/pin.js
+++ b/api/pin.js
@@ -40,7 +40,7 @@ export default async (req, res) => {
     const repoData = await fetchRepo(username, repo);
 
     let cacheSeconds = clampValue(
-      parseInt(cache_seconds || CONSTANTS.FOUR_HOURS, 10),
+      parseInt(cache_seconds || CONSTANTS.CARD_CACHE_SECONDS, 10),
       CONSTANTS.FOUR_HOURS,
       CONSTANTS.ONE_DAY,
     );
@@ -83,7 +83,12 @@ export default async (req, res) => {
       }),
     );
   } catch (err) {
-    res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.
+    res.setHeader(
+      "Cache-Control",
+      `max-age=${CONSTANTS.ERROR_CACHE_SECONDS / 2}, s-maxage=${
+        CONSTANTS.ERROR_CACHE_SECONDS
+      }, stale-while-revalidate=${CONSTANTS.ONE_DAY}`,
+    ); // Use lower cache period for errors.
     return res.send(renderError(err.message, err.secondaryMessage));
   }
 };
diff --git a/api/top-langs.js b/api/top-langs.js
index fdb6702..5596751 100644
--- a/api/top-langs.js
+++ b/api/top-langs.js
@@ -63,7 +63,7 @@ export default async (req, res) => {
     );
 
     let cacheSeconds = clampValue(
-      parseInt(cache_seconds || CONSTANTS.FOUR_HOURS, 10),
+      parseInt(cache_seconds || CONSTANTS.CARD_CACHE_SECONDS, 10),
       CONSTANTS.FOUR_HOURS,
       CONSTANTS.ONE_DAY,
     );
@@ -99,7 +99,12 @@ export default async (req, res) => {
       }),
     );
   } catch (err) {
-    res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.
+    res.setHeader(
+      "Cache-Control",
+      `max-age=${CONSTANTS.ERROR_CACHE_SECONDS / 2}, s-maxage=${
+        CONSTANTS.ERROR_CACHE_SECONDS
+      }, stale-while-revalidate=${CONSTANTS.ONE_DAY}`,
+    ); // Use lower cache period for errors.
     return res.send(renderError(err.message, err.secondaryMessage));
   }
 };
diff --git a/api/wakatime.js b/api/wakatime.js
index ec0c813..65f026e 100644
--- a/api/wakatime.js
+++ b/api/wakatime.js
@@ -42,7 +42,7 @@ export default async (req, res) => {
     const stats = await fetchWakatimeStats({ username, api_domain });
 
     let cacheSeconds = clampValue(
-      parseInt(cache_seconds || CONSTANTS.FOUR_HOURS, 10),
+      parseInt(cache_seconds || CONSTANTS.CARD_CACHE_SECONDS, 10),
       CONSTANTS.FOUR_HOURS,
       CONSTANTS.ONE_DAY,
     );
@@ -50,10 +50,6 @@ export default async (req, res) => {
       ? parseInt(process.env.CACHE_SECONDS, 10) || cacheSeconds
       : cacheSeconds;
 
-    if (!cache_seconds) {
-      cacheSeconds = CONSTANTS.FOUR_HOURS;
-    }
-
     res.setHeader(
       "Cache-Control",
       `max-age=${
@@ -82,7 +78,12 @@ export default async (req, res) => {
       }),
     );
   } catch (err) {
-    res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.
+    res.setHeader(
+      "Cache-Control",
+      `max-age=${CONSTANTS.ERROR_CACHE_SECONDS / 2}, s-maxage=${
+        CONSTANTS.ERROR_CACHE_SECONDS
+      }, stale-while-revalidate=${CONSTANTS.ONE_DAY}`,
+    ); // Use lower cache period for errors.
     return res.send(renderError(err.message, err.secondaryMessage));
   }
 };
diff --git a/src/common/retryer.js b/src/common/retryer.js
index 74f9a0f..3f294d3 100644
--- a/src/common/retryer.js
+++ b/src/common/retryer.js
@@ -72,5 +72,5 @@ const retryer = async (fetcher, variables, retries = 0) => {
   }
 };
 
-export { retryer };
+export { retryer, RETRIES };
 export default retryer;
diff --git a/src/common/utils.js b/src/common/utils.js
index 871c493..3cfd0e1 100644
--- a/src/common/utils.js
+++ b/src/common/utils.js
@@ -373,11 +373,21 @@ const noop = () => {};
 const logger =
   process.env.NODE_ENV !== "test" ? console : { log: noop, error: noop };
 
+// Cache settings.
+const CARD_CACHE_SECONDS = 14400;
+const ERROR_CACHE_SECONDS = 600;
+
 const CONSTANTS = {
+  ONE_MINUTE: 60,
+  FIVE_MINUTES: 300,
+  TEN_MINUTES: 600,
+  FIFTEEN_MINUTES: 900,
   THIRTY_MINUTES: 1800,
   TWO_HOURS: 7200,
   FOUR_HOURS: 14400,
   ONE_DAY: 86400,
+  CARD_CACHE_SECONDS,
+  ERROR_CACHE_SECONDS,
 };
 
 const SECONDARY_ERROR_MESSAGES = {


Test patch:
diff --git a/tests/api.test.js b/tests/api.test.js
index d29db22..1353f1f 100644
--- a/tests/api.test.js
+++ b/tests/api.test.js
@@ -184,13 +184,18 @@ describe("Test /api/", () => {
     ]);
   });
 
-  it("should not store cache when error", async () => {
+  it("should set shorter cache when error", async () => {
     const { req, res } = faker({}, error);
     await api(req, res);
 
     expect(res.setHeader.mock.calls).toEqual([
       ["Content-Type", "image/svg+xml"],
-      ["Cache-Control", `no-cache, no-store, must-revalidate`],
+      [
+        "Cache-Control",
+        `max-age=${CONSTANTS.ERROR_CACHE_SECONDS / 2}, s-maxage=${
+          CONSTANTS.ERROR_CACHE_SECONDS
+        }, stale-while-revalidate=${CONSTANTS.ONE_DAY}`,
+      ],
     ]);
   });
 
diff --git a/tests/retryer.test.js b/tests/retryer.test.js
index 257a70d..b0b4bd7 100644
--- a/tests/retryer.test.js
+++ b/tests/retryer.test.js
@@ -1,6 +1,6 @@
 import { jest } from "@jest/globals";
 import "@testing-library/jest-dom";
-import { retryer } from "../src/common/retryer.js";
+import { retryer, RETRIES } from "../src/common/retryer.js";
 import { logger } from "../src/common/utils.js";
 import { expect, it, describe } from "@jest/globals";
 
@@ -44,7 +44,7 @@ describe("Test Retryer", () => {
     try {
       await retryer(fetcherFail, {});
     } catch (err) {
-      expect(fetcherFail).toBeCalledTimes(8);
+      expect(fetcherFail).toBeCalledTimes(RETRIES + 1);
       expect(err.message).toBe("Downtime due to GitHub API rate limiting");
     }
   });