Repository navigation
fix: preserve daemon worker routes on request timeouts - #1181
Merged
Merged
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
github-actions
Bot
added
size:S
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (14)
🧰 Additional context used📓 Path-based instructions (6)Review documentation for technical accuracy against the current API, command correctness, and consistency across language bindings.⚙️ CodeRabbit configuration file Files:
Tests should cover the behavior promised by the changed API surface, including error paths and cross-request isolation where relevant.⚙️ CodeRabbit configuration file Files:
Source excerpt: In MDX files, top-of-file comments must use JSX comment delimiters: `{/*` to open and `*/}` to close.📄 CodeRabbit inference engine (.agents/skills/contribute-docs/SKILL.md) Files:
Source excerpt: Verify MDX files use JSX delimiters for top-of-file SPDX comments.📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md) Files:
Source excerpt: Search documentation source for references to the old version and update current-version install commands, package examples, and configuration examples to `` where appropriate: Review matches before changing th...📄 CodeRabbit inference engine (.agents/skills/prepare-code-freeze/SKILL.md) Files:
Source excerpt: Preserve MDX front matter and the JSX SPDX comment.📄 CodeRabbit inference engine (.agents/skills/draft-release-notes/SKILL.md) Files:
🔇 Additional comments (7)
WalkthroughThe daemon now returns response-head timeouts without disconnecting the worker or changing its route when no route rejection occurs. Worker communication failures and staged-worker cancellations pass explicit reasons to cancellation handling. ChangesDaemon worker handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change preserves worker routes after response-header timeouts, and the added regression covers a successful subsequent request in both routing modes. No identified issue blocks merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
bbednarski9
approved these changes
Oct 2, 2026
2 tasks done
willkill07
added a commit
that referenced
this pull request
Oct 2, 2026
#### Overview Compaction can deadlock when a dynamic plugin's conditional subscriber gate snapshots a scope stack that the compaction path holds for writing. Legitimate long compaction and model/tool execution can also exceed fixed response and plugin RPC deadlines. Release the scope lock before evaluating gates and let long execution complete or end through caller cancellation. - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. #### Details - Preserve subscriber selection before marking the agent fresh, while evaluating conditional plugin callbacks without the compaction write guard. - Default daemon, worker, and standalone gateway provider waits to no deadline. Add `[upstream] response_timeout_secs` (`0` disables it), include it in the gateway bootstrap fingerprint, and document its behavior. - Remove fixed RPC deadlines from tool/LLM execution intercepts and LLM stream opening. Keep connection, startup, health, and short callback deadlines separate; preserve caller cancellation and resource cleanup. - Add a process-bounded deadlock regression and simulated hour-long wait tests covering completion, cancellation, cleanup, and explicit timeout expiry. - Build on merged #1181 (`dd86041ee5cd8a685605f57820de80db6f05e4ba`). Adapt its route-preservation regression to configure a 60-second timeout and update its deadline documentation. This PR targets the updated `main` and contains only the compaction and execution-deadline changes. Validation: `just test-rust`, `just test-python`, `just test-node`, `just docs`, and `uv run pre-commit run`. The Rust suite and staged hooks validate the combined source with #1181. Rebasing onto its squash commit preserved the tested tree exactly. Python, Node, and documentation checks passed for the compaction patch before stacking. The documentation redirect comparison was skipped because FDR returned HTTP 403; other documentation checks passed. Behavior change: execution and provider waits default to no deadline. A positive `response_timeout_secs` enables response-header limits on daemon/worker hops and idle read limits in the standalone gateway. There are no public API signature or worker protocol changes. #### Where should the reviewer start? Start with `crates/core/src/api/scope.rs` and `crates/core/tests/integration/worker_plugin_tests.rs` for the lock ordering and deadlock reproduction. Then review `crates/cli/src/configuration/types.rs`, its forwarding call sites, and `crates/core/src/plugin/dynamic/worker.rs` for timeout and cancellation behavior. The PR diff starts from the merged #1181 fix. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Closes #1180 - Relates to #1181 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an optional `response_timeout_secs` setting for provider response waits. It defaults to `0` (no deadline); positive values set a limit. In the standalone gateway, the setting also limits idle response-body reads. * Tool and LLM execution callbacks can wait for downstream work without a fixed RPC deadline; caller cancellation still ends the invocation. * **Documentation** * Updated configuration and plugin guidance to explain timeout behavior, cancellation, and the separation between provider response waits and other deadlines. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Will Killian <wkillian@nvidia.com>
This branch was successfully deployed
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
2 participants
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.
Overview
Preserve the assigned worker route when a request exceeds the daemon's 60-second response-header deadline. Previously, a slow middleware or provider request returned
504and also forced the worker to reconnect, temporarily sending subsequent requests through passthrough or returning503with--require-worker.Details
504for a response-header timeout while retaining the worker route. Transport failures, response-body failures, and explicit worker route rejection continue to trigger recovery.worker_request_failedfor the timeout and include a specific reason inworker_communication_failed.activation_expired.This PR addresses broker timeout handling and diagnostics. The runtime compaction deadlock reported in #1180 requires a separate fix.
Validation:
just test-rust: all 5,609 workspace and standalone example tests passed before rebasing onto the updated upstreammain.cargo nextest run --locked -p nemo-relay-cli --features __test-cli-port-override,__skip-implicit-config --profile ci -E 'test(daemon::broker::server::)'withNEMO_RELAY_TEST_SKIP_IMPLICIT_CONFIG=1and an isolatedXDG_CONFIG_HOME: all 94 daemon server/socket tests passed without retries. An initial run encountered a daemon trust-pin collision and passed on retry.cargo clippy --locked -p nemo-relay-cli --lib --tests --features __test-cli-port-override,__skip-implicit-config -- -D warningscargo fmt --all -- --checkandgit diff --checkjust docs: passed; Fern skipped the remote redirect comparison after HTTP403.uv run --no-sync pre-commit run: all applicable staged-change hooks passed, including documentation link checks, workspace Clippy, and workspace build checks.Where should the reviewer start?
Start with
forward_to_workerincrates/cli/src/daemon/broker/server.rsandworker_response_head_timeout_preserves_the_route_and_next_requestincrates/cli/tests/coverage/daemon/server_tests.rs. Then review cancellation-reason propagation throughcancel_staged_activationandHub::cancel_worker.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)