mirror of
https://github.com/Tencent/WeKnora.git
synced 2026-09-21 05:43:43 +08:00
fix(knowledge): stop async delete from leaving visible/zombie rows (#2192)
Two backend fixes for the v0.7.0 regression where a deleted document keeps
showing in the list ('delete success' toast) and a follow-up reparse then
reports the file is missing:
1. List filtering: applyKnowledgeListFilter now hides rows in the transient
'deleting' state from the default document list. The async delete pipeline
marks a row 'deleting' before tearing down its resources, so without this
the entry lingers as a normal doc until it is physically gone. Rows whose
delete task exhausts retries are flipped to 'failed' by the dead-letter
callback and stay visible so the failure remains actionable; an explicit
parse_status=deleting filter can still surface in-flight rows.
2. Delete ordering: DeleteKnowledge / DeleteKnowledgeList now delete the DB
row FIRST and defer physical file (+ extracted image) cleanup until after
the row is gone. Previously file removal ran concurrently with the
index/chunk/graph cleanup; when one of those failed the row was kept but
the file could already be deleted, producing a 'file missing but row
present' zombie that could neither be reparsed nor cleanly re-deleted.
Orphaning a file after the row is gone is the tolerable failure mode.
Adds a repository regression test covering the default/explicit deleting
visibility rules.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user