refactor(api): remove Drain RPC, consolidate proto files, trim EngineError - #28
Merged
Conversation
Remove Control.Drain and its DrainRequest/DrainResponse/DrainState messages. Graceful shutdown does not need an engine-facing drain RPC: the client stops admitting (discovery unregister), lets in-flight Generate streams complete, polls GetLoad for quiescence, and cancels stragglers via gRPC stream cancellation into the engine's existing Abort path. Drop lifecycle.proto's now-unused error.proto import (only DrainResponse used EngineError; Health/Abort do not). Keep the drain-adjacent state signals that stand on their own -- HEALTH_STATE_DRAINING, supports_drain on KvConnectorInfo (validated by the sidecar for disagg connectors), and ERROR_CODE_DRAINING -- since an engine still drains on its own SIGTERM. Sync docs/api.md and the README tables. Validated with protoc and markdownlint-cli2. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Reduce the openengine/v1 file set from 10 to 8 by folding two small files into the domain file that owns their concern, and remove dead message/field surface. Package, message names, and field numbers are unchanged -- wire- and (whole-package) codegen-compatible. - Fold observability.proto into server.proto. Post-cleanup it held only load reporting (GetLoadRequest/LoadInfo/RankLoadInfo); "observability" was a misnomer. server.proto now covers static identity/capacity plus dynamic load -- everything the server reports about itself. - Fold input.proto into generation.proto, its sole remaining consumer, and delete the dead task-input types (TaskInput, MultimodalTaskInput, TaskInputType) orphaned when the non-generative task RPCs were removed. TokenIds/Modality/MediaItem move as-is. - Trim EngineError: drop `details` (never set or read by any engine/sidecar) and `retry_after_ms` (only ever absent), and the now-unused struct import. Keep `retryable` -- servers populate it (SGLang computes it from a 503) even though the sidecar does not yet read it. Sync docs/api.md and the proto README table. Validated with protoc and markdownlint-cli2. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
connorcarpenter15
marked this pull request as ready for review
July 23, 2026 15:03
connorcarpenter15
requested review from
alec-flowers,
dagil-nvidia,
nv-anants,
nvda-mesharma and
saturley-hall
as code owners
July 23, 2026 15:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #27. Two related contract cleanups in one PR:
Control.DrainRPC (+DrainRequest/DrainResponse/DrainState).Package name, message names, and field numbers are unchanged, so the consolidation is wire-compatible and transparent to whole-package codegen — only which file a message lives in changes.
1. Drain removal
Graceful shutdown doesn't need an engine-facing drain RPC. The Dynamo sidecar (the single OpenEngine client) can drain without it: stop admitting (discovery unregister), let in-flight
Generatestreams complete, pollGetLoadfor quiescence, and cancel stragglers via gRPC stream cancellation into the engine's existingAbortpath.Dropped
lifecycle.proto's now-unusederror.protoimport. Kept the drain-adjacent state signals that stand on their own —HEALTH_STATE_DRAINING,KvConnectorInfo.supports_drain(validated by the sidecar for disagg connectors), andERROR_CODE_DRAINING— since an engine still drains on its own SIGTERM.2. Consolidation
observability.proto→server.proto. After the runtime-events removal it held only load reporting (GetLoadRequest/LoadInfo/RankLoadInfo) — "observability" had become a misnomer. Load is the dynamic half of whatserver.protoalready reports (static identity/capacity/roles).input.proto→generation.proto, its sole remaining consumer.TokenIds/Modality/MediaItemmove as-is.3. Dead-code / field trim
TaskInput,MultimodalTaskInput,TaskInputType) — orphaned when Embed/Classify/Score were removed. Deleted.EngineErrorfields — droppeddetails(never set or read) andretry_after_ms(only ever absent), plus the now-unusedstructimport. Keptretryable: all three servers populate it (SGLang computes it from a503) even though the sidecar doesn't yet read it — removing it would discard a real signal, not dead code.Result
Control surface:
GetServerInfo·GetModelInfo·GetLoad·Health·Abort·LoadLora·UnloadLora·ListLoras·GetKvEventSources·SubscribeKvEvents(+Inference.Generate).File set 10 → 8:
openengine·generation(+input) ·kv·model·server(+observability/load) ·lifecycle·lora·error.Validation
protoc -I proto --experimental_allow_proto3_optionalbuilds the full descriptor set clean.markdownlint-cli2clean on all Markdown.Consumer follow-up (separate repos, not this PR)
drain()override and the three servers'Drainhandlers are removed;LLMEngine::drainreverts to the default no-op and shutdown relies on theis_quiescent/GetLoad+ grace-period + stream-cancellation path already inWorker.details/retry_after_ms(no reader exists, so nothing breaks);retryablehandling is unchanged.Compatibility
Wire-breaking for the removed RPC and the two
EngineErrorfields (which no consumer reads). The file consolidation itself changes no wire format.