fix(dcode): reap completed managed sessions - #6721
Conversation
|
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:
📝 WalkthroughWalkthroughManaged Deep Agents Code one-shot invocations now use the session supervisor. The supervisor detects stdin disconnects and bounds child cleanup, while unit and end-to-end checks cover headless completion, interactive disconnects, Ctrl-C termination, process cleanup, and sandbox readiness. ChangesManaged session lifecycle
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant dcode_launcher
participant dcode_session_supervisor
participant LangGraphTree
Caller->>dcode_launcher: Start headless or interactive dcode session
dcode_launcher->>dcode_session_supervisor: Launch supervised session
dcode_session_supervisor->>LangGraphTree: Run managed child tree
Caller-->>dcode_session_supervisor: Complete or disconnect
dcode_session_supervisor->>LangGraphTree: Terminate and reap descendants
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-6721.docs.buildwithfern.com/nemoclaw |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Target Results — ❌ Some tests failedRun: 29200689140
|
PR Review Advisor — Changes requestedMerge posture: Do not merge until required findings are fixed Required before merge
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.sh (1)
485-489: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider polling
sandbox_is_readyinstead of a single check.Unlike
wait_for_dcode_process_baseline, this check runs once immediately after headless completion. If sandbox status takes a moment to settle back toReady, this could flake in CI even though the sandbox eventually recovers.♻️ Proposed retry wrapper
- if sandbox_is_ready; then + local ready_deadline=$((SECONDS + PROCESS_CLEANUP_TIMEOUT)) + local is_ready=1 + while [ "$SECONDS" -lt "$ready_deadline" ]; do + if sandbox_is_ready; then + is_ready=0 + break + fi + sleep 1 + done + if [ "$is_ready" -eq 0 ]; then pass "sandbox remained Ready after headless completion" else fail_test "sandbox was not Ready after headless completion" fi🤖 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 `@test/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.sh` around lines 485 - 489, Update the post-headless-completion check around sandbox_is_ready to poll until the sandbox returns to Ready, using the existing retry or wait pattern from wait_for_dcode_process_baseline where appropriate. Only call fail_test after the polling timeout expires, while preserving the current pass/fail messages.
🤖 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.
Nitpick comments:
In `@test/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.sh`:
- Around line 485-489: Update the post-headless-completion check around
sandbox_is_ready to poll until the sandbox returns to Ready, using the existing
retry or wait pattern from wait_for_dcode_process_baseline where appropriate.
Only call fail_test after the polling timeout expires, while preserving the
current pass/fail messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc7333b8-e870-4f29-97c9-f6c8c880c8e8
📒 Files selected for processing (5)
agents/langchain-deepagents-code/dcode-launcher.shdocs/get-started/quickstart-langchain-deepagents-code.mdxtest/deepagents-code-tui-startup-check.test.tstest/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.shtest/langchain-deepagents-code-proxy-launcher.test.ts
💤 Files with no reviewable changes (1)
- agents/langchain-deepagents-code/dcode-launcher.sh
E2E Target Results — ❌ Some tests failedRun: 29200982050
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/deepagents-code-tui-startup-check.test.ts`:
- Around line 101-109: Make the disconnect test’s EOF event depend on the
simulated termination in the fake exec path. Update the Tcl fake event setup and
the fake exec procedure around `fake_closed` so `eof` is enqueued only after the
expected `kill -TERM 4242` call, or make the EOF handling fail unless
`fake_closed` is set. Apply the same requirement to the additional
disconnect-test occurrence.
🪄 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: 41162f2d-c81a-4320-a505-b53c5533d210
📒 Files selected for processing (2)
test/deepagents-code-tui-startup-check.test.tstest/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.sh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/dcode-session-supervisor.test.ts (1)
253-262: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard against leaked long-sleeping processes on harness timeout.
supervisor.wait(timeout=10)has noexcept subprocess.TimeoutExpired. If a future regression under-terminates, this raises unhandled, failing the test correctly but leavingsession.py's SIGTERM-ignoring descendant (30s sleep) running in the CI sandbox for the remainder of its sleep. Consider force-killing the tracked PIDs in afinally/exceptbefore re-raising, to bound worst-case leakage on test failure.🤖 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 `@test/dcode-session-supervisor.test.ts` around lines 253 - 262, Update the generated supervisor cleanup script around supervisor.wait(timeout=10) to catch subprocess.TimeoutExpired, force-kill all tracked PIDs before re-raising the timeout, and preserve the existing post-wait liveness check for successful termination. Ensure cleanup runs on failure so SIGTERM-ignoring descendants cannot remain sleeping in the CI sandbox.
🤖 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.
Nitpick comments:
In `@test/dcode-session-supervisor.test.ts`:
- Around line 253-262: Update the generated supervisor cleanup script around
supervisor.wait(timeout=10) to catch subprocess.TimeoutExpired, force-kill all
tracked PIDs before re-raising the timeout, and preserve the existing post-wait
liveness check for successful termination. Ensure cleanup runs on failure so
SIGTERM-ignoring descendants cannot remain sleeping in the CI sandbox.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ff7447b8-f8a8-4da0-945d-e17712b1629e
📒 Files selected for processing (2)
agents/langchain-deepagents-code/dcode-session-supervisor.pytest/dcode-session-supervisor.test.ts
E2E Target Results — ❌ Some tests failedRun: 29201297543
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ❌ Some tests failedRun: 29201625618
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.sh (1)
560-568: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject normal exit markers in disconnect mode.
The branch only requires
NEMOCLAW_TUI_DISCONNECTED; a regression could emit both that marker andNEMOCLAW_TUI_EXIT_CAPTURED:*and still pass. Mirror the unit test’s negative assertion so disconnect sessions prove they did not fall through the Ctrl-C exit path.As per path instructions, this test should validate the observable disconnect behavior without accepting a mixed terminal state.
Suggested fix
- if grep -q "NEMOCLAW_TUI_DISCONNECTED" "$plain_capture_file"; then + if grep -q "NEMOCLAW_TUI_DISCONNECTED" "$plain_capture_file" && + ! grep -q "NEMOCLAW_TUI_EXIT_CAPTURED:" "$plain_capture_file"; then🤖 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 `@test/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.sh` around lines 560 - 568, Update the termination_mode="disconnect" branch in the session validation logic to require NEMOCLAW_TUI_DISCONNECTED and reject any NEMOCLAW_TUI_EXIT_CAPTURED:* marker, mirroring the unit test’s negative assertion. Keep the existing clean-exit validation for non-disconnect modes unchanged.Source: Path instructions
🤖 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.
Outside diff comments:
In `@test/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.sh`:
- Around line 560-568: Update the termination_mode="disconnect" branch in the
session validation logic to require NEMOCLAW_TUI_DISCONNECTED and reject any
NEMOCLAW_TUI_EXIT_CAPTURED:* marker, mirroring the unit test’s negative
assertion. Keep the existing clean-exit validation for non-disconnect modes
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 791376e7-112b-4a0d-9f38-9a4c90aeb21e
📒 Files selected for processing (2)
test/deepagents-code-tui-startup-check.test.tstest/e2e/e2e-cloud-experimental/checks/10-deepagents-code-tui-startup.sh
E2E Target Results — ❌ Some tests failedRun: 29201937216
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ✅ All selected tests passedRun: 29202218652
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
E2E Target Results — ✅ All selected tests passedRun: 29202535370
|
|
PRA-1 investigation / scope note I exercised the requested production relay-disconnect case before scoping this PR to the issue's independently reproducible headless defect:
The pinned OpenShell v0.0.72 gateway bridge explains the result: client-stream EOF shuts down only the relay write half while the relay read half remains owned and waiting on the supervisor. The supervisor relay therefore retains its SSH Unix stream, so the remote PTY stays open and the in-sandbox DCode supervisor receives no SIGHUP or PTY HUP. An inactivity timeout would break legitimate idle interactive sessions and is not a safe substitute. I removed the ineffective polling workaround rather than make the test pass without the production event. The PR body now says Current evidence:
PRA-1 is valid for the full issue outcome, but it cannot be satisfied safely within NemoClaw's in-sandbox launcher on OpenShell 0.0.72. |
<!-- markdownlint-disable MD041 --> ## Summary Release-prep documentation for v0.0.82 now summarizes user-facing changes merged since v0.0.81. It also closes stale wording in the stopped-sandbox backup, snapshot-clone, Ollama selection, and custom-policy authoring guidance. ## Changes - Add the `v0.0.82` section to `docs/about/release-notes.mdx` with links to the focused user guides. - Document that snapshot clones receive a destination-owned dashboard port before destructive replacement begins. - Align `backup-all` guidance with eligible stopped Docker-driver sandboxes that NemoClaw starts temporarily. - Describe the running and stopped Ollama menu states without claiming one fixed label. - Document runtime rejection of catch-all hosts in custom policy files. ### Source summary - [#6748](#6748) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/lifecycle.mdx`, and `docs/reference/commands.mdx`: Summarize non-destructive sandbox `stop` and `start` commands. - [#6723](#6723) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/backup-restore.mdx`, and `docs/reference/commands.mdx`: Record temporary startup and cleanup for eligible stopped-sandbox backups. - [#6749](#6749) -> `docs/about/release-notes.mdx` and `docs/manage-sandboxes/backup-restore.mdx`: Document destination-owned dashboard ports for snapshot clones. - [#6764](#6764) -> `docs/about/release-notes.mdx`: Summarize installer handling of route-only onboarding placeholders. - [#6771](#6771) -> `docs/about/release-notes.mdx`, `docs/inference/set-up-vllm.mdx`, `docs/inference/choose-inference-provider.mdx`, `docs/reference/commands.mdx`, and `docs/reference/platform-support.mdx`: Summarize managed-vLLM storage gates, immutable image digests, and the explicit override boundary. - [#6759](#6759) -> `docs/about/release-notes.mdx`: Record early, actionable OpenShell gateway-port conflict diagnostics. - [#6753](#6753) -> `docs/about/release-notes.mdx` and `docs/inference/set-up-ollama.mdx`: Document truthful running and stopped Ollama menu states. - [#6776](#6776) -> `docs/about/release-notes.mdx`: Summarize proxy-independent loopback readiness checks. - [#6769](#6769) -> `docs/about/release-notes.mdx`: Record compatible endpoint and agent guidance when Chat Completions is unavailable. - [#6730](#6730) -> `docs/about/release-notes.mdx`: Summarize bounded reuse of an eligible successful Chat Completions check. - [#6768](#6768) -> `docs/about/release-notes.mdx`: Record route-reservation repair during resumed onboarding. - [#6742](#6742) -> `docs/about/release-notes.mdx`: Summarize pre-mutation resolution of secret-free sandbox create intent. - [#6721](#6721) -> `docs/about/release-notes.mdx` and `docs/get-started/quickstart-langchain-deepagents-code.mdx`: Record bounded cleanup of completed managed Deep Agents headless sessions. - [#6731](#6731) -> `docs/about/release-notes.mdx` and `docs/network-policy/customize-network-policy.mdx`: Document runtime rejection of catch-all custom-policy destinations. - [#6729](#6729) -> `docs/about/release-notes.mdx` and `docs/get-started/prerequisites.mdx`: Record the Node.js 22.19 minimum. - [#6735](#6735) -> `docs/about/release-notes.mdx` and `docs/reference/platform-support.mdx`: Summarize the Ubuntu 26.04 userspace contract without claiming pending host or live validation. - [#6775](#6775) -> `docs/about/release-notes.mdx` and `docs/resources/community-contributions.mdx`: Route independent solutions outside canonical supported-product documentation. - [#6740](#6740) -> `docs/about/release-notes.mdx`: Summarize the semantic dependency-upgrade contributor workflow. - [#6777](#6777) -> `docs/about/release-notes.mdx` and `docs/CONTRIBUTING.md`: Summarize the route-safe documentation-refactor workflow. - [#6741](#6741) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Summarize reviewed npm archive verification and audit enforcement. - [#6739](#6739) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Record the locked offline dependency graph for the managed OpenClaw WeChat runtime. - [#6737](#6737) -> `docs/about/release-notes.mdx`: Record removal of the messaging build plan from final OpenClaw and Hermes image environments. - [#6733](#6733) -> `docs/about/release-notes.mdx`: Summarize cached plugin dependency layers for source and blueprint rebuilds. ### Skipped from docs-skip - None. No commit or changed path in `v0.0.81..origin/main` matched `openclaw-sandbox-permissive.yaml` or `config-show`, and the drafted content contains none of the configured skip terms. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This is a documentation-only release-prep update; behavior is protected by the merged source PRs, and the documentation build validates the changed routes and agent variants. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — tests are not applicable for this documentation-only change. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not run for this documentation-only change. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — 0 errors; two pre-existing Fern warnings remain. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — no new pages. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated release notes with improvements to sandbox recovery, onboarding, session management, policy validation, storage checks, and system requirements. * Clarified Ollama setup instructions and status labels. * Documented safer snapshot restoration, including dedicated ports and protection against destructive failures. * Expanded `backup-all` coverage to include eligible stopped sandboxes. * Added guidance rejecting broad or catch-all network destinations in custom policies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
wscurran
added
bug-fix
3 participants
Summary
Managed Deep Agents Code headless sessions now run through the existing session supervisor, so completion reaps their DCode and LangGraph descendants instead of leaving a retained process tree. The live Deep Code check now covers real headless completion, process-count recovery, bounded sandbox readiness, and reuse.
The interactive abrupt-relay case remains open: testing against pinned OpenShell
0.0.72showed that its gateway retains the server-side relay/PTY after the client relay disappears, so the in-sandbox supervisor receives no SIGHUP, PTY HUP, or other safe disconnect signal. The issue explicitly permits fixing the UI-independent headless reproduction separately.Related Issue
Refs #6720
Changes
-nand--non-interactivesessions use the same bounded descendant cleanup as interactive sessions.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run test/dcode-session-supervisor.test.ts test/deepagents-code-tui-startup-check.test.ts test/langchain-deepagents-code-proxy-launcher.test.ts: 42 passed, 5 platform-skipped on macOS.ubuntu-repo-cloud-langchain-deepagents-coderun 29202535370: 13 passed, 0 failed in the lifecycle check; the overall selected target passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not run; the issue defines the focused lifecycle suite and branch-level live target for this scoped launcher fix.npm run docsbuilds without warnings (doc changes only) — pinned Fern check passed with 0 errors and 2 existing warnings; route and agent-variant checks passed.Signed-off-by: Julie Yaunches jyaunches@nvidia.com