fix(eval): keep leftover processes through framework timeout - #3191
fix(eval): keep leftover processes through framework timeout#31911625567290 wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWhat this PR solvesFramework timeouts now stop only the subject leader. The process group and environment remain available for verifier scoring. If the leader does not exit within the TERM/KILL deadlines, the relay fails closed. It destroys the environment and does not publish a scoreable timeout frame. Host abort behavior remains unchanged. It still quiesces and may destroy the trial. The hosted-execution client uses Source of truthThis PR extends the existing relay and hosted-execution paths. It does not create a parallel timeout path. The relay remains responsible for process signaling and timeout-frame safety. The hosted-execution client provides the explicit environment-preserving abort contract. Scope and simplicityThe solution separates framework timeout behavior from host abort behavior:
The detached state and abort-policy branches are necessary to prevent environment destruction during an admitted, environment-preserving abort. No deletion or simplification is identified that would preserve the same behavior and regression coverage. Optional cleanup findings remain non-blocking. Validation
Complexity delta
The PR increases local maintenance complexity. The added complexity is necessary to separate framework timeout from host abort and to protect scoring safety. Total maintenance complexity stays justified. Review-relevant risksThe PR changes user-visible evaluation behavior. Framework timeouts can preserve background processes and allow verifier scoring. The PR changes the public The PR changes process signaling, environment lifetime, cancellation, and trial teardown behavior. Errors could affect process isolation, scoring, resource cleanup, or cancellation results. Material changes in user-visible behavior, public contracts, process isolation, scoring, or teardown require independent human review under repository policy. The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughFramework timeouts preserve the subject environment for verification. The relay stops only the subject leader and confirms exit. Host aborts still destroy the environment. Hosted execution supports environment-preserving detachment with indeterminate status. ChangesTimeout environment preservation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The change preserves leftover subject processes and the environment after framework timeouts so timed-out trials can still be scored, while host aborts continue teardown. The remaining merge-readiness concern is bounded: a lifecycle test relies on scheduler timing and may not reliably cover repeated cancellation during timeout cleanup, so merge is reasonable with explicit owner follow-up. Sequence Diagram(s)sequenceDiagram
participant EvalRelay
participant HostedExecution
participant RuntimeHost
participant SubjectLeader
participant VerificationEnvironment
EvalRelay->>HostedExecution: start execution with preserve_environment
EvalRelay->>SubjectLeader: stop leader on framework timeout
HostedExecution->>RuntimeHost: release host and close connection
SubjectLeader-->>EvalRelay: confirm subject exit
EvalRelay->>VerificationEnvironment: run verification in preserved environment
EvalRelay->>VerificationEnvironment: destroy environment if exit is unconfirmed
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoPreserve evaluation environments across framework timeouts
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/eval/harbor/test_relay_lifecycle.py (1)
545-580: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe comment does not describe what this fixture exercises.
FrameworkTimeoutEnvironmentinherits thepgid=branch fromSimultaneousEnvironment, which returnsreturn_code=3._signal_leadertherefore returnsFalse, and the relay takes the vanished-leader branch. No signal is ever delivered, so the subject cannot "never acknowledge" one. Correct the comment, or make the fixture return0for the leader-stop command so the test covers a subject that ignores both TERM and KILL. The second option is the one that adds coverage.
🧹 Nitpick comments (1)
packages/eval/harbor/relay_agent.py (1)
537-551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe destroy-and-cancel block duplicates
_settle_or_destroy.Lines 540-550 repeat the destroy, cancel, and bounded-await sequence at lines 570-580 verbatim. Two copies of the same teardown authority drift apart the moment one deadline rule changes. Extract one helper, for example
_destroy_environment(environment, execution, deadline, loop), and call it from both places.As per path instructions: "Choose remedies in this order: delete an unnecessary path, consolidate duplicated authority, reuse the closest existing seam…".
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ea312524-cc98-492f-8579-47c13d3c358a
📒 Files selected for processing (7)
packages/eval/README.mdpackages/eval/harbor/relay_agent.pypackages/eval/harbor/test_relay_lifecycle.pypackages/eval/src/__tests__/lifecycle-boundaries.test.tspackages/eval/src/harbor-maka-subject.tspackages/runtime-host/src/__tests__/hosted-execution-client.test.tspackages/runtime-host/src/client/hosted-execution.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 471f8fe3-f0db-4b0c-8db9-364148cd9f23
📒 Files selected for processing (4)
packages/eval/harbor/relay_agent.pypackages/eval/harbor/test_relay_lifecycle.pypackages/runtime-host/src/__tests__/hosted-execution-client.test.tspackages/runtime-host/src/client/hosted-execution.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/runtime-host/src/tests/hosted-execution-client.test.ts
- packages/eval/harbor/test_relay_lifecycle.py
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
|
Thanks for the fix — the inconsistency is real (on timeout the old Conclusion: PASS with two P2s (Windows lane infra-flake + end-to-end validation gap) — no P0/P1. P2-1 — the other side of this contract lives outside the repo: end-to-end behavior is unverified against the real framework. The fix keeps leftovers alive only if the framework's own timeout mechanism doesn't independently cancel P2-2 — the failed Windows lane is an environment flake, not this PR. The failure ("Validate installed CLI Windows x64 / Node 24" → "checking the interactive TUI setup path" → P3 (optional): the AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：PASS（2 个 P2：Windows lane 环境 flake + 端到端验证缺口；无 P0/P1）。问题真实（timeout 时旧 run() 无条件 cancel recorded process group 且可删环境→leftover 在 timeout 时消失、clean exit 时存活），方案正确：_host_teardown_requested 区分 host abort（仍 settle/destroy）与 framework timeout（只停 leader）；_stop_subject_for_timeout 预算 math 正确（TERM≤20s/KILL≤10s 各 slice 被 remaining 约束、destroy 保留 min(20,0.2*timeout)，30s 预算推算成立）；_signal_leader 的 kill -- "$pgid" 在 bash 语义下正确（pgid 文件=setsid'd sh PID=组 leader，exec 后即 subject）；fail-closed 路径（leader 无视 TERM/KILL、vanished leader 且 exec 未确认）销毁环境并停止发布可计分 frame，均有测试。架构边界保持：只改 Runtime Host client（abortPolicy 新选项，默认 'cancel' 不变）与 eval relay；@maka/eval 未构造 Runtime、未碰 host 实现。实跑 relay 测试（20/20 + harbor lifecycle 20（3 skipped））并把 main 的 relay_agent.py 换回后 4 个新 timeout 测试全失败——red→green 证明测试真实；实况测试用真实进程验证 background service 在 leader TERM 后仍可 TCP 连接。P2-1：修复的另一侧在仓库外——framework 自身 timeout 机制若独立 cancel recorded process group 或销毁环境，则 end-to-end 不成立；框架是 pip 依赖（run_trial.py:98-100 安装）不在 diff 中，仓库内无法证实/证伪；实况测试用 LocalEnvironment 不覆盖真实 docker exec 的 cancel 语义。建议用真实 Harbor cell 验证（启动 background service→触发 framework timeout→验证阶段服务可达）或至少在描述中声明前提与验证方式。P2-2：Windows lane 失败是环境 flake 非本 PR 引入——同一错误签名在无关 dependabot PR（32105259238）同步骤逐字复现、本 PR 更早 commits 同 lane 通过、最后 commit 只改 eval relay Python（Windows smoke 不执行）与 hosted-execution client（不在 TUI 启动路径）。建议重跑该 lane 或按已知 infra 问题延后。P3（可选）：relay_agent.py:201-209 的 elif 分支已成死代码（_stop_subject_for_timeout 不可能返回 None，结果帧缺失兜底 frame 在 timeout 路径已无法产生）；hosted-execution.ts:184 closeForDetach 无超时（挂起时 relay 预算耗尽→destroy+fail-closed，安全但无界）；_signal_leader 对待瞬态 exec 异常一律当"leader 已消失"，若 subject 未在 remaining 内完成则环境被毁、trial 不可计分（文档化 fail-closed，概率低）；README 两段挤一行、_stop_subject_for_timeout 标注 -> Any|None 但从不返回 None。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Separating framework timeout from host abort is the correct ownership model: the verifier may need subject leftovers, while an abandoned trial still requires destructive cleanup. The current implementation loses that distinction at two race boundaries—before Hosted Execution admission is known, and when timeout cleanup itself is cancelled or fails.
The first-principles solution is an explicit server-owned admission token plus one shielded, bounded cleanup state machine. Preserve/detach is allowed only after admission is acknowledged; a second cancellation or execution error transitions that same cleanup state machine to environment destruction before cancellation is re-raised. This is simpler than inferring ownership from a client-side boolean and nested exception handlers.
Review performed with two Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I verified both failure paths against the latest head and current main.
中文评论
区分 framework timeout 与 host abort 是正确的 ownership 模型：verifier 可能需要 subject leftovers，而被放弃的 trial 仍必须执行破坏性清理。当前实现会在两个 race 边界丢失这一区分：Hosted Execution admission 尚未确认时，以及 timeout cleanup 自身再次被取消或失败时。
更符合第一性原理的方案是显式的 server-owned admission token，加上一套 shielded、bounded cleanup state machine。只有 admission ack 后才允许 preserve/detach；第二次 cancellation 或 execution error 必须让同一 cleanup state machine 转入 environment destruction，完成后再重新抛出 cancellation。这比依赖客户端 boolean 和嵌套 exception handler 推断 ownership 更简单可靠。
本次审查使用了两位 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已依据最新 head 和当前 main 复核两个失败路径。
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd32ad82-cf2a-4924-a458-5bff0ddc543b
📒 Files selected for processing (4)
packages/eval/harbor/relay_agent.pypackages/eval/harbor/test_relay_lifecycle.pypackages/runtime-host/src/__tests__/hosted-execution-client.test.tspackages/runtime-host/src/client/hosted-execution.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/eval/harbor/relay_agent.py
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The latest commits improve the timeout boundary: leader shutdown now uses the remaining deadline, unconfirmed shutdown fails closed, and the second-cancel test now enters the intended cleanup window. Two authority gaps remain.
The simplest correct model is a server-owned admission token followed by one shielded, deadline-bounded cleanup state machine. Transport dispatch is not admission, and every exception raised while finalizing cancellation must converge on settle-or-destroy before being rethrown. This removes the current sibling-exception special cases.
Reviewed with Codex using two independent review passes and DeepSeek V4 Flash as an external adversarial pass; I verified the cited state transitions against this exact head and current main.
中文
最新提交改进了 timeout 边界：leader stop 使用剩余 deadline，无法确认退出时 fail-closed，二次 cancel 测试也进入了正确窗口。但仍有两个权威缺口。
最小正确模型是：由 server 返回 admission token；随后所有取消收尾都进入一个 shielded、带 deadline 的状态机。transport dispatch 不能代表 admission，收尾中的任何异常都必须先 settle-or-destroy 再重抛。
本次由 Codex 两轮独立审查，并使用 DeepSeek V4 Flash 做外部对抗审查；我核对了当前 head 与最新 main。
The verifier still scores a timed-out trial, so the relay must not quiesce the process group or delete the environment. Stop only the subject leader so exec can return. Host abort still settles or destroys, because that trial is abandoned. Fixes apache#3150
Harbor exec completion routinely exceeds 100 ms, so a vanished-leader grace of 0.1 s turned a finished subject into an infrastructure failure. Wait for the reserved stop deadline instead. Test predicates now match the group-signal form the relay actually emits.
A preserve abort before hosted.execution.start must not claim the execution continues. Leader-stop execs now take the remaining stop deadline instead of a fixed five seconds.
A preserve abort now inspects whether hosted.execution.start was dispatched. A queued, not-yet-admitted request settles as cancelled. Cancel cleanup uncancels the task so a second cancel still destroys the environment. Leader-stop execs use the remaining stop deadline.
The recancel fixture now sets an event when the leader-stop exec starts. The test waits on that event so the second cancel lands during cleanup, not before it.
Transport in-flight is not Host admission. preserve_environment now calls hosted.execution.admit and detaches only after that token returns. A frame-written interrupt without the token settles the Host instead of claiming execution continues. Cancelled relay cleanup funnels every finalize exception through settle-or-destroy before rethrowing, including execution and persist failures during host abort. Fixes apache#3150
eed0e21 to
9281bc9
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The current head closes the two earlier authority gaps. Framework timeout and Host abort now enter distinct relay paths; cancellation cleanup converges on bounded settle-or-destroy; and preserve/detach is permitted only after a server-owned admission acknowledgement. If that acknowledgement is lost, the client deliberately fails closed and settles the owned Host rather than claiming preservation it cannot prove.
The solution keeps Runtime Host as the execution authority and uses one cleanup state machine instead of inferring ownership from transport dispatch. I found no blocking correctness issue. The inline P3 removes an unreachable fallback left behind by the stronger fail-closed contract.
CI note: all completed checks are green; test_workspaces is still in progress, so merge readiness still depends on that final check.
AI-assisted review disclosure: Codex performed the final review using two independent reviewer passes and OpenCode Go DeepSeek V4 Flash (high effort) as an adversarial advisory pass. I verified admission/abort state transitions, deadline handling, exact head 9281bc99b, and live CI; no local tests were run.
中文评论
当前 head 已关闭此前两个 authority 缺口：framework timeout 与 Host abort 进入不同 relay 路径；取消清理统一收敛到有界的 settle-or-destroy；只有收到 server-owned admission ack 后才允许 preserve/detach。如果 ack 丢失，client 会刻意 fail closed 并 settle owned Host，而不会声称一个无法证明的 preservation。
方案保持 Runtime Host 为 execution authority，并使用单一 cleanup state machine，不再从 transport dispatch 推断 ownership。未发现阻塞性正确性问题。行内 P3 只删除强化 fail-closed 契约后留下的不可达 fallback。
CI 说明：所有已完成 checks 均为绿色；test_workspaces 仍在运行，因此是否可合并还取决于这个最后检查。
AI 辅助审查说明：Codex 使用两轮独立 reviewer 审查，并以 OpenCode Go DeepSeek V4 Flash（high effort）进行对抗性辅助审查；我已核对 admission/abort 状态转换、deadline、精确 head 9281bc99b 与实时 CI。本轮未运行本地测试。
2 participants
Summary
The verifier still scores a trial that hits the framework timeout (
subject_failedplus the verifier reward). The relay used the same cancel path as host abort, so it signalled the recorded process group and could delete the environment — leftover services survived a clean subject exit and disappeared on timeout.environment.execcan return. It does not signal the process group.abortPolicy: preserve_environmenton this path, so a timeout abort does not cancel Host-owned work the verifier is about to score.Fixes #3150
Verification
python3 harbor/test_relay_lifecycle.py— 14 pass, 3 skippedpython3 harbor/test_relay_contract.py— 20/20Checklist
Does this PR entail a change in behavior?
framework_timeout