feat(platform): Deduplicate insufficient funds Discord + email notifications #11672
Conversation
### Changes 🏗️ Chat should be disabled by default; otherwise, it flashes, and if Launch Darkly fails to fail, it is dangerous. ### Checklist 📋 #### For code changes: - [x] I have clearly listed my changes in the PR description - [x] I have made a test plan - [x] I have tested my changes according to the test plan: - [x] Run locally with Launch Darkly disabled and test the above
Add Redis-based deduplication for Discord notifications when users have insufficient credits. This prevents spamming the PRODUCT Discord channel with repeated alerts for the same user+agent combination. Changes: - Add Redis SET NX to track sent notifications per user+agent - Clear notification flags when user tops up credits - Add comprehensive tests for the deduplication logic The notification flags use a 30-day TTL as a fallback cleanup, but are actively cleared when users top up via _top_up_credits or fulfill_checkout.
…unds notifications Move the Redis deduplication check to the beginning of the function so both email and Discord notifications are skipped for duplicates. Users only need one notification per agent when they run out of funds.
ntindle
requested review from
Bentlybro and
kcze
and removed request for
a team
github-project-automation
bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR targets the Automatically setting the base branch to |
|
Thank you for this PR to deduplicate insufficient funds Discord notifications. The implementation looks solid with good test coverage. Feedback:
Please address the missing checklist and clarify the feature flag change before we can proceed with merging. |
|
Thank you for this PR implementing Redis-based deduplication for Discord notifications. The feature looks well-designed and thoroughly tested. However, according to our PR requirements, you need to include the checklist in your PR description. Please update your PR description to include the standard checklist, making sure to check off all the relevant items to show that you've:
The implementation itself looks solid with:
Please update the PR description with the checklist, and this should be ready for approval. |
There was a problem hiding this comment.
Pull request overview
This PR adds Redis-based deduplication to prevent spamming Discord notifications when users have insufficient credits. The system tracks sent notifications per user+agent combination and automatically clears these flags when users top up credits.
Key changes:
- Implements Redis SET NX deduplication logic with 30-day TTL fallback for insufficient funds Discord alerts
- Adds
clear_insufficient_funds_notifications()function that removes all notification flags when users add credits - Includes comprehensive test coverage for deduplication scenarios, Redis errors, and multi-agent handling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
autogpt_platform/backend/backend/executor/manager.py |
Adds Redis deduplication logic to _handle_insufficient_funds_notif and implements clear_insufficient_funds_notifications function |
autogpt_platform/backend/backend/data/credit.py |
Integrates notification clearing in _top_up_credits and fulfill_checkout to reset flags when users add credits |
autogpt_platform/backend/backend/executor/manager_insufficient_funds_test.py |
Adds comprehensive test suite covering first-time notifications, duplicate skipping, multi-agent scenarios, Redis errors, and cleanup functionality |
autogpt_platform/frontend/src/services/feature-flags/use-get-flag.ts |
Changes CHAT feature flag from true to false in mock flags |
autogpt_platform/frontend/src/services/feature-flags/use-get-flag.ts
Outdated
Show resolved
Hide resolved
| # Lazy import to avoid circular dependency with executor.manager | ||
| from backend.executor.manager import clear_insufficient_funds_notifications | ||
|
|
||
| clear_insufficient_funds_notifications(user_id) |
There was a problem hiding this comment.
The synchronous function clear_insufficient_funds_notifications is being called from async context without using asyncio.to_thread or similar mechanisms. This could block the event loop when performing Redis operations (scan_iter and delete). Consider making this function async and using get_redis_async() instead, or wrap the call in asyncio.to_thread() to avoid blocking the event loop during I/O operations.
| # Lazy import to avoid circular dependency with executor.manager | ||
| from backend.executor.manager import clear_insufficient_funds_notifications | ||
|
|
||
| clear_insufficient_funds_notifications(credit_transaction.userId) |
There was a problem hiding this comment.
The synchronous function clear_insufficient_funds_notifications is being called from async context without using asyncio.to_thread or similar mechanisms. This could block the event loop when performing Redis operations (scan_iter and delete). Consider making this function async and using get_redis_async() instead, or wrap the call in asyncio.to_thread() to avoid blocking the event loop during I/O operations.
|
Thank you for this PR implementing Redis-based deduplication for insufficient funds notifications. The code changes look well-structured with comprehensive test coverage. However, I noticed that the PR description is missing the required checklist section for code changes. Could you please update your PR description to include the checklist with items such as:
Once you've added this information, we can proceed with the review process. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
autogpt_platform/frontend/src/services/feature-flags/use-get-flag.ts
Outdated
Show resolved
Hide resolved
ntindle
changed the title
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
|
@claude can you update the pr description to match the current code + the pr template |
|
Thanks for this PR to implement Redis-based deduplication for insufficient funds notifications! The code changes look well-implemented, with comprehensive test coverage covering different scenarios. The approach of using Redis SET NX with TTL is a good solution for this problem. However, according to our PR guidelines, your PR description is missing the required checklist section that should include information about how you've tested these changes. Although you've added excellent test coverage, please update the PR description to include the checklist section from our template with your test plan. Once you add the checklist with a proper test plan, this PR should be ready for approval. The implementation itself looks solid and the documentation in the code is clear. |
|
Claude finished @ntindle's task in 1m 5s —— View job Updated PR Description
I've updated the PR description to include:
|
Add Redis-based deduplication for insufficient funds notifications (both Discord alerts and user emails) when users run out of credits. This prevents spamming users and the PRODUCT Discord channel with repeated alerts for the same user+agent combination.
Changes 🏗️
Redis-based deduplication (
backend/executor/manager.py):INSUFFICIENT_FUNDS_NOTIFIED_PREFIXconstant for Redis key prefixINSUFFICIENT_FUNDS_NOTIFIED_TTL_SECONDS(30 days) as fallback cleanup_handle_insufficient_funds_notifusing RedisSET NXZERO_BALANCE) and Discord notifications for duplicate alerts per user+agentclear_insufficient_funds_notifications(user_id)function to remove all notification flags for a userClear flags on credit top-up (
backend/data/credit.py):clear_insufficient_funds_notificationsin_top_up_creditsafter successful auto-chargeclear_insufficient_funds_notificationsinfulfill_checkoutafter successful manual top-upComprehensive test coverage (
backend/executor/manager_insufficient_funds_test.py):Checklist 📋
For code changes:
For configuration changes:
.env.defaultis updated or already compatible with my changes (no changes needed - uses existing Redis)docker-compose.ymlis updated or already compatible with my changes (no changes needed)