fix(server): honor rejected identity checks during port reclaim - #4036
fix(server): honor rejected identity checks during port reclaim#4036luvs01 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: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe port reclamation logic no longer terminates allowlisted PIDs after failed OCX verification. Such holders block TCP-row cleanup until they exit or a later verification succeeds. Tests and lifecycle documentation now describe this behavior. ChangesPort reclamation verification enforcement
Priority: ⬇️ Low — Defer this port-reclamation change because it narrowly protects live holders when OCX identity checks are rejected. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to Port reclamation now protects live holders rejected by identity verification, but the new regression test has a TypeScript compilation error. The test must be corrected before merge. 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. |
|
@coderabbitai review Please review head Port reclamation now honors a rejected OCX verifier result even for a captured teardown PID. The allowlist narrows candidates but cannot override rejection. Rejected live holders also prevent TCP-row cleanup; subsequent scans can proceed after accepted verification or an observed exit. Existing positive verification, kill flags, and dead/respawn handling remain intact. The tradeoff is retaining an unverifiable legitimate holder until the bounded wait expires. The existing classification cache is unchanged, so this is not a claim of fresh process-instance verification or complete PID-reuse protection. Seven focused cases failed before the fix. The complete reclaim file now passes 28 tests / 69 assertions; ten existing graceful-stop/update-caller controls pass another 26 assertions. Process and TCP operations are mocked. Typecheck, privacy scan, diff check, and the 425-page documentation build passed. Independent read-only review found no required corrections. Full contributor CI and maintainer review are still pending. |
⏳ DRAFT
What to do
Review readiness checklist
2/4 boxes ticked. This PR stays in draft until every box above is ticked. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='29bb221c3cfad89e4920ac411c9681073929c152'
head='24c068326cb61a57babbb0a143dd62f329a17baf'
printf '%s\n' '== Commit relationship =='
git merge-base "$base" "$head"
git diff --check "$base" "$head"
printf '%s\n' '== Changed files =='
git diff --name-status "$base" "$head"
printf '%s\n' '== Diff =='
git diff --find-renames --find-copies --unified=80 "$base" "$head"
printf '%s\n' '== Candidate source and test files =='
fd -t f 'port-reclaim|process-control-graceful|update-job' . || true
printf '%s\n' '== Outlines =='
while IFS= read -r file; do
ast-grep outline "$file" --items all --view expanded || true
done < <(fd -t f 'port-reclaim|process-control-graceful|update-job' .)Length of output: 63725 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== port-reclaim implementation (numbered) =='
nl -ba src/server/port-reclaim.ts
printf '%s\n' '== focused test symbols =='
ast-grep outline tests/server/port-reclaim.test.ts --items all --view expanded || true
printf '%s\n' '== focused test cases (numbered) =='
nl -ba tests/server/port-reclaim.test.ts
printf '%s\n' '== reclaimListenPort call sites and surrounding code =='
rg -n -C 12 -P '\breclaimListenPort\s*\(' --glob '*.ts' .
printf '%s\n' '== option names at reclaim call sites =='
rg -n -C 8 -P '\b(killOcxHolders|onlyKillPids|killAllOcxOnPort|dropTcpRows)\s*:' --glob '*.ts' src tests
printf '%s\n' '== process identity verifier contract =='
rg -n -C 16 -P '\bfunction\s+verifyPidIdentity\b|\bverifyPidIdentity\s*=' --glob '*.ts' srcLength of output: 1184 🏁 Script executed: #!/bin/bash
set -euo pipefail
numbered() {
awk '{ printf "%6d\t%s\n", NR, $0 }' "$1"
}
printf '%s\n' '== port-reclaim implementation (numbered) =='
numbered src/server/port-reclaim.ts
printf '%s\n' '== port-reclaim focused tests (numbered) =='
numbered tests/server/port-reclaim.test.ts
printf '%s\n' '== reclaimListenPort call sites and surrounding code =='
rg -n -C 12 -P '\breclaimListenPort\s*\(' --glob '*.ts' .
printf '%s\n' '== option names at reclaim call sites =='
rg -n -C 8 -P '\b(killOcxHolders|onlyKillPids|killAllOcxOnPort|dropTcpRows)\s*:' --glob '*.ts' src tests
printf '%s\n' '== verifyPidIdentity definition and direct uses =='
rg -n -C 20 -P '\b(verifyPidIdentity|function\s+verifyPidIdentity)\b' --glob '*.ts' src/config src/server src/libLength of output: 130466 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='29bb221c3cfad89e4920ac411c9681073929c152'
printf '%s\n' '== Baseline reclaim loop =='
git show "${base}:src/server/port-reclaim.ts" | sed -n '180,310p'
printf '%s\n' '== Head reclaim loop =='
sed -n '177,289p' src/server/port-reclaim.ts
printf '%s\n' '== Direct termination and TCP-drop calls in reclaim module =='
rg -n -C 5 -P '\b(killFn|dropTcpFn)\s*\(' src/server/port-reclaim.ts
printf '%s\n' '== Updated test declarations =='
rg -n -P '^\s*test(?:\.each)?\(' tests/server/port-reclaim.test.ts
printf '%s\n' '== References to old allowlist-override terminology =='
rg -n -i -C 3 -P 'allowlisted.*(fail|reject)|fail.*allowlisted|trusted teardown' \
src/server/port-reclaim.ts tests/server/port-reclaim.test.ts docs-site/src/content/docs/reference/cli/lifecycle.md docs-site/src/content/docs/ko/reference/cli/lifecycle.md structure/01_runtime.md || trueLength of output: 15964
I found no actionable issues. The stricter allowlist contract is enforced in
The caller contracts remain consistent:
This was a static review. I did not run the test suite in the sandbox. ✅ Action performedReview finished.
|
리뷰 · 우선순위 62 / 80이 PR은 중지·업데이트 뒤 포트 회수( 왜 지금 점수가 60대인 이유는 실제 안전(잘못된 kill) 쪽이고 디프도 라인 port-reclaim.ts allowlisted+!isOcx kill 분기 - HEAD의 “allowlist면 verify 실패해도 kill”을 제거한 방향이 맞다. 거절된 live holder를 보호하는 쪽이 안전하다 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
24c0683 to
a4a87b7
Compare
|
@coderabbitai review Please confirm the reviewed result for current head The prior contributor CI remains failed evidence: the unchanged idleDeadline reset/postpone test was the only test failure in the macOS control. It expected four 40 ms waits to resume within successive 120 ms idle windows. The callback had fired before the assertion (432.21 ms test duration); exact per-callback timing was not logged. The primitive uses setTimeout/clearTimeout, not Date.now, and the port fixture restores its clock spy in finally. Current-head contributor CI is being run once after the base alignment. No runtime assertion or unrelated test was changed to hide this failure, and the PR remains a draft pending green CI and the maintainer's allowlist-contract judgment. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
old_base='29bb221c3cfad89e4920ac411c9681073929c152'
old_head='24c068326cb61a57babbb0a143dd62f329a17baf'
new_base='7dc7dc99e65268bc8764e19840952256b030bce9'
new_head='a4a87b70f4d865af53892733560b23b6dd23e792'
printf '%s\n' '== Commit availability and ancestry =='
git cat-file -e "${old_base}^{commit}"
git cat-file -e "${old_head}^{commit}"
git cat-file -e "${new_base}^{commit}"
git cat-file -e "${new_head}^{commit}"
printf 'new merge-base: '
git merge-base "$new_base" "$new_head"
printf 'old merge-base: '
git merge-base "$old_base" "$old_head"
printf '%s\n' '== Diff checks =='
git diff --check "$old_base" "$old_head"
git diff --check "$new_base" "$new_head"
printf '%s\n' '== Changed paths (old and new) =='
git diff --name-status "$old_base" "$old_head"
printf '%s\n' '--'
git diff --name-status "$new_base" "$new_head"
printf '%s\n' '== Patch identity =='
git diff --binary --full-index "$old_base" "$old_head" > /tmp/old.patch
git diff --binary --full-index "$new_base" "$new_head" > /tmp/new.patch
sha256sum /tmp/old.patch /tmp/new.patch
cmp -s /tmp/old.patch /tmp/new.patch && echo 'patches: byte-identical' || echo 'patches: DIFFER'
printf '%s\n' '== Per-path postimage blobs =='
while IFS= read -r path; do
old_blob=$(git rev-parse "${old_head}:${path}")
new_blob=$(git rev-parse "${new_head}:${path}")
printf '%s\t%s\t%s\t' "$path" "$old_blob" "$new_blob"
test "$old_blob" = "$new_blob" && echo identical || echo DIFFER
done < <(git diff --name-only "$new_base" "$new_head")
printf '%s\n' '== Current implementation map =='
ast-grep outline src/server/port-reclaim.ts --items all --view expanded
printf '%s\n' '== Current contract-critical code =='
rg -n -C 10 -P '\b(verifyOcxFn|foreignLive|onlyKillPids|killAllOcxOnPort|dropTcpFn|killFn)\b' src/server/port-reclaim.ts
printf '%s\n' '== Current focused test declarations =='
ast-grep outline tests/server/port-reclaim.test.ts --items all --view expanded
rg -n -C 3 -P '^\s*test(?:\.each)?\(' tests/server/port-reclaim.test.tsLength of output: 3286 🏁 Script executed: #!/bin/bash
set -euo pipefail
old_head='24c068326cb61a57babbb0a143dd62f329a17baf'
new_base='7dc7dc99e65268bc8764e19840952256b030bce9'
new_head='a4a87b70f4d865af53892733560b23b6dd23e792'
paths_file=/tmp/rebased-pr-paths.txt
git diff --name-only "$new_base" "$new_head" > "$paths_file"
printf '%s\n' '== Per-path postimage blobs =='
while IFS= read -r path; do
old_blob=$(git rev-parse "${old_head}:${path}")
new_blob=$(git rev-parse "${new_head}:${path}")
if [ "$old_blob" = "$new_blob" ]; then
printf '%s\tidentical\t%s\n' "$path" "$new_blob"
else
printf '%s\tDIFFER\told=%s\tnew=%s\n' "$path" "$old_blob" "$new_blob"
fi
done < "$paths_file"
printf '%s\n' '== Current port-reclaim relevant region =='
git show "${new_head}:src/server/port-reclaim.ts" | nl -ba | sed -n '180,300p'
printf '%s\n' '== Current focused test names =='
git show "${new_head}:tests/server/port-reclaim.test.ts" |
rg -n -C 3 -P '^\s*test(?:\.each)?\('
printf '%s\n' '== Current patch statistics =='
git diff --stat "$new_base" "$new_head"
git diff --check "$new_base" "$new_head"Length of output: 1951
The merge base is exactly The rebased patch is byte-identical to the previously reviewed patch:
Therefore, the prior static result remains valid. I found no actionable issues. The reviewed contract remains unchanged in
The sandbox did not run tests. The initial recheck script could not print the source region because The macOS ✅ Action performedReview finished.
|
2 participants
Summary
Port recovery after stop or update can terminate a live holder even when its OCX identity verifier rejects it, provided the numeric PID appeared in the earlier teardown allowlist. That fallback also lets TCP-row cleanup proceed after the termination. A failed pre-kill recheck can reach the same fallback on a later scan.
Honor verifier rejection for every live candidate, including allowlisted PIDs. The holder stays protected from termination and keeps TCP cleanup blocked for that scan. Later accepted verification or an observed exit still permits recovery. The existing explicit kill flags, successful verification path, respawn handling, and scan-failure protections remain in use.
This intentionally favors retaining an unverified holder over reclaiming its port: a legitimate leftover that cannot be verified can keep the port busy until the existing deadline. The English/Korean lifecycle reference describes that tradeoff. This change uses the existing verifier and does not introduce fresh process-instance proof or change its classification cache.
Verification
a4a87b70f4d865af53892733560b23b6dd23e792, rebased ontodev7dc7dc99e65268bc8764e19840952256b030bce9; Bun 1.4.0 on Windows. All five PR files are byte-identical to the previously reviewed24c068326; the 22 upstream commits touch none of them. Typecheck and diff check passed again after rebasing.finally, and all process/TCP operations are injected mocks.bun run test -- --timeout 60000 --parallel=1 tests/server/port-reclaim.test.ts: 28 tests / 69 assertions passed, 1.10 seconds. Coverage includes initial and subsequent rejection, different returned PIDs under both kill-all settings, later accepted verification, normal verified cleanup, scan failure, surviving holders, and observed dead/respawn transitions.tests/lib/process-control-graceful.test.tsand three selectedtests/update/update-job.test.tscases. These check the unchanged caller contracts through their mock seams.bun run typecheck,bun run privacy:scan, andgit diff --checkpassed. The verifier/cache and stop/update callers have no diff.24c068326and found no actionable issues, including the unchanged caller contracts and later-scan recovery. It confirmed the same result for current heada4a87b70f, checking identical patches and postimage blobs.24c068326failed in the unchangedtests/lib/abort-idle-deadline.test.tsreset/postpone case: the idle callback had fired before the four resets finished (432.21 ms observed for the test). The fixture assumes each 40 ms sleep resumes before its 120 ms idle deadline. The macOS control reported 21,807 pass / 40 skip / 1 fail; all other test jobs passed.idleDeadlineusessetTimeout/clearTimeout, notDate.now, and the scoped port-test clock is restored infinally. Scheduling contention is the strongest explanation; individual callback timing was not logged. This failure remains failed evidence; its focused test correction is tracked separately in test(lib): make idle deadline reset timing deterministic #4041.a4a87b70finitially failed only in Windows 5/6:codex-cli-update-zero-effect.test.tstimed out its Node launcher child after 15,010.28 ms, before the report assertions. That job reported 3,391 pass / 20 skip / 1 fail. The test, Node launcher, and CLI entry have no diff in this PR. The exact delay stage is unknown because child output was not included in the timeout assertion. On this same head, an isolated Windows run withCI=true, Bun 1.4.0, and Node 24.18.0 passed 3 tests / 21 assertions in 1.37 seconds, with the affected case taking 750 ms. After this bounded investigation, the failed Windows job was retried once; that retry is pending. No timeout or assertion was relaxed, and this remains a draft pending successful full CI.Checklist
Review readiness checklist