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.)

Signed-off-by: Simeon Nakov <[email protected]>

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.

konstantinabl · 2025-11-18T14:00:33Z

docs/configuration.md

 | `TX_DEFAULT_GAS`                            | "400000"                              | Default gas for transactions that do not specify gas.                                                                                                                                                                                                                                                                                                                                                                                                                                      |
 | `TXPOOL_API_ENABLED`                        | "false"                               | Enables all txpool related methods.                                                                                                                                                                                                                                                                                                                                                                                                                                                        |
 | `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.                                                                                                                                                                                                                                                                                                           |


I wonder, should we use the same variable as used in local lock strategy?

konstantinabl · 2025-11-18T14:01:35Z

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

+   * @throws Error if acquisition times out or Redis connection fails.
+   */
+  async acquireLock(address: string): Promise<string> {
+    const normalizedAddress = this.normalizeAddress(address);


This can be a utils function and be reused in LocalLockStrategy

We could also add it as a static method on LockService or something and reuse it in LocalLockStrategy as well. This would keep it within the Lock module and reduce indirection.

konstantinabl · 2025-11-18T14:09:15Z

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

+        await this.cleanupFromQueue(queueKey, sessionKey, normalizedAddress);
+      }
+
+      const redisError = new RedisCacheError(error);


Maybe we should use another error, RedisCache would be confusing, or rename the error

Agree. Probably just use the original error thrown without needing to wrap it into RedisCacheError it can cause confusion

konstantinabl · 2025-11-18T14:13:04Z

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

+
+      try {
+        await redisLockStrategy.acquireLock(testAddress);
+        expect.fail('Should have thrown error');


We don't assert with try/catch, let's try and use unified approaches in tests
https://github.com/hiero-ledger/hiero-json-rpc-relay/blob/main/packages/relay/tests/lib/mirrorNodeClient.spec.ts#L216

konstantinabl · 2025-11-18T14:13:16Z

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

+
+      try {
+        await redisLockStrategy.acquireLock(testAddress);
+        expect.fail('Should have thrown error');


Same here, don't use try/catch

konstantinabl · 2025-11-18T14:14:06Z

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

+
+      try {
+        await redisLockStrategy.acquireLock(testAddress);
+        expect.fail('Should have thrown error');


Edit assertion here ass well to not use try/catch

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?

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

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

+   * @throws Error if acquisition times out or Redis connection fails.
+   */
+  async acquireLock(address: string): Promise<string> {
+    const normalizedAddress = this.normalizeAddress(address);


We could also add it as a static method on LockService or something and reuse it in LocalLockStrategy as well. This would keep it within the Lock module and reduce indirection.

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.

quiet-node · 2025-11-19T18:55:17Z

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

+        await this.cleanupFromQueue(queueKey, sessionKey, normalizedAddress);
+      }
+
+      const redisError = new RedisCacheError(error);


Agree. Probably just use the original error thrown without needing to wrap it into RedisCacheError it can cause confusion

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.

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?

feat: adds redis lock strategy

643c24f

Signed-off-by: Simeon Nakov <[email protected]>

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

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 by konstantinabl 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!

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 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

simzzz commented Nov 17, 2025 •

edited by konstantinabl

Loading