fix: validate the managed runtime idle timeout - #4111
NIU-123370 wants to merge 1 commit into
Conversation
huangruiteng
left a comment
There was a problem hiding this comment.
动机
本评审绑定 exact head 1419307906f6b941f35deb5a3d395f102f97409a。PR 修复 LOOPX_EFFECT_RUNTIME_IDLE_MS 未校验的问题:旧的 TypeScript server 直接 Number(...) 后传给 setTimeout(),非法、零、负数或溢出值可能变成立即关闭,Python managed launcher 最终只看到误导性的 runtime_exited_before_ready。影响面是所有通过 effect_runtime_result() 冷启动 TypeScript runtime 的控制面调用者,以及直接启动 effect_runtime_server.ts 的路径。合理目标是:冷启动前给出稳定 invalid_runtime_idle_ms,不留下可用 info 文件,同时保留合法短 TTL 的请求与 idle shutdown。
改动思路
PR 在两个真实 owner 上各加一道边界:Python _validate_effect_runtime_idle_ms() 在 Popen 前负责 managed-launch 的稳定错误码;新建的 TypeScript effectRuntimeIdleMs() 在 listen/setTimeout 前保护直接 server 启动。两边都要求 ASCII 十进制整数,范围 1..2147483647,unset 仍使用 5 分钟默认值。这个跨语言分离有实际必要:只改 TS 无法让 Python 稳定区分配置错误,只改 Python 又挡不住 direct server。不过两套语法、常量和消息存在漂移成本,最好用同一个小型 parity fixture 约束,而不是再引入更大的配置框架。
完整 caller inventory 里还有一个未覆盖的 warm-reuse 分支:effect_runtime_request() 先按仅包含源码的 fingerprint 查 _read_info(),命中后直接请求现有进程;新校验只位于 _start_runtime() 的后半段。因此已有 runtime 时,当前调用者的非法或不同 idle 配置完全不会被验证或绑定。
具体改动
loopx/control_plane/effect_runtime.py:新增 21 行,包括上界常量、Python parser 和 Popen 前校验。loopx/control_plane/effect_runtime_config.ts:新增 22 行,封装默认值、上界与 direct-server parser。loopx/control_plane/effect_runtime_server.ts:用 typed parser 替换宽松Number(...)。- Python integration tests 新增 25 行,覆盖 7 个非法值并断言 Popen 不应发生;TS tests 新增 22 行,覆盖默认、合法边界和非法输入。既有
test_managed_runtime_releases_memory_after_idle_timeout已通过真实 Python→Node 路径验证 150ms ping 与 idle shutdown。
关键代码讲解
_validate_effect_runtime_idle_ms(Python 第 38 行)正确把冷启动配置错误映射到稳定invalid_runtime_idle_ms,且在 spawn 前失败。effectRuntimeIdleMs(TS 第 4 行)把 Node timer 的实际2^31-1上界显式化,避免 NaN/overflow coercion;effect_runtime_server.ts第 43 行在 listen 前调用。effect_runtime_request(Python 第 544 行)是关键遗漏:_read_info()命中就绕过_start_runtime(),而 runtime fingerprint/info 又没有记录 effective idle value。相同源码、不同环境意图因此错误地共享同一进程。
对主干的风险
[P1] warm runtime 会绕过新校验并忽略配置变化。 我用同一份真实 managed Python→TypeScript harness 在基线 04ff4204d1abb133d97dc7353d31242bb032eb97 和本 head 比较:冷态 not-a-number 已从 runtime_exited_before_ready 正确变为 invalid_runtime_idle_ms;150ms 在两边都能 ping 且随后清除 info;但是先以 1000ms 启动、再把当前环境改成 not-a-number 时,本 head 仍复用同一个 PID 并成功返回。这样“非法配置产生专用诊断”的合同只覆盖冷态,合法的 1000→150 变化也会被静默忽略。
最小修复应先于 warm reuse 校验当前环境,并把规范化后的 idle 值绑定进 runtime identity/info;如果产品选择 sticky runtime 配置,也必须显式拒绝后续冲突值,而不能无声复用。请增加真实 public path 回归:1000ms 启动后切到非法值,下一请求必须得到 invalid_runtime_idle_ms 且不能 same-PID 成功;再明确 1000→150 的预期。
[P2] 两个 parser 需要共享 parity 数据。 当前 Python/TS 测试手工复制了大部分输入集,但没有同一 fixture 的跨语言比对。建议用一个很小的 public-safe table 驱动两边,防止常量、空白、边界或错误消息日后分叉。
验证:TS config tests 8/8 通过;Python effect-runtime integration 34/34 通过;同 fixture 的 baseline/head real-runtime 对比已执行并捕获上述 warm-reuse 反例;GitHub Sign-off、build、checks、测试分片、pytest、Stage2C、Windows、Sonar、merge-gate 全部成功。npm run typecheck:control-plane 在 detached review worktree 因缺少本地 tsc 未能启动,远端 build/checks 已覆盖类型检查结果。
我的整体评价
这个修复的动机、放置和总体规模都合理,冷启动诊断与 direct Node timer 的核心改动也正确;新增 TypeScript 模块有真实调用者,不是测试脚手架,也没有扩大 authority/default-off 合同。当前 REQUEST_CHANGES 的原因是配置意图没有贯穿 runtime reuse 身份:只验证 spawn 分支会让同一个公开入口在冷态与暖态产生不同语义。补齐 P1 的 identity/reuse 合同与真实回归后再复审;P2 可作为同一小批次的耐久性加强。
English verdict: REQUEST_CHANGES for exact head 1419307906f6b941f35deb5a3d395f102f97409a. Cold invalid startup now reports invalid_runtime_idle_ms, the valid short-timeout lifecycle passes, and 42 focused tests are green. However, a ready source-matching runtime bypasses validation: changing the caller environment from 1000 to not-a-number reuses the same PID and succeeds. Validate before warm reuse and bind the normalized idle value to runtime identity/info (or explicitly reject conflicting sticky configuration), then add a real-path regression; also parity-test the duplicated Python/TypeScript parsers.
1419307 to
5f9ea65
Compare
|
Addressed the requested changes in
Validation: 36/36 Effect Runtime integration tests, 8/8 TypeScript config tests, and |
Signed-off-by: NIU-123370 <191000457+NIU-123370@users.noreply.github.com>
5f9ea65 to
5043e1b
Compare
huangruiteng
left a comment
There was a problem hiding this comment.
动机
本次复审绑定 exact head 5043e1b02ebabf096be4b4c24403c5a80f2f56ee,全量基线为 04ff4204d1abb133d97dc7353d31242bb032eb97,上一轮评审 head 为 1419307906f6b941f35deb5a3d395f102f97409a。
PR 要修复 LOOPX_EFFECT_RUNTIME_IDLE_MS 非法值在 Node timer coercion 后变成立即退出、Python 只能看到误导性 runtime_exited_before_ready 的问题。影响面包括所有 effect_runtime_result() / effect_runtime_request() managed 启动调用,以及直接启动 TypeScript Effect runtime 的路径。当前 head 已补齐上一轮指出的 warm reuse 漏洞:先校验当前环境,把规范化后的 idle_ms 写进 runtime info,并在 ready reuse、startup-lock race、readiness 中显式拒绝冲突配置。
但这次继续按“作者测试全绿时,最强语义承诺还可能在哪里为假”做反例验证后,发现两端所谓的共享 parser parity 仍不成立。
改动思路
架构放置整体合理:Python 边界负责 managed launcher 的稳定诊断和 spawn 前失败;TypeScript 边界负责 direct server 在创建 timer 前的实际数值;runtime info 是 warm reuse 的配置收据。只改其中一端都不够,共享小 fixture 也比引入通用配置框架更合适。
正路径已经形成闭环:unset 得到 300000ms;ASCII 空格包围的 250 与最大值两端都通过;cold start 记录 idle_ms;同值 warm reuse 继续使用原 PID。负路径也覆盖了非法值 spawn 前失败与有效值冲突的 runtime_idle_ms_conflict。
问题在于 fixture 只是同时喂给两个实现的一小组样例,它没有独立定义完整语法。Python 使用 str.strip(),TypeScript 使用 String.trim();这两个标准库的 Unicode 空白集合不同,因此“共用 fixture”没有真正形成相同语义的 oracle。
具体改动
全量 6 文件,+246/-10:3 个 production 文件 +110/-9,3 个测试/fixture 文件 +136/-1;没有生成文件或机械搬运。
effect_runtime.py:validator 现在返回有效整数;runtime info 强制包含合法idle_ms;新 matcher 统一拒绝 warm reuse 的配置冲突;request、startup-lock race、ready 等路径都复用该判断。effect_runtime_config.ts/effect_runtime_server.ts:direct server 通过独立 parser 获得 timer 值,保留 5 分钟默认值与 Node timer 上界。- Python integration:新增真实 warm-runtime 回归,证明非法配置和 1000→150 冲突不会再 same-PID 静默成功。
- shared fixture 与 TS tests:把常见有效/非法样例同时提供给两端,但目前只覆盖普通 ASCII 空格,没有覆盖两端 trim 集合的差异。
关键代码讲解
effect_runtime.py:39的_validate_effect_runtime_idle_ms()是 managed caller 的输入决策点;第 43 行raw.strip()同时引入了 Python 自己的 Unicode 空白定义。effect_runtime.py:59的_require_matching_runtime_idle_ms()将规范化后的整数绑定到运行中进程,正确修复上轮的 warm-reuse 身份遗漏。effect_runtime.py:568的effect_runtime_request()现在在 lookup 前校验,并让 conflict 直接返回而不删除 info 重试,失败所有权清晰。effect_runtime_config.ts:4的effectRuntimeIdleMs()是 direct server 的对应决策点;第 6 行value.trim()与 Python 看似等价,实际接受集不同。effect_runtime_server.ts:43消费 TS parser 的结果设置 idle timer,说明该差异确实位于生产入口,不是 helper-only 测试问题。
对主干的风险
[P1] 两个生产 parser 对同一环境字符串仍有不同接受集,共享 fixture 没有抓住语义漂移。 在 exact head 直接调用两端真实 parser:U+0085 NEL + 250 + NEL 和 U+001C File Separator + 250 + File Separator 被 Python 接受为 250,但被 TypeScript 拒绝;U+FEFF BOM + 250 + BOM 则被 Python 拒绝为 invalid_runtime_idle_ms,却被 TypeScript 接受为 250。于是 managed Python 启动和 direct TS 启动对相同配置可能一个成功、一个失败,违背本 PR 的核心 parity 承诺。
最小修复不是再加框架,而是明确一套语法(建议只允许 ASCII 数字,并明确是否只剥离 ASCII 空格/制表,或者干脆不允许外围空白),两端显式实现同一集合;把 NEL、BOM、File Separator、tab/newline、普通空格与纯数字写进同一 fixture。测试还应证明把任一端故意换回不同 normalizer 时 parity oracle 会失败,避免 fixture 继续只证明“当前列出的几个样例都过了”。
本地验证:41 个 focused Python tests、8 个 TS parser tests、TypeScript control-plane typecheck、8 个 maintainability ratchet tests 全部通过;上述 Unicode 反例探针稳定复现。远端 test-shard (2) 是 3465 passed / 19 skipped / 1 failed,聚合 pytest 和 merge-gate 因此失败;该单测在 exact head 与 GitHub merge ref 本地都单独通过,暂时更像偶发问题,但 required checks 仍需在修复后干净重跑,不能当作已通过。
我的整体评价
上一轮的 P1 已被正确修复:校验时机、sticky config identity、race 路径和真实 warm regression 都补齐,规模与 owner 也合适。此次 REQUEST_CHANGES 不是因为要求更多抽象,而是当前 PR 明确要保证跨语言配置解析一致,而两端在真实输入上仍有可复现的接受/拒绝差异。
请用显式 grammar + 扩充后的共享 fixture 收口,再重跑 required CI。我会按新的 exact head 全量复审;当前 head 不批准、不合并。
English verdict: REQUEST_CHANGES at exact head 5043e1b02ebabf096be4b4c24403c5a80f2f56ee. The previous warm-reuse blocker is fixed and focused validation passes, but Python str.strip() and JavaScript String.trim() accept different Unicode padding. Python accepts NEL/File Separator around 250 while TypeScript rejects them; TypeScript accepts BOM padding while Python rejects it. Define one explicit grammar in both parsers, add these cases to the shared fixture with a mutation-sensitive parity check, and rerun the currently failed required CI checks.
|
I implemented the requested cross-language grammar fix on top of the current #4111 head:
Validation: 39/39 real Python-to-Node integration tests, 11/11 TypeScript parser tests, full TypeScript typecheck, Ruff, public-boundary scan, and 9/9 standard premerge checks passed. |
Closing this: the validation scope landed on
|
| Rejected value | diagnostic_code on main |
Expected |
|---|---|---|
U+0085 + 250 + U+0085 |
runtime_exited_before_ready |
invalid_idle_timeout |
U+2028 + 250 + U+2028 |
runtime_exited_before_ready |
invalid_idle_timeout |
U+2029 + 250 + U+2029 |
runtime_exited_before_ready |
invalid_idle_timeout |
U+001C + 250 + U+001C |
invalid_idle_timeout |
already correct |
space + 250 + space |
invalid_idle_timeout |
already correct |
The split is exactly the intersection of "honoured by splitlines()" and "not escaped by JSON.stringify", which is why U+001C — also a splitlines() break — reports correctly: it is escaped to \u001c and never reaches the stream as a break.
Delivered separately. #5057 records this and #5058 fixes it: the reader frames on \n, the separator the server writes and the one the response reader in the same module already uses, with the boundary parameters above added to the existing parametrised test and the framing rule pinned at the reader itself. Two files, production change is one expression.
Closing this PR so it stops occupying review attention. Everything in the description above is reproducible from main without this branch.
Summary
LOOPX_EFFECT_RUNTIME_IDLE_MSinvalid_runtime_idle_msValidation
node --experimental-strip-types --test tests/control_plane_ts/effect_runtime_config.test.ts(8 passed)python -m pytest tests/control_plane/test_effect_runtime_integration.py(36 passed)npm run typecheck:control-planeCloses #4106