fix(governance): safely delete version dependencies

Remove terminal review tasks before deleting an allowed skill version. Lock all versions of the aggregate in stable order so concurrent deletes preserve the last-version invariant and return business errors instead of 500 responses. Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
2026-09-08 22:21:09 +00:00 · 2026-07-30 17:18:44 +08:00 · 2026-07-30 17:18:44 +08:00 · 41704e151e
commit 41704e151e
parent 6817d98007
11 changed files with 459 additions and 13 deletions
--- a/docs/verification/issue-611.md
+++ b/docs/verification/issue-611.md
@ -0,0 +1,151 @@
+# Issue #611 Verification Report
+
+- Issue: [#611 — Direct version deletion leaves child FKs and returns 500](https://github.com/iflytek/skillhub/issues/611)
+- Status: Local verification complete; `big-main` remote verification pending
+- Baseline: `origin/main` at `6817d98007c7890c1a8ecb3e822272287bb37a05`
+- Working branch: `fix/issue-611-child-fks`
+
+## Decision
+
+### Does the bug exist?
+
+Yes. Against the baseline commit, a real PostgreSQL 16 instance and the real HTTP endpoint reproduced
+the failure three times:
+
+```http
+DELETE /api/web/skills/global/issue-611-repro/versions/1.0.0
+```
+
+The target version was `REJECTED`, had a terminal `REJECTED` review task, and was not the skill's
+last version. Every request returned HTTP 500. PostgreSQL reported SQL state `23503` because
+`review_task_skill_version_id_fkey` still referenced the version. The transaction rolled back and
+preserved both versions and the review task.
+
+As a control, deleting only the review task before retrying the same request changed the result to
+HTTP 200 and the version was deleted.
+
+### Is it worth fixing?
+
+Yes:
+
+- The supported deletion path for a normally rejected version fails deterministically.
+- The failure is exposed as an unexpected HTTP 500 rather than a business response.
+- The endpoint cannot fulfil the documented lifecycle operation or free the version string for
+  reuse.
+- The fix can reuse an existing repository operation already used by same-version replacement,
+  keeping implementation and regression risk small.
+
+### Is a broader change appropriate?
+
+The fix should cover every review task owned by the deleted version, regardless of task status. A
+broader generic child-cleanup framework or schema migration is not justified:
+
+- `skill_tag.version_id` can only be assigned to a `PUBLISHED` version through the domain service.
+- `promotion_request.source_version_id` can only be assigned to a `PUBLISHED` version.
+- Direct version deletion rejects `PUBLISHED` and `YANKED` versions before mutation.
+- `security_audit` intentionally retains soft-deleted history; migration V38 removed its version
+  foreign key for this purpose.
+- `skill_file` is already deleted explicitly and `skill_version_stats` uses `ON DELETE CASCADE`.
+
+The implementation will therefore delete all `review_task` rows for the target version before
+deleting `skill_version`, using the existing `ReviewTaskRepository.deleteBySkillVersionIdIn`
+operation.
+
+## Operation Log
+
+| Time (UTC+8) | Operation | Result |
+|--------------|-----------|--------|
+| 2026-07-30 14:41–14:47 | Reproduced the issue on the baseline using isolated PostgreSQL 16, Redis 7, and backend port 18081 | Three HTTP 500 responses; PostgreSQL FK violation confirmed |
+| 2026-07-30 14:46 | Removed the target review task and repeated the request | HTTP 200; version count changed from 2 to 1 |
+| 2026-07-30 14:47 | Stopped the backend and removed diagnostic containers and temporary files | Cleanup complete; repository remained unchanged |
+| 2026-07-30 14:55 | Fetched `origin/main` and `origin/big-main` | Baselines unchanged at `6817d980` and `7774935d` |
+| 2026-07-30 14:56 | Created `fix/issue-611-child-fks` from `origin/main` | Working branch established |
+| 2026-07-30 15:02 | Reviewed every schema reference to `skill_version` and the related lifecycle services | Scope limited to reachable review-task ownership |
+| 2026-07-30 15:02–16:06 | Implemented review-task cleanup and stable aggregate locking, then ran unit, integration, PostgreSQL 16, frontend, and staging checks | Fix and adjacent concurrency cases passed |
+| 2026-07-30 16:06 | Completed backend regression excluding the independently verified polluted auth class | 659 tests passed, 1 skipped; reactor build succeeded |
+| 2026-07-30 16:08 | Removed the isolated local containers, network, images, staging overrides, and temporary test data | Local cleanup complete |
+| 2026-07-30 16:09 | Inventoried the remote test host without changing its runtime | Docker 29.4.0 and PostgreSQL 16 runtime found; isolated deployment required because ports 8080/8081 and 5432/6379 are in use |
+
+## Test Matrix
+
+| Category | Scenario | Expected result | Status |
+|----------|----------|-----------------|--------|
+| Regression | Delete a non-last `REJECTED` version with a terminal review task | HTTP 200; version and its review tasks removed | Local passed; remote pending |
+| Happy path | Delete non-last `DRAFT`, `UPLOADED`, and `SCAN_FAILED` versions | HTTP 200; target artifacts removed | Unit passed; remote pending |
+| Multiple children | A version has more than one historical review task | All review tasks for only that version are removed | Local integration passed; remote pending |
+| Isolation | Another version has its own review task | Other version and task remain unchanged | Local integration passed; remote pending |
+| Boundary | Delete the last remaining version | Business 4xx; all data remains | Unit and PostgreSQL concurrency passed; remote pending |
+| Lifecycle guard | Delete a `PUBLISHED` version referenced by a tag or promotion | Business 4xx; references remain unchanged | Unit passed; remote pending |
+| Permission | Unauthorized member deletes another owner's version | HTTP 403; all data remains | Unit passed; remote pending |
+| Audit retention | Deletable version has security audit history | Version deletes; audit is soft-deleted and retained | Unit passed; remote pending |
+| File integrity | Deletable version has file rows and bundle storage | Database rows delete; storage cleanup runs after commit | Unit passed; remote pending |
+| Statistics | Deletable version has `skill_version_stats` | Statistics row cascades without affecting other versions | Schema reviewed; remote pending |
+| Concurrency | Two requests target the same version | No 500; one succeeds and the other gets a deterministic missing-resource response | PostgreSQL 16 passed; remote pending |
+| Concurrency | Two requests target different versions of a two-version skill | No 500 and at least one version remains | PostgreSQL 16 passed; remote pending |
+| Rollback | A precondition or authorization check rejects deletion | No database or storage mutation | Unit passed; remote pending |
+| Backward compatibility | Successful response contract | Existing response shape and action remain unchanged | Controller and integration passed; remote pending |
+| Full regression | Backend, frontend checks, staging smoke tests | Required checks pass | Local passed with documented baseline issues; remote pending |
+
+## Test Report
+
+### Local environment
+
+- Baseline: `6817d98007c7890c1a8ecb3e822272287bb37a05`
+- Database/runtime: PostgreSQL 16 Alpine, Redis 7 Alpine, Eclipse Temurin 21 runtime image
+- Isolated API: `127.0.0.1:18081`
+- Final PostgreSQL behavior image:
+  `skillhub-issue-611:local-aggregate-lock`
+  (`sha256:69c7e1810d7bfed8549d2109db09f8203d63748a473d1770ce154093eaa7cdc5`)
+- Staging image:
+  `skillhub-server:staging`
+  (`sha256:5344d26b56e07132ae3b0b3f8359e43b678a10329e415610c95b4e0383018aa5`)
+
+#### Automated checks
+
+| Check | Result |
+|-------|--------|
+| `SkillGovernanceServiceTest` | 15/15 passed |
+| App lifecycle/controller/persistence targeted set | 11/11 passed |
+| `SkillVersionDeleteFlowIntegrationTest` | Passed against the H2 persistence test profile |
+| Frontend typecheck | Passed |
+| Frontend lint | Passed |
+| Frontend Vitest | 181 files, 618 tests passed |
+| Staging smoke | 15/15 passed |
+| Backend reactor excluding `ApiTokenAuthenticationFilterTest` | 659 tests passed, 1 skipped; all 7 dependent modules succeeded |
+| `ApiTokenAuthenticationFilterTest` in isolation | 9/9 passed |
+
+The normal backend aggregate run was blocked by a pre-existing test-isolation defect:
+`ApiTokenAuthenticationFilterTest.shouldIgnoreNonBearerAuthorizationHeader` observed a
+`SecurityContext` left by another auth test. This change does not touch auth. The class passed in
+isolation, and every other backend test passed in a single reactor run.
+
+Staging also exposed two baseline environment conflicts:
+
+1. Host port 5432 was already owned by an unrelated `postgres-local` container, so staging used
+   temporary isolated port overrides instead of stopping it.
+2. The staging web service does not define `SKILLHUB_TRUST_FORWARDED_PROTO`; setting it to `false`
+   in the temporary override was required for Nginx startup.
+
+Both overrides were outside the repository and were deleted after the 15/15 smoke pass.
+
+#### Real PostgreSQL behavior
+
+- Baseline: three identical rejected-version deletes returned HTTP 500 with SQL state `23503`.
+- Fixed rejected-version delete: HTTP 200; the target version and its review task were deleted.
+- Same-version concurrent delete: one HTTP 200 and one HTTP 400
+  (`Version not found`); no 500 or optimistic-lock exception.
+- Different-version concurrent delete on a two-version skill: one HTTP 200 and one HTTP 400
+  (`Cannot delete the last remaining version`); exactly one version remained.
+- No unexpected `ERROR` or exception was present in the application log after the fixed scenarios.
+
+### `big-main` remote environment
+
+Pending integration merge, unique image build, isolated deployment, and full remote matrix.
+
+### Remaining risks
+
+- The remote PostgreSQL 16 matrix has not yet run against the exact `big-main` merge commit.
+- The aggregate-lock query is PostgreSQL/H2-specific native SQL by design; any future database
+  implementation must provide equivalent stable pessimistic locking.
+- The unrelated auth test-isolation defect can still make the default backend aggregate command
+  fail depending on test order.
--- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLifecycleAppService.java
+++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLifecycleAppService.java
@ -98,7 +98,7 @@ public class SkillLifecycleAppService {
                                                        Map<Long, NamespaceRole> userNamespaceRoles,
                                                        AuditRequestContext auditContext) {
        Skill skill = findSkill(namespace, slug, userId);
-        SkillVersion skillVersion = findVersion(skill.getId(), version);
+        SkillVersion skillVersion = findVersionForUpdate(skill.getId(), version);
        skillGovernanceService.deleteVersion(
                skill,
                skillVersion,
@ -261,6 +261,13 @@ public class SkillLifecycleAppService {
                .orElseThrow(() -> new DomainBadRequestException("error.skill.version.notFound", version));
    }

+    private SkillVersion findVersionForUpdate(Long skillId, String version) {
+        return skillVersionRepository.findBySkillIdForUpdate(skillId).stream()
+                .filter(candidate -> candidate.getVersion().equals(version))
+                .findFirst()
+                .orElseThrow(() -> new DomainBadRequestException("error.skill.version.notFound", version));
+    }
+
    private Map<Long, NamespaceRole> normalizeRoles(Map<Long, NamespaceRole> userNamespaceRoles) {
        return userNamespaceRoles != null ? userNamespaceRoles : Map.of();
    }
--- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SkillLifecycleControllerTest.java
+++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SkillLifecycleControllerTest.java
@ -144,7 +144,8 @@ class SkillLifecycleControllerTest {
        given(namespaceRepository.findBySlug("global")).willReturn(java.util.Optional.of(namespace));
        given(skillSlugResolutionService.resolve(1L, "demo-skill", "usr_1", SkillSlugResolutionService.Preference.CURRENT_USER))
                .willReturn(skill);
-        given(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).willReturn(java.util.Optional.of(version));
+        given(skillVersionRepository.findBySkillIdForUpdate(1L))
+                .willReturn(java.util.List.of(version));

        mockMvc.perform(delete("/api/web/skills/global/demo-skill/versions/1.0.0")
                        .requestAttr("userId", "usr_1")
--- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SkillVersionDeleteFlowIntegrationTest.java
+++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SkillVersionDeleteFlowIntegrationTest.java
@ -0,0 +1,144 @@
+package com.iflytek.skillhub.controller.portal;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.Mockito.verify;
+import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.authentication;
+import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
+
+import com.iflytek.skillhub.TestRedisConfig;
+import com.iflytek.skillhub.auth.device.DeviceAuthService;
+import com.iflytek.skillhub.auth.rbac.PlatformPrincipal;
+import com.iflytek.skillhub.domain.namespace.Namespace;
+import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository;
+import com.iflytek.skillhub.domain.namespace.NamespaceRepository;
+import com.iflytek.skillhub.domain.review.ReviewTask;
+import com.iflytek.skillhub.domain.review.ReviewTaskRepository;
+import com.iflytek.skillhub.domain.review.ReviewTaskStatus;
+import com.iflytek.skillhub.domain.skill.Skill;
+import com.iflytek.skillhub.domain.skill.SkillRepository;
+import com.iflytek.skillhub.domain.skill.SkillVersion;
+import com.iflytek.skillhub.domain.skill.SkillVersionRepository;
+import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
+import com.iflytek.skillhub.domain.skill.SkillVisibility;
+import com.iflytek.skillhub.storage.ObjectStorageService;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.mock.mockito.MockBean;
+import org.springframework.context.annotation.Import;
+import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
+import org.springframework.security.core.authority.SimpleGrantedAuthority;
+import org.springframework.test.context.ActiveProfiles;
+import org.springframework.test.web.servlet.MockMvc;
+
+@SpringBootTest
+@AutoConfigureMockMvc
+@ActiveProfiles("test")
+@Import(TestRedisConfig.class)
+class SkillVersionDeleteFlowIntegrationTest {
+
+    @Autowired
+    private MockMvc mockMvc;
+
+    @Autowired
+    private NamespaceRepository namespaceRepository;
+
+    @Autowired
+    private SkillRepository skillRepository;
+
+    @Autowired
+    private SkillVersionRepository skillVersionRepository;
+
+    @Autowired
+    private ReviewTaskRepository reviewTaskRepository;
+
+    @MockBean
+    private ObjectStorageService objectStorageService;
+
+    @MockBean
+    private NamespaceMemberRepository namespaceMemberRepository;
+
+    @MockBean
+    private DeviceAuthService deviceAuthService;
+
+    @Test
+    void deleteRejectedVersion_removesOnlyItsReviewHistory() throws Exception {
+        String ownerId = "owner-1";
+        String suffix = UUID.randomUUID().toString().substring(0, 8);
+        Namespace namespace = namespaceRepository.save(
+                new Namespace("version-delete-" + suffix, "Version Delete " + suffix, ownerId)
+        );
+
+        Skill skill = new Skill(namespace.getId(), "demo-skill-" + suffix, ownerId, SkillVisibility.PUBLIC);
+        skill.setCreatedBy(ownerId);
+        skill.setUpdatedBy(ownerId);
+        skill = skillRepository.save(skill);
+
+        SkillVersion rejectedVersion = new SkillVersion(skill.getId(), "1.0.0", ownerId);
+        rejectedVersion.setStatus(SkillVersionStatus.REJECTED);
+        rejectedVersion = skillVersionRepository.save(rejectedVersion);
+
+        SkillVersion retainedVersion = new SkillVersion(skill.getId(), "2.0.0", ownerId);
+        retainedVersion.setStatus(SkillVersionStatus.REJECTED);
+        retainedVersion = skillVersionRepository.save(retainedVersion);
+
+        ReviewTask rejectedTask = new ReviewTask(rejectedVersion.getId(), namespace.getId(), ownerId);
+        rejectedTask.setStatus(ReviewTaskStatus.REJECTED);
+        rejectedTask = reviewTaskRepository.save(rejectedTask);
+
+        ReviewTask approvedTask = new ReviewTask(rejectedVersion.getId(), namespace.getId(), ownerId);
+        approvedTask.setStatus(ReviewTaskStatus.APPROVED);
+        approvedTask = reviewTaskRepository.save(approvedTask);
+
+        ReviewTask retainedTask = new ReviewTask(retainedVersion.getId(), namespace.getId(), ownerId);
+        retainedTask.setStatus(ReviewTaskStatus.REJECTED);
+        retainedTask = reviewTaskRepository.save(retainedTask);
+
+        Long skillId = skill.getId();
+        Long rejectedVersionId = rejectedVersion.getId();
+
+        mockMvc.perform(delete("/api/web/skills/{namespace}/{slug}/versions/{version}",
+                        namespace.getSlug(), skill.getSlug(), rejectedVersion.getVersion())
+                        .with(authentication(portalAuth(ownerId, "USER")))
+                        .with(csrf()))
+                .andExpect(status().isOk())
+                .andExpect(jsonPath("$.code").value(0))
+                .andExpect(jsonPath("$.data.skillId").value(skillId))
+                .andExpect(jsonPath("$.data.versionId").value(rejectedVersionId))
+                .andExpect(jsonPath("$.data.action").value("DELETE_VERSION"))
+                .andExpect(jsonPath("$.data.status").value("1.0.0"));
+
+        assertThat(skillVersionRepository.findById(rejectedVersion.getId())).isEmpty();
+        assertThat(skillVersionRepository.findById(retainedVersion.getId())).isPresent();
+        assertThat(reviewTaskRepository.findById(rejectedTask.getId())).isEmpty();
+        assertThat(reviewTaskRepository.findById(approvedTask.getId())).isEmpty();
+        assertThat(reviewTaskRepository.findById(retainedTask.getId())).isPresent();
+        verify(objectStorageService).deleteObjects(argThat(keys ->
+                keys.equals(List.of("packages/" + skillId + "/" + rejectedVersionId + "/bundle.zip"))
+        ));
+    }
+
+    private UsernamePasswordAuthenticationToken portalAuth(String userId, String... roles) {
+        PlatformPrincipal principal = new PlatformPrincipal(
+                userId,
+                userId,
+                userId + "@example.com",
+                "",
+                "session",
+                Set.of(roles)
+        );
+        List<SimpleGrantedAuthority> authorities = Arrays.stream(roles)
+                .map(role -> new SimpleGrantedAuthority("ROLE_" + role))
+                .toList();
+        return new UsernamePasswordAuthenticationToken(principal, null, authorities);
+    }
+}
--- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillLifecycleAppServiceTest.java
+++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/SkillLifecycleAppServiceTest.java
@ -14,7 +14,9 @@ import com.iflytek.skillhub.domain.namespace.NamespaceRepository;
 import com.iflytek.skillhub.domain.namespace.NamespaceRole;
 import com.iflytek.skillhub.domain.review.ReviewService;
 import com.iflytek.skillhub.domain.skill.Skill;
+import com.iflytek.skillhub.domain.skill.SkillVersion;
 import com.iflytek.skillhub.domain.skill.SkillVersionRepository;
+import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
 import com.iflytek.skillhub.domain.skill.SkillVisibility;
 import com.iflytek.skillhub.domain.skill.service.SkillGovernanceService;
 import com.iflytek.skillhub.domain.skill.service.SkillPublishService;
@ -76,4 +78,50 @@ class SkillLifecycleAppServiceTest {
        assertThat(response.status()).isEqualTo("ARCHIVED");
        verify(skillGovernanceService).archiveSkill(11L, "owner-1", Map.of(7L, NamespaceRole.OWNER), "127.0.0.1", "JUnit", "cleanup");
    }
+
+    @Test
+    void deleteVersion_locksAllSkillVersionsBeforeDelegatingLifecycleMutation() {
+        Namespace namespace = new Namespace("global", "Global", "owner-1");
+        ReflectionTestUtils.setField(namespace, "id", 7L);
+        Skill skill = new Skill(7L, "demo-skill", "owner-1", SkillVisibility.PUBLIC);
+        ReflectionTestUtils.setField(skill, "id", 11L);
+        SkillVersion version = new SkillVersion(11L, "1.0.0", "owner-1");
+        ReflectionTestUtils.setField(version, "id", 13L);
+        version.setStatus(SkillVersionStatus.REJECTED);
+        SkillVersion retainedVersion = new SkillVersion(11L, "2.0.0", "owner-1");
+        ReflectionTestUtils.setField(retainedVersion, "id", 14L);
+        retainedVersion.setStatus(SkillVersionStatus.UPLOADED);
+
+        when(namespaceRepository.findBySlug("global")).thenReturn(Optional.of(namespace));
+        when(skillSlugResolutionService.resolve(
+                7L,
+                "demo-skill",
+                "owner-1",
+                SkillSlugResolutionService.Preference.CURRENT_USER
+        )).thenReturn(skill);
+        when(skillVersionRepository.findBySkillIdForUpdate(11L))
+                .thenReturn(java.util.List.of(version, retainedVersion));
+
+        var response = service.deleteVersion(
+                "global",
+                "demo-skill",
+                "1.0.0",
+                "owner-1",
+                Map.of(7L, NamespaceRole.OWNER),
+                new AuditRequestContext("127.0.0.1", "JUnit")
+        );
+
+        assertThat(response.versionId()).isEqualTo(13L);
+        assertThat(response.action()).isEqualTo("DELETE_VERSION");
+        verify(skillVersionRepository).findBySkillIdForUpdate(11L);
+        verify(skillGovernanceService).deleteVersion(
+                skill,
+                version,
+                "owner-1",
+                Map.of(7L, NamespaceRole.OWNER),
+                "127.0.0.1",
+                "JUnit",
+                "global"
+        );
+    }
 }
--- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/stream/ScanTaskConsumerLoggingTest.java
+++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/stream/ScanTaskConsumerLoggingTest.java
@ -249,6 +249,11 @@ class ScanTaskConsumerLoggingTest {
            throw new UnsupportedOperationException();
        }

+        @Override
+        public List<SkillVersion> findBySkillIdForUpdate(Long skillId) {
+            throw new UnsupportedOperationException();
+        }
+
        @Override
        public List<SkillVersion> findBySkillIdAndStatus(Long skillId, SkillVersionStatus status) {
            throw new UnsupportedOperationException();
--- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/stream/ScanTaskConsumerTest.java
+++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/stream/ScanTaskConsumerTest.java
@ -415,6 +415,11 @@ class ScanTaskConsumerTest {
            throw unsupported();
        }

+        @Override
+        public List<SkillVersion> findBySkillIdForUpdate(Long skillId) {
+            throw unsupported();
+        }
+
        @Override
        public List<SkillVersion> findBySkillIdAndStatus(Long skillId, SkillVersionStatus status) {
            throw unsupported();
--- 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
@ -12,6 +12,7 @@ public interface SkillVersionRepository {
    List<SkillVersion> findBySkillIdIn(List<Long> skillIds);
    List<SkillVersion> findBySkillIdInAndStatus(List<Long> skillIds, SkillVersionStatus status);
    List<SkillVersion> findBySkillId(Long skillId);
+    List<SkillVersion> findBySkillIdForUpdate(Long skillId);
    Optional<SkillVersion> findBySkillIdAndVersion(Long skillId, String version);
    List<SkillVersion> findBySkillIdAndStatus(Long skillId, SkillVersionStatus status);
    SkillVersion save(SkillVersion version);
--- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java
+++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java
@ -3,6 +3,7 @@ package com.iflytek.skillhub.domain.skill.service;
 import com.iflytek.skillhub.domain.audit.AuditLogService;
 import com.iflytek.skillhub.domain.event.SkillStatusChangedEvent;
 import com.iflytek.skillhub.domain.namespace.NamespaceRole;
+import com.iflytek.skillhub.domain.review.ReviewTaskRepository;
 import com.iflytek.skillhub.domain.security.SecurityScanService;
 import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException;
 import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException;
@ -41,6 +42,7 @@ public class SkillGovernanceService {
    private final SkillRepository skillRepository;
    private final SkillVersionRepository skillVersionRepository;
    private final SkillFileRepository skillFileRepository;
+    private final ReviewTaskRepository reviewTaskRepository;
    private final ObjectStorageService objectStorageService;
    private final AuditLogService auditLogService;
    private final ApplicationEventPublisher eventPublisher;
@ -51,6 +53,7 @@ public class SkillGovernanceService {
    public SkillGovernanceService(SkillRepository skillRepository,
                                  SkillVersionRepository skillVersionRepository,
                                  SkillFileRepository skillFileRepository,
+                                  ReviewTaskRepository reviewTaskRepository,
                                  ObjectStorageService objectStorageService,
                                  AuditLogService auditLogService,
                                  ApplicationEventPublisher eventPublisher,
@ -60,6 +63,7 @@ public class SkillGovernanceService {
        this.skillRepository = skillRepository;
        this.skillVersionRepository = skillVersionRepository;
        this.skillFileRepository = skillFileRepository;
+        this.reviewTaskRepository = reviewTaskRepository;
        this.objectStorageService = objectStorageService;
        this.auditLogService = auditLogService;
        this.eventPublisher = eventPublisher;
@ -172,6 +176,8 @@ public class SkillGovernanceService {
            throw new DomainBadRequestException("error.skill.version.delete.lastVersion", version.getVersion());
        }

+        // Rejected versions retain terminal review history whose FK must not outlive the version.
+        reviewTaskRepository.deleteBySkillVersionIdIn(List.of(version.getId()));
        List<SkillFile> files = skillFileRepository.findByVersionId(version.getId());
        List<String> storageKeys = new ArrayList<>();
        files.stream()
--- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceServiceTest.java
+++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceServiceTest.java
@ -5,40 +5,43 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyList;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.doThrow;
-import static org.mockito.BDDMockito.given;

 import com.iflytek.skillhub.domain.audit.AuditLogService;
 import com.iflytek.skillhub.domain.event.SkillStatusChangedEvent;
 import com.iflytek.skillhub.domain.namespace.NamespaceRole;
+import com.iflytek.skillhub.domain.review.ReviewTaskRepository;
 import com.iflytek.skillhub.domain.security.SecurityScanService;
 import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException;
 import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException;
 import com.iflytek.skillhub.domain.skill.Skill;
 import com.iflytek.skillhub.domain.skill.SkillFile;
 import com.iflytek.skillhub.domain.skill.SkillFileRepository;
-import com.iflytek.skillhub.domain.skill.SkillStatus;
 import com.iflytek.skillhub.domain.skill.SkillRepository;
+import com.iflytek.skillhub.domain.skill.SkillStatus;
 import com.iflytek.skillhub.domain.skill.SkillVersion;
 import com.iflytek.skillhub.domain.skill.SkillVersionRepository;
 import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
 import com.iflytek.skillhub.storage.ObjectStorageService;
 import java.time.Clock;
 import java.time.Instant;
-import java.util.Optional;
-import java.util.Map;
 import java.time.ZoneOffset;
-import org.springframework.context.ApplicationEventPublisher;
-import org.springframework.transaction.support.TransactionSynchronization;
-import org.springframework.transaction.support.TransactionSynchronizationManager;
+import java.util.Map;
+import java.util.Optional;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InOrder;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import org.springframework.context.ApplicationEventPublisher;
+import org.springframework.transaction.support.TransactionSynchronization;
+import org.springframework.transaction.support.TransactionSynchronizationManager;

@ExtendWith(MockitoExtension.class)
 class SkillGovernanceServiceTest {
@ -52,6 +55,8 @@ class SkillGovernanceServiceTest {
    @Mock
    private SkillFileRepository skillFileRepository;
    @Mock
+    private ReviewTaskRepository reviewTaskRepository;
+    @Mock
    private ObjectStorageService objectStorageService;
    @Mock
    private AuditLogService auditLogService;
@ -70,6 +75,7 @@ class SkillGovernanceServiceTest {
                skillRepository,
                skillVersionRepository,
                skillFileRepository,
+                reviewTaskRepository,
                objectStorageService,
                auditLogService,
                eventPublisher,
@ -229,6 +235,35 @@ class SkillGovernanceServiceTest {
        verify(auditLogService).record("owner", "DELETE_SKILL_VERSION", "SKILL_VERSION", 2L, null, "127.0.0.1", "JUnit", "{\"version\":\"1.0.0\"}");
    }

+    @Test
+    void deleteVersion_removesReviewTasksBeforeRejectedVersion() {
+        Skill skill = new Skill(1L, "demo", "owner", com.iflytek.skillhub.domain.skill.SkillVisibility.PUBLIC);
+        setField(skill, "id", 1L);
+        SkillVersion rejectedVersion = new SkillVersion(1L, "1.0.0", "owner");
+        setField(rejectedVersion, "id", 2L);
+        rejectedVersion.setStatus(SkillVersionStatus.REJECTED);
+        SkillVersion otherVersion = new SkillVersion(1L, "2.0.0", "owner");
+        setField(otherVersion, "id", 3L);
+        otherVersion.setStatus(SkillVersionStatus.DRAFT);
+        given(skillVersionRepository.findBySkillId(1L))
+                .willReturn(java.util.List.of(rejectedVersion, otherVersion));
+        given(skillFileRepository.findByVersionId(2L)).willReturn(java.util.List.of());
+
+        service.deleteVersion(
+                skill,
+                rejectedVersion,
+                "owner",
+                Map.of(),
+                "127.0.0.1",
+                "JUnit",
+                "test-ns"
+        );
+
+        InOrder deletionOrder = inOrder(reviewTaskRepository, skillVersionRepository);
+        deletionOrder.verify(reviewTaskRepository).deleteBySkillVersionIdIn(java.util.List.of(2L));
+        deletionOrder.verify(skillVersionRepository).delete(rejectedVersion);
+    }
+
    @Test
    void deleteVersion_deletesStorageAfterCommitWhenSynchronizationIsActive() {
        Skill skill = new Skill(1L, "demo", "owner", com.iflytek.skillhub.domain.skill.SkillVisibility.PUBLIC);
@ -309,10 +344,36 @@ class SkillGovernanceServiceTest {
        assertThrows(DomainBadRequestException.class,
                () -> service.deleteVersion(skill, version, "owner", Map.of(), "127.0.0.1", "JUnit", "test-ns"));

+        verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(anyList());
        verify(skillVersionRepository, never()).delete(any());
        verify(objectStorageService, never()).deleteObject(any());
    }

+    @Test
+    void deleteVersion_rejectsUnauthorizedUserWithoutDeletingReviewTasks() {
+        Skill skill = new Skill(1L, "demo", "owner", com.iflytek.skillhub.domain.skill.SkillVisibility.PUBLIC);
+        setField(skill, "id", 1L);
+        SkillVersion version = new SkillVersion(1L, "1.0.0", "owner");
+        setField(version, "id", 2L);
+        version.setStatus(SkillVersionStatus.REJECTED);
+
+        assertThrows(
+                DomainForbiddenException.class,
+                () -> service.deleteVersion(
+                        skill,
+                        version,
+                        "member",
+                        Map.of(1L, NamespaceRole.MEMBER),
+                        "127.0.0.1",
+                        "JUnit",
+                        "test-ns"
+                )
+        );
+
+        verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(anyList());
+        verify(skillVersionRepository, never()).delete(any());
+    }
+
    @Test
    void deleteVersion_rejectsLastRemainingVersion() {
        Skill skill = new Skill(1L, "demo", "owner", com.iflytek.skillhub.domain.skill.SkillVisibility.PUBLIC);
@ -326,6 +387,7 @@ class SkillGovernanceServiceTest {
                () -> service.deleteVersion(skill, version, "owner", Map.of(), "127.0.0.1", "JUnit", "test-ns"));
        assertThat(ex.messageCode()).isEqualTo("error.skill.version.delete.lastVersion");

+        verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(anyList());
        verify(skillVersionRepository, never()).delete(any());
    }

--- 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
@ -3,16 +3,22 @@ package com.iflytek.skillhub.infra.jpa;
 import com.iflytek.skillhub.domain.skill.SkillVersion;
 import com.iflytek.skillhub.domain.skill.SkillVersionRepository;
 import com.iflytek.skillhub.domain.skill.SkillVersionStatus;
+import java.util.List;
+import java.util.Optional;
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.Pageable;
 import org.springframework.data.jpa.repository.JpaRepository;
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.repository.query.Param;
 import org.springframework.stereotype.Repository;

-import java.util.List;
-import java.util.Optional;
-
 /**
 * JPA-backed repository for skill version history and status-oriented version queries.
+ *
+ * <p>The deletion lock uses explicit {@code FOR UPDATE} SQL because Hibernate's PostgreSQL dialect
+ * emits {@code FOR NO KEY UPDATE}, which H2's PostgreSQL compatibility mode cannot execute. It
+ * locks every version in stable ID order so concurrent deletions cannot both remove the last
+ * versions of one skill.
 */
@Repository
 public interface SkillVersionJpaRepository extends JpaRepository<SkillVersion, Long>, SkillVersionRepository {
@ -22,6 +28,16 @@ public interface SkillVersionJpaRepository extends JpaRepository<SkillVersion, L
    List<SkillVersion> findBySkillIdInAndStatusOrderByCreatedAtDesc(List<Long> skillIds, SkillVersionStatus status);
    Optional<SkillVersion> findBySkillIdAndVersion(Long skillId, String version);

+    @Override
+    @Query(value = """
+        SELECT skill_version.*
+        FROM skill_version
+        WHERE skill_version.skill_id = :skillId
+        ORDER BY skill_version.id
+        FOR UPDATE
+    """, nativeQuery = true)
+    List<SkillVersion> findBySkillIdForUpdate(@Param("skillId") Long skillId);
+
    @Override
    default List<SkillVersion> findBySkillIdAndStatus(Long skillId, SkillVersionStatus status) {
        return findBySkillIdAndStatusOrderByCreatedAtDesc(skillId, status);