docs(reports): archive v4.5.0 PASS review reports (Issue #18)
Final HEAD263d6bf: claude, agy and grok PASS (delta review of the lifecycle-lock commits on top of theirab842f1PASS), codex PASS (its own round after F1/F2). Full suite at263d6bf: 906 passed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
This commit is contained in:
GodopuandClaude Fable 5.1 committed
1 parent
263d6bf30d
commit
5611859930
4 files changed
+332
No files matched your search
@@ -0,0 +1,132 @@
|
|||||||
|
# 🔍 Review Report — Job 48b1b0f8 (Issue #18 델타 리뷰, reviewed HEAD `263d6bf`)
|
||||||
|
|
||||||
|
- **Reviewer**: `planner-reviewer-claude-01` (role: planner,reviewer) · 2026-09-14
|
||||||
|
- **대상**: `git diff ab842f1...HEAD` 2커밋, 4파일 +273/−10.
|
||||||
|
- `2b1d590`: 잡별 lifecycle lock, 구현 세션 커밋
|
||||||
|
- `263d6bf`: codex F1·F2 교정, creator 커밋
|
||||||
|
- 전체 브랜치는 내가 `ab842f1` 에 PASS 한 범위다(`98215f55`).
|
||||||
|
- 브리프의 "변경분" 은 작업 트리의 추적되지 않은 codex 리포트 파일 하나뿐이었다. 그래서 델타는 git 으로 직접 확인했다.
|
||||||
|
- HEAD 는 이 잡 내내 `263d6bf` 이다(`completed` 발행 명령에서 다시 확인).
|
||||||
|
- **방법**:
|
||||||
|
- 델타를 정독했다.
|
||||||
|
- 임시 레지스트리에서 실제 타이머 프로세스로 독립 실험 4건을 했다(브로커 없음, 닫힌 포트·블랙홀 주소).
|
||||||
|
- 선택 테스트 9파일을 실행하고 정적 검사를 했다.
|
||||||
|
- 이 잡은 `started --heartbeat 30` 으로 시작했다. 코드는 수정하지 않았다.
|
||||||
|
- **안전 규칙 준수**:
|
||||||
|
- 실제 `.mam/jobs` 레코드는 수정하지 않았다(실행 전후 비교).
|
||||||
|
- monitor 데몬과 `.mam/monitor.lock` 은 건드리지 않았다. purge 는 실행하지 않았다.
|
||||||
|
- 실험 타이머 잔여는 0 이다.
|
||||||
|
|
||||||
|
## 0. 경위
|
||||||
|
|
||||||
|
| 리뷰 | HEAD | 판정 | 요지 |
|
||||||
|
|---|---|---|---|
|
||||||
|
| claude `98215f55`·agy·grok | `ab842f1` | PASS | — |
|
||||||
|
| codex (4회차) | `ab842f1` | NOT PASS | 동시 `start_timer` 2건이 둘 다 spawn 하고 하나만 추적됨 → `2b1d590` |
|
||||||
|
| codex `d1eb9f50` | `2b1d590` | NOT PASS | **F1**: 종료 중인 타이머가 drain 중인 부모가 쥔 lock 을 기다리는 유한 교착(항상 SIGKILL 로 끝남). **F2**: pidfile 기록 실패 뒤 추적되지 않는 타이머가 남음 → `263d6bf` |
|
||||||
|
| codex `bad3ef4b` | `263d6bf` | PASS | — |
|
||||||
|
|
||||||
|
## 1. 검증 항목
|
||||||
|
|
||||||
|
### (1) lifecycle lock 과 레지스트리·seq lock 의 순서, 교착 가능성
|
||||||
|
|
||||||
|
- **구조**: lifecycle lock(`heartbeat.py:187-209`, `<job>.heartbeat.lock`)은 레지스트리·seq lock 과 **별개의 flock** 이다. 잡는 곳은 세 군데뿐이다.
|
||||||
|
- 터미널 `stop_timer`(`:221`)
|
||||||
|
- `start_timer`(`:312`)
|
||||||
|
- 종료하는 타이머의 정리(`:473`, **non-blocking**)
|
||||||
|
- **lifecycle lock 안에서 하는 일**:
|
||||||
|
- `_stop_timer_locked` 의 drain 은 레지스트리 lock 을 잡지 않는다.
|
||||||
|
- `load_job` 은 lock 없는 읽기다(`mqtt_common.py:393-399`). `barrier_blocks` 도 파일 읽기뿐이다.
|
||||||
|
- 그 뒤 spawn 과 pidfile 기록을 한다.
|
||||||
|
- **레지스트리 lock 을 잡는 곳**: `raise_barrier`(`heartbeat.py:270`)와 `next_seq` 뿐이다. 둘 다 lifecycle lock 을 잡지 않는다.
|
||||||
|
- 터미널 publish 는 `raise_barrier`(레지스트리 lock 해제) → `stop_timer`(lifecycle) 순으로 **중첩 없이 차례로** 잡는다.
|
||||||
|
- 따라서 역순 중첩이 없고, lock 순서 교착은 불가능하다.
|
||||||
|
- **drain 중 in-flight beat**: beat 는 seq lock 만 필요하다. 그 lock 은 drain 하는 쪽이 쥐고 있지 않으므로 beat 는 끝까지 갈 수 있다.
|
||||||
|
- **F1 교정**: 종료하는 타이머는 lock 이 바쁘면 기다리지 않고 정리를 소유 연산에 맡긴다. 소유 연산(start·stop)은 모두 `_stop_timer_locked` 로 시작하며, 그 안에서 pidfile 을 지운다.
|
||||||
|
- **실험 E1 — 유휴 타이머 정지** (interval 60, 2 s 대기 뒤 `stop_timer(drain=5)`):
|
||||||
|
- **0.17 s** 에 반환했고, pidfile 이 제거되었으며, "did not drain" 경고가 없었다.
|
||||||
|
- codex 가 `2b1d590` 에서 잰 값은 2.09 s, `-9` 였다. 교착이 해소되었다.
|
||||||
|
- **실험 E4 — 와이어에 걸린 beat 가 있는 drain** (브로커 `10.255.255.1` 블랙홀, interval 1):
|
||||||
|
- `stop_timer(drain=1)` 가 **1.12 s** 에 SIGKILL 로 끝났다. 이 경우의 "did not drain" 경고 1건은 정당하다.
|
||||||
|
- 직후 `next_seq` 는 **0.001 s** 에 반환했다(seq lock 은 비어 있음). pidfile 은 제거되었다.
|
||||||
|
- drain 과 seq lock 사이에 교착이 없다.
|
||||||
|
|
||||||
|
### (2) 동시 start, barrier 뒤 start, pidfile 기록 실패
|
||||||
|
|
||||||
|
- **실험 E2 — 동시 `start_timer` 5건**(스레드 barrier 로 동시 출발):
|
||||||
|
- 오류 0, 살아남은 타이머 **정확히 1개**, pidfile 이 그 타이머를 가리킨다.
|
||||||
|
- `stop_timer` 뒤 생존 0, pidfile 없음.
|
||||||
|
- **실험 E3 — `start_timer` 와 터미널 `completed` publish 경쟁, 8라운드**(시작 간격 0–14 ms 로 변화):
|
||||||
|
- **위반 0/8**: 매 라운드 레코드 `completed`, 생존 타이머 0, pidfile 없음, events.log 에 `completed` 뒤 이벤트 0.
|
||||||
|
- 1라운드는 start 가 먼저 arm 해서 터미널이 drain 했다. 7라운드는 barrier·터미널을 보고 **arm 하지 않았다**.
|
||||||
|
- **pidfile 기록 실패 (F2)**(`:343-364`): spawn 성공 뒤 pidfile 기록이 실패하면 lock 을 쥔 채 그 `Popen` 을 kill·wait 한다. 그리고 이 시도의 부분 pidfile 만 지운 뒤 예외를 다시 던진다.
|
||||||
|
- 시험 `test_a_timer_whose_pid_cannot_be_recorded_is_killed_at_once` 가 SIGKILL 종료, pidfile 없음, lock 재사용 가능을 단정한다.
|
||||||
|
|
||||||
|
### (3) 예외 경로에서 lock 해제와 pidfile 일관성
|
||||||
|
|
||||||
|
- `lifecycle_lock` 은 `try/finally` 로 `LOCK_UN` 하고, `with open(...)` 가 fd 를 닫으므로 예외에서도 해제된다.
|
||||||
|
- non-blocking 이 바쁘면 `False` 를 내주고, 호출자는 소유 상태를 건드리지 않는다(`:473-478`).
|
||||||
|
- **spawn 전 실패**(`Popen` 오류): pidfile 을 쓰지 않고 lock 은 해제된다. 시험 `…cannot_be_spawned_leaves_nothing_behind` 가 확인한다.
|
||||||
|
- **spawn 후 실패**: 이전 소유자의 pidfile 은 이미 `_stop_timer_locked` 가 지웠다. 따라서 남은 내용은 이번 시도의 것뿐이고, pid 접두 비교로 지우는 판단이 옳다.
|
||||||
|
- P3:
|
||||||
|
- `lifecycle_lock` 은 `os.makedirs(registry_dir)` 를 한다. 그래서 잘못된 레지스트리 경로로 터미널 publish 를 하면 디렉터리가 생긴다.
|
||||||
|
- 모든 터미널 publish 가 잡마다 `<job>.heartbeat.lock` 을 남긴다(기존 `.heartbeat.barrier` 와 같은 성질). `.json` 이 아니므로 잡 목록과 #15 가드에는 영향이 없다. 레지스트리 정리 정책에서 함께 다룰 것을 권한다.
|
||||||
|
|
||||||
|
### (4) 새 회귀 시험의 결정성과 프로세스 정리
|
||||||
|
|
||||||
|
| 시험 | 판단 |
|
||||||
|
|---|---|
|
||||||
|
| `test_concurrent_start_timer_calls_leave_exactly_one_tracked_owner` | 스레드 2개로 순서를 강제하지 않는다(codex 도 지적). 위의 5건 동시 실험이 보완한다. `finally` 에서 두 pid 를 SIGKILL 한다 |
|
||||||
|
| `test_start_timer_after_the_terminal_barrier_does_not_arm` | 결정적(프로세스 없음) |
|
||||||
|
| `test_an_idle_timer_stops_gracefully_well_before_the_drain_deadline` | 2 s 대기로 TERM 핸들러 설치 뒤를 노린다. 과부하로 핸들러 설치 전에 TERM 이 가도 기본 동작으로 곧 끝나므로 `< 3 s` 단정은 유지된다. `finally` 에서 정리한다 |
|
||||||
|
| `test_a_timer_whose_pid_cannot_be_recorded_is_killed_at_once` | `builtins.open` 을 잠깐 패치하고 `finally` 에서 복구한다. spawn 된 프로세스는 `finally` 에서 kill·wait 한다 |
|
||||||
|
| `test_a_timer_that_cannot_be_spawned_leaves_nothing_behind` | 없는 python 경로를 쓴다. 두 번째 타이머는 `stop_timer`·SIGKILL 로 정리한다 |
|
||||||
|
|
||||||
|
- 실행 결과는 §2 에 있다.
|
||||||
|
|
||||||
|
### (5) 문서·수치·락스텝
|
||||||
|
|
||||||
|
- MESSAGING §2.5 heartbeat 행이 코드와 일치한다: "lifecycle lock(`<registry>/<job>.heartbeat.lock`, seq lock 과 별개) — 끝난 attempt 는 arm 하지 않음, 종료 타이머는 lock 을 기다리지 않음, pid 기록 실패 시 즉시 종료". VERSIONS 항목 1 도 같다.
|
||||||
|
- VERSIONS 품질 절 "신규 테스트 31개"(`VERSIONS.md:54`)는 `grep -c '^def test_'` 결과 31 과 같다.
|
||||||
|
- 4.5.0 락스텝 유지: `verify_framework_version` OK, `SKILL.md` 8개 4.5.0.
|
||||||
|
- `git diff --check 139bdd4...HEAD` 클린. `py_compile`(`heartbeat.py`, 시험) 통과.
|
||||||
|
|
||||||
|
## 2. 테스트 실행 결과
|
||||||
|
|
||||||
|
| 실행 | 결과 |
|
||||||
|
|---|---|
|
||||||
|
| **선택 9파일** (issue18 31, issue14, issue6 fail_closed·turn_handshake, phase1, phase2a, tier1, tier2, version) @ `263d6bf`(전후 동일) | **355 passed, 0 failed, 742.30 s**, skipped 0. 브로커 의존 AC1 22.09 s, AC3 18.92 s, pane-busy 17.93 s, 벽시계 30.12 s |
|
||||||
|
| 독립 실험 E1–E4 | 모두 기대대로(§1). 실험 타이머 잔여 0 |
|
||||||
|
| creator `31a24383` (3파일) | 100 passed |
|
||||||
|
|
||||||
|
- **누수 (실행 전후)**:
|
||||||
|
- 실제 monitor 데몬 수가 같다(3개, 실행 전후 동일 PID).
|
||||||
|
- pytest 경로의 heartbeat 타이머·구독자·원격 러너는 0 이다.
|
||||||
|
- 실제 `.mam/jobs` 추가는 0 이다.
|
||||||
|
|
||||||
|
## 3. 비차단 권고
|
||||||
|
|
||||||
|
1. (이월, T-1) 브로커 의존 시간 임계 시험의 여유를 넓힌다: AC1·AC3·AC5 의 K×N, issue14 벽시계·pane-busy, send_keys `< 11`.
|
||||||
|
2. 잡마다 남는 `.heartbeat.lock`·`.heartbeat.barrier` 파일의 정리 정책을 정한다. `lifecycle_lock` 의 `makedirs` 부작용도 함께 본다.
|
||||||
|
3. (이월) AC5 는 단일 원격 잡 시뮬레이션과 러너 라벨 단위 시험까지만 검증되었다. 루프 종단 수렴은 후속 E2E 로 남는다.
|
||||||
|
|
||||||
|
## 4. GM 참고
|
||||||
|
|
||||||
|
- **추적되지 않은 파일**: 작업 트리에 `.agents/reports/planner-reviewer-codex-01/report-bad3ef4b.md` 가 있다(codex 리포트 사본). 브리프의 "변경분" 이 이것 하나로 채워졌다. 나는 건드리지 않았다.
|
||||||
|
- **커밋 주체**: `2b1d590` 은 구현 세션(`…01VCgu9Fa9iT7TbJUSzdq4Ff`), `263d6bf` 는 creator(`…01HCGpJXhSZasJLU38ksDghe`)의 커밋이다.
|
||||||
|
- **브랜치 ref**: 11커밋 모두 로컬 `main` 에 있다(`origin/main` 보다 11 앞, 미푸시). `fix/issue-18` 은 `139bdd4` 다. 병합 전에 ref 정리가 필요하다.
|
||||||
|
|
||||||
|
## 5. 종합
|
||||||
|
|
||||||
|
- `2b1d590` 의 잡별 lifecycle lock 은 레지스트리·seq lock 과 중첩 없이 분리되어 있어 교착이 없다.
|
||||||
|
- `263d6bf` 의 non-blocking 종료 정리(F1)와 spawn 후 pid 기록 실패 시 종료(F2)가 정확하다.
|
||||||
|
- 독립 실험으로 확인했다:
|
||||||
|
- 유휴 정지 0.17 s
|
||||||
|
- 동시 start 5건에서 추적 소유자 1개
|
||||||
|
- start 와 터미널 경쟁 8라운드에서 위반 0
|
||||||
|
- 걸린 beat 가 있어도 seq lock 이 즉시 비어 있음
|
||||||
|
- 새 시험들이 이 경로들을 고정한다. 선택 스위트 355건이 모두 통과했다.
|
||||||
|
- 문서·수치(31)·4.5.0 락스텝이 정합한다.
|
||||||
|
- 남은 것은 비차단 권고뿐이며 설계 재검토는 필요 없다.
|
||||||
|
|
||||||
|
[VERDICT: PASS]
|
||||||
@@ -0,0 +1,65 @@
|
|||||||
|
# Issue #18 — final follow-up review
|
||||||
|
|
||||||
|
Reviewed HEAD `263d6bf30d4681ef677d9917b136501b831ab584`, base `139bdd4`. Reviewed the complete new delta from `2b1d590` and the cumulative lifecycle delta from `ab842f1`, carrying forward the unchanged branch review from jobs `6ff39140` and `d1eb9f50`. The brief's introductory HEAD is stale; its supplied correction diff matches the actual follow-up commit. Working tree was clean at the initial and final source checks.
|
||||||
|
|
||||||
|
**PASS. Both findings from the previous review are resolved. No remaining blocking finding was identified in the reviewed scope.**
|
||||||
|
|
||||||
|
## Closure evidence
|
||||||
|
|
||||||
|
### F1: parent/child shutdown lock cycle — resolved
|
||||||
|
|
||||||
|
`heartbeat.lifecycle_lock(blocking=False)` now reports unavailable ownership without waiting. The exiting timer uses that mode and only reads/compares/removes the pidfile when it acquires ownership. A parent performing stop/replacement can retain the lifecycle lock while waiting for the child to exit; the child no longer needs that lock to finish. The parent removes the old pidfile while it still owns the lifecycle operation.
|
||||||
|
|
||||||
|
I independently repeated the prior reproduction with a real idle timer, interval 60, no in-flight heartbeat, and `stop_timer(drain=2)` in a temporary registry. Result:
|
||||||
|
|
||||||
|
```text
|
||||||
|
idle_stop_seconds: 0.15
|
||||||
|
returncode: 0
|
||||||
|
```
|
||||||
|
|
||||||
|
The previous revision took 2.09 seconds and exited through SIGKILL (`-9`). The new regression also verifies an idle timer stops well before a ten-second drain budget without the failure-to-drain warning.
|
||||||
|
|
||||||
|
### F2: untracked child after PID-file failure — resolved
|
||||||
|
|
||||||
|
After a successful spawn, failure while recording the PID now kills and waits for that exact `Popen` process before releasing lifecycle ownership. Cleanup removes an empty/partial pidfile attributable to that attempted owner and propagates the original exception. Failures before successful spawn do not write a PID, and the context manager releases the lock.
|
||||||
|
|
||||||
|
Independent checks injected `ENOSPC` at two boundaries: opening the PID file for writing, and writing after two PID digits had already been flushed. Both used actual owned subprocesses and temporary registries. Results:
|
||||||
|
|
||||||
|
```text
|
||||||
|
open failure: child_returncode=-9, pidfile_exists=False, lock_reusable=True
|
||||||
|
partial-write fault: child_returncode=-9, pidfile_exists=False, lock_reusable=True
|
||||||
|
```
|
||||||
|
|
||||||
|
Lock reuse was verified with nonblocking flock acquisition. All owned processes were reaped and temporary resources removed. The added tests separately cover pre-spawn failure and post-spawn PID-file failure; both pass.
|
||||||
|
|
||||||
|
## Lifecycle and cumulative consistency
|
||||||
|
|
||||||
|
- **Concurrent starts remain serialized.** The observe/drain/spawn/PID publication operation still holds the per-job lifecycle lock. The previous controlled concurrency check established that a second caller cannot enter the ownership body while the first holds it. The concurrent-start regression passes at this HEAD; nonblocking behavior is used only by exiting-timer cleanup.
|
||||||
|
- **Lock ordering remains sound in the reviewed paths.** Draining does not hold the registry/sequence lock, so admitted publishers can finish their registry work. Terminal `raise_barrier` releases the registry lock before acquiring lifecycle ownership for stop. The former child-exit dependency is removed.
|
||||||
|
- **Terminal ordering remains protected.** Starts check terminal record/barrier status while owning the lifecycle operation. If the barrier appears after that check, terminal stop subsequently drains the published owner. Heartbeat admission checks the barrier under the sequence lock. The sequential replacement, in-flight terminal, concurrent-owner and post-barrier-start tests pass.
|
||||||
|
- **PID cleanup preserves the current owner.** When exit cleanup acquires ownership, it still compares the file contents with its own PID before unlinking. When ownership is busy, it does not touch that file. Spawn-failure cleanup runs before another compliant owner can publish its PID.
|
||||||
|
- **Earlier accepted findings remain closed:** R1 remote attempt rollback, R2 command-group timeout cleanup, R4 replayed-state liveness renewal, and B2 watched-agent death. Their focused regressions pass. The wrapper's protocol acknowledgement and disk-based liveness behavior, remote signature/attempt checks and registry transition boundary remain unchanged.
|
||||||
|
- **Documentation and version:** MESSAGING.md and VERSIONS.md now describe lifecycle serialization, nonblocking child cleanup and failed-spawn ownership cleanup. VERSIONS.md correctly counts 31 issue-18 tests. v4.5.0 lockstep remains intact; the public contract/remote additions justify the MINOR release. N/K/T defaults remain 30/4/180 with derived idle 120; documented wrapper grace can extend final closure beyond initial K×N silence detection.
|
||||||
|
|
||||||
|
## Validation
|
||||||
|
|
||||||
|
```text
|
||||||
|
PYTHONDONTWRITEBYTECODE=1 .venv/bin/python -m pytest -p no:cacheprovider \
|
||||||
|
tests/test_issue18_liveness_contract.py tests/test_version_consistency.py -q
|
||||||
|
34 passed in 114.18s; no skips
|
||||||
|
|
||||||
|
git diff 139bdd4...HEAD --check
|
||||||
|
passed
|
||||||
|
|
||||||
|
bash -n .agents/skills/lib.sh \
|
||||||
|
.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job
|
||||||
|
passed
|
||||||
|
```
|
||||||
|
|
||||||
|
The 34 tests comprise 31 issue-18 cases and three version checks. Broker-dependent cases ran; one passing run does not establish network timing determinism. The concurrency regression itself uses concurrent threads without forcing the old interleaving; the previously controlled ownership check supplements that coverage. This round's independent checks specifically re-exercised graceful shutdown and both PID-file fault boundaries.
|
||||||
|
|
||||||
|
Acceptance 5 remains a single remote-job broker simulation plus runner-label unit coverage, **not demonstrated full remote planner/creator/reviewer convergence**. Legacy `MAM_LOOP_PY=0` remote non-support remains documented. The creator-assigned phase1 suite and a full repository suite were not rerun or claimed as passing here.
|
||||||
|
|
||||||
|
No source edits, commits, pushes, issue changes, production monitor operations, real-home purges or direct production job-record edits were performed. Only this report and authorized lifecycle events were written in the workspace. No planner escalation is needed.
|
||||||
|
|
||||||
|
[VERDICT: PASS]
|
||||||
@@ -0,0 +1,82 @@
|
|||||||
|
# Delta Review Report: Fix Issue #18 (`ab842f1`...`263d6bf`)
|
||||||
|
|
||||||
|
**Reviewer**: Antigravity (`herdr:reviewer-agy-01`)
|
||||||
|
**Job ID**: `abd2777b`
|
||||||
|
**Target Commit Range**: [`ab842f1`](file:///Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux/ab842f1)...[`263d6bf`](file:///Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux/263d6bf) on branch `fix/issue-18` (Base: `main` @ `139bdd4`, v4.4.5)
|
||||||
|
**Commits in Delta**:
|
||||||
|
- `2b1d590` — `fix(issue-18): serialize the heartbeat timer lifecycle per job`
|
||||||
|
- `263d6bf` — `fix(issue-18): let an exiting heartbeat timer skip the busy lifecycle lock; kill a timer whose pid cannot be recorded`
|
||||||
|
|
||||||
|
**Date**: 2026-09-14
|
||||||
|
**Status**: Review Complete
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. Scope & Objective
|
||||||
|
|
||||||
|
The full `fix/issue-18` implementation through commit `ab842f1` was previously reviewed and approved (`[VERDICT: PASS]`). This delta review evaluates the two subsequent follow-up commits (`2b1d590` and `263d6bf`) addressing codex feedback regarding heartbeat timer lifecycle synchronization, deadlocks, and untracked child processes.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. Delta Code Analysis & Verification
|
||||||
|
|
||||||
|
### 2.1 Per-Job Lifecycle Serialization (`lifecycle_lock`, `2b1d590`)
|
||||||
|
- **Mechanism**: Introduced `lifecycle_lock(job_id, registry_dir, blocking=True)` using `<registry>/<job>.heartbeat.lock` via `fcntl.flock`.
|
||||||
|
- **Atomic Operations**: `start_timer` and `stop_timer` execute under this lock. Observing, draining, killing a previous owner, spawning a replacement, and publishing the PID to `.heartbeat.pid` now constitute a single serialized ownership transaction.
|
||||||
|
- **Barrier Check Before Arming**: `start_timer` checks job status and `barrier_blocks()` under the lifecycle lock. If a terminal event was raised, `start_timer` logs and returns 0 without spawning a timer.
|
||||||
|
- **Lock Hierarchy & Deadlock Freedom**:
|
||||||
|
- Draining in-flight beats during `stop_timer` does **not** hold the `registry_lock` / sequence lock. In-flight beats are free to acquire `registry_lock`, check the barrier, allocate sequence numbers, and finish.
|
||||||
|
- In `publish_event.py`, `raise_barrier` acquires and releases `registry_lock`, then `stop_timer` acquires and releases `lifecycle_lock`, and finally `prepare_event` acquires `registry_lock` for the terminal event. Lock nesting is non-existent.
|
||||||
|
|
||||||
|
### 2.2 Codex F1: Non-Blocking Shutdown Lock Cycle Removal (`263d6bf`)
|
||||||
|
- **Problem**: When a parent (stopper or replacement `start_timer`) holds `lifecycle_lock` while draining an exiting timer, if the child timer attempted to acquire `lifecycle_lock(blocking=True)` during shutdown to remove its own pidfile, a parent/child lock cycle occurred, forcing the parent to wait until the drain deadline and kill the child with `SIGKILL`.
|
||||||
|
- **Resolution**: `heartbeat.run()` now attempts `lifecycle_lock(job_id, registry_dir, blocking=False) as owned`.
|
||||||
|
- If `owned` is `True`, it compares and unlinks `own_pidfile`.
|
||||||
|
- If `owned` is `False`, it means another lifecycle operation (stop/replace) currently owns the lifecycle and is draining this process. The exiting timer cleanly skips pidfile manipulation, allowing the parent to handle pidfile deletion.
|
||||||
|
- **Verification**: Verified by `test_an_idle_timer_stops_gracefully_well_before_the_drain_deadline`. An idle timer terminates cleanly within 0.15s without triggering the `SIGKILL` fallback.
|
||||||
|
|
||||||
|
### 2.3 Codex F2: Untracked Child Process & Fault Recovery (`263d6bf`)
|
||||||
|
- **Problem**: If writing the pidfile failed after spawning the background timer process (e.g. disk full / ENOSPC), an untracked heartbeat timer could remain running in the background.
|
||||||
|
- **Resolution**: In `_spawn_timer_locked`, writing the pidfile is wrapped in `try...except BaseException`. On failure:
|
||||||
|
1. `proc.kill()` and `proc.wait(timeout=5)` terminate and reap the newly spawned process immediately.
|
||||||
|
2. Any partial or corrupted pidfile matching `proc.pid` is unlinked.
|
||||||
|
3. The exception is re-raised, and `lifecycle_lock` is safely released in the context manager's `finally:` block.
|
||||||
|
- **Verification**: Verified by `test_a_timer_whose_pid_cannot_be_recorded_is_killed_at_once` (injected ENOSPC fault) and `test_a_timer_that_cannot_be_spawned_leaves_nothing_behind`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. Verification & Test Suite Execution
|
||||||
|
|
||||||
|
All 31 tests in `tests/test_issue18_liveness_contract.py` and regression checks were executed:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
.venv/bin/python -m pytest tests/test_issue18_liveness_contract.py -v
|
||||||
|
```
|
||||||
|
**Result**: **31 passed in 109.41s** (0 failures, 0 skips).
|
||||||
|
|
||||||
|
```bash
|
||||||
|
.venv/bin/python -m pytest tests/test_version_consistency.py -v
|
||||||
|
```
|
||||||
|
**Result**: **3 passed in 0.85s**.
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git diff 139bdd4...HEAD --check
|
||||||
|
bash -n .agents/skills/lib.sh .agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job
|
||||||
|
```
|
||||||
|
**Result**: Clean rc 0 across formatting and syntax checks.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. Documentation & Version Consistency
|
||||||
|
|
||||||
|
- **`MESSAGING.md` §2.5**: Accurately describes the per-job lifecycle lock `<job>.heartbeat.lock`, non-blocking child cleanup, and failed-spawn cleanup.
|
||||||
|
- **`VERSIONS.md`**: Updated to state 31 tests in `tests/test_issue18_liveness_contract.py`, documenting the 4th/5th review round additions.
|
||||||
|
- **Lockstep 4.5.0**: Maintained across all skills and framework manifests.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 5. Final Verdict
|
||||||
|
|
||||||
|
The delta changes in `2b1d590` and `263d6bf` are correct, eliminate potential deadlocks and untracked timer leaks, and cleanly pass all regression tests.
|
||||||
|
|
||||||
|
[VERDICT: PASS]
|
||||||
@@ -0,0 +1,53 @@
|
|||||||
|
# 🔍 Review Report — Job 9b88790c (델타 `ab842f1...263d6bf`)
|
||||||
|
|
||||||
|
- **Reviewer**: `reviewer-grok-01` (role: reviewer) · 2026-09-14
|
||||||
|
- **범위**: `git diff`/`git log` **`ab842f1...HEAD`** (그 이전 브랜치는 PASS 범위)
|
||||||
|
- **HEAD**: `263d6bf` `fix(issue-18): let an exiting heartbeat timer skip the busy lifecycle lock; kill a timer whose pid cannot be recorded`
|
||||||
|
- **선행**: `2b1d590` 잡별 lifecycle flock
|
||||||
|
- **안전**: 실제 레지스트리·모니터·purge 미접촉. 소스 무변경.
|
||||||
|
|
||||||
|
`git diff --check ab842f1...HEAD` 클린.
|
||||||
|
|
||||||
|
## (1) 락 순서·교착
|
||||||
|
|
||||||
|
두 flock: 레지스트리/seq (`registry.lock`) vs 타이머 소유권 (`<job>.heartbeat.lock`).
|
||||||
|
|
||||||
|
- `start_timer` / `stop_timer` 는 **lifecycle 만** 잡는다. drain 대기 중 seq lock 을 안 잡으므로, 이미 admit 된 beat 가 `next_seq` 로 seq lock 을 잡을 수 있다. 주석대로 drain×seq 교착을 피한다.
|
||||||
|
- 터미널: `raise_barrier`(seq lock, 해제) → `stop_timer`(lifecycle). seq 를 잡은 채 lifecycle 을 기다리지 않는다.
|
||||||
|
- **F1**: 종료 타이머는 `lifecycle_lock(..., blocking=False)` (`heartbeat.py:472-478`). 부모가 drain 동안 lock 을 쥐고 있어도 자식은 기다리지 않고 빠져나간다. lock 을 얻으면 pidfile 이 **자기 pid** 일 때만 unlink. 바쁜 lock 은 건드리지 않음 — 홀더가 pidfile 을 지운다.
|
||||||
|
- 재현 테스트: `test_an_idle_timer_stops_gracefully_well_before_the_drain_deadline` (drain=10, 종료 < 3s, SIGKILL 경고 없음).
|
||||||
|
|
||||||
|
## (2) 동시 start · barrier · pidfile 실패
|
||||||
|
|
||||||
|
- 동시 `start_timer`: observe/drain/spawn/pidfile 이 한 lock. `test_concurrent_start_timer_calls_leave_exactly_one_tracked_owner` — 생존 1, pidfile 일치, stop 후 둘 다 사망.
|
||||||
|
- barrier/터미널 뒤: lock 아래 재확인, arm 안 함 (`:318-320`). `test_start_timer_after_the_terminal_barrier_does_not_arm`.
|
||||||
|
- **F2**: pidfile `open`/`write` 실패 시 그 `Popen` 을 kill+wait, 부분 pidfile 은 이 pid 접두일 때만 삭제, 예외 재발생, lock 은 context `finally` 로 해제 (`:344-364`). spawn 자체 실패는 pidfile 없음.
|
||||||
|
|
||||||
|
## (3) 예외·일관성
|
||||||
|
|
||||||
|
`lifecycle_lock` 은 획득 후에만 UNLOCK. `BlockingIOError` 는 yield False 후 return (UNLOCK 안 함). spawn 실패·pidfile 실패 모두 lock 해제. 테스트가 lock 재획득 `LOCK_NB` 로 확인.
|
||||||
|
|
||||||
|
## (4) 테스트 결정성
|
||||||
|
|
||||||
|
신규: 동시 start, barrier 후 미arm, idle 정상 종료, pidfile ENOSPC, spawn 실패. 모두 프로세스 finally 회수. 동시 start 는 인터리빙을 강제하지 않으나 lock 이 직렬화하므로 한 소유자면 충분.
|
||||||
|
|
||||||
|
## (5) 문서·락스텝
|
||||||
|
|
||||||
|
MESSAGING §2.5 가 lifecycle 직렬화·non-blocking 자식 정리·pidfile 실패 시 살처분을 반영. VERSIONS **31** = collect 31. **4.5.0** 유지 (`lib.sh`/`eager.py`).
|
||||||
|
|
||||||
|
## 테스트 실행
|
||||||
|
|
||||||
|
```
|
||||||
|
env -u MAM_VERSION .venv/bin/python -m pytest -q \
|
||||||
|
tests/test_issue18_liveness_contract.py \
|
||||||
|
tests/test_issue5_phase1_delegation.py \
|
||||||
|
tests/test_version_consistency.py
|
||||||
|
```
|
||||||
|
|
||||||
|
**100 passed in 127.96s** (issue18 31 + phase1 + version). 브로커 의존 건 이번 실행 skip 없음.
|
||||||
|
|
||||||
|
## 종합
|
||||||
|
|
||||||
|
F1 교착과 F2 미추적 타이머가 닫혔고, seq lock 과 lifecycle lock 의 순서는 drain 과 양립한다. 설계 재작업 불필요.
|
||||||
|
|
||||||
|
[VERDICT: PASS]
|
||||||
Reference in new issue
Block a user