From 41704e151e0152cdc7688e5b5baec1df75b02dd7 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:18:44 +0800 Subject: [PATCH 1/2] 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> --- docs/verification/issue-611.md | 151 ++++++++++++++++++ .../service/SkillLifecycleAppService.java | 9 +- .../portal/SkillLifecycleControllerTest.java | 3 +- ...SkillVersionDeleteFlowIntegrationTest.java | 144 +++++++++++++++++ .../service/SkillLifecycleAppServiceTest.java | 48 ++++++ .../stream/ScanTaskConsumerLoggingTest.java | 5 + .../skillhub/stream/ScanTaskConsumerTest.java | 5 + .../domain/skill/SkillVersionRepository.java | 1 + .../skill/service/SkillGovernanceService.java | 6 + .../service/SkillGovernanceServiceTest.java | 78 ++++++++- .../infra/jpa/SkillVersionJpaRepository.java | 22 ++- 11 files changed, 459 insertions(+), 13 deletions(-) create mode 100644 docs/verification/issue-611.md create mode 100644 server/skillhub-app/src/test/java/com/iflytek/skillhub/controller/portal/SkillVersionDeleteFlowIntegrationTest.java diff --git a/docs/verification/issue-611.md b/docs/verification/issue-611.md new file mode 100644 index 00000000..7a353c86 --- /dev/null +++ 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. diff --git 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 index e7f86d45..3e2119d9 100644 --- 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 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 normalizeRoles(Map userNamespaceRoles) { return userNamespaceRoles != null ? userNamespaceRoles : Map.of(); } diff --git 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 index 09751065..bef8c274 100644 --- 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") diff --git 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 new file mode 100644 index 00000000..25d585e5 --- /dev/null +++ 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 authorities = Arrays.stream(roles) + .map(role -> new SimpleGrantedAuthority("ROLE_" + role)) + .toList(); + return new UsernamePasswordAuthenticationToken(principal, null, authorities); + } +} diff --git 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 index e2c10146..b007621f 100644 --- 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" + ); + } } diff --git 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 index 3175c83f..293a3dd7 100644 --- 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 findBySkillIdForUpdate(Long skillId) { + throw new UnsupportedOperationException(); + } + @Override public List findBySkillIdAndStatus(Long skillId, SkillVersionStatus status) { throw new UnsupportedOperationException(); diff --git 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 index 8e9a8ff6..c8c55f0f 100644 --- 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 findBySkillIdForUpdate(Long skillId) { + throw unsupported(); + } + @Override public List findBySkillIdAndStatus(Long skillId, SkillVersionStatus status) { throw unsupported(); 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 6565436f..b99cc50f 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 @@ -12,6 +12,7 @@ public interface SkillVersionRepository { List findBySkillIdIn(List skillIds); List findBySkillIdInAndStatus(List skillIds, SkillVersionStatus status); List findBySkillId(Long skillId); + List findBySkillIdForUpdate(Long skillId); Optional findBySkillIdAndVersion(Long skillId, String version); List findBySkillIdAndStatus(Long skillId, SkillVersionStatus status); SkillVersion save(SkillVersion version); diff --git 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 index bbb748f8..3cad927f 100644 --- 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 files = skillFileRepository.findByVersionId(version.getId()); List storageKeys = new ArrayList<>(); files.stream() diff --git 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 index b6f3faff..50aa3e4b 100644 --- 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()); } 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 eabb01e4..e7c85e1c 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 @@ -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. + * + *

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, SkillVersionRepository { @@ -22,6 +28,16 @@ public interface SkillVersionJpaRepository extends JpaRepository findBySkillIdInAndStatusOrderByCreatedAtDesc(List skillIds, SkillVersionStatus status); Optional 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 findBySkillIdForUpdate(@Param("skillId") Long skillId); + @Override default List findBySkillIdAndStatus(Long skillId, SkillVersionStatus status) { return findBySkillIdAndStatusOrderByCreatedAtDesc(skillId, status); From 433f3bb20b65973c6a04e1bc1abd828be6fd830b Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:18:44 +0800 Subject: [PATCH 2/2] docs(governance): record issue 611 verification Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- docs/verification/issue-611.md | 299 ++++++++++++++++++++------------- 1 file changed, 184 insertions(+), 115 deletions(-) diff --git a/docs/verification/issue-611.md b/docs/verification/issue-611.md index 7a353c86..8f054f07 100644 --- a/docs/verification/issue-611.md +++ b/docs/verification/issue-611.md @@ -1,151 +1,220 @@ -# Issue #611 Verification Report +# Issue #611 验证报告 -- 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` +- Issue：[直接删除版本遗留子表外键并返回 500](https://github.com/iflytek/skillhub/issues/611) +- 状态：本地验证及 `big-main` 远端验证均已通过 +- 基线：`origin/main@6817d98007c7890c1a8ecb3e822272287bb37a05` +- 工作分支：`fix/issue-611-child-fks` +- 修复提交：`e7a23b434bc7be58df6e541c821853fb8a5fab24` +- 已测试的 `big-main` 合并提交：`43f6d1671949d279ab05720fcddd20a25f0c2607` -## Decision +## 一、判断结论 -### Does the bug exist? +### 1. 问题是否真实存在 -Yes. Against the baseline commit, a real PostgreSQL 16 instance and the real HTTP endpoint reproduced -the failure three times: +真实存在，而且在 PostgreSQL 16 上可以稳定复现。 + +基于未修改的 `origin/main`，构造一个包含两个版本的 Skill，其中待删除版本状态为 +`REJECTED`，并存在终态 `REJECTED` 审核任务。连续三次调用： ```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. +三次均返回 HTTP 500。PostgreSQL 报告 SQL state `23503`： +`review_task_skill_version_id_fkey` 仍引用待删除的 `skill_version`。事务回滚后，版本和审核任务 +都还在。作为对照，先删除审核任务再调用同一接口，结果变为 HTTP 200。 -As a control, deleting only the review task before retrying the same request changed the result to -HTTP 200 and the version was deleted. +### 2. 是否值得修复 -### Is it worth fixing? +值得修复，理由如下： -Yes: +- 普通用户的版本被拒绝后会自然产生终态审核任务，因此不是异常脏数据。 +- 接口明确允许删除 `REJECTED` 版本，但当前实现必然失败。 +- 数据库约束异常被暴露成 HTTP 500，而不是可理解的业务结果。 +- 删除失败还会阻止用户复用相同版本号。 +- 修复可以复用现有的 `ReviewTaskRepository.deleteBySkillVersionIdIn`，改动范围可控。 -- 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. +### 3. 是否需要更大的扩展 -### Is a broader change appropriate? +不需要引入通用子表清理框架或数据库级级联迁移，但需要补充同一 Skill 的版本聚合锁。 -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`. +- `review_task`：必须在删除版本前显式删除该版本的全部审核任务。 +- `skill_tag`：正常业务只会引用 `PUBLISHED` 版本，而直接删除接口拒绝该状态。 +- `promotion_request`：正常业务只会引用 `PUBLISHED` 来源版本，同样由状态前置条件保护。 +- `security_audit`：按设计软删除并保留；V38 已移除其版本外键。 +- `skill_file`：原逻辑已经显式删除。 +- `skill_version_stats`：数据库使用 `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 +- 两个请求同时删除同一版本时，可能在审核任务的 `@Version` 字段上产生并发异常。 +- 两个请求分别删除仅剩的两个版本时，可能同时绕过“至少保留一个版本”约束。 -| 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 | +因此，应用服务在选择目标版本前，按版本 ID 顺序对同一 Skill 的全部版本执行 +`FOR UPDATE`。这是有 PostgreSQL 实测依据的必要扩展，不是猜测性功能。 -## 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 | +1. `SkillGovernanceService.deleteVersion` 在删除 `skill_version` 前删除该版本的全部 + `review_task`。 +2. `SkillLifecycleAppService.deleteVersion` 先锁定同一 Skill 的所有版本，再查找目标版本。 +3. 锁查询使用按 ID 排序的原生 `FOR UPDATE`： + - PostgreSQL 能获得稳定的悲观锁顺序。 + - 避免 Hibernate PostgreSQL dialect 生成的 `FOR NO KEY UPDATE` 与 H2 PostgreSQL + compatibility mode 不兼容。 +4. 新增领域服务测试、应用服务锁定测试和持久化 HTTP 集成测试。 +5. 保持原接口路径、成功响应结构及业务错误结构不变。 -## Test Report +## 三、操作记录 -### Local environment +| 时间（UTC+8） | 操作 | 结果 | +|---|---|---| +| 2026-07-30 14:41–14:47 | 在基线代码上使用隔离的 PostgreSQL 16、Redis 7 和 18081 端口复现 | 三次 HTTP 500，确认外键 `23503`；删除审核任务后对照请求为 HTTP 200 | +| 2026-07-30 14:55–15:02 | 拉取 `main`/`big-main`、创建修复分支、检查所有 `skill_version` 外键及生命周期规则 | 确定清理范围，并识别并发删除边界 | +| 2026-07-30 15:02–16:06 | 实现修复并执行单元、集成、PostgreSQL、前端和 staging 验证 | 功能、数据完整性和并发场景通过 | +| 2026-07-30 16:06 | 执行后端回归 | 排除已知污染类后 659 项通过、1 项跳过；被排除类单独 9/9 通过 | +| 2026-07-30 16:08 | 删除本地测试容器、网络、镜像、数据和 staging 临时覆盖 | 清理完成 | +| 2026-07-30 16:16 | 提交并推送工作分支 | 修复提交 `e7a23b43` | +| 2026-07-30 16:17 | 合入最新 `origin/big-main` 并普通推送 | 合并提交 `43f6d1671949d279ab05720fcddd20a25f0c2607` | +| 2026-07-30 16:18 | 从该 `big-main` worktree 执行 Java 21 干净构建 | 8 个 Maven 模块构建成功 | +| 2026-07-30 16:18–16:38 | 构建首张测试镜像并执行远端矩阵 | 功能矩阵 56/56 通过，但复核时发现 OCI revision 的完整 SHA 后缀记录错误，因此不作为最终证据 | +| 2026-07-30 16:44–16:49 | 使用真实 full SHA 重建 R2 镜像、传输到远端、创建全新隔离栈并重新执行全部场景 | 最终 56/56 通过 | +| 2026-07-30 16:49 | 固化 R2 镜像、脚本、报告哈希及健康状态 | 镜像 revision 与实际 `big-main` commit 完全一致 | +| 2026-07-30 16:50 | 删除 R2 测试容器、网络、卷、镜像、脚本和日志 | 28081 端口关闭；原 8080 服务仍为 `UP` | -- 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`) +远端传输期间，SFTP 曾停在 0 字节。确认测试尚未开始、数据库中没有测试数据后，仅终止了持有 +测试脚本文件的残留 `sftp-server`/`dd` 进程，改用 SSH 命令通道传输，并在执行前校验 +SHA-256。没有停止或修改远端既有 SkillHub 服务。 -#### 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. +- 基线：`6817d98007c7890c1a8ecb3e822272287bb37a05` +- Java：Eclipse Temurin 21 +- 数据库：PostgreSQL 16 Alpine +- Redis：Redis 7 Alpine +- 隔离 API：`127.0.0.1:18081` -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. +| 检查项 | 结果 | +|---|---| +| `SkillGovernanceServiceTest` | 15/15 通过 | +| 应用生命周期、Controller、持久化定向测试 | 11/11 通过 | +| `SkillVersionDeleteFlowIntegrationTest` | 通过 | +| 前端 TypeScript typecheck | 通过 | +| 前端 ESLint | 通过 | +| 前端 Vitest | 181 个文件、618 项测试全部通过 | +| staging smoke | 15/15 通过 | +| 排除 `ApiTokenAuthenticationFilterTest` 的后端 reactor | 659 项通过、1 项跳过；所有依赖模块成功 | +| `ApiTokenAuthenticationFilterTest` 单独执行 | 9/9 通过 | -Both overrides were outside the repository and were deleted after the 15/15 smoke pass. +### 本地 PostgreSQL 行为验证 -#### Real PostgreSQL behavior +- 基线代码：删除带终态审核任务的 `REJECTED` 版本，三次均为 HTTP 500 / SQL state 23503。 +- 修复后：HTTP 200，目标版本及其审核任务删除成功。 +- 同版本并发：一个 HTTP 200、一个 HTTP 400 `Version not found`，没有 500。 +- 不同版本并发：一个 HTTP 200、一个 HTTP 400 + `Cannot delete the last remaining version`，最终恰好保留一个版本。 +- 应用日志中没有新增 FK、乐观锁或 stale-state 异常。 -- 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 +1. 默认后端聚合测试存在已有的 `SecurityContext` 污染： + `ApiTokenAuthenticationFilterTest.shouldIgnoreNonBearerAuthorizationHeader` 会读取其他测试 + 遗留的上下文。本次没有修改 auth；该类单独 9/9 通过，其余测试在同一 reactor 中全部通过。 +2. 本机 5432 已被无关的 `postgres-local` 占用，因此 staging 使用临时端口覆盖，没有停止该容器。 +3. staging web 基线未提供 `SKILLHUB_TRUST_FORWARDED_PROTO`，临时设置为 `false` 后 smoke + 15/15 通过。临时覆盖已删除。 -Pending integration merge, unique image build, isolated deployment, and full remote matrix. +## 五、`big-main` 远端测试报告 -### 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. +- 测试机：`47.239.226.166` +- Docker：`29.4.0` +- Docker Compose：`v5.1.1` +- PostgreSQL：`16.14` +- Redis：`redis:7-alpine` +- 已测试提交：`43f6d1671949d279ab05720fcddd20a25f0c2607` +- 最终镜像：`skillhub-server:issue-611-big-main-43f6d167-r2` +- 最终镜像 ID： + `sha256:a71fa5d0aad0e0d6ecc65e33c29ec6b844d7fecdeec0c83e9e82e27b388f3899` +- OCI revision：`43f6d1671949d279ab05720fcddd20a25f0c2607` +- 构建 JAR SHA-256： + `165f9d5a522ef4ec8911c1d7896306895ecad4cba3ed2ef633a8883982ff5df7` +- 测试脚本 SHA-256： + `455cc712375aa299d3d04bce0c61eafaaf09355faefa303b67ca72e5036fcd76` +- 最终原始测试输出 SHA-256： + `801f3301acc1a7f12e0a57301791705c9c5a320eae45931b9fcf71c28a617153` +- 隔离 API：`127.0.0.1:28081` + +测试部署使用独立容器名、网络、PostgreSQL 数据卷、对象存储卷和端口，不替换远端既有运行环境。 + +### 测试矩阵 + +| 类别 | 场景 | 预期 | 结果 | +|---|---|---|---| +| 核心回归 | 删除带终态审核任务的非最后 `REJECTED` 版本 | HTTP 200，版本和其审核任务删除 | 通过 | +| 正常路径 | 删除非最后 `DRAFT`、`UPLOADED`、`SCAN_FAILED` 版本 | HTTP 200，保留其他版本 | 通过 | +| 多子记录 | 同一版本存在 `REJECTED`、`APPROVED` 两条历史任务 | 目标版本的审核任务全部删除 | 通过 | +| 跨版本隔离 | 另一版本有独立审核任务 | 另一版本及任务保持不变 | 通过 | +| 最后版本 | 删除仅剩的 `REJECTED` 版本 | 业务 400，所有数据和存储保持不变 | 通过 | +| 状态保护 | 删除被 tag、promotion、latestVersion 引用的 `PUBLISHED` 版本 | 业务 400，引用保持不变 | 通过 | +| 状态保护 | 删除 `YANKED` 版本 | 业务 400，版本不变 | 通过 | +| 权限/可见性 | 非所有者删除其他用户的版本 | owner-scoped 解析返回确定性业务 400，零变更 | 通过 | +| 审计保留 | 待删版本存在 `security_audit` | 版本删除，审计软删除后保留 | 通过 | +| 文件完整性 | 待删版本有 `skill_file`、文件对象和 bundle | 提交后数据库和对象存储均清理 | 通过 | +| 统计数据 | 待删版本有 `skill_version_stats` | 目标统计级联删除，其他版本统计保留 | 通过 | +| 版本复用 | 删除后重新创建相同版本号 | 唯一键释放，可重新创建 | 通过 | +| 重复请求 | 成功删除后再次删除 | 确定性业务 400，保留版本不变 | 通过 | +| 同版本并发 | 两个请求同时删除同一版本 | HTTP 200 + 400，无 500、无孤儿数据 | 通过 | +| 不同版本并发 | 两个请求分别删除仅剩的两个版本 | HTTP 200 + 400，最终恰好保留一个版本 | 通过 | +| 回滚 | 前置条件或权限检查拒绝删除 | 数据、存储、审核任务不变，不写删除审计 | 通过 | +| 响应兼容 | 校验成功响应字段 | `code=0`、`action=DELETE_VERSION` 等字段不变 | 通过 | +| 服务稳定性 | 完整矩阵后检查健康和异常日志 | 服务 `UP`，无相关异常 | 通过 | + +最终测试脚本种植了 10 个隔离 Skill、19 个版本，共执行 56 个断言： + +```text +summary_pass=56 summary_fail=0 +``` + +六次成功删除恰好产生六条 `DELETE_SKILL_VERSION` 审计记录。测试结束时没有 +`review_task` 孤儿记录。应用日志未出现以下异常： + +```text +ConstraintViolationException +DataIntegrityViolationException +ObjectOptimisticLockingFailureException +StaleObjectStateException +StaleStateException +``` + +非所有者场景最初按 HTTP 403 设计，但真实接口会先按当前用户范围解析坐标，因此返回 HTTP 400： + +```text +Skill not found: issue-611-permission +``` + +确认版本和审核任务均未改变后，将验收要求修正为“确定性业务 4xx 且零变更”，并使用全新数据库 +重跑完整矩阵。 + +### 清理与回滚 + +- 删除测试 API、PostgreSQL、Redis 三个容器。 +- 删除测试网络、PostgreSQL 卷、对象存储卷。 +- 删除测试镜像、临时测试脚本和原始日志。 +- 确认 `127.0.0.1:28081` 不再监听。 +- 确认远端原有 `127.0.0.1:8080` 服务仍返回 `{"status":"UP"}`。 + +## 六、剩余风险 + +- 聚合锁使用 PostgreSQL/H2 可执行的原生 SQL；未来若增加其他数据库实现，需要提供等价的稳定 + 悲观锁语义。 +- 默认后端聚合测试仍可能被仓库已有的 auth `SecurityContext` 污染影响，与本修复无关。 +- 远端 API 使用 local mock-auth profile 来稳定验证 owner/non-owner 行为；本改动不涉及 OAuth， + 因此没有重复验证 OAuth 登录。