diff --git a/apps/server-nestjs/src/modules/events/app-events.service.spec.ts b/apps/server-nestjs/src/modules/events/app-events.service.spec.ts index 88393b119f..fcdd4393a4 100644 --- a/apps/server-nestjs/src/modules/events/app-events.service.spec.ts +++ b/apps/server-nestjs/src/modules/events/app-events.service.spec.ts @@ -102,8 +102,8 @@ describe('appEventsService', () => { await service.emitProjectEvent('project.upsert', project, { action: 'Update Project' }) - expect(prisma.project.update).toHaveBeenCalledWith({ - where: { id: project.id }, + expect(prisma.project.updateMany).toHaveBeenCalledWith({ + where: { id: project.id, status: { not: 'archived' } }, data: { status: 'failed' }, }) }) @@ -122,6 +122,20 @@ describe('appEventsService', () => { }) }) + it('does not overwrite an archived project when a replayed delete reports a stale KO', async () => { + const project = makeProject({ status: 'archived' }) + eventEmitter.emitAsync.mockResolvedValue([ + { keycloak: { status: 'KO', message: 'unknown_error', executionTime: 20, error: new Error('unknown_error') } }, + ]) + + await service.emitProjectEvent('project.delete', project, { action: 'Delete all project resources' }) + + expect(prisma.project.updateMany).toHaveBeenCalledWith({ + where: { id: project.id, status: { not: 'archived' } }, + data: { status: 'failed' }, + }) + }) + it('does not touch the project status after a successful delete', async () => { const project = makeProject() eventEmitter.emitAsync.mockResolvedValue([ diff --git a/apps/server-nestjs/src/modules/events/app-events.service.ts b/apps/server-nestjs/src/modules/events/app-events.service.ts index 32e1e6d612..f0ed714350 100644 --- a/apps/server-nestjs/src/modules/events/app-events.service.ts +++ b/apps/server-nestjs/src/modules/events/app-events.service.ts @@ -87,7 +87,7 @@ export class AppEventsService { * Pass a project id in the general case: the project is loaded here, after the * emitting transaction commits, so every listener sees the same committed state. * Pass an already-loaded snapshot when the row no longer reflects what listeners - * must act on (e.g. archiving renames the slug before emitting `project.delete`). + * must act on (e.g. archiving emits `project.delete` before renaming the slug). */ async emitProjectEvent( event: ProjectEventName, @@ -167,8 +167,9 @@ export class AppEventsService { /** * Reflects the listeners' outcome on the project row (legacy hooks behavior): * any KO result marks the project `failed`; a fully successful upsert marks it - * `created` and records the provisioning version. A successful `project.delete` - * leaves the `archived` status set when the project was archived. + * `created` and records the provisioning version. `project.delete` results are + * not written here: at emit time the row is not yet archived, and a success must + * leave the status untouched (the archiving transaction owns it). */ private async updateProjectStatus( event: ProjectEventName, @@ -179,7 +180,13 @@ export class AppEventsService { if (failed.length) { this.logger.warn(`${event} marked project as failed (projectId=${projectId}, failed=${failed.join(',')})`) - await this.prisma.project.update({ where: { id: projectId }, data: { status: 'failed' } }) + // Never overwrite `archived`: a replayed `project.delete` on an already + // archived project is a retry of a cleanup that finished since, and its + // stale KO must not resurrect the row. + await this.prisma.project.updateMany({ + where: { id: projectId, status: { not: 'archived' } }, + data: { status: 'failed' }, + }) return } diff --git a/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.spec.ts b/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.spec.ts index ff5e39b91c..1169d3efd0 100644 --- a/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.spec.ts +++ b/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.spec.ts @@ -309,6 +309,18 @@ describe('deleteGroup', () => { await expect(service.deleteGroup('gone-id')).resolves.toBeUndefined() }) + + it('should swallow a 500 on delete when the group is gone on re-fetch (concurrent deletion)', async () => { + // Keycloak 26.7.0 answers 500 unknown_error instead of 404 when the deletion + // races another deletion of the same group + server.use( + http.get(childrenUrl, () => HttpResponse.json([])), + http.delete(groupUrl, () => HttpResponse.json({ error: 'unknown_error' }, { status: 500 })), + http.get(groupUrl, () => HttpResponse.json({ error: 'Not found' }, { status: 404 })), + ) + + await expect(service.deleteGroup('raced-id')).resolves.toBeUndefined() + }) }) describe('getOrCreateGroupByPath root resolution (issue #2518)', () => { diff --git a/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.ts b/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.ts index ed0cc872a0..d05da01b3f 100644 --- a/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.ts +++ b/apps/server-nestjs/src/modules/keycloak/keycloak-client.service.ts @@ -101,9 +101,18 @@ export class KeycloakClientService implements OnModuleInit { await this.client.groups.del({ id }) } catch (err) { // The group may already be gone (deleted concurrently or pruned in a - // prior pass); a 404 is not a failure worth surfacing to the caller. - if (getErrorResponseStatus(err) !== 404) throw err - this.logger.warn(`Keycloak group ${id} was already deleted; skipping`) + // prior pass). Keycloak answers 404 in the simple case but can also + // answer 500 unknown_error when the deletion races another one, so a + // 500 is only fatal if the group still resolves on re-fetch. + if (getErrorResponseStatus(err) === 404) { + this.logger.warn(`Keycloak group ${id} was already deleted; skipping`) + return + } + if (getErrorResponseStatus(err) === 500 && !(await this.client.groups.findOne({ id }))) { + this.logger.warn(`Keycloak group ${id} was deleted concurrently (500 on delete, gone on re-fetch); skipping`) + return + } + throw err } } diff --git a/apps/server-nestjs/src/modules/project/project.service.spec.ts b/apps/server-nestjs/src/modules/project/project.service.spec.ts index dc06b884ea..cbc3ea2b03 100644 --- a/apps/server-nestjs/src/modules/project/project.service.spec.ts +++ b/apps/server-nestjs/src/modules/project/project.service.spec.ts @@ -6,9 +6,11 @@ import type { UserContext } from '../infrastructure/auth/auth-user.decorator' import { faker } from '@faker-js/faker' import { BadRequestException, + ConflictException, ForbiddenException, InternalServerErrorException, NotFoundException, + UnprocessableEntityException, } from '@nestjs/common' import { Test } from '@nestjs/testing' import { beforeEach, describe, expect, it } from 'vitest' @@ -462,30 +464,44 @@ describe('projectService', () => { }) describe('archive', () => { - it('deletes related data, emits event, renames and archives project', async () => { + it('emits the delete event on the intact project, then renames and archives', async () => { const projectId = faker.string.uuid() const pwd = makeProjectWithDetails({ id: projectId, name: 'myproject', slug: 'myproject' }) + prisma.project.findUnique.mockResolvedValue(pwd) const tx = mockDeep() - tx.project.findUnique.mockResolvedValue(pwd) tx.repository.deleteMany.mockResolvedValue({ count: 2 }) tx.environment.deleteMany.mockResolvedValue({ count: 3 }) tx.deployment.deleteMany.mockResolvedValue({ count: 1 }) tx.project.update.mockResolvedValue(makeProject({ id: projectId })) prisma.$transaction.mockImplementation(async cb => cb(tx)) + // the lock write bumps `updatedAt` (@updatedAt); the post-lock row is the + // conflict baseline, so the archived row must match it, not the pre-lock snapshot + prisma.project.update.mockResolvedValue({ ...pwd, updatedAt: new Date('2026-01-02T00:00:00.000Z') }) + tx.project.findUnique.mockResolvedValue(makeProjectWithDetails({ id: projectId, updatedAt: new Date('2026-01-02T00:00:00.000Z') })) + appEvents.emitProjectEvent.mockResolvedValue({}) const requestId = faker.string.uuid() const requestorId = faker.string.uuid() await service.archive(projectId, requestorId, requestId) - expect(tx.repository.deleteMany).toHaveBeenCalledWith({ where: { projectId } }) - expect(tx.environment.deleteMany).toHaveBeenCalledWith({ where: { projectId } }) - expect(tx.deployment.deleteMany).toHaveBeenCalledWith({ where: { projectId } }) expect(appEvents.emitProjectEvent).toHaveBeenCalledWith('project.delete', pwd, { action: 'Delete all project resources', userId: requestorId, requestId, }) + // invariant: the row is locked before listeners run, and emit happens before + // any database write in the archiving transaction + expect(prisma.project.update).toHaveBeenCalledWith({ where: { id: projectId }, data: { locked: true } }) + expect(prisma.project.update.mock.invocationCallOrder[0]).toBeLessThan( + appEvents.emitProjectEvent.mock.invocationCallOrder[0], + ) + expect(appEvents.emitProjectEvent.mock.invocationCallOrder[0]).toBeLessThan( + tx.project.update.mock.invocationCallOrder[0], + ) + expect(tx.repository.deleteMany).toHaveBeenCalledWith({ where: { projectId } }) + expect(tx.environment.deleteMany).toHaveBeenCalledWith({ where: { projectId } }) + expect(tx.deployment.deleteMany).toHaveBeenCalledWith({ where: { projectId } }) expect(tx.project.update).toHaveBeenCalledWith( expect.objectContaining({ where: { id: projectId }, @@ -498,30 +514,56 @@ describe('projectService', () => { ) }) - it('throws NotFoundException when project does not exist', async () => { + it('leaves the project untouched but locked when a listener reports a KO result, so a retry can re-drive cleanup', async () => { + const projectId = faker.string.uuid() + const pwd = makeProjectWithDetails({ id: projectId }) + prisma.project.findUnique.mockResolvedValue(pwd) + prisma.project.update.mockResolvedValue(pwd) + appEvents.emitProjectEvent.mockResolvedValue({ + gitlab: { status: 'KO', message: 'boom', executionTime: 1, error: null }, + }) + + await expect(service.archive(projectId)) + .rejects.toThrow(new UnprocessableEntityException('Echec des services à la suppression du projet')) + + expect(prisma.project.update).toHaveBeenCalledWith({ where: { id: projectId }, data: { locked: true } }) + expect(prisma.$transaction).not.toHaveBeenCalled() + }) + + it('keeps the lock set before cleanup when a mutation raced the snapshot (409, no unlock)', async () => { + const projectId = faker.string.uuid() + const pwd = makeProjectWithDetails({ id: projectId, updatedAt: new Date('2026-01-01T00:00:00.000Z') }) + prisma.project.findUnique.mockResolvedValue(pwd) + prisma.project.update.mockResolvedValue(pwd) + appEvents.emitProjectEvent.mockResolvedValue({}) const tx = mockDeep() - tx.project.findUnique.mockResolvedValue(null) + tx.project.findUnique.mockResolvedValue(makeProjectWithDetails({ id: projectId, updatedAt: new Date('2026-01-02T00:00:00.000Z') })) prisma.$transaction.mockImplementation(async cb => cb(tx)) + await expect(service.archive(projectId)) + .rejects.toThrow(new ConflictException('Le projet a été modifié pendant la suppression, rejouez la requête')) + + expect(prisma.project.update).toHaveBeenCalledWith({ where: { id: projectId }, data: { locked: true } }) + expect(tx.project.update).not.toHaveBeenCalled() + }) + + it('throws NotFoundException when project does not exist', async () => { + prisma.project.findUnique.mockResolvedValue(null) + await expect(service.archive(faker.string.uuid())) .rejects.toThrow(NotFoundException) }) it('rejects an already archived project without side effects', async () => { const projectId = faker.string.uuid() - const tx = mockDeep() - tx.project.findUnique.mockResolvedValue( + prisma.project.findUnique.mockResolvedValue( makeProjectWithDetails({ id: projectId, status: 'archived', locked: true }), ) - prisma.$transaction.mockImplementation(async cb => cb(tx)) await expect(service.archive(projectId)) .rejects.toThrow(new BadRequestException('Le projet est archivé')) - expect(tx.repository.deleteMany).not.toHaveBeenCalled() - expect(tx.environment.deleteMany).not.toHaveBeenCalled() - expect(tx.deployment.deleteMany).not.toHaveBeenCalled() - expect(tx.project.update).not.toHaveBeenCalled() + expect(prisma.$transaction).not.toHaveBeenCalled() expect(appEvents.emitProjectEvent).not.toHaveBeenCalled() }) }) diff --git a/apps/server-nestjs/src/modules/project/project.service.ts b/apps/server-nestjs/src/modules/project/project.service.ts index 9a7f29c8a7..5316f0c2cb 100644 --- a/apps/server-nestjs/src/modules/project/project.service.ts +++ b/apps/server-nestjs/src/modules/project/project.service.ts @@ -4,13 +4,14 @@ import type { Prisma } from '@prisma/client' import type { UserContext } from '../infrastructure/auth/auth-user.decorator' import type { ProjectDataExport, ProjectUpdateContext, ProjectWithDetails } from './project-queries.utils' import { AdminAuthorized } from '@cpn-console/shared' -import { BadRequestException, ForbiddenException, Inject, Injectable, InternalServerErrorException, Logger, NotFoundException } from '@nestjs/common' +import { BadRequestException, ConflictException, ForbiddenException, Inject, Injectable, InternalServerErrorException, Logger, NotFoundException, UnprocessableEntityException } from '@nestjs/common' import { trace } from '@opentelemetry/api' import { baseConfigFactory } from '../../config/base.config' import { AppEventsService } from '../events/app-events.service' import { PrismaService } from '../infrastructure/database/prisma.service' import { StartActiveSpan } from '../infrastructure/telemetry/telemetry.decorator' import { LogService } from '../log/log.service' +import { getFailedPlugins } from '../plugin/plugin.utils' import { createProjectMember, deleteProjectMember } from '../project-members/project-members-queries.utils' import { createProject, @@ -189,10 +190,49 @@ export class ProjectService { span?.setAttribute('project.id', projectId) this.logger.log(`project.archive started (projectId=${projectId})`) try { - const project = await this.prisma.$transaction(async (tx) => { + const project = await getProject(this.prisma, projectId) + if (!project) throw new NotFoundException('Projet introuvable') + if (project.status === 'archived') throw new BadRequestException('Le projet est archivé') + + // Serialize external cleanup against project mutations: setting `locked` before + // the emit fences every mutating route (`@RequireProjectLocked(false)` and the + // in-transaction `locked` re-check in `update`), so no upsert can reconcile + // resources behind the cleanup's back. The lock persists on a failed cleanup + // (`failed` status): the DELETE is replayable, and so is the unlock by an admin. + const locked = await this.prisma.project.update({ where: { id: projectId }, data: { locked: true } }) + // emit before touching the rest of the database: listeners clean up resources + // named after the intact slug and still see repos and environments; a failed + // cleanup marks the project `failed` (AppEventsService) and a new DELETE can + // retry, listeners are idempotent (ensure-not-exists), the row was never + // renamed. The lock is deliberately never released here: it must also hold on + // failure (KO -> 422, conflict -> 409) so no mutation can reconcile resources + // between a failed cleanup and its replay. + const results = await this.appEvents.emitProjectEvent('project.delete', project, { + action: 'Delete all project resources', + userId: requestorUserId, + requestId, + }) + const failed = getFailedPlugins(results) + if (failed.length) { + this.logger.warn(`project.archive external cleanup failed (projectId=${projectId}, failed=${failed.join(',')})`) + throw new UnprocessableEntityException('Echec des services à la suppression du projet') + } + + await this.prisma.$transaction(async (tx) => { const loaded = await getProject(tx, projectId) if (!loaded) throw new NotFoundException('Projet introuvable') - if (loaded.status === 'archived') throw new BadRequestException('Le projet est archivé') + if (loaded.status === 'archived') { + this.logger.warn(`project already archived (projectId=${projectId}), answering 204 without writing`) + return + } + // The cleanup ran against the row as returned by the lock write. If it + // moved since (a mutation that raced the lock), resources were reconciled + // behind the cleanup: refuse to archive a stale snapshot, the DELETE is + // replayable. The lock write itself bumps `updatedAt` (`@updatedAt`), so + // the baseline must be the post-lock row, never the pre-lock snapshot. + if (loaded.updatedAt.getTime() !== locked.updatedAt.getTime()) { + throw new ConflictException('Le projet a été modifié pendant la suppression, rejouez la requête') + } await deleteProjectDependencies(tx, projectId) @@ -204,16 +244,8 @@ export class ProjectService { locked: true, clusters: { set: [] }, }) - - return loaded - }) - // pass the pre-archive snapshot: the row was renamed (slug suffixed) in the - // transaction above, listeners must clean up resources named after the old slug - await this.appEvents.emitProjectEvent('project.delete', project, { - action: 'Delete all project resources', - userId: requestorUserId, - requestId, }) + span?.setAttribute('project.slug', project.slug) this.logger.log(`project.archive completed (projectId=${projectId}, slug=${project.slug})`) } catch (error) { diff --git a/packages/shared/src/contracts/project.ts b/packages/shared/src/contracts/project.ts index 55d8323c8f..8e02e0ea9d 100644 --- a/packages/shared/src/contracts/project.ts +++ b/packages/shared/src/contracts/project.ts @@ -161,6 +161,7 @@ export const projectContract = contractInstance.router({ body: ContractNoBody, responses: { 204: null, + 422: ErrorSchema, 500: ErrorSchema, }, },