test(cli): make recover liveness deterministic - #774
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
hojinzs
left a comment
hojinzs
left a comment
There was a problem hiding this comment.
Approve ✅
이슈 #769 의 완료 기준을 독립적으로 재현·검증했습니다. P1/P2 없음, nit 3건만 인라인으로 남겼습니다 — 모두 머지 차단 사유가 아닙니다.
1. 요구사항 반영 여부 — 충족
| 이슈 완료 기준 | 검증 방법 | 결과 |
|---|---|---|
시뮬레이션 조건에서 pnpm --filter @gh-symphony/cli test 결정적 통과 |
이슈에 기술된 pid 리터럴 집합(1, 2, 111, 123, 222, 456, 1234, 2468, 4321, 5678, 999999, 999999999, 음수 pid 포함)에 대해 process.kill(pid, 0) 을 성공시키는 --require 하니스로 실행 |
43 files / 609 tests 통과 |
pnpm --filter @gh-symphony/cli test 10회 연속 통과 |
평문 10회 + 시뮬레이션 10회 | 평문 10/10, 시뮬레이션 10/10 |
| changeset 불필요 | 프로덕션 동작 무변경 확인(아래 Smoke Test) | 해당 없음 — 미첨부가 맞습니다 |
수정이 실제로 원인을 닫는지도 확인했습니다. base(a9d32e7)의 recover.ts / lifecycle.test.ts 로 되돌려 동일 시뮬레이션을 돌리면 이슈에 적힌 그 지점에서 그대로 깨집니다:
FAIL src/commands/lifecycle.test.ts > reports recoverable runs without invoking recovery in dry-run mode
AssertionError: expected false to be true
❯ src/commands/lifecycle.test.ts:611:7
Tests 1 failed | 15 passed (16)
head 에서는 통과 — 즉 이 PR 이 플레이크의 원인을 닫는 것이 맞습니다.
이슈가 "수정하는 김에 같이 점검" 으로 남긴 pid: 1, pid: 2 항목도 위 시뮬레이션에 포함해 돌렸고 깨지지 않았습니다. daemon-liveness.ts 경로가 getProcessIdentity / getProcessCwd 로 한 번 더 걸러내기 때문에 실제 liveness 조회 결과가 판정을 뒤집지 못합니다 — 이슈의 추정과 일치하며, 추가 조치 불필요합니다.
접근 방식도 이슈가 지정한 그대로(#766 의 orchestrator 패턴 = 프로브 주입 + 해당 pid 평가 단언)이고, 진단은 2개 파일 +33/-16 입니다. 요구사항에 없는 오버엔지니어링이나 임의 결정은 없습니다.
2. Smoke Test — 프로덕션 동작 무변경 확인
Preview 환경이 없어 빌드된 CLI(packages/cli/dist/index.js)로 블랙박스 실행했습니다. 임시 config fixture 에 4가지 run 상태를 심고 repo recover --dry-run 을 base / head 각각 실행:
| 시나리오 | 결과 |
|---|---|
죽은 pid (999999, 이 호스트에서 실제 dead) |
worker process is no longer running |
| 살아있는 pid + 시작 30분 이내 | 후보에서 제외 (정상) |
| 살아있는 pid + 시작 90분 경과 | worker appears stuck |
status: retrying + nextRetryAt 경과 |
retry window has elapsed |
base 와 head 출력이 바이트 단위로 동일합니다. --json 출력도 확인했습니다. 즉 주입 seam 이 추가됐어도 기본값이 진짜 프로브로 남아있고 분류 로직 4개 분기가 모두 그대로입니다.
이로써 PR 본문의
Post-merge / human validation첫 번째 항목(dry-run 출력 무변경)은 자동으로 확인 완료입니다. 별도 수동 확인 불필요합니다.
3. 품질 게이트
| 명령 | 결과 |
|---|---|
pnpm lint |
✅ 통과 |
pnpm typecheck |
✅ 통과 |
pnpm build |
✅ 통과 |
pnpm test (cli, 20회) |
✅ 전부 통과 |
CI (Test, Container Smoke) |
✅ 둘 다 success |
pnpm format 은 실패하지만 이 PR 과 무관한 기존 상태입니다. 변경된 2개 파일은 Prettier clean 이고, packages/**/*.ts 위반 파일 수는 base 54건 → head 53건으로 오히려 1건 줄었습니다(recover.ts 의 import 정리 덕분). 별도 이슈로 다룰 사안입니다.
4. 이전 리뷰 후속 조치
사람 리뷰 이력 없음. Codex 리뷰는 8643b9d 기준 findings 없이 완료. 미해결 스레드 없습니다.
결론
Approve. 인라인 nit 3건은 후속 판단에 맡기며, 이 PR 을 막지 않습니다. 그대로 머지하셔도 됩니다.
Generated by Claude Code
| const candidates = await listRecoverCandidates( | ||
| runtimeRoot, | ||
| projectId, | ||
| dependencies.isProcessRunning ?? isProcessRunning |
There was a problem hiding this comment.
nit: 이 seam 은 --dry-run 분기에만 연결되어 있고, 실제 복구 경로(L82 orchestratorRunCli)에서는 dependencies 가 조용히 무시됩니다.
실제 복구는 orchestrator CLI 로 위임되고 그쪽에 이미 자체 seam(OrchestratorService.dependencies.isProcessRunning, #766)이 있으므로 동작상 문제는 없습니다. 다만 코드만 봐서는 "주입했는데 안 먹는" 경우가 있다는 걸 알 수 없으니, RecoverDependencies 선언부(L18)에 한 줄 주석으로 dry-run 분류 전용 이라는 점을 남겨두면 다음 사람이 헷갈리지 않을 것 같습니다. 블로커는 아닙니다.
Generated by Claude Code
| const startedAt = run.startedAt ? new Date(run.startedAt).getTime() : 0; | ||
| const runningForMs = Date.now() - startedAt; | ||
| if (isProcessRunning(run.processId) && runningForMs > 30 * 60 * 1000) { | ||
| const processRunning = isRunning(run.processId); |
There was a problem hiding this comment.
nit (의도 확인): 여기서 liveness 를 두 번 조회하던 것을 스냅샷 1회로 바꾼 것은 이슈 #769 가 요구한 범위(주입 가능한 프로브 + seam 단언)를 살짝 넘어서는 동작 변경입니다.
기존 코드에서는 두 process.kill 호출 사이에 프로세스가 죽으면 running && !stuck → !running 으로 흘러 "worker process is no longer running" 으로 분류됐는데, 지금은 단일 스냅샷이라 그 TOCTOU 경로가 사라집니다.
미반영 요청은 아닙니다 — 하나의 분류가 "stuck" 과 "dead" 를 동시에 근거로 삼을 수 없게 되므로 오히려 일관적이고, syscall 도 절반이 됩니다. 다만 테스트 단언이 toHaveBeenCalledWith (호출 횟수 아님) 라서 이 변경 없이도 이슈는 닫혔을 것이므로, 의식적인 결정이었는지만 확인 부탁드립니다. 아래 세 분기 모두 base 와 출력이 바이트 단위로 동일한 것은 확인했습니다(리뷰 본문 Smoke Test 참고).
Generated by Claude Code
| }); | ||
|
|
||
| expect(orchestratorRunCli).not.toHaveBeenCalled(); | ||
| expect(isProcessRunning).toHaveBeenCalledWith(999_999); |
There was a problem hiding this comment.
nit: seam 이 실제로 평가됐다는 단언은 정확히 이슈가 요구한 가드입니다 👍
한 가지 남는 공백: 프로덕션 기본값이 여전히 진짜 프로브인지 를 지키는 자동 테스트가 없습니다. 누군가 recover.ts:63 의 ?? isProcessRunning 폴백을 지워도 이 스위트는 그대로 green 이고(주입 경로만 타므로), 실제 repo recover --dry-run 은 그때 깨집니다. 커버리지 리포트에서도 해당 파일 branch 가 28.57% 로 잡힌 이유입니다.
dependencies 를 넘기지 않고 dry-run 을 호출해 process.kill spy 가 불렸는지만 확인하는 케이스 하나면 이 방향도 닫힙니다. 이번 PR 범위 밖이라 봐도 무방하고, 저는 수동 블랙박스로 기본 경로가 살아있는 것을 확인했습니다(리뷰 본문 참고).
Generated by Claude Code
2 participants
TL;DR
Change-point diagram
Start here
packages/cli/src/commands/recover.ts— recovery classification and dependency boundary.packages/cli/src/commands/lifecycle.test.ts— dry-run recovery integration coverage.Changes
999999reaches the seam.Evidence
pnpm --filter @gh-symphony/cli test -- src/commands/lifecycle.test.ts— 16 tests passed before implementation.NODE_OPTIONS="--require /tmp/issue-769-process-kill-sim.cjs" pnpm --filter @gh-symphony/cli test— 43 files, 609 tests passed with the listed positive and negative PIDs treated as live.pnpm --filter @gh-symphony/cli testrepeated 10 consecutive times — 609/609 tests passed in every run.pnpm lint— passed.pnpm test— passed after the clean workspace packages were built.pnpm typecheck— passed.pnpm build— passed.Risks & rollback
Changed files
packages/cli/src/commands/recover.ts— injectable liveness dependency with production default.packages/cli/src/commands/lifecycle.test.ts— deterministic dry-run recovery coverage and seam assertion.Post-merge / human validation
Issues
Closed #769