From 3094f1b88616fb10433527b8a29e268eeda4ab66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Wed, 22 May 2024 14:53:23 +0200 Subject: [PATCH] fix(core): Detect DB connection aquisition deadlocks (no-changelog) (#9485) Co-authored-by: Danny Martini --- .github/workflows/ci-postgres-mysql.yml | 1 + .../credentials/credentials.service.ts | 1 + .../handlers/workflows/workflows.service.ts | 5 +-- packages/cli/src/WaitTracker.ts | 7 +--- .../cli/src/commands/import/credentials.ts | 38 ++++++++----------- packages/cli/src/commands/import/workflow.ts | 23 ++++------- .../src/credentials/credentials.service.ts | 5 ++- .../repositories/project.repository.ts | 6 ++- .../databases/subscribers/UserSubscriber.ts | 25 ++++++------ packages/cli/src/services/import.service.ts | 8 ++-- 10 files changed, 53 insertions(+), 66 deletions(-) diff --git a/.github/workflows/ci-postgres-mysql.yml b/.github/workflows/ci-postgres-mysql.yml index 2277917532..9cf864b707 100644 --- a/.github/workflows/ci-postgres-mysql.yml +++ b/.github/workflows/ci-postgres-mysql.yml @@ -102,6 +102,7 @@ jobs: timeout-minutes: 20 env: DB_POSTGRESDB_PASSWORD: password + DB_POSTGRESDB_POOL_SIZE: 1 # Detect connection pooling deadlocks steps: - uses: actions/checkout@v4.1.1 - run: corepack enable diff --git a/packages/cli/src/PublicApi/v1/handlers/credentials/credentials.service.ts b/packages/cli/src/PublicApi/v1/handlers/credentials/credentials.service.ts index 6a7cfa208d..20b7d5f949 100644 --- a/packages/cli/src/PublicApi/v1/handlers/credentials/credentials.service.ts +++ b/packages/cli/src/PublicApi/v1/handlers/credentials/credentials.service.ts @@ -69,6 +69,7 @@ export async function saveCredential( const personalProject = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail( user.id, + transactionManager, ); Object.assign(newSharedCredential, { diff --git a/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts b/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts index bb7b8bebd2..d301e61c93 100644 --- a/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts +++ b/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts @@ -7,7 +7,6 @@ import { SharedWorkflow, type WorkflowSharingRole } from '@db/entities/SharedWor import { WorkflowRepository } from '@db/repositories/workflow.repository'; import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; import type { Project } from '@/databases/entities/Project'; -import { WorkflowTagMappingRepository } from '@db/repositories/workflowTagMapping.repository'; import { TagRepository } from '@db/repositories/tag.repository'; import { License } from '@/License'; import { WorkflowSharingService } from '@/workflows/workflowSharing.service'; @@ -113,9 +112,7 @@ export async function getWorkflowTags(workflowId: string) { export async function updateTags(workflowId: string, newTags: string[]): Promise { await Db.transaction(async (transactionManager) => { - const oldTags = await Container.get(WorkflowTagMappingRepository).findBy({ - workflowId, - }); + const oldTags = await transactionManager.findBy(WorkflowTagMapping, { workflowId }); if (oldTags.length > 0) { await transactionManager.delete(WorkflowTagMapping, oldTags); } diff --git a/packages/cli/src/WaitTracker.ts b/packages/cli/src/WaitTracker.ts index ae6dbf9a62..067a58e224 100644 --- a/packages/cli/src/WaitTracker.ts +++ b/packages/cli/src/WaitTracker.ts @@ -3,7 +3,7 @@ import { ErrorReporterProxy as ErrorReporter, WorkflowOperationError, } from 'n8n-workflow'; -import { Container, Service } from 'typedi'; +import { Service } from 'typedi'; import type { ExecutionStopResult, IWorkflowExecutionDataProcess } from '@/Interfaces'; import { WorkflowRunner } from '@/WorkflowRunner'; import { ExecutionRepository } from '@db/repositories/execution.repository'; @@ -137,10 +137,7 @@ export class WaitTracker { fullExecutionData.waitTill = null; fullExecutionData.status = 'canceled'; - await Container.get(ExecutionRepository).updateExistingExecution( - executionId, - fullExecutionData, - ); + await this.executionRepository.updateExistingExecution(executionId, fullExecutionData); return { mode: fullExecutionData.mode, diff --git a/packages/cli/src/commands/import/credentials.ts b/packages/cli/src/commands/import/credentials.ts index 03e96aa646..6c47a25d96 100644 --- a/packages/cli/src/commands/import/credentials.ts +++ b/packages/cli/src/commands/import/credentials.ts @@ -13,9 +13,9 @@ import { BaseCommand } from '../BaseCommand'; import type { ICredentialsEncrypted } from 'n8n-workflow'; import { ApplicationError, jsonParse } from 'n8n-workflow'; import { UM_FIX_INSTRUCTION } from '@/constants'; -import { UserRepository } from '@db/repositories/user.repository'; import { ProjectRepository } from '@/databases/repositories/project.repository'; -import type { Project } from '@/databases/entities/Project'; +import { Project } from '@/databases/entities/Project'; +import { User } from '@/databases/entities/User'; export class ImportCredentialsCommand extends BaseCommand { static description = 'Import credentials'; @@ -75,13 +75,13 @@ export class ImportCredentialsCommand extends BaseCommand { ); } - const project = await this.getProject(flags.userId, flags.projectId); - const credentials = await this.readCredentials(flags.input, flags.separate); await Db.getConnection().transaction(async (transactionManager) => { this.transactionManager = transactionManager; + const project = await this.getProject(flags.userId, flags.projectId); + const result = await this.checkRelations(credentials, flags.projectId, flags.userId); if (!result.success) { @@ -130,19 +130,6 @@ export class ImportCredentialsCommand extends BaseCommand { } } - private async getOwnerProject() { - const owner = await Container.get(UserRepository).findOneBy({ role: 'global:owner' }); - if (!owner) { - throw new ApplicationError(`Failed to find owner. ${UM_FIX_INSTRUCTION}`); - } - - const project = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail( - owner.id, - ); - - return project; - } - private async checkRelations( credentials: ICredentialsEncrypted[], projectId?: string, @@ -244,7 +231,7 @@ export class ImportCredentialsCommand extends BaseCommand { }); if (sharedCredential && sharedCredential.project.type === 'personal') { - const user = await Container.get(UserRepository).findOneByOrFail({ + const user = await this.transactionManager.findOneByOrFail(User, { projectRelations: { role: 'project:personalOwner', projectId: sharedCredential.projectId, @@ -263,13 +250,20 @@ export class ImportCredentialsCommand extends BaseCommand { private async getProject(userId?: string, projectId?: string) { if (projectId) { - return await Container.get(ProjectRepository).findOneByOrFail({ id: projectId }); + return await this.transactionManager.findOneByOrFail(Project, { id: projectId }); } - if (userId) { - return await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(userId); + if (!userId) { + const owner = await this.transactionManager.findOneBy(User, { role: 'global:owner' }); + if (!owner) { + throw new ApplicationError(`Failed to find owner. ${UM_FIX_INSTRUCTION}`); + } + userId = owner.id; } - return await this.getOwnerProject(); + return await Container.get(ProjectRepository).getPersonalProjectForUserOrFail( + userId, + this.transactionManager, + ); } } diff --git a/packages/cli/src/commands/import/workflow.ts b/packages/cli/src/commands/import/workflow.ts index 7a6b7c38f2..87bb590d6b 100644 --- a/packages/cli/src/commands/import/workflow.ts +++ b/packages/cli/src/commands/import/workflow.ts @@ -160,19 +160,6 @@ export class ImportWorkflowsCommand extends BaseCommand { this.logger.info(`Successfully imported ${total} ${total === 1 ? 'workflow.' : 'workflows.'}`); } - private async getOwnerProject() { - const owner = await Container.get(UserRepository).findOneBy({ role: 'global:owner' }); - if (!owner) { - throw new ApplicationError(`Failed to find owner. ${UM_FIX_INSTRUCTION}`); - } - - const project = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail( - owner.id, - ); - - return project; - } - private async getWorkflowOwner(workflow: WorkflowEntity) { const sharing = await Container.get(SharedWorkflowRepository).findOne({ where: { workflowId: workflow.id, role: 'workflow:owner' }, @@ -234,10 +221,14 @@ export class ImportWorkflowsCommand extends BaseCommand { return await Container.get(ProjectRepository).findOneByOrFail({ id: projectId }); } - if (userId) { - return await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(userId); + if (!userId) { + const owner = await Container.get(UserRepository).findOneBy({ role: 'global:owner' }); + if (!owner) { + throw new ApplicationError(`Failed to find owner. ${UM_FIX_INSTRUCTION}`); + } + userId = owner.id; } - return await this.getOwnerProject(); + return await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(userId); } } diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index d23dbf0cc1..8ce9cdb1d1 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -262,7 +262,10 @@ export class CredentialsService { const project = projectId === undefined - ? await this.projectRepository.getPersonalProjectForUserOrFail(user.id) + ? await this.projectRepository.getPersonalProjectForUserOrFail( + user.id, + transactionManager, + ) : await this.projectService.getProjectWithScope( user, projectId, diff --git a/packages/cli/src/databases/repositories/project.repository.ts b/packages/cli/src/databases/repositories/project.repository.ts index faae0bb9cf..086dfbc7cf 100644 --- a/packages/cli/src/databases/repositories/project.repository.ts +++ b/packages/cli/src/databases/repositories/project.repository.ts @@ -17,8 +17,10 @@ export class ProjectRepository extends Repository { }); } - async getPersonalProjectForUserOrFail(userId: string) { - return await this.findOneOrFail({ + async getPersonalProjectForUserOrFail(userId: string, entityManager?: EntityManager) { + const em = entityManager ?? this.manager; + + return await em.findOneOrFail(Project, { where: { type: 'personal', projectRelations: { userId, role: 'project:personalOwner' } }, }); } diff --git a/packages/cli/src/databases/subscribers/UserSubscriber.ts b/packages/cli/src/databases/subscribers/UserSubscriber.ts index e5fad5bf53..b925965a0c 100644 --- a/packages/cli/src/databases/subscribers/UserSubscriber.ts +++ b/packages/cli/src/databases/subscribers/UserSubscriber.ts @@ -1,12 +1,12 @@ +import { Container } from 'typedi'; import type { EntitySubscriberInterface, UpdateEvent } from '@n8n/typeorm'; import { EventSubscriber } from '@n8n/typeorm'; -import { User } from '../entities/User'; -import Container from 'typedi'; -import { ProjectRepository } from '../repositories/project.repository'; import { ApplicationError, ErrorReporterProxy } from 'n8n-workflow'; import { Logger } from '@/Logger'; -import { UserRepository } from '../repositories/user.repository'; + import { Project } from '../entities/Project'; +import { User } from '../entities/User'; +import { UserRepository } from '../repositories/user.repository'; @EventSubscriber() export class UserSubscriber implements EntitySubscriberInterface { @@ -27,14 +27,17 @@ export class UserSubscriber implements EntitySubscriberInterface { fields.includes('email') ) { const oldUser = event.databaseEntity; - const name = + const userEntity = newUserData instanceof User - ? newUserData.createPersonalProjectName() - : Container.get(UserRepository).create(newUserData).createPersonalProjectName(); + ? newUserData + : Container.get(UserRepository).create(newUserData); - const project = await Container.get(ProjectRepository).getPersonalProjectForUser( - oldUser.id, - ); + const projectName = userEntity.createPersonalProjectName(); + + const project = await event.manager.findOneBy(Project, { + type: 'personal', + projectRelations: { userId: oldUser.id }, + }); if (!project) { // Since this is benign we're not throwing the exception. We don't @@ -47,7 +50,7 @@ export class UserSubscriber implements EntitySubscriberInterface { return; } - project.name = name; + project.name = projectName; await event.manager.save(Project, project); } diff --git a/packages/cli/src/services/import.service.ts b/packages/cli/src/services/import.service.ts index c2226c65b9..96892e2745 100644 --- a/packages/cli/src/services/import.service.ts +++ b/packages/cli/src/services/import.service.ts @@ -1,4 +1,4 @@ -import Container, { Service } from 'typedi'; +import { Service } from 'typedi'; import { v4 as uuid } from 'uuid'; import { type INode, type INodeCredentialsDetails } from 'n8n-workflow'; @@ -8,11 +8,11 @@ import { CredentialsRepository } from '@db/repositories/credentials.repository'; import { TagRepository } from '@db/repositories/tag.repository'; import { SharedWorkflow } from '@db/entities/SharedWorkflow'; import { replaceInvalidCredentials } from '@/WorkflowHelpers'; +import { Project } from '@db/entities/Project'; import { WorkflowEntity } from '@db/entities/WorkflowEntity'; import { WorkflowTagMapping } from '@db/entities/WorkflowTagMapping'; import type { TagEntity } from '@db/entities/TagEntity'; import type { ICredentialsDb } from '@/Interfaces'; -import { ProjectRepository } from '@/databases/repositories/project.repository'; @Service() export class ImportService { @@ -59,9 +59,7 @@ export class ImportService { const upsertResult = await tx.upsert(WorkflowEntity, workflow, ['id']); const workflowId = upsertResult.identifiers.at(0)?.id as string; - const personalProject = await Container.get(ProjectRepository).findOneByOrFail({ - id: projectId, - }); + const personalProject = await tx.findOneByOrFail(Project, { id: projectId }); // Create relationship if the workflow was inserted instead of updated. if (!exists) {