Throw an error if the redis client is not connected or the query time…

…s out
safe-global · Jan 10, 2025 · 3af66c5 · 3af66c5
1 parent 5ed8890
commit 3af66c5
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 69 deletions.
diff --git a/src/datasources/accounts/accounts.datasource.ts b/src/datasources/accounts/accounts.datasource.ts
@@ -253,15 +253,14 @@ export class AccountsDatasource implements IAccountsDatasource, OnModuleInit {
         `Invalid client IP while creating account: ${clientIp}`,
       );
     } else {
-      const current =
-        (await this.cacheService.increment(
-          CacheRouter.getRateLimitCacheKey(
-            `${AccountsDatasource.ACCOUNT_CREATION_CACHE_PREFIX}_${clientIp}`,
-          ),
-          this.accountCreationRateLimitPeriodSeconds,
-          // If the current value cannot be retrieved from Redis (e.g., due to an error or timeout),
-          // we allow the user to proceed without blocking their operation.
-        )) ?? 0;
+      const current = await this.cacheService.increment(
+        CacheRouter.getRateLimitCacheKey(
+          `${AccountsDatasource.ACCOUNT_CREATION_CACHE_PREFIX}_${clientIp}`,
+        ),
+        this.accountCreationRateLimitPeriodSeconds,
+        // If the current value cannot be retrieved from Redis (e.g., due to an error or timeout),
+        // we allow the user to proceed without blocking their operation.
+      );
       if (current > this.accountCreationRateLimitCalls) {
         this.loggingService.warn(
           `Limit of ${this.accountCreationRateLimitCalls} reached for IP ${clientIp}`,

diff --git a/src/datasources/accounts/counterfactual-safes/counterfactual-safes.datasource.ts b/src/datasources/accounts/counterfactual-safes/counterfactual-safes.datasource.ts
@@ -161,14 +161,14 @@ export class CounterfactualSafesDatasource
 
   private async checkCreationRateLimit(account: Account): Promise<void> {
     const current =
-      (await this.cacheService.increment(
+      await this.cacheService.increment(
         CacheRouter.getRateLimitCacheKey(
           `${CounterfactualSafesDatasource.COUNTERFACTUAL_SAFES_CREATION_CACHE_PREFIX}_${account.address}`,
         ),
         this.counterfactualSafesCreationRateLimitPeriodSeconds,
         // If the current value cannot be retrieved from Redis (e.g., due to an error or timeout),
         // we allow the user to proceed without blocking their operation.
-      )) ?? 0;
+      );
     if (current > this.counterfactualSafesCreationRateLimitCalls) {
       this.loggingService.warn(
         `Limit of ${this.counterfactualSafesCreationRateLimitCalls} reached for account ${account.address}`,

diff --git a/src/datasources/balances-api/zerion-balances-api.service.ts b/src/datasources/balances-api/zerion-balances-api.service.ts
@@ -381,14 +381,14 @@ export class ZerionBalancesApi implements IBalancesApi {
 
   private async _checkRateLimit(): Promise<void> {
     const current =
-      (await this.cacheService.increment(
+      await this.cacheService.increment(
         CacheRouter.getRateLimitCacheKey(
           ZerionBalancesApi.RATE_LIMIT_CACHE_KEY_PREFIX,
         ),
         this.limitPeriodSeconds,
         // If the current value cannot be retrieved from Redis (e.g., due to an error or timeout),
         // we allow the user to proceed without blocking their operation.
-      )) ?? 0;
+      );
     if (current > this.limitCalls) throw new LimitReachedError();
   }
 }
diff --git a/src/datasources/cache/cache.service.interface.ts b/src/datasources/cache/cache.service.interface.ts
@@ -13,12 +13,12 @@ export interface ICacheService {
 
   hGet(cacheDir: CacheDir): Promise<string | undefined>;
 
-  deleteByKey(key: string): Promise<number | undefined>;
+  deleteByKey(key: string): Promise<number>;
 
   increment(
     cacheKey: string,
     expireTimeSeconds: number | undefined,
-  ): Promise<number | undefined>;
+  ): Promise<number>;
 
   setCounter(
     key: string,

diff --git a/src/datasources/cache/redis.cache.service.ts b/src/datasources/cache/redis.cache.service.ts
@@ -1,4 +1,9 @@
-import { Inject, Injectable, OnModuleDestroy } from '@nestjs/common';
+import {
+  Inject,
+  Injectable,
+  OnModuleDestroy,
+  ServiceUnavailableException,
+} from '@nestjs/common';
 import { RedisClientType } from '@/datasources/cache/cache.module';
 import { ICacheService } from '@/datasources/cache/cache.service.interface';
 import { CacheDir } from '@/datasources/cache/entities/cache-dir.entity';
@@ -40,11 +45,7 @@ export class RedisCacheService
   }
 
   async getCounter(key: string): Promise<number | null> {
-    if (!this.ready()) {
-      this.logRedisClientUnreadyState('getCounter');
-
-      return null;
-    }
+    this.validatgeRedisClientIsReady();
 
     const value = await this.client.get(this._prefixKey(key));
     const numericValue = Number(value);
@@ -56,16 +57,12 @@ export class RedisCacheService
     value: string,
     expireTimeSeconds: number | undefined,
   ): Promise<void> {
+    this.validatgeRedisClientIsReady();
+
     if (!expireTimeSeconds || expireTimeSeconds <= 0) {
       return;
     }
 
-    if (!this.ready()) {
-      this.logRedisClientUnreadyState('hSet');
-
-      return undefined;
-    }
-
     const key = this._prefixKey(cacheDir.key);
 
     try {
@@ -80,22 +77,14 @@ export class RedisCacheService
   }
 
   async hGet(cacheDir: CacheDir): Promise<string | undefined> {
-    if (!this.ready()) {
-      this.logRedisClientUnreadyState('hGet');
-
-      return undefined;
-    }
+    this.validatgeRedisClientIsReady();
 
     const key = this._prefixKey(cacheDir.key);
     return await this.timeout(this.client.hGet(key, cacheDir.field));
   }
 
-  async deleteByKey(key: string): Promise<number | undefined> {
-    if (!this.ready()) {
-      this.logRedisClientUnreadyState('deleteByKey');
-
-      return undefined;
-    }
+  async deleteByKey(key: string): Promise<number> {
+    this.validatgeRedisClientIsReady();
 
     const keyWithPrefix = this._prefixKey(key);
     // see https://redis.io/commands/unlink/
@@ -113,12 +102,8 @@ export class RedisCacheService
   async increment(
     cacheKey: string,
     expireTimeSeconds: number | undefined,
-  ): Promise<number | undefined> {
-    if (!this.ready()) {
-      this.logRedisClientUnreadyState('increment');
-
-      return undefined;
-    }
+  ): Promise<number> {
+    this.validatgeRedisClientIsReady();
 
     const transaction = this.client.multi().incr(cacheKey);
     if (expireTimeSeconds !== undefined && expireTimeSeconds > 0) {
@@ -133,11 +118,7 @@ export class RedisCacheService
     value: number,
     expireTimeSeconds: number,
   ): Promise<void> {
-    if (!this.ready()) {
-      this.logRedisClientUnreadyState('setCounter');
-
-      return undefined;
-    }
+    this.validatgeRedisClientIsReady();
 
     await this.client.set(key, value, { EX: expireTimeSeconds, NX: true });
   }
@@ -167,11 +148,8 @@ export class RedisCacheService
    * instance is not responding it invokes {@link forceQuit}.
    */
   async onModuleDestroy(): Promise<void> {
-    if (!this.ready()) {
-      this.logRedisClientUnreadyState('onModuleDestroy');
+    this.validatgeRedisClientIsReady();
 
-      return undefined;
-    }
     this.loggingService.info('Closing Redis connection...');
     try {
       await promiseWithTimeout(
@@ -190,11 +168,7 @@ export class RedisCacheService
    * Forces the closing of the Redis connection associated with this service.
    */
   private async forceQuit(): Promise<void> {
-    if (!this.ready()) {
-      this.logRedisClientUnreadyState('forceQuit');
-
-      return undefined;
-    }
+    this.validatgeRedisClientIsReady();
     this.loggingService.warn('Forcing Redis connection to close...');
     try {
       await this.client.disconnect();
@@ -207,25 +181,25 @@ export class RedisCacheService
   private async timeout<T>(
     queryObject: Promise<T>,
     timeout?: number,
-  ): Promise<T | undefined> {
+  ): Promise<T> {
     timeout =
       timeout ?? this.configurationService.getOrThrow<number>('redis.timeout');
     try {
       return await promiseWithTimeout(queryObject, timeout);
     } catch (error) {
       if (error instanceof PromiseTimeoutError) {
         this.loggingService.error('Redis Query Timed out!');
-
-        return undefined;
       }
 
       throw error;
     }
   }
 
-  private logRedisClientUnreadyState(operation: string): void {
-    this.loggingService.error(
-      `Redis client is not ready. Redis ${operation} failed!`,
-    );
+  private validatgeRedisClientIsReady(): void {
+    if (!this.ready()) {
+      this.loggingService.error(`Redis client is not ready`);
+
+      throw new ServiceUnavailableException('Redis client is not ready');
+    }
   }
 }
diff --git a/src/domain/common/utils/promise.ts b/src/domain/common/utils/promise.ts
@@ -1,16 +1,22 @@
 import { HttpException, HttpStatus } from '@nestjs/common';
 
-export const promiseWithTimeout = <T>(
+export const promiseWithTimeout = async <T>(
   promise: Promise<T>,
   timeout: number,
-): Promise<T | undefined> => {
+): Promise<T> => {
   let timeoutId: NodeJS.Timeout;
 
-  return Promise.race<T | undefined>([
+  return Promise.race<T>([
     promise,
     new Promise((_, reject) => {
       timeoutId = setTimeout(
-        () => reject(new PromiseTimeoutError('Promise timed out!', 500)),
+        () =>
+          reject(
+            new PromiseTimeoutError(
+              'Promise timed out!',
+              HttpStatus.INTERNAL_SERVER_ERROR,
+            ),
+          ),
         timeout,
       );
     }),