fix(onboard): allocate Hermes API ports past route-only reservations - #9324
Conversation
Signed-off-by: Rui Luo <ruluo@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
4 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe change distinguishes route-only reservations from durable Hermes sandboxes during API port resolution. Route-only reservations retry allocation when a port is busy. Durable sandboxes preserve ChangesHermes API port allocation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change restores correct Hermes API-port allocation for route-only reservations, with targeted behavior tests passing; no actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed draft commit 8d3094f. I found no blocking defect.
One non-blocking coverage gap remains: add a case where pendingRouteReservation is true and createdAt is present. That row is durable, so an EADDRINUSE result must propagate after one default-port attempt instead of starting fresh allocation.
Security review:
- Secrets and credentials: PASS. Port allocation does not change credential flow.
- Input validation and data sanitization: PASS. The allocator reuses the canonical route-only reservation predicate.
- Authentication and authorization: PASS. This change adds no authorization path.
- Dependencies and third-party libraries: PASS. This change adds no dependency.
- Error handling and logging: PASS. EADDRINUSE retries apply only to allocation, while durable identity collisions still propagate.
- Cryptography and data protection: PASS. This change adds no cryptographic or protected-data flow.
- Configuration and security headers: PASS. The selected port stays within the established Hermes range.
- Security testing: PASS with the non-blocking durable pending-reservation coverage gap above.
- System security: PASS. The change preserves durable sandbox identity and fixes only the pre-create route-reservation path.
Cross-issue sweep: no adjacent fix or conflict found.
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed exact commit 2c0e97ea08bef65616256e7cf51e3dea917d94ec; 35 focused Hermes port tests passed locally.
Security review:
- Secrets and credentials — PASS: no credential changes.
- Input validation — PASS: route-only reservations use the canonical registry predicate; durable rows remain distinct.
- Authentication and authorization — PASS: no access-control changes.
- Dependencies and supply chain — PASS: no dependency changes.
- Error handling and logging — PASS: transient reservations retry
EADDRINUSE; durable conflicts still propagate. - Cryptography — PASS: no cryptographic changes.
- Policy and network isolation — PASS: allocation remains within the existing Hermes API port range.
- Testing and regression safety — PASS: route-only, pending-route-with-timestamp, durable, and busy-port cases are covered.
- System security — PASS: durable sandbox identity is not silently reallocated.
The earlier failing CI shard was canceled while installing runner packages; it did not report a source-test failure.
Summary
Onboarding a second Hermes sandbox failed with
EADDRINUSEon API port 8642 even though#8577allocates per-sandbox ports in8642-8652. Provider inference writes a route-only registry row before sandbox create; the Hermes API-port allocator treated that row as durable identity and pinned the default port without the allocation retry loop. Route-only reservations now allocate like an unregistered name, so a second Hermes sandbox can take the next free API port while durable legacy rows withouthermesApiPortstill keep 8642.Related Issue
Fixes #9291
Changes
src/lib/onboard/hermes-api-port.ts: ignoreisRouteOnlySandboxReservationrows inreserveCreateSandboxHermesApiPortandresolveOnboardHermesApiPortidentity checks; allocate (with EADDRINUSE retry) instead of pinning 8642.src/lib/onboard/hermes-api-port.test.ts: cover route-only resolve -> allocate and route-only reserve -> skip busy 8642 -> 8643; keep durable{}-> 8642 behavior.Type of Change
Quality Gates
#8577already documents per-sandbox API ports in8642-8652. This restores that allocator for the onboard route-reservation path.Documentation Writer Review
no-docs-needed#8577; this PR only fixes allocator identity for route-only registry rows.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run src/lib/onboard/hermes-api-port.test.ts-> 29 passednpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes - command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Rui Luo ruluo@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests