Conversation
Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
|
Exact-head final CI attribution for
No failure names the changed cleanup test or reproduces the Windows access-denied race. The workflow therefore proves the target fix while remaining red on inherited main baselines. No merge action was taken. |
huangruiteng
left a comment
There was a problem hiding this comment.
动机
未发现阻塞问题。这个改动针对 test_runtime_drains_admitted_write_before_exit 的清理竞争:locator 消失不意味着操作系统已完成进程退出,旧 finally 会马上对仍被探测到的 PID 发 SIGTERM。作者报告 Windows PermissionError;我没有独立复现该 Windows 故障,也没有用远端 CI 来替代本地验证。
改动思路
保留既有真实 runtime/journal 验证,只在清理阶段先给进程自然退出的机会。使用既有 child reaper 和 PID probe,最多等待约 3 秒;超时仍走原 SIGTERM fallback。没有改变 production runtime、accepted-write、idle/shutdown 或断言语义。
具体改动
关键代码讲解
test_runtime_drains_admitted_write_before_exit的finally(985–992 行):先 release 测试锁,再用 monotonic deadline + 25 ms probe 等待自然退出;仍存活才 kill。try 中关于写入不得丢弃、locator 不得提前消失和最终 journal 精确内容的断言完全保留。effect_runtime._reap_exited_runtime_child:复用现有 helper;POSIX 的 nonblocking waitpid 只回收已退出的被管理子进程,不杀死活进程;Windows 路径是 no-op。effect_runtime._pid_is_alive:仍是既有退出探测,不把 locator 文件当作进程退出证明。
独立验证:在 immutable base 29151dfc81d81f6d590a8d6f83f3918146c63235 与 exact head 556a5ee24e21f488f860fabcf67a180d780780c3,运行 uv run --extra test python -m pytest -q tests/control_plane/test_effect_runtime_integration.py,两端均 62 passed(Python 3.13.13 / Node 24.21.0)。包含 idle/shutdown × connected/timed-out socket 的 4 个真实 Node admission/drain 场景;锁阻塞时工作仍服务,解锁后 journal durable exact-once,随后 locator 退休。另做 actual cleanup AST 的 controlled-clock characterization:already exited、delayed natural exit、POSIX zombie、persistent live 四类,head 全部符合独立 oracle;base 在 delayed exit/zombie 两类仍发 kill,违反先自然退出的预期。永久存活的 head 仍在约 3.025 秒调用一次 fallback。这个 controlled probe 不是 Windows 原生验证,且不向真实外部 PID 发信号。
Ruff 和 diff whitespace 通过;没有查询、轮询或等待远端 CI。当前测试仍是 workflow 使用的真实集成入口,没有新增一次性 smoke 或削弱 qualification。
对主干的风险
增加的是失败/退出清理的有界等待,不是生产请求 timeout 或调度退避。主测试失败仍传播,超时 fallback 保留,不能保证 deadline 后最后一次 probe 与 kill 之间完全无竞争。macOS 的 base 本来就通过,因此本次并不声称修复所有 Windows 权限/进程竞态;Windows 原始故障是最强的未独立验证项。
前端/Lark/CLI 产品交互和持久化契约无改动,不需要 companion UI。审视相邻重构后认为不需要额外 helper/TS 移动:6 行复用现有 reaper,单一测试清理边界已经局部、易审阅且可回滚。扫描既有相邻清理、最新主干以及作者近期相关 PR,未发现相同修复已经交付或一次性测试批量重复;没有贡献限制依据。
我的整体评价
批准这个测试维护增量:它减少清理时的强杀竞争,同时保留真实持久化和退出断言。批准不是 Windows 故障完全闭环、不是 remote CI 或 merge readiness 结论,也不执行合并。批准发布后按 capability 再读 blocking reviews;只有问题已逐项验证解决且有 owner/仓库权限时才撤销旧评审。
English verdict: APPROVE - The bounded cleanup wait reuses the managed-child reaper, preserves all durable-write assertions and the timeout fallback, and passes the real Node integration suite on both base and head. The original Windows race was not independently reproduced.
Summary
Why
test_runtime_drains_admitted_write_before_exitcurrently removes the lock, waits for the runtime locator to disappear, then immediately sendsSIGTERMif the PID still probes live. On Windows, locator retirement can precede the final process exit by a short interval, andos.killraces that exit withPermissionError: [WinError 5] Access is denied.The same baseline failure reproduced on the main workflow and on Windows reruns for #5375 and #5389. This change only stabilizes test cleanup; it does not change runtime behavior or weaken the leak fallback.
Validation
../loopx/.venv/bin/python -m pytest tests/control_plane/test_effect_runtime_integration.py::test_runtime_drains_admitted_write_before_exit -q(4 passed)../loopx/.venv/bin/python -m ruff check tests/control_plane/test_effect_runtime_integration.pygit diff --check origin/main...HEADCI evidence
36914904063: Windows cleanup failure attests/control_plane/test_effect_runtime_integration.py:98636914253858, attempt 2: same failure36914240641, attempt 2: same failure