feat(Rest): better handling of global rate limit and invalid request tracking (#4711)

Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
Co-authored-by: Tristan Guichaoua <33934311+tguichaoua@users.noreply.github.com>
Co-authored-by: Papaia <43409674+Papaia@users.noreply.github.com>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>

src/client/Client.js

@@ -471,6 +471,9 @@ class Client extends BaseClient {
     if (typeof options.messageSweepInterval !== 'number' || isNaN(options.messageSweepInterval)) {
       throw new TypeError('CLIENT_INVALID_OPTION', 'messageSweepInterval', 'a number');
     }
+    if (typeof options.invalidRequestWarningInterval !== 'number' || isNaN(options.invalidRequestWarningInterval)) {
+      throw new TypeError('CLIENT_INVALID_OPTION', 'invalidRequestWarningInterval', 'a number');
+    }
     if (!Array.isArray(options.partials)) {
       throw new TypeError('CLIENT_INVALID_OPTION', 'partials', 'an Array');
     }
@@ -480,6 +483,9 @@ class Client extends BaseClient {
     if (typeof options.restRequestTimeout !== 'number' || isNaN(options.restRequestTimeout)) {
       throw new TypeError('CLIENT_INVALID_OPTION', 'restRequestTimeout', 'a number');
     }
+    if (typeof options.restGlobalRateLimit !== 'number' || isNaN(options.restGlobalRateLimit)) {
+      throw new TypeError('CLIENT_INVALID_OPTION', 'restGlobalRateLimit', 'a number');
+    }
     if (typeof options.restSweepInterval !== 'number' || isNaN(options.restSweepInterval)) {
       throw new TypeError('CLIENT_INVALID_OPTION', 'restSweepInterval', 'a number');
     }

src/rest/RESTManager.js

@@ -13,7 +13,10 @@ class RESTManager {
     this.handlers = new Collection();
     this.tokenPrefix = tokenPrefix;
     this.versioned = true;
-    this.globalTimeout = null;
+    this.globalLimit = client.options.restGlobalRateLimit > 0 ? client.options.restGlobalRateLimit : Infinity;
+    this.globalRemaining = this.globalLimit;
+    this.globalReset = null;
+    this.globalDelay = null;
     if (client.options.restSweepInterval > 0) {
       client.setInterval(() => {
         this.handlers.sweep(handler => handler._inactive);

src/rest/RequestHandler.js

@@ -4,7 +4,7 @@ const AsyncQueue = require('./AsyncQueue');
 const DiscordAPIError = require('./DiscordAPIError');
 const HTTPError = require('./HTTPError');
 const {
-  Events: { RATE_LIMIT },
+  Events: { RATE_LIMIT, INVALID_REQUEST_WARNING },
 } = require('../util/Constants');
 const Util = require('../util/Util');
 
@@ -21,6 +21,15 @@ function calculateReset(reset, serverDate) {
   return new Date(Number(reset) * 1000).getTime() - getAPIOffset(serverDate);
 }
 
+/* Invalid request limiting is done on a per-IP basis, not a per-token basis.
+ * The best we can do is track invalid counts process-wide (on the theory that
+ * users could have multiple bots run from one process) rather than per-bot.
+ * Therefore, store these at file scope here rather than in the client's
+ * RESTManager object.
+ */
+let invalidCount = 0;
+let invalidCountResetTime = null;
+
 class RequestHandler {
   constructor(manager) {
     this.manager = manager;
@@ -28,7 +37,6 @@ class RequestHandler {
     this.reset = -1;
     this.remaining = -1;
     this.limit = -1;
-    this.retryAfter = -1;
   }
 
   async push(request) {
@@ -40,18 +48,56 @@ class RequestHandler {
     }
   }
 
+  get globalLimited() {
+    return this.manager.globalRemaining <= 0 && Date.now() < this.manager.globalReset;
+  }
+
+  get localLimited() {
+    return this.remaining <= 0 && Date.now() < this.reset;
+  }
+
   get limited() {
-    return Boolean(this.manager.globalTimeout) || (this.remaining <= 0 && Date.now() < this.reset);
+    return this.globalLimited || this.localLimited;
   }
 
   get _inactive() {
     return this.queue.remaining === 0 && !this.limited;
   }
 
+  globalDelayFor(ms) {
+    return new Promise(resolve => {
+      this.manager.client.setTimeout(() => {
+        this.manager.globalDelay = null;
+        resolve();
+      }, ms);
+    });
+  }
+
   async execute(request) {
-    // After calculations and requests have been done, pre-emptively stop further requests
-    if (this.limited) {
-      const timeout = this.reset + this.manager.client.options.restTimeOffset - Date.now();
+    /*
+     * After calculations have been done, pre-emptively stop further requests
+     * Potentially loop until this task can run if e.g. the global rate limit is hit twice
+     */
+    while (this.limited) {
+      const isGlobal = this.globalLimited;
+      let limit, timeout, delayPromise;
+
+      if (isGlobal) {
+        // Set the variables based on the global rate limit
+        limit = this.manager.globalLimit;
+        timeout = this.manager.globalReset + this.manager.client.options.restTimeOffset - Date.now();
+        // If this is the first task to reach the global timeout, set the global delay
+        if (!this.manager.globalDelay) {
+          // The global delay function should clear the global delay state when it is resolved
+          this.manager.globalDelay = this.globalDelayFor(timeout);
+        }
+        delayPromise = this.manager.globalDelay;
+      } else {
+        // Set the variables based on the route-specific rate limit
+        limit = this.limit;
+        timeout = this.reset + this.manager.client.options.restTimeOffset - Date.now();
+        delayPromise = Util.delayFor(timeout);
+      }
 
       if (this.manager.client.listenerCount(RATE_LIMIT)) {
         /**
@@ -63,24 +109,29 @@ class RequestHandler {
          * @param {string} rateLimitInfo.method HTTP method used for request that triggered this event
          * @param {string} rateLimitInfo.path Path used for request that triggered this event
          * @param {string} rateLimitInfo.route Route used for request that triggered this event
+         * @param {boolean} rateLimitInfo.global Whether the rate limit that was reached was the global limit
          */
         this.manager.client.emit(RATE_LIMIT, {
           timeout,
-          limit: this.limit,
+          limit,
           method: request.method,
           path: request.path,
           route: request.route,
+          global: isGlobal,
         });
       }
 
-      if (this.manager.globalTimeout) {
-        await this.manager.globalTimeout;
-      } else {
-        // Wait for the timeout to expire in order to avoid an actual 429
-        await Util.delayFor(timeout);
-      }
+      // Wait for the timeout to expire in order to avoid an actual 429
+      await delayPromise; // eslint-disable-line no-await-in-loop
     }
 
+    // As the request goes out, update the global usage information
+    if (!this.manager.globalReset || this.manager.globalReset < Date.now()) {
+      this.manager.globalReset = Date.now() + 1000;
+      this.manager.globalRemaining = this.manager.globalLimit;
+    }
+    this.manager.globalRemaining--;
+
     // Perform the request
     let res;
     try {
@@ -95,33 +146,64 @@ class RequestHandler {
       return this.execute(request);
     }
 
+    let sublimitTimeout;
     if (res && res.headers) {
       const serverDate = res.headers.get('date');
       const limit = res.headers.get('x-ratelimit-limit');
       const remaining = res.headers.get('x-ratelimit-remaining');
       const reset = res.headers.get('x-ratelimit-reset');
-      const retryAfter = res.headers.get('retry-after');
-
       this.limit = limit ? Number(limit) : Infinity;
       this.remaining = remaining ? Number(remaining) : 1;
       this.reset = reset ? calculateReset(reset, serverDate) : Date.now();
-      this.retryAfter = retryAfter ? Number(retryAfter) * 1000 : -1;
 
       // https://github.com/discordapp/discord-api-docs/issues/182
       if (request.route.includes('reactions')) {
         this.reset = new Date(serverDate).getTime() - getAPIOffset(serverDate) + 250;
       }
 
-      // Handle global ratelimit
-      if (res.headers.get('x-ratelimit-global')) {
-        // Set the manager's global timeout as the promise for other requests to "wait"
-        this.manager.globalTimeout = Util.delayFor(this.retryAfter);
+      // Handle retryAfter, which means we have actually hit a rate limit
+      let retryAfter = res.headers.get('retry-after');
+      retryAfter = retryAfter ? Number(retryAfter) * 1000 : -1;
+      if (retryAfter > 0) {
+        // If the global ratelimit header is set, that means we hit the global rate limit
+        if (res.headers.get('x-ratelimit-global')) {
+          this.manager.globalRemaining = 0;
+          this.manager.globalReset = Date.now() + retryAfter;
+        } else if (!this.localLimited) {
+          /*
+           * This is a sublimit (e.g. 2 channel name changes/10 minutes) since the headers don't indicate a
+           * route-wide rate limit. Don't update remaining or reset to avoid rate limiting the whole
+           * endpoint, just set a reset time on the request itself to avoid retrying too soon.
+           */
+          sublimitTimeout = retryAfter;
+        }
+      }
+    }
 
-        // Wait for the global timeout to resolve before continuing
-        await this.manager.globalTimeout;
+    // Count the invalid requests
+    if (res.status === 401 || res.status === 403 || res.status === 429) {
+      if (!invalidCountResetTime || invalidCountResetTime < Date.now()) {
+        invalidCountResetTime = Date.now() + 1000 * 60 * 10;
+        invalidCount = 0;
+      }
+      invalidCount++;
 
-        // Clean up global timeout
-        this.manager.globalTimeout = null;
+      const emitInvalid =
+        this.manager.client.listenerCount(INVALID_REQUEST_WARNING) &&
+        this.manager.client.options.invalidRequestWarningInterval > 0 &&
+        invalidCount % this.manager.client.options.invalidRequestWarningInterval === 0;
+      if (emitInvalid) {
+        /**
+         * Emitted periodically when the process sends invalid messages to let users avoid the
+         * 10k invalid requests in 10 minutes threshold that causes a ban
+         * @event Client#invalidRequestWarning
+         * @param {number} invalidRequestWarningInfo.count Number of invalid requests that have been made in the window
+         * @param {number} invalidRequestWarningInfo.remainingTime Time in ms remaining before the count resets
+         */
+        this.manager.client.emit(INVALID_REQUEST_WARNING, {
+          count: invalidCount,
+          remainingTime: invalidCountResetTime - Date.now(),
+        });
       }
     }
 
@@ -136,8 +218,11 @@ class RequestHandler {
       // Handle ratelimited requests
       if (res.status === 429) {
         // A ratelimit was hit - this should never happen
-        this.manager.client.emit('debug', `429 hit on route ${request.route}`);
-        await Util.delayFor(this.retryAfter);
+        this.manager.client.emit('debug', `429 hit on route ${request.route}${sublimitTimeout ? ' for sublimit' : ''}`);
+        // If caused by a sublimit, wait it out here so other requests on the route can be handled
+        if (sublimitTimeout) {
+          await Util.delayFor(sublimitTimeout);
+        }
         return this.execute(request);
       }
 

src/util/Constants.js

@@ -19,6 +19,9 @@ const { Error, RangeError } = require('../errors');
  * @property {number} [messageSweepInterval=0] How frequently to remove messages from the cache that are older than
  * the message cache lifetime (in seconds, 0 for never)
  * @property {MessageMentionOptions} [allowedMentions] Default value for {@link MessageOptions#allowedMentions}
+ * @property {number} [invalidRequestWarningInterval=0] The number of invalid REST requests (those that return
+ * 401, 403, or 429) in a 10 minute window between emitted warnings (0 for no warnings). That is, if set to 500,
+ * warnings will be emitted at invalid request number 500, 1000, 1500, and so on.
  * @property {PartialType[]} [partials] Structures allowed to be partial. This means events can be emitted even when
  * they're missing all the data for a particular structure. See the "Partials" topic listed in the sidebar for some
  * important usage information, as partials require you to put checks in place when handling data.
@@ -29,6 +32,8 @@ const { Error, RangeError } = require('../errors');
  * @property {number} [restRequestTimeout=15000] Time to wait before cancelling a REST request, in milliseconds
  * @property {number} [restSweepInterval=60] How frequently to delete inactive request buckets, in seconds
  * (or 0 for never)
+ * @property {number} [restGlobalRateLimit=0] How many requests to allow sending per second (0 for unlimited, 50 for
+ * the standard global limit used by Discord)
  * @property {number} [retryLimit=1] How many times to retry on 5XX errors (Infinity for indefinite amount of retries)
  * @property {PresenceData} [presence={}] Presence data to use upon login
  * @property {IntentsResolvable} intents Intents to enable for this connection
@@ -40,9 +45,11 @@ exports.DefaultOptions = {
   messageCacheMaxSize: 200,
   messageCacheLifetime: 0,
   messageSweepInterval: 0,
+  invalidRequestWarningInterval: 0,
   partials: [],
   restWsBridgeTimeout: 5000,
   restRequestTimeout: 15000,
+  restGlobalRateLimit: 0,
   retryLimit: 1,
   restTimeOffset: 500,
   restSweepInterval: 60,
@@ -218,6 +225,7 @@ exports.VoiceOPCodes = {
 
 exports.Events = {
   RATE_LIMIT: 'rateLimit',
+  INVALID_REQUEST_WARNING: 'invalidRequestWarning',
   CLIENT_READY: 'ready',
   GUILD_CREATE: 'guildCreate',
   GUILD_DELETE: 'guildDelete',

typings/index.d.ts

@@ -376,6 +376,7 @@ declare module 'discord.js' {
     };
     Events: {
       RATE_LIMIT: 'rateLimit';
+      INVALID_REQUEST_WARNING: 'invalidRequestWarning';
       CLIENT_READY: 'ready';
       RESUMED: 'resumed';
       GUILD_CREATE: 'guildCreate';
@@ -2424,6 +2425,7 @@ declare module 'discord.js' {
     messageUpdate: [oldMessage: Message | PartialMessage, newMessage: Message | PartialMessage];
     presenceUpdate: [oldPresence: Presence | undefined, newPresence: Presence];
     rateLimit: [rateLimitData: RateLimitData];
+    invalidRequestWarning: [invalidRequestWarningData: InvalidRequestWarningData];
     ready: [];
     invalidated: [];
     roleCreate: [role: Role];
@@ -2447,10 +2449,12 @@ declare module 'discord.js' {
     messageCacheLifetime?: number;
     messageSweepInterval?: number;
     allowedMentions?: MessageMentionOptions;
+    invalidRequestWarningInterval?: number;
     partials?: PartialTypes[];
     restWsBridgeTimeout?: number;
     restTimeOffset?: number;
     restRequestTimeout?: number;
+    restGlobalRateLimit?: number;
     restSweepInterval?: number;
     retryLimit?: number;
     presence?: PresenceData;
@@ -3209,6 +3213,12 @@ declare module 'discord.js' {
     method: string;
     path: string;
     route: string;
+    global: boolean;
+  }
+
+  interface InvalidRequestWarningData {
+    count: number;
+    remainingTime: number;
   }
 
   interface RawOverwriteData {