fix(#138): 修 test_restart_daemons 泄漏 detached daemon 进程#151
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
🤖 quality review: 小死码清理建议TL;DR
详细说明这次改动集中在 我只卡两个小的死码/可读性点: 📎 完整 codex 原始输出(存档备查)---
pr: 151
role: quality
verdict: comment
---
## Verdict
Comment: the change is focused and the cleanup helpers are readable, but two small dead-code leftovers should be removed.
## Evidence
- skills/codex-refactor-loop/scripts/test_restart_daemons.py:109 defines `REFACTOR_SELF_DOCUMENTATION`, but nothing reads it. The actual required self-doc already exists as the class comment at lines 104-108, so the string constant is duplicated dead code rather than a useful contract.
- skills/codex-refactor-loop/scripts/test_restart_daemons.py:487 assigns `command = ["bash", ...]` after the subprocess calls were moved into `_start_helper_process`; the local is now unused and makes the race test look like it still uses an inline command path.
## What would change your verdict
Delete the unused `REFACTOR_SELF_DOCUMENTATION` constant and the unused `command` local. The remaining refactor self-doc comment is clear enough and self-contained.
REVIEW_DONE::quality:comment
⟦AI:AUTO-LOOP⟧⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 Architect review approve: test-only cleanup, no runtime contract expansionTL;DR
详细说明我按 PR 的真实 base/head 复核了三点:实际 diff 只有 关键证据是 📎 完整 codex 原始输出(存档备查)pr: 151
|
Contributor
Author
🤖 tests review: sleep 轮询需要收口TL;DR
详细说明这轮测试确实覆盖了核心业务结果: 阻塞点是稳定性规则。 我本地验证过 focused tests 仍然能过: 📎 完整 codex 原始输出(存档备查)---
pr: 151
role: tests
verdict: reject
---
## Verdict
Reject: the regression coverage is meaningful, but the PR adds sleep-based polling in test cleanup without an allowlist entry or documented exception.
## Evidence
- skills/codex-refactor-loop/scripts/test_restart_daemons.py:243 `test_restart_daemons.py` adds `_wait_for_pid_exit`, which polls process liveness until a timeout and sleeps with `time.sleep(0.02)` on each iteration; this violates the repo rule in `skills/codex-refactor-loop/SKILL.md` that tests must use deterministic awaiters and not sleep/delay-based pacing.
- skills/codex-refactor-loop/scripts/test_restart_daemons.py:250 adds the same timeout + `time.sleep(0.02)` pattern for process-group exit. I found no `test_polling_allowlist.txt` or equivalent allowlist entry in the worktree documenting this as an accepted unstable/polling exception.
- Positive coverage note: `test_cleanup_daemons_reaps_tmp_root_wrappers_and_children` does assert the real leak outcome, and `test_restart_helper_test_cleanup_is_test_only_tmp_root_scoped` pins the test-only source contract. The failure is the added pacing mechanism, not missing business-outcome assertions.
## What would change your verdict
Replace the sleep-polling cleanup waits with a deterministic process wait/reap path, or add the repository's documented polling exception/allowlist entry with a specific reason for why process-exit waiting cannot be deterministic here.
REVIEW_DONE::tests:reject
⟦AI:AUTO-LOOP⟧⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 PR #151 r1 fix: 已移除 sleep 轮询TL;DR
详细说明我只在 PR 原本的两个测试文件内改动。 另外 quality reviewer 的两个 comment 不是 blocking demand, 但都在 scope 内:删除了 unused 📎 完整 codex 原始输出(存档备查)# Fix report for PR 151 round 1
## Applied
- (A) skills/codex-refactor-loop/scripts/test_restart_daemons.py:238: replaced the sleep-based `_wait_for_pid_exit` loop with deterministic OS process-exit awaiters: Linux `pidfd_open` + `select`, and macOS/BSD `kqueue` `KQ_FILTER_PROC`. This addresses reviewer:tests evidence #1 citing `time.sleep(0.02)` at the former line 243 and the `skills/codex-refactor-loop/SKILL.md` rule "No sleep/delay-based test pacing; use deterministic awaiters."
- (A) skills/codex-refactor-loop/scripts/test_restart_daemons.py:247: replaced the sleep-based process-group wait loop with per-process deterministic exit waits over the tmp-root-scoped group members. This addresses reviewer:tests evidence #2 citing `time.sleep(0.02)` at the former line 250 and preserves the issue #138 judge boundary: test-only process-group cleanup, no `restart-daemons.sh` runtime contract change.
- (A) skills/codex-refactor-loop/scripts/test_anti_stop_restart_helper_contract.py:210: extended the source-regression contract to require `pidfd_open`, `KQ_FILTER_PROC`, and absence of `time.sleep(` in `test_restart_daemons.py`, preventing the reject pattern from silently returning.
- (A) skills/codex-refactor-loop/scripts/test_restart_daemons.py:103: removed unused `REFACTOR_SELF_DOCUMENTATION` string constant. This was advisory reviewer:quality context, not blocking reject evidence, but it is in-scope dead-code cleanup and the class-level `Refactor (iter1/issue-138)` Old/New comment remains.
- (A) skills/codex-refactor-loop/scripts/test_restart_daemons.py:516: removed unused `command` local from the race test. This was advisory reviewer:quality context, not blocking reject evidence.
## Rejected as false positive
- None. The only reject demand was reviewer:tests' sleep-polling objection, and it was valid.
## Blocked (cannot fix this round)
- None. Local files `.refactor-loop/runs/review-pr151-tests.md` and `.refactor-loop/runs/review-pr151-quality.md` were absent, so I used the PR #151 GitHub comments as the available reviewer source. The missing local artifacts did not block a concrete fix because the GitHub comments contained the full verdict, evidence, and "What would change your verdict" sections.
## Build status
- build: pass (`git diff --check`)
- tests: pass (`python3 skills/codex-refactor-loop/scripts/test_restart_daemons.py`: 10 tests; `python3 skills/codex-refactor-loop/scripts/test_anti_stop_restart_helper_contract.py`: 11 tests; `python3 -m unittest discover -s skills/codex-refactor-loop/scripts -p 'test_*.py'`: 399 tests)
## Recommendation for next round
- expect MERGE or MERGE_WITH_COMMENTS: the only reject was addressed, architect already approved, and quality's advisory dead-code requests were also applied.
⟦AI:AUTO-LOOP⟧⟦AI:AUTO-LOOP⟧ |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
🤖 quality review approve: 当前轮可读性通过TL;DR
详细说明这轮 diff 仍然只落在两个测试文件里, 范围很干净。
📎 完整 codex 原始输出(存档备查)---
pr: 151
role: quality
verdict: approve
---
## Verdict
Approve: the current PR is focused, readable, free of the earlier dead-code leftovers, and the refactor self-doc is clear.
## Evidence
- skills/codex-refactor-loop/scripts/test_restart_daemons.py:104 has a readable `Refactor (iter1/issue-138)` Old/New block that explains why cleanup moved from tracked wrapper PID cleanup to tmp-root-scoped wrapper/group/descendant cleanup.
- skills/codex-refactor-loop/scripts/test_restart_daemons.py:167 keeps cleanup orchestration in one small method, while the process termination, group termination, deterministic wait, and tmp-root scan details are split into named helpers instead of one large branchy block.
- skills/codex-refactor-loop/scripts/test_restart_daemons.py:117, skills/codex-refactor-loop/scripts/test_restart_daemons.py:179, skills/codex-refactor-loop/scripts/test_restart_daemons.py:368, and skills/codex-refactor-loop/scripts/test_restart_daemons.py:385 show the new `helper_process_groups` state is reachable: helper launches register process groups, and teardown consumes them.
- skills/codex-refactor-loop/scripts/test_restart_daemons.py:193 still has a caller at skills/codex-refactor-loop/scripts/test_restart_daemons.py:565, so the compatibility wrapper is not dead code.
- skills/codex-refactor-loop/scripts/test_anti_stop_restart_helper_contract.py:210 adds source-regression coverage for the test-only cleanup contract without adding production abstractions or unrelated cleanup.
REVIEW_DONE::quality:approve
⟦AI:AUTO-LOOP⟧⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构审查通过: test-only daemon leak cleanupTL;DR
详细说明我按 看了实际 diff, 净改动只有 和 。新增进程探测只留在测试清理里, 并且 source-regression 明确禁止 PID TTY TIME CMD 架构规则映射: 这次改动满足 的 behavior test + source-regression 要求, 也保持 controller-runtime 例外的 narrow allowlist / no lifecycle authority 边界。没有发现 actor/store 拆分、外部 repo reference、compat shim 或 scope drift。 📎 完整 codex 原始输出(存档备查)⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 tests review approve: 泄漏回归测试已收口TL;DR
详细说明我复核的是当前 上一轮 tests reject 的阻塞点是 本地验证通过: 📎 完整 codex 原始输出(存档备查)---
pr: 151
role: tests
verdict: approve
---
## Verdict
Approve: the current branch has meaningful leak-regression coverage, source-regression coverage for the test-only boundary, and no remaining sleep/delay pacing or assertion weakening.
## Evidence
- `skills/codex-refactor-loop/scripts/test_restart_daemons.py:528` adds `test_cleanup_daemons_reaps_tmp_root_wrappers_and_children`, which asserts the real outcome for issue #138: tmp-root daemon wrapper/child processes exist before cleanup and are gone after `_cleanup_daemons()`.
- `skills/codex-refactor-loop/scripts/test_restart_daemons.py:238` now waits for process exit through deterministic OS awaiters, `pidfd_open` on Linux and `KQ_FILTER_PROC` on kqueue platforms, instead of the earlier `time.sleep(0.02)` polling pattern.
- `skills/codex-refactor-loop/scripts/test_anti_stop_restart_helper_contract.py:210` adds source-regression coverage for the new cleanup rule: it pins `start_new_session=True`, `os.killpg`, tmp-root scoping, deterministic awaiters, and `self.assertNotIn("time.sleep(", self.helper_test)`.
- `skills/codex-refactor-loop/scripts/test_anti_stop_restart_helper_contract.py:228` keeps the production helper free of process-probe/process-group contract tokens such as `ps`, `pgrep`, `setsid`, and `killpg`, so the new cleanup machinery stays in tests.
- I found no added `[Skip]`, manual-category equivalent, sleep/delay pacing, loosened assertions, or mock-only pseudo-coverage in the current diff.
- Verification passed locally: `python3 skills/codex-refactor-loop/scripts/test_restart_daemons.py` ran 10 tests OK; `python3 skills/codex-refactor-loop/scripts/test_anti_stop_restart_helper_contract.py` ran 11 tests OK; `python3 -m unittest discover -s skills/codex-refactor-loop/scripts -p 'test_*.py'` ran 399 tests OK; `git diff --check origin/auto-refact-dev...HEAD` passed.
REVIEW_DONE::tests:approve
⟦AI:AUTO-LOOP⟧⟦AI:AUTO-LOOP⟧ |
Contributor
Author
🤖 架构审查通过: test-only daemon leak cleanupTL;DR
详细说明上一条同 headline 评论的 raw artifact 是正确的, 但 TL;DR 被 shell 展开污染；本条是 canonical review comment。 我按 架构规则映射: 这次改动满足 📎 完整 codex 原始输出(存档备查)⟦AI:AUTO-LOOP⟧ |
…ssue138-test-daemon-leak # Conflicts: # skills/codex-refactor-loop/scripts/test_restart_daemons.py
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 9 r3 consensus(hybrid test-only cleanup,no production contract change)。tearDown 按 tmp_root/进程组回收 detached daemon + behavior test 断言无残留。全量绿。Closes #138
⟦AI:AUTO-LOOP⟧