feat: Add Redis lock strategy #4617

simzzz · 2025-11-17T14:24:53Z

Description

This PR introduces the class that will implement locks in Redis in order to support the new Nonce Ordering feature

Related issue(s)

Fixes #4594

Changes from original design (optional)

N/A

Additional work needed (optional)

N/A

Checklist

I've assigned an assignee to this PR and related issue(s) (if applicable)
I've assigned a label to this PR and related issue(s) (if applicable)
I've assigned a milestone to this PR and related issue(s) (if applicable)
I've updated documentation (code comments, README, etc. if applicable)
I've done sufficient testing (unit, integration, etc.)

github-actions · 2025-11-17T14:49:58Z

Test Results

20 files ±0 272 suites ±0 21m 44s ⏱️ -29s
791 tests ±0 787 ✅ +1 4 💤 ±0 0 ❌ - 1
807 runs ±0 803 ✅ +1 4 💤 ±0 0 ❌ - 1

Results for commit 643c24f. ± Comparison against base commit c004a57.

codecov · 2025-11-18T13:18:17Z

Codecov Report

❌ Patch coverage is 65.91928% with 76 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../src/lib/services/lockService/RedisLockStrategy.ts	63.90%	74 Missing ⚠️
...rc/lib/services/lockService/LockStrategyFactory.ts	33.33%	2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (65.91%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

@@                        Coverage Diff                        @@
##             feat/nonce-ordering-with-locks    #4617   +/-   ##
=================================================================
  Coverage                                  ?   94.11%           
=================================================================
  Files                                     ?      133           
  Lines                                     ?    21232           
  Branches                                  ?     1719           
=================================================================
  Hits                                      ?    19982           
  Misses                                    ?     1229           
  Partials                                  ?       21

Flag	Coverage Δ
config-service	`98.85% <100.00%> (?)`
relay	`90.57% <63.46%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ckages/config-service/src/services/globalConfig.ts	`100.00% <100.00%> (ø)`
...rc/lib/services/lockService/LockStrategyFactory.ts	`78.12% <33.33%> (ø)`
.../src/lib/services/lockService/RedisLockStrategy.ts	`63.90% <63.90%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

docs/configuration.md

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

packages/relay/tests/lib/services/lockService/RedisLockStrategy.spec.ts

quiet-node

Left some concerns and comments

quiet-node · 2025-11-19T18:12:24Z

docs/configuration.md

 | `USE_ASYNC_TX_PROCESSING`                   | "true"                                | Set to `true` to enable `eth_sendRawTransaction` to return the transaction hash immediately after passing all prechecks, while processing the transaction asynchronously in the background.                                                                                                                                                                                                                                                                                                |
+| `LOCK_MAX_HOLD_MS`                          | "30000"                               | Maximum time in milliseconds a lock can be held before automatic force-release. This TTL prevents deadlocks when transaction processing hangs or crashes. Default is 30 seconds.                                                                                                                                                                                                                                                                                                           |
+| `LOCK_QUEUE_POLL_INTERVAL_MS`               | "50"                                  | Polling interval in milliseconds for Redis lock queue checks. Lower values reduce latency but increase Redis load. Default is 50ms.                                                                                                                                                                                                                                                                                                                                                        |
+| `LOCK_REDIS_PREFIX`                         | "lock"                                | Redis key prefix for lock-related keys. Useful for namespace isolation in shared Redis instances. Default is "lock".                                                                                                                                                                                                                                                                                                                                                                       |


Do we need this as a configuration? I think a hardcoded value inside Redis Lock strategy is good enough, right?

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

quiet-node · 2025-11-19T18:20:14Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+  async acquireLock(address: string): Promise<string> {
+    const normalizedAddress = this.normalizeAddress(address);
+    const sessionKey = randomUUID();
+    const lockKey = this.getLockKey(normalizedAddress);


I feel like we’ve had this conversation before, right? Not sure when but anywho, requiring the key to be normalized before being passed in adds unnecessary complexity and risks producing different types of keys for different operations, since it allows unnormalized parameters to be passed. It also leads to repeated calls to this.normalizeAddress() at the call sites.

Would it make sense to handle normalization inside getLockKey? This way, whenever we call this.getLockKey() with any type of parameter, it would automatically be normalized, ensuring that getLockKey() reliably produces a lock key that is always consistent and accurate, regardless of the case sensitivity of the input.

quiet-node · 2025-11-19T18:35:47Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+    const normalizedAddress = this.normalizeAddress(address);
+    const sessionKey = randomUUID();
+    const lockKey = this.getLockKey(normalizedAddress);
+    const queueKey = this.getQueueKey(normalizedAddress);


Same for getQueueKey, I think it would be more intuitive to have normalization inside getQueueKey.

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

quiet-node · 2025-11-19T19:04:52Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+      });
+
+      if (result === 1) {
+        this.logger.info(`Lock released: address=${normalizedAddress}, sessionKey=${sessionKey}`);


Same for this log would it make more sense to use debug?

quiet-node · 2025-11-19T19:07:55Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+
+      const redisError = new RedisCacheError(error);
+      this.logger.error(redisError, `Failed to acquire lock: address=${normalizedAddress}, sessionKey=${sessionKey}`);
+      throw redisError;


Also on Tuesday the team decided to go with Fail Open approach so we don't need to throw here but rather return null and ignore.

quiet-node · 2025-11-19T19:09:47Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+        }
+      }
+    } catch (error) {
+      const redisError = new RedisCacheError(error);


Let's not wrap it into RedisCacheError so we can avoid confusion

quiet-node · 2025-11-19T19:11:32Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+   * @param sessionKey - The session key to remove.
+   * @param address - The address (for logging).
+   */
+  private async cleanupFromQueue(queueKey: string, sessionKey: string, address: string): Promise<void> {


Should we repurpose this and make it usable for both removing it after successful acquisition or fail acquisition? rPop(queueKey) and lRem(queueKey, 1, sessionKey) technically work the same, right? If it makes sense to repurpose for both successful and fail cases we can have it triggered in the finally() block of the acquireLock() method, it'll be a little cleaner.

They're not quite the same and I think it's better to keep it as it is, rPop removes the last element in list (which is the first in the queue) while lRem removes the first occurrence of a specific value in the list searching from head to tail. For the success case we use rPop because we know that we're at the end of the list. For failed acquisition we use lRem because we might be anywhere in the queue. If an error occurs during the polling loop, we could be in the middle of the queue, not necessarily at the end.

Ah yeah I understand that. I meant we should use lRem for both cases, not rPop. Since sessionKeys are unique, rPop removes the last item in the queue, which is the current sessionKey, and lrem(queueKey, sessionKey, 1) would remove that same sessionKey as well.

So we can update the method to use lRem, and then call it only once in the finally block because we want to remove the sessionKey regardless of lock acquisistion status.

This makes the code cleaner while keeping the behavior exactly the same.

I understood you now, yes that's a cleaner approach. I will apply it

quiet-node · 2025-11-19T19:14:43Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+    this.maxLockHoldMs = ConfigService.get('LOCK_MAX_HOLD_MS' as any) as number;
+    this.pollIntervalMs = ConfigService.get('LOCK_QUEUE_POLL_INTERVAL_MS' as any) as number;
+    this.keyPrefix = ConfigService.get('LOCK_REDIS_PREFIX' as any) as string;
+  }


Why do we need to add as any to these calls? ConfigService should automatically define the type based on what we put for these configs in GlobalConfig object, right?

Signed-off-by: Simeon Nakov <simeon.nakov@limechain.tech>

simzzz · 2025-11-20T11:27:29Z

your comments have been addressed @quiet-node @konstantinabl

Signed-off-by: Simeon Nakov <simeon.nakov@limechain.tech>

quiet-node

Looking better left some more items

quiet-node · 2025-11-20T16:43:26Z

packages/relay/src/lib/services/lockService/LockStrategyFactory.ts

+      return new RedisLockStrategy(redisClient, logger.child({ name: 'redis-lock-strategy' }));
    }

    return new LocalLockStrategy(logger);


