Skip to content

test(runtime): wait for natural exit before cleanup - #5442

Merged
huangruiteng merged 1 commit into
loopx-project:mainfrom
Duang777:codex/fix-effect-runtime-windows-cleanup
Oct 2, 2026
Merged

huangruiteng merged 1 commit into
loopx-project:mainfrom
Duang777:codex/fix-effect-runtime-windows-cleanup

Conversation

@Duang777

@Duang777 Duang777 commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • allow the managed Node runtime up to three seconds to finish its expected natural exit after retiring its locator
  • reap an exited direct child on POSIX so zombie state is not mistaken for a live process
  • retain the existing forced termination fallback when the process remains live past the cleanup deadline

Why

test_runtime_drains_admitted_write_before_exit currently removes the lock, waits for the runtime locator to disappear, then immediately sends SIGTERM if the PID still probes live. On Windows, locator retirement can precede the final process exit by a short interval, and os.kill races that exit with PermissionError: [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.py
  • git diff --check origin/main...HEAD

CI evidence

Signed-off-by: duanjialing.777 <duanjialing.777@bytedance.com>
@Duang777

Duang777 commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator Author

Exact-head final CI attribution for 556a5ee24e21f488f860fabcf67a180d780780c3:

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 huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

动机

未发现阻塞问题。这个改动针对 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants