fix(server-nestjs): make GitLab repo create and commit idempotent

apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts

@@ -14,6 +14,7 @@ import { GITLAB_REST_CLIENT, GitlabClientService } from './gitlab-client.service
 import {
   makeAccessTokenExposedSchema,
   makeAccessTokenSchema,
+  makeCommitAction,
   makeExpandedGroupSchema,
   makeExpandedUserSchema,
   makeGitbeakerRequestError,
@@ -183,6 +184,91 @@ describe('gitlab-client', () => {
 
       expect(gitlabApi.Commits.create).not.toHaveBeenCalled()
     })
+
+    it('should tolerate an already-applied commit (race) when the file now exists', async () => {
+      const repoId = 1
+      const repo = makeProjectSchema({ id: repoId })
+      const message = 'ci: :robot_face: Sync file'
+      const alreadyExistsError = makeGitbeakerRequestError({ description: 'has already been taken', status: 400, statusText: 'Bad Request' })
+      gitlabApi.Commits.create.mockRejectedValue(alreadyExistsError)
+      gitlabApi.RepositoryFiles.show.mockResolvedValue(makeRepositoryFileExpandedSchema())
+
+      await expect(service.maybeCreateCommit(repo, message, [makeCommitAction({ action: 'create' })]))
+        .resolves.toBeUndefined()
+      expect(gitlabApi.Commits.create).toHaveBeenCalledOnce()
+    })
+
+    it('should never create a second commit when two syncs race on the same file', async () => {
+      // Sync A creates the file (commits, file now present). Sync B started before
+      // A landed, so B also emits a `create` action: its commit collides and the
+      // guard must swallow it instead of committing again or throwing.
+      const repoId = 1
+      const repo = makeProjectSchema({ id: repoId })
+      const content = 'content'
+      const filePath = 'file.txt'
+      const message = 'ci: :robot_face: Sync file'
+      // Sync A: file absent -> create action -> commit succeeds.
+      const gitlabRepositoryFilesShowMock = gitlabApi.RepositoryFiles.show as MockedFunction<typeof gitlabApi.RepositoryFiles.show>
+      gitlabRepositoryFilesShowMock.mockRejectedValueOnce(makeGitbeakerRequestError({ description: '404 File Not Found' }))
+      const actionA = await service.generateCreateOrUpdateAction(repo, 'main', filePath, content)
+      await service.maybeCreateCommit(repo, message, actionA ? [actionA] : [])
+
+      // Sync B: still saw the file absent, tries the same `create` and collides.
+      gitlabRepositoryFilesShowMock.mockRejectedValueOnce(makeGitbeakerRequestError({ description: '404 File Not Found' }))
+      gitlabApi.Commits.create.mockRejectedValueOnce(makeGitbeakerRequestError({ description: 'has already been taken', status: 400, statusText: 'Bad Request' }))
+      // The guard's post-collision getFile must see the winner's file.
+      gitlabApi.RepositoryFiles.show.mockResolvedValue(makeRepositoryFileExpandedSchema())
+      const actionB = await service.generateCreateOrUpdateAction(repo, 'main', filePath, content)
+      await service.maybeCreateCommit(repo, message, actionB ? [actionB] : [])
+
+      expect(gitlabApi.Commits.create).toHaveBeenCalledTimes(2)
+      expect(gitlabApi.Commits.create).toHaveBeenLastCalledWith(
+        repoId,
+        'main',
+        message,
+        [{ action: 'create', filePath, content }],
+      )
+      // A third run sees the file present and unchanged: no commit at all.
+      gitlabRepositoryFilesShowMock.mockResolvedValue(makeRepositoryFileExpandedSchema({ content_sha256: 'ed7002b439e9ac845f22357d822bac1444730fbdb6016d3ec9432297b9ec9f73' }))
+      const actionC = await service.generateCreateOrUpdateAction(repo, 'main', filePath, content)
+      await service.maybeCreateCommit(repo, message, actionC ? [actionC] : [])
+      expect(gitlabApi.Commits.create).toHaveBeenCalledTimes(2)
+    })
+
+    it('should rethrow when the file is still absent after an already-exists commit error', async () => {
+      const repoId = 1
+      const repo = makeProjectSchema({ id: repoId })
+      const message = 'ci: :robot_face: Sync file'
+      const alreadyExistsError = makeGitbeakerRequestError({ description: 'has already been taken', status: 400, statusText: 'Bad Request' })
+      gitlabApi.Commits.create.mockRejectedValue(alreadyExistsError)
+      gitlabApi.RepositoryFiles.show.mockRejectedValue(makeGitbeakerRequestError({ description: '404 File Not Found' }))
+
+      await expect(service.maybeCreateCommit(repo, message, [makeCommitAction({ action: 'create' })]))
+        .rejects.toThrow()
+      expect(gitlabApi.Commits.create).toHaveBeenCalledOnce()
+    })
+  })
+
+  describe('ensureGroupRepo', () => {
+    it('should reload the existing repo on a create collision (race)', async () => {
+      const groupId = 99
+      const repoName = 'observability-values'
+      const existingRepo = makeProjectSchema({ id: 42, name: repoName, path_with_namespace: `forge/${repoName}` })
+      const collisionError = makeGitbeakerRequestError({ description: 'has already been taken', status: 400, statusText: 'Bad Request' })
+      gitlabApi.Projects.create.mockRejectedValue(collisionError)
+      const gitlabProjectsAllMock = gitlabApi.Projects.all as MockedFunction<typeof gitlabApi.Projects.all>
+      gitlabProjectsAllMock.mockResolvedValueOnce({ data: [existingRepo], paginationInfo: { next: null } })
+
+      const result = await service.createGroupRepo(groupId, repoName)
+
+      expect(result).toEqual(existingRepo)
+      expect(gitlabApi.Projects.create).toHaveBeenCalledWith(expect.objectContaining({
+        name: repoName,
+        path: repoName,
+        namespaceId: groupId,
+      }))
+      expect(gitlabProjectsAllMock).toHaveBeenCalledOnce()
+    })
   })
 
   describe('getOrCreateProjectGroup', () => {
@@ -997,7 +1083,8 @@ describe('gitlab-client', () => {
         }),
       )
 
-      gitlabApi.Users.all.mockResolvedValueOnce([] as never)
+      const allMock = gitlabApi.Users.all as MockedFunction<typeof gitlabApi.Users.all>
+      allMock.mockResolvedValueOnce([])
       // Race path: the flattened description now matches, so the existing-user
       // lookup runs, finds nothing, and the original error is rethrown.
       await expect(service.createUser({ email, username, name }))

apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts

@@ -34,7 +34,7 @@ import {
   TOPIC_SYSTEM_MANAGED,
   USER_ID_CUSTOM_ATTRIBUTE_KEY,
 } from './gitlab.constants'
-import { generateGitlabCIConfigContent, generateMirrorScriptContent, hasFileContentChanged, hasGitbeakerCause, isGitbeakerNotFound, isGitbeakerUnauthorized } from './gitlab.utils'
+import { ensure, generateGitlabCIConfigContent, generateMirrorScriptContent, hasFileContentChanged, hasGitbeakerCause, isCommitAlreadyApplied, isGitbeakerNotFound, isGitbeakerUnauthorized } from './gitlab.utils'
 
 export const GITLAB_REST_CLIENT = Symbol('GITLAB_REST_CLIENT')
 
@@ -294,14 +294,24 @@ export class GitlabClientService {
 
   async createGroupRepo(groupId: number, repoName: string, description?: string) {
     this.logger.log(`Creating a GitLab repository in a standalone group (groupId=${groupId}, repoName=${repoName})`)
-    const created = await this.client.Projects.create({
-      name: repoName,
-      path: repoName,
-      namespaceId: groupId,
-      description,
-      defaultBranch: defaultBranchName,
+    return ensure({
+      create: () => this.client.Projects.create({
+        name: repoName,
+        path: repoName,
+        namespaceId: groupId,
+        description,
+        defaultBranch: defaultBranchName,
+      }),
+      reload: () => find(
+        this.offsetPaginate(opts => this.client.Projects.all({
+          search: repoName,
+          orderBy: 'path',
+          ...opts,
+        })),
+        p => p.name === repoName,
+      ),
+      onCollision: () => this.logger.warn(`GitLab repository already exists (race); reloading (groupId=${groupId}, repoName=${repoName})`),
     })
-    return created
   }
 
   async getFile(repo: CondensedProjectSchemaWith<'id'>, filePath: string, ref: string = 'main') {
@@ -327,7 +337,21 @@ export class GitlabClientService {
       return
     }
     this.logger.log(`Creating a GitLab commit (repoId=${repo.id}, ref=${ref}, actions=${actions.length})`)
-    await this.client.Commits.create(repo.id, ref, message, actions)
+    try {
+      await this.client.Commits.create(repo.id, ref, message, actions)
+    } catch (error) {
+      // Two overlapping syncs can both see a file absent and both emit a `create`
+      // action; the loser's Commits.create collides with the winner's commit (400).
+      // Treat that as already-committed and continue when the file is now present.
+      if (isCommitAlreadyApplied(error)) {
+        const createAction = actions.find(action => action.action === 'create')
+        if (!createAction || await this.getFile(repo, createAction.filePath, ref)) {
+          this.logger.warn(`GitLab commit already applied (race); continuing (repoId=${repo.id}, ref=${ref})`)
+          return
+        }
+      }
+      throw error
+    }
     this.logger.verbose(`GitLab commit created (repoId=${repo.id}, ref=${ref}, actions=${actions.length})`)
   }
 

apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts

@@ -0,0 +1,58 @@
+import { describe, expect, it, vi } from 'vitest'
+import { makeGitbeakerRequestError } from './gitlab-testing.utils'
+import { ensure, isCommitAlreadyApplied, isGitbeakerRace } from './gitlab.utils'
+
+describe('isGitbeakerRace', () => {
+  it('should match collision errors only', () => {
+    expect(isGitbeakerRace(makeGitbeakerRequestError({ description: 'has already been taken', status: 400 }))).toBe(true)
+    expect(isGitbeakerRace(makeGitbeakerRequestError({ description: 'Reference already exists', status: 400 }))).toBe(true)
+    expect(isGitbeakerRace(makeGitbeakerRequestError({ description: '404 File Not Found', status: 404 }))).toBe(false)
+    expect(isGitbeakerRace(makeGitbeakerRequestError({ description: 'Internal Server Error', status: 500 }))).toBe(false)
+    expect(isGitbeakerRace(new Error('has already been taken'))).toBe(false)
+  })
+})
+
+describe('isCommitAlreadyApplied', () => {
+  it('should match name collisions and duplicate-commit 400s', () => {
+    expect(isCommitAlreadyApplied(makeGitbeakerRequestError({ description: 'has already been taken', status: 400 }))).toBe(true)
+    expect(isCommitAlreadyApplied(makeGitbeakerRequestError({ description: 'Internal Server Error', status: 400, statusText: 'Bad Request' }))).toBe(true)
+    expect(isCommitAlreadyApplied(makeGitbeakerRequestError({ description: '404 File Not Found', status: 404 }))).toBe(false)
+    expect(isCommitAlreadyApplied(makeGitbeakerRequestError({ description: 'Internal Server Error', status: 500 }))).toBe(false)
+  })
+})
+
+describe('ensure', () => {
+  it('should return the created value when create succeeds', async () => {
+    const created = { id: 1 }
+    await expect(ensure({ create: async () => created, reload: async () => undefined })).resolves.toBe(created)
+  })
+
+  it('should reload once on a create collision and never retry create', async () => {
+    const existing = { id: 2 }
+    const create = vi.fn(async () => {
+      throw makeGitbeakerRequestError({ description: 'has already been taken', status: 400 })
+    })
+    const onCollision = vi.fn()
+    const reload = vi.fn(async () => existing)
+
+    await expect(ensure({ create, reload, onCollision })).resolves.toBe(existing)
+
+    expect(create).toHaveBeenCalledOnce()
+    expect(onCollision).toHaveBeenCalledOnce()
+    expect(reload).toHaveBeenCalledOnce()
+  })
+
+  it('should rethrow the original error when a collision finds nothing on reload', async () => {
+    const error = makeGitbeakerRequestError({ description: 'has already been taken', status: 400 })
+    await expect(ensure({ create: async () => { throw error }, reload: async () => undefined })).rejects.toBe(error)
+  })
+
+  it('should rethrow non-race errors without reloading', async () => {
+    const error = makeGitbeakerRequestError({ description: 'Internal Server Error', status: 500 })
+    const reload = vi.fn()
+
+    await expect(ensure({ create: async () => { throw error }, reload })).rejects.toBe(error)
+
+    expect(reload).not.toHaveBeenCalled()
+  })
+})

apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts

@@ -263,3 +263,44 @@ export function hasGitbeakerCause(error: unknown, pattern: string | RegExp): err
 export function isGitbeakerUnauthorized(error: unknown): error is GitbeakerRequestError {
   return error instanceof GitbeakerRequestError && error.cause?.response?.status === 401
 }
+
+// Whether a Gitbeaker error signals an entity already existing (race collision):
+// "has already been taken" / "already exists" messages.
+export function isGitbeakerRace(error: unknown): error is GitbeakerRequestError {
+  return hasGitbeakerCause(error, 'has already been taken')
+    || hasGitbeakerCause(error, /already exists/i)
+}
+
+// Whether a rejected commit request actually landed: GitLab reports name
+// collisions and duplicate commits on the same endpoint signatures, plus a bare
+// 400 (Bad Request) for a duplicate commit payload.
+export function isCommitAlreadyApplied(error: unknown): error is GitbeakerRequestError {
+  return isGitbeakerRace(error)
+    || (error instanceof GitbeakerRequestError && error.cause?.response?.status === 400)
+}
+
+// Runs an idempotent write: tries `create`, and on a GitLab race collision
+// reloads via `reload` and returns the existing entity instead of failing.
+// `onCollision` is invoked once when a collision is detected. If the reload
+// finds nothing, the original error is rethrown so genuine failures are not
+// swallowed.
+export async function ensure<T>({
+  create,
+  reload,
+  onCollision,
+}: {
+  create: () => Promise<T>
+  reload: () => Promise<T | undefined>
+  onCollision?: (error: unknown) => void
+}): Promise<T> {
+  try {
+    return await create()
+  } catch (error) {
+    if (isGitbeakerRace(error)) {
+      onCollision?.(error)
+      const existing = await reload()
+      if (existing) return existing
+    }
+    throw error
+  }
+}