fix(inference): bound OpenRouter outbound deadline before connection establishment - #7281
Conversation
…establishment The OpenRouter runtime adapter timed the outbound request with a connected-socket inactivity timer, so DNS, TCP, and TLS establishment ran unbounded and could outlive upstreamTimeoutMs. Replace it with a single total deadline started before the request, returning a redacted 504 upstream_timeout on expiry and destroying the outbound request, while a genuine transport failure stays a redacted 502. Guard the settle path against a double error side effect and drop a late upstream response after the deadline. Cover pre-connect expiry and immediate connection failure without assuming a fixed refused port. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
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:
📝 WalkthroughWalkthroughThe OpenRouter forwarder now enforces a total outbound deadline, discards late upstream responses, streams response chunks with backpressure handling, and adds coverage for connection refusal, pre-connect timeout, stalled headers, and SSE streaming. ChangesOpenRouter forwarding deadline and streaming
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdapterCaller
participant forwardOpenRouterRequest
participant ClientRequest
participant IncomingMessage
participant DownstreamResponse
AdapterCaller->>forwardOpenRouterRequest: start OpenRouter request
forwardOpenRouterRequest->>ClientRequest: create request and start total deadline
ClientRequest->>IncomingMessage: deliver upstream response
IncomingMessage->>DownstreamResponse: stream body chunks
DownstreamResponse-->>IncomingMessage: drain resumes upstream stream
forwardOpenRouterRequest-->>AdapterCaller: return completed response or 504 upstream_timeout
forwardOpenRouterRequest->>IncomingMessage: destroy late response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 770612a in the TypeScript / code-coverage/cliThe overall coverage in commit 770612a in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. Since last review: 0 prior items resolved · 0 still apply · 0 new items found E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Remove the guard branch around removing the probe server from the close-tracking array; the index is always present immediately after listen() pushes it, so the check only tripped the growth-guardrails test-linearity gate. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/lib/inference/openrouter-runtime-adapter.test.ts`:
- Line 195: Guard the removal in the test’s server cleanup flow by storing the
result of servers.indexOf(probe) and only calling splice when the index is
non-negative. Preserve the existing behavior when probe is present and avoid
modifying an unrelated entry when it is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 883813ec-4523-4521-906e-38e96d8d589f
📒 Files selected for processing (1)
src/lib/inference/openrouter-runtime-adapter.test.ts
servers.indexOf(probe) returning -1 fed splice(-1, 1), which would silently drop an unrelated tracked server if probe were ever absent. Replace the index-based removal with a filter-and-replace that is a no-op when probe is not present. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
cjagwani
left a comment
There was a problem hiding this comment.
The current revision still has the response-commit race identified by the PR Review Advisor. The total deadline remains armed after the upstream response callback writes headers and pipes the body. If upstream sends 200 headers and then stalls, deadline expiry can only destroy the downstream response because headersSent is already true; clients receive a partial 200/network error instead of the accepted redacted 504 upstream_timeout contract.
Please add the headers-then-stall regression and resolve the contract before approval. Either define the deadline as time-to-headers and clear it before committing the upstream response (with the issue/PR contract updated accordingly), or preserve a total-response deadline with a bounded design that can still emit 504 before downstream headers are committed. The latter changes buffering/streaming behavior and should be treated as a deliberate design choice, not an incidental patch.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/lib/inference/openrouter-runtime-adapter.test.ts`:
- Around line 367-394: Update the test around the upstream server in “returns a
redacted timeout when upstream sends headers and then stalls (`#7248`)” to signal
immediately after flushHeaders(), await that signal before issuing or awaiting
the client response, and use a less aggressive upstreamTimeoutMs. Preserve the
existing 504 status and upstream_timeout assertions while ensuring the test
cannot pass through the pre-connect timeout path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec209567-89ff-4fb6-ac5b-4ec766e23915
📒 Files selected for processing (2)
src/lib/inference/openrouter-runtime-adapter-forward.tssrc/lib/inference/openrouter-runtime-adapter.test.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
The requested headers-then-stall regression and redacted 504 timeout contract are implemented and verified at current head 45a4ecc. Dismissing the stale changes-requested state so required CI can be rerun.
|
Maintainer note: the trusted E2E rerun completed successfully: https://github.com/NVIDIA/NemoClaw/actions/runs/30094920704. The PR Gate still reports |
|
Follow-up: #7488 is merged, this branch is updated to current |
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical pre-tag `## v0.0.95` release entry to `docs/changelog/2026-07-24.mdx`, before the existing v0.0.94 entry. The entry summarizes approved user-visible changes merged since v0.0.94 and excludes internal-only prerequisites. ## Changes - Adds the v0.0.95 summary and detailed bullets for gateway lifecycle, recovery, state transfer, inference compatibility, sandbox security, Discord policy, and E2E evidence. - Links each user-facing theme to the most specific published documentation. - Records the release entry in the shared native changelog used by the OpenClaw, Hermes, and Deep Agents guides. Source summary: - [#7246](#7246), [#7228](#7228), [#7267](#7267), [#7489](#7489), [#7509](#7509), [#7351](#7351), and [#7290](#7290) -> `docs/changelog/2026-07-24.mdx`: Gateway authority, forward teardown and retry, managed recovery, Hermes restart recovery, scoped uninstall, and orphan-aware backup behavior. - [#7344](#7344) and [#7416](#7416) -> `docs/changelog/2026-07-24.mdx`: Atomic SQLite restore and host download verification. - [#7476](#7476), [#7347](#7347), [#7281](#7281), [#7485](#7485), [#7491](#7491), and [#7422](#7422) -> `docs/changelog/2026-07-24.mdx`: Windows Ollama reuse, CDI fallback, bounded OpenRouter connection setup, Nemotron-3 request compatibility, and managed Deep Agents retry and provider-error behavior. - [#6884](#6884), [#7481](#7481), [#6878](#6878), [#7467](#7467), [#7502](#7502), [#7503](#7503), [#7504](#7504), and [#7486](#7486) -> `docs/changelog/2026-07-24.mdx`: Trusted base-image overrides, local rebuild images, runtime validation, config preservation, reviewed package updates, and fewer final-image payload layers. - [#7303](#7303) -> `docs/changelog/2026-07-24.mdx`: Scoped Discord application-command management. - [#7488](#7488), [#7465](#7465), [#7497](#7497), [#7464](#7464), [#7501](#7501), [#7494](#7494), and [#7493](#7493) -> `docs/changelog/2026-07-24.mdx`: Selected-test risk signals, retry cleanup, full root-image validation, direct-main Hermes setup, executed PR-gate evidence, nightly history, and runner wait reporting. - [#7447](#7447) is an internal pinned-runtime prerequisite and is intentionally excluded from canonical supported-integration documentation. - [#7370](#7370) adds maintainer-only advisory reconciliation tooling and does not change supported user behavior. - [#7495](#7495) updates existing documentation and does not add a new v0.0.95 behavior claim. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates the dated changelog structure, heading uniqueness, and published links. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/changelog/2026-07-24.mdx`; writing rules, documentation style, factual release meaning, and published links reviewed at exact head `58b02f2bf`. - Agent: Codex documentation writer reviewer <!-- docs-review-head-sha: 58b02f2 --> <!-- docs-review-agents-blob-sha: 9c9b36d --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/changelog-docs.test.ts` passed 6 tests. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — the build passed with 0 errors and 2 Fern warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new v0.0.95 changelog entry above v0.0.94. * Documented improved externally supervised gateway lifecycle ownership. * Improved snapshot restore reliability and SQLite state handling. * Tightened CLI `backup-all` behavior and host artifact verification. * Updated Windows onboarding guidance (including Ollama service reuse and CDI directory fallback). * Noted inference compatibility fixes, deeper agent failure classification, stricter base-image validation, updated Discord bot command permissions, and refined E2E release automation evidence handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Summary
The OpenRouter runtime adapter exposed
upstreamTimeoutMsas its outbound timeout, but applied it through a connected-socket inactivity timer, so DNS, TCP, and TLS establishment ran unbounded and a pre-connect attempt could outlive the configured timeout. The adapter now starts one total deadline before the request begins: expiry returns a redacted504 upstream_timeoutand destroys the outbound request, while a genuine transport failure still returns a redacted502 openrouter_runtime_error.Related Issue
Fixes #7248
Changes
src/lib/inference/openrouter-runtime-adapter-forward.ts: replaceClientRequest.setTimeoutwith a singlesetTimeoutdeadline created before the outbound request, cleared once settled. On expiry the adapter fails with504 upstream_timeoutand destroys the request without an error argument.writeHeadon an already-sent response.src/lib/inference/openrouter-runtime-adapter.test.ts: drop the fixed-port-1 assumption; add deterministic coverage for immediate connection failure (redacted 502), pre-connect deadline expiry (redacted 504 with request destroy), and late-response discard. Existing post-connect stall and mid-response abort coverage is retained.ClientRequest.setTimeoutpattern and needs the same bounding; tracked as a follow-up on that PR rather than in this change.Type of Change
Quality Gates
Documentation Writer Review
no-docs-needed770612a2; internal OpenRouter runtime-adapter behavior only. Targeted adapter tests passed 8/8 and CLI typecheck passed; full CI and trusted E2E are rerunning after the latest main update.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/inference/openrouter-runtime-adapter.test.ts— 8/8 passed;npm run typecheck:cli— passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests