From 0c60fa9c234932c716787b682738d855dd0b112b Mon Sep 17 00:00:00 2001 From: Marie <51697796+ijreilly@users.noreply.github.com> Date: Wed, 14 May 2025 15:41:24 +0200 Subject: [PATCH] Fix cacheData not found after recomputes (#12032) Closes https://github.com/twentyhq/core-team-issues/issues/861 sentries: [User workspace role map not found after recompute](https://twenty-v7.sentry.io/issues/6575092700/events/f9825338a30b470eb2345fe78c1e3479/?project=4507072499810304&query=is%3Aunresolved%20issue.priority%3A%5Bhigh%2C%20medium%5D%20not%20found%20after%20recompute&referrer=next-event&stream_index=0) (64 events in 90d), [Feature flag map not found after recompute](https://twenty-v7.sentry.io/issues/6547696076/?project=4507072499810304&query=is%3Aunresolved%20issue.priority%3A%5Bhigh%2C%20medium%5D%20not%20found%20after%20recompute&referrer=issue-stream&stream_index=1) (23 events in 90d) We have a structural issue with cached data and our locking mechanism: if a data is missing in cache and queried at the same time by two different entities, the first one will recompute the data and indicate a lock during the operation, while the second one will seek to recompute the data too but be stopped because of the ongoing lock, and be left with no data to use, condemned to throw an error. In this PR I considered that it was more important to avoid that error and chose to ignoreLock instead when the data is being queried (through getFromCacheWithRecompute), but this is maybe questionnable. An important note is that I can't figure out how users that regularly use twenty (as it has been the case since this error occured on our internal workspace) can encounter that error as once computed, **the key should always be present in the workspace**: the corresponding value it is always updated, never deleted (until this PR) and recreated. I was not able understand how this happened For our data cached without a version to refer to in the database, I also chose to ignore the lock when the recompute is triggered by a data change (eg feature flag enabling or assigning user to a role or adding an objectPermission on a role, etc.), as we never want to imped the recompute in that case to avoid potential stale data. --- .../services/feature-flag.service.ts | 4 +- .../object-metadata.service.ts | 2 + .../object-permission.service.ts | 1 + .../metadata-modules/role/role.service.ts | 3 + .../user-role/user-role.service.ts | 1 + ...rkspace-feature-flags-map-cache.service.ts | 15 +++- ...space-permissions-cache-storage.service.ts | 14 ++-- .../workspace-permissions-cache.service.ts | 77 ++++++++++++------- .../factories/workspace-datasource.factory.ts | 2 +- ...get-data-from-cache-with-recompute.util.ts | 52 ++++++++++--- 10 files changed, 124 insertions(+), 47 deletions(-) diff --git a/packages/twenty-server/src/engine/core-modules/feature-flag/services/feature-flag.service.ts b/packages/twenty-server/src/engine/core-modules/feature-flag/services/feature-flag.service.ts index 327e8a5af..bed48e12a 100644 --- a/packages/twenty-server/src/engine/core-modules/feature-flag/services/feature-flag.service.ts +++ b/packages/twenty-server/src/engine/core-modules/feature-flag/services/feature-flag.service.ts @@ -74,6 +74,7 @@ export class FeatureFlagService { await this.workspaceFeatureFlagsMapCacheService.recomputeFeatureFlagsMapCache( { workspaceId: workspaceId, + ignoreLock: true, }, ); } @@ -132,7 +133,8 @@ export class FeatureFlagService { await this.workspaceFeatureFlagsMapCacheService.recomputeFeatureFlagsMapCache( { - workspaceId: workspaceId, + workspaceId, + ignoreLock: true, }, ); diff --git a/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts b/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts index c8731d7ae..dcecc8db5 100644 --- a/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts @@ -214,6 +214,7 @@ export class ObjectMetadataService extends TypeOrmQueryService this.recomputeFeatureFlagsMapCache(params), cachedEntityName: FEATURE_FLAG_MAP, exceptionCode: TwentyORMExceptionCode.FEATURE_FLAG_MAP_VERSION_NOT_FOUND, + logger: this.logger, }); } @@ -64,8 +65,18 @@ export class WorkspaceFeatureFlagsMapCacheService { workspaceId, ); - if (!ignoreLock && isAlreadyCaching) { - return; + if (isAlreadyCaching) { + if (ignoreLock) { + this.logger.warn( + `Feature flags map cache is already being cached (workspace ${workspaceId}), respecting lock and returning no data`, + ); + + return; + } else { + this.logger.warn( + `Feature flags map cache is already being cached (workspace ${workspaceId}), ignoring lock`, + ); + } } await this.workspaceCacheStorageService.addFeatureFlagMapOngoingCachingLock( diff --git a/packages/twenty-server/src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache-storage.service.ts b/packages/twenty-server/src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache-storage.service.ts index 1d05c15eb..a4b044b5c 100644 --- a/packages/twenty-server/src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache-storage.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache-storage.service.ts @@ -89,10 +89,8 @@ export class WorkspacePermissionsCacheStorageService { async setUserWorkspaceRoleMap( workspaceId: string, userWorkspaceRoleMap: UserWorkspaceRoleMap, - ): Promise<{ - newUserWorkspaceRoleMapVersion: string; - }> { - const [, newUserWorkspaceRoleMapVersion] = await Promise.all([ + ): Promise { + await Promise.all([ this.cacheStorageService.set( `${WorkspaceCacheKeys.MetadataPermissionsUserWorkspaceRoleMap}:${workspaceId}`, userWorkspaceRoleMap, @@ -100,8 +98,6 @@ export class WorkspacePermissionsCacheStorageService { ), this.setUserWorkspaceRoleMapVersion(workspaceId), ]); - - return { newUserWorkspaceRoleMapVersion }; } async setUserWorkspaceRoleMapVersion(workspaceId: string) { @@ -146,6 +142,12 @@ export class WorkspacePermissionsCacheStorageService { ); } + removeUserWorkspaceRoleMap(workspaceId: string) { + return this.cacheStorageService.del( + `${WorkspaceCacheKeys.MetadataPermissionsUserWorkspaceRoleMap}:${workspaceId}`, + ); + } + getUserWorkspaceRoleMapOngoingCachingLock( workspaceId: string, ): Promise { diff --git a/packages/twenty-server/src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache.service.ts b/packages/twenty-server/src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache.service.ts index 138d2326b..de41fde6a 100644 --- a/packages/twenty-server/src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache.service.ts @@ -52,13 +52,21 @@ export class WorkspacePermissionsCacheService { ignoreLock?: boolean; roleIds?: string[]; }): Promise { - if (!ignoreLock) { - const isAlreadyCaching = - await this.workspacePermissionsCacheStorageService.getRolesPermissionsOngoingCachingLock( - workspaceId, + const isAlreadyCaching = + await this.workspacePermissionsCacheStorageService.getRolesPermissionsOngoingCachingLock( + workspaceId, + ); + + if (isAlreadyCaching) { + if (ignoreLock) { + this.logger.warn( + `RolesPermissions data is already being cached (workspace ${workspaceId}), ignoring lock`, + ); + } else { + this.logger.warn( + `RolesPermissions data is already being cached (workspace ${workspaceId}), respecting lock and returning no data`, ); - if (isAlreadyCaching) { return; } } @@ -112,33 +120,48 @@ export class WorkspacePermissionsCacheService { workspaceId: string; ignoreLock?: boolean; }): Promise { - if (!ignoreLock) { + try { const isAlreadyCaching = await this.workspacePermissionsCacheStorageService.getUserWorkspaceRoleMapOngoingCachingLock( workspaceId, ); if (isAlreadyCaching) { - return; + if (ignoreLock) { + this.logger.warn( + `UserWorkspaceRoleMap data is already being cached (workspace ${workspaceId}), ignoring lock`, + ); + } else { + this.logger.warn( + `UserWorkspaceRoleMap data is already being cached (workspace ${workspaceId}), respecting lock and returning no data`, + ); + + return; + } } - } - await this.workspacePermissionsCacheStorageService.addUserWorkspaceRoleMapOngoingCachingLock( - workspaceId, - ); - - try { - const freshUserWorkspaceRoleMap = - await this.getUserWorkspaceRoleMapFromDatabase({ - workspaceId, - }); - - await this.workspacePermissionsCacheStorageService.setUserWorkspaceRoleMap( + await this.workspacePermissionsCacheStorageService.addUserWorkspaceRoleMapOngoingCachingLock( workspaceId, - freshUserWorkspaceRoleMap, ); - } finally { - await this.workspacePermissionsCacheStorageService.removeUserWorkspaceRoleMapOngoingCachingLock( + + try { + const freshUserWorkspaceRoleMap = + await this.getUserWorkspaceRoleMapFromDatabase({ + workspaceId, + }); + + await this.workspacePermissionsCacheStorageService.setUserWorkspaceRoleMap( + workspaceId, + freshUserWorkspaceRoleMap, + ); + } finally { + await this.workspacePermissionsCacheStorageService.removeUserWorkspaceRoleMapOngoingCachingLock( + workspaceId, + ); + } + } catch (error) { + // Flush stale userWorkspaceRoleMap + await this.workspacePermissionsCacheStorageService.removeUserWorkspaceRoleMap( workspaceId, ); } @@ -162,6 +185,7 @@ export class WorkspacePermissionsCacheService { recomputeCache: (params) => this.recomputeRolesPermissionsCache(params), cachedEntityName: ROLES_PERMISSIONS, exceptionCode: TwentyORMExceptionCode.ROLES_PERMISSIONS_VERSION_NOT_FOUND, + logger: this.logger, }); } @@ -169,22 +193,19 @@ export class WorkspacePermissionsCacheService { workspaceId, }: { workspaceId: string; - }): Promise> { - return getFromCacheWithRecompute({ + }): Promise> { + return getFromCacheWithRecompute({ workspaceId, getCacheData: () => this.workspacePermissionsCacheStorageService.getUserWorkspaceRoleMap( workspaceId, ), - getCacheVersion: () => - this.workspacePermissionsCacheStorageService.getUserWorkspaceRoleMapVersion( - workspaceId, - ), recomputeCache: (params) => this.recomputeUserWorkspaceRoleMapCache(params), cachedEntityName: USER_WORKSPACE_ROLE_MAP, exceptionCode: TwentyORMExceptionCode.USER_WORKSPACE_ROLE_MAP_VERSION_NOT_FOUND, + logger: this.logger, }); } diff --git a/packages/twenty-server/src/engine/twenty-orm/factories/workspace-datasource.factory.ts b/packages/twenty-server/src/engine/twenty-orm/factories/workspace-datasource.factory.ts index 9a75a5f5b..254afad52 100644 --- a/packages/twenty-server/src/engine/twenty-orm/factories/workspace-datasource.factory.ts +++ b/packages/twenty-server/src/engine/twenty-orm/factories/workspace-datasource.factory.ts @@ -234,10 +234,10 @@ export class WorkspaceDatasourceFactory { recomputeCache: () => this.workspacePermissionsCacheService.recomputeRolesPermissionsCache({ workspaceId, - ignoreLock: true, }), cachedEntityName: ROLES_PERMISSIONS, exceptionCode: TwentyORMExceptionCode.ROLES_PERMISSIONS_VERSION_NOT_FOUND, + logger: this.logger, }); } diff --git a/packages/twenty-server/src/engine/utils/get-data-from-cache-with-recompute.util.ts b/packages/twenty-server/src/engine/utils/get-data-from-cache-with-recompute.util.ts index c46762b31..dfed3a206 100644 --- a/packages/twenty-server/src/engine/utils/get-data-from-cache-with-recompute.util.ts +++ b/packages/twenty-server/src/engine/utils/get-data-from-cache-with-recompute.util.ts @@ -1,3 +1,5 @@ +import { Logger } from '@nestjs/common'; + import { isDefined } from 'twenty-shared/utils'; import { @@ -17,36 +19,68 @@ const getFromCacheWithRecompute = async ({ recomputeCache, cachedEntityName, exceptionCode, + logger, }: { workspaceId: string; getCacheData: (workspaceId: string) => Promise; - getCacheVersion: (workspaceId: string) => Promise; - recomputeCache: (params: { workspaceId: string }) => Promise; + getCacheVersion?: (workspaceId: string) => Promise; + recomputeCache: (params: { + workspaceId: string; + ignoreLock?: boolean; + }) => Promise; cachedEntityName: string; exceptionCode: TwentyORMExceptionCode; + logger: Logger; }): Promise> => { let cachedVersion: T | undefined; let cachedData: U | undefined; - cachedVersion = await getCacheVersion(workspaceId); + const expectCacheVersion = isDefined(getCacheVersion); + + if (expectCacheVersion) { + cachedVersion = await getCacheVersion(workspaceId); + } + cachedData = await getCacheData(workspaceId); - if (!isDefined(cachedData) || !isDefined(cachedVersion)) { - await recomputeCache({ workspaceId }); + if ( + !isDefined(cachedData) || + (expectCacheVersion && !isDefined(cachedVersion)) + ) { + logger.warn( + `Triggering cache recompute for ${cachedEntityName} (workspace ${workspaceId})`, + { + cachedVersion, + cachedData, + }, + ); + await recomputeCache({ workspaceId, ignoreLock: true }); cachedData = await getCacheData(workspaceId); - cachedVersion = await getCacheVersion(workspaceId); + if (expectCacheVersion) { + cachedVersion = await getCacheVersion(workspaceId); + } - if (!isDefined(cachedData) || !isDefined(cachedVersion)) { + if ( + !isDefined(cachedData) || + (expectCacheVersion && !isDefined(cachedVersion)) + ) { + logger.warn( + `Data still missing after recompute for ${cachedEntityName} (workspace ${workspaceId})`, + { + cachedVersion, + cachedData, + }, + ); throw new TwentyORMException( - `${cachedEntityName} not found after recompute for workspace ${workspaceId}`, + `${cachedEntityName} not found after recompute for workspace ${workspaceId} (missingData: ${!isDefined(cachedData)}, missingVersion: ${expectCacheVersion && !isDefined(cachedVersion)})`, exceptionCode, ); } } return { - version: cachedVersion, + version: cachedVersion as T, data: cachedData, }; };