fix(knowledge): bubble embedding model resolution failure for retry - #2620
fix(knowledge): bubble embedding model resolution failure for retry#2620mdrkrg wants to merge 7 commits into
Conversation
processChunks silently returned when GetEmbeddingModel failed, leaving the knowledge row stuck in "processing" with no retry or error message. It now bubbles the error so asynq retries transient outages; only the final attempt marks the row failed.
- Unify the repeated mark-failed and mark-on-last-retry blocks - failKnowledge delegates to the helpers
markKnowledgeFailed now re-checks the row and skips the failed write when it has been cancelled or is being deleted while the failing call was in flight, so a late failure can't overwrite a newer user action.
- sync passage path (processDocumentFromPassage) marks failed immediately - async triggerManualProcessing goroutine marks failed itself - ProcessManualUpdate returns the error for retry and marks failed on the final attempt - markKnowledgeFailed leaves a user-cancelled row alone
jiahao6635
left a comment
jiahao6635
left a comment
There was a problem hiding this comment.
The main retry/final-state direction looks good, but the async manual-reparse path still has a context-lifetime hole.
triggerManualProcessing correctly detaches background work into newCtx, and processChunks(newCtx, ...) runs with it. On failure, however, the goroutine calls markKnowledgeFailed(ctx, ...) with the original request context. Once the move request returns or is cancelled, markKnowledgeFailed first calls isKnowledgeAborted through that cancelled context. A repository read can then return context canceled; isKnowledgeAborted deliberately treats any read error as deleting, so the terminal failed write is skipped and the row can remain stuck in processing/pending—the same class of state this PR is trying to remove.
I reproduced this with a repository fake that returns ctx.Err() from GetKnowledgeByID: the async regression times out on this head. Changing the call to s.markKnowledgeFailed(newCtx, knowledge, err.Error()) makes it pass. Please use the detached context for the whole goroutine failure path and add a cancelled-parent-context regression test.
Other validation I ran on this head: the focused embedding-failure tests, repeated service tests, the service package, and go vet passed; this is the only blocking finding from my review.
Pins the failure write surviving a cancelled parent request context.
The failure write used the cancelled request context, which made isKnowledgeAborted skip it and left the row stuck in processing.
Port Tencent#2620 latest HEAD into stability Wave 2. Includes the detached-context follow-up and cancelled-parent regression so async manual processing cannot remain stuck in processing after an embedding model resolution failure.
Port Tencent#2540, Tencent#2620, Tencent#2593, and Tencent#2124 after current-main review and fork-side validation.
2 participants
Description
processChunks在获取 embedding 模型失败时直接静默返回，ProcessDocument因此将任务视为成功，knowledge 停留在processing状态修复将错误向上传递：
processDocumentFromPassage、triggerManualProcessing的 goroutine），在出错时立即标记失败提取
markKnowledgeFailed/failKnowledgeRetryable两个公共 helper，统一散落的失败标记逻辑Type of Change
Related Issue
Testing
processingfailed并写入错误信息，不再卡在processingprocessingfailed并写入错误信息failed并写入错误信息failed并写入错误信息Checklist
git diff --check origin/main...HEADpassesgolangci-lint run --new-from-rev=origin/main ./...)docs/, Swagger annotations, etc.)Screenshots / Recordings