fix(onboard): measure the host reuse window from collection completion - #10733
fix(onboard): measure the host reuse window from collection completion#10733Dongni-Yang wants to merge 6 commits into
Conversation
`assertOnboardHostReadiness` drove `collectHostObservations` with a clock pinned to the caller's `observedAt`, so the snapshot's `completedAt` was stamped before the host assessment it reports on had run. The reuse window then measured the assessment's own duration. A host whose Docker probes took longer than the 30-second window aged out facts that had just been gathered successfully, and `projectHostReadiness` replaced every observation and capability with `unknown`. Onboarding reported nine required capabilities as unconfirmed on a host whose Docker daemon was installed, reachable and supported. #9325 established that the window starts when a collection finishes and removed the same pinning from `collectOnboardHostReadiness`. It left the `assertOnboardHostReadiness` seam, which fresh onboarding and both resume checkpoints still use. `observedAt` is now provenance only: the collection stamps its own completion, and the caller's start time is applied to the snapshot afterwards. Snapshot reuse still fails closed. `collectAdmittedReadinessPair` re-projects a held snapshot against a later clock, and that path is unchanged. Signed-off-by: Dongni Yang <dongniy@nvidia.com>
… list When the host projection cannot evaluate a capability it files the reason as evidence, `host.probe.failure` or `host.probe.stale`, and as a warning-severity `host.probe.inconclusive` finding. Admission reports only blocking findings, and `printReadinessFailure` takes only findings, so neither reached the operator. A collapse printed a bare list of unconfirmed capabilities, which reads as a Docker fault on a host whose Docker is healthy. `assertOnboardSystemReadiness` now prints that evidence beside the capability list, matching what `assertOnboardGatewayReadiness` already does for the gateway projection. #9325 recorded the measured age and the applied window on `host.probe.stale` for this purpose; no caller printed them until now. Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
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)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughRuntime preflight now separates host observation provenance from collection completion time. Freshness checks use collection completion time. Readiness failures report filtered host or gateway evidence. Tests cover timestamp validation, timing, and diagnostics. ChangesRuntime preflight
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized onboarding timing change preserves fail-closed handling while correcting freshness measurement and adding bounded diagnostics; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant RuntimePreflight
participant HostCollection
participant GPUProbe
participant ReadinessValidation
RuntimePreflight->>HostCollection: record observedAt
HostCollection->>GPUProbe: run host and GPU probes
GPUProbe-->>HostCollection: return refreshed host facts
HostCollection-->>RuntimePreflight: return collectedAt
RuntimePreflight->>ReadinessValidation: validate observedAt and collectedAt
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes address issue [
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 25bee42 in the TypeScript / code-coverage/cliThe overall line coverage in commit 25bee42 in the Show a line coverage summary of the most impacted files.
Updated |
Anchoring the reuse window to `collectHostObservations` alone overstated freshness on the resume path. That caller assesses the host, detects the GPU, then awaits gateway readiness before the host gate. Because it hands the gate precomputed facts, the collector stamped completion after that await, so a slow gateway collection left the host facts unbounded. `assertOnboardHostReadiness` now accepts `collectedAt`, the moment the caller's own probes finished, and applies it to the snapshot. The probes' duration is no longer charged, which is the #10670 fix; everything after them still is. Both resume checkpoints and fresh onboarding stamp it directly after their probes. Replaces the cached-resume test that pinned the superseded rule. It required rejection when `assessHost` outran the window, which production no longer does, and it asserted that inside its own stub. It now pins the split the fix depends on: the probe duration lands in `collectedAt`, the gateway wait after it. Folds `printHostReadinessEvidence` and `printGatewayReadinessEvidence` into one `printReadinessEvidence(evidence, ids)` with both id sets hoisted to module scope, and states the host-evidence comment directly. Both from advisor review. Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
Advisor findings for Behavior — blocker, accepted. Correct, and it is the sharper version of the bug this PR set out to fix. Anchoring the window to Taken as specified — Migration completion — accepted. Documentation — accepted. Comment reworded close to the suggestion. Code reduction — accepted, over dependency-use's objection. The two loops were identical apart from the id set, and the gateway set was rebuilt on every failure. Folded into Design and architecture, test design, trust, operations — no finding. No action. Verification after the changes: One note on CI: Signed-off-by: Dongni Yang dongniy@nvidia.com |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
PR Review Advisor finished for commit |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
Outcome
When successful host and GPU probes take longer than the 30-second reuse window, onboarding no longer reports every required capability as unconfirmed. It still rejects stale, invalid, or out-of-order timing evidence. The failure output now prints bounded evidence for an inconclusive host projection.
Reason
assertOnboardHostReadinessused the caller's observation-start timestamp as both provenance and collection completion. Slow probes therefore aged out the facts they had just collected. Separating those timestamps also required preserving fail-closed behavior when the clock moves backward or a caller supplies invalid timing metadata.Related issues
Closes #10670
Refs #10734
Changes
observedAtas provenance and measure host-fact freshness from collection completion.host.probe.failurepath without emitting invalid provenance.Verification
npx vitest run --project cli src/lib/onboard/fatal-runtime-preflight.test.ts src/lib/onboard/machine/handlers/preflight.test.ts— 47 passed.npx vitest run --project cli src/lib/readiness src/lib/onboard/fatal-runtime-preflight.test.ts src/lib/onboard/machine src/lib/onboard/preflight.test.ts— 1,331 passed across 85 files.npx vitest run --project integration test/automation/pull-requests/growth-guardrails.test.ts— 33 passed.npm run source-shape:check— passed with no source-shape tests and all registered exceptions valid.npm run checks:repository— passed.npm run typecheck:cli— passed.npx oxlintand Oxfmt checks on the four changed files — passed.git diff --check— passed.npm run validate:pr— passed.no-docs-needednpm run validate:prpassed.Review notes
The accepted scope is the #10670 timing defect. The platform-specific advisory wording or gating choice remains separate in #10734 and #10738.
An independent correctness, documentation, and nine-category security review passed after the timestamp-ordering repair. No live E2E is needed for this deterministic clock and projection boundary. Repository-owned review and merge gates still apply.
Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit