fix: prevent CNClaim Finalize stuck and scale-in race during migration

[xzxiong]

Summary

修复 CNClaim Finalize 卡死 + CNClaimSet scale-in 选中迁移中 claim 的 4 个 bug，解决 dev freetier-01 环境观察到的 claim 错乱、无限重试问题。

The PR fixes all 4 bugs from issue #591 (related to unit-agent#289):

1. Finalize stuck — now releases stale labels and returns (true, nil) to complete finalization
2. Scale-in race — excludes migrating claims (spec.SourcePod != nil) from deletion candidates
3. Lost claim stale spec — clears spec.PodName/spec.NodeName when Pod not found
4. Watch gap — watchPodChange now also triggers reconcile for CNClaims referencing pod via spec.podName

关联

• Closes CNClaim Finalize 卡死 + Scale-in 选中迁移中 claim (dev freetier-01, 日志确认) #591
• Related: matrixorigin/unit-agent#289

变更内容

Bug 1: Finalize() 卡死（主因）

问题：当所有 owned Pod 都被另一个 CNClaim 引用（迁移场景下 ensureOwnership 覆盖了 label），Finalize() 正确 skip reclaim 但返回 (false, nil) → 永远无法完成。

修复：skip 时主动清理自己残留的 claimed-by label，让 owning claim 正常管理 Pod；所有 owned CN 处理完后返回 (true, nil) 完成 finalization。

Bug 2: Scale-in 选中迁移中 claim

问题：scaleIn() 不排除 spec.SourcePod != nil 的 claim，导致迁移进行到一半时 claim 被删除，产生 Finalize 与 migration 冲突。

修复：在 scaleIn() 中过滤掉正在迁移的 claim，不作为 scale-in 候选。

Bug 3: Sync() Pod NotFound 不清理 spec

问题：Pod 不存在时只设 status.phase=Lost，但 spec.podName 未清空，claim 永远卡在 Lost 且保留过期的 Pod 引用。

修复：Pod NotFound 时同时清空 spec.PodName 和 spec.NodeName。

Bug 4: watchPodChange 关联不完整

问题：Pod 删除事件仅通过 claimed-by label 关联 CNClaim，如果 label 已在迁移/reclaim 中被清除，则 CNClaim 不会被 reconcile。

修复：watchPodChange 使用 manager client 额外检索 spec.podName 匹配的 CNClaim，确保 Pod 删除时所有相关 claim 都被触发 reconcile。

测试

• Test_scaleIn_skipsMigratingClaims: 验证迁移中 claim 不被选为 scale-in 候选
• Test_containsRequest: 验证 reconcile request 去重辅助函数
• 现有 Test_sortClaimsToDelete 和 Test_buildPodClaimIndex 继续通过

Checklist

• 代码编译通过 (go build ./pkg/controllers/cnclaim/ ./pkg/controllers/cnclaimset/)
• go vet 通过
• 单元测试通过 (go test ./pkg/controllers/cnclaimset/)
• cnclaim 测试编译通过（链接阶段因环境缺 -lmo 库无法本地执行，CI 环境可正常运行）

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[xzxiong]

Code Review: fix: prevent CNClaim Finalize stuck and scale-in race during migration

• PR: #592 fix: prevent CNClaim Finalize stuck and scale-in race during migration
• 分支: worktree-claims → main | 变更: +148 / -22，5 个文件
• 作者: xzxiong | 关联 Issue: CNClaim Finalize 卡死 + Scale-in 选中迁移中 claim (dev freetier-01, 日志确认) #591
• CI: checks ✅ | e2e ✅

---

〇、总结（TL;DR）

修复 CNClaim 生命周期中 4 个相互关联的 bug：Finalize 卡死、scale-in 与 migration 竞态、Pod NotFound 残留、watch 事件丢失。每个 fix 独立且最小化。

问题统计：🔴 0 | 🟡 2 | 🟢 2

PR 描述质量：优秀 — 四维结构完整（背景/原因/方案/结果），含事件链还原、代码级定位和修复建议。

合并建议：✅ 建议合并

---

一、PR 描述评审

维度	评价	说明
背景（Context）	✅ 充分	关联 Issue #591 提供了完整的环境信息、日志时间线、事件链还原
原因（Why）	✅ 充分	4 个 bug 逐一定位到具体代码行，含根因分析和竞态条件描述
方案（How）	✅ 充分	每个 fix 有修复前/后代码对比，方案清晰
结果（Expected Outcome）	✅ 充分	Checklist 明确验收标准，CI 已通过

---

二、方案评审

2.0 变更可视化

Finalize 状态机变更：

