Skip to content

test(eval): harden relay descendant cleanup against process-group timing - #3996

Merged
Astro-Han merged 1 commit into
apache:mainfrom
testikun:codex/issue-3770-relay-quiescence
Aug 27, 2026
Merged

test(eval): harden relay descendant cleanup against process-group timing#3996
Astro-Han merged 1 commit into
apache:mainfrom
testikun:codex/issue-3770-relay-quiescence

Conversation

@testikun

Copy link
Copy Markdown
Contributor

Summary

Closes #3770.

The Eval relay lifecycle regression now exercises the real leader-exits-first boundary instead of relying on a fixed delay:

  • keeps a FIFO open from the test side so the descendant can report readiness and remain blocked;
  • records the descendant PID and verifies it shares the scope process group;
  • waits for the leader's execution to finish before calling settlement;
  • releases the descendant only after settlement, so a missed process-group cleanup produces an observable late-write;
  • cleans up the FIFO and any surviving descendant on failure paths.

This is test-only hardening; production relay behavior is unchanged.

Verification

  • python3 harbor/test_relay_lifecycle.py: 14 passed
  • python3 harbor/test_eval_framework.py: 3 passed
  • python3 harbor/test_relay_contract.py: 30 passed
  • python3 harbor/test_egress_filter.py: 18 passed
  • python3 harbor/test_run_trial_policy.py: 4 passed
  • python3 harbor/test_relay_artifacts.py: 1 passed
  • python3 -m py_compile harbor/test_relay_lifecycle.py: passed
  • git diff --check: passed
  • The descendant-process test was repeated 20 times on macOS with GNU setsid; all runs passed.
  • The full TypeScript build was not run because this fresh worktree has no installed Node dependencies. Linux CI remains authoritative for process-group behavior.

中文摘要

本 PR 仅强化 Eval relay 生命周期回归测试,不修改生产逻辑。测试会先确认 descendant 已经和 scope 处于同一进程组,并等待 leader 退出;settlement 完成后才释放 descendant。如果进程组清理失效,descendant 会写入 late-write,测试将明确失败。同时补充失败路径清理,避免遗留进程。

@testikun
testikun force-pushed the codex/issue-3770-relay-quiescence branch from 2e0925d to b178592 Compare August 27, 2026 09:15
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Aug 27, 2026
@Astro-Han

Copy link
Copy Markdown
Contributor

This is a synthesis of the independent blind review by @Sol-404ARE at exact head b178592e9d1a7e13a9091d430f1aacc15c28e88a (base 45824cc78e, 1 file +58/-6). I verified the diff and the exact-head CI myself; the file:line findings below are from Sol's sealed review.

What I checked myself:

  • Read gh pr diff 3996 (1 file packages/eval/harbor/test_relay_lifecycle.py:608-702) and confirmed the test replaces a 300ms race with FIFO-gated atomic readiness, PGID assertion, and settled release.
  • Checked exact-head CI: test run 33057727797 SUCCESS, OPEN/MERGEABLE/BLOCKED/REVIEW_REQUIRED.

Findings from Sol's review (file:line anchored):

Standards — NO-GO (2×P2 + 2×P3, worst P2)

  • P2 — The PR body omits the mandatory AI-use declaration — required select-one AI use: Tool / Scope section is missing. This violates CONTRIBUTING.md:30-34 and the PR template; trailer applicability cannot be determined.

  • P2 — The required template checklist and behavior declaration are absent — the body omits the Checklist and the required behavior yes/no attestations, including the regression-fails-without-change attestation required by CONTRIBUTING.md:79-83 and the PR template.

  • P3 — The branch codex/issue-3770-relay-quiescence does not use the conventional <type>/<description> prefix aligned with the title test(eval): harden relay... and the commit message.

  • P3 — The KILL fallback remains unexercisedpackages/eval/harbor/test_relay_lifecycle.py:622-640 leaves default SIGTERM handling, so the regression deterministically verifies TERM cleanup but cannot catch a broken _quiesce_scope KILL fallback for TERM-resistant descendants. This is an evidence limitation.

The code change itself is focused and localized with no meaningful duplication; a provisional concern about the one-second negative observation was withdrawn as too low confidence.

Spec — GO (0 P0–P3)

  • The existing test_relay_lifecycle.py:608-640 now correctly replaces the fixed 300 ms race with FIFO gating plus atomic readiness.
  • 660-677 waits for scope/child readiness, proves child PGID equals scope PGID, then waits for leader execution to finish.
  • 678-686 settles before release so a survivor would be observable as late-write; 687-702 releases/kills/waits/closes/unlinks on failure paths.
  • The change matches issue test(eval): harden relay descendant cleanup against process-group timing #3770's hardening of the leader-exits-first test and leaves production behavior unchanged.

Verification: git diff --check PASS, Python py_compile PASS, exact target PASS 1/1 locally (GNU setsid --wait shim), hosted test SUCCESS, worktree clean, head unchanged.

What I did not judge:

  • KILL fallback for TERM-resistant descendants remains uncovered beyond the negative observation — a future test with a TERM-ignoring child would close that gap.

Gate: two P2 template findings (AI declaration and Checklist) remain; no P0/P1 code defect was found, test is green, but the PR cannot be merged until the mandatory PR body declarations are completed.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This replaces a timing assertion with a causal one, which is the right fix for #3770. The old test slept 0.4s and asserted the file was absent — it could pass because cleanup worked or because the descendant simply hadn't got there yet, and those are indistinguishable.

The new shape makes them distinguishable: the descendant blocks on a FIFO the test holds open O_RDWR (so its own open never blocks and there's always a writer), publishes its PID through a tmp-then-os.replace so the marker is never read half-written, and is released only after settlement returns. A missed process-group cleanup therefore shows up as a real late-write instead of being raced against. Asserting os.getpgid(child_pid) == pid before any of that pins the premise the whole test rests on.

The failure paths are covered too — releasing again, SIGKILLing the descendant, killing the leader through _signal, closing the fd, unlinking the PID file — so a mid-test failure doesn't leak a blocked process.

AI-assisted review: I traced the FIFO handshake, the release ordering against _settle, and the cleanup paths at head b178592e9. No tests run. AI review is not independent human review.

@Astro-Han
Astro-Han merged commit a2a085c into apache:main Aug 27, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(eval): harden relay descendant cleanup against process-group timing

2 participants