diff --git a/docs/Knowledge与Retrieval-Core问题与修复复盘.md b/docs/Knowledge与Retrieval-Core问题与修复复盘.md new file mode 100644 index 0000000..928b85c --- /dev/null +++ b/docs/Knowledge与Retrieval-Core问题与修复复盘.md @@ -0,0 +1,502 @@ +# Knowledge Core 与 Retrieval Core:PR 审阅问题与修复复盘 + +> 本文记录 `feat/knowledge-retrieval-core` 合并前后的两轮代码审阅、问题复现、修复过程与工程经验。 +> 它既是团队内部的问题档案,也可作为后续技术文档、课程报告和博客文章的素材底稿。 + +## 1. 背景 + +Knowledge Core 与 Retrieval Core 建立了项目第一条完整的本地知识检索链路: + +```text +Markdown Vault +→ Markdown Block 解析 +→ SQLite 元数据 / FTS5 / sqlite-vec +→ FTS 与向量双路召回 +→ RRF 融合与轻量 Reranker +→ Metadata Filter +→ Citation 与 Snippet +``` + +功能分支完成后进行了两轮审阅。第一轮主要检查路径安全、索引一致性、PATCH 语义、检索分页和重建行为;第二轮重点验证第一轮修复是否真正覆盖事务失败和大数据边界。 + +审阅不是只看现有测试是否通过,而是主动构造失败条件,例如: + +- 让 embedding、向量删除或索引元信息写入主动抛出异常; +- 创建同目录、同标题的重复笔记; +- 使用 `../`、Windows 盘符等恶意 folder; +- 制造超过候选池以及超过 1000 条的 FTS 命中; +- 比较 `blocks` 与 `vec_blocks` 的 ID 集合; +- 在接口报错后重新读取 Markdown、SQLite 和向量表,检查是否发生部分提交。 + +## 2. 审阅与修复时间线 + +| 阶段 | 提交 | 内容 | +| --- | --- | --- | +| 功能分支首轮修复 | `8771745` | 路径逃逸、更新部分提交、失效向量和基础分页 | +| 功能分支二次修复 | `6cf531f` | 跨表事务、Metadata Filter、PATCH tags、rebuild 回滚 | +| 合并提交 | `9348225` | 将 Knowledge/Retrieval Core 合并到 `main` | +| 合并后第一批修复 | `af8ccb0` | `index_meta` 事务一致性、重复创建冲突 | +| 合并后第二批修复 | `32f249c` | 删除一致性、FTS 精确过滤与分页 | + +最终回归测试结果: + +```text +43 passed +``` + +## 3. 问题总览 + +| 编号 | 问题 | 主要风险 | 最终处理 | +| --- | --- | --- | --- | +| P-01 | folder 可逃逸 Vault | 任意位置写入或删除 Markdown | 路径标准化,并校验 resolve 后仍位于 Vault | +| P-02 | VectorStore 的 upsert 实际为 INSERT | 正常 PATCH 主键冲突 | delete-then-insert,实现幂等 upsert | +| P-03 | 元数据与向量分开提交 | 接口失败但数据库已部分改变 | 共用连接和 SQLite 事务 | +| P-04 | 更新正文后遗留旧向量 | Top-K 被无效向量占据 | 比较新旧 Block ID,事务内清理失效向量 | +| P-05 | PATCH tags 语义错误 | 省略字段却清空标签,空数组无法清空 | 严格区分 `None` 与 `[]` | +| P-06 | 固定候选池破坏过滤与分页 | 合法结果漏召回,total 不准确 | 先扩充候选,最终改为 FTS SQL 侧过滤和分页 | +| P-07 | rebuild 忽略 scope/note_ids 且先清空 | 请求语义错误,失败留下半成品 | 拒绝未支持范围,扫描在前,备份与失败恢复 | +| P-08 | `index_meta` 在主事务外写入 | 文件回滚但索引已更新 | 将 `index_meta` 纳入同一事务 | +| P-09 | POST 同路径静默覆盖 | 原笔记无提示丢失 | 数据库预检 + 文件排他创建 + 409 | +| P-10 | 删除由三个独立操作组成 | notes、向量和文件互相不一致 | tombstone + SQLite 同事务 + 失败恢复 | + +## 4. P-01:folder 路径逃逸 + +### 4.1 问题原因 + +最初的笔记路径由下面的逻辑生成: + +```python +folder_part = folder.strip().strip("/") +rel_path = f"{folder_part}/{safe_title}.md" +absolute_path = vault / rel_path +``` + +代码只去除了 folder 两端的斜杠,没有处理: + +- `..` 和 `.` 路径段; +- Windows 反斜杠; +- `C:\...` 等盘符路径; +- NUL 字节; +- 符号链接或规范化后越过 Vault 的路径。 + +因此 `folder="../outside"` 会使最终路径落到 Vault 之外。 + +### 4.2 后果 + +攻击者或错误的前端输入可以在 Vault 外创建 Markdown。数据库会保存这个越界路径,后续读取和删除接口还会继续访问该文件,风险从“越界写入”扩大为“越界读取和删除”。 + +### 4.3 解决思路 + +路径安全不能只依赖字符串替换,需要同时完成两层校验: + +1. 输入层拒绝明显非法的路径片段; +2. 文件系统层对最终路径执行 `resolve()`,确认它仍属于 Vault。 + +### 4.4 最终方案 + +`note_service.py` 新增 `_normalize_folder()` 和安全的 `_abs_path()`: + +- 同时按 `/` 和 `\` 拆分目录; +- 拒绝 `.`、`..`、盘符和 NUL; +- 对 Vault 与目标执行 `resolve()`; +- 使用 `Path.is_relative_to()` 验证目标未逃逸; +- 所有读、写、删操作统一经过 `_abs_path()`。 + +回归测试覆盖 `../../outside`、`..\..\etc`、`C:\Windows` 和 `a/../b` 等输入。 + +## 5. P-02 与 P-03:伪 upsert 和索引部分提交 + +### 5.1 问题原因 + +Block ID 根据 `note_id + heading_path + content` 稳定生成。只修改标题或标签时,正文 Block ID 不会改变。 + +最初 `SqliteVecStore.upsert()` 名为 upsert,实际执行的却是普通 INSERT: + +```sql +INSERT INTO vec_blocks (block_id, embedding) VALUES (?, ?) +``` + +同一个 Block 再次写入时会触发 sqlite-vec 主键冲突。同时,元数据和 FTS 已经在另一个事务中提交,向量失败无法回滚前面的修改。 + +### 5.2 后果 + +一次仅修改标题的 PATCH 就可能出现: + +```text +HTTP:500 +Markdown:已写入 +notes / blocks / FTS:已更新 +vec_blocks:写入失败 +``` + +调用方看到的是“更新失败”,系统内部却已经改变。这类部分提交比直接失败更危险,因为客户端重试可能继续扩大不一致。 + +### 5.3 解决思路 + +- upsert 必须具备幂等性; +- notes、blocks、FTS 与 vec_blocks 都在同一个 SQLite 文件内,应共用连接和事务; +- Repository 和 VectorStore 既要支持独立调用,也要支持加入调用方事务。 + +### 5.4 最终方案 + +VectorStore 的 upsert 改为: + +```text +DELETE existing block_id +→ INSERT new embedding +``` + +`repository.replace_note_metadata()`、`vector_store.upsert()` 和 `vector_store.delete()` 都可接收外部连接。`index_note()` 打开一个连接和事务,把元数据、FTS、失效向量删除、新向量插入放在同一提交边界中。 + +## 6. P-04:失效向量残留 + +### 6.1 问题原因 + +更新正文后,Repository 会删除旧 Block 并插入新 Block,但早期实现只写入新向量,没有删除内容已经不存在的旧向量。vec0 没有到 blocks 表的外键约束,因此这些向量不会自动级联删除。 + +### 6.2 后果 + +实测一次全文更新后出现: + +```text +blocks = 1 +vec_blocks = 2 +``` + +旧向量仍会参加 KNN 排名,但随后无法取得 Block 上下文。反复编辑后,失效向量可能占满 Top-K,使真正有效的结果无法进入候选集。 + +### 6.3 解决思路与方案 + +更新前取得旧 `block_id`,解析后得到新 `block_id`,计算集合差: + +```text +stale = old_ids - new_ids +missing = new_ids - old_ids +``` + +- 删除 `stale` 对应向量; +- 只为 `missing` 写入新向量; +- 未变化 Block 沿用原向量; +- 整个过程与 Block 替换使用同一事务。 + +测试以集合不变量验证修复: + +```text +set(vec_blocks.block_id) == set(blocks.block_id) +``` + +## 7. P-05:PATCH tags 语义错误 + +### 7.1 问题原因 + +解析器曾使用真值判断: + +```python +resolved_tags = list(tags) if tags else parse_frontmatter_tags() +``` + +但 PATCH 中的 `None` 和 `[]` 含义不同: + +- `None`:客户端没有提交 tags,应保留原值; +- `[]`:客户端明确要求清空标签。 + +真值判断把两种状态混为一谈。另外,更新服务直接把 `None` 传给解析器,导致原有 API 标签被 frontmatter 或空数组替换。 + +### 7.2 后果 + +- 仅修改标题也会意外清空标签; +- 显式提交空数组时,反而可能重新读取 frontmatter 标签,无法清空。 + +### 7.3 最终方案 + +解析器改为判断 `tags is not None`。更新服务根据 PATCH 语义计算有效标签: + +```python +effective_tags = record.tags if tags is None else tags +``` + +回归测试分别覆盖省略、替换和清空三种情况。 + +## 8. P-06:候选池、Metadata Filter 与分页 + +### 8.1 第一阶段问题 + +初版 FTS 和向量召回都固定只取 50 条候选,然后才执行 Metadata Filter 和分页: + +```text +Top 50 → Metadata Filter → offset/limit +``` + +这会产生两个错误: + +1. 第 51 条以后的页面永远无法访问; +2. 满足 folder/tag 条件的结果如果排在第 51 条以后,会被错误地过滤掉。 + +实测 60 个匹配 Block 请求 `offset=50` 时,响应为 `total=50, items=[]`。 + +### 8.2 中间修复为什么仍不完整 + +第一轮修复根据 `offset + limit` 扩大候选池,并在有过滤条件时 overscan;FTS 模式则把上限扩大到 1000。 + +这解决了常见的小数据场景,但本质只是把硬边界从 50 移到了 1000。第二轮实测 1010 个匹配 Block,请求 `offset=1000` 时仍得到: + +```json +{"total": 1000, "items": []} +``` + +### 8.3 最终解决思路 + +全文检索分页不应先把所有结果加载到 Python 再切片,应由数据库完成: + +```text +FTS MATCH ++ Metadata WHERE +→ COUNT(*) 得到精确 total +→ ORDER BY bm25 +→ LIMIT / OFFSET +``` + +### 8.4 最终方案 + +Repository 新增 FTS 精确分页查询: + +- JOIN `blocks` 与 `notes`; +- folder 和 note_id 使用 SQL `IN`; +- tags 使用 `json_each()`; +- 时间范围使用 `julianday()` 比较; +- 相同过滤条件分别用于 COUNT 和数据页查询; +- `RetrievalEngine` 为纯 FTS 模式使用专用查询路径。 + +最终测试确认 1010 个匹配 Block 在 `offset=1000, limit=10` 时返回: + +```text +total = 1010 +items = 10 +``` + +向量与 Hybrid 模式仍属于 Top-K 近邻召回,其 `total` 表示候选集数量,不等价于全文数据库的全局命中数。这是检索模型本身的语义差异,需要在接口文档和前端展示中保持明确。 + +## 9. P-07:rebuild 请求语义与失败恢复 + +### 9.1 问题原因 + +接口模型公开了 `scope=all/notes/vectors` 和 `note_ids`,但最初所有请求都会无条件清空全部索引并全量扫描 Vault。旧索引在扫描和 embedding 之前就被删除,中途失败会留下空库或半成品。 + +### 9.2 后果 + +- 调用方请求只重建向量,实际却清空 notes 和 FTS; +- 指定 note_ids 仍触发全量重建; +- 单个损坏文件或 embedding 异常即可破坏原本可用的索引。 + +### 9.3 最终方案 + +当前 MVP 明确只支持全量重建: + +- 非 `scope="all"` 或非空 `note_ids` 返回 `400 UNSUPPORTED_SCOPE`; +- 清理旧索引前先把 Vault 文件全部读取到内存,读取失败不影响旧索引; +- 重建前备份 SQLite 文件; +- 中途失败时恢复备份,并记录 failed Job; +- 成功或失败后清理临时备份。 + +`force` 当前仍是预留字段,没有行为差异。后续实现增量重建时,应一起明确它的契约,避免再次出现“参数被接受但没有效果”。 + +## 10. P-08:`index_meta` 位于主事务之外 + +### 10.1 问题如何被复审发现 + +第一轮修复已经把 notes、blocks、FTS 和向量放进同一事务,看起来解决了原子性问题。但 `index_meta` 仍在该事务提交后,通过新连接单独写入。 + +通过故障注入让 `set_index_meta()` 抛出异常后发现: + +```text +主索引事务:已经提交新内容 +index_meta:失败 +update_note:捕获异常,恢复旧 Markdown +最终结果:Markdown 是旧内容,索引是新内容 +``` + +这说明“把主要写操作放进事务”还不够,只要事务外仍有能够决定接口成功或失败的步骤,部分提交依然存在。 + +### 10.2 最终方案 + +`repository.set_index_meta()` 增加可选连接参数: + +- 独立调用时自行开连接、开事务并提交; +- 收到外部连接时加入已有事务,不自行提交和关闭。 + +`index_note()` 在主事务退出前写入 `index_meta`。现在任何一步失败都会回滚全部 SQLite 修改,外层再恢复 Markdown,从而保持文件与索引一致。 + +## 11. P-09:POST 同路径静默覆盖 + +### 11.1 问题原因 + +笔记的相对路径和 note_id 都由 `folder + title` 确定。创建接口写文件前没有检查同路径资源是否存在,Repository 又使用 `ON CONFLICT DO UPDATE`,因此第二次 POST 会被当作更新执行。 + +### 11.2 后果 + +连续创建同目录、同标题笔记时: + +```text +第一次:original +第二次:replacement +note_id:相同 +最终正文:replacement +``` + +原笔记没有任何冲突提示就被覆盖。更严重的是,早期失败回滚逻辑会在第二次索引失败时删除目标文件,连第一篇原文也一并丢失。 + +### 11.3 解决思路与方案 + +创建和更新必须具有不同语义: + +- POST 只创建新资源,冲突时返回 409; +- PATCH 才允许修改已有资源。 + +最终实现包含双重保护: + +1. 根据稳定 note_id 查询 SQLite,存在则返回 `409 RESOURCE_CONFLICT`; +2. 使用 `Path.open("x")` 排他创建文件,处理数据库检查与文件写入之间的并发竞争。 + +只有本次请求确实创建了新文件,后续索引失败时才会删除它,因此不会误删旧文件。 + +## 12. P-10:删除操作部分提交 + +### 12.1 问题原因 + +早期删除流程是三个独立步骤: + +```text +提交 notes / blocks / FTS 删除 +→ 单独提交向量删除 +→ 删除 Markdown +``` + +如果第二步向量删除失败,第一步无法回滚,第三步不会执行。 + +### 12.2 后果 + +故障注入后的状态为: + +```text +notes:已删除 +blocks / FTS:已删除 +vec_blocks:仍存在 +Markdown:仍存在 +HTTP:失败 +``` + +系统无法通过普通读取接口找到笔记,但文件和孤立向量仍在,调用方也无法判断是否应该重试。 + +### 12.3 解决思路 + +SQLite 内的四类数据可以使用同一事务;Markdown 文件不能加入 SQLite 事务,因此需要补偿事务。直接先删文件不利于恢复,先提交数据库又会在文件删除失败时不一致,所以采用可恢复的 tombstone: + +```text +Markdown 原子 rename 为非 .md tombstone +→ 同一 SQLite 事务删除 metadata / blocks / FTS / vectors +→ 成功:删除 tombstone +→ 失败:rollback SQLite,并把 tombstone rename 回原路径 +``` + +### 12.4 最终方案 + +- `repository.delete_note()` 和 `vector_store.delete()` 都支持外部连接; +- `note_service.delete_note()` 负责建立统一事务; +- 原文件先移动为同目录下带随机 UUID 的 `.deleting` 文件; +- SQLite 失败时恢复原文件; +- SQLite 成功后清理 tombstone; +- tombstone 不以 `.md` 结尾,因此即使最终清理因系统原因失败,也不会被 Vault 扫描器重新索引。 + +回归测试通过让向量删除主动失败,确认 notes、Block、向量和 Markdown 全部恢复到删除前状态。 + +## 13. 测试策略 + +这次审阅新增的测试不只验证成功路径,还验证跨层不变量和失败恢复。 + +### 13.1 路径安全 + +- `test_create_note_rejects_path_traversal` + +### 13.2 写入与回滚 + +- `test_update_note_rolls_back_file_on_index_error` +- `test_update_note_rolls_back_index_when_index_meta_fails` +- `test_create_note_rejects_existing_path_without_overwrite` +- `test_delete_note_rolls_back_when_vector_delete_fails` + +### 13.3 向量一致性 + +- `test_update_removes_stale_vectors` +- `test_patch_partial_content_no_orphan_vectors` + +### 13.4 PATCH 契约 + +- `test_patch_tags_semantics` + +### 13.5 检索过滤与分页 + +- `test_search_pagination_total_reflects_all_matches` +- `test_fts_pagination_is_not_truncated_at_one_thousand` +- `test_fts_metadata_filter_recalls_beyond_candidate_pool` + +### 13.6 rebuild + +- `test_rebuild_rejects_unsupported_scope_and_note_ids` +- `test_rebuild_failure_restores_old_index` + +测试运行方式: + +```powershell +cd backend +uv run pytest -q +``` + +## 14. 可复用的工程经验 + +### 14.1 函数名不能替代语义验证 + +一个函数叫 `upsert`,不代表它真的具备幂等更新能力。审阅时应继续检查底层 SQL、唯一键行为和重复调用结果。 + +### 14.2 原子性要覆盖完整的成功判定路径 + +只把主要数据表放进事务还不够。日志、索引版本、元信息等后续步骤如果仍会让接口失败,也必须加入事务,或者被设计为不影响主操作结果。 + +### 14.3 文件系统与数据库需要补偿事务 + +SQLite 可以回滚,文件系统通常不能参与数据库事务。跨两种存储介质时,可采用临时文件、原子 rename、tombstone 和失败恢复构造可补偿流程。 + +### 14.4 PATCH 必须区分“未提供”和“显式为空” + +`None`、空列表、空字符串和缺失字段经常具有不同业务含义。使用简单真值判断容易破坏部分更新语义。 + +### 14.5 固定扩大候选池不是分页方案 + +把上限从 50 调到 1000 只能推迟问题。只要 API 允许更大的 offset,硬截断就会再次暴露。能由数据库完成的过滤、计数和分页,应尽量下推到数据库。 + +### 14.6 回归测试应验证失败后的状态 + +只断言“抛出了异常”无法证明回滚正确。异常后还应重新读取每个存储层,检查文件、元数据、FTS 和向量是否满足不变量。 + +## 15. 后续可以继续改进的方向 + +- 为 rebuild 使用临时数据库或 SQLite Backup API,成功后原子切换,替代直接复制数据库文件; +- 明确并实现 `force`、`scope` 和 `note_ids` 的完整语义; +- 为 tombstone 增加启动时清理或恢复机制; +- 对并发创建、更新、删除增加集成测试; +- 明确 FTS 的精确 total 与 Vector/Hybrid 候选 total 在 API 层的差异; +- 建立更大规模的检索 Benchmark,持续观察召回率、延迟和索引体积; +- 在服务层引入结构化日志,记录回滚、tombstone 清理失败和 rebuild 恢复结果。 + +## 16. 相关文件 + +```text +backend/app/services/note_service.py Note CRUD、文件补偿与索引编排 +backend/app/repository.py SQLite 元数据、FTS 查询与事务接入 +backend/app/retrieval/vectorstore.py sqlite-vec 幂等写入和删除 +backend/app/retrieval/engine.py FTS/Vector/Hybrid 检索编排 +backend/app/services/index_service.py 全量重建与失败恢复 +backend/app/knowledge/parser.py Block ID 与 tags 解析语义 +backend/tests/test_retrieval.py 审阅回归测试 +docs/Knowledge与Retrieval-Core开发说明.md 模块开发说明 +``` +