Skip to content

fix(server): honor rejected identity checks during port reclaim - #4101

Open
lidge-jun wants to merge 4 commits into
devfrom
codex/pr4036-port-reclaim
Open

fix(server): honor rejected identity checks during port reclaim#4101
lidge-jun wants to merge 4 commits into
devfrom
codex/pr4036-port-reclaim

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 9, 2026

Copy link
Copy Markdown
Owner

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 stop section in docs-site/src/content/docs/reference/cli/lifecycle.md and its Korean counterpart. Each merges cleanly against dev on 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 restart section 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-tree against current dev: the original pair conflicts on the second merge, while this carry and #4070 are clean in both merge orders.

Verification

  • Remote CI on the carried source head 3822eff: Cross-platform CI 34329107886, React Doctor 34329107894.
  • The only delta from that head is the documentation relocation plus this attribution commit; git diff over src/, tests/, and structure/ is empty.
  • Local 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

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Carried-from: #4036
Carried-from-commit: 3822eff
Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com

Summary by CodeRabbit

  • Bug Fixes

    • Port recovery now requires successful OCX process verification before terminating a process or removing its TCP tracking entry.
    • Unverified live processes remain protected, preventing accidental termination and preserving the port conflict until the process exits or verification succeeds.
    • Recovery waits now honor a bounded timeout; if the port remains occupied, restart may fail until the current port holder is resolved.
  • Documentation

    • Updated CLI lifecycle and runtime documentation to explain port recovery behavior and recommended retry steps.

luvs01 and others added 4 commits September 9, 2026 09:44
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.
Carries pull request #4036 with the documentation anchor relocated so it and
#4070 merge serially without a conflict. Runtime, tests and authorship are
unchanged.

Carried-from: #4036
Carried-from-commit: 3822eff
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 9, 2026 08:31
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-09T08:35:08.599089Z c7e7589 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: dad199af-f857-488a-b1b1-40a53fe02181

📥 Commits

Reviewing files that changed from the base of the PR and between 0480891 and c7e7589.

📒 Files selected for processing (5)
  • docs-site/src/content/docs/ko/reference/cli/lifecycle.md
  • docs-site/src/content/docs/reference/cli/lifecycle.md
  • src/server/port-reclaim.ts
  • structure/01_runtime.md
  • tests/server/port-reclaim.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Port reclamation safety

Layer / File(s) Summary
Reclamation verification contract
src/server/port-reclaim.ts, structure/01_runtime.md, docs-site/src/content/docs/reference/cli/lifecycle.md, docs-site/src/content/docs/ko/reference/cli/lifecycle.md
At src/server/port-reclaim.ts:5-7, 30-40, 177-178, 234-238, termination requires the kill option, candidate allowlisting, and successful OCX verification. Rejected live holders remain protected and block TCP-row cleanup. The runtime and CLI lifecycle documentation describe the same behavior and timeout outcome.
Reclamation behavior tests
tests/server/port-reclaim.test.ts
At tests/server/port-reclaim.test.ts:1-23, 316-320, 329-347, 367-376, 391-392, 531-541, 554-587, tests use a mocked clock and verify rejected candidates, PID mismatches, repeated scans, later successful verification, and deadline behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to c7e75

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)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main runtime change: rejected identity checks are honored during server port reclaim.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (3 skipped: 3 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/pr4036-port-reclaim

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 65 / 80

이 PR은 #4036의 메인테이너 캐리입니다. 런타임·테스트·저작권은 원본 헤드 3822effd9와 같고, 문서 앵커만 ocx stop 끝에서 ocx restart 절로 옮겼습니다. 지금 dev HEAD 04808916csrc/server/port-reclaim.ts에는 아직 “allowlist에 있으면 verify 실패해도 kill” 분기가 있습니다. 살아 있는 후보가 verifyOcxFn(pid) === pid에 실패해도 onlyKillPids에 들어 있으면 kill을 시도하고, 그 뒤 Windows TCP 행 정리까지 이어질 수 있습니다. 파일 머리 주석도 “trusted teardown PIDs, including allowlisted holders that fail ocx revalidate”라고 그 구멍을 인정합니다.

이 캐리가 하는 일은 그 분기를 지우고, 거절된 살아 있는 holder는 죽이지 않으며 TCP cleanup도 막는다로 바꾸는 것입니다. allowlist는 후보를 좁힐 뿐 신원 증거가 아닙니다. 같은 PID가 나중에 검증을 통과하거나 프로세스가 실제로 죽으면 기존 스캔 루프가 다시 회수를 시도합니다. 호출부는 그대로입니다. stop 경로는 src/lib/process-control.ts가 중지한 PID를 onlyKillPids로 넘기고, 업데이트 경로는 src/update/index.tssrc/update/job.tskillAllOcxOnPort까지 켭니다. 검증을 통과한 ocx만 죽이는 쪽은 유지됩니다.

왜 문서만 옮겼을까요. #4036과 #4070이 둘 다 docs-site/.../reference/cli/lifecycle.md(영·한)의 ocx stop 절 끝에 단락을 붙입니다. 각각은 dev에 단독으로 깨끗하지만, 하나를 넣은 뒤 다른 하나를 넣으면 손이 갑니다. 이 브랜치는 포트 회수 단락을 ocx restart로 옮깁니다. 글자 내용은 같고, 문장도 “충돌을 풀고 재시작을 다시 시도하라”로 끝나서 restart 절이 더 맞습니다. #4070은 건드리지 않습니다. 작성자가 git merge-tree로 양쪽 순서가 깨끗하다고 확인했습니다. structure/01_runtime.md에도 “거절된 검증은 종료·TCP 삭제를 막고, allowlist는 신원 증거가 아니다”를 한 단락으로 남깁니다.

테스트는 tests/server/port-reclaim.test.ts에서 Date.now spy mock clock으로 스캔·deadline을 고정합니다. 거절 유지·TCP 미드롭·다른 verifier PID·나중에 통과하면 회수 같은 기대값이 HEAD의 “allowlist면 그래도 kill” 테스트와 반대 방향으로 바뀝니다. types.ts/config.ts 대분할과 무관해서 close-don't-rebase 대상이 아닙니다. tip은 #4098 docs fence이고 package는 2.49.0입니다. 이 픽스는 제품 tip과 별개인 lifecycle 안전 버그픽스라 60대 중반이 맞습니다. #4036은 이미 grok-bot 리뷰(우선순위 62)가 있고 아직 OPEN이니, 이 캐리가 랜딩되면 원본을 landed-via-maintainer로 닫아야 합니다.

라인 port-reclaim.ts !isOcx 분기 - HEAD의 allowlist+verify-fail kill 경로를 제거하고 foreignLive로 막아 안전 방향이 맞다
라인 onlyKillPids JSDoc - allowlist는 후보 축소만 하고 verify 거절을 뒤집지 않는다고 명시한 점이 코드와 일치한다
경로/심볼 foreignLive vs protectedOcxListener - 거절된 allowlist PID도 foreignLive로 분류된다. TCP 드롭 차단 효과는 같지만 이름은 “외국 프로세스”보다 “거절된 holder”에 가깝다. 동작 버그는 아니다
경로/심볼 lifecycle.md restart 절 - #4070과 stop 앵커 충돌을 피하려고 옮긴 선택이 타당하다. 텍스트는 재시작 재시도를 말하므로 위치도 맞다
경로/심볼 #4036 원본 - 캐리 머지 후 원본 PR을 landed-via-maintainer로 닫고 남기지 말 것. 열린 PR 수를 부풀리지 않는 기존 규칙이다다
경로/심볼 CI - 이 헤드의 Cross-platform/hygiene는 아직 pending이다. 원본 3822eff CI는 통과했다고 본문에 적혀 있으니, 이 브랜치 게이트가 초록인지 확인한 뒤 머지하면 된다

메인테이너의 판단이 필요한 지점

  • “검증 실패 PID는 절대 안 죽인다” 계약을 dev에 고정할지(이 캐리가 그 답을 yes로 고정한다)
  • Windows 업데이트/서비스 respawn에서 포트가 deadline까지 busy로 남는 체감 회귀를 감수할지
  • #4070과 어떤 순서로 넣을지(작성자 merge-tree상 양쪽 다 깨끗하다고 하니 순서 자유, 다만 원본 #4036은 이 캐리로 대체)
  • pending CI를 이 헤드에서 다시 볼지, 원본 헤드 그린만으로 충분할지

너의 추천
CI가 초록이면 머지하세요. 런타임은 #4036과 동일하고, 문서 앵커만 #4070과 직렬 머지 가능하게 고친 캐리입니다. 머지 직후 #4036에 Landed via #4101 at <commit>을 남기고 landed-via-maintainer로 닫으세요. #4070은 그대로 이어서 넣으면 됩니다.

이 댓글은 grok-bot이 작성했습니다

@github-actions github-actions Bot added the bug Something isn't working label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

lidge-jun added a commit that referenced this pull request Sep 9, 2026
lidge-jun added a commit that referenced this pull request Sep 9, 2026
lidge-jun added a commit that referenced this pull request Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants