fix(shields): recover stale transition lock owners

[HwangJohn]

Summary

shields up and shields down now recover transition-lock records whose owner is definitively stale because the recorded process is dead or the PID has been reused. Ambiguous owners, malformed records, live owners, identity-unavailable owners, and replacement races continue to fail closed with manual recovery guidance.

Related Issue

Fixes #6751

Changes

• Reuse the existing inode-checked quarantine removal path for stale dead-owner and PID-reused transition-lock records during lock acquisition.
• Preserve fail-closed behavior for ambiguous owner state, malformed stale records, unsafe paths, and replacement races.
• Add sync and async transition-lock tests for dead-owner recovery, PID-reuse recovery, and replacement-race preservation.
• Update the TCB boundary documentation to describe definitive stale-owner quarantine recovery.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• 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:
• Tests not applicable — justification:
• 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: maintainer security review requested; no waiver claimed before review.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

• PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable — local Windows note: npm run check:diff reaches repository checks but the existing runner hits Windows-only spawnSync("tsx.cmd") and executable-bit limitations; manual equivalent repository checks, npx commitlint --from origin/main --to HEAD, and npx prek run --from-ref origin/main --to-ref HEAD --stage pre-push passed.
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: npx vitest run --project cli src/lib/shields/transition-lock.test.ts passed, 28 tests.
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only) — local Windows note: npm run docs:check-agent-variants, npm run docs:check-routes, and PowerShell-equivalent Fern check passed with 0 errors; full npm run docs does not complete under Windows PowerShell because the existing script uses POSIX FERN_VERSION=$(...) syntax.
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

Signed-off-by: HwangJohn angelic805@gmail.com

Summary by CodeRabbit

• Bug Fixes

  • Improved stale-owner recovery for definitively dead and PID-reused cases, with clearer wait/recovery messaging and stricter quarantine validation.
  • Centralized wait-timeout enforcement so the overall limit is applied only after the first retry cycle.
  • Hardened replacement handling during recovery to prevent overwriting preserved replacements and better detect replacement-race failures.
  • Improved cross-platform lock acquisition handling, including permission checks and safer path/symlink behavior.

• Documentation

  • Clarified the trusted-input ownership validation and the explicit stale-owner recovery contract.

• Tests

  • Expanded stale-recovery, sync/async timeout, race, and platform-specific permission/path/symlink coverage.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Transition-lock stale-owner recovery now removes definitively dead or PID-reused owners, preserves replacement races, updates recovery messaging, adjusts platform-specific tests, and documents the expanded trusted-input contract.

Changes

Transition-lock recovery

Layer / File(s)	Summary
Owner expectation and quarantine validation src/lib/shields/transition-lock.ts	Stale-owner removal now accepts generalized owner expectations, compares complete owner records, and validates quarantined replacements against those expectations.
Acquisition recovery flow src/lib/shields/transition-lock.ts	Synchronous and asynchronous acquisition recover observed dead or PID-reused owners and use centralized timeout enforcement with distinct stale-condition messages.
Recovery validation and documented contract src/lib/shields/transition-lock.test.ts, docs/security/tcb-boundary.mdx	Tests cover successful stale recovery, timeout behavior, PID reuse, replacement races, and platform-specific path behavior; security documentation expands the trusted-input contract.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Acquisition
  participant StaleRecovery
  participant LockFile
  Acquisition->>StaleRecovery: Recover observed dead or pid-reused owner
  StaleRecovery->>LockFile: Remove matching owner atomically
  LockFile-->>StaleRecovery: Report recovery or preserved replacement
  StaleRecovery-->>Acquisition: Continue acquisition or throw

Loading

Suggested reviewers: cv, ericksoa

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: recovering stale transition-lock owners.
Linked Issues check	✅ Passed	The PR implements automatic recovery for dead and PID-reused owners while keeping ambiguous cases fail-closed, matching #6751.
Out of Scope Changes check	✅ Passed	The changes stay focused on stale-lock recovery, related tests, and documentation with no clear unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/lib/shields/transition-lock.ts (1)

657-674: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Recovery-driven continue bypasses waitTimeoutMs enforcement.

