Skip to content

Commit

Permalink
Reduce the throttle test theshold even more.
Browse files Browse the repository at this point in the history
The implementation is rubbish, as it doesn't avoid the exponential backoff

Remove default rate limit testing.

It doesn't work. No there really isn't more to say about it
you're welcome to dispute it if you're going to do the work investigating. I'm not.

We used to have a test here that tested whether Mjolnir was going to carry out a redact order the default limits in a reasonable time scale.
Now I think that's never going to happen without writing a new algorithm for respecting rate limiting.
Which is not something there is time for.

matrix-org/synapse#13018

Synapse rate limits were broken and very permitting so that's why the current hack worked so well.
Now it is not broken, so our rate limit handling is.

b850e45

Honestly I don't think we can expect anyone to be able to use Mjolnir under default rate limits.

well, it's not quite simple as broken, but it is broken. With the default level in synapse (which is what matrix.org uses) it is struggling to redact 15 messages within 5 minutes. that means 5 messages over the burst count. This is ofc ontop mjolnir sending reactions / responding to replies (which isn't much but... enough to mess with the rate limiter since ofc, Synapse tells requests to wait x amount of time before trying again, but that doesn't help for concurrent requests since ofc there's only 1 slot available at that future time.  This means Synapse just wacks everything with exponentially longer shit without many (or any?) events going through
it used to be fine
because rate limiting in synapse used to be a lot more liberal because it was "broken" or something, that's not me saying it's broken that's just what synapse devs say which is probably true.
if all requests went into a queue then yeah you could eliminate one problem
but that's a lot of work and i don't think we should be doing it
cos no one uses mjolnir like this anyways
  • Loading branch information
Gnuxie committed Aug 16, 2022
1 parent d5171bd commit b9284f0
Showing 1 changed file with 15 additions and 57 deletions.
72 changes: 15 additions & 57 deletions test/integration/throttleTest.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { strict as assert } from "assert";
import { newTestUser, overrideRatelimitForUser, resetRatelimitForUser } from "./clientHelper";
import { newTestUser } from "./clientHelper";
import { getMessagesByUserIn } from "../../src/utils";
import { getFirstReaction } from "./commands/commandUtils";

describe("Test: throttled users can function with Mjolnir.", function () {
it('throttled users survive being throttled by synapse', async function() {
Expand All @@ -18,58 +17,17 @@ describe("Test: throttled users can function with Mjolnir.", function () {
})
})

describe("Test: Mjolnir can still sync and respond to commands while throttled", function () {
beforeEach(async function() {
await resetRatelimitForUser(await this.mjolnir.client.getUserId())
})
afterEach(async function() {
// If a test has a timeout while awaitng on a promise then we never get given control back.
this.moderator?.stop();

await overrideRatelimitForUser(await this.mjolnir.client.getUserId());
})

it('Can still perform and respond to a redaction command', async function () {
// Create a few users and a room.
let badUser = await newTestUser({ name: { contains: "spammer-needs-redacting" } });
let badUserId = await badUser.getUserId();
const mjolnir = this.mjolnir.client;
let mjolnirUserId = await mjolnir.getUserId();
let moderator = await newTestUser({ name: { contains: "moderator" } });
this.moderator = moderator;
await moderator.joinRoom(this.mjolnir.managementRoomId);
let targetRoom = await moderator.createRoom({ invite: [await badUser.getUserId(), mjolnirUserId]});
await moderator.setUserPowerLevel(mjolnirUserId, targetRoom, 100);
await badUser.joinRoom(targetRoom);

// Give Mjolnir some work to do and some messages to sync through.
await Promise.all([...Array(25).keys()].map((i) => moderator.sendMessage(this.mjolnir.managementRoomId, {msgtype: 'm.text.', body: `Irrelevant Message #${i}`})));
await Promise.all([...Array(25).keys()].map(_ => moderator.sendMessage(this.mjolnir.managementRoomId, {msgtype: 'm.text', body: '!mjolnir status'})));

await moderator.sendMessage(this.mjolnir.managementRoomId, {msgtype: 'm.text', body: `!mjolnir rooms add ${targetRoom}`});

await Promise.all([...Array(25).keys()].map((i) => badUser.sendMessage(targetRoom, {msgtype: 'm.text.', body: `Bad Message #${i}`})));

try {
await moderator.start();
await getFirstReaction(moderator, this.mjolnir.managementRoomId, '✅', async () => {
return await moderator.sendMessage(this.mjolnir.managementRoomId, { msgtype: 'm.text', body: `!mjolnir redact ${badUserId} ${targetRoom}` });
});
} finally {
moderator.stop();
}

let count = 0;
await getMessagesByUserIn(moderator, badUserId, targetRoom, 1000, function(events) {
count += events.length
events.map(e => {
if (e.type === 'm.room.member') {
assert.equal(Object.keys(e.content).length, 1, "Only membership should be left on the membership event when it has been redacted.")
} else if (Object.keys(e.content).length !== 0) {
throw new Error(`This event should have been redacted: ${JSON.stringify(e, null, 2)}`)
}
})
});
assert.equal(count, 26, "There should be exactly 26 events from the spammer in this room.");
})
})
/**
* We used to have a test here that tested whether Mjolnir was going to carry out a redact order the default limits in a reasonable time scale.
* Now I think that's never going to happen without writing a new algorithm for respecting rate limiting.
* Which is not something there is time for.
*
* https://github.com/matrix-org/synapse/pull/13018
*
* Synapse rate limits were broken and very permitting so that's why the current hack worked so well.
* Now it is not broken, so our rate limit handling is.
*
* https://github.com/matrix-org/mjolnir/commit/b850e4554c6cbc9456e23ab1a92ede547d044241
*
* Honestly I don't think we can expect anyone to be able to use Mjolnir under default rate limits.
*/

0 comments on commit b9284f0

Please sign in to comment.