fix(server): honor rejected identity checks during port reclaim - #4036
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 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 describe this behavior. ChangesPort reclamation verification enforcement
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Port reclamation now preserves live holders whose OCX identity verification is rejected, avoiding unintended termination or TCP cleanup; recovery resumes after verified identity or process exit. The bounded timeout behavior is documented and covered by focused tests. 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. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
🧩 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.
|
a4a87b7 to
b93415e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
… never overrides verification
Carries pull request lidge-jun#4036 with the documentation anchor relocated so it and lidge-jun#4070 merge serially without a conflict. Runtime, tests and authorship are unchanged. Carried-from: lidge-jun#4036 Carried-from-commit: 3822eff Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
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. 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 kill flags, successful verification path, respawn handling, and scan-failure protections remain in use.
An unverified leftover can therefore keep its port busy until the existing deadline. The English/Korean lifecycle reference describes that tradeoff. The runtime structure and option documentation state that the allowlist narrows candidates and never overrides verifier rejection. This change uses the existing verifier; it does not introduce fresh process-instance proof or change its classification cache.
Verification
3822effd9e2e19ce9d6ac5ab47763636a61dbe07. The latest maintainer-authored commit adds only three option-comment lines to the previously testedb93415e7e; runtime behavior and tests are unchanged.finally.dev8026405d; currentdev04808916is four commits ahead, within the repository's ten-commit readiness allowance. Those commits touch none of this PR's five files. No rebase is needed for this readiness restoration.Checklist
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
Bug Fixes
Documentation