diff --git a/internal/application/repository/knowledge.go b/internal/application/repository/knowledge.go index a43022f74..40d70cd5c 100644 --- a/internal/application/repository/knowledge.go +++ b/internal/application/repository/knowledge.go @@ -136,6 +136,13 @@ func applyKnowledgeListFilter(query *gorm.DB, filter types.KnowledgeListFilter) } if filter.ParseStatus != "" { query = query.Where("parse_status = ?", filter.ParseStatus) + } else { + // Hide rows that are mid-deletion so an async delete never lingers in the + // document list as if it were a normal entry (issue #2192). The delete + // pipeline marks the row `deleting` before tearing down its resources; a + // row whose delete task exhausts its retries is flipped to `failed` by the + // dead-letter callback and stays visible so the failure remains actionable. + query = query.Where("parse_status <> ?", types.ParseStatusDeleting) } if !filter.UpdatedFrom.IsZero() { query = query.Where("updated_at >= ?", filter.UpdatedFrom) diff --git a/internal/application/repository/knowledge_list_filter_test.go b/internal/application/repository/knowledge_list_filter_test.go new file mode 100644 index 000000000..ad970c47e --- /dev/null +++ b/internal/application/repository/knowledge_list_filter_test.go @@ -0,0 +1,69 @@ +package repository + +import ( + "context" + "testing" + + "github.com/Tencent/WeKnora/internal/types" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" +) + +// insertKnowledgeInKB seeds a knowledge row in the given KB with the given +// parse_status so we can assert the list filter's visibility rules. +func insertKnowledgeInKB(t *testing.T, db *gorm.DB, tenantID uint64, kbID, status string) string { + t.Helper() + id := uuid.New().String() + require.NoError(t, db.Exec(` + INSERT INTO knowledges (id, tenant_id, knowledge_base_id, type, title, source, parse_status) + VALUES (?, ?, ?, 'file', 'list-filter-test', 'manual', ?) + `, id, tenantID, kbID, status).Error) + return id +} + +// TestListPaged_ExcludesDeletingByDefault documents the fix for issue #2192: +// a knowledge row that is mid-deletion (parse_status = 'deleting') must not +// appear in the default document list, otherwise a successful async delete +// keeps showing the entry until the row is physically gone. Rows in other +// states (including the terminal 'failed' a stuck delete resolves to) stay +// visible, and an explicit parse_status=deleting filter can still surface them. +func TestListPaged_ExcludesDeletingByDefault(t *testing.T) { + db := setupKnowledgeTestDB(t) + repo := NewKnowledgeRepository(db).(*knowledgeRepository) + ctx := context.Background() + + const tenantID = uint64(1) + kbID := uuid.New().String() + + completedID := insertKnowledgeInKB(t, db, tenantID, kbID, "completed") + failedID := insertKnowledgeInKB(t, db, tenantID, kbID, "failed") + deletingID := insertKnowledgeInKB(t, db, tenantID, kbID, types.ParseStatusDeleting) + + page := &types.Pagination{Page: 1, PageSize: 100} + + // Default listing (no explicit parse_status): deleting rows are hidden. + rows, total, err := repo.ListPagedKnowledgeByKnowledgeBaseID( + ctx, tenantID, kbID, page, types.KnowledgeListFilter{}, + ) + require.NoError(t, err) + ids := make([]string, 0, len(rows)) + for _, r := range rows { + ids = append(ids, r.ID) + } + assert.ElementsMatch(t, []string{completedID, failedID}, ids, + "default list must include completed + failed but exclude deleting") + assert.Equal(t, int64(2), total, "count must match the filtered rows") + + // Explicit parse_status=deleting still surfaces the in-flight row so + // operators / tooling can inspect stuck deletes. + rows, total, err = repo.ListPagedKnowledgeByKnowledgeBaseID( + ctx, tenantID, kbID, page, + types.KnowledgeListFilter{ParseStatus: types.ParseStatusDeleting}, + ) + require.NoError(t, err) + require.Len(t, rows, 1) + assert.Equal(t, deletingID, rows[0].ID) + assert.Equal(t, int64(1), total) +} diff --git a/internal/application/service/knowledge_delete.go b/internal/application/service/knowledge_delete.go index 5d3d98371..4616117f3 100644 --- a/internal/application/service/knowledge_delete.go +++ b/internal/application/service/knowledge_delete.go @@ -156,22 +156,6 @@ func (s *knowledgeService) DeleteKnowledge(ctx context.Context, id string) error return nil }) - // Delete the physical file and extracted images if they exist - wg.Go(func() error { - if knowledge.FilePath != "" { - if err := kbFileSvc.DeleteFile(ctx, knowledge.FilePath); err != nil { - logger.GetLogger(ctx).WithField("error", err).Errorf("DeleteKnowledge delete file failed") - } - } - deleteExtractedImages(ctx, kbFileSvc, imageURLs) - tenantInfo := ctx.Value(types.TenantInfoContextKey).(*types.Tenant) - tenantInfo.StorageUsed -= knowledge.StorageSize - if err := s.tenantRepo.AdjustStorageUsed(ctx, tenantInfo.ID, -knowledge.StorageSize); err != nil { - logger.GetLogger(ctx).WithField("error", err).Errorf("DeleteKnowledge update tenant storage used failed") - } - return nil - }) - // Delete the knowledge graph wg.Go(func() error { namespace := types.NameSpace{KnowledgeBase: knowledge.KnowledgeBaseID, Knowledge: knowledge.ID} @@ -188,8 +172,32 @@ func (s *knowledgeService) DeleteKnowledge(ctx context.Context, id string) error if err := s.repo.DeleteKnowledgeTagRelations(ctx, id); err != nil { logger.Warnf(ctx, "Failed to delete tag relations for knowledge %s: %v", id, err) } - // Delete the knowledge entry itself from the database - return s.repo.DeleteKnowledge(ctx, ctx.Value(types.TenantIDContextKey).(uint64), id) + // Delete the knowledge row FIRST, then drop its physical file. Physical + // cleanup is deliberately deferred until the row is gone: if any of the + // index/chunk/graph cleanups above failed we already returned early with the + // row (and its file) intact, so the queued retry — or a user-triggered + // reparse — can still read the original file. Deleting the file before the + // row could leave a "file missing but row present" zombie that can neither be + // reparsed nor cleanly re-deleted (issue #2192). Orphaning a file after the + // row is gone is the tolerable failure mode instead. + if err := s.repo.DeleteKnowledge(ctx, tenantID, id); err != nil { + return err + } + + // Best-effort physical cleanup. Errors here only leak storage; they must not + // fail the delete now that the row is already gone. + if knowledge.FilePath != "" { + if err := kbFileSvc.DeleteFile(ctx, knowledge.FilePath); err != nil { + logger.GetLogger(ctx).WithField("error", err).Errorf("DeleteKnowledge delete file failed") + } + } + deleteExtractedImages(ctx, kbFileSvc, imageURLs) + tenantInfo := ctx.Value(types.TenantInfoContextKey).(*types.Tenant) + tenantInfo.StorageUsed -= knowledge.StorageSize + if err := s.tenantRepo.AdjustStorageUsed(ctx, tenantInfo.ID, -knowledge.StorageSize); err != nil { + logger.GetLogger(ctx).WithField("error", err).Errorf("DeleteKnowledge update tenant storage used failed") + } + return nil } // cleanupWikiOnKnowledgeDelete handles wiki pages when a source document is deleted. @@ -557,34 +565,6 @@ func (s *knowledgeService) DeleteKnowledgeList(ctx context.Context, ids []string return nil }) - // 5. Delete the physical file and extracted images if they exist - wg.Go(func() error { - storageAdjust := int64(0) - for _, knowledge := range knowledgeList { - if knowledge.FilePath != "" { - fSvc := kbFileServices[knowledge.KnowledgeBaseID] - if err := fSvc.DeleteFile(ctx, knowledge.FilePath); err != nil { - logger.GetLogger(ctx).WithField("error", err).Errorf("DeleteKnowledge delete file failed") - } - } - storageAdjust -= knowledge.StorageSize - } - // Delete extracted images per KB - for kbID, urls := range kbImageURLs { - fSvc := kbFileServices[kbID] - if fSvc == nil { - logger.Warnf(ctx, "No file service for KB %s, skipping %d image deletions", kbID, len(urls)) - continue - } - deleteExtractedImages(ctx, fSvc, urls) - } - tenantInfo.StorageUsed += storageAdjust - if err := s.tenantRepo.AdjustStorageUsed(ctx, tenantInfo.ID, storageAdjust); err != nil { - logger.GetLogger(ctx).WithField("error", err).Errorf("DeleteKnowledge update tenant storage used failed") - } - return nil - }) - // Delete the knowledge graph wg.Go(func() error { namespaces := []types.NameSpace{} @@ -609,8 +589,39 @@ func (s *knowledgeService) DeleteKnowledgeList(ctx context.Context, ids []string logger.Warnf(ctx, "Failed to delete tag relations for knowledge %s: %v", knowledgeID, err) } } - // 6. Delete the knowledge entry itself from the database - return s.repo.DeleteKnowledgeList(ctx, tenantInfo.ID, ids) + // 6. Delete the knowledge rows FIRST, then drop their physical files. See + // DeleteKnowledge for the rationale: deferring file removal until the rows are + // gone avoids "file missing but row present" zombies that break reparse / + // re-delete when an earlier cleanup step failed (issue #2192). A failure below + // only orphans storage. + if err := s.repo.DeleteKnowledgeList(ctx, tenantInfo.ID, ids); err != nil { + return err + } + + storageAdjust := int64(0) + for _, knowledge := range knowledgeList { + if knowledge.FilePath != "" { + fSvc := kbFileServices[knowledge.KnowledgeBaseID] + if err := fSvc.DeleteFile(ctx, knowledge.FilePath); err != nil { + logger.GetLogger(ctx).WithField("error", err).Errorf("DeleteKnowledge delete file failed") + } + } + storageAdjust -= knowledge.StorageSize + } + // Delete extracted images per KB + for kbID, urls := range kbImageURLs { + fSvc := kbFileServices[kbID] + if fSvc == nil { + logger.Warnf(ctx, "No file service for KB %s, skipping %d image deletions", kbID, len(urls)) + continue + } + deleteExtractedImages(ctx, fSvc, urls) + } + tenantInfo.StorageUsed += storageAdjust + if err := s.tenantRepo.AdjustStorageUsed(ctx, tenantInfo.ID, storageAdjust); err != nil { + logger.GetLogger(ctx).WithField("error", err).Errorf("DeleteKnowledge update tenant storage used failed") + } + return nil } func (s *knowledgeService) cleanupKnowledgeResources(ctx context.Context, knowledge *types.Knowledge) error {