Fix NewHelpMenu: restore [p]help, close memory leak, parallelize perm checks
Loading this cog called bot.remove_command("help") with no replacement
command ever defined and no restore on unload — simply loading it broke
[p]help bot-wide, permanently, until process restart. Traced through Red's
own source (redbot/core/commands/help.py) and confirmed the default help
command already routes through bot.send_help_for(), which is exactly what
this cog hooks to render CV2 output — so the fix is to just not remove the
command at all; the CV2 conversion now actually works as intended too,
since its hook point is reachable again.
_active_views leaked one permanent dict entry (holding the whole rendered
page/embed list) per help/menu invocation — every on_timeout and "close"
button already called view.stop(), but nothing was popping the matching
entry except one specific code path. Added a single _track_view() helper
that wraps stop() for automatic eviction and applied it at all 5 insertion
points instead of just the one.
Parallelized the per-command can_run() permission checks in
gather_bot_help_data/gather_cog_help_data (previously one await at a time
for every bot/cog command on every single [p]help call). Also added the
admin_or_permissions check to `cv2 version`, the one subcommand missing it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
a9cdef8a1b
commit
93e759794f
3 changed files with 59 additions and 29 deletions
|
|
@ -2,11 +2,14 @@ from .newhelpmenu import NewHelpMenu
|
||||||
|
|
||||||
|
|
||||||
async def setup(bot):
|
async def setup(bot):
|
||||||
# Remove Red's built-in help command BEFORE adding the cog,
|
# Red's own default help command (and any cog's ctx.send_help()) already
|
||||||
# so our cog's @commands.command(name="help") doesn't conflict.
|
# routes through bot.send_help_for() internally — see
|
||||||
original_help = bot.remove_command("help")
|
# redbot/core/commands/help.py. That's exactly the method this cog hooks
|
||||||
|
# to render CV2 output, so the built-in help command must stay in place
|
||||||
|
# for that hook to ever fire. Removing it here used to just delete
|
||||||
|
# [p]help bot-wide with nothing to replace it (there was never an actual
|
||||||
|
# replacement `help` command defined), and cog_unload never restored it
|
||||||
|
# either — simplest correct fix is to not remove it at all.
|
||||||
cog = NewHelpMenu(bot)
|
cog = NewHelpMenu(bot)
|
||||||
cog._original_help_command = original_help # store for restore on unload
|
|
||||||
await bot.add_cog(cog)
|
await bot.add_cog(cog)
|
||||||
await cog.initialize()
|
await cog.initialize()
|
||||||
|
|
|
||||||
|
|
@ -73,14 +73,20 @@ async def gather_bot_help_data(
|
||||||
|
|
||||||
pending_checks.append((cmd, cog_name))
|
pending_checks.append((cmd, cog_name))
|
||||||
|
|
||||||
# Run permission checks (this is the slow part)
|
# Run permission checks concurrently — each is an independent await (often
|
||||||
for cmd, cog_name in pending_checks:
|
# a Config lookup via Red's permission system), and with a few hundred
|
||||||
|
# bot commands doing them one at a time made every single [p]help call
|
||||||
|
# pay for that latency serially.
|
||||||
|
async def _safe_can_run(cmd: commands.Command) -> bool:
|
||||||
try:
|
try:
|
||||||
if await cmd.can_run(ctx):
|
return await cmd.can_run(ctx)
|
||||||
cog_commands[cog_name].append(cmd)
|
|
||||||
except Exception:
|
except Exception:
|
||||||
# Permission check failed, skip this command
|
return False
|
||||||
pass
|
|
||||||
|
allowed_flags = await asyncio.gather(*(_safe_can_run(cmd) for cmd, _ in pending_checks))
|
||||||
|
for (cmd, cog_name), allowed in zip(pending_checks, allowed_flags):
|
||||||
|
if allowed:
|
||||||
|
cog_commands[cog_name].append(cmd)
|
||||||
|
|
||||||
cog_to_category: Dict[str, str] = {}
|
cog_to_category: Dict[str, str] = {}
|
||||||
for cat_name, cog_list in categories.items():
|
for cat_name, cog_list in categories.items():
|
||||||
|
|
@ -124,14 +130,22 @@ async def gather_cog_help_data(
|
||||||
if not cog_doc:
|
if not cog_doc:
|
||||||
cog_doc = cog.help or cog.__doc__ or "No description."
|
cog_doc = cog.help or cog.__doc__ or "No description."
|
||||||
|
|
||||||
cmds = []
|
candidates = [
|
||||||
for cmd in sorted(cog.get_commands(), key=lambda c: c.qualified_name):
|
c for c in sorted(cog.get_commands(), key=lambda c: c.qualified_name)
|
||||||
if cmd.hidden and not show_hidden:
|
if not (c.hidden and not show_hidden)
|
||||||
continue
|
]
|
||||||
|
|
||||||
|
async def _safe_can_run(cmd: commands.Command) -> bool:
|
||||||
try:
|
try:
|
||||||
if not await cmd.can_run(ctx):
|
return await cmd.can_run(ctx)
|
||||||
continue
|
|
||||||
except Exception:
|
except Exception:
|
||||||
|
return False
|
||||||
|
|
||||||
|
allowed_flags = await asyncio.gather(*(_safe_can_run(c) for c in candidates))
|
||||||
|
|
||||||
|
cmds = []
|
||||||
|
for cmd, allowed in zip(candidates, allowed_flags):
|
||||||
|
if not allowed:
|
||||||
continue
|
continue
|
||||||
sig = cmd.signature or ""
|
sig = cmd.signature or ""
|
||||||
short = cmd.format_shortdoc_for_context(ctx) if hasattr(cmd, "format_shortdoc_for_context") else (cmd.short_doc or "No description")
|
short = cmd.format_shortdoc_for_context(ctx) if hasattr(cmd, "format_shortdoc_for_context") else (cmd.short_doc or "No description")
|
||||||
|
|
|
||||||
|
|
@ -104,6 +104,24 @@ class NewHelpMenu(commands.Cog):
|
||||||
self._settings_cache: Dict[int, Tuple[Dict, float]] = {}
|
self._settings_cache: Dict[int, Tuple[Dict, float]] = {}
|
||||||
self._cache_ttl = 60.0 # Cache for 60 seconds
|
self._cache_ttl = 60.0 # Cache for 60 seconds
|
||||||
|
|
||||||
|
def _track_view(self, msg_id: int, view: "ui.LayoutView") -> None:
|
||||||
|
"""Register a view in _active_views with automatic eviction on stop.
|
||||||
|
|
||||||
|
Every view stored here previously stayed forever: on_timeout and every
|
||||||
|
"close" button already call view.stop(), but nothing popped the
|
||||||
|
matching entry, so every single help/menu send permanently retained
|
||||||
|
one dict entry (holding the whole rendered page/embed list) — an
|
||||||
|
unbounded memory leak on any active server.
|
||||||
|
"""
|
||||||
|
self._active_views[msg_id] = view
|
||||||
|
original_stop = view.stop
|
||||||
|
|
||||||
|
def cleanup_stop():
|
||||||
|
original_stop()
|
||||||
|
self._active_views.pop(msg_id, None)
|
||||||
|
|
||||||
|
view.stop = cleanup_stop
|
||||||
|
|
||||||
async def cog_load(self) -> None:
|
async def cog_load(self) -> None:
|
||||||
"""Called when cog is loaded — set up monkey patches and replace help."""
|
"""Called when cog is loaded — set up monkey patches and replace help."""
|
||||||
await self._apply_patches()
|
await self._apply_patches()
|
||||||
|
|
@ -286,13 +304,7 @@ class NewHelpMenu(commands.Cog):
|
||||||
|
|
||||||
# Track for cleanup with automatic removal on stop
|
# Track for cleanup with automatic removal on stop
|
||||||
if msg:
|
if msg:
|
||||||
cog_ref._active_views[msg.id] = layout
|
cog_ref._track_view(msg.id, layout)
|
||||||
# Add cleanup callback
|
|
||||||
original_stop = layout.stop
|
|
||||||
def cleanup_stop():
|
|
||||||
original_stop()
|
|
||||||
cog_ref._active_views.pop(msg.id, None)
|
|
||||||
layout.stop = cleanup_stop
|
|
||||||
|
|
||||||
return msg
|
return msg
|
||||||
|
|
||||||
|
|
@ -373,7 +385,7 @@ class NewHelpMenu(commands.Cog):
|
||||||
)
|
)
|
||||||
msg = await destination.send(view=view)
|
msg = await destination.send(view=view)
|
||||||
view.message = msg
|
view.message = msg
|
||||||
self._active_views[msg.id] = view
|
self._track_view(msg.id, view)
|
||||||
else:
|
else:
|
||||||
embeds, select_opts = build_bot_help_embeds(
|
embeds, select_opts = build_bot_help_embeds(
|
||||||
ctx, self.bot, cat_data, total, accent_color=accent
|
ctx, self.bot, cat_data, total, accent_color=accent
|
||||||
|
|
@ -399,7 +411,7 @@ class NewHelpMenu(commands.Cog):
|
||||||
for comp in components:
|
for comp in components:
|
||||||
layout.add_item(comp)
|
layout.add_item(comp)
|
||||||
msg = await destination.send(view=layout)
|
msg = await destination.send(view=layout)
|
||||||
self._active_views[msg.id] = layout
|
self._track_view(msg.id, layout)
|
||||||
else:
|
else:
|
||||||
embed = build_command_help_embed(ctx, data, accent_color=accent)
|
embed = build_command_help_embed(ctx, data, accent_color=accent)
|
||||||
await destination.send(embed=embed)
|
await destination.send(embed=embed)
|
||||||
|
|
@ -414,7 +426,7 @@ class NewHelpMenu(commands.Cog):
|
||||||
for comp in components:
|
for comp in components:
|
||||||
layout.add_item(comp)
|
layout.add_item(comp)
|
||||||
msg = await destination.send(view=layout)
|
msg = await destination.send(view=layout)
|
||||||
self._active_views[msg.id] = layout
|
self._track_view(msg.id, layout)
|
||||||
else:
|
else:
|
||||||
embed = build_cog_help_embed(ctx, cog_name, cog_doc, cmds, accent_color=accent)
|
embed = build_cog_help_embed(ctx, cog_name, cog_doc, cmds, accent_color=accent)
|
||||||
await destination.send(embed=embed)
|
await destination.send(embed=embed)
|
||||||
|
|
@ -445,7 +457,7 @@ class NewHelpMenu(commands.Cog):
|
||||||
layout = ui.LayoutView()
|
layout = ui.LayoutView()
|
||||||
layout.add_item(container)
|
layout.add_item(container)
|
||||||
msg = await destination.send(view=layout)
|
msg = await destination.send(view=layout)
|
||||||
self._active_views[msg.id] = layout
|
self._track_view(msg.id, layout)
|
||||||
else:
|
else:
|
||||||
bot_avatar = self.bot.user.display_avatar.url if self.bot.user else None
|
bot_avatar = self.bot.user.display_avatar.url if self.bot.user else None
|
||||||
embed = discord.Embed(color=discord.Colour(accent))
|
embed = discord.Embed(color=discord.Colour(accent))
|
||||||
|
|
@ -1024,6 +1036,7 @@ class NewHelpMenu(commands.Cog):
|
||||||
await ctx.send("✅ All Components V2 settings reset to defaults (disabled).")
|
await ctx.send("✅ All Components V2 settings reset to defaults (disabled).")
|
||||||
|
|
||||||
@cv2.command(name="version")
|
@cv2.command(name="version")
|
||||||
|
@checks.admin_or_permissions(manage_guild=True)
|
||||||
async def cv2_version(self, ctx: commands.Context):
|
async def cv2_version(self, ctx: commands.Context):
|
||||||
"""Show the cog version."""
|
"""Show the cog version."""
|
||||||
await ctx.send(f"**NewHelpMenu** v{self.__version__} by {self.__author__}")
|
await ctx.send(f"**NewHelpMenu** v{self.__version__} by {self.__author__}")
|
||||||
|
|
@ -1073,5 +1086,5 @@ class NewHelpMenu(commands.Cog):
|
||||||
)
|
)
|
||||||
msg = await ctx.send(view=view)
|
msg = await ctx.send(view=view)
|
||||||
view.message = msg
|
view.message = msg
|
||||||
self._active_views[msg.id] = view
|
self._track_view(msg.id, view)
|
||||||
return msg
|
return msg
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue