From d224c5a8baaebe4696b699ecd5ad2c5124202c37 Mon Sep 17 00:00:00 2001 From: FenjuFu <92919259+FenjuFu@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:27:45 +0800 Subject: [PATCH 1/2] fix(publish): return deterministic conflict on concurrent coordinate race Concurrent publishes for the same (namespace_id, slug, owner_id) or (skill_id, version) coordinate both pass the check-then-create reads and race on the database unique constraints. The losing request surfaced an unhandled DataIntegrityViolationException as HTTP 500. Translate the constraint violation at both insert points into a deterministic DomainBadRequestException (error.skill.publish.concurrentConflict), matching the existing idiom in LabelDefinitionService/ReviewService/ PromotionService. No same-transaction re-read is attempted, so the losing publish rolls back cleanly and returns a retryable conflict instead of a 500. Add the i18n key (en/zh) and two unit tests covering the skill-insert and version-insert races. Closes #617 Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com> --- .../src/main/resources/messages.properties | 1 + .../src/main/resources/messages_zh.properties | 1 + .../skill/service/SkillPublishService.java | 18 ++++- .../service/SkillPublishServiceTest.java | 73 +++++++++++++++++++ 4 files changed, 91 insertions(+), 2 deletions(-) diff --git a/server/skillhub-app/src/main/resources/messages.properties b/server/skillhub-app/src/main/resources/messages.properties index 82b255e5..c350aff7 100644 --- a/server/skillhub-app/src/main/resources/messages.properties +++ b/server/skillhub-app/src/main/resources/messages.properties @@ -148,6 +148,7 @@ error.admin.user.status.invalid=Invalid user status: {0} error.admin.user.status.unsupported=Only ACTIVE or DISABLED status can be managed here error.skill.publish.nameConflict=A published skill with name ''{0}'' already exists in this namespace error.skill.publish.nameConflict.private=A private skill with name ''{0}'' has already been published in this namespace +error.skill.publish.concurrentConflict=A concurrent publish for skill ''{0}'' is already being processed; please retry. error.skill.approve.nameConflict=Cannot approve: a published skill with name ''{0}'' already exists in this namespace error.skill.version.submit.notUploaded=Version ''{0}'' is not in UPLOADED status and cannot be submitted for review error.skill.version.confirm.notUploaded=Version ''{0}'' is not in UPLOADED status and cannot be confirmed diff --git a/server/skillhub-app/src/main/resources/messages_zh.properties b/server/skillhub-app/src/main/resources/messages_zh.properties index 0f2e1808..25b879ef 100644 --- a/server/skillhub-app/src/main/resources/messages_zh.properties +++ b/server/skillhub-app/src/main/resources/messages_zh.properties @@ -148,6 +148,7 @@ error.admin.user.status.invalid=无效的用户状态：{0} error.admin.user.status.unsupported=这里只允许管理 ACTIVE 或 DISABLED 状态的用户 error.skill.publish.nameConflict=该命名空间下已存在名为"{0}"的已发布技能，无法提交 error.skill.publish.nameConflict.private=该命名空间下已存在名为"{0}"的已发布私有技能，无法提交 +error.skill.publish.concurrentConflict=技能"{0}"存在并发的发布请求正在处理，请稍后重试。 error.skill.approve.nameConflict=无法通过审核：该命名空间下已存在名为"{0}"的已发布技能 error.skill.version.submit.notUploaded=版本"{0}"不在 UPLOADED 状态，无法提交审核 error.skill.version.confirm.notUploaded=版本"{0}"不在 UPLOADED 状态，无法确认发布 diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java index 06eb39c3..380b06ca 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java @@ -28,6 +28,7 @@ import com.iflytek.skillhub.storage.ObjectStorageService; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.yaml.snakeyaml.Yaml; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.context.ApplicationEventPublisher; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; @@ -422,7 +423,14 @@ public class SkillPublishService { .orElseGet(() -> { Skill newSkill = new Skill(namespace.getId(), skillSlug, publisherId, visibility); newSkill.setCreatedBy(publisherId); - return skillRepository.save(newSkill); + try { + return skillRepository.save(newSkill); + } catch (DataIntegrityViolationException ex) { + // A concurrent publish for the same (namespace, slug, owner) coordinate inserted + // the skill first and won the unique-constraint race. Surface a deterministic + // business conflict instead of leaking the violation as an HTTP 500. + throw new DomainBadRequestException("error.skill.publish.concurrentConflict", skillSlug); + } }); if (skill.getStatus() == SkillStatus.ARCHIVED) { @@ -476,7 +484,13 @@ public class SkillPublishService { throw new IllegalStateException("Failed to serialize metadata", e); } - version = skillVersionRepository.save(version); + try { + version = skillVersionRepository.save(version); + } catch (DataIntegrityViolationException ex) { + // A concurrent publish inserted the same (skillId, version) coordinate first and won the + // unique-constraint race. Surface a deterministic business conflict rather than an HTTP 500. + throw new DomainBadRequestException("error.skill.publish.concurrentConflict", skillSlug); + } // 9. Upload each file to storage and compute SHA-256 List skillFiles = new ArrayList<>(); diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java index c084c2f8..67ce616c 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java @@ -22,6 +22,7 @@ import com.iflytek.skillhub.domain.skill.validation.SkillPackageValidator; import com.iflytek.skillhub.domain.skill.validation.ValidationResult; import com.iflytek.skillhub.storage.ObjectStorageService; import org.springframework.context.ApplicationEventPublisher; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.transaction.support.TransactionSynchronization; import org.springframework.transaction.support.TransactionSynchronizationManager; import org.junit.jupiter.api.AfterEach; @@ -1754,6 +1755,78 @@ class SkillPublishServiceTest { return List.of(skillMd, readme); } + @Test + void testPublishFromEntries_concurrentSkillInsertReturnsBusinessConflict() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-100"; + String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody"; + + PackageEntry skillMd = new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"); + List entries = List.of(skillMd); + + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + NamespaceMember member = mock(NamespaceMember.class); + SkillMetadata metadata = new SkillMetadata("test-skill", "Test", "1.0.0", "Body", Map.of()); + + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); + when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass()); + when(skillMetadataParser.parse(skillMdContent)).thenReturn(metadata); + when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass()); + // No other owner's skill blocks the name, and this owner has none yet either. + when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of()); + when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(any(), eq("test-skill"), eq(publisherId))) + .thenReturn(Optional.empty()); + // A concurrent publish inserts the same (namespace, slug, owner) coordinate first and wins the race. + when(skillRepository.save(any())).thenThrow(new DataIntegrityViolationException("duplicate key")); + + DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () -> service.publishFromEntries( + namespaceSlug, entries, publisherId, SkillVisibility.PUBLIC, Set.of())); + + assertEquals("error.skill.publish.concurrentConflict", exception.messageCode()); + assertEquals("test-skill", String.valueOf(exception.messageArgs()[0])); + verify(skillVersionRepository, never()).save(any(SkillVersion.class)); + } + + @Test + void testPublishFromEntries_concurrentVersionInsertReturnsBusinessConflict() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-100"; + String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody"; + + PackageEntry skillMd = new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"); + List entries = List.of(skillMd); + + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + NamespaceMember member = mock(NamespaceMember.class); + SkillMetadata metadata = new SkillMetadata("test-skill", "Test", "1.0.0", "Body", Map.of()); + + Skill skill = new Skill(1L, "test-skill", publisherId, SkillVisibility.PUBLIC); + setId(skill, 1L); + + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); + when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass()); + when(skillMetadataParser.parse(skillMdContent)).thenReturn(metadata); + when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass()); + when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of(skill)); + when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(any(), eq("test-skill"), eq(publisherId))) + .thenReturn(Optional.of(skill)); + when(skillVersionRepository.findBySkillIdAndVersion(any(), eq("1.0.0"))).thenReturn(Optional.empty()); + // A concurrent publish inserts the same (skillId, version) coordinate first and wins the race. + when(skillVersionRepository.save(any(SkillVersion.class))) + .thenThrow(new DataIntegrityViolationException("duplicate key")); + + DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () -> service.publishFromEntries( + namespaceSlug, entries, publisherId, SkillVisibility.PUBLIC, Set.of())); + + assertEquals("error.skill.publish.concurrentConflict", exception.messageCode()); + // Nothing should be uploaded to object storage once the coordinate race is lost. + verify(objectStorageService, never()).putObject(anyString(), any(), anyLong(), anyString()); + } + private void setId(Object entity, Long id) throws Exception { Field idField = entity.getClass().getDeclaredField("id"); idField.setAccessible(true); From 182f7bacef8374f34566e878d6e1af8a52a44156 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:30:33 +0800 Subject: [PATCH 2/2] fix(publish): flush concurrent coordinate writes Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- .../domain/skill/service/SkillPublishService.java | 8 +++++++- .../domain/skill/service/SkillPublishServiceTest.java | 11 +++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java index 380b06ca..bc6e0b0a 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java @@ -424,7 +424,11 @@ public class SkillPublishService { Skill newSkill = new Skill(namespace.getId(), skillSlug, publisherId, visibility); newSkill.setCreatedBy(publisherId); try { - return skillRepository.save(newSkill); + Skill savedSkill = skillRepository.save(newSkill); + // save() may defer the unique-constraint check until transaction commit. + // Flush here so this boundary can translate the coordinate race. + skillRepository.flush(); + return savedSkill; } catch (DataIntegrityViolationException ex) { // A concurrent publish for the same (namespace, slug, owner) coordinate inserted // the skill first and won the unique-constraint race. Surface a deterministic @@ -486,6 +490,8 @@ public class SkillPublishService { try { version = skillVersionRepository.save(version); + // Detect the version-coordinate race before object storage writes begin. + skillVersionRepository.flush(); } catch (DataIntegrityViolationException ex) { // A concurrent publish inserted the same (skillId, version) coordinate first and won the // unique-constraint race. Surface a deterministic business conflict rather than an HTTP 500. diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java index 67ce616c..a680e320 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java @@ -476,7 +476,7 @@ class SkillPublishServiceTest { verify(reviewTaskRepository).deleteBySkillVersionIdIn(List.of(8L)); verify(skillFileRepository).deleteByVersionId(8L); verify(skillVersionRepository).delete(rejectedVersion); - verify(skillVersionRepository).flush(); + verify(skillVersionRepository, times(2)).flush(); verify(objectStorageService).deleteObjects(List.of("skills/1/8/SKILL.md", "packages/1/8/bundle.zip")); ArgumentCaptor reviewTaskCaptor = ArgumentCaptor.forClass(ReviewTask.class); @@ -1779,13 +1779,15 @@ class SkillPublishServiceTest { when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(any(), eq("test-skill"), eq(publisherId))) .thenReturn(Optional.empty()); // A concurrent publish inserts the same (namespace, slug, owner) coordinate first and wins the race. - when(skillRepository.save(any())).thenThrow(new DataIntegrityViolationException("duplicate key")); + when(skillRepository.save(any())).thenAnswer(invocation -> invocation.getArgument(0)); + doThrow(new DataIntegrityViolationException("duplicate key")).when(skillRepository).flush(); DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () -> service.publishFromEntries( namespaceSlug, entries, publisherId, SkillVisibility.PUBLIC, Set.of())); assertEquals("error.skill.publish.concurrentConflict", exception.messageCode()); assertEquals("test-skill", String.valueOf(exception.messageArgs()[0])); + verify(skillRepository).flush(); verify(skillVersionRepository, never()).save(any(SkillVersion.class)); } @@ -1816,13 +1818,14 @@ class SkillPublishServiceTest { .thenReturn(Optional.of(skill)); when(skillVersionRepository.findBySkillIdAndVersion(any(), eq("1.0.0"))).thenReturn(Optional.empty()); // A concurrent publish inserts the same (skillId, version) coordinate first and wins the race. - when(skillVersionRepository.save(any(SkillVersion.class))) - .thenThrow(new DataIntegrityViolationException("duplicate key")); + when(skillVersionRepository.save(any(SkillVersion.class))).thenAnswer(invocation -> invocation.getArgument(0)); + doThrow(new DataIntegrityViolationException("duplicate key")).when(skillVersionRepository).flush(); DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () -> service.publishFromEntries( namespaceSlug, entries, publisherId, SkillVisibility.PUBLIC, Set.of())); assertEquals("error.skill.publish.concurrentConflict", exception.messageCode()); + verify(skillVersionRepository).flush(); // Nothing should be uploaded to object storage once the coordinate race is lost. verify(objectStorageService, never()).putObject(anyString(), any(), anyLong(), anyString()); }