Can be updated in the next follow up PR but can we add a name for this logger?

quiet-node · 2025-11-20T16:46:20Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+  private readonly redisClient: RedisClientType;
+  private readonly logger: Logger;
+  private readonly maxLockHoldMs: number;
+  private readonly pollIntervalMs = 50;


I think I made a misleading comment earlier. I meant we just need the keyPrefix to be a hardcoded value, but pollIntervalMs can still be configurable, right? Also can address in next follow up PR

quiet-node · 2025-11-20T16:51:05Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+      return null;
+    } finally {
+      // Always remove from queue if we joined it (whether success or failure)
+      if (joinedQueue) {


non blocking but looks like we don't need joinedQueue, right? lRem(queueKey, 1, sessionKey) will remove sessionKey if it's there or it will ignore if it's not there, right?

In the case that lPush fails we would try to remove from the queue something that's guaranteed to not be there which would be an extra network call to Redis. It's a very small optimization but I suggest we keep it

Oh yeah that's fair! Thanks for the explanation

quiet-node · 2025-11-20T16:52:15Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+  private async removeFromQueue(queueKey: string, sessionKey: string, address: string): Promise<void> {
+    try {
+      await this.redisClient.lRem(queueKey, 1, sessionKey);
+      this.logger.trace(`Removed from queue: address=${address}, sessionKey=${sessionKey}`);


Let's still add the log level guard check to make sure JS doesn't build the string if not necessary

quiet-node · 2025-11-20T16:53:56Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+      );
+
+      if (result === 1) {
+        this.logger.debug(`Lock released: address=${address}, sessionKey=${sessionKey}`);


Let's still add the log level guard check to make sure JS doesn't build the string if not necessary

quiet-node · 2025-11-20T16:57:42Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+            const acquisitionDuration = Date.now() - startTime;
+            const queueLength = await this.redisClient.lLen(queueKey);
+
+            this.logger.debug(


Let's still add the log level guard check to make sure JS doesn't build the string if not necessary

quiet-node · 2025-11-20T16:59:21Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+      await this.redisClient.lRem(queueKey, 1, sessionKey);
+      this.logger.trace(`Removed from queue: address=${address}, sessionKey=${sessionKey}`);
+    } catch (error) {
+      this.logger.error(error, `Failed to remove from queue: address=${address}, sessionKey=${sessionKey}`);


Should we make this as warn level, not an error? I don't think it's critical enough to be an error yeah?

quiet-node · 2025-11-20T17:01:45Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+        }
+      }
+    } catch (error) {
+      this.logger.error(error, `Failed to release lock: address=${address}, sessionKey=${sessionKey}`);


Should we make this only warn, not an error? I don't think it's critical enough to be an error yeah?

This would likely be a Redis error due to a operational failure. I suggest we keep it as error

yeah okay that works too

quiet-node · 2025-11-20T17:02:47Z

packages/relay/src/lib/services/lockService/RedisLockStrategy.ts

+  protected generateSessionKey(): string {
+    return randomUUID();
+  }
+


I think randomUUID() itself is clean and compact enough, and it doesn't need a wrapper method for it. Should we just directly use it to avoid indirection?

crypto.randomUUID cannot be stubbed directly in the tests and results in the following error TypeError: Descriptor for property randomUUID is non-configurable and non-writable. This wrapper was added as a workaround to address that limitation. Let me know if you have a better approach for this scenario

Ah I see thanks

Signed-off-by: Simeon Nakov <simeon.nakov@limechain.tech>

quiet-node

LGTM Great work!

konstantinabl changed the title ~~4594 redis lock strategy~~ feat: Add Redis lock strategy Nov 18, 2025

konstantinabl assigned simzzz Nov 18, 2025

konstantinabl added the enhancement New feature or request label Nov 18, 2025

konstantinabl added this to the 0.74.0 milestone Nov 18, 2025

konstantinabl marked this pull request as ready for review November 18, 2025 11:27

konstantinabl requested review from a team as code owners November 18, 2025 11:27

konstantinabl requested a review from acuarica November 18, 2025 11:27

konstantinabl changed the base branch from main to feat/nonce-ordering-with-locks November 18, 2025 13:17

konstantinabl requested changes Nov 18, 2025

View reviewed changes

konstantinabl requested a review from a team November 18, 2025 14:20

quiet-node reviewed Nov 19, 2025

View reviewed changes

simzzz added 2 commits November 20, 2025 10:31

feat: adds redis lock strategy

03b96d2

Signed-off-by: Simeon Nakov <simeon.nakov@limechain.tech>

addressed comments

11b8763

Signed-off-by: Simeon Nakov <simeon.nakov@limechain.tech>

simzzz force-pushed the 4594-redis-lock-strategy branch from 643c24f to 11b8763 Compare November 20, 2025 11:15

simzzz requested review from konstantinabl and quiet-node November 20, 2025 11:15

added new config values to example files

7318212

Signed-off-by: Simeon Nakov <simeon.nakov@limechain.tech>

changed to only use lRem

6690240

Signed-off-by: Simeon Nakov <simeon.nakov@limechain.tech>

quiet-node reviewed Nov 20, 2025

View reviewed changes

addressed comments

917e314

Signed-off-by: Simeon Nakov <simeon.nakov@limechain.tech>

simzzz requested a review from quiet-node November 21, 2025 10:32

quiet-node approved these changes Nov 21, 2025

View reviewed changes

feat: Add Redis lock strategy #4617

Are you sure you want to change the base?

feat: Add Redis lock strategy #4617

Conversation

simzzz commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue(s)

Changes from original design (optional)

Additional work needed (optional)

Checklist

Uh oh!

github-actions bot commented Nov 17, 2025

Test Results

Uh oh!

codecov bot commented Nov 18, 2025

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

quiet-node left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simzzz commented Nov 20, 2025

Uh oh!

quiet-node left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quiet-node Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simzzz commented Nov 17, 2025 •

edited

Loading

quiet-node Nov 20, 2025 •

edited

Loading