Skip to content

Release stale deployment locks and add TTL#107

Merged
0xFirekeeper merged 2 commits into
mainfrom
firekeeper/deploy-lock
May 29, 2026
Merged

Release stale deployment locks and add TTL#107
0xFirekeeper merged 2 commits into
mainfrom
firekeeper/deploy-lock

Conversation

@0xFirekeeper

@0xFirekeeper 0xFirekeeper commented May 29, 2026

Copy link
Copy Markdown
Member

Reclaim stale deployment locks and prevent permanent lockout: aa-core now releases a stale lock when the account isn't deployed and returns NotDeployed so callers can retry. The external bundler uses Redis SET with NX + EX via SetOptions/SetExpiry and adds a 300s fallback LOCK_TTL_SECONDS to ensure locks auto-expire if a holder dies. This avoids stuck deployments and makes lock acquisition atomic with an expiry.

Summary by CodeRabbit

  • Bug Fixes
    • Deployment status detection improved: stale locks trigger a re-check of chain state and are safely reclaimed when the account is not deployed, preventing incorrect "deployed" reporting.
    • Locks now auto-expire after a fixed TTL and include ownership-protected release to avoid indefinite persistence and race conditions.

Review Change Stack

Reclaim stale deployment locks and prevent permanent lockout: aa-core now releases a stale lock when the account isn't deployed and returns NotDeployed so callers can retry. The external bundler uses Redis SET with NX + EX via SetOptions/SetExpiry and adds a 300s fallback LOCK_TTL_SECONDS to ensure locks auto-expire if a holder dies. This avoids stuck deployments and makes lock acquisition atomic with an expiry.
@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 494faa63-c80d-4c69-a339-bf63d520a888

📥 Commits

Reviewing files that changed from the base of the PR and between 351d14b and 65bf187.

📒 Files selected for processing (2)
  • aa-core/src/userop/deployment.rs
  • executors/src/external_bundler/deployment.rs

Walkthrough

Adds TTL-backed Redis deployment locks and an owner-checked unlock; stale locks trigger a chain re-check and are released when the account is confirmed not deployed.

Changes

Stale Deployment Lock Handling

Layer / File(s) Summary
Redis lock expiration and owner-checked release
executors/src/external_bundler/deployment.rs
Adds LOCK_TTL_SECONDS, switches acquisition to SET NX EX(...) via options, and implements release_lock_if_owner using a Lua compare-and-delete script.
Stale lock detection and cleanup
aa-core/src/userop/deployment.rs
DeploymentLock trait gains release_lock_if_owner. check_deployment_status re-checks chain on stale locks and calls release_lock_if_owner(..., &lock_id) then returns NotDeployed when the account is not deployed.

Sequence Diagram

sequenceDiagram
  participant Client
  participant DeploymentManager
  participant RedisDeploymentLock
  participant Redis
  participant Chain

  Client->>RedisDeploymentLock: acquire_lock
  RedisDeploymentLock->>Redis: SET key NX EX(LOCK_TTL_SECONDS)
  Redis-->>RedisDeploymentLock: Lock acquired atomically

  Client->>DeploymentManager: check_deployment_status
  DeploymentManager->>RedisDeploymentLock: Inspect lock age
  Note over DeploymentManager: Detect stale lock
  DeploymentManager->>Chain: check_chain
  Chain-->>DeploymentManager: Account not deployed
  DeploymentManager->>RedisDeploymentLock: release_lock_if_owner(chain_id, addr, lock_id)
  RedisDeploymentLock->>Redis: EVAL compare-and-delete Lua script
  Redis-->>RedisDeploymentLock: Deleted? (true/false)
  DeploymentManager-->>Client: Return NotDeployed
Loading

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Release stale deployment locks and add TTL' directly addresses the two main changes: stale lock release logic and TTL-based lock expiration, matching the core objectives.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch firekeeper/deploy-lock

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@aa-core/src/userop/deployment.rs`:
- Around line 108-111: The stale-lock cleanup currently calls
self.lock.release_lock(chain_id, account_address) unconditionally; change the
call site in deployment.rs to pass the observed lock_id returned by
check_lock(), and update the LockStore::release_lock implementation to perform
an atomic compare-and-delete (compare stored lock value to the provided lock_id
and only DEL if equal) e.g. via a Redis EVAL/Lua script or Redis CAS primitive;
keep the return behavior (Ok(DeploymentStatus::NotDeployed)) but ensure only the
exact observed lock_id is removed to avoid deleting a freshly re-acquired lock.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 66a6fecf-d45d-446e-8721-44307c523d21

📥 Commits

Reviewing files that changed from the base of the PR and between b6f39f4 and 351d14b.

📒 Files selected for processing (2)
  • aa-core/src/userop/deployment.rs
  • executors/src/external_bundler/deployment.rs

Comment thread aa-core/src/userop/deployment.rs
Add a DeploymentLock::release_lock_if_owner method and use it when reclaiming stale deployment locks to avoid deleting a lock that another worker may have acquired. Implemented RedisDeploymentLock::release_lock_if_owner with an atomic compare-and-delete Lua script (GET + cjson.decode + DEL if lock_id matches) and map Redis errors to EngineError. Update aa-core deployment flow to call the new method instead of unconditionally deleting the lock.
@0xFirekeeper 0xFirekeeper merged commit b6b7a0b into main May 29, 2026
3 checks passed
@0xFirekeeper 0xFirekeeper deleted the firekeeper/deploy-lock branch May 29, 2026 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant