fix(shields): retire timer authority after lifecycle gates - #9866
Conversation
|
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. |
|
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)
Included review availability: Your plan provides up to 12 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughAuto-restore now defers timer-marker cleanup until restoration completes and both lifecycle lock generations are released. The lifecycle deadline fence supports an ChangesTimer authority cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change retires successful timer authority only after lifecycle gates are released, with targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Timer
participant DeadlineFence
participant LifecycleGates
participant TimerMarker
Timer->>DeadlineFence: complete auto-restore
DeadlineFence->>LifecycleGates: release main lock and deadline gate
DeadlineFence->>Timer: invoke onReleased
Timer->>TimerMarker: remove owned marker
Suggested reviewers: 🚥 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 0bcf321 in the TypeScript / code-coverage/cliThe overall line coverage in commit 0bcf321 in the Show a line coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Maintainer review note: focused lifecycle/security review passes at |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Successful timer cleanup now occurs only after both lifecycle gate generations are released. Exact marker authority is preserved through quarantine and identity checks; release or cleanup failures enter existing retry or containment handling; durable-containment paths retain both gates and marker authority. Focused tests cover release ordering, marker cleanup, retries, and retained containment. Required checks pass and the PR is ready for review.
senthilr-nv
added
bug-fix
## Outcome A completed Shields auto-restore no longer leaves its sandbox permanently blocked when the detached timer exits after restoring persisted state but before releasing its lifecycle gates. The next Shields-aware command retires only the proven orphaned timer generation. Ordinary commands, interactive `sandbox connect`, and two-sandbox snapshot restore then proceed normally; live, foreign, replaced, or otherwise ambiguous process identity remains denied. ## Reason The timeout path in #10094 can persist `shieldsDown: false` and finish the restore, then lose the timer process before final cleanup releases the main/deadline gates and marker. Ordinary acquisition subsequently commits process-tree containment for the dead owner, but the prior fail-closed code had no proof-backed path to distinguish this completed generation from unresolved containment. ### Related issues Fixes #10094 Relates to #9750 Relates to #9866 ## Changes - Recognize recovery authority only when Shields state is already UP, the exact 32-hex timer marker is expired and abandoned, and no transition for that token remains. - Recover only stale local main/deadline owners bound to the marker PID, sandbox, host identity, PID namespace, process identity, protected filesystem generation, and takeover token. Reapers and active, foreign, replaced, corrupt, or unrelated generations remain denied. - Require exact structured containment evidence and retire containment, main, then deadline gates so every partial cleanup failure leaves at least one admission gate in place. Marker cleanup uses generation-pinned rename/revalidation, directory durability checks, and restore-or-retain behavior for retry. - Reject command admission unless every completed timer artifact is verifiably removed. A replacement injected after the authority check remains preserved and blocks the command. - Run the shared recovery before normal lifecycle locking, before interactive connect starts, and in stable source/destination order before snapshot restore locks either sandbox. - Add process-backed unit/integration regressions for orphan reproduction, successful recovery, idempotent retry, unrelated acquisition, interactive connect, snapshot endpoints, marker replacement, live-owner denial, cleanup failures, and terminal-containment preservation. - Document the bounded automatic path and exact fail-closed operator procedure, including the separate retention rules for invalid or mismatched artifacts. ## Verification Latest PR commit: `561845d3b5485813a9b4b4d0708efccdac8262e8` Current upstream base/workflow: `d0b120958600e9651001957503c99945248c30d8` - `npm run validate:pr` — passed pre-commit, commitlint, pre-push, repository architecture, source-shape, growth, secret, lint, format, and CLI TypeScript checks on the latest PR commit. - Focused merged-base lifecycle/security Vitest — 165 tests passed across regular-file reads, snapshot recovery, completed auto-restore command admission, launch routing, timer control, timer lifecycle, OpenClaw transition, and lifecycle identity/authority. - `npm run docs` — passed with 0 errors and 2 pre-existing unrelated Fern warnings. - [CI / Pull Request](https://github.com/NVIDIA/NemoClaw/actions/runs/33299035713) — passed on the latest PR commit and current base. - [PR Review Advisor](https://github.com/NVIDIA/NemoClaw/actions/runs/33299034668) — all nine specialists passed; Behavior, Trust, Design, Operations, Documentation, Test Design, Migration, Dependency, and Code Reduction reports found no issue. - [E2E / Pull Request](https://github.com/NVIDIA/NemoClaw/actions/runs/33299036504) — the selected sandbox, gateway-isolation, port-override, and non-root smoke jobs passed; the generic-GPU lane was policy-skipped by selection. - [CodeQL](https://github.com/NVIDIA/NemoClaw/actions/runs/33299034216) and [Security Code Scanning](https://github.com/NVIDIA/NemoClaw/actions/runs/33299035677) — passed. - CodeRabbit completed its latest-commit review with no new inline finding; its check is green. The earlier replacement-authority security finding is fixed by rejecting every timer-retirement result other than verified removal. - [Trusted focused Shields E2E](https://github.com/NVIDIA/NemoClaw/actions/runs/33290527333) — passed. [Selected job](https://github.com/NVIDIA/NemoClaw/actions/runs/33290527333/job/99201448158). Receipt kind `nemoclaw-e2e-dispatch-v2`; correlation `bc7af474-d7d9-4918-99c5-694c6d31593e`; target `shields-config`; candidate/base/workflow SHAs matched that tested revision; risk signal reported 1 passed, 0 failed/skipped/pending/unhandled; all 14 Shields assertions passed; cleanup reported no failures. Later commits tighten proof reads, cleanup failure handling, diagnostic display, and recovery guidance without expanding the E2E surface. The first current-base dispatch, [run 33289861067](https://github.com/NVIDIA/NemoClaw/actions/runs/33289861067), stopped before candidate checkout because its bounded five-minute prerequisite wait expired while current-main managed images were still publishing. After [that publication](https://github.com/NVIDIA/NemoClaw/actions/runs/33289214786) succeeded, the same tested revision/base selection passed in run 33290527333. ## Review notes This changes a security-sensitive fail-closed lifecycle path. Final review covers credentials, input/filesystem validation, exact authorization identity, dependency use, error/retry behavior, cryptographic tokens and hashes, configuration boundaries, negative/race tests, and system/TOCTOU behavior. Recovery requires exact local stale identity and marker/state authority; ambiguous live identity, foreign ownership, reaper presence, changed generations, incomplete durable cleanup, and terminal auto-restore containment remain denied. PR #10571 was explicitly checked and is now merged at `b0e242b8bb29993e2bebfd48f00d649cb103f55f`. It owns retained-sandbox onboarding/destroy cleanup and does not change the Shields timer, MCP lifecycle-lock, or containment files. This PR is the smallest independent completed-auto-restore recovery change and does not duplicate that work. No new billable infrastructure was provisioned. This validation used the existing authorized Shields lane in mock inference mode. DGX Spark, Jetson, and staging dispatch were disabled, so no hardware evidence is claimed. --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com>
3 participants
Summary
Successful shields auto-restore now retires its exact timer authority only after the lifecycle main and deadline gates are released. This prevents a normal timer exit from leaving stale gates that indefinitely block snapshot operations.
Related Issue
Fixes #9750
Changes
onReleasedlifecycle-fence callback because only the fence helper owns both release operations; the lifecycle-lock regression test protects this ordering.Type of Change
Quality Gates
0bcf321a50715d533daa4e280cd6ea03985aa72a.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx --no-install vitest run --project cli src/lib/shields/timer.test.ts src/lib/state/mcp-lifecycle-lock-acquisition.test.ts -t 'retires successful timer authority only after lifecycle gates are released|runs deadline release completion only after exact gates are absent'passed 2/2;npx --no-install vitest run --project integration test/mcp-lifecycle-lock.test.tspassed 44/44 on the host.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Documentation Writer Review
docs-not-neededdocs/manage-sandboxes/runtime-controls.mdx:136-166anddocs/reference/commands.mdx:1668-1688already document lifecycle-gate retention during restoration, failure containment, and recovery. This fix restores that behavior after successful timed Shields auto-restore.Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests