fix(inference): surface real reason for a slow inference invocation probe - #11287
Open
yimoj wants to merge 2 commits into
Open
fix(inference): surface real reason for a slow inference invocation probe#11287yimoj wants to merge 2 commits into
yimoj wants to merge 2 commits into
Conversation
…robe The sandbox inference invocation probe reported a generic "sandbox inference invocation probe was unavailable" whenever the transport returned null, which happens when the outer exec timeout SIGTERM-kills the curl subprocess. Because curl used a fixed --max-time 90 while the status readiness timeout was 30s, a healthy-but-slow endpoint (30-90s) was killed by the outer timeout and misreported as unavailable, even though route reachability, backend, and upstream all reported healthy (#11162). - command-transport: classify a null result as timeout (ETIMEDOUT / SIGTERM) vs subprocess error via a shared classifySandboxCommandTransportFailure and report it through an optional onTransportFailure callback. Only the error code crosses the boundary; no captured output leaks. - inference-invocation-probe: surface the real reason instead of the generic message, report curl exit 28 as an endpoint timeout, and derive curl --max-time from the outer exec timeout with a 5s buffer so curl bounds the request (clean exit 28) before the outer timeout can kill it, for both the readiness (30s -> 25s) and rebuild (100s -> 95s) paths. A genuine timeout still fails, but is reported accurately. Fixes #11162 Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe inference probe now budgets curl timeouts from the outer sandbox timeout and reports classified transport, HTTP, curl, and launcher failures. Sandbox command transport exposes callbacks for timeout and subprocess failures while retaining fallback behavior. ChangesInference probe reliability
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Inference probes now reserve time for curl to return a specific result before the sandbox execution deadline, improving diagnostics for slow endpoints and transport failures without an identified merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant InferenceProbe
participant CommandTransport
participant OpenShell
participant CurlEndpoint
InferenceProbe->>CommandTransport: execute timeout-aware inference command
CommandTransport->>OpenShell: run sandbox subprocess
OpenShell->>CurlEndpoint: send curl request
CurlEndpoint-->>OpenShell: return HTTP response or curl failure
OpenShell-->>CommandTransport: return result or transport outcome
CommandTransport-->>InferenceProbe: report result or classified failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 95d25e4 in the TypeScript / code-coverage/cliThe overall line coverage in commit 95d25e4 in the Show a line coverage summary of the most impacted files.
Updated |
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Contributor
|
PR Review Advisor finished for commit |
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
1 participant
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.
Outcome
The sandbox inference invocation probe no longer reports a false "sandbox inference invocation probe was unavailable" when the route/backend/upstream are healthy but the endpoint is merely slow. Before: a healthy-but-slow endpoint was SIGTERM-killed by the outer status timeout and collapsed into a generic "unavailable" message while Phase, route reachability, and backend all read healthy. After: curl's request budget sits safely inside the outer timeout so a slow-but-healthy endpoint is served, and when the probe genuinely does fail it reports the real reason (timeout vs subprocess error vs curl exit).
Reason
curlran with a fixed--max-time 90while the readiness/status outer exec timeout was30_000ms. An endpoint responding in 30–90 s was killed by the outer timeout (ETIMEDOUT/SIGTERM) before curl returned. The transport returnednullon that kill, and the probe collapsed anynullinto the generic "was unavailable" withhttpStatus: null, discarding the real cause. A commenter confirmed a ~35 s endpoint response reproduces exactly this on the current release; the rebuild path used a longer outer timeout but curl's fixed--max-time 90could still race it.Related issues
Fixes #11162
Changes
src/lib/adapters/sandbox/command-transport.ts: addedSandboxCommandTransportFailureand a single exportedclassifySandboxCommandTransportFailurethat distinguishes a timeout (ETIMEDOUT/SIGTERM) from a subprocess error, and reports it through a new optionalonTransportFailurecallback onSandboxExecCommandOptions. Only the subprocess error code crosses the boundary (no captured output). The callback fires only when the final result isnull(including after a failed local fallback), so existing callers that pass no callback are unaffected.src/lib/actions/sandbox/inference-invocation-probe.ts: on anulltransport result the probe now surfaces the real reason (timed out after Ns/subprocess failed (<code>)) instead of the generic message; a curl-level timeout (exit 28) is reported as an endpoint timeout and other curl exits ascurl exit N.resolveInferenceInvocationMaxTimeSecondsderives curl--max-timefrom the outer exec timeout with a 5 s buffer so curl (clean exit 28) always finishes before the outer timeout can SIGTERM the subprocess — readiness 30 s →--max-time 25, rebuild 100 s →--max-time 95. A genuine timeout still fails; it is only reported accurately, never masked. The sametimeoutMsthreads through both the exec and Deep Agents Code (runOpenshell) probe paths and all four consumers (status, start,inference set-provider, rebuild preflight).--max-time < outer timeoutalignment for both budgets.New mechanism justification: the
onTransportFailurecallback is the minimal seam.executeSandboxExecCommand/executeSandboxExecCommandTransportreturnSandboxCommandResult | nullto many callers; widening the return type would ripple across unrelated call sites, so an additive optional callback that only the probe consumes keeps transport semantics unchanged for everyone else. It is protected by the new command-transport and probe unit tests.Verification
npx vitest run src/lib/actions/sandbox/inference-invocation-probe.test.ts src/lib/adapters/sandbox/command-transport.test.ts— 49 passednpm run typecheck:cli— passed for the changed files (two pre-existing plugin-artifact errors unrelated to this diff)yimoj-colossus-dev, sandboxwt11162-repro(OpenClaw, local Ollamallama3.2:1b, OpenShell 0.0.106), inducing the endpoint-slower-than-outer-timeout condition via the productionNEMOCLAW_SANDBOX_EXEC_TIMEOUT_MSoverride:node ./bin/nemoclaw.js wt11162-repro status→Inference: unreachable ... sandbox inference invocation probe was unavailable, exit 1, while route reachability + Ollama backend + auth proxy healthy and Phase Ready (false failure reproduced).... probe timed out after 100ms., exit 1 — accurate reason, no masking.node ./bin/nemoclaw.js wt11162-repro status→Inference: healthy, exit 0.npm test(broad suite) — 148 pre-existing failures in unrelated subsystems (onboarding Dockerfile patch, CI-failure classifier, blueprint runner, messaging QR, npm cache seed, docs generation, dashboard remote-bind). Every failing file fails identically on the clean base with this change stashed and rebuilt; no failing file imports the modified modules. The two modified files' tests pass fully.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit