mirror of
https://github.com/iflytek/skillhub.git
synced 2026-08-28 11:25:00 +00:00
fix(search): make index writes REQUIRES_NEW to survive async caller-runs fallback
The @Async rebuildSkill fix relied on a fresh thread giving a clean transaction boundary. But skillhubEventExecutor uses CallerRunsPolicy: under saturation the rejected task runs on the caller (request) thread, back inside the afterCommit synchronization phase — the original failure context where the @Transactional index write is silently dropped. Mark SearchIndexService.index as REQUIRES_NEW so it always suspends any lingering post-commit synchronization and commits in its own transaction, independent of whether the async dispatch actually happened. Add regression tests: detach removes the label keyword, and a synchronous rebuild inside the afterCommit phase still persists the document (fails without REQUIRES_NEW). Signed-off-by: shychee <shychee96@gmail.com>
This commit is contained in:
parent
74bad000e3
commit
de033da537
2 changed files with 134 additions and 1 deletions
|
|
@ -17,6 +17,7 @@ import com.iflytek.skillhub.domain.skill.SkillVisibility;
|
|||
import com.iflytek.skillhub.infra.jpa.SkillSearchDocumentEntity;
|
||||
import com.iflytek.skillhub.infra.jpa.SkillSearchDocumentJpaRepository;
|
||||
import com.iflytek.skillhub.search.SearchEmbeddingService;
|
||||
import com.iflytek.skillhub.search.SearchRebuildService;
|
||||
import java.time.Duration;
|
||||
import java.time.Instant;
|
||||
import java.util.List;
|
||||
|
|
@ -30,6 +31,9 @@ import org.springframework.boot.test.context.SpringBootTest;
|
|||
import org.springframework.boot.test.mock.mockito.MockBean;
|
||||
import org.springframework.context.annotation.Import;
|
||||
import org.springframework.test.context.ActiveProfiles;
|
||||
import org.springframework.transaction.support.TransactionSynchronization;
|
||||
import org.springframework.transaction.support.TransactionSynchronizationManager;
|
||||
import org.springframework.transaction.support.TransactionTemplate;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.mockito.ArgumentMatchers.anyString;
|
||||
|
|
@ -69,6 +73,12 @@ class LabelSearchSyncIntegrationTest {
|
|||
@Autowired
|
||||
private SkillSearchDocumentJpaRepository skillSearchDocumentJpaRepository;
|
||||
|
||||
@Autowired
|
||||
private SearchRebuildService searchRebuildService;
|
||||
|
||||
@Autowired
|
||||
private TransactionTemplate transactionTemplate;
|
||||
|
||||
@MockBean
|
||||
private SearchEmbeddingService searchEmbeddingService;
|
||||
|
||||
|
|
@ -124,6 +134,65 @@ class LabelSearchSyncIntegrationTest {
|
|||
.contains(labelDisplayName);
|
||||
}
|
||||
|
||||
@Test
|
||||
void detachingLabel_removesKeywordFromSearchIndex() throws Exception {
|
||||
Fixture f = createFixture();
|
||||
|
||||
skillLabelAppService.attachLabel(
|
||||
f.namespaceSlug, f.skillSlug, f.labelSlug, f.ownerId, f.ownerRoles, auditContext());
|
||||
SkillSearchDocumentEntity afterAttach = awaitIndexedDocument(f.skillId);
|
||||
assertThat(afterAttach.getKeywords())
|
||||
.as("precondition: label keyword indexed after attach")
|
||||
.contains(f.labelDisplayName);
|
||||
|
||||
// Act: detach the same label.
|
||||
skillLabelAppService.detachLabel(
|
||||
f.namespaceSlug, f.skillSlug, f.labelSlug, f.ownerId, f.ownerRoles, auditContext());
|
||||
|
||||
// Assert: the rebuilt document must no longer contain the label keyword.
|
||||
awaitKeywordAbsent(f.skillId, f.labelDisplayName);
|
||||
}
|
||||
|
||||
/**
|
||||
* Guards against the {@code CallerRunsPolicy} regression: when the executor is
|
||||
* saturated, {@code rebuildSkill} runs synchronously on the request thread inside
|
||||
* the {@code afterCommit} phase — the exact context where the index write used to be
|
||||
* dropped. This exercises that path directly (no async hop) and asserts the document
|
||||
* is still persisted, proving the fix relies on {@code REQUIRES_NEW}, not on the
|
||||
* executor having spare capacity.
|
||||
*/
|
||||
@Test
|
||||
void syncRebuildInAfterCommitPhase_persistsIndex() throws Exception {
|
||||
Fixture f = createFixture();
|
||||
|
||||
// Establish the skill-label association and a baseline index via the normal path.
|
||||
skillLabelAppService.attachLabel(
|
||||
f.namespaceSlug, f.skillSlug, f.labelSlug, f.ownerId, f.ownerRoles, auditContext());
|
||||
awaitIndexedDocument(f.skillId);
|
||||
|
||||
// Clear the index so we can observe the synchronous rebuild in isolation.
|
||||
transactionTemplate.executeWithoutResult(
|
||||
status -> skillSearchDocumentJpaRepository.deleteBySkillId(f.skillId));
|
||||
assertThat(skillSearchDocumentJpaRepository.findBySkillId(f.skillId)).isEmpty();
|
||||
|
||||
// Rebuild synchronously on the caller thread, inside a post-commit synchronization
|
||||
// (mirrors the CallerRuns fallback from afterCommit(() -> rebuildSkill(...))).
|
||||
transactionTemplate.executeWithoutResult(status ->
|
||||
TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
|
||||
@Override
|
||||
public void afterCommit() {
|
||||
searchRebuildService.rebuildBySkill(f.skillId);
|
||||
}
|
||||
}));
|
||||
|
||||
SkillSearchDocumentEntity indexed = skillSearchDocumentJpaRepository.findBySkillId(f.skillId)
|
||||
.orElseThrow(() -> new AssertionError(
|
||||
"synchronous rebuild in afterCommit phase must persist the index document"));
|
||||
assertThat(indexed.getKeywords())
|
||||
.as("label keyword must be indexed even on the synchronous caller-runs path")
|
||||
.contains(f.labelDisplayName);
|
||||
}
|
||||
|
||||
private SkillSearchDocumentEntity awaitIndexedDocument(Long skillId) throws InterruptedException {
|
||||
Instant deadline = Instant.now().plus(Duration.ofSeconds(15));
|
||||
Optional<SkillSearchDocumentEntity> indexed = skillSearchDocumentJpaRepository.findBySkillId(skillId);
|
||||
|
|
@ -134,4 +203,67 @@ class LabelSearchSyncIntegrationTest {
|
|||
return indexed.orElseThrow(
|
||||
() -> new AssertionError("Expected search document for skill " + skillId));
|
||||
}
|
||||
|
||||
private void awaitKeywordAbsent(Long skillId, String keyword) throws InterruptedException {
|
||||
Instant deadline = Instant.now().plus(Duration.ofSeconds(15));
|
||||
while (Instant.now().isBefore(deadline)) {
|
||||
Optional<SkillSearchDocumentEntity> indexed =
|
||||
skillSearchDocumentJpaRepository.findBySkillId(skillId);
|
||||
if (indexed.isPresent() && !indexed.get().getKeywords().contains(keyword)) {
|
||||
return;
|
||||
}
|
||||
Thread.sleep(100L);
|
||||
}
|
||||
String keywords = skillSearchDocumentJpaRepository.findBySkillId(skillId)
|
||||
.map(SkillSearchDocumentEntity::getKeywords)
|
||||
.orElse("<no document>");
|
||||
throw new AssertionError(
|
||||
"Expected keyword '" + keyword + "' to be removed from index for skill "
|
||||
+ skillId + " but keywords were: " + keywords);
|
||||
}
|
||||
|
||||
private AuditRequestContext auditContext() {
|
||||
return new AuditRequestContext("127.0.0.1", "junit");
|
||||
}
|
||||
|
||||
private Fixture createFixture() {
|
||||
String suffix = UUID.randomUUID().toString().substring(0, 8);
|
||||
String ownerId = "owner-" + suffix;
|
||||
// ASCII display name so the tokenizer keeps it as a single searchable token.
|
||||
String labelDisplayName = "MachineLearning" + suffix;
|
||||
String labelSlug = "ml-" + suffix;
|
||||
|
||||
Namespace namespace = new Namespace("ns-" + suffix, "NS " + suffix, ownerId);
|
||||
namespace.setType(NamespaceType.GLOBAL);
|
||||
namespace = namespaceRepository.save(namespace);
|
||||
|
||||
Skill skill = new Skill(namespace.getId(), "skill-" + suffix, ownerId, SkillVisibility.PUBLIC);
|
||||
skill.setDisplayName("Skill " + suffix);
|
||||
skill.setSummary("A skill used to reproduce the label search sync bug.");
|
||||
skill.setCreatedBy(ownerId);
|
||||
skill.setUpdatedBy(ownerId);
|
||||
skill = skillRepository.save(skill);
|
||||
skillRepository.flush();
|
||||
|
||||
LabelDefinition label = labelDefinitionRepository.save(
|
||||
new LabelDefinition(labelSlug, LabelType.RECOMMENDED, true, 0, ownerId));
|
||||
labelTranslationRepository.saveAll(List.of(
|
||||
new LabelTranslation(label.getId(), "en", labelDisplayName)));
|
||||
labelTranslationRepository.flush();
|
||||
|
||||
return new Fixture(
|
||||
namespace.getSlug(), skill.getSlug(), skill.getId(),
|
||||
labelSlug, labelDisplayName, ownerId,
|
||||
Map.of(namespace.getId(), NamespaceRole.OWNER));
|
||||
}
|
||||
|
||||
private record Fixture(
|
||||
String namespaceSlug,
|
||||
String skillSlug,
|
||||
Long skillId,
|
||||
String labelSlug,
|
||||
String labelDisplayName,
|
||||
String ownerId,
|
||||
Map<Long, NamespaceRole> ownerRoles) {
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ import com.iflytek.skillhub.search.SearchEmbeddingService;
|
|||
import com.iflytek.skillhub.search.SearchIndexService;
|
||||
import com.iflytek.skillhub.search.SkillSearchDocument;
|
||||
import org.springframework.stereotype.Service;
|
||||
import org.springframework.transaction.annotation.Propagation;
|
||||
import org.springframework.transaction.annotation.Transactional;
|
||||
|
||||
import java.util.List;
|
||||
|
|
@ -32,7 +33,7 @@ public class PostgresFullTextIndexService implements SearchIndexService {
|
|||
}
|
||||
|
||||
@Override
|
||||
@Transactional
|
||||
@Transactional(propagation = Propagation.REQUIRES_NEW)
|
||||
public void index(SkillSearchDocument document) {
|
||||
SkillSearchDocument normalizedDocument = normalize(document);
|
||||
Optional<SkillSearchDocumentEntity> existing = repository.findBySkillId(document.skillId());
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue