From d1729d97494773231abae6975d1fbe83e071bf00 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Mon, 3 Aug 2026 10:21:25 +0000 Subject: [PATCH 1/2] fix(shields): refuse a corrupt transition lock without a raw stack trace An unreadable owner record can never be recovered by waiting, but shields commands polled it for the full wait timeout and then let a plain Error reach the user. Refuse as soon as the record is observed and report it through the shields exit sentinel, so up, down, and read-only status all fail closed with one line plus the recovery hint. Signed-off-by: Tinson Lai --- src/lib/shields/index.ts | 6 ++ src/lib/shields/transition-lock.test.ts | 61 ++++++++++++++++ src/lib/shields/transition-lock.ts | 91 +++++++++++++++++++++--- test/cli/shields-transition-lock.test.ts | 69 ++++++++++++++++++ 4 files changed, 216 insertions(+), 11 deletions(-) create mode 100644 test/cli/shields-transition-lock.test.ts diff --git a/src/lib/shields/index.ts b/src/lib/shields/index.ts index cd3110b8d3f..8e69dbbf58b 100644 --- a/src/lib/shields/index.ts +++ b/src/lib/shields/index.ts @@ -65,6 +65,7 @@ const { verifyShieldsLockState }: typeof import("./verify-lock") = require("./ve const { relockAndReconfirm }: typeof import("./relock-reconfirm") = require("./relock-reconfirm"); const { inspectShieldsTransitionLockOwner, + isShieldsTransitionLockUnavailable, takeoverShieldsTransitionLock, withShieldsTransitionLock, }: typeof import("./transition-lock") = require("./transition-lock"); @@ -785,6 +786,11 @@ function failShieldsCommand(message: string, _shouldThrow?: boolean): never { } function completeDeferredShieldsExit(error: unknown, shouldThrow = false): never { + if (isShieldsTransitionLockUnavailable(error)) { + console.error(` ${error.summary}`); + if (error.recovery) console.error(` Recovery: ${error.recovery}`); + return failShieldsCommand(error.summary, shouldThrow); + } if (error instanceof DeferredShieldsExit && !shouldThrow) { process.exit(error.exitCode); } diff --git a/src/lib/shields/transition-lock.test.ts b/src/lib/shields/transition-lock.test.ts index 5d966815b98..e41f62a2c19 100644 --- a/src/lib/shields/transition-lock.test.ts +++ b/src/lib/shields/transition-lock.test.ts @@ -8,8 +8,10 @@ import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { + isShieldsTransitionLockUnavailable, ShieldsTransitionLockManager, type ShieldsTransitionLockOwner, + type ShieldsTransitionLockUnavailableError, shieldsTransitionLockPath, } from "./transition-lock"; @@ -1031,6 +1033,65 @@ describe("host shields transition lock", () => { expect(fs.readFileSync(lockPath, "utf8")).toBe("{incomplete"); }); + it("refuses an old malformed owner record without waiting out the timeout (#8108)", () => { + const lockPath = writeOwner("alpha", "{incomplete"); + fs.utimesSync(lockPath, new Date(1_000), new Date(1_000)); + let nowMs = 60_000; + let sleepCalls = 0; + const locker = manager({ + now: () => nowMs, + sleep: (milliseconds) => { + sleepCalls += 1; + nowMs += milliseconds; + }, + }); + + let caught: unknown; + try { + locker.withShieldsTransitionLock("alpha", "nemoclaw alpha shields status", () => undefined, { + waitTimeoutMs: 30_000, + pollIntervalMs: 50, + malformedStaleMs: 30_000, + }); + } catch (error) { + caught = error; + } + + expect(sleepCalls).toBe(0); + expect(nowMs).toBe(60_000); + expect(isShieldsTransitionLockUnavailable(caught)).toBe(true); + const failure = caught as ShieldsTransitionLockUnavailableError; + expect(failure.summary).toContain("Cannot acquire shields transition lock"); + expect(failure.summary).toContain("the owner record is incomplete"); + expect(failure.summary).not.toContain("will not remove"); + expect(failure.recovery).toContain(`remove '${lockPath}' manually`); + expect(failure.lockPath).toBe(lockPath); + expect(fs.readFileSync(lockPath, "utf8")).toBe("{incomplete"); + }); + + it("refuses an old malformed owner record on the async path (#8108)", async () => { + const lockPath = writeOwner("alpha", "{incomplete"); + fs.utimesSync(lockPath, new Date(1_000), new Date(1_000)); + let sleepCalls = 0; + const locker = manager({ + now: () => 60_000, + sleepAsync: async () => { + sleepCalls += 1; + }, + }); + + await expect( + locker.withShieldsTransitionLockAsync( + "alpha", + "nemoclaw alpha shields status", + async () => undefined, + { waitTimeoutMs: 30_000, pollIntervalMs: 50, malformedStaleMs: 30_000 }, + ), + ).rejects.toThrow(/Cannot acquire shields transition lock/); + expect(sleepCalls).toBe(0); + expect(fs.readFileSync(lockPath, "utf8")).toBe("{incomplete"); + }); + it.skipIf(process.platform === "win32")("rejects symbolic-link lock paths", () => { const target = path.join(root, "target"); fs.writeFileSync(target, "{}", { mode: 0o600 }); diff --git a/src/lib/shields/transition-lock.ts b/src/lib/shields/transition-lock.ts index b31eb7a635c..00109a6123f 100644 --- a/src/lib/shields/transition-lock.ts +++ b/src/lib/shields/transition-lock.ts @@ -318,28 +318,82 @@ function staleOwnerRecovery(lockPath: string): string { return `NemoClaw could not safely recover the stale lock automatically. ${manualRecovery(lockPath)}`; } -function formatWaitReason(reason: WaitReason | null, lockPath: string): string { - if (!reason) return "the lock changed during inspection; retry the command"; +interface WaitReasonDescription { + reason: string; + recovery: string; +} + +function describeWaitReason(reason: WaitReason | null, lockPath: string): WaitReasonDescription { + if (!reason) { + return { reason: "the lock changed during inspection; retry the command", recovery: "" }; + } if (reason.kind === "recent-malformed") { - return `the owner record is incomplete and only ${Math.max(0, Math.floor(reason.ageMs))}ms old. Retry after the writer finishes`; + return { + reason: `the owner record is incomplete and only ${Math.max(0, Math.floor(reason.ageMs))}ms old.`, + recovery: "Retry after the writer finishes", + }; } if (reason.kind === "stale-malformed") { - return `the owner record is incomplete and ${Math.max(0, Math.floor(reason.ageMs))}ms old. ${malformedStaleRecovery(lockPath)}`; + return { + reason: `the owner record is incomplete and ${Math.max(0, Math.floor(reason.ageMs))}ms old.`, + recovery: malformedStaleRecovery(lockPath), + }; } const owner = reason.owner; if (reason.kind === "dead") { - return `recorded owner PID ${String(owner.pid)} is not running (${owner.command}). ${staleOwnerRecovery(lockPath)}`; + return { + reason: `recorded owner PID ${String(owner.pid)} is not running (${owner.command}).`, + recovery: staleOwnerRecovery(lockPath), + }; } if (reason.kind === "pid-reused") { - return `recorded owner PID ${String(owner.pid)} now has process-start identity '${reason.currentProcessStartIdentity}' instead of '${owner.processStartIdentity}' (${owner.command}). ${staleOwnerRecovery(lockPath)}`; + return { + reason: `recorded owner PID ${String(owner.pid)} now has process-start identity '${reason.currentProcessStartIdentity}' instead of '${owner.processStartIdentity}' (${owner.command}).`, + recovery: staleOwnerRecovery(lockPath), + }; } if (reason.kind === "identity-unavailable") { - return `PID ${String(owner.pid)} is alive but its process-start identity cannot be verified (${owner.command}). Verify the active process and retry`; + return { + reason: `PID ${String(owner.pid)} is alive but its process-start identity cannot be verified (${owner.command}).`, + recovery: "Verify the active process and retry", + }; } if (reason.kind === "same-process") { - return `another async chain in this process still owns the lock (${owner.command}). Wait for that operation to finish and retry`; + return { + reason: `another async chain in this process still owns the lock (${owner.command}).`, + recovery: "Wait for that operation to finish and retry", + }; } - return `PID ${String(owner.pid)} is still running (${owner.command}). Wait for that operation to finish and retry`; + return { + reason: `PID ${String(owner.pid)} is still running (${owner.command}).`, + recovery: "Wait for that operation to finish and retry", + }; +} + +export class ShieldsTransitionLockUnavailableError extends Error { + readonly lockPath: string; + readonly summary: string; + readonly recovery: string; + + constructor(summary: string, recovery: string, lockPath: string) { + super(recovery ? `${summary} ${recovery}` : summary); + this.name = "ShieldsTransitionLockUnavailableError"; + this.lockPath = lockPath; + this.summary = summary; + this.recovery = recovery; + } +} + +export function isShieldsTransitionLockUnavailable( + error: unknown, +): error is ShieldsTransitionLockUnavailableError { + if (!error || typeof error !== "object") return false; + const candidate = error as { name?: unknown; summary?: unknown; recovery?: unknown }; + return ( + candidate.name === "ShieldsTransitionLockUnavailableError" && + typeof candidate.summary === "string" && + typeof candidate.recovery === "string" + ); } export function shieldsTransitionLockPath( @@ -817,6 +871,7 @@ export class ShieldsTransitionLockManager { if (!observed) continue; lastWaitReason = observed; if (this.recoveredObservedStaleOwner(sandboxName, observed)) continue; + this.failFastOnUnrecoverableOwner(state, observed); } this.sleep(this.waitDuration(state, lastWaitReason)); } @@ -848,6 +903,7 @@ export class ShieldsTransitionLockManager { if (!observed) continue; lastWaitReason = observed; if (this.recoveredObservedStaleOwner(sandboxName, observed)) continue; + this.failFastOnUnrecoverableOwner(state, observed); } await this.sleepAsync(this.waitDuration(state, lastWaitReason)); } @@ -954,12 +1010,25 @@ export class ShieldsTransitionLockManager { 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)}`, + const described = describeWaitReason(reason, state.lockPath); + throw new ShieldsTransitionLockUnavailableError( + `Timed out after ${String(state.waitTimeoutMs)}ms waiting for shields transition lock '${state.lockPath}': ${described.reason}`, + described.recovery, + state.lockPath, ); } } + private failFastOnUnrecoverableOwner(state: AcquisitionState, reason: WaitReason): void { + if (reason.kind !== "stale-malformed") return; + const described = describeWaitReason(reason, state.lockPath); + throw new ShieldsTransitionLockUnavailableError( + `Cannot acquire shields transition lock '${state.lockPath}': ${described.reason}`, + described.recovery, + state.lockPath, + ); + } + private enforceWaitTimeoutBeforeRetry( state: AcquisitionState, reason: WaitReason | null, diff --git a/test/cli/shields-transition-lock.test.ts b/test/cli/shields-transition-lock.test.ts new file mode 100644 index 00000000000..bc58c0a2ccb --- /dev/null +++ b/test/cli/shields-transition-lock.test.ts @@ -0,0 +1,69 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import fs from "node:fs"; +import path from "node:path"; + +import { describe, expect, test as it } from "../helpers/owned-test-resources"; + +import { runWithEnv, testTimeoutOptions, writeSandboxRegistry } from "./helpers"; + +const REFUSAL_BUDGET_MS = 15_000; + +function writeCorruptTransitionLock(home: string, sandboxName: string): string { + const stateDir = path.join(home, ".nemoclaw", "state"); + fs.mkdirSync(stateDir, { recursive: true }); + const lockPath = path.join(stateDir, `shields-transition-lock-${sandboxName}.json`); + fs.writeFileSync(lockPath, "", { mode: 0o600 }); + fs.utimesSync(lockPath, new Date(1_000), new Date(1_000)); + return lockPath; +} + +function expectCleanRefusal(out: string): void { + expect(out).toContain("the owner record is incomplete"); + expect(out).toContain("Recovery:"); + expect(out).toContain("manually"); + expect(out).not.toContain("ShieldsTransitionLockManager"); + expect(out).not.toMatch(/^\s+at /m); + expect(out).not.toContain("Node.js v"); +} + +describe("shields commands with a corrupt transition lock", () => { + it( + "refuses read-only status without a raw stack trace (#8108)", + testTimeoutOptions(30_000), + ({ testHome }) => { + const { home } = testHome; + writeSandboxRegistry(home); + const lockPath = writeCorruptTransitionLock(home, "alpha"); + + const startedAt = Date.now(); + const status = runWithEnv("alpha shields status 2>&1", testHome.environment()); + const elapsedMs = Date.now() - startedAt; + + expect(status.code).toBe(1); + expectCleanRefusal(status.out); + expect(elapsedMs).toBeLessThan(REFUSAL_BUDGET_MS); + expect(fs.readFileSync(lockPath, "utf8")).toBe(""); + }, + ); + + it( + "refuses shields down without a raw stack trace (#8108)", + testTimeoutOptions(30_000), + ({ testHome }) => { + const { home } = testHome; + writeSandboxRegistry(home); + const lockPath = writeCorruptTransitionLock(home, "alpha"); + + const startedAt = Date.now(); + const down = runWithEnv("alpha shields down --reason test 2>&1", testHome.environment()); + const elapsedMs = Date.now() - startedAt; + + expect(down.code).toBe(1); + expectCleanRefusal(down.out); + expect(elapsedMs).toBeLessThan(REFUSAL_BUDGET_MS); + expect(fs.readFileSync(lockPath, "utf8")).toBe(""); + }, + ); +}); From 6ef49bbaa96fa206b96a66a01087ec06e724b3e1 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Mon, 3 Aug 2026 04:37:17 -0700 Subject: [PATCH 2/2] test(shields): cover corrupt lock refusal for shields up Signed-off-by: Carlos Villela --- test/cli/shields-transition-lock.test.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/cli/shields-transition-lock.test.ts b/test/cli/shields-transition-lock.test.ts index bc58c0a2ccb..8e3afe26e11 100644 --- a/test/cli/shields-transition-lock.test.ts +++ b/test/cli/shields-transition-lock.test.ts @@ -48,6 +48,25 @@ describe("shields commands with a corrupt transition lock", () => { }, ); + it( + "refuses shields up without a raw stack trace (#8108)", + testTimeoutOptions(30_000), + ({ testHome }) => { + const { home } = testHome; + writeSandboxRegistry(home); + const lockPath = writeCorruptTransitionLock(home, "alpha"); + + const startedAt = Date.now(); + const up = runWithEnv("alpha shields up 2>&1", testHome.environment()); + const elapsedMs = Date.now() - startedAt; + + expect(up.code).toBe(1); + expectCleanRefusal(up.out); + expect(elapsedMs).toBeLessThan(REFUSAL_BUDGET_MS); + expect(fs.readFileSync(lockPath, "utf8")).toBe(""); + }, + ); + it( "refuses shields down without a raw stack trace (#8108)", testTimeoutOptions(30_000),