fix(server): honor rejected identity checks during port reclaim - #4101
Conversation
… never overrides verification
The paragraph landed at the end of `ocx stop`, the same anchor #4070 appends its teardown-confirmation paragraph to, so the two pull requests cannot be merged serially without a hand resolution. Nothing else in either change overlaps. `ocx restart` is where the note belongs anyway: it ends by telling the reader to retry the restart once the port conflict is resolved, and the restart section is what documents attested-PID restart behaviour. Text unchanged.
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. |
|
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; 1 remains after this review. 📝 WalkthroughWalkthroughThe change requires successful OCX verification before port-reclaim termination or TCP-row removal. Rejected live holders remain protected. Tests cover repeated scans, PID mismatches, later verification success, and timeout behavior. CLI and runtime documentation describe the updated restart behavior. ChangesPort reclamation safety
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Port recovery now preserves live processes whose OCX identity cannot be verified, preventing termination or TCP cleanup based only on a stale allowlisted PID. The documented timeout behavior matches the covered reclaim scenarios, with no remaining actionable merge risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
리뷰 · 우선순위 65 / 80이 PR은 #4036의 메인테이너 캐리입니다. 런타임·테스트·저작권은 원본 헤드 이 캐리가 하는 일은 그 분기를 지우고, 거절된 살아 있는 holder는 죽이지 않으며 TCP cleanup도 막는다로 바꾸는 것입니다. allowlist는 후보를 좁힐 뿐 신원 증거가 아닙니다. 같은 PID가 나중에 검증을 통과하거나 프로세스가 실제로 죽으면 기존 스캔 루프가 다시 회수를 시도합니다. 호출부는 그대로입니다. stop 경로는 왜 문서만 옮겼을까요. #4036과 #4070이 둘 다 테스트는 라인 port-reclaim.ts !isOcx 분기 - HEAD의 allowlist+verify-fail kill 경로를 제거하고 foreignLive로 막아 안전 방향이 맞다 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed c7e7589 against merge base 0480891. Approving the scoped port-reclaim correction.
A historical allowlist now only narrows eligible candidates; it cannot override a failed live identity check. I traced both scan-time rejection and immediate pre-kill revalidation: either protects the listener and prevents TCP-row cleanup for that scan. Dead ghost owners are still skipped, and a later accepted verification can proceed. The regressions preserve those positive controls while explicitly observing no termination/no TCP drop for rejected holders across multiple scans.
The existing verifier and its caching semantics remain unchanged; this is not new process-instance attestation. The user-facing consequence—bounded reclaim may fail with a busy port instead of killing an unverifiable holder—is documented in both lifecycle pages and the runtime contract. The carry retains source attribution and avoids the #4070 documentation anchor.
Exact-head repository CI 34329581138 has successful ci/gates and Linux/macOS jobs. The Windows full matrix was skipped; no live process termination, local reproduction or protected-home operation was performed here. This is a scoped source/contract review, not a broad security scan. Approval only; final integration remains maintainer-controlled.
|
Maintainer integration into dev at exact head |
Summary
Maintainer carry of #4036 with one documentation anchor moved, so that this change and #4070 can be merged serially without a hand resolution. Runtime, tests, and authorship are unchanged from 3822eff.
Port reclaim after a stop or update honored an allowlist path that could skip a failed OCX identity check, so a live holder whose identity was rejected could still be killed. The fix keeps a rejected live holder running and refuses the TCP-row cleanup that would follow, which means an unverifiable holder now expires the bounded recovery wait with the port still busy instead of being terminated on a stale PID match. The allowlist narrows candidates; it never overrides verification.
Why this carry exists
Both #4036 and #4070 append a paragraph at the identical anchor — the end of the
ocx stopsection indocs-site/src/content/docs/reference/cli/lifecycle.mdand its Korean counterpart. Each merges cleanly againstdevon its own, so per-PR checks report green, and the collision only appears when both are in the same tree. Merging one and then the other fails.This branch moves the port-recovery paragraph into the
ocx restartsection instead. The text is byte-identical; only its position changed. That section is a better home anyway, since the paragraph ends by telling the reader to retry the restart once the port conflict is resolved. #4070 stays untouched.Verified with
git merge-treeagainst currentdev: the original pair conflicts on the second merge, while this carry and #4070 are clean in both merge orders.Verification
git diffoversrc/,tests/, andstructure/is empty.bun run typecheck/bun run test: NOT RUN (local execution restricted in this environment). Repository CI on this branch's head is the gate.Checklist
Carried-from: #4036
Carried-from-commit: 3822eff
Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Documentation