fix(router): verify shutdown convergence - #7663
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesModel-router shutdown
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant stopModelRouterProcess
participant HealthEndpoint
participant ProcessState
participant SignalDelivery
Caller->>stopModelRouterProcess: stop PID on port
stopModelRouterProcess->>ProcessState: verify PID owns router port
stopModelRouterProcess->>HealthEndpoint: check endpoint health
stopModelRouterProcess->>SignalDelivery: send SIGTERM
stopModelRouterProcess->>ProcessState: poll PID state
stopModelRouterProcess->>HealthEndpoint: poll endpoint state
stopModelRouterProcess->>SignalDelivery: send SIGKILL if needed
stopModelRouterProcess-->>Caller: resolve or throw shutdown error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology 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: 1 optional E2E recommendation
1 warning · 0 suggestionsWarningsWarnings do not block.
|
035e31f to
ddcc793
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/onboard/model-router-process.ts (2)
176-190: 🔒 Security & Privacy | 🔵 TrivialResidual PID-reuse window remains between the ownership recheck and the signal call.
Rechecking ownership right before SIGKILL substantially narrows the PID-reuse race this PR targets, but since
doesModelRouterProcessOwnPortandkill()are two separate OS syscalls, there is no true atomicity guarantee — the OS could still reap and reuse the PID in the (very small) gap between them. Worth noting as a known residual limitation of PID+signal-based shutdown; a fully atomic guarantee would require something like Linuxpidfd-based signaling, which is likely out of scope here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/model-router-process.ts` around lines 176 - 190, Document the residual PID-reuse limitation in the shutdown flow after the doesModelRouterProcessOwnPort recheck and before SIGKILL: ownership validation and kill remain separate syscalls, so they cannot provide atomic protection against PID reuse. Note that pidfd-based signaling would be required for a fully atomic guarantee, while keeping the existing behavior unchanged.
160-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "send signal, then poll for convergence" logic.
The SIGTERM block (161-174) and SIGKILL block (192-205) are near-identical: try/catch around
kill()that checks convergence on failure, then asleep+convergence poll loop, differing only in the signal and attempt count. Extracting a small helper reduces duplication in an already flagged high-complexity function.♻️ Proposed helper extraction
+async function signalAndAwaitConvergence( + pid: number, + port: number, + signal: NodeJS.Signals, + attempts: number, + { isRunning, isHealthy, kill, sleep }: { + isRunning: (pid: number) => boolean; + isHealthy: (port: number, timeoutMs?: number) => Promise<boolean>; + kill: (pid: number, signal: NodeJS.Signals) => void; + sleep: (delayMs: number) => Promise<void>; + }, +): Promise<boolean> { + try { + kill(pid, signal); + } catch (error) { + if (!isRunning(pid) && !(await isHealthy(port, 1000))) return true; + throw new Error( + `Failed to send ${signal} to model router PID ${pid}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + for (let _attempt = 0; _attempt < attempts; _attempt++) { + await sleep(500); + if (!isRunning(pid) && !(await isHealthy(port, 1000))) return true; + } + return false; +} + - try { - kill(pid, "SIGTERM"); - } catch (error) { - if (!isRunning(pid) && !(await isHealthy(port, 1000))) return; - throw new Error( - `Failed to send SIGTERM to model router PID ${pid}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - } - for (let _attempt = 0; _attempt < 10; _attempt++) { - await sleep(500); - if (!isRunning(pid) && !(await isHealthy(port, 1000))) return; - } + if (await signalAndAwaitConvergence(pid, port, "SIGTERM", 10, { isRunning, isHealthy, kill, sleep })) + return;Apply the same substitution to the SIGKILL block with
attempts = 5.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/model-router-process.ts` around lines 160 - 205, Extract the duplicated signal-and-poll flow from the shutdown function into a helper that accepts the PID, port, signal, and poll-attempt count. Preserve the existing kill-error convergence checks and error messages, then replace the SIGTERM block with 10 attempts and the SIGKILL block with 5 attempts while keeping the ownership validation between them.src/lib/onboard/model-router-process.test.ts (1)
71-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the untested fail-closed branches.
This suite covers ownership drift, SIGTERM failure, and SIGKILL escalation well, but two safety-relevant branches in
stopModelRouterProcesshave no direct test:
- The initial
!isRunning(pid)fork (source lines 144-149): neither "already stopped + port also unhealthy → silent success" nor "already stopped + port still healthy → refuse to replace" is exercised.- A SIGKILL-stage signal-delivery failure (source lines 194-201), mirroring the existing SIGTERM EPERM test at Lines 107-119.
Since these are exactly the "explicit shutdown failure" guarantees called out in the PR objectives, adding tests here protects against silent regressions.
✅ Example additional test cases
it("returns immediately when the process is already gone and the port is unhealthy", async () => { await expect( stopModelRouterProcess(123, 4000, { isRunning: () => false, readCommandLine: () => ROUTER_ARGS, isHealthy: async () => false, kill: () => {}, sleep: async () => {}, }), ).resolves.toBeUndefined(); }); it("refuses to replace when the process is gone but the port is still healthy", async () => { await expect( stopModelRouterProcess(123, 4000, { isRunning: () => false, readCommandLine: () => ROUTER_ARGS, isHealthy: async () => true, kill: () => {}, sleep: async () => {}, }), ).rejects.toThrow("remains healthy"); }); it("fails closed when SIGKILL cannot be delivered", async () => { await expect( stopModelRouterProcess(123, 4000, { isRunning: () => true, readCommandLine: () => ROUTER_ARGS, isHealthy: async () => true, kill: (_pid, signal) => { if (signal === "SIGKILL") throw new Error("EPERM"); }, sleep: async () => {}, }), ).rejects.toThrow("Failed to send SIGKILL"); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/model-router-process.test.ts` around lines 71 - 192, Add direct tests in the stopModelRouterProcess suite for both initial !isRunning outcomes: resolve when the endpoint is unhealthy and reject when it remains healthy. Also add a SIGKILL-stage delivery failure test using the existing owned-process setup, making kill throw only for SIGKILL and asserting the “Failed to send SIGKILL” error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/onboard/model-router-process.test.ts`:
- Around line 71-192: Add direct tests in the stopModelRouterProcess suite for
both initial !isRunning outcomes: resolve when the endpoint is unhealthy and
reject when it remains healthy. Also add a SIGKILL-stage delivery failure test
using the existing owned-process setup, making kill throw only for SIGKILL and
asserting the “Failed to send SIGKILL” error.
In `@src/lib/onboard/model-router-process.ts`:
- Around line 176-190: Document the residual PID-reuse limitation in the
shutdown flow after the doesModelRouterProcessOwnPort recheck and before
SIGKILL: ownership validation and kill remain separate syscalls, so they cannot
provide atomic protection against PID reuse. Note that pidfd-based signaling
would be required for a fully atomic guarantee, while keeping the existing
behavior unchanged.
- Around line 160-205: Extract the duplicated signal-and-poll flow from the
shutdown function into a helper that accepts the PID, port, signal, and
poll-attempt count. Preserve the existing kill-error convergence checks and
error messages, then replace the SIGTERM block with 10 attempts and the SIGKILL
block with 5 attempts while keeping the ownership validation between them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4982b810-680b-4b52-bac7-eafcca4a7042
📒 Files selected for processing (2)
src/lib/onboard/model-router-process.test.tssrc/lib/onboard/model-router-process.ts
wscurran
added
area: routing
|
@cv @NVIDIA/nemoclaw-maintainer Could you please help with the protected E2E disposition for exact head ddcc793?
I cannot authorize or repair that trusted-run path from the fork. I will keep this head unchanged so checks are not reset again. Please either authorize/retrigger the repository-supported E2E path for this exact head, identify any remaining code or evidence blocker I can address, or mark the PR superseded if its outcome is no longer wanted. |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head ddcc793. The shutdown path now verifies router ownership, waits for both PID and endpoint convergence, rechecks ownership before escalation, and reports non-convergence. I found no blocking defect. Non-blocking fast follow: address the existing advisor warning with a narrowly scoped identity-stable signaling design for the check-to-signal PID-reuse window. This PR materially improves the prior unconditional numeric-PID signaling behavior. The failed E2E gate is a trusted-verdict timeout, not an attributable test failure.
Signed-off-by: Ho Lim <subhoya@gmail.com>
ddcc793 to
b269a48
Compare
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit 1fe5650bf. The ownership check at the start of stopModelRouterProcess is already the check immediately preceding SIGTERM; inserting another command-line read before the next syscall would not close the PID-reuse interval. A fully atomic guarantee requires an identity-stable primitive such as Linux pidfd signaling, which is a separate platform design. I therefore do not treat Advisor finding PRA-1 as a defect introduced by this change. The SIGKILL path correctly rechecks after the grace interval, and tests cover ownership drift, signal failure, surviving PID, surviving endpoint, and convergence.
The maintainer update from main is complete and CI is running for this commit. Approval waits for all required checks and a refreshed documentation-writer receipt. The receipt currently names an older commit and leaves its completion checkbox clear.
Signed-off-by: Ho Lim <subhoya@gmail.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
The SIGKILL escalation still has a PID-reuse race. Ownership is read and kill(pid, SIGKILL) occurs in a separate operation, so an unrelated process can receive the recycled PID after the final check. Use a PID-stable process handle where supported, or do not escalate when ownership cannot be made atomic. Add a regression that changes the owner between final validation and escalation and proves no signal reaches the replacement process. Refresh onto current main and rerun required and E2E checks.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Additionally, please review this PR description and diff for comms and documentation guidelines in WRITING.md and linked artifacts.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Addressed the PID-reuse escalation review at exact head bb05666 (fix commit 8dec1d6, followed by the current-main merge). The session stores only a numeric PID, so the stop path now sends SIGTERM and verifies PID plus endpoint convergence. If the process survives, it fails closed and does not send SIGKILL. This removes the non-atomic check-to-signal window instead of documenting it as residual risk. The new regression changes ownership immediately after the final command-line observation and proves the replacement receives no signal. Existing cases still cover initial ownership rejection, SIGTERM delivery failure, graceful convergence, ownership drift, and a surviving health endpoint. Validation on the refreshed head:
I also reviewed the changed comment, error text, and test titles against WRITING.md: each names the invariant or observable behavior, uses one shutdown term consistently, and avoids unsupported safety claims. Requesting code and documentation re-review. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Security review — latest PR commit
No blocking finding remains. Fresh GitHub checks and commit verification are still required before merge. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed latest PR commit 2db7913ffaf1ed9c6be813666ba803f55812e095. The change verifies the recorded Model Router process, removes PID-based SIGKILL, requires process and health-endpoint convergence, and preserves durable state on failure. The nine-category security review has no blocking finding. Approval remains subject to current required checks and GitHub commit verification.
Dismissed as stale: the latest commit removes PID-based SIGKILL, refuses replacement until both process and endpoint state converge, and adds PID-reuse and failure-path regression coverage.
Summary
Make Model Router replacement fail closed unless shutdown converges. NemoClaw validates the recorded process before
SIGTERM, requires the recorded PID to stop reporting as running and the health endpoint to stop returning HTTP 2xx, and refuses replacement when either condition is not met. The session stores a numeric PID, so ownership validation andSIGTERMdelivery remain separate OS operations.Changes
model-router proxycommand for the configured port beforeSIGTERM.SIGTERM, require both process and endpoint convergence before starting a replacement.SIGKILLwithout a PID-stable handle.Type of Change
Quality Gates
07faa39e86286a07a5813d041c82c25e334120d5. Eight categories pass. System Security has one nonblocking warning: numeric-PID ownership validation andSIGTERMdelivery are separate OS operations, so PID reuse can redirectSIGTERMwithout a PID-stable handle. This interval predates the PR. The change removes PID-basedSIGKILL, fails closed when shutdown does not converge, and adds negative-path tests.Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, andnpm run validate:prpassed after refreshingorigin/mainnpx vitest run --project cli src/lib/onboard/model-router-process.test.ts src/lib/onboard/model-router.test.ts src/lib/onboard/model-router-python.test.ts src/lib/onboard/routed-inference.test.ts src/lib/onboard/runtime-control-flow.test.ts(54 passed)npm run validate:pr;npm --prefix nemoclaw run build;npm run build:clinpm run docsbuilds without warnings (doc changes only)Signed-off-by: Ho Lim subhoya@gmail.com