Skip to content

fix(runtime): bound graceful shutdown drain - #10705

Merged
grahamking merged 4 commits into
ai-dynamo:mainfrom
michaelfeil:mf/graceful-shutdown-timeout
Jun 18, 2026
Merged

fix(runtime): bound graceful shutdown drain#10705
grahamking merged 4 commits into
ai-dynamo:mainfrom
michaelfeil:mf/graceful-shutdown-timeout

Conversation

@michaelfeil

@michaelfeil michaelfeil commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Bound Runtime::shutdown() Phase 2 graceful endpoint draining with DYN_RUNTIME_GRACEFUL_SHUTDOWN_TIMEOUT_SECS.

Closes: #10704

Signed-off-by: Michael Feil 63565275+michaelfeil@users.noreply.github.com

Summary by CodeRabbit

  • Documentation

    • Updated graceful-shutdown documentation to clarify that endpoint/draining waits are bounded by a configurable timeout.
  • New Features

    • Added a runtime configuration for graceful-shutdown timeout (default: 15 minutes). If in-flight requests don’t finish within the timeout, the system logs how many endpoints remain and continues with shutdown.
  • Bug Fixes

    • Improved shutdown sequencing so backend-disconnect proceeds appropriately even if the graceful-drain phase times out.

Signed-off-by: Michael Feil <63565275+michaelfeil@users.noreply.github.com>
@michaelfeil
michaelfeil requested a review from a team as a code owner June 14, 2026 22:45
@michaelfeil
michaelfeil requested a review from a team June 14, 2026 22:45
@copy-pr-bot

copy-pr-bot Bot commented Jun 14, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot added external-contribution Pull request is from an external contributor fix documentation Improvements or additions to documentation labels Jun 14, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment thread lib/runtime/src/config/environment_names.rs
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Adds a configurable graceful-shutdown timeout to the Dynamo runtime via a new DYN_RUNTIME_GRACEFUL_SHUTDOWN_TIMEOUT_SECS environment variable constant, a graceful_shutdown_timeout() helper defaulting to 900s, and a tokio::time::timeout-bounded wait around endpoint draining in the shutdown task. Documentation and tests are updated accordingly.

Changes

Graceful Shutdown Timeout

Layer / File(s) Summary
Environment variable constant and validation
lib/runtime/src/config/environment_names.rs
Declares DYN_RUNTIME_GRACEFUL_SHUTDOWN_TIMEOUT_SECS constant under pub mod runtime and registers it in duplicate-name and naming-convention tests.
Timeout helper, shutdown orchestration, and tests
lib/runtime/src/runtime.rs
Defines graceful_shutdown_timeout() reading the env var with 900s fallback; wraps GracefulShutdownTracker::wait_for_completion() in tokio::time::timeout, logs remaining endpoints on timeout, and proceeds to Phase 3 teardown regardless of completion; adds unit tests covering env override, invalid-env fallback, and cancellation-token ordering during timeout.
Documentation update
docs/fault-tolerance/graceful-shutdown.md
Updates graceful-shutdown.md to describe the DYN_RUNTIME_GRACEFUL_SHUTDOWN_TIMEOUT_SECS bound (default 900s) on runtime.shutdown() endpoint draining, documents logging behavior on timeout, and rewrites the graceful_shutdown=True table entry to reflect the bounded wait.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The pull request description is minimal and lacks required sections from the template, including Overview, Details, and Where should the reviewer start. Expand the description to include Overview section describing the changes, Details section explaining what was modified, and Where should the reviewer start section highlighting key files. The Related Issues section is present but should follow the full template format.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(runtime): bound graceful shutdown drain' directly summarizes the main change - bounding the graceful shutdown drain phase with a timeout.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Jun 14, 2026
@michaelfeil
michaelfeil force-pushed the mf/graceful-shutdown-timeout branch from cdfa97e to 83cb4df Compare June 14, 2026 22:55
Signed-off-by: Michael Feil <63565275+michaelfeil@users.noreply.github.com>
@michaelfeil
michaelfeil force-pushed the mf/graceful-shutdown-timeout branch from 83cb4df to b3f5f4a Compare June 14, 2026 23:00
@michaelfeil

Copy link
Copy Markdown
Contributor Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@grahamking

Copy link
Copy Markdown
Contributor

/ok to test 3d90898

Comment thread lib/runtime/src/runtime.rs Outdated
Comment thread lib/runtime/src/runtime.rs Outdated
Comment thread lib/runtime/src/runtime.rs Outdated
Signed-off-by: Michael Feil <63565275+michaelfeil@users.noreply.github.com>
@michaelfeil

Copy link
Copy Markdown
Contributor Author

all comments addressed, the signoff should be correct without revealing my personal email. All commits were written/authored by me.

@grahamking
grahamking enabled auto-merge (squash) June 15, 2026 16:09
@grahamking

Copy link
Copy Markdown
Contributor

/ok to test 95f1634

@michaelfeil

Copy link
Copy Markdown
Contributor Author

should be all addressed, go to merge

@grahamking
grahamking merged commit 65d5e76 into ai-dynamo:main Jun 18, 2026
94 checks passed
nnshah1 added a commit that referenced this pull request Jun 30, 2026
On endpoint teardown the graceful-shutdown path waited on
`while inflight > 0 { notify.notified().await }` with no timeout. A single
stuck inflight request (e.g. a request whose engine can no longer make
progress and cannot be aborted) keeps inflight > 0, so the drain wedges, the
serve future never returns, and `Runtime::shutdown()` is never reached — the
worker zombies (Running, /health green, unable to serve).

Extract the drain into `drain_inflight`, bounded by the existing #10705
`graceful_shutdown_timeout()` (made pub(crate); no new env/const), returning
the count still inflight when the bound fires. Tested with paused time: the
bounded wait returns instead of hanging when a request never completes, and
still drains cleanly to zero when it does.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
nnshah1 added a commit that referenced this pull request Jun 30, 2026
On endpoint teardown the graceful-shutdown path waited on
`while inflight > 0 { notify.notified().await }` with no timeout, on BOTH
request planes (NATS `PushEndpoint` and the default TCP `SharedTcpServer`). A
single stuck inflight request (e.g. one whose engine can no longer make
progress and cannot be aborted) keeps inflight > 0, so the drain wedges, the
serve future never returns, and `Runtime::shutdown()` is never reached — the
worker zombies (Running, /health green, unable to serve).

Add a shared `drain_inflight` helper in the ingress module, bounded by the
existing #10705 `graceful_shutdown_timeout()` (made pub(crate); no new
env/const), and call it from both `PushEndpoint::start` and
`SharedTcpServer::unregister_endpoint`. Tested with paused time: the bounded
wait returns instead of hanging when a request never completes, and still
drains cleanly to zero when it does.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
nnshah1 added a commit that referenced this pull request Jul 1, 2026
On endpoint teardown the graceful-shutdown path waited on
`while inflight > 0 { notify.notified().await }` with no timeout, on BOTH
request planes (NATS `PushEndpoint` and the default TCP `SharedTcpServer`). A
single stuck inflight request (e.g. one whose engine can no longer make
progress and cannot be aborted) keeps inflight > 0, so the drain wedges, the
serve future never returns, and `Runtime::shutdown()` is never reached — the
worker zombies (Running, /health green, unable to serve).

Add a shared `drain_inflight` helper in the ingress module, bounded by the
existing #10705 `graceful_shutdown_timeout()` (made pub(crate); no new
env/const), and call it from both `PushEndpoint::start` and
`SharedTcpServer::unregister_endpoint`. Tested with paused time: the bounded
wait returns instead of hanging when a request never completes, and still
drains cleanly to zero when it does.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
nnshah1 added a commit that referenced this pull request Jul 6, 2026
On endpoint teardown the graceful-shutdown path waited on
`while inflight > 0 { notify.notified().await }` with no timeout, on BOTH
request planes (NATS `PushEndpoint` and the default TCP `SharedTcpServer`). A
single stuck inflight request (e.g. one whose engine can no longer make
progress and cannot be aborted) keeps inflight > 0, so the drain wedges, the
serve future never returns, and `Runtime::shutdown()` is never reached — the
worker zombies (Running, /health green, unable to serve).

Add a shared `drain_inflight` helper in the ingress module, bounded by the
existing #10705 `graceful_shutdown_timeout()` (made pub(crate); no new
env/const), and call it from both `PushEndpoint::start` and
`SharedTcpServer::unregister_endpoint`. Tested with paused time: the bounded
wait returns instead of hanging when a request never completes, and still
drains cleanly to zero when it does.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation external-contribution Pull request is from an external contributor fix size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Bound graceful endpoint drain during runtime shutdown

3 participants