Isolate service import staging directories - #512
Conversation
|
PR Title: Isolate service import staging directories Commit: 本次变更将 internal/packageimport/importer.go 的导入流程改造为并发安全版本：
总体设计方向合理：唯一临时目录解决了并发场景下固定路径被对方 RemoveAll 的竞态，互斥锁则保护了提交阶段 replaceServiceDir（旧版本备份、rename 新版本）的原子性。已确认文件内无重入死锁：ImportRecursive 不会调用 Import，importServiceName 仅读取 store，Progress 回调是唯一的外部回调点。 主要问题：(a) 随机临时目录名使"崩溃/强杀后残留目录"失去了原有的下次运行清理机制（旧代码每次启动固定路径时先 RemoveAll），崩溃残留的 .staging-* 与 .previous-* 目录会持续累积磁盘占用；(b) 全局锁粒度偏粗，持有期间跨越用户 Progress 回调与 npm install/build 等长操作，存在重入死锁隐患，且会把不同服务的导入全部串行化。 |
![monkeyscan[bot]](https://avatars.githubusercontent.com/u/231811742?s=60&v=4){kind=small}
|
PR Title: Isolate service import staging directories Commit: 该改动对 internal/packageimport 导入器做两项修改：(1) 将原先覆盖整个 Import/ImportRecursive 的全局互斥锁 importMu 改为按 serviceID 粒度的锁表（importLocks + lockService），仅在提交阶段（commitImportedService 前后）加锁，目的是修复历史问题中“持全局锁调用 Progress 回调导致重入死锁”以及“不同服务导入被全局串行化”的缺陷；(2) 新增 sweepStaleImportDirs，在每次导入开始时清理 dataDir 下超过 24 小时的 .staging-* 与 .previous-* 临时目录，以回收崩溃残留，并补充了单元测试。总体评估：锁粒度重构方向正确，基本解决了历史两条 confirmed 问题（提交阶段不再调用回调、不同服务可并发）；sweep 能覆盖新引入的随机临时目录残留，方向合理。但新增的 sweep 与“并发导入”这一新能力之间存在未加防护的耦合：sweep 在无锁状态下扫描共享的 artifacts/services 根目录，仅凭“名称 + 目录 mtime”判断陈旧性，而 replaceServiceDir 生成的 .previous-* 备份目录会继承旧服务目录（可能远早于 24h）的 mtime，存在被并发 sweep 误删正在使用的回滚备份的竞态；同时 sweep 的任一清理错误（含并发 sweep 之间的 TOCTOU）会直接中止本可成功的导入，属可用性回归。建议将清理与提交互斥（或重置备份 mtime），并让清理失败不影响导入本身。 |
| if err := os.RemoveAll(filepath.Join(root, entry.Name())); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
无锁的 sweepStaleImportDirs 会误删并发提交中的 .previous-* 回滚备份目录
本次改动移除了全局导入锁（改为仅提交阶段持有按服务 ID 的 lockService），使不同服务的导入可并发执行；同时每次导入开始都会调用 sweepStaleImportDirs 扫描整个 artifacts/services 目录，删除名字匹配 .staging-* 或包含 .previous- 且 mtime 早于 24h 的目录。这里存在一个真实的竞态：replaceServiceDir（importer.go:1557）提交时用 MkdirTemp 生成 ..previous-* 名字后先把该临时目录 Remove 掉，再把现有 serviceDir os.Rename 成该备份名。os.Rename 不会改变目录自身的 mtime，因此这个“刚刚创建”的备份目录继承的是旧服务目录的 mtime；对于一个已稳定运行、距上次导入/更新超过一天的服务的重导入，备份目录的 mtime 天生早于 24h 截止线。此时若另一个服务的导入正在执行 sweep，就会把这个正在提交中、仍承载旧版本服务数据的备份目录判定为崩溃残留并 RemoveAll 删除。一旦随后的 Store.UpsertService 失败，commitImportedService 里的 rollback() 会执行 os.Rename(backupDir, serviceDir)，但备份已被删除导致回滚失败——磁盘上是尚未登记到存储的新版本，存储中仍是旧记录，且旧版本已无副本可恢复，造成服务目录与 DB 不一致/无法回滚。同类问题也作用于超长导入：staging 目录只在直接子项增删时刷新自身 mtime，npm install 等深层写入不会刷新顶层 staging 目录的 mtime，运行超过 24h 的导入其 staging 目录也会被并发 sweep 误删。
Problem code:
Changed code at internal/packageimport/importer.go:61-72
Recommendation:
让清理逻辑与提交过程互斥，或让年龄阈值能区分“正在使用的备份/暂存目录”与“崩溃残留”。最小改动：在 replaceServiceDir 将 serviceDir 重命名为备份目录后立即 os.Chtimes(backupDir, time.Now(), time.Now()) 重置 mtime，使提交窗口内的备份被视为新目录而不会被 sweep 命中（崩溃残留仍会在 24h 后被清理）。更稳妥的做法是为 sweep 与 replaceServiceDir 引入共享锁/活跃目录登记表，保证 sweep 不会与任何进行中的提交重叠。
Suggested diff:
--- a/internal/packageimport/importer.go
+++ b/internal/packageimport/importer.go
@@ replaceServiceDir
if err := os.Rename(serviceDir, backupDir); err != nil {
return nil, nil, err
}
+ // 备份目录继承旧 serviceDir 的 mtime，可能早于 24h 阈值；重置为当前时间以避免
+ // 并发 sweepStaleImportDirs 将进行中的回滚备份误判为陈旧残留并删除。
+ _ = os.Chtimes(backupDir, time.Now(), time.Now())
}
| } | ||
| if err := sweepStaleImportDirs(stagingBase); err != nil { | ||
| return Result{}, err | ||
| } |
There was a problem hiding this comment.
sweepStaleImportDirs 的清理错误会中止本可成功的导入（含并发 sweep 的 TOCTOU 误伤）
新引入的 sweepStaleImportDirs 属于尽力而为（best-effort）的残留清理，但调用方把它与导入本身强耦合：Import 与 ImportRecursive 中只要 sweep 返回任意错误就直接 return，使一次本可成功的导入被清理逻辑的失败所阻断。具体有两类触发路径：(1) 单个条目出错即中止——sweep 对任一目录执行 entry.Info() 或 os.RemoveAll 失败（权限不足、Windows 下目录被占用、或条目在 ReadDir 后被并发移除）都会向上返回错误并中止整个导入；(2) 并发导入间 TOCTOU——本次改动允许不同服务并发导入，多个导入会同时对本应互斥清理的同一 artifacts/services 根目录执行 sweep，A 的 sweep 删除某陈旧目录后，B 的 sweep 若已 ReadDir 到该条目，随后 entry.Info() 会得到 ENOENT 并返回错误，导致 B 的导入被误伤中止。改动前（无 sweep）这些清理失败不会影响导入可用性，因此这是本次改动引入的可用性回归。
Problem code:
Changed code at internal/packageimport/importer.go:185-187
Recommendation:
将 sweep 视为纯尽力而为的后台清理：单条目的 Info/RemoveAll 失败应记录后继续（continue）而不是中止整个 sweep；仅在根目录 ReadDir 完全失败时可选择忽略或返回，但不应让导入因此失败。更理想的做法是把清理与导入解耦（如独立的周期任务），避免每次导入的成败都依赖于清理的结果。
Suggested diff:
--- a/internal/packageimport/importer.go
+++ b/internal/packageimport/importer.go
@@ func sweepStaleImportDirs(root string) error {
info, err := entry.Info()
if err != nil {
- return err
+ // 条目可能已被并发 sweep 移除等，属尽力而为清理，跳过即可。
+ continue
}
if info.ModTime().Before(cutoff) {
if err := os.RemoveAll(filepath.Join(root, entry.Name())); err != nil {
- return err
+ continue
}
}
}
|
PR Title: Isolate service import staging directories Commit: 本次变更（internal/packageimport/importer.go）将 Importer 的 service 级导入锁从 map[string]*sync.Mutex 重构为 map[string]*serviceImportLock（内含 sync.Mutex 与引用计数 refs），目标是在最后一个持有者释放后将 map 条目删除，避免旧实现中条目永不清理导致的无限增长。 审查结论：经对 lockService 及两处调用点（Import L257-259、ImportRecursive L550-552，均为同步且恰好一次调用返回的解锁闭包）的多种并发交错推演，引用计数逻辑是正确的——每个 lockService 调用在 importMu 保护下 refs++，闭包在释放 mu 后于 importMu 保护下 refs--，计数严格配对；map 条目仅在 refs==0（无任何持有/等待者）时删除，且创建与删除均在 importMu 临界区内，不会出现同一 service 两把锁同时生效或删除时有 goroutine 仍引用旧锁的情况；解锁闭包采用“先解锁 mu、后持 importMu 递减”的顺序也不会造成死锁或提前回收。未发现该重构引入的高置信度正确性、数据完整性或并发安全缺陷。 唯一提交的发现为低严重度测试缺口：该并发原语是同一 service 提交互斥的唯一保障，而 importer_test.go（2457 行）中没有任何并发对同一 serviceID 调用 lockService 的用例来守护互斥性与条目回收行为。文件内另有两条历史已确认发现（sweepStaleImportDirs 清理与并发提交/TOCTOU 相关），不属于本 diff 引入，未重复上报。 |
|
Follow-up fixes after review:
Verification: cleanup tests pass; full packageimport CI remains the authoritative integration gate. |
| lock.mu.Unlock() | ||
| i.importMu.Lock() | ||
| lock.refs-- | ||
| if lock.refs == 0 && i.importLocks[serviceID] == lock { |
There was a problem hiding this comment.
lockService 引用计数与条目回收逻辑缺少直接并发测试
本次改动把 service 提交锁从 map[string]*sync.Mutex（条目永不清理）改为带引用计数的 serviceImportLock，并在最后一个持有者释放时从 importLocks 中删除条目。lockService 现在是防止同一 service 两个并发提交（commitImportedService）同时执行的唯一互斥原语，而新增的 refs 计数/删除逻辑非常微妙：一旦未来重构破坏 refs 配对（例如在持有锁期间提前 return 或异常路径漏调解锁闭包导致 refs 永不归零），或让删除提前发生，都可能造成同一 service 并发提交同时进入 commitImportedService（目录切换与 Store.UpsertService 交错，导致服务目录与 DB 不一致、回滚失效），或使本变更想修复的 map 无限增长问题复发。已核对 importer_test.go（2457 行）：现有测试均为顺序执行的导入/辅助函数用例，没有任何并发对同一 serviceID 调用 lockService 的测试，也未验证互斥与条目回收。该行为有实际回归风险却无测试守护，属于本变更暴露的测试缺口。
Problem code:
Changed code at internal/packageimport/importer.go:61
Recommendation:
新增一个直接针对 lockService 的单元测试：(1) 多个 goroutine 并发对同一 serviceID 调用 lockService 并在临界区内执行有状态操作，断言临界区互不重叠（同一时刻仅一个持有者）；(2) 全部解锁后断言 i.importLocks 中该 serviceID 的条目已被删除（refs 归零触发回收）；(3) 对不同 serviceID 并发加锁，断言互不阻塞、可并行持有，以验证 per-service 而非全局互斥。
1 participant
问题
普通导入和递归导入使用固定 staging 目录，并在开始时删除该目录；服务替换还使用固定的 previous 目录。
影响
并发导入会删除其他请求正在使用的文件，或互相覆盖 previous 目录，可能造成服务包、descriptor、runtime 和数据库状态不一致。
修复内容
os.MkdirTemp创建唯一 staging 目录。验证
go test ./internal/packageimport -run 'TestArchiveExtractorsRejectUnsafePaths|TestParse'通过。