Repository navigation
fix(wsl): repair platform fixtures and bounded process cleanup - #12573
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/NemoClaw/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe pull request updates systemd executable-proof timeout handling and adjusts tests across onboarding, installer, inference, and package-contract code. It adds configurable command exclusions to the isolated PATH helper and uses shared timeout helpers and controlled worker deadlines in tests. ChangesSystemd executable proof
Installer system PATH isolation
Test timeout helpers
Registry worker deadline tests
Dashboard bind test
Portable Ollama file identity test
Managed llama.cpp fixture setup
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change mainly makes test fixtures more portable and makes cleanup of stuck proof processes more reliable. No concrete defect was found. Windows/WSL runner validation is still pending, as the author notes, so confirm those CI shards pass before relying on them. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit c5237ab in the Show a line coverage summary of the most impacted files.
TypeScript / code-coverage/cliThe overall line coverage in commit c5237ab in the Show a line coverage summary of the most impacted files.
Updated |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
PR Review Advisor finished for commit Request review only when Require no Advisor blockers is green. |
Align the remaining WSL subprocess fixtures with the runtime contracts they simulate. Recognize both supported GNU timeout paths, make the recovery test detect an unmatched command instead of accepting a default no-op, and honor the Podman collector caller's command timeout in the real-child fixture. No production behavior or validation assertions change. Local targeted suites passed 27 tests, fault-injection reproductions demonstrated both repairs, and all seven growth checks passed. All 57 current checks, all nine Advisor reports, and CodeRabbit review passed on 326fa28. Refs #12285, #12281, and #12573. Actual WSL validation follows in the main-only platform workflow. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
1 participant
Outcome
Repair the platform-sensitive fixtures that blocked WSL validation, and keep TERM-resistant processes inside the bounded Ollama execution proof's cleanup boundary. The direct proof selects the separately installed GNU timeout command on Ubuntu 26.04 and keeps its shell wrapper alive for KILL escalation.
Reason
WSL run 36920397162 reached all four non-live shards after the Docker-isolation repair, then reported 13 failures across seven test files. The next main run reproduced 11 of them. Live onboarding was skipped because the non-live suite failed. PR CI also exposed the same host-isolation problem in an unchanged vLLM provenance fixture: job 110684781397 reached real host-readiness discovery while checking the setup handoff.
The failures came from host Docker and WSL state leaking into fixtures, an incidental access-time comparison, startup deadlines that ignored the configured runner budget, and the direct execution proof's process supervision. Ubuntu 26.04 installs GNU's command as
gnutimeout; the unprefixed tool does not provide the required cleanup behavior in the retained run. Separately, the old shell wrapper could exit on TERM before GNU timeout escalated against its surviving children.Related issues
Refs #12285, #12281, and #12554. Target-device validation for the original onboarding report remains separate.
Changes
/usr/bin/gnutimeoutwhen installed and retain/usr/bin/timeouton older distributions. Catch TERM in the proof wrapper so its controller can escalate against the entire group. Consolidate overlapping process tests into one wrapped-command regression that rejects an outer harness timeout and verifies descendant termination, including native signal results.Verification
Provider-provenance suite — all seven tests passed. Matching setup-flow test — passed. The existing five-second limits and provenance assertions are unchanged.
Focused source, installer, package-contract, and E2E-support suites: 172 passed; eight Windows/Linux-specific cases skipped on macOS. The selected cases ran with the WSL runner's 60-second execution/test budgets. Worker coverage was also checked with default budgets after the final fixture refinement.
Shared-path regression: the prior consumer removed a synthetic Docker command from the shared path; the corrected consumer preserved it. All seven focused fault-injection cases and all 34 installer tests passed.
Installer reproduction: injecting an unavailable Docker command into the isolated host-tool mirror reproduced all six installer failures on the base; all six passed with the fixture repair.
GNU command selection regression: failed on the base when the prefixed GNU command was available, then passed after the change.
Real GNU 9.5 process reproduction on macOS: the old wrapper left a TERM-resistant child alive and required the outer guard; the repaired wrapper completed without the outer guard and the child was gone. Linux/WSL execution remains required for the native regression.
npm run build:cli— passed.Growth guardrails (all seven), signed-commit hooks, publication validation, and CLI type checking — passed. All three published commits are GitHub Verified; the latest is
c5237ab231a9036b06c19374387f79cae11adbad.Diff inspected for secrets, API keys, and credentials; none found.
Actual Windows/WSL validation is pending. The existing main-only platform workflow must run all four WSL shards and the live onboarding step before runner success can be claimed.
Review notes
Self-review covers the current eleven-file diff against
ccf68daed3aa8dc1c6bf074b9ee3d32cad684295. The runtime change is limited to GNU command selection and the proof wrapper's TERM handling. It preserves the service-user selection, existing deadlines, exit-code remapping, and failure gates. All other changes are test fixtures and coverage. All nine exact-head Advisor reports are clear in run 36964284517. PR CI, portable-profile validation, and Docker/Podman activation passed onc5237ab231a9036b06c19374387f79cae11adbad. CodeRabbit review follows draft promotion.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit