AutoGPT icon indicating copy to clipboard operation
AutoGPT copied to clipboard

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

Open majdyz opened this issue 1 year ago โ€ข 5 comments

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 avatar Sep 26 '24 23:09 majdyz

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.

qodo-merge-pro[bot] avatar Sep 26 '24 23:09 qodo-merge-pro[bot]

Deploy Preview for auto-gpt-docs canceled.

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

netlify[bot] avatar Sep 26 '24 23:09 netlify[bot]

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

aarushik93 avatar Oct 02 '24 13:10 aarushik93

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

github-actions[bot] avatar Oct 02 '24 14:10 github-actions[bot]

Conflicts have been resolved! ๐ŸŽ‰ A maintainer will review the pull request shortly.

github-actions[bot] avatar Oct 02 '24 21:10 github-actions[bot]