test(ci): wait for descendant process reaping - #12214
Merged
Merged
Conversation
Wait for actual process absence after timeout and cancellation. The kernel and orphan reaper can briefly retain a killed descendant after the process group leader closes. Keep the two-second bound and all lifecycle assertions. Signed-off-by: Deepak Jain <deepujain@gmail.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. |
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe CI failure tests add ChangesProcess exit polling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to CI cleanup tests now wait for descendant processes to exit before asserting termination, reducing intermittent failures without changing production behavior. The change is mergeable. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 44b1a52 in the TypeScript / code-coverage/cliThe overall line coverage in commit 44b1a52 in the Show a line coverage summary of the most impacted files.
Updated |
Contributor
|
PR Review Advisor finished for commit Request review only when Require no Advisor blockers is green. |
deepujain
marked this pull request as ready for review
September 22, 2026 15:43
prekshivyas
approved these changes
Sep 22, 2026
prekshivyas
added a commit
that referenced
this pull request
Sep 25, 2026
## Outcome Ordinary sandbox commands, probes and diagnostics use native OpenShell execution. Failed or ambiguous commands do not retry through SSH or privileged local execution. Supported provider recovery, interactive SSH and file transfer retain their existing authority checks. ## Reason Multiple ordinary-command transports could run equivalent commands with different identities or repeat an ambiguous mutation through a more privileged path. ### Related issues Fixes #11263, part of #11255. Includes merged prerequisites #12214, #12222, #12236, #11911, #12258 and #12256. ## Changes - Remove ordinary SSH execution, compatibility wiring and privileged fallbacks. Preserve named-gateway targeting, runtime identity, filtered environments, timeouts and distinguishable failures. - Route status through the same native executor while preserving its deadline and nullable transport-failure behavior. The OpenClaw readiness probe also preserves unavailable transport evidence after integrating #12256. - Retire unsupported custom gateway SSH recovery after auditing shipped manifests. Keep supported recovery, interactive access and file transfer. - Prevent WeChat removal when orphaned physical-session cleanup cannot be confirmed; document that behavior even without a channel entry. - Refresh corporate-CA trust through native stop/start of the same sandbox, and bound/redact MCP diagnostics. - Avoid evaluating a sandbox-controlled shell file for ordinary commands, and test actual process boundaries to reject SSH retries. ## Verification Current candidate: `836320e005cdb041c28afe12a7986bd4b8f05a5a`, integrating canonical `main` at `350a9863cd83b5a8ee7932d4ab916393d7e69279`. The merge resolves the base conflict, preserves the newer bounded MCP HTTPS diagnostics, and repairs native-version CLI fixtures to emit the required sandbox-exec marker while rejecting SSH fallback. The final follow-up pins the Hermes diagnostics support test to its asserted runtime and restores the ambient environment after each case. Local evidence: 14 CLI integration tests, 94 E2E-support tests, and 72 focused transport/version/debug tests passed. CLI TypeScript passed with an 8 GiB heap ceiling; lint, formatting, all 18 repository checks, signed commit hooks, publication validation, and pre-push CLI/plugin TypeScript checks passed. Hosted CI on this exact head is green, including all 12 CLI shards, aggregate CLI, managed startup for OpenClaw/Hermes/Deep Agents Code, exact all-agent activation on Docker and rootless Podman, both Pi image builds, rootless lifecycle and portable profile, CodeQL, audits, docs, and static checks. The only red attempt was an external HTTP 429 fetching the pinned Hermes archive; its single rerun passed. The earlier managed-image failure at `ceca9ae56` was an external `ImagePullFailed` ("bytes remaining on stream"); fail-closed cleanup remained intact and the explicit OpenShell cleanup removed the sandbox. Evidence below that names another candidate or says current-head is historical for `836320e00`. Previous candidate: `3caba6799cc006bd6ee2a891719509d73f773d73`. This follows the conflict-resolution merge `908a0b4`, which integrated main `2e162f266f583d78392494feff44c360d1390ad0`, without another base integration. The follow-up preserves later diagnostics and archive creation when endpoint authority refuses sandbox-internals collection. Cancellation and unexpected errors still propagate. It replaces an obsolete nullable DeepAgents transport mock with typed failure coverage and adds public-create coverage proving corporate-CA refresh finishes before registration. All 94 tests across seven affected suites passed, along with CLI TypeScript, source-shape, lint and formatting. Independent review, signed commit hooks, isolated pre-push validation, container cleanup and actual push hooks passed. Current-head [required CI](https://github.com/NVIDIA/NemoClaw/actions/runs/35967107832), [all nine Advisor reports and aggregate](https://github.com/NVIDIA/NemoClaw/actions/runs/35968484852), substantive CodeRabbit review, [managed images](https://github.com/NVIDIA/NemoClaw/actions/runs/35967107774), and [portable rootless checks](https://github.com/NVIDIA/NemoClaw/actions/runs/35967107785) passed. Image qualification covered all three agents on Docker and rootless Podman, with 36 activation turns and 18 cleanup actions total. Five current-head manual runs passed: - [Native CPU startup](https://github.com/NVIDIA/NemoClaw/actions/runs/35969691815): all three agents on AMD64 and ARM64; cleanup verified. - [Docker onboarding](https://github.com/NVIDIA/NemoClaw/actions/runs/35971477529): three repair/resume scenarios; 28 cleanup actions. - [Standard Docker lifecycle](https://github.com/NVIDIA/NemoClaw/actions/runs/35972475425): eight scenarios; 38 cleanup actions. - [Docker MCP](https://github.com/NVIDIA/NemoClaw/actions/runs/35973673916): all three agents and the credential-generation-window test; 51 cleanup actions, including explicit successful removal of all three private relays. - [Hermes Docker lifecycle](https://github.com/NVIDIA/NemoClaw/actions/runs/35975851054): all eight phases, including restart, ACP and configuration integrity; seven cleanup actions. These results qualify the stated scopes only. Remaining Podman lifecycle prerequisites, development-runtime policy disposition and unexecuted provider/protected scopes still prevent a full review-readiness claim. At parent `908a0b4`, managed images activated all three agents on Docker and Podman: 18 turns and nine cleanup passes per runtime. Its portable rootless fixture failed an initial PID identity check before onboarding. The unchanged fixture passed ten isolated Linux cycles, but the hosted cause remains unresolved. Neither result qualifies this new candidate. ### Historical evidence The following results belong to earlier commits and do not qualify the current candidate. The transport repair moves ordinary execution and its environment wrapper into the sandbox transport adapter, retargets all callers, improves public health-outcome coverage, and gives the OpenClaw skill fixture an account-home workspace with immediate cleanup registration. - Local affected suites: 1,234 tests passed, with 14 skips; 943 additional integration tests passed, with 15 skips. The sole remaining affected-suite failure is the unchanged Hermes Python fixture on a host without PyYAML. The dependency-boundary follow-up passed 466 targeted tests. TypeScript, lint, formatting, mock/live parity, focused cleanup tests and the architecture census passed; four stale architecture limits were lowered, with no increases. Current-head hosted evidence remains required after publication. - At `81936456060c102fe1d88795ffe31f6830d39a6a`, CI passed and all nine Advisor reports were inspected. The architecture finding and CodeRabbit's startup-test duplication finding are addressed by this repair. - [Live validation at 8193645](https://github.com/NVIDIA/NemoClaw/actions/runs/35832734620) finished with 41 selected scenarios passing and 17 failing. Thirteen Podman scenarios reported incompatible or ambiguous gateway ownership; another failed rebuild preflight. The selected-gateway runtime fix is isolated in #12267 and has not yet qualified this main PR. - Other failures: Docker provider selection exceeded its 8-second budget; the protected amd64 OpenClaw normalization probe timed out; and the Docker skill fixture failed finalization. The unsafe skill workspace placement is corrected here, but the complete live failure cause has not been proved. No latency, timeout, or baseline waiver is claimed. - Of 64 ordinary cleanup receipts, 63 were clear. Skill CLI cleanup failed; its fallback sandbox deletion, gateway removal and home cleanup passed. Both protected qualification daemon-removal receipts passed. Historical results do not qualify the new commit. - At parent `85256b3`, CI and all nine Advisor reports passed. Exact managed images activated all three agents on Docker and Podman, with 18 turns and nine clean teardown actions per runtime. CodeRabbit identified two dead duplicate mock setups; the preceding test-only correction replaces them with fail-fast unexpected-call stubs and explicit zero-call assertions. All 52 focused/growth tests, source-shape and parity checks passed. The trusted matcher still requires fresh managed-image qualification for this candidate; no ancestor-image override is used. - At parent `4c4dc76`, CI and CodeRabbit passed. All nine Advisor reports were collected; one reduction finding identified an impossible null return in the native-command facade type. The current repair narrows that contract and removes unreachable direct-consumer branches, retaining explicit typed transport-error mappings and remote nonzero exit handling. 364 affected tests passed, 14 skipped; seven growth tests, TypeScript, parity, source-shape, lint and formatting passed. Docker and Podman core image activation passed at that parent, but its image workflow failed an upstream Pi Perl DNS test. No parent result clears the current commit. - Advisor additionally requires the OpenShell gateway-auth contract scenario. It remains part of the outstanding current-head E2E work. ## Review notes The merged Podman artifact renewal and full-install cleanup prerequisite are included. Remaining fixture prerequisites are #12267 (Hermes ACP Podman context) and #12269 (MCP cleanup). Full Podman lifecycle coverage remains pending. Protected GPU coverage needs the offline npm prerequisite. Observer coverage is tracked by #12238/#12197. The dev MCP lane conflicts with the supported installer channel and needs disposition. Real-provider messaging and exact staging Launchable coverage are not claimed. No human approval, gate waiver or merge-readiness claim is made. --- Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Sandbox version checks, diagnostics, and maintenance commands now use native OpenShell execution. * Managed sandbox onboarding refreshes corporate CA trust before completing setup when a CA is configured. * CLI recovery messages now direct you to relevant OpenShell commands. * **Reliability** * Channel removal stops when cleanup cannot be confirmed, rather than proceeding with an uncertain result. * Sandbox transport failures are reported without retrying through SSH or a local runtime. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Deepak Jain <deepujain@gmail.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
pull Bot
pushed a commit
to Stars1233/NemoClaw
that referenced
this pull request
Sep 26, 2026
## Outcome `nemoclaw uninstall --all-gateway-ports --yes` recovers each gateway's recorded custom OpenShell state directory without requiring the original override in the uninstall shell. The recorded directory survives interrupted creation and sandbox rebuilds. ## Reason The all-ports sweep clears another gateway's ambient state-directory override. Without a durable per-port value, it looks in the default directory and refuses cleanup because it cannot prove the namespace. ### Related issues Fixes NVIDIA#10665. ## Changes - Record the resolved custom directory in the sandbox registry and existing verified-create checkpoint. Recovery preserves that checkpoint value and rejects a conflicting explicit directory. Final registration must match the saved directory. - Rebuild uses the recorded directory from the same target snapshot as gateway preflight and onboarding when no explicit override is supplied, then restores the caller's environment. - Before any uninstall pass, validate recorded paths and incomplete-create bindings. Reject malformed or conflicting records and give each child only its matching directory. - Preserve selected-port overrides, legacy/default records and existing namespace, process, ownership and trusted-parent checks. Directory metadata alone does not authorize deletion. - Update the uninstall procedure and command reference for all existing agent variants. Older custom gateways without recorded metadata still use the explicit original-directory workaround. ## Verification - [Required CI](https://github.com/NVIDIA/NemoClaw/actions/runs/36255844785) passed on `9c022fa4971ca88370c2a0530bda47dc30bd1450`, including all 12 test shards. - [Managed-image qualification](https://github.com/NVIDIA/NemoClaw/actions/runs/36255844711) passed with Docker and rootless Podman. - [Selected branch E2E](https://github.com/NVIDIA/NemoClaw/actions/runs/36257499512) completed all 14 planned executions: 11 passed and 3 failed identically on the exact main base. No new or worse failure was observed in the selected coverage. The [failure comparison and maintainer leave-no-harm decision](NVIDIA#10774 (comment)) records the Podman restore failures and GPU image-build DNS failure. This is not an all-passing or full-release E2E result. - Local regressions reproduced and verified the repaired boundaries. The final preflight correction passed all 143 tests in six rebuild suites with existing assertions unchanged. Cross-process recovery covers new and legacy checkpoints, conflicting directories and foreign authority. - Documentation builds and all six generated guide pages were checked. Normal signed-commit, publication and CLI type checks passed; all six commits are GitHub Verified. - No secrets, credentials, live E2E assertions or workflows were added or changed. ## Review notes The main integration consumes NVIDIA#12214, which fixes the process-reaping assertion behind the original CLI shard failure. That classifier test is unchanged in this PR's net diff. CodeRabbit reviewed the final commit, reports minimal merge risk and has no unresolved threads. All nine Advisor specialists completed. The [Advisor disposition](NVIDIA#10774 (comment)) explains why a proposed gateway-wide schema migration is outside this fix: the saved directory is recovery metadata, existing runtime checks retain deletion authority, and conflicting records intentionally stop cleanup. The selected E2E exceptions follow the maintainer's explicit leave-no-harm rule. Their signatures and cleanup outcomes match main, and this PR does not change the failing maintenance-release implementation or image-build inputs. NVIDIA#12343 owns the Podman lifecycle repair. --- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> --------- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
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
2 participants
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 CI classifier's timeout and cancellation tests wait for descendant PIDs to disappear after termination. They retain a two-second bound and still fail if any tested process remains.
Reason
PR #12181 CI attempt 1 failed when it checked a descendant PID immediately after its process-group leader closed. The test and implementation are identical on that PR's base and current main. In an isolated Linux reproduction, 23 of 80 cases still exposed the PID at that instant; all disappeared by the next 10 ms observation.
Changes
Use the existing bounded-wait pattern for process absence in the timeout test and sibling cancellation cases. Keep exit-code, timeout, signal, and temporary-directory cleanup assertions. Production termination behavior is unchanged.
Verification
npx vitest run --project integration test/automation/classify-ci-failure.test.ts— 83 tests passed in isolated Linux with Node 24.18.1, no network, and no contributor-host credentials.npx vitest run --project integration test/repository/cli-coverage-sequencer.test.ts— 13 tests passed.44b1a520c3664aaecd1437c04445a9143af6d231.Review notes
The patch was self-reviewed against the unchanged classifier and its process-group behavior. The reproduction distinguishes asynchronous termination/reaping from a surviving process; the assertions still require actual absence. Fresh CI and automated review completed successfully; independent human review remains pending. No CI waiver or human approval is claimed. This is a separate test repair so PR #12181 can retain its current runtime-validation commit.
Signed-off-by: Deepak Jain deepujain@gmail.com
Latest remote validation
Commit
44b1a520c3664aaecd1437c04445a9143af6d231: CI passed. All nine Advisor specialists are clear, and the no-blockers gate passed. CodeRabbit reviewed the exact head and generated no actionable code comments. Independent human review remains pending; no human approval or merge is claimed.Summary by CodeRabbit