修改前：
  Finalize() → ListOwnedCNs → [有owned]
    → buildClaimIndex
    → for each cn:
        if claimIndex[cn] → skip (continue)    ← 问题点
        else → reclaimCN()
    → return (false, nil)                       ← 问题点：永远不完成

修改后：
  Finalize() → ListOwnedCNs → [有owned]
    → buildClaimIndex
    → for each cn:
        if claimIndex[cn] → Patch(delete label) ← 修改：主动清理
        else → reclaimCN()
    → return (true, nil)                        ← 修改：完成 finalize

Scale-in 数据流变更：

修改前：
  oc.owned → [全部] → sortClaimsToDelete → delete(top N)

修改后：
  oc.owned → filter(SourcePod==nil) → candidates → sort → delete(top N)
             filter(SourcePod!=nil) → migrating → 保留          ← NEW

2.1 方案合理性

A. 函数调用场景

• ✅ Finalize() 的改动不影响调用方（框架调用，返回 (true, nil) 表示 finalize 完成）
• ✅ scaleIn() 内部过滤逻辑，不改变外部接口
• ✅ Sync() 在 NotFound 分支添加 Patch，不影响其他分支
• ✅ watchPodChangeFn 签名变化通过闭包封装，调用方只需传入 mgr.GetClient()

B. 使用场景覆盖

场景	覆盖	说明
Finalize 所有 Pod 被他人引用	✅	清理 label + 完成 finalize
Finalize 部分 Pod 被引用，部分需 reclaim	✅	混合处理后统一返回 true
Finalize label Patch 失败	✅	返回 (false, err)，下次重试
Scale-in 全部 claim 都在迁移	✅	count 被 clamp 到 0，不删除
Scale-in 混合状态	✅	只删非迁移的
Pod NotFound 但在 cache 延迟内	✅	等待 cache，不清空
Pod NotFound 超过 cache 延迟	✅	设 Lost + 清空 spec
Pod 删除且 label 已被移除	✅	watchPodChangeFn 通过 List 找到

C. 架构合理性

• ✅ 职责划分清晰，每个 fix 在正确的层级
• 🟡 watchPodChangeFn 的 namespace-wide List 在大规模集群下有 O(pods×claims) 性能风险，但当前规模可接受

2.2 测试方案梳理

T1. 变更 → 测试映射

变更函数/方法	变更类型	对应 UT	覆盖判定
Actor.Finalize()	修改	无直接 UT	🟡 需 reconciler context
Actor.Sync() NotFound 分支	修改	无直接 UT	🟡 需 reconciler context
Actor.scaleIn()	修改	Test_scaleIn_skipsMigratingClaims	🟡 测试过滤逻辑但非完整调用
watchPodChangeFn()	重写	无集成测试	🟡 containsRequest 有 UT
containsRequest()	新增	Test_containsRequest	✅

T8. 回归验证

• 🟡 Test_scaleIn_skipsMigratingClaims 验证了 filtering 逻辑的不变式，但通过重复 filtering 代码而非调用 scaleIn() 本身。如果 scaleIn() 内部逻辑与 filtering 不一致（如未来重构），测试不会捕获。
• ⚡ Test_containsRequest — 精确命中、本地秒级、回归即红。

T9. 故障复现效率

• 🟡 中等 — Bug 1/3/4 的精确复现需要完整的 reconciler context（含 fake client + CNClaim + Pod 状态组合），当前无法在 UT 中秒级复现。Issue CNClaim Finalize 卡死 + Scale-in 选中迁移中 claim (dev freetier-01, 日志确认) #591 中给出了复现步骤，但需 E2E 环境。

---

三、变更概述

本 PR 修复了 CNClaim controller 和 CNClaimSet controller 中 4 个相互关联的 bug。这些 bug 在 dev freetier-01 环境中导致了 CNClaim Finalize 卡死（60+ 次/秒重试）、迁移中 claim 被误删、Pod 引用残留和 watch 事件丢失。

核心变更集中在两个 controller 包：

• pkg/controllers/cnclaim/: Finalize label 清理 + Sync spec 清理 + watch 增强
• pkg/controllers/cnclaimset/: scale-in 迁移保护

所有修改仅影响 CNClaim 生命周期管理，不涉及 API 类型变更或 CRD 变更。

---

四、代码审查（逐文件）

pkg/controllers/cnclaim/controller.go

🟡 #1 — controller.go L268-275 (Sync NotFound)

问题：c.Status.Phase = CNClaimPhaseLost 在 ctx.Patch(c, ...) 之前赋值。Patch 只修改 spec 字段，status 由框架自动同步。但如果 Patch 失败返回 error，c.Status.Phase 已被修改为 Lost（内存中）。下次 reconcile 重新读取 object 会恢复，所以无功能影响，但语义上先 Patch 再修改 status 更清晰。

建议：可选优化，将 c.Status.Phase = Lost 移到 Patch 成功之后（或不改，当前实现无功能问题）。

---

🟡 #2 — controller.go L480-500 (watchPodChangeFn)

问题：每次 Pod 事件触发时都执行 cli.List(ctx, claimList, client.InNamespace(pod.Namespace)) 全量列出 namespace 下所有 CNClaim。在高频 Pod 事件 + 大量 CNClaim 的场景下（如 1000+ claims × 数百 Pod 事件/分钟），可能产生显著的 API server/cache 压力。

建议：后续 PR 考虑为 spec.podName 添加 field indexer，将 List 替换为 indexed lookup：

mgr.GetFieldIndexer().IndexField(ctx, &v1alpha1.CNClaim{}, "spec.podName", func(obj client.Object) []string {
    c := obj.(*v1alpha1.CNClaim)
    if c.Spec.PodName == "" { return nil }
    return []string{c.Spec.PodName}
})

当前规模可接受，标记为 🟡 非阻塞。

---

🟢 #3 — controller.go L364-380 (Finalize)

问题：return true, nil 在循环结束后无条件返回。这意味着即使所有 Pod 都经过了 reclaimCN()（设为 Draining 状态），我们也返回 true 表示 finalize 完成。原逻辑中 reclaimCN 后返回 (false, nil) 等待 Pod 实际被回收，新逻辑不再等待。

分析：这实际上是正确的改进。reclaimCN 已经将 Pod 设为 Draining 并移除了 claimed-by label，所以下次 ListPods 将不会匹配到这些 Pod（因为 label 已被删除）。Finalize 会在 framework 端重新触发，如果 len(ownedCNs) == 0 则直接返回 true。所以行为等价且减少了一轮 reconcile。

建议：无需修改，当前逻辑正确。

---

🟢 #4 — controller.go watchPodChangeFn error handling

问题：if err := cli.List(ctx, claimList, ...); err == nil — List 失败时静默降级为仅依赖 label 路径。没有日志记录 List 失败。

建议：可考虑添加 else { log.Error(err, "list claims failed") }，但 map 函数中缺少 logger。当前降级行为等同修改前，可接受。

---

pkg/controllers/cnclaimset/controller.go

逻辑正确，无问题：

• 迁移中 claim 被过滤出候选列表
• migrating 追加回 oc.owned 确保后续 status 统计包含这些 claim
• count 限制在非迁移 claim 数量范围内

pkg/controllers/cnclaim/controller_test.go

测试正确，覆盖 containsRequest 的 true/false/nil 三种情况。

pkg/controllers/cnclaimset/controller_test.go

测试验证了 filtering 不变式。now 变量虽声明但仅用于 CreationTimestamp，可去掉但无害。

---

五、潜在风险检查

5.1 并发安全

• ✅ 所有修改都在 reconcile 循环内执行，controller-runtime 保证同一 object 不会并发 reconcile
• ✅ watchPodChangeFn 中的 List 操作是只读的，无竞态风险
• ✅ Finalize 中的 Patch 操作使用 optimistic concurrency（resourceVersion），冲突时返回 error 触发重试

5.2 性能

• 🟡 watchPodChangeFn 的 namespace-wide List（见 #2）
• ✅ scaleIn 过滤逻辑为 O(N) 且只遍历一次
• ✅ Finalize label 清理为 O(owned pods)，通常 <= 1

5.3 复杂度分析

函数	时间复杂度	说明
Finalize loop	O(owned × claims)	owned 通常 1，claims 通常 < 100
scaleIn filter	O(owned)	单次遍历
watchPodChangeFn	O(claims_in_ns)	每次 Pod 事件触发
containsRequest	O(N)	N = requests 长度，通常 1-2

5.4 成本

• ✅ 无额外 API 调用（Patch 是必要操作）
• ✅ 无资源泄漏风险

5.5 安全

• ✅ 无用户输入处理
• ✅ 无敏感信息泄漏

5.9 违禁操作

• ✅ 无超时硬编码（使用现有常量 waitCacheTimeout、retryPatchInterval）

5.10 测试实现质量

• ✅ 断言有效，验证核心过滤逻辑
• ✅ 无 flaky 风险（纯逻辑测试，无 timing/ordering 依赖）
• 🟡 Test_scaleIn_skipsMigratingClaims 重复了 filtering 逻辑而非调用 scaleIn()——如果将来 scaleIn 实现改变但忘记更新测试，测试仍会通过。属于可接受的 tradeoff，因为完整测试需 reconciler context。