-
Notifications
You must be signed in to change notification settings - Fork 911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
customcommands: refine delayed execCC limits #1680
Draft
jo3-l
wants to merge
13
commits into
botlabs-gg:dev
Choose a base branch
from
jo3-l:feat/improve-execcc-limiting
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
+10,669
−1,525
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
changed wording to be clearer about which tiers of Patreon grant premium slots.
…bs-gg#1672) * dcmd: add manual response marker ...designed for commands that send their responses manually as opposed to through dcmd, such as the poll command. * poll,wyr: indicate to dcmd that responses are sent manually This commit (combined with the previous) allows poll and wyr responses to be autodeleted after a delay via command overrides. Previously, the command system had no knowledge of responses to these commands (which are sent manually since reactions need to be added), so autodeletion did not work.
…otlabs-gg#1671) Currently, commands with paginated responses do not obey the 'delete response after delay' option set using command overrides. The reason is that dcmd (and in turn the YAGPDB command system) does not know about paginated messages -- which are sent manually as opposed to using dcmd's response mechanism -- and so naturally does not delete them. This commit therefore teaches dcmd about paginated messages by introducing a new type, PaginatedResponse, which implements dcmd.Response. When returned from a command run function, dcmd will then automatically call PaginatedResponse.Send to create and return the paginated message, which interopates with the existing response deletion machinery.
…g#1655) * all: migrate executed command logging away from gorm to sqlboiler (botlabs-gg#1654) * common: add schemas for executed command logs * common: configure and run sqlboiler for executed command log model * all: migrate executed command logging to sqlboiler * reminders: migrate away from gorm to sqlboiler (botlabs-gg#1652) * reminders: add database schemas * reminders: set up and run sqlboiler * reminders: migrate to sqlboiler While at it, lightly refactor and simplify the code as applicable. The most substantial (non-database) refactor is renaming and moving strReminders to DisplayReminders. To aid in the refactor, also add discordgo.ParseID as a shortcut for strconv.ParseInt(...). This operation is often needed to convert string IDs in database to int64's and having a shorter name helps. * reminders: auto-delete old entries with null guild_id as necessary * soundboard: remove stray references to gorm (botlabs-gg#1651) The soundboard module used to use gorm, but was migrated to sqlboiler some years ago in commit 628ea9a. There are still two (erroneous) references to gorm remaining which were not caught since this section of code ignores errors. This commit changes these to use sqlboiler as well. * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible (botlabs-gg#1650) * schemamigration: skip executing ALTER COLUMN SET NOT NULL queries when possible * Use quantifier * not + in regex for consistency It really should be +, but the current code works and hey -- when in Rome, do as the Romans. * youtube: migrate away from gorm to sqlboiler (botlabs-gg#1660) * youtube: add database schemas * youtube: set up and run sqlboiler * youtube: migrate to sqlboiler * notifications: migrate away from gorm to sqlboiler (botlabs-gg#1659) * notifications: decouple from configstore * notifications: add database schemas * notifications: set up and run sqlboiler * notifications: migrate to sqlboiler * moderation: migrate away from gorm to sqlboiler (botlabs-gg#1658) * moderation: decouple from configstore * moderation: add database schemas * moderation: set up and run sqlboiler * moderation: migrate to sqlboiler * web: support null.Int64 in forms and validator We currently use gorilla/schema to parse HTML form values into structs -- this allows us to specify, e,g., `<input ... name=SomeField>` and have `payload.SomeField` set correctly in Go handlers. This approach mostly works nicely, with the major exception of null.Int64 (formerly sql.NullInt64 with gorm) fields, which show up in the moderation plugin, e.g., Config.DefaultMuteDuration. Particularly, since gorilla/schema has no native support for null.Int64, we have instead instructed gorilla/schema to directly set the Int64 value using <input ... name=DefaultMuteDuration.Int64> and then manually set DefaultMuteDuration.Valid=true in the web handler, without which the changes are not saved to database. This approach is, for obvious reasons, rather brittle. In this commit we therefore register a custom gorilla/schema decoder for null.Int64, obviating the need for both pointing to the Int64 field and manually setting Valid=true. While at it, we also fix the validator so that it checks null.Int64 fields properly. * web: support validating types.Int64Array gorm uses pq.Int64Array but sqlboiler uses types.Int64Array, so we need also support the latter now. * common/configstore: remove package (botlabs-gg#1668) * all: remove last traces of gorm (botlabs-gg#1670) * common: connect to postgres via database/sql ...instead of through gorm. * common: remove gorm log hook * common: remove gorm SmallModel * deps: run go mod tidy --------- Co-authored-by: Joe L <[email protected]>
…labs-gg#1679) * decodeBase64, encodeBase64 and sha256 * decodeBase64, encodeBase64 and sha256
Fix custom IDs for modals always prepending 0 to the user-configurable portion of the ID. This was due to outdated default Custom ID code conflicting with the Custom ID validation. Signed-off-by: SoggySaussages <[email protected]>
…otlabs-gg#1674) * commands: move code handling cmd overrides for threads into YagCommand.GetSettings Previously this logic was done by the caller, but it should be the responsibility of the GetSettings procedure. This commit also incidentally fixes a minor bug in CommonContainerNotFoundHandler, which previously did not consider command overrides properly if used in a thread. * reminders: verify that member can run `remindme` in target channel
* templates: flag templates executed by inbuilt cmds Add a flag for templates which are executed from the templates of inbuilt commands. Signed-off-by: SoggySaussages <[email protected]> * moderation: block nested exec(Admin) calls Blocks nested exec/execAdmin calls by passing an "executedByCommandTemplate" flag through moderation command execution. When this flag reaches a new context created by a moderation command, it turns into ExecutedFromNextedCommandTemplate. This flag blocks moderation commands from running. Signed-off-by: SoggySaussages <[email protected]> * tickets: block nested exec(Admin) calls Blocks nested exec/execAdmin calls and createTicket calls by passing an "executedByCommandTemplate" flag through moderation command execution. When this flag reaches a new context created by a ticket, it turns into ExecutedFromNextedCommandTemplate. This flag blocks moderation commands and tickets from running. Signed-off-by: SoggySaussages <[email protected]> * moderation: fix nesting detection Signed-off-by: SoggySaussages <[email protected]> * tickets: fix nesting detection Signed-off-by: SoggySaussages <[email protected]> * templates: block execCC from inbuilt cmd templates Signed-off-by: SoggySaussages <[email protected]> * moderation: further fix nest detection Signed-off-by: SoggySaussages <[email protected]> * tickets: further fix nest detection Signed-off-by: SoggySaussages <[email protected]> --------- Signed-off-by: SoggySaussages <[email protected]>
Whoops, sent this before I finished writing the description 😔 Will edit. |
(previously per-channel.) Scale the ratelimiter parameters accordingly.
jo3-l
force-pushed
the
feat/improve-execcc-limiting
branch
from
June 22, 2024 03:39
f429f5b
to
5653121
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR proposes two changes to delayed execCC1 limits.
Current limits on
execCC
To provide some additional context between the changes here, we must first describe the status quo. At present,
execCC
calls with nonzero delay must go through the scheduled event system, and so require additional limits to not overload YAGPDB. A per-channel token bucket (see also the x/time/rate doc) is therefore used to ratelimit delayedexecCC
executions as they arrive. Each token bucket starts with 10 tokens and is replenished at a rate of 1/10s. Each execution consumes 1 token; if there are no tokens available, the request is ratelimited and does not proceed: it is silently ignored. Generally speaking, then, the current limit can be understood as follows: delayedexecCC
calls are limited per-channel to 1/10s; however, if some time has gone by with noexecCC
calls, up to 10execCC
calls are permitted in short succession.2Why current limits are imperfect
The current approach has two minor flaws:
execCC
to use random target channels to reduce the chance that they run afoul of the limit. Ideally, this workaround would not be necessary, motivating the change to a per-guild limit. A per-guild limit is also more robust: we want to cap the load that a single guild can place on YAGPDB, not a single channel.execCC
calls (causing them to no longer execute and associated user complaints.) We therefore allow executions that only slightly exceed the limit to proceed after an appropriate delay; this change maintains the effectiveness of the limit while minimizing breakage.Points of discussion
execCC
call ratelimited for up to 10 seconds will still be processed, but one ratelimited for, say, 15 seconds, would still be dropped.)Footnotes
Note that execCC calls with 0 delay are not affected by this change, nor do future revisions of this change aim to limit them further. Those calls do not go through the scheduled event system and are considerably less costly to handle. ↩
This description is imperfect as we do not consider the latency of individual
execCC
invocations but it suffices as a first approximation. ↩