feat: Add instanceId to distributed lock keys for multi-instance isolation #40966
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DistributedLockAspect
participant InstanceIdProvider
participant Redis
Caller->>DistributedLockAspect: Request lock (handleMono/handleFlux/handleBlocking)
DistributedLockAspect->>InstanceIdProvider: getInstanceId()
InstanceIdProvider-->>DistributedLockAspect: instanceId (or "unknown")
DistributedLockAspect->>DistributedLockAspect: createLockDetails(lock, instanceId)
DistributedLockAspect->>Redis: Attempt to acquire lock with key "lock:<instanceId>:<lockKey>"
Redis-->>DistributedLockAspect: Lock acquired/rejected
DistributedLockAspect-->>Caller: Result (proceed or error)
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/reactive-caching/src/main/java/com/appsmith/caching/aspects/DistributedLockAspect.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (4)
app/server/reactive-caching/src/main/java/com/appsmith/caching/aspects/DistributedLockAspect.java (4)
4-4: LGTM!The ConfigService dependency is properly injected through the constructor.
Also applies to: 27-27, 31-34
63-71: Verify the lock key format with double colon.The lock key format will be
lock::{instanceId}:{key}due to the concatenation ofLOCK_PREFIX("lock:") with an additional colon. Is this intentional?
88-109: LGTM!The reactive chain correctly handles the async lock details creation.
113-134: LGTM!Correctly uses
flatMapManyfor the Mono to Flux transition.
...erver/reactive-caching/src/main/java/com/appsmith/caching/aspects/DistributedLockAspect.java
Show resolved
Hide resolved
…nceId for lock keys - Modified DistributedLockAspect to utilize InstanceIdProvider for generating lock keys, ensuring multi-instance isolation. - Updated test cases in DistributedLockAspectTest to reflect changes in lock key generation, improving test reliability and clarity. - Adjusted releaseLock method in TestLockService to include instanceId in lock key deletion, enhancing lock management.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/DistributedLockAspectTest.java (2)
32-40: DRY up common test utilities
LOCK_PREFIX,TEST_INSTANCE_ID, andgetLockKey(...)are great, but every test still has to remember to stubinstanceIdProvider. Moving the stub and these constants into a@BeforeEach(or a nestedstaticutil class) would reduce duplication and the chance of forgetting the stub in future tests.@BeforeEach void setUp() { when(instanceIdProvider.getInstanceId()).thenReturn(Mono.just(TEST_INSTANCE_ID)); }All individual
when(...).thenReturn(...)calls below can then be removed.
145-146: Order of parameters is non-intuitive
releaseLock("persistent-lock", redisOperations, TEST_INSTANCE_ID)passes the Redis client before theinstanceId, whereas most APIs put context (instanceId) before infrastructure (redis). Swapping them or using named builders would improve readability, but at minimum document the order to avoid accidental misuse.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceIdProviderImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/DistributedLockAspectTest.java(6 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/TestLockService.java(1 hunks)app/server/reactive-caching/src/main/java/com/appsmith/caching/aspects/DistributedLockAspect.java(6 hunks)app/server/reactive-caching/src/main/java/com/appsmith/caching/components/InstanceIdProvider.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/server/reactive-caching/src/main/java/com/appsmith/caching/components/InstanceIdProvider.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/InstanceIdProviderImpl.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/reactive-caching/src/main/java/com/appsmith/caching/aspects/DistributedLockAspect.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/DistributedLockAspectTest.java (1)
38-40: Delimiter differs from PR description – verify onceThe helper builds keys as
lock:<instanceId>:<key>(single colon afterlock).
The PR description mentionslock::<instanceId>:<key>(double colon). Please confirm the delimiter is consistent across production code, docs, and tests to avoid elusive bugs in multi-instance deployments.
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/TestLockService.java
Show resolved
Hide resolved
…add-instance-id-for-distributed-lock-keys
…hub.com:appsmithorg/appsmith into chore/add-instance-id-for-distributed-lock-keys
- Introduced TestConfig class to provide a mock InstanceIdProvider for testing purposes. - Created TestInstanceIdProvider class to return a static instance ID, facilitating unit tests for caching components.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/reactive-caching/src/test/java/com/appsmith/testcaching/TestConfig.java(1 hunks)app/server/reactive-caching/src/test/java/com/appsmith/testcaching/TestInstanceIdProvider.java(1 hunks)app/server/reactive-caching/src/test/java/com/appsmith/testcaching/test/TestCachingMethods.java(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/server/reactive-caching/src/test/java/com/appsmith/testcaching/test/TestCachingMethods.java
- app/server/reactive-caching/src/test/java/com/appsmith/testcaching/TestInstanceIdProvider.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
Description
This PR modifies the
DistributedLockAspectto include the instanceId in Redis lock keys, ensuring proper isolation between different Appsmith instances in distributed deployments.Changes Made
lock:{key}tolock::{instanceId}:{key}handleMono,handleFlux,handleBlocking) to work with the new async lock details creationWhy This Change?
Previously, distributed locks were shared across all Appsmith instances using the same Redis instance, which could lead to:
Related Issues
Fixes issues related to distributed lock conflicts in multi-instance Appsmith deployments with shared Redis.
/test Sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15732404717
Commit: 06c6f14
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 18 Jun 2025 20:47:06 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes