fix: keep Discord startup alive on command quota#8061
Open
he-yufeng wants to merge 1 commit intoAstrBotDevs:masterfrom
Open
fix: keep Discord startup alive on command quota#8061he-yufeng wants to merge 1 commit intoAstrBotDevs:masterfrom
he-yufeng wants to merge 1 commit intoAstrBotDevs:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Catching a broad
Exceptionin_collect_and_register_commandsmakes it easy to hide unrelated failures; consider narrowing this to the specific Pycord/HTTP exception type you expect and logging the full exception when re-raising for easier debugging. - The
_is_daily_command_quota_errorhelper currently falls back to substring matching on'30034'in the message; if possible, prefer inspecting structured attributes (like a response code or error type) to avoid false positives and keep the behavior resilient to message text changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Catching a broad `Exception` in `_collect_and_register_commands` makes it easy to hide unrelated failures; consider narrowing this to the specific Pycord/HTTP exception type you expect and logging the full exception when re-raising for easier debugging.
- The `_is_daily_command_quota_error` helper currently falls back to substring matching on `'30034'` in the message; if possible, prefer inspecting structured attributes (like a response code or error type) to avoid false positives and keep the behavior resilient to message text changes.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/discord/discord_platform_adapter.py" line_range="443-445" />
<code_context>
+ return
+ raise
+
+ @staticmethod
+ def _is_daily_command_quota_error(error: Exception) -> bool:
+ return getattr(error, "code", None) == 30034 or "30034" in str(error)
def _create_dynamic_callback(self, cmd_name: str):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Make the quota error check more robust to avoid false positives on unrelated errors.
Using `"30034" in str(error)` can incorrectly flag unrelated errors that contain this substring or mention the code in a different context. Prefer relying on structured data (e.g., an exception `code`/`status`) and the specific exception type. If a string fallback is necessary, at least restrict it with an `isinstance` check on the expected error class to reduce misclassification.
Suggested implementation:
```python
@staticmethod
def _is_daily_command_quota_error(error: Exception) -> bool:
"""
Return True if the error represents the Discord daily application command
creation quota being reached (error code 30034).
Prefer using structured data (the `code` attribute) and only fall back to
string inspection for the expected HTTP error type to avoid false positives.
"""
code = getattr(error, "code", None)
if code == 30034:
return True
# Fallback: only inspect the message for the expected HTTP error type
# to reduce misclassification of unrelated exceptions.
if isinstance(error, ExpectedDiscordHTTPErrorType):
message = str(error)
# Restrict to a more specific pattern instead of any occurrence of "30034".
return "30034" in message and "application command" in message.lower()
return False
```
To integrate this cleanly with your codebase:
1. Replace `ExpectedDiscordHTTPErrorType` with the actual exception class used for Discord HTTP errors in your project (for example, `discord.HTTPException` or the equivalent from your Discord client library).
2. If necessary, adjust the `"application command"` substring to match the exact phrase used in the error message your library returns for code `30034` (e.g., `"application command create"` or `"application command daily limit"`).
3. Ensure the corresponding Discord library (e.g., `discord`) is imported where needed (either at the top of the module or via a scoped import) so that the chosen error type is available.
</issue_to_address>
### Comment 2
<location path="tests/test_discord_command_sync.py" line_range="42" />
<code_context>
+ code=30034,
+ )
+
+ await adapter._collect_and_register_commands()
+
+ adapter.client.sync_commands.assert_awaited_once()
</code_context>
<issue_to_address>
**suggestion (testing):** Assert that a warning log is emitted when the daily quota is hit, not just that the exception is ignored.
Since this change now logs a warning when the daily quota is reached, update the test to assert that warning via `caplog` (or your logging helper) in addition to `sync_commands` being awaited once. This will catch future refactors that might inadvertently drop the warning.
Suggested implementation:
```python
@pytest.mark.asyncio
async def test_discord_command_sync_ignores_daily_quota(monkeypatch, caplog):
adapter = _build_adapter(monkeypatch)
adapter.client.sync_commands.side_effect = DiscordSyncError(
"Max number of daily application command creates reached",
code=30034,
)
with caplog.at_level(logging.WARNING):
await adapter._collect_and_register_commands()
adapter.client.sync_commands.assert_awaited_once()
warning_records = [
record
for record in caplog.records
if record.levelname == "WARNING"
and "Max number of daily application command creates reached"
in record.getMessage()
]
assert warning_records, "Expected a WARNING log when the daily quota is reached"
```
To fully support this change, ensure that:
1. `logging` is imported at the top of `tests/test_discord_command_sync.py`, e.g.:
`import logging`
2. If the application logs through a specific logger (e.g. `logger = logging.getLogger(__name__)` in the adapter module), you may want to scope `caplog` to that logger by passing `logger=...` to `caplog.at_level` if your test suite prefers that pattern.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements error handling for Discord's daily command creation quota (error code 30034) during synchronization, ensuring the process skips gracefully with a warning instead of failing. New unit tests verify that this specific error is handled while others remain fatal. Feedback suggests improving the error detection logic by explicitly checking for discord.HTTPException to avoid potential false positives.
01ac222 to
f6b2b4c
Compare
Contributor
Author
|
Thanks for the review. I pushed f6b2b4c to address the two points:
The CI matrix is green now. |
This file contains hidden or 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
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.
Fixes #8029.
What changed
Discord command registration now treats error code
30034as a recoverable daily quota condition. When Pycord reports that Discord's daily application command create quota is exhausted, AstrBot logs a warning and continues startup instead of failing the whole platform adapter.Other
sync_commands()errors still raise. Missing access, bad tokens, network errors, and other real setup problems should remain visible instead of being silently swallowed.Why
The shutdown path already wraps
sync_commands(commands=[])with error handling, but the startup registration path calledsync_commands()bare. Discord 30034 is a quota/backoff condition: already-registered commands can still work, and restarting AstrBot should not be blocked until the next quota reset.Tests
Summary by Sourcery
Handle Discord application command sync quota errors without failing startup
Bug Fixes:
Tests: