Repository navigation
fix(platform): probe loopback ports the way runtimes bind them - #408
Conversation
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 configuration
⛔ Files ignored due to path filters (11)
📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe change adds a shared loopback TCP port availability probe and uses it in CLI and daemon port checks. It also adds a configurable fake TCP holder and tests probe results across socket configurations and connection states. ChangesLoopback TCP port checks
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established by these changes; proceed with normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 3 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 19.51% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 41 functions across 18 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the complete PR diff, the allocation and reconciliation callers, and the probe's macOS bind-conflict semantics.
- Shared availability probe: Binds IPv4 loopback and wildcard addresses independently with
SO_REUSEADDR, withoutSO_REUSEPORTor listening, and treats every probe failure as unavailable. - Allocation integration: Gateway, worker, backing-resource, DNS TCP, and foreground Gateway-port allocation use the shared probe while preserving existing assignment policies and DNS's separate UDP check.
- Diagnostics and design: Removes kernel-table listener snapshots from readiness diagnostics and documents current startup-race and persisted Gateway-port behavior.
- Acceptance coverage: Adds native separate-process socket holders and verdict snapshots for listener classes, IPv6 coexistence,
TIME_WAIT, and a killed server with an open client; also makes CLI fixtures deterministic and corrects the worker-transition port fixture.
Validation on Linux: formatting and platform Clippy passed, as did all 15 port-related state integration tests and all 3 focused unsupported-platform tests. The new socket acceptance tests are macOS-only and could not run here; the supported macOS CI lanes were still in progress at submission.
gpt-6.1-sol ｜ 𝕏
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0974b8fbf
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Assigned backing Managed Resource ports are persisted in `pv.db` per resource track. PV reuses the same port across restarts when available. If the stored port is occupied by a non-PV process, PV chooses a new free port, updates `pv.db`, restarts or reconfigures dependent runtime state, and updates PV-managed `.env` blocks during reconciliation. | ||
|
|
||
| PV does not need a separate port reservation system in v1. Reconciliation chooses a candidate free port, attempts to start the process, and if startup fails because the port was taken, chooses another free port, persists it, and retries within the same reconciliation. | ||
| PV does not need a separate port reservation system in v1. Reconciliation picks a port that passes the availability probe and starts the runtime. If another process takes the port before the runtime binds it, the runtime fails readiness with an error identifying the runtime. The next reconciliation finds the foreign listener and picks another port for Managed Resources. Persisted Gateway ports remain unchanged; the foreground `pv ports:install` command repairs their pf redirects. |
There was a problem hiding this comment.
When another process takes an already-persisted Gateway backing port, pv ports:install does not repair this condition: Database::assign_gateway_ports returns existing assignments without invoking the new availability predicate, so the command simply reloads redirects to the same occupied port. Either revalidate/reassign persisted Gateway ports during this foreground repair or document the actual recovery procedure instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 4bf0c7c as a documentation correction. Persisted Gateway ports remain fixed. The design now tells users to stop or reconfigure the process that occupies one and retry reconciliation. ports:install repairs PF redirects to the stored ports; it does not select replacement backend ports. The separate --reassign feature remains out of scope. No new test was added for this documentation-only follow-up.
c0974b8 to
4bf0c7c
Compare
|
Reviewed CodeRabbit’s documentation-coverage warning. No bulk docstring change is planned for the percentage target. The shared probe already documents its socket options and conservative failure behavior. The follow-up corrects Gateway recovery guidance in DESIGN.md; it does not add a port-reassignment feature. |
1 participant
PV now chooses ports with the same
SO_REUSEADDRbind behavior used by its runtimes. A shared platform probe checks both127.0.0.1and0.0.0.0, without listening or enablingSO_REUSEPORT. Any failed bind makes the port unavailable. The wildcard probe catches listeners a loopback-only bind could shadow; closing connections do not cause port or.envchurn.Gateway, Managed Resource, DNS TCP, and foreground Gateway-port allocation use this probe. The old local probes and unreliable listener-table readiness diagnostics are removed. The workspace uses the already-locked socket2 0.6.3, with no new dependency versions.
Separate native holder processes cover IPv4 loopback and wildcard options, dual-stack and IPv6-only listeners, bound sockets without listen, TIME_WAIT, and a killed server with a connected client. DNS UDP checks retain their own bind semantics.
The design now states the existing recovery behavior: Managed Resources can receive replacement ports during reconciliation; persisted Gateway ports stay fixed. Stop or reconfigure a process that takes one of those ports.
pv ports:installrepairs redirects to the stored ports and does not choose replacements.Validation: formatting, workspace Clippy with warnings denied, and cargo-shear pass. The combined stack passed all 1,610 normal tests on this macOS 27 Mac. The review follow-up in this PR changes documentation only, so it needs no new test. CI passed on macOS 14, 15, and 26, Linux, and Windows. The combined #403 stack also passed the full macOS 27 lane.
Based on #407; followed by #409 and #403.