From 05244dc9d2107db4681907ce1c3decd288272a39 Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Tue, 1 Sep 2026 13:26:32 +0200 Subject: [PATCH 1/2] refactor(gitlab): rename ensureCreated to ensure and extract race detection Review feedback: // style comments, shorter util name, casts dropped where inference suffices (Users.create already returns ExpandedUserSchema), and the has-already-been-taken / already-exists / 400 collision check moved into an isCommitAlreadyApplied guard used by maybeCreateCommit, with isGitbeakerRace naming entity collisions for ensure. ensure takes a single options object. Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: I728df0130286bfab2d599b9c6091731b6a6a6964 --- .../modules/gitlab/gitlab-client.service.ts | 42 +++++++++++++++---- .../src/modules/gitlab/gitlab.utils.ts | 41 ++++++++++++++++++ 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts index 1907dd0d73..9295d21181 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts +++ b/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})`) } diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts index be748a6f7e..1893e26875 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.ts +++ b/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({ + create, + reload, + onCollision, +}: { + create: () => Promise + reload: () => Promise + onCollision?: (error: unknown) => void +}): Promise { + try { + return await create() + } catch (error) { + if (isGitbeakerRace(error)) { + onCollision?.(error) + const existing = await reload() + if (existing) return existing + } + throw error + } +} From 386d7803d09704674c2d9aba7c18299f6facad4b Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Tue, 1 Sep 2026 13:26:46 +0200 Subject: [PATCH 2/2] test(gitlab): cover ensure util and race-commit idempotency isGitbeakerRace predicate matrix, ensure create/reload/collision contract (create called once, reload once, non-race rethrow), and a two-sync race scenario proving maybeCreateCommit never emits a second commit for content already synced. Rebased onto main; createGroupRepo rename adapted. Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: I6ee1064982cadefe1562f097751ce3f76a6a6964 --- .../gitlab/gitlab-client.service.spec.ts | 89 ++++++++++++++++++- .../src/modules/gitlab/gitlab.utils.spec.ts | 58 ++++++++++++ 2 files changed, 146 insertions(+), 1 deletion(-) create mode 100644 apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts index 94678ce86a..da791e8013 100644 --- a/apps/server-nestjs/src/modules/gitlab/gitlab-client.service.spec.ts +++ b/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 + 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 + 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 + 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 })) diff --git a/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts b/apps/server-nestjs/src/modules/gitlab/gitlab.utils.spec.ts new file mode 100644 index 0000000000..9aad038f23 --- /dev/null +++ b/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() + }) +})