diff --git a/backend/internal/indexer/vecdelete_test.go b/backend/internal/indexer/vecdelete_test.go new file mode 100644 index 0000000..e07756c --- /dev/null +++ b/backend/internal/indexer/vecdelete_test.go @@ -0,0 +1,108 @@ +package indexer + +import ( + "context" + "regexp" + "strings" + "testing" +) + +// vec0's idxStr opens with its plan: '1' fullscan, '2' point lookup. A +// `rowid IN (…)` outside KNN falls back to a fullscan of every vector. +var ( + vec0PointPlan = regexp.MustCompile(`chunks_vec VIRTUAL TABLE INDEX \d+:2`) + vec0Fullscan = regexp.MustCompile(`chunks_vec VIRTUAL TABLE INDEX \d+:1`) +) + +func vecPlan(t *testing.T, query string, args ...any) string { + t.Helper() + db := writeDB(t) + rows, err := db.Query(`EXPLAIN QUERY PLAN `+query, args...) + if err != nil { + t.Fatalf("explain: %v", err) + } + defer rows.Close() + var plan []string + for rows.Next() { + var id, parent, notused int + var detail string + if err := rows.Scan(&id, &parent, ¬used, &detail); err != nil { + t.Fatalf("scan: %v", err) + } + plan = append(plan, detail) + } + if err := rows.Err(); err != nil { + t.Fatalf("rows: %v", err) + } + return strings.Join(plan, "\n") +} + +// TestDeleteVecRows_reportsEveryFailure: a vector delete that fails silently +// leaves an orphaned vector answering questions about code that is gone. +func TestDeleteVecRows_reportsEveryFailure(t *testing.T) { + for _, tc := range []struct { + name, drop, query string + }{ + {"bad id query", "", `SELECT id FROM no_such_table`}, + {"id that is not an integer", "", `SELECT 'x'`}, + {"vector delete", "chunks_vec", `SELECT 1`}, + } { + t.Run(tc.name, func(t *testing.T) { + db := writeDB(t) + if tc.drop != "" { + if _, err := db.Exec(`DROP TABLE ` + tc.drop); err != nil { + t.Fatalf("drop: %v", err) + } + } + tx, err := db.Begin() + if err != nil { + t.Fatalf("begin: %v", err) + } + defer func() { _ = tx.Rollback() }() + if err := deleteVecRows(context.Background(), tx, tc.query); err == nil { + t.Error("deleteVecRows() err = nil, want the failure") + } + }) + } +} + +// TestWriter_failsWhenAMirrorCannotBeCleared: DeleteFile and ReplaceFile must +// abort, never commit chunks whose mirror rows were left behind. +func TestWriter_failsWhenAMirrorCannotBeCleared(t *testing.T) { + ctx := context.Background() + for _, tc := range []struct { + name, drop string + run func(*Writer) error + }{ + {"DeleteFile without chunks_fts", "chunks_fts", func(w *Writer) error { return w.DeleteFile(ctx, "shop", "src/A.java") }}, + {"DeleteFile without chunks_vec", "chunks_vec", func(w *Writer) error { return w.DeleteFile(ctx, "shop", "src/A.java") }}, + {"ReplaceFile without chunks_vec", "chunks_vec", func(w *Writer) error { + return w.ReplaceFile(ctx, "shop", "src/A.java", "def456", "java", 64, + sampleChunks(), [][]float32{vec(1), vec(2)}, nil, nil) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + db := writeDB(t) + testee := NewWriter(db) + if err := testee.ReplaceFile(ctx, "shop", "src/A.java", "abc123", "java", 64, + sampleChunks(), [][]float32{vec(1), vec(2)}, nil, nil); err != nil { + t.Fatalf("ReplaceFile() err = %v", err) + } + if _, err := db.Exec(`DROP TABLE ` + tc.drop); err != nil { + t.Fatalf("drop: %v", err) + } + if err := tc.run(testee); err == nil { + t.Error("err = nil, want the mirror failure") + } + }) + } +} + +func TestDeleteVecRow_isPointLookup(t *testing.T) { + if plan := vecPlan(t, deleteVecRow, 1); !vec0PointPlan.MatchString(plan) { + t.Fatalf("deleteVecRow must be a vec0 point lookup, plan:\n%s", plan) + } + if plan := vecPlan(t, `DELETE FROM chunks_vec WHERE rowid IN (SELECT id FROM chunks WHERE file_id = ?)`, 1); !vec0Fullscan.MatchString(plan) { + t.Fatalf("rowid IN should show the fullscan this guards against, plan:\n%s", plan) + } +} diff --git a/backend/internal/indexer/write.go b/backend/internal/indexer/write.go index 1574272..d47ed96 100644 --- a/backend/internal/indexer/write.go +++ b/backend/internal/indexer/write.go @@ -237,10 +237,17 @@ func (w *Writer) DeleteFile(ctx context.Context, repo, path string) error { // cascades chunks away, and chunks_vec and chunks_fts are NOT part of that // cascade. Letting it fire first would orphan them permanently, and an // orphaned vector keeps answering questions about deleted code. + // + // chunks_fts goes first because it is a write: the id read chunks_vec + // needs comes only once the transaction holds the lock. const owned = `SELECT id FROM chunks WHERE file_id IN (SELECT id FROM files WHERE repo = ?1 AND path = ?2)` + if _, err := tx.ExecContext(ctx, `DELETE FROM chunks_fts WHERE rowid IN (`+owned+`)`, repo, path); err != nil { + return fmt.Errorf("delete %s/%s: %w", repo, path, err) + } + if err := deleteVecRows(ctx, tx, owned, repo, path); err != nil { + return fmt.Errorf("delete %s/%s: %w", repo, path, err) + } for _, q := range []string{ - `DELETE FROM chunks_vec WHERE rowid IN (` + owned + `)`, - `DELETE FROM chunks_fts WHERE rowid IN (` + owned + `)`, `DELETE FROM chunks WHERE id IN (` + owned + `)`, `DELETE FROM symbols WHERE file_id IN (SELECT id FROM files WHERE repo = ?1 AND path = ?2)`, `DELETE FROM integration_tokens WHERE file_id IN (SELECT id FROM files WHERE repo = ?1 AND path = ?2)`, @@ -255,6 +262,41 @@ func (w *Writer) DeleteFile(ctx context.Context, repo, path string) error { return tx.Commit() } +// deleteVecRow deletes one vector. vec0 resolves `rowid = ?` as a point +// lookup; `rowid IN (…)` outside a KNN query falls back to a fullscan of every +// vector, so chunks_vec is always cleared one row at a time. +const deleteVecRow = `DELETE FROM chunks_vec WHERE rowid = ?` + +// deleteVecRows deletes the chunks_vec row of every chunk id query selects. +// The ids are read in full before the first delete. tx must already have +// written: a transaction that opens with this read holds a WAL snapshot and +// fails its first write with "database is locked" (see DeleteFile). +func deleteVecRows(ctx context.Context, tx *sql.Tx, query string, args ...any) error { + rows, err := tx.QueryContext(ctx, query, args...) + if err != nil { + return err + } + var ids []int64 + for rows.Next() { + var id int64 + if err := rows.Scan(&id); err != nil { + _ = rows.Close() + return err + } + ids = append(ids, id) + } + _ = rows.Close() + if err := rows.Err(); err != nil { + return err + } + for _, id := range ids { + if _, err := tx.ExecContext(ctx, deleteVecRow, id); err != nil { + return err + } + } + return nil +} + // upsertFile inserts or updates the files row and returns its id. func upsertFile(ctx context.Context, tx *sql.Tx, repo, path, sha, lang string, size int, skipReason string) (int64, error) { _, err := tx.ExecContext(ctx, ` @@ -274,13 +316,13 @@ func upsertFile(ctx context.Context, tx *sql.Tx, repo, path, sha, lang string, s // clearFileContent removes a file's chunks from all three tables and its // symbols, in the order the mirrors demand: gather the ids, delete the vec0 and -// fts5 rows by rowid, and only then the chunks themselves. +// fts5 rows by rowid, and only then the chunks themselves. Every caller has +// already written (upsertFile), so the id read holds no stale snapshot. func clearFileContent(ctx context.Context, tx *sql.Tx, fileID int64) error { - // The set form, like purgeContent and DeleteFile: the mirrors by the - // file's chunk ids in one statement each, never a read of the ids and - // a delete per id. + if err := deleteVecRows(ctx, tx, `SELECT id FROM chunks WHERE file_id = ?`, fileID); err != nil { + return err + } for _, q := range []string{ - `DELETE FROM chunks_vec WHERE rowid IN (SELECT id FROM chunks WHERE file_id = ?)`, `DELETE FROM chunks_fts WHERE rowid IN (SELECT id FROM chunks WHERE file_id = ?)`, `DELETE FROM chunks WHERE file_id = ?`, } {