Telegram request throttling per chat ID

[alexanderroidl]

Currently the Telegram Notification Adapter waits one second before sending each message:

fredy/lib/notification/adapter/telegram.js

Lines 37 to 59 in 9bb33e7

	/**
	* This is to not break the rate limit. It is to only send 1 message per second
	*/
	return new Promise((resolve, reject) => {
	setTimeout(() => {
	fetch(`https://api.telegram.org/bot${token}/sendMessage`, {
	method: 'post',
	body: JSON.stringify({
	chat_id: chatId,
	text: messageParagraphs.join('\n\n'),
	parse_mode: 'HTML',
	disable_web_page_preview: true,
	}),
	headers: { 'Content-Type': 'application/json' },
	})
	.then(() => {
	resolve();
	})
	.catch(() => {
	reject();
	});
	}, RATE_LIMIT_INTERVAL);
	});

But as each timeout is started at the same time and all are awaited simultaneously...

fredy/lib/notification/adapter/telegram.js

Line 61 in 9bb33e7

return Promise.all(promises);

...they will still be sent together, just after a one second delay.

Telegram's FAQ says this about limits:

In a single chat, avoid sending more than one message per second. We may allow short bursts that go over this limit, but eventually you'll begin receiving 429 errors.

Which is why with this MR I implemented request throttling and set it to work per Chat ID, meaning each request will be sent immediately if there was none prior and only be waited for if another message to the same ID has been sent less than a second before.

[orangecoding]

The cleanupOldThrottles function iterates over chatThrottleMap.entries() and calls chatThrottleMap.delete(chatId) during iteration. In js modifying a Map (e.g., deleting entries) while iterating over it with entries() can lead to unpredictable behavior, such as skipped entries or runtime errors, especially if chatThrottleMap is accessed concurrently by multiple send calls.

[orangecoding]

We might be able to mittigate the above effects with with increasing the rate_limit_interval to e.g. 10 secs and modifying the function like so:

function cleanupOldThrottles() {
  const now = Date.now();
  const keysToDelete = [];
  for (const [chatId, chatThrottle] of chatThrottleMap.entries()) {
    if (now - chatThrottle.lastUsedAt > RATE_LIMIT_INTERVAL) {
      keysToDelete.push(chatId);
    }
  }
  for (const chatId of keysToDelete) {
    chatThrottleMap.delete(chatId);
  }
}

WDTY?

[alexanderroidl]

Good catch!! Love the idea and changed the code accordingly.

[orangecoding]

The cleanupOldThrottles function deletes entries from chatThrottleMap if now - chatThrottle.lastUsedAt > RATE_LIMIT_INTERVAL. Since RATE_LIMIT_INTERVAL is 1 sec, entries are removed if they haven't been used in the last second. However, I think this is too aggressive for a rate-limiting system, as it could delete a throttle entry while it's still actively limiting requests. For example, if a chat sends a message at t=0ms and another at t=500ms, the throttle entry might be deleted at t=1001ms during a subsequent call, even though the throttle is still needed to enforce the rate limit for ongoing messages.

[alexanderroidl]

Great point actually! I added an extra second, do you think this will be sufficient? :)