fix(sandbox): clear gateway marker when the start supervisor exits#5569
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:
📝 WalkthroughWalkthroughAdds best-effort removal of ChangesGateway marker cleanup on supervisor exit
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GatewaySupervisor
participant ExitTrap
participant GatewayMarker
participant DockerHealthcheck
GatewaySupervisor->>ExitTrap: register cleanup handler
GatewaySupervisor->>GatewayMarker: maintain marker while active
GatewaySupervisor->>ExitTrap: exit or receive SIGTERM
ExitTrap->>GatewayMarker: remove marker
DockerHealthcheck->>GatewayMarker: check marker state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/nemoclaw-start-gateway-marker.test.ts (1)
337-337: ⚡ Quick winAssert the production trap wiring, not only the manual trap behavior.
These tests manually arm
trap clear_in_container_gateway_marker EXIT, so they would still pass if either supervisor path inscripts/nemoclaw-start.shstopped registering the production EXIT trap. Add a small source assertion for both trap registrations, or extract the actual supervisor snippets before running the behavioral checks.Suggested guard
it("drops the marker when the supervisor reaches a clean exit (`#4952`)", () => { const src = fs.readFileSync(START_SCRIPT, "utf-8"); + const exitTrapMatches = + src.match(/^\s*trap clear_in_container_gateway_marker EXIT$/gm) ?? []; + expect(exitTrapMatches).toHaveLength(2); const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-gw-exit-"));Also applies to: 384-384, 431-431
🤖 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/nemoclaw-start-gateway-marker.test.ts` at line 337, The test manually sets up the trap with the line containing "trap clear_in_container_gateway_marker EXIT" but does not verify that the production script scripts/nemoclaw-start.sh actually registers this trap in its supervisor paths. This means the test would pass even if the production code stopped registering the trap. Add source assertions to verify the actual trap registrations exist in the supervisor sections of the production script before running the behavioral checks, rather than relying solely on the manual trap setup in the test. Apply this assertion fix to all affected test lines (337, 384, and 431).scripts/nemoclaw-start.sh (1)
3846-3853: Run the sandbox lifecycle E2E set before merging.This entrypoint change affects supervisor lifecycle and sandbox boot paths that unit tests do not fully exercise. Please trigger the selective
nightly-e2e.yamljobs listed in the repo guidance for this file. As per coding guidelines,scripts/nemoclaw-start.shchanges affect every sandbox boot and are invisible to unit tests; recommended jobs aresandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e, andopenclaw-slack-pairing-e2e.Also applies to: 4104-4111
🤖 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 `@scripts/nemoclaw-start.sh` around lines 3846 - 3853, Before merging this change to the trap statement in nemoclaw-start.sh that adds clear_in_container_gateway_marker on EXIT, run the sandbox lifecycle E2E tests to validate the changes affect supervisor lifecycle and sandbox boot paths correctly. Trigger the selective nightly-e2e.yaml jobs: sandbox-survival-e2e, sandbox-operations-e2e, cloud-e2e, and openclaw-slack-pairing-e2e. These tests are necessary because the entrypoint changes are invisible to unit tests and affect every sandbox boot, so E2E validation is required before merging.Source: Coding guidelines
🤖 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 `@scripts/nemoclaw-start.sh`:
- Around line 3846-3853: Before merging this change to the trap statement in
nemoclaw-start.sh that adds clear_in_container_gateway_marker on EXIT, run the
sandbox lifecycle E2E tests to validate the changes affect supervisor lifecycle
and sandbox boot paths correctly. Trigger the selective nightly-e2e.yaml jobs:
sandbox-survival-e2e, sandbox-operations-e2e, cloud-e2e, and
openclaw-slack-pairing-e2e. These tests are necessary because the entrypoint
changes are invisible to unit tests and affect every sandbox boot, so E2E
validation is required before merging.
In `@test/nemoclaw-start-gateway-marker.test.ts`:
- Line 337: The test manually sets up the trap with the line containing "trap
clear_in_container_gateway_marker EXIT" but does not verify that the production
script scripts/nemoclaw-start.sh actually registers this trap in its supervisor
paths. This means the test would pass even if the production code stopped
registering the trap. Add source assertions to verify the actual trap
registrations exist in the supervisor sections of the production script before
running the behavioral checks, rather than relying solely on the manual trap
setup in the test. Apply this assertion fix to all affected test lines (337,
384, and 431).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70530c73-3f7b-48c1-b4e9-728cec39f0d7
📒 Files selected for processing (2)
scripts/nemoclaw-start.shtest/nemoclaw-start-gateway-marker.test.ts
|
Thanks for the review. EXIT-trap wiring assertion — addressed in 26c9c41. Rather than repeat the source check inside each behavioral test, I added a single contract-guard test that asserts both launch paths (non-root and root) register Sandbox lifecycle E2E — agreed these entrypoint changes warrant |
wscurran
added
area: sandbox
|
✨ Thanks for the proposed fix clearing the in-container gateway marker when the start supervisor exits on OpenShell docker-driver sandboxes. This proposes a way to add an EXIT trap that clears the stale pidfile so the HEALTHCHECK no longer trusts a dead gateway PID. Related open issues:
|
|
This PR is currently conflicted, its body lacks the required |
26c9c41 to
5042672
Compare
Clear the in-container gateway marker whenever the OpenClaw supervisor exits. That keeps Docker healthchecks from trusting stale gateway pidfiles after the supervisor is gone. Keep the marker present while the supervisor runs and across respawns. Cover the current cleanup_openclaw_on_signal path through marker behavior tests. Also require both production EXIT-trap registrations through the same behavior harness. Co-authored-by: Zhi Yan Liu <lzy.dev@gmail.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
5042672 to
e3568b9
Compare
|
@coderabbitai review |
|
Exact-head rewrite at |
✅ Action performedReview finished.
|
E2E Target Results — ✅ All selected jobs passedRun: 29104603734
|
|
Exact-head live E2E is green for |
|
Follow-up validation is now green at exact head |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Exact-head security review at e3568b9b10f792af5768301973131d25c18d78b2: changes required before approval.
The marker is created before EXIT and SIGINT/SIGTERM cleanup handlers are armed in both supervisor paths (scripts/nemoclaw-start.sh:4791-4804 and :5410-5442, with the non-root signal trap later still). Early startup exits or a signal in that window can leave /tmp/nemoclaw-gateway-local behind and recreate the stale-health condition this PR is meant to fix.
Please arm cleanup before the first marker write in both paths and add focused root/non-root regressions that fail or signal immediately after marking and prove the marker is removed. Ordinary CI and live E2E are green, but they do not exercise this ordering window; the GPT-5.5 advisor independently identified the same blocker.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Exact-head maintainer follow-up at e3568b9b10: ordinary CI is green, DCO and commit verification pass, and Nemotron found no blocker. The trusted GPT-5.5 advisor run 29110341655 found one valid correctness gap before live validation.
Both supervisor paths can create /tmp/nemoclaw-gateway-local before the new EXIT cleanup trap is armed. In the non-root path, mark_in_container_gateway is followed by capture_openclaw_pid_start_identity ... || exit 1, while the trap is installed later. In the root path, launch_openclaw_gateway marks and can exit on the same identity-capture failure before its caller installs the trap. On a docker-driver sandbox where the container survives this supervisor exit, that leaves exactly the stale marker and stale pidfile state this PR is intended to fix.
Please install the cleanup trap before any path can write the marker, while preserving any existing EXIT cleanup contract, and add focused regressions that simulate post-marker identity-capture failure in both the non-root and root launch shapes and assert the marker is removed.
After the fix, refresh exact-head CI and both advisors. E2E-advisor run 29110343190 requires issue-2478-crash-loop-recovery, sandbox-survival, gateway-health-honest, and target ubuntu-repo-cloud-openclaw.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Exact-head follow-up at One exact source cleanup remains: The live run currently in progress covers sandbox operations/survival, cloud onboarding, and Slack pairing, but it does not cover the advisor-required |
|
Maintainer disposition for the exact-head
This establishes a current-main hosted-runner performance-budget variance, not a functional regression in this marker-cleanup PR. I accept the two timing-only non-success results for #5569 and will not spend another runner retry on the unchanged head. The flaky budget is tracked as #6660; all functional live evidence remains required and is green. |
E2E Target Results — ❌ Some jobs failedRun: 29126042003
|
|
@cv, please re-review current exact head Current evidence is settled: ordinary CI is fully green; target The repository's current manual-advisor workflow cannot publish its bot sticky for fork PRs, so the strict gate still needs the prepared workflow/tooling repair before final approval. Your stale change request is the remaining source-level human review to clear. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
E2E Target Results — ✅ All selected jobs passedRun: 29127303354
|
E2E Target Results — ✅ All selected jobs passedRun: 29127769177
|
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
E2E Target Results — ❌ Some jobs failedRun: 29127308388
|
E2E Target Results —
|
| Job | Result |
|---|---|
| full-e2e |
E2E Target Results — ❌ Some jobs failedRun: 29127768099
|
E2E Target Results —
|
| Job | Result |
|---|---|
| live |
E2E Target Results —
|
| Job | Result |
|---|---|
| cloud-onboard | |
| full-e2e | |
| gateway-health-honest | |
| issue-2478-crash-loop-recovery | |
| sandbox-operations | |
| sandbox-survival |
E2E Target Results — ✅ All selected jobs passedRun: 29128490199
|
E2E Target Results — ✅ All requested jobs passedRun: 29128496025
|
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - #6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - #6584 / #6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - #6210 / #6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - #6624 / #6623 / #6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - #6629 / #6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - #6634 / #6626 / #6596 / #5569 / #6610 / #6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - #6578 / #5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…VIDIA#5569) ## Summary Fixes NVIDIA#5568. On OpenShell docker-driver sandboxes, `scripts/nemoclaw-start.sh` is not PID 1 — OpenShell's `sleep infinity` keeps the container alive as a sibling. The Docker HEALTHCHECK's pidfile fallback (NVIDIA#4952) trusts `/tmp/nemoclaw-gateway.pid`, which is refreshed only by `record_gateway_pid` inside this script's launch and respawn paths. When the supervise loop exits, the container lives on but nothing refreshes the pidfile. The in-container gateway marker stays in place, so the healthcheck keeps trusting a now-stale PID forever and reports a working sandbox as permanently `unhealthy`. ## Change - Add `clear_in_container_gateway_marker`, symmetric to `mark_in_container_gateway`. - Arm it via `trap ... EXIT` right after the existing `SIGTERM`/`SIGINT` trap, in both the non-root and root supervise paths. The EXIT trap covers every way the supervisor leaves: a clean gateway exit, a forwarded signal (`cleanup_on_signal` ends in `exit`), and errexit. This keeps the fix inside `nemoclaw-start.sh` and out of the shared `cleanup_on_signal` in `sandbox-init.sh`, which `agents/hermes/start.sh` also sources. The marker is re-dropped at each (re)launch by `mark_in_container_gateway`, so the respawn loop — which never exits the script — keeps it in place; only a supervisor *exit* clears it. The marker semantics become: "a supervisor is actively managing the gateway and keeping the pidfile fresh." Once the marker is gone, the healthcheck takes the marker-absent → healthy branch (NVIDIA#4503) instead of trusting a stale PID. ## Tests Extends `test/nemoclaw-start-gateway-marker.test.ts`, reusing its existing `extractShellFunctionFromSource` / `safeTmpHelpers` harness so each case runs the real shell functions in isolation against a temp marker path: - `clear_in_container_gateway_marker` removes the marker and is a no-op when it is absent. - The marker is dropped when the supervisor reaches a clean exit (real `trap ... EXIT` wiring, not a direct helper call). - The marker is dropped when the supervisor is terminated by a signal, exercising the `cleanup_on_signal` → `exit` → EXIT-trap path. - The marker survives respawns while the supervisor keeps running, and is cleared only once the script finally exits. `shfmt`, `shellcheck`, Biome, and `typecheck:cli` pass; the new and existing marker tests pass (14/14). Happy to revisit the choice of an EXIT trap over explicit cleanup at each exit site if maintainers prefer a different shape. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved gateway supervisor shutdown to reliably remove the in-container gateway marker on normal exit, SIGTERM/SIGINT termination, early launch failures, and `errexit`/`EXIT` flows (including non-PID 1 scenarios). * Reduces risk of stale marker files misleading container health checks. * **Tests** * Added shell-based behavioral coverage for marker cleanup on both launch paths, SIGTERM teardown, and marker persistence across respawn iterations until final exit. * **Documentation** * Clarified the marker lifecycle in the deployment/architecture reference. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com>
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - NVIDIA#6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - NVIDIA#6584 / NVIDIA#6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - NVIDIA#6210 / NVIDIA#6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - NVIDIA#6624 / NVIDIA#6623 / NVIDIA#6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - NVIDIA#6629 / NVIDIA#6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - NVIDIA#6634 / NVIDIA#6626 / NVIDIA#6596 / NVIDIA#5569 / NVIDIA#6610 / NVIDIA#6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - NVIDIA#6578 / NVIDIA#5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6 participants
Summary
Fixes #5568.
On OpenShell docker-driver sandboxes,
scripts/nemoclaw-start.shis not PID 1 — OpenShell'ssleep infinitykeeps the container alive as a sibling. The Docker HEALTHCHECK's pidfile fallback (#4952) trusts/tmp/nemoclaw-gateway.pid, which is refreshed only byrecord_gateway_pidinside this script's launch and respawn paths. When the supervise loop exits, the container lives on but nothing refreshes the pidfile. The in-container gateway marker stays in place, so the healthcheck keeps trusting a now-stale PID forever and reports a working sandbox as permanentlyunhealthy.Change
clear_in_container_gateway_marker, symmetric tomark_in_container_gateway.trap ... EXITright after the existingSIGTERM/SIGINTtrap, in both the non-root and root supervise paths.The EXIT trap covers every way the supervisor leaves: a clean gateway exit, a forwarded signal (
cleanup_on_signalends inexit), and errexit. This keeps the fix insidenemoclaw-start.shand out of the sharedcleanup_on_signalinsandbox-init.sh, whichagents/hermes/start.shalso sources. The marker is re-dropped at each (re)launch bymark_in_container_gateway, so the respawn loop — which never exits the script — keeps it in place; only a supervisor exit clears it.The marker semantics become: "a supervisor is actively managing the gateway and keeping the pidfile fresh." Once the marker is gone, the healthcheck takes the marker-absent → healthy branch (#4503) instead of trusting a stale PID.
Tests
Extends
test/nemoclaw-start-gateway-marker.test.ts, reusing its existingextractShellFunctionFromSource/safeTmpHelpersharness so each case runs the real shell functions in isolation against a temp marker path:clear_in_container_gateway_markerremoves the marker and is a no-op when it is absent.trap ... EXITwiring, not a direct helper call).cleanup_on_signal→exit→ EXIT-trap path.shfmt,shellcheck, Biome, andtypecheck:clipass; the new and existing marker tests pass (14/14). Happy to revisit the choice of an EXIT trap over explicit cleanup at each exit site if maintainers prefer a different shape.Summary by CodeRabbit
errexit/EXITflows (including non-PID 1 scenarios).Signed-off-by: Prekshi Vyas prekshiv@nvidia.com