diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SecurityScanRetryAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SecurityScanRetryAppService.java index d68cf460..19667b7e 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SecurityScanRetryAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SecurityScanRetryAppService.java @@ -56,19 +56,18 @@ public class SecurityScanRetryAppService { .orElseThrow(() -> new DomainBadRequestException("error.skill.notFound", skillId)); authorize(skill, userId, platformRoles, namespaceRoles); - SkillVersion observedVersion = skillVersionRepository.findById(versionId) - .filter(candidate -> candidate.getSkillId().equals(skillId)) + SkillVersionStatus observedStatus = skillVersionRepository.findStatusByIdAndSkillId(versionId, skillId) .orElseThrow(() -> new DomainBadRequestException("error.skill.version.notFound", versionId)); - if (observedVersion.getStatus() != SkillVersionStatus.SCAN_FAILED - && observedVersion.getStatus() != SkillVersionStatus.SCANNING) { - throw new DomainBadRequestException("error.security.scan.retry.status", observedVersion.getStatus()); + if (observedStatus != SkillVersionStatus.SCAN_FAILED + && observedStatus != SkillVersionStatus.SCANNING) { + throw new DomainBadRequestException("error.security.scan.retry.status", observedStatus); } if (!securityScanService.isEnabled()) { throw new DomainBadRequestException("error.security.scan.retry.disabled"); } String bundleKey = bundleKey(skillId, versionId); - if (observedVersion.getStatus() == SkillVersionStatus.SCAN_FAILED + if (observedStatus == SkillVersionStatus.SCAN_FAILED && !objectStorageService.exists(bundleKey)) { throw new DomainBadRequestException("error.security.scan.retry.bundleMissing"); } diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SecurityScanRetryAppServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SecurityScanRetryAppServiceTest.java index b5b11eb1..b0b2e60a 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SecurityScanRetryAppServiceTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SecurityScanRetryAppServiceTest.java @@ -64,7 +64,8 @@ class SecurityScanRetryAppServiceTest { @Test void retry_asOwnerCreatesNewAttemptAndAuditLog() { - given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version)); + given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L)) + .willReturn(Optional.of(SkillVersionStatus.SCAN_FAILED)); given(skillVersionRepository.findByIdForUpdate(42L)).willReturn(Optional.of(version)); given(securityScanService.isEnabled()).willReturn(true); given(objectStorageService.exists("packages/8/42/bundle.zip")).willReturn(true); @@ -84,7 +85,8 @@ class SecurityScanRetryAppServiceTest { @Test void retry_allowsNamespaceAdminAndPlatformSecurityAdmin() { - given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version)); + given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L)) + .willReturn(Optional.of(SkillVersionStatus.SCAN_FAILED)); given(skillVersionRepository.findByIdForUpdate(42L)).willReturn(Optional.of(version)); given(securityScanService.isEnabled()).willReturn(true); given(objectStorageService.exists("packages/8/42/bundle.zip")).willReturn(true); @@ -107,13 +109,14 @@ class SecurityScanRetryAppServiceTest { .isInstanceOf(DomainForbiddenException.class); verify(skillVersionRepository, never()).findByIdForUpdate(any()); - verify(skillVersionRepository, never()).findById(any()); + verify(skillVersionRepository, never()).findStatusByIdAndSkillId(any(), any()); } @Test void retry_rejectsNonFailedVersion() { version.setStatus(SkillVersionStatus.PENDING_REVIEW); - given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version)); + given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L)) + .willReturn(Optional.of(SkillVersionStatus.PENDING_REVIEW)); assertThatThrownBy(() -> service.retry( 8L, 42L, "owner-1", Set.of(), Map.of(), new AuditRequestContext(null, null))) @@ -125,7 +128,8 @@ class SecurityScanRetryAppServiceTest { @Test void retry_rejectsMissingStoredBundle() { - given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version)); + given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L)) + .willReturn(Optional.of(SkillVersionStatus.SCAN_FAILED)); given(securityScanService.isEnabled()).willReturn(true); assertThatThrownBy(() -> service.retry( @@ -139,7 +143,8 @@ class SecurityScanRetryAppServiceTest { @Test void retry_whenAttemptAlreadyStartedReturnsCurrentStateWithoutDuplicateTask() { version.setStatus(SkillVersionStatus.SCANNING); - given(skillVersionRepository.findById(42L)).willReturn(Optional.of(version)); + given(skillVersionRepository.findStatusByIdAndSkillId(42L, 8L)) + .willReturn(Optional.of(SkillVersionStatus.SCANNING)); given(skillVersionRepository.findByIdForUpdate(42L)).willReturn(Optional.of(version)); given(securityScanService.isEnabled()).willReturn(true); given(securityAuditRepository.findLatestActiveByVersionIdAndScannerType(42L, ScannerType.SKILL_SCANNER)) diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SecurityScanRetryLockingTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SecurityScanRetryLockingTest.java new file mode 100644 index 00000000..986a07ff --- /dev/null +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SecurityScanRetryLockingTest.java @@ -0,0 +1,125 @@ +package com.iflytek.skillhub.service; + +import com.iflytek.skillhub.domain.namespace.Namespace; +import com.iflytek.skillhub.domain.skill.Skill; +import com.iflytek.skillhub.domain.skill.SkillVersion; +import com.iflytek.skillhub.domain.skill.SkillVersionStatus; +import com.iflytek.skillhub.domain.skill.SkillVisibility; +import com.iflytek.skillhub.domain.user.UserAccount; +import com.iflytek.skillhub.infra.jpa.SkillVersionJpaRepository; +import jakarta.persistence.EntityManager; +import jakarta.persistence.PersistenceContext; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase; +import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.support.TransactionTemplate; +import org.testcontainers.containers.PostgreSQLContainer; +import org.testcontainers.junit.jupiter.Container; +import org.testcontainers.junit.jupiter.Testcontainers; + +import static org.assertj.core.api.Assertions.assertThat; + +@DataJpaTest +@AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE) +@ActiveProfiles("test") +@Testcontainers +class SecurityScanRetryLockingTest { + + @Container + private static final PostgreSQLContainer POSTGRES = + new PostgreSQLContainer<>("postgres:16-alpine"); + + @DynamicPropertySource + static void configurePostgres(DynamicPropertyRegistry registry) { + registry.add("spring.datasource.url", POSTGRES::getJdbcUrl); + registry.add("spring.datasource.username", POSTGRES::getUsername); + registry.add("spring.datasource.password", POSTGRES::getPassword); + registry.add("spring.datasource.driver-class-name", () -> "org.postgresql.Driver"); + registry.add("spring.jpa.database-platform", () -> "org.hibernate.dialect.PostgreSQLDialect"); + } + + @Autowired + private SkillVersionJpaRepository skillVersionRepository; + + @Autowired + private PlatformTransactionManager transactionManager; + + @PersistenceContext + private EntityManager entityManager; + + @Test + @Transactional(propagation = Propagation.NOT_SUPPORTED) + void lockReadSeesStateCommittedWhileWaitingInsteadOfCachedPreflightEntity() throws Exception { + Fixture fixture = persistFailedVersion(); + Long versionId = fixture.versionId(); + CountDownLatch firstLocked = new CountDownLatch(1); + CountDownLatch secondAboutToLock = new CountDownLatch(1); + CountDownLatch releaseFirst = new CountDownLatch(1); + TransactionTemplate transactions = new TransactionTemplate(transactionManager); + + try (var executor = Executors.newVirtualThreadPerTaskExecutor()) { + var first = executor.submit(() -> transactions.executeWithoutResult(status -> { + SkillVersion version = skillVersionRepository.findByIdForUpdate(versionId).orElseThrow(); + version.setStatus(SkillVersionStatus.SCANNING); + firstLocked.countDown(); + await(releaseFirst); + })); + + assertThat(firstLocked.await(10, TimeUnit.SECONDS)).isTrue(); + var second = executor.submit(() -> transactions.execute(status -> { + assertThat(skillVersionRepository.findStatusByIdAndSkillId(versionId, fixture.skillId())) + .contains(SkillVersionStatus.SCAN_FAILED); + secondAboutToLock.countDown(); + return skillVersionRepository.findByIdForUpdate(versionId).orElseThrow().getStatus(); + })); + + assertThat(secondAboutToLock.await(10, TimeUnit.SECONDS)).isTrue(); + releaseFirst.countDown(); + first.get(); + assertThat(second.get()).isEqualTo(SkillVersionStatus.SCANNING); + } + } + + private Fixture persistFailedVersion() { + TransactionTemplate transaction = new TransactionTemplate(transactionManager); + return transaction.execute(status -> { + UserAccount user = new UserAccount("retry-lock-user", "Retry Lock User", null, null); + entityManager.persist(user); + Namespace namespace = new Namespace("retry-lock", "Retry Lock", user.getId()); + entityManager.persist(namespace); + entityManager.flush(); + Skill skill = new Skill(namespace.getId(), "retry-lock", user.getId(), SkillVisibility.PRIVATE); + entityManager.persist(skill); + entityManager.flush(); + SkillVersion version = new SkillVersion(skill.getId(), "1.0.0", user.getId()); + version.setStatus(SkillVersionStatus.SCAN_FAILED); + entityManager.persist(version); + entityManager.flush(); + return new Fixture(version.getId(), skill.getId()); + }); + } + + private void await(CountDownLatch latch) { + try { + if (!latch.await(10, TimeUnit.SECONDS)) { + throw new IllegalStateException("Timed out waiting for concurrent retry test"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Concurrent retry test interrupted", e); + } + } + + private record Fixture(Long versionId, Long skillId) { + } +} diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/SkillVersionRepository.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/SkillVersionRepository.java index 257ef03c..90018dfc 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/SkillVersionRepository.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/SkillVersionRepository.java @@ -11,6 +11,11 @@ public interface SkillVersionRepository { default Optional findByIdForUpdate(Long id) { return findById(id); } + default Optional findStatusByIdAndSkillId(Long id, Long skillId) { + return findById(id) + .filter(version -> version.getSkillId().equals(skillId)) + .map(SkillVersion::getStatus); + } List findByIdIn(List ids); List findBySkillIdIn(List skillIds); List findBySkillIdInAndStatus(List skillIds, SkillVersionStatus status); diff --git a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillVersionJpaRepository.java b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillVersionJpaRepository.java index 4aba319c..481d6127 100644 --- a/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillVersionJpaRepository.java +++ b/server/skillhub-infra/src/main/java/com/iflytek/skillhub/infra/jpa/SkillVersionJpaRepository.java @@ -26,6 +26,10 @@ public interface SkillVersionJpaRepository extends JpaRepository findByIdForUpdate(@Param("id") Long id); + + @Override + @Query("SELECT version.status FROM SkillVersion version WHERE version.id = :id AND version.skillId = :skillId") + Optional findStatusByIdAndSkillId(@Param("id") Long id, @Param("skillId") Long skillId); List findByIdIn(List ids); List findBySkillId(Long skillId); List findBySkillIdIn(List skillIds);