perf(onboard): replace fixed readiness polls with deadline waits#7274
perf(onboard): replace fixed readiness polls with deadline waits#7274amata-human wants to merge 10 commits into
Conversation
amata-human
added
area: docs
|
🌿 Preview your docs: https://nvidia-preview-pr-7274.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit b886cc8 in the TypeScript / code-coverage/cliThe overall coverage in commit b886cc8 in the Show a code coverage summary of the most impacted files.
Updated |
|
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 change replaces fixed readiness polling with shared deadline-based waits, adaptive backoff, zero-budget handling, and formatted deadlines across agent, gateway, sandbox, and dashboard onboarding flows. Tests, traces, timeout messages, and readiness documentation are updated accordingly. ChangesOnboarding readiness waits
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant ReadinessWait
participant GatewayOrSandbox
participant Diagnostics
Onboarding->>ReadinessWait: create deadline-based wait options
ReadinessWait->>GatewayOrSandbox: poll readiness with adaptive intervals
GatewayOrSandbox-->>ReadinessWait: readiness or phase result
ReadinessWait-->>Onboarding: success or deadline outcome
Onboarding->>Diagnostics: record traces and failure details
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/onboard.ts (1)
2108-2158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the Docker-driver gateway readiness loop later.
This block mixes several concerns inline (child-exit tracking, endpoint registration, health/TCP checks, sandbox-bridge verification) inside the large
onboard.tsentrypoint. A dedicated module alongside the existingdocker-driver-gateway-*helpers would improve testability, matching the pattern already used forgateway-recovery.ts. As per path instructions, this repo's guidance forsrc/lib/onboard.tsis to keep PRs "thin" and mechanical and defer such moves to a later, separate change rather than doing it in this migration PR, so this is a deferred suggestion only.🤖 Prompt for 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. In `@src/lib/onboard.ts` around lines 2108 - 2158, No code change is required for this deferred suggestion; leave the Docker-driver gateway readiness loop inline in onboard.ts. Consider extracting the logic into a dedicated docker-driver-gateway helper module in a separate follow-up change.Source: Path instructions
src/lib/onboard/gateway-recovery.ts (1)
230-237: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMisleading failure message for legacy zero-interval recovery config.
formatReadinessDeadline(waitBudgetMs)is a plain ms/s formatter. For a legacyrecoveryPollInterval=0/recoveryPollCount>0config,waitBudgetMscomputes to0, so the thrown message reads"...within the configured 0ms recovery deadline"even though the loop actually ran up torecoveryPollCountimmediate probes (via thezeroBudgetAttemptspath). This can mislead an operator diagnosing a failed recovery into thinking no attempts occurred.formatGatewayHealthWaitLimit(used for the equivalent case insrc/lib/onboard.ts) already special-cases this by reporting"N immediate health probes"— consider reusing that same distinction here for consistent diagnostics.♻️ Possible approach
- throw new Error( - `Gateway '${gatewayName}' did not become ready within the configured ${formatReadinessDeadline( - waitBudgetMs, - )} recovery deadline (${recoveryPollInterval}s poll interval)`, - ); + const deadlineDescription = + recoveryPollInterval === 0 && recoveryPollCount > 0 + ? `${recoveryPollCount} immediate health ${recoveryPollCount === 1 ? "probe" : "probes"}` + : `${formatReadinessDeadline(waitBudgetMs)} recovery deadline (${recoveryPollInterval}s poll interval)`; + throw new Error( + `Gateway '${gatewayName}' did not become ready within the configured ${deadlineDescription}`, + );🤖 Prompt for 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. In `@src/lib/onboard/gateway-recovery.ts` around lines 230 - 237, Update the failure message in the gateway recovery timeout path to use the same zero-budget distinction as formatGatewayHealthWaitLimit: when recoveryPollInterval is zero and zeroBudgetAttempts applies, report the configured number of immediate probes instead of a 0ms recovery deadline. Preserve the existing deadline-and-poll-interval wording for nonzero budgets and normal configurations.src/lib/onboard/sandbox-readiness-tracing.ts (1)
375-429: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDashboard zero-budget path skips the
not_ready/terminal trace emitted by the other two waiters.When
budgetMsis0,waitOptionsisnullandreadyshort-circuits tofalsewithout ever callingwaitUntil, but noaddTraceEventis emitted here — unlikepollSandboxReady(Line 124:options.trace?.("not_ready", ...)) andwaitForCreatedSandboxReadyWithTrace(Line 256:addTraceEvent("not_ready", ...)), which both explicitly trace the zero-budget short-circuit. Only theconsole.warnfires, so operators relying on tracing to diagnose a misconfigured zero timeout for dashboard readiness get no signal.♻️ Proposed fix to trace the zero-budget short-circuit
return withDashboardReadinessTrace(sandboxName, port, timeoutSecs, () => { let attempt = 0; const waitOptions = createReadinessWaitOptions({ budgetMs, maxIntervalMs: 2_000, now: options.now, sleep: (ms) => sleep(ms / 1000), }); + if (!waitOptions) { + addTraceEvent("not_ready", { attempts: 0, deadline_ms: budgetMs }); + } const ready = waitOptions !== null && waitUntil(() => {🤖 Prompt for 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. In `@src/lib/onboard/sandbox-readiness-tracing.ts` around lines 375 - 429, Update waitForDashboardReadyWithTrace so the zero-budget path (when waitOptions is null) emits the same terminal "not_ready" trace event as the other readiness waiters before returning false. Preserve the existing probe, success, warning, and nonzero-budget behavior.
🤖 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 `@docs/reference/troubleshooting.mdx`:
- Around line 1504-1511: Correct the example in the troubleshooting guidance
following the “fail fast” instruction: update
NEMOCLAW_SANDBOX_READY_ERROR_DEBOUNCE to 1 and remove the contradictory comment
about tolerating 60 observations, while leaving the surrounding timeout guidance
unchanged.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 2108-2158: No code change is required for this deferred
suggestion; leave the Docker-driver gateway readiness loop inline in onboard.ts.
Consider extracting the logic into a dedicated docker-driver-gateway helper
module in a separate follow-up change.
In `@src/lib/onboard/gateway-recovery.ts`:
- Around line 230-237: Update the failure message in the gateway recovery
timeout path to use the same zero-budget distinction as
formatGatewayHealthWaitLimit: when recoveryPollInterval is zero and
zeroBudgetAttempts applies, report the configured number of immediate probes
instead of a 0ms recovery deadline. Preserve the existing
deadline-and-poll-interval wording for nonzero budgets and normal
configurations.
In `@src/lib/onboard/sandbox-readiness-tracing.ts`:
- Around line 375-429: Update waitForDashboardReadyWithTrace so the zero-budget
path (when waitOptions is null) emits the same terminal "not_ready" trace event
as the other readiness waiters before returning false. Preserve the existing
probe, success, warning, and nonzero-budget behavior.
🪄 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: bdd98380-a809-42cf-a56d-6a2169fe3750
📒 Files selected for processing (15)
docs/reference/commands.mdxdocs/reference/troubleshooting.mdxsrc/lib/agent/gateway-readiness.test.tssrc/lib/agent/gateway-readiness.tssrc/lib/agent/onboard.tssrc/lib/onboard.tssrc/lib/onboard/gateway-health-wait.test.tssrc/lib/onboard/gateway-health-wait.tssrc/lib/onboard/gateway-recovery.test.tssrc/lib/onboard/gateway-recovery.tssrc/lib/onboard/readiness-wait.test.tssrc/lib/onboard/readiness-wait.tssrc/lib/onboard/sandbox-readiness-tracing.test.tssrc/lib/onboard/sandbox-readiness-tracing.tssrc/lib/onboard/tracing.ts
Signed-off-by: Angel Mata <amata@nvidia.com>
Signed-off-by: Angel Mata <amata@nvidia.com>
Signed-off-by: Angel Mata <amata@nvidia.com>
|
CI blocker note: |
dfernandez365-rgb
left a comment
dfernandez365-rgb
left a comment
There was a problem hiding this comment.
Exact-head review: 44926b00efa4b96b3cded6f6d539ce112d063d95.
The deadline conversion shortens the GPU compatibility stale-row confirmation window from 2 seconds to 250 ms. waitForCreatedSandboxReadyWithTrace() deliberately receives stableReadyPolls: 2 on the compatibility recreate path because OpenShell can retain the old container's Ready row before publishing the replacement supervisor's Error -> Ready transition. On d5fa1942, the confirmation poll is separated by sleep(2). This head routes the same check through createReadinessWaitOptions() with the default 250 ms initial interval (sandbox-readiness-tracing.ts:249-253), so two reads of the same stale row can now satisfy the safeguard before re-registration becomes visible.
I reproduced the exact-head behavior with a deterministic clock and an always-Ready list: the function returned {ready:true} after a single 0.25 second sleep. The existing stability test remains green because it asserts only call counts, not the temporal invariant.
Please preserve a minimum two-second confirmation window for the compatibility stableReadyPolls path (or replace the count with an explicit stability duration), and add a regression test that proves a stale Ready row lasting longer than 250 ms cannot reach the GPU proof prematurely. The general fast first retry can remain for readiness paths that do not carry this stale-registration contract.
|
Addressed the exact-head readiness-stability finding in 3d1d911. The ordinary readiness paths still start at 250 ms and back off adaptively. The compatibility recreate path (stableReadyPolls greater than 1) now retains the original 2-second confirmation interval, and the stability regression test asserts that first delay explicitly. Local verification:
Please re-review the updated head. |
Signed-off-by: Angel Mata <amata@nvidia.com>
|
Updated head Fresh local verification on the merged head:
The prior E2E failure was a cold sandbox build exceeding the performance budget; the refreshed head now includes current @coderabbitai review |
|
✅ Action performedReview finished.
|
|
CI follow-up on refreshed head
These two remaining E2E failures are hosted-runner capacity/performance failures and are not safely addressable by this readiness-polling PR. The branch is clean, current with |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> # Conflicts: # docs/reference/commands.mdx
|
Synced the contributor branch normally to current The prior requested stability invariant is preserved at the exact head: Validation on the merged tree:
@dfernandez365-rgb please re-review the exact head when available. |
4 participants
Summary
Replace the remaining fixed-count onboarding readiness loops with fast, deadline-based polling that backs off to a capped interval. Fast systems now continue immediately, while slow systems receive the full configured deadline and timeout messages report the budget that was actually used.
Related Issue
Fixes #3768
Changes
readiness-wait.test.tsprotects immediate, retry, timeout, short-budget, and zero-budget behavior.NEMOCLAW_HEALTH_POLL_COUNTandNEMOCLAW_HEALTH_POLL_INTERVALby deriving the same total budget rather than adding another configuration path.NEMOCLAW_SANDBOX_READY_TIMEOUT. Preserve the compatibility recreate path’s original two-second stable-ready confirmation interval, with regression coverage for the timing invariant.Type of Change
Quality Gates
DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.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 unavailablevitest run --project cliover the eight affected readiness/onboard test files after merging currentmain: 81 passed, 1 skipped.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not required for this scoped runtime change. A full CLI run reached 9,546 passed and 1 skipped; two unrelatedbase-image.test.ts5-second timeouts reproduced in isolation.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Angel Mata amata@nvidia.com
Summary by CodeRabbit