Skip to content
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

feat(backend): Make Redis connection Sync + Use Redis as Distributed Lock #8197

Conversation

majdyz
Copy link
Contributor

@majdyz majdyz commented Sep 26, 2024

Background

Main issue:
We rely on the idea of rest_api being a singleton service to maintain our distributed lock. This will cause issues when the number of services increases.

Side issue:
The Redis operation that is used in our system has nothing to do with our API request:

  • For read & write to queue done by a background process.
  • (Added) To acquire a mutually exclusive lock.

Using Redis on asyncio will add unnecessary overhead and overscribe the event loop shared by Prisma queries.

Changes 🏗️

  • Migrate distributed lock from rest_api to Redis.
  • Switch Redis to synchronous client.
  • Cleanup on connection retry logic on db.py and redis.py (newly introduced).
  • Remove InMemoryEventQueue and make integration test use Redis queue.

Testing 🔍

Note

Only for the new autogpt platform, currently in autogpt_platform/

  • Create from scratch and execute an agent with at least 3 blocks
  • Import an agent from file upload, and confirm it executes correctly
  • Upload agent to marketplace
  • Import an agent from marketplace and confirm it executes correctly
  • Edit an agent from monitor, and confirm it executes correctly

@majdyz majdyz requested a review from aarushik93 September 26, 2024 23:16
@majdyz majdyz requested a review from a team as a code owner September 26, 2024 23:16
@majdyz majdyz requested review from Torantulino and Swiftyos and removed request for a team September 26, 2024 23:16
@github-actions github-actions bot added platform/backend AutoGPT Platform - Back end size/l labels Sep 26, 2024
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
The PR introduces Redis connection details in autogpt_platform/backend/backend/data/redis.py. While environment variables are used, there's a risk of exposing default values (e.g., "password" for Redis password) if the environment variables are not properly set.

⚡ Key issues to review

Error Handling
The connect() and disconnect() functions use logged_retry without proper error handling or maximum retry limit.

Error Handling
The RedisEventQueue methods lack proper error handling for Redis connection issues.

Potential Deadlock
The synchronized context manager acquires a lock but doesn't handle potential exceptions, which could lead to a lock not being released.

Copy link

netlify bot commented Sep 26, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit 2bfe62b
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/6703920dd0c75d00089c3cac

@majdyz majdyz requested a review from a team as a code owner September 27, 2024 01:25
@majdyz majdyz requested review from Pwuts and removed request for Torantulino and Swiftyos September 27, 2024 01:26
… zamilmajdy/secrt-866-we-currently-lock-the-rest-server-whilst-registering-next
@majdyz majdyz requested a review from kcze September 27, 2024 16:34
@Pwuts Pwuts changed the title feat(platform): Make Redis connection Sync + Use Redis as Distributed Lock feat(backend): Make Redis connection Sync + Use Redis as Distributed Lock Sep 30, 2024
@majdyz majdyz requested a review from Pwuts October 1, 2024 20:11
@aarushik93
Copy link
Contributor

testing this PR today, couldn't get to it yesterday

Copy link
Contributor

github-actions bot commented Oct 2, 2024

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Oct 2, 2024
majdyz added 2 commits October 2, 2024 23:15
…y-lock-the-rest-server-whilst-registering-next' into zamilmajdy/secrt-866-we-currently-lock-the-rest-server-whilst-registering-next
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Oct 2, 2024
Copy link
Contributor

github-actions bot commented Oct 2, 2024

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@majdyz majdyz requested a review from Swiftyos October 4, 2024 09:11
@majdyz majdyz enabled auto-merge (squash) October 7, 2024 08:38
@majdyz majdyz merged commit daa054c into master Oct 7, 2024
14 checks passed
@majdyz majdyz deleted the zamilmajdy/secrt-866-we-currently-lock-the-rest-server-whilst-registering-next branch October 7, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants