From d4a6faa92c97e95bbfa873d7177b249b5201c24a Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Tue, 24 Jun 2025 03:24:47 +0400 Subject: [PATCH 1/5] fix(folders): multiple folders being created --- backend/src/keystore/keystore.ts | 6 +- backend/src/server/routes/index.ts | 3 +- .../secret-folder/secret-folder-service.ts | 257 +++++++++++------- 3 files changed, 167 insertions(+), 99 deletions(-) diff --git a/backend/src/keystore/keystore.ts b/backend/src/keystore/keystore.ts index 6a63af776..99ab15292 100644 --- a/backend/src/keystore/keystore.ts +++ b/backend/src/keystore/keystore.ts @@ -44,7 +44,11 @@ export const KeyStorePrefixes = { IdentityAccessTokenStatusUpdate: (identityAccessTokenId: string) => `identity-access-token-status:${identityAccessTokenId}`, ServiceTokenStatusUpdate: (serviceTokenId: string) => `service-token-status:${serviceTokenId}`, - GatewayIdentityCredential: (identityId: string) => `gateway-credentials:${identityId}` + GatewayIdentityCredential: (identityId: string) => `gateway-credentials:${identityId}`, + + CreateFolderLock: (envId: string, projectId: string) => `folder-creation-${envId}-${projectId}` as const, + WaitUntilReadyCreateFolder: (envId: string, projectId: string) => + `wait-until-ready-folder-creation-${envId}-${projectId}` as const }; export const KeyStoreTtls = { diff --git a/backend/src/server/routes/index.ts b/backend/src/server/routes/index.ts index 262e8f373..e58b1487b 100644 --- a/backend/src/server/routes/index.ts +++ b/backend/src/server/routes/index.ts @@ -1187,7 +1187,8 @@ export const registerRoutes = async ( projectEnvDAL, snapshotService, projectDAL, - folderCommitService + folderCommitService, + keyStore }); const secretImportService = secretImportServiceFactory({ diff --git a/backend/src/services/secret-folder/secret-folder-service.ts b/backend/src/services/secret-folder/secret-folder-service.ts index d6007957b..4f7be1e64 100644 --- a/backend/src/services/secret-folder/secret-folder-service.ts +++ b/backend/src/services/secret-folder/secret-folder-service.ts @@ -6,7 +6,9 @@ import { ActionProjectType, TSecretFoldersInsert } from "@app/db/schemas"; import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service-types"; import { ProjectPermissionActions, ProjectPermissionSub } from "@app/ee/services/permission/project-permission"; import { TSecretSnapshotServiceFactory } from "@app/ee/services/secret-snapshot/secret-snapshot-service"; +import { KeyStorePrefixes, TKeyStoreFactory } from "@app/keystore/keystore"; import { BadRequestError, NotFoundError } from "@app/lib/errors"; +import { logger } from "@app/lib/logger"; import { OrderByDirection, OrgServiceActor } from "@app/lib/types"; import { buildFolderPath } from "@app/services/secret-folder/secret-folder-fns"; @@ -33,6 +35,7 @@ type TSecretFolderServiceFactoryDep = { folderVersionDAL: Pick; folderCommitService: Pick; projectDAL: Pick; + keyStore: Pick; }; export type TSecretFolderServiceFactory = ReturnType; @@ -44,7 +47,8 @@ export const secretFolderServiceFactory = ({ projectEnvDAL, folderVersionDAL, folderCommitService, - projectDAL + projectDAL, + keyStore }: TSecretFolderServiceFactoryDep) => { const createFolder = async ({ projectId, @@ -78,110 +82,169 @@ export const secretFolderServiceFactory = ({ }); } - const folder = await folderDAL.transaction(async (tx) => { - // the logic is simple we need to avoid creating same folder in same path multiple times - // that is this request must be idempotent - // so we do a tricky move. we try to find the to be created folder path if that is exactly match return that - // else we get some path before that then we will start creating remaining folder - const pathWithFolder = path.join(secretPath, name); - const parentFolder = await folderDAL.findClosestFolder(projectId, environment, pathWithFolder, tx); - // no folder found is not possible root should be their - if (!parentFolder) { - throw new NotFoundError({ - message: `Folder with path '${pathWithFolder}' in environment with slug '${environment}' not found` + const lock = await keyStore + .acquireLock([KeyStorePrefixes.CreateFolderLock(env.id, projectId)], 5000) + .catch(() => null); + + try { + if (!lock) { + await keyStore.waitTillReady({ + key: KeyStorePrefixes.WaitUntilReadyCreateFolder(env.id, projectId), + keyCheckCb: (val) => val === "true", + waitingCb: () => logger.debug("CreateFolder: Waiting for key store lock."), + delay: 500 }); } - // exact folder - if (parentFolder.path === pathWithFolder) return parentFolder; - let parentFolderId = parentFolder.id; - if (parentFolder.path !== secretPath) { - // this is upsert folder in a path - // we are not taking snapshots of this because - // snapshot will be removed from automatic for all commits to user click or cron based - const missingSegment = secretPath.substring(parentFolder.path.length).split("/").filter(Boolean); - if (missingSegment.length) { - const newFolders: Array = missingSegment.map((segment) => { - const newFolder = { - name: segment, - parentId: parentFolderId, - id: uuidv4(), - envId: env.id, - version: 1 - }; - parentFolderId = newFolder.id; - return newFolder; + const folder = await folderDAL.transaction(async (tx) => { + const pathWithFolder = path.join(secretPath, name); + const parentFolder = await folderDAL.findClosestFolder(projectId, environment, pathWithFolder, tx); + + if (!parentFolder) { + throw new NotFoundError({ + message: `Parent folder for path '${pathWithFolder}' not found` }); - parentFolderId = newFolders.at(-1)?.id as string; - const docs = await folderDAL.insertMany(newFolders, tx); - const folderVersions = await folderVersionDAL.insertMany( - docs.map((doc) => ({ - name: doc.name, - envId: doc.envId, - version: doc.version, - folderId: doc.id, - description: doc.description - })), - tx - ); - await folderCommitService.createCommit( - { - actor: { - type: actor, - metadata: { - id: actorId - } - }, - message: "Folder created", - folderId: parentFolderId, - changes: folderVersions.map((fv) => ({ - type: CommitType.ADD, - folderVersionId: fv.id - })) - }, - tx - ); } - } - const doc = await folderDAL.create( - { name, envId: env.id, version: 1, parentId: parentFolderId, description }, - tx - ); - const folderVersion = await folderVersionDAL.create( - { - name: doc.name, - envId: doc.envId, - version: doc.version, - folderId: doc.id, - description: doc.description - }, - tx - ); - await folderCommitService.createCommit( - { - actor: { - type: actor, - metadata: { - id: actorId - } + // check if the exact folder already exists + const existingFolder = await folderDAL.findOne( + { + envId: env.id, + parentId: parentFolder.id, + name, + isReserved: false }, - message: "Folder created", - folderId: parentFolderId, - changes: [ - { - type: CommitType.ADD, - folderVersionId: folderVersion.id - } - ] - }, - tx - ); - return doc; - }); + tx + ); - await snapshotService.performSnapshot(folder.parentId as string); - return folder; + if (existingFolder) { + return existingFolder; + } + + // exact folder case + if (parentFolder.path === pathWithFolder) { + return parentFolder; + } + + let currentParentId = parentFolder.id; + let currentPath = parentFolder.path; + + // build the full path we need by processing each segment + if (parentFolder.path !== secretPath) { + const missingSegments = secretPath.substring(parentFolder.path.length).split("/").filter(Boolean); + + const newFolders: TSecretFoldersInsert[] = []; + + // process each segment sequentially + for (const segment of missingSegments) { + // eslint-disable-next-line no-await-in-loop + const existingSegment = await folderDAL.findOne( + { + name: segment, + parentId: currentParentId, + envId: env.id, + isReserved: false + }, + tx + ); + + if (existingSegment) { + // use existing folder and update the path / parent + currentParentId = existingSegment.id; + currentPath = path.join(currentPath, segment); + } else { + const newFolder = { + name: segment, + parentId: currentParentId, + id: uuidv4(), + envId: env.id, + version: 1 + }; + + currentParentId = newFolder.id; + currentPath = path.join(currentPath, segment); + newFolders.push(newFolder); + } + } + + if (newFolders.length) { + const docs = await folderDAL.insertMany(newFolders, tx); + const folderVersions = await folderVersionDAL.insertMany( + docs.map((doc) => ({ + name: doc.name, + envId: doc.envId, + version: doc.version, + folderId: doc.id, + description: doc.description + })), + tx + ); + await folderCommitService.createCommit( + { + actor: { + type: actor, + metadata: { + id: actorId + } + }, + message: "Folder created", + folderId: currentParentId, + changes: folderVersions.map((fv) => ({ + type: CommitType.ADD, + folderVersionId: fv.id + })) + }, + tx + ); + } + } + + const doc = await folderDAL.create( + { name, envId: env.id, version: 1, parentId: currentParentId, description }, + tx + ); + + const folderVersion = await folderVersionDAL.create( + { + name: doc.name, + envId: doc.envId, + version: doc.version, + folderId: doc.id, + description: doc.description + }, + tx + ); + + await folderCommitService.createCommit( + { + actor: { + type: actor, + metadata: { + id: actorId + } + }, + message: "Folder created", + folderId: doc.id, + changes: [ + { + type: CommitType.ADD, + folderVersionId: folderVersion.id + } + ] + }, + tx + ); + + return doc; + }); + + await keyStore.setItemWithExpiry(KeyStorePrefixes.WaitUntilReadyCreateFolder(env.id, projectId), 10, "true"); + + await snapshotService.performSnapshot(folder.parentId as string); + return folder; + } finally { + await lock?.release(); + } }; const updateManyFolders = async ({ From 305f2d79de898ba7649f7f739b5c68349eb7a316 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Tue, 24 Jun 2025 03:32:18 +0400 Subject: [PATCH 2/5] remove unused path --- backend/src/services/secret-folder/secret-folder-service.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/backend/src/services/secret-folder/secret-folder-service.ts b/backend/src/services/secret-folder/secret-folder-service.ts index 4f7be1e64..0a1c2b2c1 100644 --- a/backend/src/services/secret-folder/secret-folder-service.ts +++ b/backend/src/services/secret-folder/secret-folder-service.ts @@ -127,7 +127,6 @@ export const secretFolderServiceFactory = ({ } let currentParentId = parentFolder.id; - let currentPath = parentFolder.path; // build the full path we need by processing each segment if (parentFolder.path !== secretPath) { @@ -151,7 +150,6 @@ export const secretFolderServiceFactory = ({ if (existingSegment) { // use existing folder and update the path / parent currentParentId = existingSegment.id; - currentPath = path.join(currentPath, segment); } else { const newFolder = { name: segment, @@ -162,7 +160,6 @@ export const secretFolderServiceFactory = ({ }; currentParentId = newFolder.id; - currentPath = path.join(currentPath, segment); newFolders.push(newFolder); } } From b336c0c3d65abcea57ef89d2abf0f7d4bc5074ec Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Tue, 24 Jun 2025 03:33:45 +0400 Subject: [PATCH 3/5] Update secret-folder-service.ts --- backend/src/services/secret-folder/secret-folder-service.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/backend/src/services/secret-folder/secret-folder-service.ts b/backend/src/services/secret-folder/secret-folder-service.ts index 0a1c2b2c1..a4046cfa6 100644 --- a/backend/src/services/secret-folder/secret-folder-service.ts +++ b/backend/src/services/secret-folder/secret-folder-service.ts @@ -135,8 +135,7 @@ export const secretFolderServiceFactory = ({ const newFolders: TSecretFoldersInsert[] = []; // process each segment sequentially - for (const segment of missingSegments) { - // eslint-disable-next-line no-await-in-loop + for await (const segment of missingSegments) { const existingSegment = await folderDAL.findOne( { name: segment, From f1bfea61d0cf8a78e764a99145ef397fc81552b9 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Tue, 24 Jun 2025 18:54:18 +0400 Subject: [PATCH 4/5] fix: replace keystore lock with postgres lock --- backend/src/keystore/keystore.ts | 9 +- backend/src/server/routes/index.ts | 3 +- .../secret-folder/secret-folder-service.ts | 278 ++++++++---------- 3 files changed, 133 insertions(+), 157 deletions(-) diff --git a/backend/src/keystore/keystore.ts b/backend/src/keystore/keystore.ts index 99ab15292..1e641e813 100644 --- a/backend/src/keystore/keystore.ts +++ b/backend/src/keystore/keystore.ts @@ -11,7 +11,8 @@ export const PgSqlLock = { OrgGatewayRootCaInit: (orgId: string) => pgAdvisoryLockHashText(`org-gateway-root-ca:${orgId}`), OrgGatewayCertExchange: (orgId: string) => pgAdvisoryLockHashText(`org-gateway-cert-exchange:${orgId}`), SecretRotationV2Creation: (folderId: string) => pgAdvisoryLockHashText(`secret-rotation-v2-creation:${folderId}`), - CreateProject: (orgId: string) => pgAdvisoryLockHashText(`create-project:${orgId}`) + CreateProject: (orgId: string) => pgAdvisoryLockHashText(`create-project:${orgId}`), + CreateFolder: (envId: string, projectId: string) => pgAdvisoryLockHashText(`create-folder:${envId}-${projectId}`) } as const; // all the key prefixes used must be set here to avoid conflict @@ -44,11 +45,7 @@ export const KeyStorePrefixes = { IdentityAccessTokenStatusUpdate: (identityAccessTokenId: string) => `identity-access-token-status:${identityAccessTokenId}`, ServiceTokenStatusUpdate: (serviceTokenId: string) => `service-token-status:${serviceTokenId}`, - GatewayIdentityCredential: (identityId: string) => `gateway-credentials:${identityId}`, - - CreateFolderLock: (envId: string, projectId: string) => `folder-creation-${envId}-${projectId}` as const, - WaitUntilReadyCreateFolder: (envId: string, projectId: string) => - `wait-until-ready-folder-creation-${envId}-${projectId}` as const + GatewayIdentityCredential: (identityId: string) => `gateway-credentials:${identityId}` }; export const KeyStoreTtls = { diff --git a/backend/src/server/routes/index.ts b/backend/src/server/routes/index.ts index e58b1487b..262e8f373 100644 --- a/backend/src/server/routes/index.ts +++ b/backend/src/server/routes/index.ts @@ -1187,8 +1187,7 @@ export const registerRoutes = async ( projectEnvDAL, snapshotService, projectDAL, - folderCommitService, - keyStore + folderCommitService }); const secretImportService = secretImportServiceFactory({ diff --git a/backend/src/services/secret-folder/secret-folder-service.ts b/backend/src/services/secret-folder/secret-folder-service.ts index a4046cfa6..3614d9e24 100644 --- a/backend/src/services/secret-folder/secret-folder-service.ts +++ b/backend/src/services/secret-folder/secret-folder-service.ts @@ -6,9 +6,8 @@ import { ActionProjectType, TSecretFoldersInsert } from "@app/db/schemas"; import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service-types"; import { ProjectPermissionActions, ProjectPermissionSub } from "@app/ee/services/permission/project-permission"; import { TSecretSnapshotServiceFactory } from "@app/ee/services/secret-snapshot/secret-snapshot-service"; -import { KeyStorePrefixes, TKeyStoreFactory } from "@app/keystore/keystore"; +import { PgSqlLock } from "@app/keystore/keystore"; import { BadRequestError, NotFoundError } from "@app/lib/errors"; -import { logger } from "@app/lib/logger"; import { OrderByDirection, OrgServiceActor } from "@app/lib/types"; import { buildFolderPath } from "@app/services/secret-folder/secret-folder-fns"; @@ -35,7 +34,6 @@ type TSecretFolderServiceFactoryDep = { folderVersionDAL: Pick; folderCommitService: Pick; projectDAL: Pick; - keyStore: Pick; }; export type TSecretFolderServiceFactory = ReturnType; @@ -47,8 +45,7 @@ export const secretFolderServiceFactory = ({ projectEnvDAL, folderVersionDAL, folderCommitService, - projectDAL, - keyStore + projectDAL }: TSecretFolderServiceFactoryDep) => { const createFolder = async ({ projectId, @@ -82,165 +79,148 @@ export const secretFolderServiceFactory = ({ }); } - const lock = await keyStore - .acquireLock([KeyStorePrefixes.CreateFolderLock(env.id, projectId)], 5000) - .catch(() => null); + const folder = await folderDAL.transaction(async (tx) => { + await tx.raw("SELECT pg_advisory_xact_lock(?)", [PgSqlLock.CreateFolder(env.id, env.projectId)]); - try { - if (!lock) { - await keyStore.waitTillReady({ - key: KeyStorePrefixes.WaitUntilReadyCreateFolder(env.id, projectId), - keyCheckCb: (val) => val === "true", - waitingCb: () => logger.debug("CreateFolder: Waiting for key store lock."), - delay: 500 + const pathWithFolder = path.join(secretPath, name); + const parentFolder = await folderDAL.findClosestFolder(projectId, environment, pathWithFolder, tx); + + if (!parentFolder) { + throw new NotFoundError({ + message: `Parent folder for path '${pathWithFolder}' not found` }); } - const folder = await folderDAL.transaction(async (tx) => { - const pathWithFolder = path.join(secretPath, name); - const parentFolder = await folderDAL.findClosestFolder(projectId, environment, pathWithFolder, tx); + // check if the exact folder already exists + const existingFolder = await folderDAL.findOne( + { + envId: env.id, + parentId: parentFolder.id, + name, + isReserved: false + }, + tx + ); - if (!parentFolder) { - throw new NotFoundError({ - message: `Parent folder for path '${pathWithFolder}' not found` - }); - } + if (existingFolder) { + return existingFolder; + } - // check if the exact folder already exists - const existingFolder = await folderDAL.findOne( - { - envId: env.id, - parentId: parentFolder.id, - name, - isReserved: false - }, - tx - ); + // exact folder case + if (parentFolder.path === pathWithFolder) { + return parentFolder; + } - if (existingFolder) { - return existingFolder; - } + let currentParentId = parentFolder.id; - // exact folder case - if (parentFolder.path === pathWithFolder) { - return parentFolder; - } + // build the full path we need by processing each segment + if (parentFolder.path !== secretPath) { + const missingSegments = secretPath.substring(parentFolder.path.length).split("/").filter(Boolean); - let currentParentId = parentFolder.id; + const newFolders: TSecretFoldersInsert[] = []; - // build the full path we need by processing each segment - if (parentFolder.path !== secretPath) { - const missingSegments = secretPath.substring(parentFolder.path.length).split("/").filter(Boolean); - - const newFolders: TSecretFoldersInsert[] = []; - - // process each segment sequentially - for await (const segment of missingSegments) { - const existingSegment = await folderDAL.findOne( - { - name: segment, - parentId: currentParentId, - envId: env.id, - isReserved: false - }, - tx - ); - - if (existingSegment) { - // use existing folder and update the path / parent - currentParentId = existingSegment.id; - } else { - const newFolder = { - name: segment, - parentId: currentParentId, - id: uuidv4(), - envId: env.id, - version: 1 - }; - - currentParentId = newFolder.id; - newFolders.push(newFolder); - } - } - - if (newFolders.length) { - const docs = await folderDAL.insertMany(newFolders, tx); - const folderVersions = await folderVersionDAL.insertMany( - docs.map((doc) => ({ - name: doc.name, - envId: doc.envId, - version: doc.version, - folderId: doc.id, - description: doc.description - })), - tx - ); - await folderCommitService.createCommit( - { - actor: { - type: actor, - metadata: { - id: actorId - } - }, - message: "Folder created", - folderId: currentParentId, - changes: folderVersions.map((fv) => ({ - type: CommitType.ADD, - folderVersionId: fv.id - })) - }, - tx - ); - } - } - - const doc = await folderDAL.create( - { name, envId: env.id, version: 1, parentId: currentParentId, description }, - tx - ); - - const folderVersion = await folderVersionDAL.create( - { - name: doc.name, - envId: doc.envId, - version: doc.version, - folderId: doc.id, - description: doc.description - }, - tx - ); - - await folderCommitService.createCommit( - { - actor: { - type: actor, - metadata: { - id: actorId - } + // process each segment sequentially + for await (const segment of missingSegments) { + const existingSegment = await folderDAL.findOne( + { + name: segment, + parentId: currentParentId, + envId: env.id, + isReserved: false }, - message: "Folder created", - folderId: doc.id, - changes: [ - { + tx + ); + + if (existingSegment) { + // use existing folder and update the path / parent + currentParentId = existingSegment.id; + } else { + const newFolder = { + name: segment, + parentId: currentParentId, + id: uuidv4(), + envId: env.id, + version: 1 + }; + + currentParentId = newFolder.id; + newFolders.push(newFolder); + } + } + + if (newFolders.length) { + const docs = await folderDAL.insertMany(newFolders, tx); + const folderVersions = await folderVersionDAL.insertMany( + docs.map((doc) => ({ + name: doc.name, + envId: doc.envId, + version: doc.version, + folderId: doc.id, + description: doc.description + })), + tx + ); + await folderCommitService.createCommit( + { + actor: { + type: actor, + metadata: { + id: actorId + } + }, + message: "Folder created", + folderId: currentParentId, + changes: folderVersions.map((fv) => ({ type: CommitType.ADD, - folderVersionId: folderVersion.id - } - ] + folderVersionId: fv.id + })) + }, + tx + ); + } + } + + const doc = await folderDAL.create( + { name, envId: env.id, version: 1, parentId: currentParentId, description }, + tx + ); + + const folderVersion = await folderVersionDAL.create( + { + name: doc.name, + envId: doc.envId, + version: doc.version, + folderId: doc.id, + description: doc.description + }, + tx + ); + + await folderCommitService.createCommit( + { + actor: { + type: actor, + metadata: { + id: actorId + } }, - tx - ); + message: "Folder created", + folderId: doc.id, + changes: [ + { + type: CommitType.ADD, + folderVersionId: folderVersion.id + } + ] + }, + tx + ); - return doc; - }); + return doc; + }); - await keyStore.setItemWithExpiry(KeyStorePrefixes.WaitUntilReadyCreateFolder(env.id, projectId), 10, "true"); - - await snapshotService.performSnapshot(folder.parentId as string); - return folder; - } finally { - await lock?.release(); - } + await snapshotService.performSnapshot(folder.parentId as string); + return folder; }; const updateManyFolders = async ({ From 3c9a7c77ff7cf86a7899911e71d564523ab89798 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Tue, 24 Jun 2025 18:58:03 +0400 Subject: [PATCH 5/5] chore: re-add comment --- backend/src/services/secret-folder/secret-folder-service.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/backend/src/services/secret-folder/secret-folder-service.ts b/backend/src/services/secret-folder/secret-folder-service.ts index 3614d9e24..da29c0f36 100644 --- a/backend/src/services/secret-folder/secret-folder-service.ts +++ b/backend/src/services/secret-folder/secret-folder-service.ts @@ -80,6 +80,10 @@ export const secretFolderServiceFactory = ({ } const folder = await folderDAL.transaction(async (tx) => { + // the logic is simple we need to avoid creating same folder in same path multiple times + // that is this request must be idempotent + // so we do a tricky move. we try to find the to be created folder path if that is exactly match return that + // else we get some path before that then we will start creating remaining folder await tx.raw("SELECT pg_advisory_xact_lock(?)", [PgSqlLock.CreateFolder(env.id, env.projectId)]); const pathWithFolder = path.join(secretPath, name);