test: probe for a free pid instead of assuming 4242 is dead - #3042
test: probe for a free pid instead of assuming 4242 is dead#3042lifrary wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesThe tests add Dead PID test fixtures
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR makes test dead-process fixtures use kernel-verified unused PIDs instead of a hardcoded value, improving reliability without changing production behavior. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
리뷰 · 우선순위 43 / 80설명 이 PR은 제품 코드를 안 고친다. 죽은 프로세스처럼 보이게 하려고 시험이 pid 4242를 박아 두던 자리를, 커널에 물어보고 진짜로 없는 pid를 고르게 바꾼다. 지금 왜 이 시험이 깨졌는가. 제품 코드는 그 pid가 살아 있는지 커널에 묻는다. 시험이 4242를 죽었다고 가정하면, 그 번호가 다른 프로세스에 붙어 있는 순간 제품은 올바르게 살아 있다고 답한다. 시험은 그걸 실패로 읽는다. 작성자의 macOS에서 4242는 프로브는 원래 있었다. 이 PR은 draft다. 체크리스트 0/4다. 준비 게이트가 아직 안 열렸다. #3032를 확인하다가 찾았다고 하고, 둘은 서로 기다리지 않는다. #3032는 이미 서명된 리뷰가 있다. types.ts/config.ts 분할과 무관하다. 중복이 아니다. 닫지 않는다. 점수 43. 런타임을 고치지는 않지만, macOS와 공유 CI에서 가짜 실패를 만드는 픽스처 구멍은 진짜다. 다만 draft라서 지금 머지 버튼은 아니다.
메인테이너의 판단이 필요한 지점
너의 추천 닫지 않는다. 체크리스트 네 칸을 채우고 draft를 푼 뒤에 머지한다. 제품 코드를 이 PR에 섞지 않는다. #3032와 순서를 묶지 않는다. types/config 분할 무효화 해당 없음. 중복 닫기 해당 없음. 프리뷰 배포는 계획에 없다. 이 댓글은 grok-bot이 작성했습니다 |
|
Thanks — the checklist is filled and the gate has taken this out of draft, so the first judgment point is resolved. On the two others: Windows. You are right that the body only carries a macOS positive control, and I want to be precise about what I can and cannot claim. I have no Windows host here, so I have not seen Worth noting the failure mode if that assumption is wrong: the helper throws rather than returning a sentinel, so a Windows shard would fail loudly with 5252 and 6262. Agreed, and for the reason you gave rather than as a scope cut. One correction to my own account, since it is in this PR's history. When I first hit the four |
|
The test-only change still looks correct on exact head Please rebase onto current |
Nine sites across three suites stood in for an exited process with a
hardcoded pid:
const deadPid = process.pid === 4242 ? 4243 : 4242;
The code under test asks the kernel whether that owner is still alive, so
the pid is only dead until an unrelated process happens to hold it. Then
production answers correctly, the test reads that as a miss, and the
failure looks like a defect in the feature rather than in the fixture.
That is not hypothetical. On the macOS host where this was found, pid
4242 was `liveactivitiesd`, and five tests failed together on a clean
`dev` checkout: `periodic reclaim frees abandoned temps without any
continuation access`, both `doctor reclaim wiring (end to end)` cases,
and both `status reports stale process records end to end` cases. The
three files together went 186 pass / 5 fail before this change and 191
pass / 0 fail after it, with pid 4242 still held by `liveactivitiesd`
across both runs.
The probe already existed. `tests/responses-state.test.ts` did it inline
for one test, with a comment naming this exact hazard on a shared CI
runner, while four sites in the same file and four more in
`tests/cli-status-json.test.ts` and `tests/doctor.test.ts` kept the
assumption. This lifts that probe into `tests/helpers/dead-pid.ts` and
uses it at every site, so the knowledge lives in one place rather than in
a comment beside one of nine copies.
The helper throws rather than returning a sentinel: the inline version
needed `expect(deadPid).toBeGreaterThan(0)` at its call site, and a throw
gives every caller that guarantee without repeating the assertion. ESRCH
is the only accepted answer — a successful `kill(pid, 0)` means alive and
EPERM means alive but owned by somebody else.
Other `4242` literals in the suite are injected fixture data read through
mocked accessors, never probed against the kernel, and are left alone.
Verified on macOS: bun run typecheck clean, bun run privacy:scan passed,
bun run test 16514 pass / 0 fail across 998 files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
58531a0 to
d3c3e3a
Compare
|
Rebased onto current
The readiness checklist was reset by the push, as designed; I have re-ticked it. Ready for the exact-head Cross-platform run whenever you can start it. |
|
The recut is patch-equivalent ( #3076 is the one-line |
3 participants
Summary
Nine sites across three suites stand in for an exited process with a hardcoded pid:
The code under test asks the kernel whether that owner is still alive, so the pid is only dead until an unrelated process happens to hold it. Production then answers correctly, the test reads that as a miss, and the failure looks like a defect in the feature rather than in the fixture.
That is not hypothetical. On the macOS host where this was found, pid 4242 is
liveactivitiesd, and five tests fail together on a cleandevcheckout:Responses previous_response_id state > periodic reclaim frees abandoned temps without any continuation accessdoctor reclaim wiring (end to end) > the default run reports the file and leaves it on diskdoctor reclaim wiring (end to end) > the opt-in flag removes itstatus reports stale process records end to end > a dead owner record surfaces in --json and in human outputstatus reports stale process records end to end > a fallback-port record is judged on the recorded port, not the configured oneThe probe already existed.
tests/responses-state.test.tsdid it inline for one test, with a comment naming this exact hazard on a shared CI runner, while four sites in that same file and four more intests/cli-status-json.test.tsandtests/doctor.test.tskept the assumption. This lifts that probe intotests/helpers/dead-pid.tsand uses it at every site, so the knowledge lives in one place rather than in a comment beside one of nine copies.The helper throws rather than returning a sentinel. The inline version needed
expect(deadPid).toBeGreaterThan(0)at its call site; a throw gives every caller that guarantee without repeating the assertion. ESRCH is the only accepted answer — a successfulkill(pid, 0)means alive, and EPERM means alive but owned by somebody else.Other
4242literals in the suite (proxy-liveness,local-management-capability,server-management-auth,process-control-graceful,codex-app-server-processes) are injected fixture data read through mocked accessors, never probed against the kernel, and are left alone.No production code changes.
Verification
macOS, branch on
dev@4180067b. Pid 4242 was held byliveactivitiesdthroughout, so the before/after pair is a live positive control rather than a constructed one:dev: 186 pass / 5 fail.bun run typecheck— clean, exit 0.bun run test:changed— 191 pass, 0 fail across 3 files.bun run privacy:scan— passed.bun run test— 16514 pass, 0 fail across 998 files, no runner timeout./usr/bin/grep -rn "process.pid === 4242 ? 4243 : 4242" tests— 9 matches before, 0 after.Found while verifying #3032; that PR is unaffected by this one and neither depends on the other.
Checklist
kill(pid, 0)sends no signal and the helper never writes.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit