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

fix(backend): Added locking status check before releasing to avoid releasing timing out lock #9135

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

majdyz
Copy link
Contributor

@majdyz majdyz commented Dec 27, 2024

Exception:

nid:ce829f66-14b0-4bd3-b748-791e46666cb6|-] Failed node execution ce829f66-14b0-4bd3-b748-791e46666cb6: Cannot release an unlocked lock {}\u001b[0m",
Traceback (most recent call last):\n  File \"/app/autogpt_platform/backend/backend/integrations/creds_manager.py\", line 145, in _locked\n    yield\n  File \"/app/autogpt_platform/backend/backend/integrations/creds_manager.py\", line 115, in acquire\n    lock = self._acquire_lock(user_id, credentials_id)",
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^",
  File \"/app/autogpt_platform/backend/backend/integrations/creds_manager.py\", line 139, in _acquire_lock",
    return self._locks.acquire(key)",
           ^^^^^^^^^^^^^^^^^^^^^^^^",
  File \"/app/autogpt_platform/autogpt_libs/autogpt_libs/utils/synchronize.py\", line 44, in acquire",
    lock.acquire()",
  File \"/usr/local/lib/python3.11/site-packages/redis/lock.py\", line 218, in acquire",
    mod_time.sleep(sleep)",
  File \"/app/autogpt_platform/backend/backend/executor/manager.py\", line 471, in <lambda>",
    signal.SIGTERM, lambda _, __: cls.on_node_executor_sigterm()",
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^",
  File \"/app/autogpt_platform/backend/backend/executor/manager.py\", line 498, in on_node_executor_sigterm",
    sys.exit(0)",
SystemExit: 0",
During handling of the above exception, another exception occurred:",
Traceback (most recent call last):\n  File \"/app/autogpt_platform/backend/backend/executor/manager.py\", line 539, in _on_node_execution\n    for execution in execute_node(\n  File \"/app/autogpt_platform/backend/backend/executor/manager.py\", line 175, in execute_node\n    credentials, creds_lock = creds_manager.acquire(user_id, credentials_meta.id)",
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^",
  File \"/app/autogpt_platform/backend/backend/integrations/creds_manager.py\", line 114, in acquire",
    with self._locked(user_id, credentials_id, \"!time_sensitive\"):",
  File \"/usr/local/lib/python3.11/contextlib.py\", line 158, in __exit__",
    self.gen.throw(typ, value, traceback)",
  File \"/app/autogpt_platform/backend/backend/integrations/creds_manager.py\", line 147, in _locked",
    lock.release()",
  File \"/usr/local/lib/python3.11/site-packages/redis/lock.py\", line 254, in release",
    raise LockError(\"Cannot release an unlocked lock\", lock_name=self.name)",
redis.exceptions.LockError: Cannot release an unlocked lock",

Changes 🏗️

try:
   lock.acquire()
   ...
finally:
   lock.release()

pattern can cause an error where the lock is already released due to timeout.

The scope of the change is to manually check the lock status before releasing.

Checklist 📋

For code changes:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • ...
Example test plan
  • 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

For configuration changes:

  • .env.example is updated or already compatible with my changes
  • docker-compose.yml is updated or already compatible with my changes
  • I have included a list of my configuration changes in the PR description (under Changes)
Examples of configuration changes
  • Changing ports
  • Adding new services that need to communicate with each other
  • Secrets or environment variable changes
  • New or infrastructure changes such as databases

@majdyz majdyz requested a review from a team as a code owner December 27, 2024 22:44
@github-actions github-actions bot added the platform/backend AutoGPT Platform - Back end label Dec 27, 2024
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Lock Release Validation

The new lock release condition in the release() method checks for lock.owned(), but this ownership check may not be sufficient in all distributed scenarios. Consider validating if this covers all edge cases.

if (lock := self.locks.get(key)) and lock.locked() and lock.owned():
    lock.release()

Copy link

netlify bot commented Dec 27, 2024

Deploy Preview for auto-gpt-docs-dev canceled.

Name Link
🔨 Latest commit e45f9ab
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs-dev/deploys/676f2de54a8c1f0008818811

Copy link

netlify bot commented Dec 27, 2024

Deploy Preview for auto-gpt-docs ready!

Name Link
🔨 Latest commit e45f9ab
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/676f2de5f5b3ac0008f29ad1
😎 Deploy Preview https://deploy-preview-9135--auto-gpt-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@majdyz majdyz enabled auto-merge December 27, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant