fix(backend): harden administrative route lifecycle - #13259
Conversation
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
9ac2a27 to
bcae8c8
Compare
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
WalkthroughChangesEngine route lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The PR hardens administrative route lifecycle behavior and reports passing validation; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🔇 Additional comments (7)
lib/backend-common/src/engine.rs (1)
337-348: LGTM!lib/backend-common/src/worker.rs (6)
261-267: LGTM!Also applies to: 346-356, 407-412, 439-442, 746-755, 1402-1407, 1423-1456, 2147-2162
1052-1127: LGTM!
1501-1701: LGTM!
3277-3290: 📐 Maintainability & Code Quality
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the local-runtime fixture APIs and the feature gate.
test_local_endpointavoids an external NATS server. The enclosing module is still gated behind theintegrationfeature, so the four new lifecycle tests do not run in a defaultcargo test. Confirm that this gating is intended. If the lifecycle tests need no external service, move them into the always-compiledtestsmodule so they run in CI.Also confirm the
dynamo-runtimeAPI shapes used here.
3348-3402: LGTM!
3524-3703: LGTM!Also applies to: 3754-3754
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2d5cb9d7-23ea-412c-8865-3bdf7be83569
📒 Files selected for processing (2)
lib/backend-common/src/engine.rslib/backend-common/src/worker.rs
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Overview:
Harden Backend SDK administrative routes so controls and updates cannot race worker startup, discovery registration, or shutdown.
This is PR 1 of 4 in the vLLM RL-control stack.
Details:
Where should the reviewer start?
Start with
EngineRouteLifecycleandwrap_engine_control_callbackinlib/backend-common/src/worker.rs, then review the four integration regressions in the same file.Validation:
cargo fmt --all -- --checkcargo test --locked -p dynamo-backend-common— 137 passedcargo clippy --locked -p dynamo-backend-common --all-targets --features integration -- -D warningscargo test --locked -p dynamo-backend-common --features integration— all four new process-local lifecycle regressions passed; five unrelated existing tests require a live NATS serverStack:
Linear: DIS-2671
Related Issues
🚫 This PR is NOT linked to a GitHub issue:
Summary by CodeRabbit
New Features
Bug Fixes