From f84a25107449fa7e4a2e61c8e9891ef2d2b32abe Mon Sep 17 00:00:00 2001 From: Shikanime Deva <22115108+shikanime@users.noreply.github.com> Date: Mon, 5 Oct 2026 14:54:33 +0200 Subject: [PATCH 1/5] fix(project): piloter la suppression externe avant d'archiver la ligne MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit L'archivage committait le renommage _archived et le verrouillage avant d'émettre project.delete. Un échec d'un listener laissait donc le projet verrouillé dans un état sans voie de récupération. L'émission passe avant la transaction : les listeners nettoyent les ressources sous le slug intact et voient encore les dépôts et environnements, un KO lève une 422 et laisse la ligne inchangée (statut failed, projet rejouable par un nouveau DELETE). Le renommage et le verrouillage ne sont posés qu'après un nettoyage externe réussi. Related: https://github.com/cloud-pi-native/console/issues/2813 Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: Ic331cffbf0034681ad123ff26fe1159e6a6a6964 Signed-off-by: Shikanime Deva <22115108+shikanime@users.noreply.github.com> --- .../modules/project/project.service.spec.ts | 38 ++++++++++++------- .../src/modules/project/project.service.ts | 35 +++++++++++------ packages/shared/src/contracts/project.ts | 1 + 3 files changed, 48 insertions(+), 26 deletions(-) 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..98f2b0c692 100644 --- a/apps/server-nestjs/src/modules/project/project.service.spec.ts +++ b/apps/server-nestjs/src/modules/project/project.service.spec.ts @@ -9,6 +9,7 @@ import { ForbiddenException, InternalServerErrorException, NotFoundException, + UnprocessableEntityException, } from '@nestjs/common' import { Test } from '@nestjs/testing' import { beforeEach, describe, expect, it } from 'vitest' @@ -462,9 +463,10 @@ 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 }) @@ -472,20 +474,21 @@ describe('projectService', () => { tx.deployment.deleteMany.mockResolvedValue({ count: 1 }) tx.project.update.mockResolvedValue(makeProject({ id: projectId })) prisma.$transaction.mockImplementation(async cb => cb(tx)) + 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, }) + 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,10 +501,22 @@ describe('projectService', () => { ) }) + it('leaves the project untouched 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) + appEvents.emitProjectEvent.mockResolvedValue({ + gitlab: { status: 'KO', message: 'boom', executionTime: 1 }, + }) + + await expect(service.archive(projectId)) + .rejects.toThrow(new UnprocessableEntityException('Echec des services à la suppression du projet')) + + expect(prisma.$transaction).not.toHaveBeenCalled() + }) + it('throws NotFoundException when project does not exist', async () => { - const tx = mockDeep() - tx.project.findUnique.mockResolvedValue(null) - prisma.$transaction.mockImplementation(async cb => cb(tx)) + prisma.project.findUnique.mockResolvedValue(null) await expect(service.archive(faker.string.uuid())) .rejects.toThrow(NotFoundException) @@ -509,19 +524,14 @@ describe('projectService', () => { 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..82e633146c 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, 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,29 @@ 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é') + + // emit before touching 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 nor locked + 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') return await deleteProjectDependencies(tx, projectId) @@ -204,15 +224,6 @@ 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})`) 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, }, }, From b03ac9153765a73fe2b3a8875c4a758f42073c8c Mon Sep 17 00:00:00 2001 From: Shikanime Deva <22115108+shikanime@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:11:46 +0200 Subject: [PATCH 2/5] fix(server-nestjs): fiabiliser la suppression concurrente et le statut archive Un DELETE rejoue pendant une suppression en cours fait repondre Keycloak 500 unknown_error au lieu de 404 : le 500 est desormais tolere quand le groupe n'existe plus au re-fetch, et le statut failed pose par un KO obsolete ne peut plus ecraser un projet deja archive. Related: #2813 Co-authored-by: Automata Signed-off-by: William Phetsinorath Signed-off-by: Shikanime Deva <22115108+shikanime@users.noreply.github.com> --- .../modules/events/app-events.service.spec.ts | 18 ++++++++++++++++-- .../src/modules/events/app-events.service.ts | 8 +++++++- .../keycloak/keycloak-client.service.spec.ts | 12 ++++++++++++ .../keycloak/keycloak-client.service.ts | 15 ++++++++++++--- 4 files changed, 47 insertions(+), 6 deletions(-) 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..f64c2cbc4e 100644 --- a/apps/server-nestjs/src/modules/events/app-events.service.ts +++ b/apps/server-nestjs/src/modules/events/app-events.service.ts @@ -179,7 +179,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 } } From 056af44dc1526d4e7e2ee93324325aa80f273812 Mon Sep 17 00:00:00 2001 From: Shikanime Deva <22115108+shikanime@users.noreply.github.com> Date: Thu, 8 Oct 2026 12:07:25 +0200 Subject: [PATCH 3/5] fix(server-nestjs): rejeter l'archivage d'un snapshot obsolete Un update concurrent pendant le nettoyage externe reconsiliait des ressources derriere la suppression : l'archivage compare maintenant updatedAt au snapshot pre-emit et repond 409 sans ecrire, la requete reste rejouable. Trace du 204 silencieux sur ligne deja archivee et assertion d'ordre emission-avant-ecriture dans le spec. Signed-off-by: William Phetsinorath Signed-off-by: Shikanime Deva <22115108+shikanime@users.noreply.github.com> --- .../modules/project/project.service.spec.ts | 22 ++++++++++++++++++- .../src/modules/project/project.service.ts | 13 +++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) 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 98f2b0c692..f598377e0d 100644 --- a/apps/server-nestjs/src/modules/project/project.service.spec.ts +++ b/apps/server-nestjs/src/modules/project/project.service.spec.ts @@ -6,6 +6,7 @@ import type { UserContext } from '../infrastructure/auth/auth-user.decorator' import { faker } from '@faker-js/faker' import { BadRequestException, + ConflictException, ForbiddenException, InternalServerErrorException, NotFoundException, @@ -486,6 +487,10 @@ describe('projectService', () => { userId: requestorId, requestId, }) + // invariant: emit happens before any database write + 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 } }) @@ -506,7 +511,7 @@ describe('projectService', () => { const pwd = makeProjectWithDetails({ id: projectId }) prisma.project.findUnique.mockResolvedValue(pwd) appEvents.emitProjectEvent.mockResolvedValue({ - gitlab: { status: 'KO', message: 'boom', executionTime: 1 }, + gitlab: { status: 'KO', message: 'boom', executionTime: 1, error: null }, }) await expect(service.archive(projectId)) @@ -515,6 +520,21 @@ describe('projectService', () => { expect(prisma.$transaction).not.toHaveBeenCalled() }) + it('refuses to archive a stale snapshot when the row moved during cleanup (concurrent update)', 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) + appEvents.emitProjectEvent.mockResolvedValue({}) + const tx = mockDeep() + 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(tx.project.update).not.toHaveBeenCalled() + }) + it('throws NotFoundException when project does not exist', async () => { prisma.project.findUnique.mockResolvedValue(null) diff --git a/apps/server-nestjs/src/modules/project/project.service.ts b/apps/server-nestjs/src/modules/project/project.service.ts index 82e633146c..fdcf6329e3 100644 --- a/apps/server-nestjs/src/modules/project/project.service.ts +++ b/apps/server-nestjs/src/modules/project/project.service.ts @@ -4,7 +4,7 @@ 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, UnprocessableEntityException } 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' @@ -212,7 +212,16 @@ export class ProjectService { await this.prisma.$transaction(async (tx) => { const loaded = await getProject(tx, projectId) if (!loaded) throw new NotFoundException('Projet introuvable') - if (loaded.status === 'archived') return + if (loaded.status === 'archived') { + this.logger.warn(`project already archived (projectId=${projectId}), answering 204 without writing`) + return + } + // The cleanup ran against a snapshot taken before the emit. If the row moved + // since (concurrent update), resources were reconciled behind the cleanup: + // refuse to archive a stale snapshot, the DELETE is replayable. + if (loaded.updatedAt.getTime() !== project.updatedAt.getTime()) { + throw new ConflictException('Le projet a été modifié pendant la suppression, rejouez la requête') + } await deleteProjectDependencies(tx, projectId) From 0094e10df697a459719c3b96c79f6a7a17520fea Mon Sep 17 00:00:00 2001 From: William Date: Thu, 8 Oct 2026 15:32:24 +0200 Subject: [PATCH 4/5] fix(server-nestjs): verrouiller le projet pendant la suppression externe Poser `locked` avant l'emission de `project.delete` fence toutes les routes de mutation (`@RequireProjectLocked(false)` + re-check in-transaction dans `update`) : aucune reconciliation externe ne peut plus passer derriere le nettoyage, y compris apres un echec (422) ou un conflit (409) - le verrou est conserve pour la reprise. Corrige la revue bloquante de #2816 (course nettoyage externe vs mutations), met a jour les docstrings obsoletes sur l'ordre emission/archivage et rebase sur main. Co-authored-by: Automata Signed-off-by: William Phetsinorath --- .../src/modules/events/app-events.service.ts | 7 +++--- .../modules/project/project.service.spec.ts | 15 +++++++++--- .../src/modules/project/project.service.ts | 23 ++++++++++++++----- 3 files changed, 33 insertions(+), 12 deletions(-) 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 f64c2cbc4e..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, 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 f598377e0d..c99af935ba 100644 --- a/apps/server-nestjs/src/modules/project/project.service.spec.ts +++ b/apps/server-nestjs/src/modules/project/project.service.spec.ts @@ -475,6 +475,7 @@ describe('projectService', () => { tx.deployment.deleteMany.mockResolvedValue({ count: 1 }) tx.project.update.mockResolvedValue(makeProject({ id: projectId })) prisma.$transaction.mockImplementation(async cb => cb(tx)) + prisma.project.update.mockResolvedValue(pwd) appEvents.emitProjectEvent.mockResolvedValue({}) const requestId = faker.string.uuid() @@ -487,7 +488,12 @@ describe('projectService', () => { userId: requestorId, requestId, }) - // invariant: emit happens before any database write + // 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], ) @@ -506,10 +512,11 @@ describe('projectService', () => { ) }) - it('leaves the project untouched when a listener reports a KO result, so a retry can re-drive cleanup', 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 }, }) @@ -517,10 +524,11 @@ describe('projectService', () => { 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('refuses to archive a stale snapshot when the row moved during cleanup (concurrent update)', async () => { + 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) @@ -532,6 +540,7 @@ describe('projectService', () => { 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() }) diff --git a/apps/server-nestjs/src/modules/project/project.service.ts b/apps/server-nestjs/src/modules/project/project.service.ts index fdcf6329e3..942d15bb15 100644 --- a/apps/server-nestjs/src/modules/project/project.service.ts +++ b/apps/server-nestjs/src/modules/project/project.service.ts @@ -194,10 +194,19 @@ export class ProjectService { if (!project) throw new NotFoundException('Projet introuvable') if (project.status === 'archived') throw new BadRequestException('Le projet est archivé') - // emit before touching 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 nor locked + // 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. + 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, @@ -217,8 +226,9 @@ export class ProjectService { return } // The cleanup ran against a snapshot taken before the emit. If the row moved - // since (concurrent update), resources were reconciled behind the cleanup: - // refuse to archive a stale snapshot, the DELETE is replayable. + // since (concurrent update or a mutation that raced the lock), resources were + // reconciled behind the cleanup: refuse to archive a stale snapshot, the + // DELETE is replayable. if (loaded.updatedAt.getTime() !== project.updatedAt.getTime()) { throw new ConflictException('Le projet a été modifié pendant la suppression, rejouez la requête') } @@ -234,6 +244,7 @@ export class ProjectService { clusters: { set: [] }, }) }) + span?.setAttribute('project.slug', project.slug) this.logger.log(`project.archive completed (projectId=${projectId}, slug=${project.slug})`) } catch (error) { From 738afd0fb9ca6ff4165f30a16ee7265ef3a32ab6 Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Fri, 9 Oct 2026 16:58:39 +0200 Subject: [PATCH 5/5] fix(server-nestjs): comparer l'archivage au snapshot pose du verrou Le controle de conflit comparait le projet recharge a un snapshot anterieur a l'ecriture du verrou ; l'annotation @updatedAt de Prisma avance ce champ sur cette ecriture, donc chaque suppression repondait 409. La comparaison se fait desormais sur la ligne renvoyee par la pose du verrou, et le spec de succes verrouille ce comportement (le mock du verrou bump updatedAt) : la comparaison a l'ancienne echoue le spec. Corrige la revue bloquante de #2816. Co-authored-by: Automata Signed-off-by: William Phetsinorath Change-Id: I2d6b0f7bbcc3061a5c980bb3a197c83a6a6a6964 --- .../src/modules/project/project.service.spec.ts | 7 +++++-- .../src/modules/project/project.service.ts | 13 +++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) 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 c99af935ba..cbc3ea2b03 100644 --- a/apps/server-nestjs/src/modules/project/project.service.spec.ts +++ b/apps/server-nestjs/src/modules/project/project.service.spec.ts @@ -469,13 +469,15 @@ describe('projectService', () => { 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)) - prisma.project.update.mockResolvedValue(pwd) + // 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() @@ -532,6 +534,7 @@ describe('projectService', () => { 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(makeProjectWithDetails({ id: projectId, updatedAt: new Date('2026-01-02T00:00:00.000Z') })) diff --git a/apps/server-nestjs/src/modules/project/project.service.ts b/apps/server-nestjs/src/modules/project/project.service.ts index 942d15bb15..5316f0c2cb 100644 --- a/apps/server-nestjs/src/modules/project/project.service.ts +++ b/apps/server-nestjs/src/modules/project/project.service.ts @@ -199,7 +199,7 @@ export class ProjectService { // 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. - await this.prisma.project.update({ where: { id: projectId }, data: { locked: true } }) + 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 @@ -225,11 +225,12 @@ export class ProjectService { this.logger.warn(`project already archived (projectId=${projectId}), answering 204 without writing`) return } - // The cleanup ran against a snapshot taken before the emit. If the row moved - // since (concurrent update or a mutation that raced the lock), resources were - // reconciled behind the cleanup: refuse to archive a stale snapshot, the - // DELETE is replayable. - if (loaded.updatedAt.getTime() !== project.updatedAt.getTime()) { + // 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') }