waitDuration() (called at the bottom of the loop) is the only place that checks elapsedMs >= state.waitTimeoutMs and throws the timeout error. Both if (!observed) continue; and the new if (this.recoveredObservedStaleOwner(sandboxName, observed)) continue; skip straight back to the top of the loop without ever calling waitDuration. Since recoveredObservedStaleOwner returns true not only when it actually removes an owner, but also for "missing", "owner-mismatch", and "path-changed" (i.e. contention/races that don't guarantee progress), a sustained sequence of such races — e.g. concurrent recoverers competing for the same stale lock — can spin indefinitely without ever hitting the configured waitTimeoutMs, defeating the very "don't hang" goal this PR targets (#6751).

Enforce the timeout unconditionally at the top of each loop iteration (both in acquire and acquireAsync), not only in the sleep path.

⏱️ Suggested fix

+  private enforceWaitTimeout(state: AcquisitionState, reason: WaitReason | null): void {
+    const elapsedMs = Math.max(0, this.now() - state.startedAtMs);
+    if (elapsedMs >= state.waitTimeoutMs) {
+      throw new Error(
+        `Timed out after ${String(state.waitTimeoutMs)}ms waiting for shields transition lock '${state.lockPath}': ${formatWaitReason(reason, state.lockPath)}`,
+      );
+    }
+  }
+
   private waitDuration(state: AcquisitionState, reason: WaitReason | null): number {
-    const elapsedMs = Math.max(0, this.now() - state.startedAtMs);
-    if (elapsedMs >= state.waitTimeoutMs) {
-      throw new Error(
-        `Timed out after ${String(state.waitTimeoutMs)}ms waiting for shields transition lock '${state.lockPath}': ${formatWaitReason(reason, state.lockPath)}`,
-      );
-    }
+    this.enforceWaitTimeout(state, reason);
+    const elapsedMs = Math.max(0, this.now() - state.startedAtMs);
     return Math.min(state.pollIntervalMs, state.waitTimeoutMs - elapsedMs);
   }

     while (true) {
+      this.enforceWaitTimeout(state, lastWaitReason);
       const inProcess = this.held.get(sandboxName);

(apply the same one-line addition at the top of both the acquire and acquireAsync loops)

Also applies to: 677-703, 751-772

🤖 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 `@src/lib/shields/transition-lock.ts` around lines 657 - 674, Add an
unconditional elapsed-time timeout check at the start of every iteration in both
the acquire and acquireAsync loops, before held-lock checks, creation attempts,
observation, or recovery continues. Reuse the existing waitTimeoutMs enforcement
used by waitDuration so all retry paths, including !observed and
recoveredObservedStaleOwner, cannot bypass the configured timeout.

🤖 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 `@src/lib/shields/transition-lock.ts`:
- Around line 657-674: Add an unconditional elapsed-time timeout check at the
start of every iteration in both the acquire and acquireAsync loops, before
held-lock checks, creation attempts, observation, or recovery continues. Reuse
the existing waitTimeoutMs enforcement used by waitDuration so all retry paths,
including !observed and recoveredObservedStaleOwner, cannot bypass the
configured timeout.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f26711a9-dd2b-44b9-9d18-6913df5795f3

📥 Commits

Reviewing files that changed from the base of the PR and between 3de1de6 and 6ef5d45.

📒 Files selected for processing (3)

• docs/security/tcb-boundary.mdx
• src/lib/shields/transition-lock.test.ts
• src/lib/shields/transition-lock.ts

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/lib/shields/transition-lock.ts (1)

657-675: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Recovery continue paths bypass the wait timeout.

In both acquire and acquireAsync, waitDuration(state, lastWaitReason) — the only place state.waitTimeoutMs is enforced — is only reached via the trailing this.sleep(...)/await this.sleepAsync(...) call. Both if (!observed) continue; and the new if (this.recoveredObservedStaleOwner(sandboxName, observed)) continue; skip straight back to the top of the loop without ever checking elapsed time.

recoveredObservedStaleOwner returns true not only when it actually removed the stale owner, but also for "missing", "owner-mismatch", and "path-changed" (Lines 766-770) — i.e. whenever another process raced the recovery. Under sustained contention on the same stale lock (multiple waiters recovering concurrently), this can repeatedly hit those "immediate retry" branches, so the loop can run past waitTimeoutMs without ever throwing the timeout error. That reintroduces (in a worse, unbounded form) the exact "wait indefinitely" problem issue #6751 asks to fix.

🛠️ Proposed fix: enforce the timeout independent of the sleep path

+  private enforceWaitTimeout(state: AcquisitionState, reason: WaitReason | null): void {
+    const elapsedMs = Math.max(0, this.now() - state.startedAtMs);
+    if (elapsedMs >= state.waitTimeoutMs) {
+      throw new Error(
+        `Timed out after ${String(state.waitTimeoutMs)}ms waiting for shields transition lock '${state.lockPath}': ${formatWaitReason(reason, state.lockPath)}`,
+      );
+    }
+  }
+
   private waitDuration(state: AcquisitionState, reason: WaitReason | null): number {
-    const elapsedMs = Math.max(0, this.now() - state.startedAtMs);
-    if (elapsedMs >= state.waitTimeoutMs) {
-      throw new Error(
-        `Timed out after ${String(state.waitTimeoutMs)}ms waiting for shields transition lock '${state.lockPath}': ${formatWaitReason(reason, state.lockPath)}`,
-      );
-    }
+    this.enforceWaitTimeout(state, reason);
+    const elapsedMs = Math.max(0, this.now() - state.startedAtMs);
     return Math.min(state.pollIntervalMs, state.waitTimeoutMs - elapsedMs);
   }

Then call this.enforceWaitTimeout(state, lastWaitReason); at the top of the while (true) loop in both acquire and acquireAsync, before the inProcess/tryCreate branch.

Also applies to: 685-703, 751-772

🤖 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 `@src/lib/shields/transition-lock.ts` around lines 657 - 675, Enforce the wait
timeout independently of sleeping by calling enforceWaitTimeout(state,
lastWaitReason) at the start of each while (true) loop in both acquire and
acquireAsync, before checking held ownership or attempting creation. Preserve
the existing retry behavior for missing observations and recovered stale owners
while ensuring every iteration can terminate once waitTimeoutMs is exceeded.

🧹 Nitpick comments (1)

src/lib/shields/transition-lock.test.ts (1)

380-408: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock-call assertions lock the test to internal quarantine naming.

Lines 403-405 assert liveness/rename/unlink mock call details (including the .takeover-stale- naming substring), while lines 406-407 already prove the same outcome through observable state (existsSync, readdirSync). The mock-call assertions are redundant and couple the test to an internal implementation detail (the quarantine filename pattern) rather than the public behavior contract.

♻️ Suggested trim

-    expect(liveness).toHaveBeenCalledWith(202);
-    expect(rename).toHaveBeenCalledWith(lockPath, expect.stringContaining(".takeover-stale-"));
-    expect(unlink).not.toHaveBeenCalledWith(lockPath);
     expect(fs.existsSync(lockPath)).toBe(false);
     expect(fs.readdirSync(stateDir)).toEqual([]);

As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."

🤖 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 `@src/lib/shields/transition-lock.test.ts` around lines 380 - 408, Trim the
redundant mock-call assertions in the stale-lock recovery test around
withShieldsTransitionLock, removing expectations on liveness, rename, unlink,
and the quarantine filename pattern. Keep the observable assertions that the
replacement acquires the lock and the stale lock state directory is empty.

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 `@src/lib/shields/transition-lock.ts`:
- Around line 657-675: Enforce the wait timeout independently of sleeping by
calling enforceWaitTimeout(state, lastWaitReason) at the start of each while
(true) loop in both acquire and acquireAsync, before checking held ownership or
attempting creation. Preserve the existing retry behavior for missing
observations and recovered stale owners while ensuring every iteration can
terminate once waitTimeoutMs is exceeded.

---

Nitpick comments:
In `@src/lib/shields/transition-lock.test.ts`:
- Around line 380-408: Trim the redundant mock-call assertions in the stale-lock
recovery test around withShieldsTransitionLock, removing expectations on
liveness, rename, unlink, and the quarantine filename pattern. Keep the
observable assertions that the replacement acquires the lock and the stale lock
state directory is empty.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 21a18d53-9605-4b42-ac7e-68870507bdd8

📥 Commits

Reviewing files that changed from the base of the PR and between 6ef5d45 and dc218f6.

📒 Files selected for processing (3)

• docs/security/tcb-boundary.mdx
• src/lib/shields/transition-lock.test.ts
• src/lib/shields/transition-lock.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• docs/security/tcb-boundary.mdx

[github-actions]

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: No actionable findings remain in the canonical review ledger.

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 1 blocker · 2 warnings · 0 suggestions
• Model comparison: normalized findings differ; normalized E2E selections differ; Nemotron reported 1 more blocker, 2 more warnings, the same number of suggestions.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: credential-sanitization, security-posture

1 optional E2E recommendation

• shields-config

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[cv]

Blocking correctness issues remain at commit 22fa713:

1. npm run build:cli fails in src/lib/shields/transition-lock.ts:721: closeSnapshot(guard.owner) receives a HeldLock without the required mtimeMs.
2. A crash after creating .recovering while the stale canonical lock remains wedges future acquisition: the canonical lock is inspected first, the existing recovery guard is never reclaimed, and both paths persist until timeout.
3. removeObservedStaleRecoveryGuard() has a check-then-remove race that can delete a live replacement guard installed between identity validation and pathname cleanup, removing mutual exclusion while another recoverer is active.
4. An orphaned owner.json.acquire-*.tmp leaves the guard non-empty and permanently unreclaimable.

Please redesign guard ownership/cleanup to make replacement identity atomic and fail closed, then add regressions for crashed recovery, replacement races, and orphaned temp entries. The current focused tests do not cover these failures.

[cv]

Approved after the failure-atomic recovery-guard redesign at commit 60266c6. The compile failure, crashed-guard wedge, replacement deletion race, and orphan temp blockage are fixed with focused regressions. Local build, 73 focused tests, 697 changed tests, hooks, pre-push, independent security review, documentation review, and current ordinary CI all pass; CodeRabbit and advisors are clear.